From: Viktor Dukhovni Date: Thu, 6 Feb 2025 09:07:11 +0000 (+1100) Subject: Reject import of private keys that fail PCT X-Git-Tag: openssl-3.5.0-alpha1~513 X-Git-Url: http://git.ipfire.org/?a=commitdiff_plain;h=2ea9903c160fe4212b07ec8af630071e35098ceb;p=thirdparty%2Fopenssl.git Reject import of private keys that fail PCT - Also added a provider "validate" method that wraps the PCT test. Reviewed-by: Shane Lontis Reviewed-by: Tim Hudson Reviewed-by: Matt Caswell (Merged from https://github.com/openssl/openssl/pull/26656) --- diff --git a/crypto/encode_decode/decoder_lib.c b/crypto/encode_decode/decoder_lib.c index 2e74816ee1d..b43581efe5b 100644 --- a/crypto/encode_decode/decoder_lib.c +++ b/crypto/encode_decode/decoder_lib.c @@ -312,6 +312,16 @@ OSSL_DECODER_INSTANCE *ossl_decoder_instance_dup(const OSSL_DECODER_INSTANCE *sr return NULL; } +void ossl_decoder_ctx_set_harderr(OSSL_DECODER_CTX *ctx) +{ + ctx->harderr = 1; +} + +int ossl_decoder_ctx_get_harderr(const OSSL_DECODER_CTX *ctx) +{ + return ctx->harderr; +} + int ossl_decoder_ctx_add_decoder_inst(OSSL_DECODER_CTX *ctx, OSSL_DECODER_INSTANCE *di) { diff --git a/crypto/encode_decode/decoder_pkey.c b/crypto/encode_decode/decoder_pkey.c index eb1be1c9805..c33eed21d5a 100644 --- a/crypto/encode_decode/decoder_pkey.c +++ b/crypto/encode_decode/decoder_pkey.c @@ -65,6 +65,7 @@ struct decoder_pkey_data_st { STACK_OF(EVP_KEYMGMT) *keymgmts; char *object_type; /* recorded object data type, may be NULL */ void **object; /* Where the result should end up */ + OSSL_DECODER_CTX *ctx; /* The parent decoder context */ }; static int decoder_construct_pkey(OSSL_DECODER_INSTANCE *decoder_inst, @@ -148,7 +149,15 @@ static int decoder_construct_pkey(OSSL_DECODER_INSTANCE *decoder_inst, * result in the keymgmt. */ if (keymgmt_prov == decoder_prov) { + /* + * When load returns NULL, because, though the provided key material + * is syntactically valid (parsed OK), it is not an acceptable key, + * the reason why the key is rejected would be lost, unless we + * signal a hard error, and suppress resetting for another try. + */ keydata = evp_keymgmt_load(keymgmt, object_ref, object_ref_sz); + if (keydata == NULL) + ossl_decoder_ctx_set_harderr(data->ctx); } else { struct evp_keymgmt_util_try_import_data_st import_data; @@ -561,6 +570,7 @@ ossl_decoder_ctx_for_pkey_dup(OSSL_DECODER_CTX *src, process_data_dest->object = (void **)pkey; process_data_dest->libctx = process_data_src->libctx; process_data_dest->selection = process_data_src->selection; + process_data_dest->ctx = dest; if (!OSSL_DECODER_CTX_set_construct_data(dest, process_data_dest)) { ERR_raise(ERR_LIB_OSSL_DECODER, ERR_R_OSSL_DECODER_LIB); goto err; @@ -576,11 +586,7 @@ ossl_decoder_ctx_for_pkey_dup(OSSL_DECODER_CTX *src, return dest; err: - if (process_data_dest != NULL) { - OPENSSL_free(process_data_dest->propq); - sk_EVP_KEYMGMT_pop_free(process_data_dest->keymgmts, EVP_KEYMGMT_free); - OPENSSL_free(process_data_dest); - } + decoder_clean_pkey_construct_arg(process_data_dest); OSSL_DECODER_CTX_free(dest); return NULL; } diff --git a/crypto/encode_decode/encoder_local.h b/crypto/encode_decode/encoder_local.h index 91e601aeaff..824909ff2cd 100644 --- a/crypto/encode_decode/encoder_local.h +++ b/crypto/encode_decode/encoder_local.h @@ -12,6 +12,7 @@ #include #include #include +#include "crypto/decoder.h" #include "internal/cryptlib.h" #include "internal/passphrase.h" #include "internal/property.h" @@ -156,6 +157,9 @@ struct ossl_decoder_ctx_st { /* For any function that needs a passphrase reader */ struct ossl_passphrase_data_st pwdata; + + /* Signal that further processing should not continue. */ + int harderr; }; const OSSL_PROPERTY_LIST * diff --git a/crypto/ml_kem/ml_kem.c b/crypto/ml_kem/ml_kem.c index 3b12e720e12..4b9d3b6a3a2 100644 --- a/crypto/ml_kem/ml_kem.c +++ b/crypto/ml_kem/ml_kem.c @@ -1536,8 +1536,8 @@ int add_storage(scalar *p, int private, ML_KEM_KEY *key) * After freeing the storage associated with a key that failed to be * constructed, reset the internal pointers back to NULL. */ -static void -free_storage(ML_KEM_KEY *key) +void +ossl_ml_kem_key_reset(ML_KEM_KEY *key) { if (key->t == NULL) return; @@ -1684,7 +1684,7 @@ void ossl_ml_kem_key_free(ML_KEM_KEY *key) OPENSSL_free(key->encoded_dk); } } - free_storage(key); + ossl_ml_kem_key_reset(key); OPENSSL_free(key); } @@ -1771,7 +1771,7 @@ int ossl_ml_kem_parse_public_key(const uint8_t *in, size_t len, ML_KEM_KEY *key) ret = parse_pubkey(in, mdctx, key); if (!ret) - free_storage(key); + ossl_ml_kem_key_reset(key); EVP_MD_CTX_free(mdctx); return ret; } @@ -1799,7 +1799,7 @@ int ossl_ml_kem_parse_private_key(const uint8_t *in, size_t len, ret = parse_prvkey(in, mdctx, key); if (!ret) - free_storage(key); + ossl_ml_kem_key_reset(key); EVP_MD_CTX_free(mdctx); return ret; } @@ -1849,7 +1849,7 @@ int ossl_ml_kem_genkey(uint8_t *pubenc, size_t publen, ML_KEM_KEY *key) EVP_MD_CTX_free(mdctx); if (!ret) { - free_storage(key); + ossl_ml_kem_key_reset(key); return 0; } diff --git a/crypto/store/store_result.c b/crypto/store/store_result.c index 27323ad2b0f..517495d3345 100644 --- a/crypto/store/store_result.c +++ b/crypto/store/store_result.c @@ -20,6 +20,7 @@ #include #include "internal/provider.h" #include "internal/passphrase.h" +#include "crypto/decoder.h" #include "crypto/evp.h" #include "crypto/x509.h" #include "store_local.h" @@ -259,7 +260,8 @@ static EVP_PKEY *try_key_ref(struct extracted_param_data_st *data, static EVP_PKEY *try_key_value(struct extracted_param_data_st *data, OSSL_STORE_CTX *ctx, OSSL_PASSPHRASE_CALLBACK *cb, void *cbarg, - OSSL_LIB_CTX *libctx, const char *propq) + OSSL_LIB_CTX *libctx, const char *propq, + int *harderr) { EVP_PKEY *pk = NULL; OSSL_DECODER_CTX *decoderctx = NULL; @@ -294,6 +296,8 @@ static EVP_PKEY *try_key_value(struct extracted_param_data_st *data, /* No error if this couldn't be decoded */ (void)OSSL_DECODER_from_data(decoderctx, &pdata, &pdatalen); + /* Save the hard error state. */ + *harderr = ossl_decoder_ctx_get_harderr(decoderctx); OSSL_DECODER_CTX_free(decoderctx); return pk; @@ -388,6 +392,7 @@ static int try_key(struct extracted_param_data_st *data, OSSL_STORE_INFO **v, OSSL_LIB_CTX *libctx, const char *propq) { store_info_new_fn *store_info_new = NULL; + int harderr = 0; if (data->object_type == OSSL_OBJECT_UNKNOWN || data->object_type == OSSL_OBJECT_PKEY) { @@ -409,7 +414,7 @@ static int try_key(struct extracted_param_data_st *data, OSSL_STORE_INFO **v, OSSL_PASSPHRASE_CALLBACK *cb = ossl_pw_passphrase_callback_dec; void *cbarg = &ctx->pwdata; - pk = try_key_value(data, ctx, cb, cbarg, libctx, propq); + pk = try_key_value(data, ctx, cb, cbarg, libctx, propq, &harderr); /* * Desperate last maneuver, in case the decoders don't support @@ -418,7 +423,7 @@ static int try_key(struct extracted_param_data_st *data, OSSL_STORE_INFO **v, * This is the same as der2key_decode() does, but in a limited * way and within the walls of libcrypto. */ - if (pk == NULL) + if (pk == NULL && harderr == 0) pk = try_key_value_legacy(data, &store_info_new, ctx, cb, cbarg, libctx, propq); } @@ -450,7 +455,7 @@ static int try_key(struct extracted_param_data_st *data, OSSL_STORE_INFO **v, EVP_PKEY_free(pk); } - return 1; + return harderr == 0; } static int try_cert(struct extracted_param_data_st *data, OSSL_STORE_INFO **v, diff --git a/include/crypto/decoder.h b/include/crypto/decoder.h index a0d5de65211..e4195786f50 100644 --- a/include/crypto/decoder.h +++ b/include/crypto/decoder.h @@ -25,6 +25,8 @@ void *ossl_decoder_from_algorithm(int id, const OSSL_ALGORITHM *algodef, OSSL_DECODER_INSTANCE * ossl_decoder_instance_new(OSSL_DECODER *decoder, void *decoderctx); void ossl_decoder_instance_free(OSSL_DECODER_INSTANCE *decoder_inst); +int ossl_decoder_ctx_get_harderr(const OSSL_DECODER_CTX *ctx); +void ossl_decoder_ctx_set_harderr(OSSL_DECODER_CTX *ctx); OSSL_DECODER_INSTANCE *ossl_decoder_instance_dup(const OSSL_DECODER_INSTANCE *src); int ossl_decoder_ctx_add_decoder_inst(OSSL_DECODER_CTX *ctx, OSSL_DECODER_INSTANCE *di); diff --git a/include/crypto/ml_kem.h b/include/crypto/ml_kem.h index 37ebee78cde..d2df667f51c 100644 --- a/include/crypto/ml_kem.h +++ b/include/crypto/ml_kem.h @@ -203,6 +203,8 @@ typedef struct ossl_ml_kem_key_st { */ ML_KEM_KEY *ossl_ml_kem_key_new(OSSL_LIB_CTX *libctx, const char *properties, int evp_type); +/* Reset a key clearing all public and private key material */ +void ossl_ml_kem_key_reset(ML_KEM_KEY *key); /* Deallocate the key */ void ossl_ml_kem_key_free(ML_KEM_KEY *key); /* diff --git a/providers/implementations/keymgmt/ml_kem_kmgmt.c b/providers/implementations/keymgmt/ml_kem_kmgmt.c index 6c65183a4ca..16f5736ab30 100644 --- a/providers/implementations/keymgmt/ml_kem_kmgmt.c +++ b/providers/implementations/keymgmt/ml_kem_kmgmt.c @@ -42,6 +42,7 @@ static OSSL_FUNC_keymgmt_set_params_fn ml_kem_set_params; static OSSL_FUNC_keymgmt_settable_params_fn ml_kem_settable_params; static OSSL_FUNC_keymgmt_has_fn ml_kem_has; static OSSL_FUNC_keymgmt_match_fn ml_kem_match; +static OSSL_FUNC_keymgmt_validate_fn ml_kem_validate; static OSSL_FUNC_keymgmt_import_fn ml_kem_import; static OSSL_FUNC_keymgmt_export_fn ml_kem_export; static OSSL_FUNC_keymgmt_import_types_fn ml_kem_imexport_types; @@ -60,24 +61,26 @@ typedef struct ml_kem_gen_ctx_st { uint8_t *seed; } PROV_ML_KEM_GEN_CTX; -#ifdef FIPS_MODULE static int ml_kem_pairwise_test(const ML_KEM_KEY *key) { - int ret = 0; +#ifdef FIPS_MODULE OSSL_SELF_TEST *st = NULL; OSSL_CALLBACK *cb = NULL; void *cbarg = NULL; + unsigned char entropy[ML_KEM_RANDOM_BYTES]; +#endif unsigned char secret[ML_KEM_SHARED_SECRET_BYTES]; unsigned char out[ML_KEM_SHARED_SECRET_BYTES]; - unsigned char entropy[ML_KEM_RANDOM_BYTES]; unsigned char *ctext = NULL; const ML_KEM_VINFO *v = ossl_ml_kem_key_vinfo(key); int operation_result = 0; + int ret = 0; /* Unless we have both a public and private key, we can't do the test */ - if (!ossl_ml_kem_have_prvkey(key)) + if (!ossl_ml_kem_have_prvkey(key) || !ossl_ml_kem_have_pubkey(key)) return 1; +#ifdef FIPS_MODULE /* * The functions `OSSL_SELF_TEST_*` will return directly if parameter `st` * is NULL. @@ -90,41 +93,56 @@ static int ml_kem_pairwise_test(const ML_KEM_KEY *key) OSSL_SELF_TEST_onbegin(st, OSSL_SELF_TEST_TYPE_PCT, OSSL_SELF_TEST_DESC_PCT_ML_KEM); - - /* - * Initialise output buffers to avoid collecting random stack memory. - * The `entropy' buffer is filled with an arbitrary non-zero value. - */ - memset(out, 0, sizeof(out)); - memset(entropy, 0125, sizeof(entropy)); +#endif /* FIPS_MODULE */ ctext = OPENSSL_malloc(v->ctext_bytes); if (ctext == NULL) goto err; + memset(out, 0, sizeof(out)); +#ifdef FIPS_MODULE + /* + * The FIPS module does a PCT on power-on, and would leak the RNG + * handle if use random entropy here. So we use fixed entropy in + * the FIPS case. Ideally, the leak will be fixed, and the test + * will also use random entropy in FIPS mode. + */ + memset(entropy, 0125, sizeof(entropy)); operation_result = ossl_ml_kem_encap_seed(ctext, v->ctext_bytes, secret, sizeof(secret), entropy, sizeof(entropy), key); +#else + operation_result = ossl_ml_kem_encap_rand(ctext, v->ctext_bytes, + secret, sizeof(secret), key); +#endif if (operation_result != 1) goto err; +#ifdef FIPS_MODULE OSSL_SELF_TEST_oncorrupt_byte(st, ctext); +#endif operation_result = ossl_ml_kem_decap(out, sizeof(out), ctext, v->ctext_bytes, key); - if (operation_result != 1 - || memcmp(out, secret, sizeof(out)) != 0) + if (operation_result != 1 || memcmp(out, secret, sizeof(out)) != 0) goto err; ret = 1; err: - OPENSSL_free(ctext); +#ifdef FIPS_MODULE OSSL_SELF_TEST_onend(st, ret); OSSL_SELF_TEST_free(st); +#else + if (ret == 0) { + ERR_raise_data(ERR_LIB_PROV, PROV_R_INVALID_KEY, + "public part of %s private key fails to match private", + v->algorithm_name); + } +#endif + OPENSSL_free(ctext); return ret; } -#endif /* FIPS_MODULE */ static void *ml_kem_new(PROV_CTX *ctx, const char *propq, int evp_type) { @@ -174,6 +192,18 @@ static int ml_kem_match(const void *vkey1, const void *vkey2, int selection) return ossl_ml_kem_pubkey_cmp(key1, key2); } +static int ml_kem_validate(const void *vkey, int selection, int check_type) +{ + const ML_KEM_KEY *key = vkey; + + if (!ml_kem_has(key, selection)) + return 0; + + if ((selection & OSSL_KEYMGMT_SELECT_KEYPAIR) == OSSL_KEYMGMT_SELECT_KEYPAIR) + return ml_kem_pairwise_test(key); + return 1; +} + static int ml_kem_export(void *vkey, int selection, OSSL_CALLBACK *param_cb, void *cbarg) { @@ -362,12 +392,13 @@ static int ml_kem_import(void *vkey, int selection, const OSSL_PARAM params[]) include_private = selection & OSSL_KEYMGMT_SELECT_PRIVATE_KEY ? 1 : 0; res = ml_kem_key_fromdata(key, params, include_private); -#ifdef FIPS_MODULE if (res > 0 && include_private && !ml_kem_pairwise_test(key)) { +#ifdef FIPS_MODULE ossl_set_error_state(OSSL_SELF_TEST_TYPE_PCT); +#endif + ossl_ml_kem_key_reset(key); res = 0; } -#endif /* FIPS_MODULE */ return res; } @@ -411,7 +442,13 @@ void *ml_kem_load(const void *reference, size_t reference_sz) goto err; } else if (encoded_dk != NULL) { if (!ossl_ml_kem_parse_private_key(encoded_dk, - key->vinfo->prvkey_bytes, key)) + key->vinfo->prvkey_bytes, key)) { + ERR_raise_data(ERR_LIB_PROV, PROV_R_INVALID_KEY, + "error parsing %s private key", + key->vinfo->algorithm_name); + goto err; + } + if (!ml_kem_pairwise_test(key)) goto err; } OPENSSL_free(encoded_dk); @@ -710,6 +747,7 @@ static void *ml_kem_dup(const void *vkey, int selection) { OSSL_FUNC_KEYMGMT_SETTABLE_PARAMS, (OSSL_FUNC) ml_kem_settable_params }, \ { OSSL_FUNC_KEYMGMT_HAS, (OSSL_FUNC) ml_kem_has }, \ { OSSL_FUNC_KEYMGMT_MATCH, (OSSL_FUNC) ml_kem_match }, \ + { OSSL_FUNC_KEYMGMT_VALIDATE, (OSSL_FUNC) ml_kem_validate }, \ { OSSL_FUNC_KEYMGMT_GEN_INIT, (OSSL_FUNC) ml_kem_##bits##_gen_init }, \ { OSSL_FUNC_KEYMGMT_GEN_SET_PARAMS, (OSSL_FUNC) ml_kem_gen_set_params }, \ { OSSL_FUNC_KEYMGMT_GEN_SETTABLE_PARAMS, (OSSL_FUNC) ml_kem_gen_settable_params }, \ diff --git a/test/recipes/15-test_ml_kem_codecs.t b/test/recipes/15-test_ml_kem_codecs.t index ade7c8b5c4a..587fc95b757 100644 --- a/test/recipes/15-test_ml_kem_codecs.t +++ b/test/recipes/15-test_ml_kem_codecs.t @@ -25,8 +25,9 @@ my @formats = qw(seed-priv priv-only seed-only oqskeypair bare-seed bare-priv); plan skip_all => "ML-KEM isn't supported in this build" if disabled("ml-kem"); -plan tests => @algs * (18 + 10 * @formats); +plan tests => @algs * (23 + 10 * @formats); my $seed = join ("", map {sprintf "%02x", $_} (0..63)); +my $weed = join ("", map {sprintf "%02x", $_} (1..64)); my $ikme = join ("", map {sprintf "%02x", $_} (0..31)); foreach my $alg (@algs) { @@ -158,4 +159,36 @@ foreach my $alg (@algs) { ok(!compare(data_file($txt), $out), sprintf("text form private key: %s with %s", $alg, $f)); } + + # (5 tests): Test import/load PCT failure + my $real = sprintf('real-%s.der', $alg); + my $fake = sprintf('fake-%s.der', $alg); + my $mixt = sprintf('mixt-%s.der', $alg); + my $slen = $alg * 3 / 2; # Secret vector |s| + my $plen = $slen + 64; # Public |t|, |rho| and hash + my $zlen = 32; # FO implicit reject seed + ok(run(app([qw(openssl genpkey -algorithm), "ml-kem-$alg", + qw(-provparam ml-kem.output_formats=bare-priv -pkeyopt), + "hexseed:$seed", qw(-outform DER -out), $real], + sprintf("create real private key: %s", $alg)))); + ok(run(app([qw(openssl genpkey -algorithm), "ml-kem-$alg", + qw(-provparam ml-kem.output_formats=bare-priv -pkeyopt), + "hexseed:$weed", qw(-outform DER -out), $fake], + sprintf("create fake private key: %s", $alg)))); + my $realfh = IO::File->new($real, "r"); + my $fakefh = IO::File->new($fake, "r"); + local $/ = undef; + my $realder = <$realfh>; + my $fakeder = <$fakefh>; + ok (length($realder) == 24 + $slen + $plen + $zlen + && length($fakeder) == 24 + $slen + $plen + $zlen); + my $mixtder = substr($realder, 0, 24 + $slen) + . substr($fakeder, 24 + $slen, $plen) + . substr($realder, 24 + $slen + $plen, $zlen); + my $mixtfh = IO::File->new($mixt, "w"); + print $mixtfh $mixtder; + ok(run(app([qw(openssl pkey -inform DER -noout -in), $real], + sprintf("accept valid keypair: %s", $alg)))); + ok(!run(app([qw(openssl pkey -inform DER -noout -in), $mixt], + sprintf("reject real private and fake public: %s", $alg)))); }