Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

openssl3 integration: use EVP_PKEY_get1_RSA to avoid modifying const RSA key #3473

Closed
wants to merge 5 commits into from
Closed

openssl3 integration: use EVP_PKEY_get1_RSA to avoid modifying const RSA key #3473

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
5fa2525
use EVP_PKEY_get1_RSA to avoid modifying const RSA key and free copy …
toidiu Aug 30, 2022
7ed8e66
pr_comments
toidiu Aug 30, 2022
29d28e2
ensure ref
toidiu Aug 31, 2022
635eaf4
cleanup_comment
toidiu Aug 31, 2022
8f860a7
comments
toidiu Aug 31, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
24 changes: 14 additions & 10 deletions crypto/s2n_rsa_pss.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,18 +17,15 @@
#include <openssl/rsa.h>
#include <stdint.h>

#include "error/s2n_errno.h"

#include "stuffer/s2n_stuffer.h"

#include "crypto/s2n_evp_signing.h"
#include "crypto/s2n_hash.h"
#include "crypto/s2n_openssl.h"
#include "crypto/s2n_rsa.h"
#include "crypto/s2n_rsa_pss.h"
#include "crypto/s2n_rsa_signing.h"
#include "crypto/s2n_pkey.h"

#include "error/s2n_errno.h"
#include "stuffer/s2n_stuffer.h"
#include "utils/s2n_blob.h"
#include "utils/s2n_random.h"
#include "utils/s2n_safety.h"
Expand Down Expand Up @@ -144,7 +141,7 @@ static int s2n_rsa_validate_params_match(const struct s2n_pkey *pub, const struc
POSIX_ENSURE_REF(priv);

/* OpenSSL Documentation Links:
* - https://www.openssl.org/docs/manmaster/man3/EVP_PKEY_get0_RSA.html
* - https://www.openssl.org/docs/manmaster/man3/EVP_PKEY_get1_RSA.html
* - https://www.openssl.org/docs/manmaster/man3/RSA_get0_key.html
*/
RSA *pub_rsa_key = pub->key.rsa_key.rsa;
Expand Down Expand Up @@ -176,14 +173,21 @@ static int s2n_rsa_pss_keys_match(const struct s2n_pkey *pub, const struct s2n_p

static int s2n_rsa_pss_key_free(struct s2n_pkey *pkey)
{
/* This object does not own the reference to the key --
* s2n_pkey handles it. */
POSIX_ENSURE_REF(pkey);
struct s2n_rsa_key *rsa_key = &pkey->key.rsa_key;
if (rsa_key->rsa == NULL) {
return 0;
}
Comment on lines +179 to +180
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit

Suggested change
return 0;
}
return S2N_SUCCESS;
}


RSA_free(rsa_key->rsa);
rsa_key->rsa = NULL;

return 0;
}

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);
POSIX_ENSURE_REF(pub_rsa_key);

S2N_ERROR_IF(s2n_rsa_is_private_key(pub_rsa_key), S2N_ERR_KEY_MISMATCH);

Expand All @@ -193,7 +197,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);
RSA *priv_rsa_key = EVP_PKEY_get1_RSA(pkey);
POSIX_ENSURE_REF(priv_rsa_key);
Comment on lines +200 to 201
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


/* Documentation: https://www.openssl.org/docs/man1.1.1/man3/RSA_check_key.html */
Expand Down
23 changes: 14 additions & 9 deletions tests/unit/s2n_rsa_pss_rsae_test.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,9 +22,11 @@
#include "crypto/s2n_hash.h"
#include "crypto/s2n_rsa.h"
#include "crypto/s2n_rsa_pss.h"
#include "error/s2n_errno.h"
#include "stuffer/s2n_stuffer.h"
#include "tls/s2n_connection.h"
#include "tls/s2n_config.h"
#include "utils/s2n_safety.h"
#include "utils/s2n_random.h"

#define HASH_ALG S2N_HASH_SHA256
Expand Down Expand Up @@ -195,7 +197,7 @@ int main(int argc, char **argv)
EXPECT_SUCCESS(s2n_pkey_free(&rsa_public_key));
}

#if RSA_PSS_CERTS_SUPPORTED
#if RSA_PSS_CERTS_SUPPORTED

struct s2n_cert_chain_and_key *rsa_pss_cert_chain;
EXPECT_SUCCESS(s2n_test_cert_chain_and_key_new(&rsa_pss_cert_chain,
Expand Down Expand Up @@ -245,11 +247,10 @@ int main(int argc, char **argv)
EXPECT_SUCCESS(s2n_asn1der_to_public_key_and_type(&rsa_pss_public_key, &rsa_pss_pkey_type, &rsa_pss_cert_chain->cert_chain->head->raw));
EXPECT_EQUAL(rsa_pss_pkey_type, S2N_PKEY_TYPE_RSA_PSS);

/* Set the keys equal */
const BIGNUM *n, *e, *d;
RSA_get0_key(EVP_PKEY_get0_RSA(rsa_public_key.pkey), &n, &e, &d);
EXPECT_SUCCESS(RSA_set0_key(EVP_PKEY_get0_RSA(rsa_pss_public_key.pkey),
BN_dup(n), BN_dup(e), BN_dup(d)));
/* Set the keys equal. */
RSA *rsa_key_copy = EVP_PKEY_get1_RSA(rsa_public_key.pkey);
POSIX_GUARD_OSSL(EVP_PKEY_set1_RSA(rsa_pss_public_key.pkey, rsa_key_copy), S2N_ERR_KEY_INIT);
RSA_free(rsa_key_copy);

/* RSA signed with PSS, RSA_PSS verified with PSS */
{
Expand Down Expand Up @@ -285,12 +286,15 @@ int main(int argc, char **argv)
&rsa_cert_chain->cert_chain->head->raw));
EXPECT_EQUAL(rsa_pkey_type, S2N_PKEY_TYPE_RSA);

RSA *rsa_key = EVP_PKEY_get0_RSA(rsa_public_key.pkey);
/* Modify the rsa_public_key for each test_case. */
RSA *rsa_key_copy = EVP_PKEY_get1_RSA(rsa_public_key.pkey);
BIGNUM *n = BN_new(), *e = BN_new(), *d = BN_new();
EXPECT_SUCCESS(BN_hex2bn(&n, test_case.key_param_n));
EXPECT_SUCCESS(BN_hex2bn(&e, test_case.key_param_e));
EXPECT_SUCCESS(BN_hex2bn(&d, test_case.key_param_d));
EXPECT_SUCCESS(RSA_set0_key(rsa_key, n, e, d));
EXPECT_SUCCESS(RSA_set0_key(rsa_key_copy, n, e, d));
POSIX_GUARD_OSSL(EVP_PKEY_set1_RSA(rsa_public_key.pkey, rsa_key_copy), S2N_ERR_KEY_INIT);
RSA_free(rsa_key_copy);

struct s2n_stuffer message_stuffer = { 0 }, signature_stuffer = { 0 };
s2n_stuffer_alloc_ro_from_hex_string(&message_stuffer, test_case.message);
Expand All @@ -311,7 +315,8 @@ int main(int argc, char **argv)
}

EXPECT_SUCCESS(s2n_cert_chain_and_key_free(rsa_pss_cert_chain));
#endif
#endif

EXPECT_SUCCESS(s2n_cert_chain_and_key_free(rsa_cert_chain));
END_TEST();
}