openssl3 integration: update EVP_PKEY_get/set methods #3459

toidiu · 2022-08-19T07:32:06Z

Description of changes:

https://www.openssl.org/docs/manmaster/man3/EVP_PKEY_get0_RSA.html
Previously EVP_PKEY_get0_RSA returned a mutable pkey, but now returns an immutable(const) one. This causes -Werror to emit a warning. It is possible to use EVP_PKEY_get1_RSA, which is not const, but this is the cached version of the key and cannot be modified.

https://www.openssl.org/docs/man3.0/man3/EVP_PKEY_set1_RSA.html

Note that if an EVP_PKEY was not constructed using one of the deprecated functions such as EVP_PKEY_set1_RSA(),then the internal key will be managed by a provider. In that case the key returned by EVP_PKEY_get1_RSA() will be a cached copy of the provider's key.

For the same reason, we were able to call EVP_PKEY_get0_RSA and then modify the key inline. Now since an immutable PKEY is returned, the low level API EVP_PKEY_set1_RSA must be used to set PKEY after it is modified.

Testing:

openssl 1.0.2 (all unit tests pass)
openssl 1.1.1 (all unit tests pass)
openssl 3 (passes)
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.

loqs · 2022-08-19T09:00:41Z

crypto/s2n_rsa_pss.c

