openssl3 integration: use EVP_PKEY_get1_RSA to avoid modifying const RSA key

[toidiu]

This change is based on top of a previous PR and the relevant commit is a7f134e

#3442

Description of changes:

The API and behavior for EVP_PKEY_get0_RSA has change in openssl3. The main difference is that the return value is now marked as const, which triggers warnings across the codebase. The second difference is that the return value is no longer direct reference to the PKEY, but instead a reference to a pre-cached value.

oss1 EVP_PKEY_get0_RSA: direct reference, not-const, should not be freed
oss3 EVP_PKEY_get0_RSA: pre-cached copy, const, should not be freed

Our codebase is making a few assumptions at the moment:

• for a rsa_pss key we use EVP_PKEY_get0_RSA which is not a copy, we DO NOT need to free the RSA key
• s2n_rsa_pss reuses the (s2n_rsa_key) rsa_key->rsa field in pkey
• for a s2n_rsa key we use EVP_PKEY_get1_RSA, which does need to be freed

---

Solution

1. The solution to the above issue is that we should use the same implementation for s2n_rsa and s2n_rsa_pss keys.. switch to using EVP_PKEY_get1_RSA. Since EVP_PKEY_get1_RSA returns a copy, and needs to be freed, we need to free the key in s2n_rsa_pass.c.
2. We need to update all usage of EVP_PKEY_get0_RSA -> EVP_PKEY_get1_RSA in tests and subsequently call free.
3. We need to call EVP_PKEY_set1_RSA to set an PKEY since in ossl3 a pre-cached value is returned rather than the direct key

Testing:

• openssl 1.0.2 (unit tests pass)
• openssl 3 (unit tests and asan tests passes locally)
  Running /home/ubuntu/projects/rsync/s2n-tls/tests/unit/s2n_rsa_pss_rsae_test.c ... PASSED 213 tests

Code analysis:

• EVP_PKEY_get0_RSA
  • https://github.com/openssl/openssl/blob/openssl-3.0.5/crypto/evp/p_legacy.c#L43
    • https://github.com/openssl/openssl/blob/openssl-3.0.5/crypto/evp/p_lib.c#L2069
      • returns pk->legacy_cache_pkey.ptr
  • https://github.com/openssl/openssl/blob/OpenSSL_1_1_1/crypto/evp/p_lib.c#L461
    • returns pkey->pkey.rsa
• EVP_PKEY_set1_RSA
  • https://github.com/openssl/openssl/blob/openssl-3.0.5/crypto/evp/p_legacy.c#L25
    • https://github.com/openssl/openssl/blob/openssl-3.0.5/crypto/evp/p_lib.c#L741
      • sets pkey->pkey.ptr = key; which is similar to modifying EVP_PKEY_get0_RSA value in openssl1.1.1

---

Is this a refactor change? If so, how have you proved that the intended behavior hasn't changed?

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

[lrstewart]

Are you sure we don't want RSA* to be const? Now that the behavior has changed and modifying the RSA key isn't reflected back in the EVP_PKEY, isn't this likely to cause bugs? How do you know we're not relying on that behavior?

[toidiu]

This usage is consistent with rsa and ecdsa. That means that we make similar assumptions in all the other implementations also:

The assumptions in ossl1:

• we are getting a raw ref to the key
• the ref count is incremented so we need to call free
• since its a raw ref, we can mutate it

The assumptions in ossl3:

• we are getting a ref to a pre-cached copy of the key
• the ref count of the copy is incremented so we need to call free
• we can can mutate the copy, but the changes are not reflected back to the original key
  • this is ok since we dont modify the key (except free) (based on my investigation of methods allowed on keys)
  • this is ok if we store the copy in s2n_rsa_key(the copy) and dont actually use the original

---

In conclusion, why PR impl is Safe but brittle:

• get1 allows us to get around the api changes introduced with get0
  • this forces us to call free on the key
• ossl3 now returns a pre-cached copy, but that is ok since the key is not mutated
• if we do mutate then we violate the above behavior

---

Alternative and reasons why I didnt go that route:

• use PKEY directly: this is a large refactor change (if we are really uncomfortable with the current behavior then this would be the recommended approach. This can/should also be follow up work since the above is not a 1-way door)
• use get0 and const: this is a large refactor change since s2n_rsa.c and rsa_signing will need to change. its also less prefered to using PKEY directly imo

[lrstewart]

As discussed offline, "if we do mutate then we violate the above behavior" is what I'm concerned about. The new Openssl 3.0 behavior effectively makes the RSA* object const, but that's not reflected anywhere in our code. Nothing will stop a future dev from trying to modify the key, and things will not behave as someone reading this code would expect them to. This seems like a recipe for bugs.

If we're not going to make RSA* const, then we minimally need very scary comments everywhere that they key is referenced.

[toidiu]

specifically here is an impl which stores PKEY and converts to RSA when needed. Its also the recommended ossl approach: b9fd783

[lrstewart]

Nit

Suggested change

	return 0;
	}
	return S2N_SUCCESS;
	}

[toidiu]

closing in favor of #3474

[toidiu]

closing in favor of #3474