From: Viktor Dukhovni Date: Sun, 24 May 2026 13:12:21 +0000 (+1000) Subject: LMS, DH: harden empty fromdata X-Git-Url: http://git.ipfire.org/gitweb.cgi?a=commitdiff_plain;h=4749c84433242ec5845a7c21a16da89cb48eea02;p=thirdparty%2Fopenssl.git LMS, DH: harden empty fromdata EVP_PKEY_fromdata for the LMS keymgmt accepted an OSSL_PARAM[] that omits OSSL_PKEY_PARAM_PUB_KEY, returning success with an LMS_KEY whose lms_params and ots_params remain NULL. Without even basic algorithm parameters (derived from the key content) the key is malformed. EVP_PKEY_fromdata for DH/DHX accepts an empty array and yields a DH with NULL params.p / params.g. Several DH check entry points (DH_check, DH_check_params, DH_check_pub_key) then read dh->params.p / .g via BN_num_bits or BN_is_odd before any NULL check. Add defensive guards at the top of each that report failure via *ret without dereferencing NULL; the existing return-1-with-flags contract is preserved. A new test_fromdata in endecode_test drives every supported keymgmt with an empty OSSL_PARAM[] for both EVP_PKEY_PUBLIC_KEY and EVP_PKEY_KEYPAIR selections, and tests that any returned key is sufficiently well behaved. Reviewed-by: Neil Horman Reviewed-by: Bob Beck MergeDate: Thu Jun 18 08:02:33 2026 (Merged from https://github.com/openssl/openssl/pull/31252) --- diff --git a/crypto/dh/dh_check.c b/crypto/dh/dh_check.c index 3002609b68f..96256f92834 100644 --- a/crypto/dh/dh_check.c +++ b/crypto/dh/dh_check.c @@ -74,6 +74,14 @@ int DH_check_params(const DH *dh, int *ret) BN_CTX *ctx = NULL; *ret = 0; + /* + * A DH with no modulus or generator cannot be checked. Report + * the failure via |*ret| rather than dereferencing NULL below. + */ + if (dh->params.p == NULL || dh->params.g == NULL) { + *ret = DH_NOT_SUITABLE_GENERATOR | DH_CHECK_P_NOT_PRIME; + return 1; + } ctx = BN_CTX_new_ex(dh->libctx); if (ctx == NULL) goto err; @@ -150,6 +158,11 @@ int DH_check(const DH *dh, int *ret) int nid = DH_get_nid((DH *)dh); *ret = 0; + /* A DH with no modulus or generator cannot be checked. */ + if (dh->params.p == NULL || dh->params.g == NULL) { + *ret = DH_NOT_SUITABLE_GENERATOR | DH_CHECK_P_NOT_PRIME; + return 1; + } if (nid != NID_undef) return 1; @@ -250,6 +263,15 @@ int DH_check_pub_key_ex(const DH *dh, const BIGNUM *pub_key) */ int DH_check_pub_key(const DH *dh, const BIGNUM *pub_key, int *ret) { + *ret = 0; + /* + * Without a modulus we cannot check anything; signal failure via + * |*ret| rather than crashing in BN_num_bits below. + */ + if (dh->params.p == NULL) { + *ret = DH_CHECK_PUBKEY_INVALID; + return 1; + } /* Don't do any checks at all with an excessively large modulus */ if (BN_num_bits(dh->params.p) > OPENSSL_DH_CHECK_MAX_MODULUS_BITS) { ERR_raise(ERR_LIB_DH, DH_R_MODULUS_TOO_LARGE); diff --git a/crypto/lms/lms_pubkey_decode.c b/crypto/lms/lms_pubkey_decode.c index b5bb3dc74bc..8c2ee0ff5e4 100644 --- a/crypto/lms/lms_pubkey_decode.c +++ b/crypto/lms/lms_pubkey_decode.c @@ -122,11 +122,17 @@ err: */ int ossl_lms_pubkey_from_params(const OSSL_PARAM *pub, LMS_KEY *lmskey) { - if (pub != NULL) { - if (pub->data == NULL - || pub->data_type != OSSL_PARAM_OCTET_STRING - || !ossl_lms_pubkey_decode(pub->data, pub->data_size, lmskey)) - return 0; - } + /* + * An LMS_KEY with no public key material is unusable - lms_params + * and ots_params would remain NULL and any subsequent consumer op + * (lms_get_params -> ossl_lms_key_get_pub_len, lms_verify_msg_init + * -> setdigest, ...) would dereference NULL. Require the caller + * to supply a well-formed OSSL_PKEY_PARAM_PUB_KEY. + */ + if (pub == NULL + || pub->data == NULL + || pub->data_type != OSSL_PARAM_OCTET_STRING + || !ossl_lms_pubkey_decode(pub->data, pub->data_size, lmskey)) + return 0; return 1; } diff --git a/test/endecode_test.c b/test/endecode_test.c index 8afbee59ba3..a2c5d4009d1 100644 --- a/test/endecode_test.c +++ b/test/endecode_test.c @@ -1009,19 +1009,13 @@ static int test_dup(const char *type, EVP_PKEY *key) dup = NULL; /* - * 3. Dup an embryonic key (just algorithm + domain parameters, no - * key bits). Not every keymgmt allows building such a key: RSA's - * dup op, for instance, deliberately refuses any selection without - * keypair bits ("do not allow creating empty keys by duplication"), - * which makes EVP_PKEY_copy_parameters() fail for RSA / RSA-PSS. - * Treat that as a graceful skip rather than a hard failure. - * - * Where an embryo can be built, the right comparator is - * EVP_PKEY_parameters_eq(): EVP_PKEY_eq() returns 0 for material- - * less keys because it requires public bits to match. Algorithms - * without domain parameters may legitimately answer -2 ("nothing - * to compare"). We only reject 0 (definitively unequal) and -1 - * (different keymgmts). + * 3. Dup an embryonic key (algorithm + domain parameters only, + * no key bits). Some keymgmts (RSA, RSA-PSS) refuse to build + * such a key via EVP_PKEY_copy_parameters(); treat that as a + * graceful skip. Where an embryo can be built we compare via + * EVP_PKEY_parameters_eq() since EVP_PKEY_eq() requires key + * bits to match. Algorithms with no domain parameters may + * legitimately answer -2 ("nothing to compare"). */ embryo = make_embryonic_copy(key); if (embryo != NULL) { @@ -1052,6 +1046,254 @@ end: return ok; } +/* + * Drive EVP_PKEY_fromdata with the supplied OSSL_PARAM[] (NULL = + * empty array) for the given selection. Either outcome is accepted: + * fromdata may reject the input, or it may succeed and yield a key + * with at most algorithm-bound parameters. In the success case a + * battery of common consumer ops must not crash on the resulting + * key; their return values are not asserted. + */ +static int run_empty_fromdata_probe(const char *type, EVP_PKEY_CTX *cctx, + int selection, OSSL_PARAM *params, const char *selname) +{ + EVP_PKEY *pkey = NULL; + OSSL_PARAM empty[1]; + int r; + int ok = 0; + + if (params == NULL) { + empty[0] = OSSL_PARAM_construct_end(); + params = empty; + } + + if (!TEST_int_gt(EVP_PKEY_fromdata_init(cctx), 0)) { + TEST_info("%s: fromdata_init failed (%s)", type, selname); + goto end; + } + r = EVP_PKEY_fromdata(cctx, &pkey, selection, params); + if (r <= 0) { + /* Rejection is fine, but the out-pointer must remain NULL. */ + if (!TEST_ptr_null(pkey)) { + TEST_info("%s: fromdata returned %d but pkey != NULL (%s)", + type, r, selname); + goto end; + } + ok = 1; + goto end; + } + if (!TEST_ptr(pkey)) { + TEST_info("%s: fromdata returned %d but pkey == NULL (%s)", + type, r, selname); + goto end; + } + /* + * Walk a battery of common consumer ops on the resulting key. + * Their return values are not asserted - a contentless key may + * fail every op - only crashing is forbidden. + */ + (void)EVP_PKEY_get_bits(pkey); + (void)EVP_PKEY_get_security_bits(pkey); + (void)EVP_PKEY_get_size(pkey); + (void)EVP_PKEY_eq(pkey, pkey); + (void)EVP_PKEY_parameters_eq(pkey, pkey); + { + OSSL_PARAM *out = NULL; + + if (EVP_PKEY_todata(pkey, selection, &out) > 0) + OSSL_PARAM_free(out); + } + { + EVP_PKEY *clone = EVP_PKEY_dup(pkey); + + EVP_PKEY_free(clone); + } + { + BIO *bio = BIO_new(BIO_s_null()); + + if (bio != NULL) { + (void)EVP_PKEY_print_public(bio, pkey, 0, NULL); + (void)EVP_PKEY_print_private(bio, pkey, 0, NULL); + (void)EVP_PKEY_print_params(bio, pkey, 0, NULL); + BIO_free(bio); + } + } + { + /* The param/public/private/pairwise check family. */ + EVP_PKEY_CTX *vctx = EVP_PKEY_CTX_new_from_pkey(NULL, pkey, NULL); + + if (vctx != NULL) { + (void)EVP_PKEY_param_check(vctx); + (void)EVP_PKEY_param_check_quick(vctx); + (void)EVP_PKEY_public_check(vctx); + (void)EVP_PKEY_public_check_quick(vctx); + (void)EVP_PKEY_private_check(vctx); + (void)EVP_PKEY_pairwise_check(vctx); + EVP_PKEY_CTX_free(vctx); + } + } + ok = 1; +end: + EVP_PKEY_free(pkey); + return ok; +} + +static int probe_empty_fromdata(const char *type, EVP_PKEY *prototype, + int selection, const char *selname) +{ + EVP_PKEY_CTX *cctx = NULL; + int ok = 0; + + if (!TEST_ptr(cctx = EVP_PKEY_CTX_new_from_pkey(NULL, prototype, NULL))) { + TEST_info("%s: CTX alloc failed for empty fromdata (%s)", + type, selname); + goto end; + } + ok = run_empty_fromdata_probe(type, cctx, selection, NULL, selname); +end: + EVP_PKEY_CTX_free(cctx); + return ok; +} + +/* + * Same probe driven from an algorithm name rather than a prototype + * key, for keymgmts without a keygen path (e.g. LMS). Algorithms + * not loadable under the active provider set are silently skipped. + * |params| may be NULL (= empty OSSL_PARAM[]) or a caller-built + * partial array. + */ +static int probe_fromdata_by_name(const char *name, int selection, + OSSL_PARAM *params, const char *selname) +{ + EVP_PKEY_CTX *cctx = NULL; + int ok = 1; + + cctx = EVP_PKEY_CTX_new_from_name(NULL, name, NULL); + if (cctx == NULL) + return 1; + ok = run_empty_fromdata_probe(name, cctx, selection, params, selname); + EVP_PKEY_CTX_free(cctx); + return ok; +} + +/* + * Drive EVP_PKEY_fromdata with an empty OSSL_PARAM[] for both + * EVP_PKEY_PUBLIC_KEY and EVP_PKEY_KEYPAIR selections. Either + * outcome is acceptable: fromdata rejects, or it succeeds and the + * resulting key survives the consumer-op battery without crashing. + */ +static int test_fromdata(const char *type, EVP_PKEY *prototype) +{ + if (!TEST_ptr(prototype)) { + TEST_info("%s: no prototype key", type); + return 0; + } + if (!probe_empty_fromdata(type, prototype, EVP_PKEY_PUBLIC_KEY, + "EVP_PKEY_PUBLIC_KEY")) + return 0; + if (!probe_empty_fromdata(type, prototype, EVP_PKEY_KEYPAIR, + "EVP_PKEY_KEYPAIR")) + return 0; + return 1; +} + +/* + * Companion to test_fromdata for keymgmts without a keygen path + * (LMS and similar verify-only / signature-only algorithms), plus + * named-group-only partial-shape variants for the prototype-matrix + * algorithms. Each entry is one (name, selection, params-builder) + * shape; a NULL builder means "use an empty OSSL_PARAM[]". + * Algorithms not loadable under the active provider set are + * silently skipped. + */ +typedef int (*fromdata_shape_build_fn)(OSSL_PARAM_BLD *bld); + +struct fromdata_shape { + const char *name; /* keymgmt algorithm name */ + int selection; /* EVP_PKEY_PUBLIC_KEY / KEYPAIR / etc. */ + fromdata_shape_build_fn build; /* NULL -> empty OSSL_PARAM[] */ + const char *label; /* diagnostic label */ +}; + +#ifndef OPENSSL_NO_DH +static int build_dh_named_group(OSSL_PARAM_BLD *bld) +{ + return OSSL_PARAM_BLD_push_utf8_string(bld, OSSL_PKEY_PARAM_GROUP_NAME, + "ffdhe2048", 0); +} +#endif + +#ifndef OPENSSL_NO_EC +static int build_ec_named_group(OSSL_PARAM_BLD *bld) +{ + return OSSL_PARAM_BLD_push_utf8_string(bld, OSSL_PKEY_PARAM_GROUP_NAME, + "P-256", 0); +} +#ifndef OPENSSL_NO_SM2 +static int build_sm2_named_group(OSSL_PARAM_BLD *bld) +{ + return OSSL_PARAM_BLD_push_utf8_string(bld, OSSL_PKEY_PARAM_GROUP_NAME, + "SM2", 0); +} +#endif +#endif + +static const struct fromdata_shape no_keygen_shapes[] = { + /* Empty OSSL_PARAM[] for LMS (no keygen path). */ + { "LMS", EVP_PKEY_PUBLIC_KEY, NULL, "LMS / empty / PUBLIC_KEY" }, + { "LMS", EVP_PKEY_KEYPAIR, NULL, "LMS / empty / KEYPAIR" }, +/* Named-group-only partial shapes. */ +#ifndef OPENSSL_NO_DH + { "DH", EVP_PKEY_KEYPAIR, build_dh_named_group, + "DH / named group only / KEYPAIR" }, + { "DH", EVP_PKEY_PUBLIC_KEY, build_dh_named_group, + "DH / named group only / PUBLIC_KEY" }, +#endif +#ifndef OPENSSL_NO_EC + { "EC", EVP_PKEY_KEYPAIR, build_ec_named_group, + "EC / group only / KEYPAIR" }, + { "EC", EVP_PKEY_PUBLIC_KEY, build_ec_named_group, + "EC / group only / PUBLIC_KEY" }, +#ifndef OPENSSL_NO_SM2 + { "SM2", EVP_PKEY_KEYPAIR, build_sm2_named_group, + "SM2 / group only / KEYPAIR" }, +#endif +#endif +}; + +static int test_fromdata_no_keygen(void) +{ + size_t i; + + for (i = 0; i < OSSL_NELEM(no_keygen_shapes); i++) { + const struct fromdata_shape *s = &no_keygen_shapes[i]; + OSSL_PARAM_BLD *bld = NULL; + OSSL_PARAM *params = NULL; + int ok; + + if (s->build != NULL) { + if (!TEST_ptr(bld = OSSL_PARAM_BLD_new())) + return 0; + if (!s->build(bld)) { + TEST_info("%s: builder failed", s->label); + OSSL_PARAM_BLD_free(bld); + return 0; + } + params = OSSL_PARAM_BLD_to_param(bld); + if (!TEST_ptr(params)) { + OSSL_PARAM_BLD_free(bld); + return 0; + } + } + ok = probe_fromdata_by_name(s->name, s->selection, params, s->label); + OSSL_PARAM_free(params); + OSSL_PARAM_BLD_free(bld); + if (!ok) + return 0; + } + return 1; +} + #define KEYS(KEYTYPE) \ static EVP_PKEY *key_##KEYTYPE = NULL #define MAKE_KEYS(KEYTYPE, KEYTYPEstr, params) \ @@ -1105,6 +1347,10 @@ end: static int test_dup_##KEYTYPE(void) \ { \ return test_dup(KEYTYPEstr, key_##KEYTYPE); \ + } \ + static int test_fromdata_##KEYTYPE(void) \ + { \ + return test_fromdata(KEYTYPEstr, key_##KEYTYPE); \ } #define ADD_TEST_SUITE(KEYTYPE) \ @@ -1118,6 +1364,7 @@ end: ADD_TEST(test_public_##KEYTYPE##_via_DER); \ ADD_TEST(test_public_##KEYTYPE##_via_PEM); \ ADD_TEST(test_dup_##KEYTYPE); \ + ADD_TEST(test_fromdata_##KEYTYPE); \ } \ } while (0) @@ -1840,6 +2087,15 @@ int setup_tests(void) ADD_TEST_SUITE(SLH_DSA_SHAKE_256f); } #endif /* OPENSSL_NO_SLH_DSA */ + + /* + * Cover keymgmts that have no keygen path and so don't + * appear in the prototype matrix above (LMS, and any future + * verify-only / signature-only algorithms). The probe is a + * no-op for algorithms not loadable under the active provider + * set (e.g. LMS under FIPS), so the test stays portable. + */ + ADD_TEST(test_fromdata_no_keygen); } return ok;