openssl3 integration: store const RSA and EC_KEY

[toidiu]

tracking issue: #3442

previous closed PR: #3473 (this stored a non-const RSA key. while this worked, it was brittle)

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
• for a s2n_rsa key we use EVP_PKEY_get1_RSA, which does need to be freed

---

Solution

1. switch to using EVP_PKEY_get1_RSA and store const 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. Update ecdsa code to also store const EC_KEY
3. We need to update all usage of EVP_PKEY_get0_RSA -> EVP_PKEY_get1_RSA in tests and subsequently call free.
4. 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

Callouts:

I also attempted to store the PKEY directly instead of RSA, however ossl1.0.2 doesnt have EVP_PKEY_get0_RSA API, which means excessive calls to EVP_PKEY_get1_RSA and free when an RSA key is needed. The solution in this PR instead calls EVP_PKEY_get1_RSA once and stores the RSA key.

---

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.

[toidiu]

EVP_PKEY_get0_EC_KEY doesnt seem to be available in ossl 1.0.2

As a work around I think we need to use EVP_PKEY_get1_EC_KEY and call free after every usage, which makes this kind of ugly. We can go back to storing EC_KEY for ecdsa only but then rsa and ecdsa impls are different.

Any other suggestions also welcome.

[loqs]

EVP_PKEY_get0_EC_KEY doesnt seem to be available in [ossl 1.0.2]

1.0.2 has EVP_PKEY_get0, can you use that to access the EC_KEY in 1.0.2?

[toidiu]

1.0.2 has EVP_PKEY_get0, can you use that to access the EC_KEY in 1.0.2?

I havnt explored this yet no. Its a pretty old method and so I dont know if we want to start using that. If we needed #ifdef that might also be a dealbreaker since that makes the code more complicated to understand and we have other options.

At the moment I am thinking of:

• using get1 and casting to non-const
• using get1 for ecdsa and get0 for rsa
• storing EC_KEY for ecdsa and EVP_PKEY for rsa
• some combination of the above

[camshaft]

You're missing clang

[toidiu]

thanks for catching thttps://github.com//pull/3474/commits/52c5df3534746ae4e4ae9b48c5e3e84f44508996

[lrstewart]

Nit: spacing

Suggested change

	if(ecdsa_key == NULL) {
	return NULL;
	if (ecdsa_key == NULL) {
	return NULL;

[lrstewart]

If you wanted to, you could make this a PTR_ENSURE_REF (error if ecdsa_key is NULL). Leaving it as-is is fine too.

My comments last time were about checking if ecdsa_key->ec_key was NULL, not checking if ecdsa_key was NULL. We use ecdsa_key in this method, but we don't use ecdsa_key->ec_key.

[toidiu]

PTR_ENSURE_REF +1

[lrstewart]

I just noticed that you're swapping EVP_PKEY_get0_RSA for EVP_PKEY_get1_RSA for RSA-PSS. That does introduce a new (probably small?) allocation. Are you just doing this for consistency? What's the reasoning?

[toidiu]

Consistency is the main reason. I have listed a few differences below and since ossl functions are quite complicated I think its better to use a smaller surface area and behavior (only get1) rather than optimize for a single allocation.

Some differences between get1 and get0:

• get1 is used in rsa and ecdsa and code is more consistent
• ossl3 changed get0 to return const while get1 remains non-const
• get1 requires a call to free while get0 doesnt
• get0 is not available in ossl1.0.2

Future work:
If we are able to deprecate ossl1.0.2, we should replace usage with PKEY and then use get0 where we need the underlying key.

[toidiu]

Regarding allocation: btw get1 doe NOT actually do an extra allocation compared with get0. get1 simply does a ref_count++ and therefore requires a call to free.

https://github.com/openssl/openssl/blob/31b7f23d2f958491d46c8a8e61c2b77b1b546f3e/crypto/evp/p_legacy.c#L53

[lrstewart]

I asked because most of those difference don't apply to RSA-PSS, or are in get0's favor. We want a const, the extra free call is more work, and RSA-PSS isn't available in Openssl-1.0.2 either :)

But consistency seems like an okay reason to switch, especially if there's no allocation cost!