@@ -183,7 +183,7 @@ static int s2n_rsa_pss_key_free(struct s2n_pkey *pkey)
 }

 int s2n_evp_pkey_to_rsa_pss_public_key(struct s2n_rsa_key *rsa_key, EVP_PKEY *pkey) {
-    RSA *pub_rsa_key = EVP_PKEY_get0_RSA(pkey);
+    RSA *pub_rsa_key = EVP_PKEY_get1_RSA(pkey);


What frees the extra reference this is adding?
How is this preferable to RSA *priv_rsa_key = (RSA *)EVP_PKEY_get0_RSA(pkey);

I would really hope the compiler would scream about that xD Let's try to avoid casting to remove the const. That's just hiding the problem, and could lead to bugs if we actually try to modify the key.

That's just hiding the problem

yea I dont like casting since that simply hides the issue. more discussion on this below. #3459 (comment)

If we keep EVP_PKEY_get0_RSA then we shouldnt be calling free: #3459 (comment)

https://www.openssl.org/docs/manmaster/man3/EVP_PKEY_get0_RSA.html

... EVP_PKEY_get0_RSA(), ... return the referenced key in pkey or NULL if the key is not of the correct type. The reference count of the returned key is not incremented and so the key must not be freed after use.

crypto/s2n_rsa_pss.c

lrstewart · 2022-08-19T18:59:49Z

crypto/s2n_rsa_pss.c


    S2N_ERROR_IF(s2n_rsa_is_private_key(pub_rsa_key), S2N_ERR_KEY_MISMATCH);


Is the problem here that s2n_rsa_is_private_key doesn't take a const RSA*? Can you just change it so that it does? Same below.

s2n-tls/crypto/s2n_rsa_pss.c

Line 190 in aa1ec64

rsa_key->rsa = pub_rsa_key;

Does not take a const RSA* either. When everything is changed RSA function calls such as below require the const be removed

s2n-tls/crypto/s2n_rsa.c

Line 101 in b55ab81

int r = RSA_public_encrypt(in->size, ( unsigned char * )in->data, ( unsigned char * )out->data, key->rsa,

So why not remove the const from the reference returned by EVP_PKEY_get0_RSA?

Because the const is important information. Removing it is deceptive and could lead to strange bugs.

If we need to call RSA_public_encrypt, we could either use EVP_PKEY_get1_RSA (but with proper cleanup) or do the cast in the call to RSA_public_encrypt, with a comment explaining why that's safe.

EVP_PKEY_get1_RSA should also be treated as const it did not receive const for legacy reasons.
https://www.openssl.org/docs/man3.0/man3/EVP_PKEY_get1_RSA.html Warnings section

The keys returned from the functions EVP_PKEY_get0_RSA(), EVP_PKEY_get0_DSA(), EVP_PKEY_get0_DH() and EVP_PKEY_get0_EC_KEY() were changed to have a "const" return type in OpenSSL 3.0. As described above the keys returned may be cached copies of the key held in a provider. Due to this, and unlike in earlier versions of OpenSSL, they should be considered read-only copies of the key. Updates to these keys will not be reflected back in the provider side key. The EVP_PKEY_get1_RSA(), EVP_PKEY_get1_DSA(), EVP_PKEY_get1_DH() and EVP_PKEY_get1_EC_KEY() functions were not changed to have a "const" return type in order that applications can "free" the return value. However applications should still consider them as read-only copies.

EVP_PKEY_get0_RSA from 3.0.5 https://github.com/openssl/openssl/blob/openssl-3.0.5/crypto/evp/p_legacy.c#L45
EVP_PKEY_get1_RSA from 3.0.5 https://github.com/openssl/openssl/blob/openssl-3.0.5/crypto/evp/p_legacy.c#L50
Both obtaining their reference from evp_pkey_get0_RSA_int(pkey);

Another alternative would be to not convert EVP_PKEY * to RSA *, not store RSA * and use EVP_PKEY * directly in RSS PSS RSAE.

Preserving the const until the call to RSA primitives to provide some idea of the size of the change:

diff --git a/crypto/s2n_rsa.c b/crypto/s2n_rsa.c index 164496f..49e5c26 100644 --- a/crypto/s2n_rsa.c +++ b/crypto/s2n_rsa.c @@ -31,7 +31,7 @@ #include "utils/s2n_result.h" #include "utils/s2n_safety.h" -static S2N_RESULT s2n_rsa_modulus_check(RSA *rsa) +static S2N_RESULT s2n_rsa_modulus_check(const RSA *rsa) { /* RSA was made opaque starting in Openssl 1.1.0 */ #if S2N_OPENSSL_VERSION_AT_LEAST(1, 1, 0) && !defined(LIBRESSL_VERSION_NUMBER) @@ -98,7 +98,7 @@ static int s2n_rsa_encrypt(const struct s2n_pkey *pub, struct s2n_blob *in, stru S2N_ERROR_IF(out->size < size, S2N_ERR_NOMEM); const s2n_rsa_public_key *key = &pub->key.rsa_key; - int r = RSA_public_encrypt(in->size, ( unsigned char * )in->data, ( unsigned char * )out->data, key->rsa, + int r = RSA_public_encrypt(in->size, ( unsigned char * )in->data, ( unsigned char * )out->data, (RSA *)key->rsa, RSA_PKCS1_PADDING); S2N_ERROR_IF(r != out->size, S2N_ERR_SIZE_MISMATCH); @@ -118,7 +118,7 @@ static int s2n_rsa_decrypt(const struct s2n_pkey *priv, struct s2n_blob *in, str POSIX_GUARD_RESULT(s2n_get_public_random_data(out)); const s2n_rsa_private_key *key = &priv->key.rsa_key; - int r = RSA_private_decrypt(in->size, ( unsigned char * )in->data, intermediate, key->rsa, RSA_NO_PADDING); + int r = RSA_private_decrypt(in->size, ( unsigned char * )in->data, intermediate, (RSA *)key->rsa, RSA_NO_PADDING); S2N_ERROR_IF(r != expected_size, S2N_ERR_SIZE_MISMATCH); s2n_constant_time_pkcs1_unpad_or_dont(out->data, intermediate, r, out->size); @@ -153,7 +153,7 @@ static int s2n_rsa_key_free(struct s2n_pkey *pkey) struct s2n_rsa_key *rsa_key = &pkey->key.rsa_key; if (rsa_key->rsa == NULL) { return 0; } - RSA_free(rsa_key->rsa); + RSA_free((RSA *)rsa_key->rsa); rsa_key->rsa = NULL; return 0; diff --git a/crypto/s2n_rsa.h b/crypto/s2n_rsa.h index 3bba4ff..2cb090b 100644 --- a/crypto/s2n_rsa.h +++ b/crypto/s2n_rsa.h @@ -28,7 +28,7 @@ struct s2n_pkey; struct s2n_rsa_key { - RSA *rsa; + const RSA *rsa; }; typedef struct s2n_rsa_key s2n_rsa_public_key; diff --git a/crypto/s2n_rsa_pss.c b/crypto/s2n_rsa_pss.c index da034d6..8a2cccd 100644 --- a/crypto/s2n_rsa_pss.c +++ b/crypto/s2n_rsa_pss.c @@ -55,7 +55,7 @@ static S2N_RESULT s2n_rsa_pss_size(const struct s2n_pkey *key, uint32_t *size_ou return S2N_RESULT_OK; } -static int s2n_rsa_is_private_key(RSA *rsa_key) +static int s2n_rsa_is_private_key(const RSA *rsa_key) { const BIGNUM *d = NULL; RSA_get0_key(rsa_key, NULL, NULL, &d); @@ -147,8 +147,8 @@ static int s2n_rsa_validate_params_match(const struct s2n_pkey *pub, const struc * - https://www.openssl.org/docs/manmaster/man3/EVP_PKEY_get0_RSA.html * - https://www.openssl.org/docs/manmaster/man3/RSA_get0_key.html */ - RSA *pub_rsa_key = pub->key.rsa_key.rsa; - RSA *priv_rsa_key = priv->key.rsa_key.rsa; + const RSA *pub_rsa_key = pub->key.rsa_key.rsa; + const RSA *priv_rsa_key = priv->key.rsa_key.rsa; POSIX_ENSURE_REF(pub_rsa_key); POSIX_ENSURE_REF(priv_rsa_key); @@ -183,7 +183,7 @@ static int s2n_rsa_pss_key_free(struct s2n_pkey *pkey) } int s2n_evp_pkey_to_rsa_pss_public_key(struct s2n_rsa_key *rsa_key, EVP_PKEY *pkey) { - RSA *pub_rsa_key = EVP_PKEY_get0_RSA(pkey); + const RSA *pub_rsa_key = EVP_PKEY_get0_RSA(pkey); S2N_ERROR_IF(s2n_rsa_is_private_key(pub_rsa_key), S2N_ERR_KEY_MISMATCH); @@ -193,7 +193,7 @@ int s2n_evp_pkey_to_rsa_pss_public_key(struct s2n_rsa_key *rsa_key, EVP_PKEY *pk int s2n_evp_pkey_to_rsa_pss_private_key(struct s2n_rsa_key *rsa_key, EVP_PKEY *pkey) { - RSA *priv_rsa_key = EVP_PKEY_get0_RSA(pkey); + const RSA *priv_rsa_key = EVP_PKEY_get0_RSA(pkey); POSIX_ENSURE_REF(priv_rsa_key); /* Documentation: https://www.openssl.org/docs/man1.1.1/man3/RSA_check_key.html */ diff --git a/crypto/s2n_rsa_signing.c b/crypto/s2n_rsa_signing.c index 57a3117..5b3e02a 100644 --- a/crypto/s2n_rsa_signing.c +++ b/crypto/s2n_rsa_signing.c @@ -67,7 +67,7 @@ int s2n_rsa_pkcs1v15_sign_digest(const struct s2n_pkey *priv, s2n_hash_algorithm const s2n_rsa_private_key *key = &priv->key.rsa_key; unsigned int signature_size = signature->size; - POSIX_GUARD_OSSL(RSA_sign(NID_type, digest->data, digest->size, signature->data, &signature_size, key->rsa), S2N_ERR_SIGN); + POSIX_GUARD_OSSL(RSA_sign(NID_type, digest->data, digest->size, signature->data, &signature_size, (RSA *)key->rsa), S2N_ERR_SIGN); POSIX_ENSURE(signature_size <= signature->size, S2N_ERR_SIZE_MISMATCH); signature->size = signature_size; @@ -105,7 +105,7 @@ int s2n_rsa_pkcs1v15_verify(const struct s2n_pkey *pub, struct s2n_hash_state *d uint8_t digest_out[S2N_MAX_DIGEST_LEN]; POSIX_GUARD(s2n_hash_digest(digest, digest_out, digest_length)); - POSIX_GUARD_OSSL(RSA_verify(digest_NID_type, digest_out, digest_length, signature->data, signature->size, key->rsa), S2N_ERR_VERIFY_SIGNATURE); + POSIX_GUARD_OSSL(RSA_verify(digest_NID_type, digest_out, digest_length, signature->data, signature->size, (RSA *)key->rsa), S2N_ERR_VERIFY_SIGNATURE); return 0; } diff --git a/tests/unit/s2n_rsa_pss_rsae_test.c b/tests/unit/s2n_rsa_pss_rsae_test.c index 49ac7af..65adc85 100644 --- a/tests/unit/s2n_rsa_pss_rsae_test.c +++ b/tests/unit/s2n_rsa_pss_rsae_test.c @@ -292,6 +292,8 @@ int main(int argc, char **argv) EXPECT_SUCCESS(BN_hex2bn(&d, test_case.key_param_d)); EXPECT_SUCCESS(RSA_set0_key(rsa_key, n, e, d)); + EVP_PKEY_set1_RSA(rsa_public_key.pkey, rsa_key); + struct s2n_stuffer message_stuffer = { 0 }, signature_stuffer = { 0 }; s2n_stuffer_alloc_ro_from_hex_string(&message_stuffer, test_case.message); s2n_stuffer_alloc_ro_from_hex_string(&signature_stuffer, test_case.signature);

As of now I am leaning towards the 'cast at call site' approach #3459 (comment). This way we can add #pragma GCC diagnostic ignored "-Wcast-qual" along with a # Safety comment explaining why its ok.

tests/unit/s2n_rsa_pss_rsae_test.c

toidiu · 2022-08-24T20:01:10Z

EVP_PKEY_get0_RSA()... return the referenced key in pkey or NULL if the key is not of the correct type. The reference count of the returned key is not incremented and so the key must not be freed after use.

I dont think RSA_free is a safe call so storing the pkey with get0 wont work. ac9753b

loqs · 2022-08-25T02:16:22Z

crypto/s2n_rsa_pss.c

@@ -183,7 +183,7 @@ static int s2n_rsa_pss_key_free(struct s2n_pkey *pkey)
 }

 int s2n_evp_pkey_to_rsa_pss_public_key(struct s2n_rsa_key *rsa_key, EVP_PKEY *pkey) {
-    RSA *pub_rsa_key = EVP_PKEY_get0_RSA(pkey);
+    const RSA *pub_rsa_key = EVP_PKEY_get0_RSA(pkey);



Should there be POSIX_ENSURE_REF(pub_rsa_key); as there is in

s2n-tls/crypto/s2n_rsa_pss.c

Lines 196 to 197 in 061e8c6

const RSA *priv_rsa_key = EVP_PKEY_get0_RSA(pkey);

POSIX_ENSURE_REF(priv_rsa_key);

lrstewart · 2022-08-26T04:49:12Z

crypto/s2n_rsa.c

+#if S2N_GCC_VERSION_AT_LEAST(4,6,0)
+#pragma GCC diagnostic push
+#pragma GCC diagnostic ignored "-Wcast-qual"
+#endif


These preprocess directives seem like they impact readability. Maybe we could confine them to one helper method that takes a const RSA* and returns a normal RSA*? It could have a scary name like "s2n_unsafe_get_read_only_rsa"

Also, how/why was that GCC version chosen? Is that the first version with "-Wcast-qual"?

yep, agreed that we should put this all behind a macro. i punted on this because I think the current solution wont work and i am working on that first.

Also, how/why was that GCC version chosen?
i got the 4.6 from a stackoverflow post but cant find any official docs stating the same so I'll need to revisit this.

toidiu · 2022-08-30T19:49:48Z

closing in favor of #3473

toidiu requested a review from a team as a code owner August 19, 2022 07:32

github-actions bot added the s2n-core team label Aug 19, 2022

loqs reviewed Aug 19, 2022

View reviewed changes

lrstewart reviewed Aug 19, 2022

View reviewed changes

tests/unit/s2n_rsa_pss_rsae_test.c Show resolved Hide resolved

toidiu added 3 commits August 24, 2022 00:41

handle const rsa key

061e8c6

safety

f4cc8df

not_safe

ac9753b

toidiu force-pushed the ak-openssl_rsapss branch from aa1ec64 to ac9753b Compare August 24, 2022 07:55

toidiu mentioned this pull request Aug 25, 2022

Tracking issue: openssl3 support #3442

Closed

10 tasks

loqs reviewed Aug 25, 2022

View reviewed changes

lrstewart reviewed Aug 26, 2022

View reviewed changes

toidiu closed this Aug 30, 2022

toidiu deleted the ak-openssl_rsapss branch August 30, 2022 19:49

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

openssl3 integration: update EVP_PKEY_get/set methods #3459

openssl3 integration: update EVP_PKEY_get/set methods #3459

toidiu commented Aug 19, 2022 •

edited

Loading

loqs Aug 19, 2022

lrstewart Aug 19, 2022

toidiu Aug 24, 2022 •

edited

Loading

toidiu Aug 24, 2022 •

edited

Loading

lrstewart Aug 19, 2022

loqs Aug 19, 2022

lrstewart Aug 19, 2022

loqs Aug 19, 2022

loqs Aug 19, 2022

loqs Aug 19, 2022

toidiu Aug 24, 2022 •

edited

Loading

toidiu commented Aug 24, 2022

loqs Aug 25, 2022

lrstewart Aug 26, 2022 •

edited

Loading

lrstewart Aug 26, 2022

toidiu Aug 29, 2022

toidiu commented Aug 30, 2022


		S2N_ERROR_IF(s2n_rsa_is_private_key(pub_rsa_key), S2N_ERR_KEY_MISMATCH);

	const RSA *priv_rsa_key = EVP_PKEY_get0_RSA(pkey);
	POSIX_ENSURE_REF(priv_rsa_key);

openssl3 integration: update EVP_PKEY_get/set methods #3459

openssl3 integration: update EVP_PKEY_get/set methods #3459

Conversation

toidiu commented Aug 19, 2022 • edited Loading

Description of changes:

Testing:

Code analysis:

Choose a reason for hiding this comment

Choose a reason for hiding this comment

toidiu Aug 24, 2022 • edited Loading

Choose a reason for hiding this comment

toidiu Aug 24, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

toidiu Aug 24, 2022 • edited Loading

Choose a reason for hiding this comment

toidiu commented Aug 24, 2022

Choose a reason for hiding this comment

lrstewart Aug 26, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

toidiu commented Aug 30, 2022

toidiu commented Aug 19, 2022 •

edited

Loading

toidiu Aug 24, 2022 •

edited

Loading

toidiu Aug 24, 2022 •

edited

Loading

toidiu Aug 24, 2022 •

edited

Loading

lrstewart Aug 26, 2022 •

edited

Loading