Reject import of private keys that fail PCT

- Also added a provider "validate" method that wraps the PCT test.

Reviewed-by: Shane Lontis <shane.lontis@oracle.com>
Reviewed-by: Tim Hudson <tjh@openssl.org>
Reviewed-by: Matt Caswell <matt@openssl.org>
(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 2e74816ee1d04477359dec5c2124e8c1d2e0ce62..b43581efe5b214a6cf4d3381ec8b16cab9c66a6b 100644 (file)
--- 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 eb1be1c98054314e708cae82aa1b144b5e80dffe..c33eed21d5ad9069e06d8402c9b1f56bd6529ade 100644 (file)
--- 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 91e601aeafffdca8c1d526aefd97a70ef8335452..824909ff2cd186fc91aa3999fcd3303df0173908 100644 (file)
--- a/crypto/encode_decode/encoder_local.h
+++ b/crypto/encode_decode/encoder_local.h
@@ -12,6 +12,7 @@
 #include <openssl/safestack.h>
 #include <openssl/encoder.h>
 #include <openssl/decoder.h>
+#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 3b12e720e12404930c652cdb9cf6eecbf9986e1e..4b9d3b6a3a2d72b1de94c63dab3a74993e90f870 100644 (file)
--- 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 27323ad2b0fa9767bba943ac46b18e2fca084df5..517495d33456b31711f71a4853c8ce8e70670988 100644 (file)
--- a/crypto/store/store_result.c
+++ b/crypto/store/store_result.c
@@ -20,6 +20,7 @@
 #include <openssl/store.h>
 #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 a0d5de65211eb7b0b9ed4decd056507ef0368718..e4195786f505ab91e29837c809a6af37a2921f52 100644 (file)
--- 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 37ebee78cde2d5df1b2bb59df38215f013ff0dc7..d2df667f51c69a378c7d6622e1a536d3d41744b0 100644 (file)
--- 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 6c65183a4cabcd17086c12c2c49b34797bac0190..16f5736ab301aaf59a72706299e803e97a9200a0 100644 (file)
--- 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 ade7c8b5c4a5a66a5cfdd2681338e743c64eb7f2..587fc95b75756b7e0fc6fa74d1f140334e7d47f8 100644 (file)
--- 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))));
 }