From 1329595786938e01aa861b614559fe2de860b351 Mon Sep 17 00:00:00 2001 From: Yosuke Shimizu Date: Fri, 7 Aug 2026 14:56:04 +0900 Subject: [PATCH] Enforce minimum DH group size on validate and derive --- src/wp_dh_exch.c | 5 + src/wp_dh_kmgmt.c | 42 ++++++- test/test_dh.c | 272 +++++++++++++++++++++++++++++++++++++++++++++- test/unit.c | 2 + test/unit.h | 2 + 5 files changed, 312 insertions(+), 11 deletions(-) diff --git a/src/wp_dh_exch.c b/src/wp_dh_exch.c index 05ea777f..094f593e 100644 --- a/src/wp_dh_exch.c +++ b/src/wp_dh_exch.c @@ -280,6 +280,11 @@ static int wp_dh_derive_secret(wp_DhCtx* ctx, unsigned char* secret, if (!WP_FITS_WORD32(*secLen)) { ok = 0; } + /* Imported groups are not size checked on import. */ + if (ok && (mp_count_bits(&wp_dh_get_key(ctx->key)->p) < WP_DH_MIN_BITS)) { + ERR_raise(ERR_LIB_PROV, PROV_R_KEY_SIZE_TOO_SMALL); + ok = 0; + } /* Get our private key data. */ if (ok && (!wp_dh_get_priv(ctx->key, &priv, &privSz))) { ok = 0; diff --git a/src/wp_dh_kmgmt.c b/src/wp_dh_kmgmt.c index 81b099e5..04adfd94 100644 --- a/src/wp_dh_kmgmt.c +++ b/src/wp_dh_kmgmt.c @@ -1061,17 +1061,26 @@ static int wp_dh_validate(const wp_Dh* dh, int selection, int checkType) if (((selection & OSSL_KEYMGMT_SELECT_DOMAIN_PARAMETERS) != 0) && (dh->id == 0)) { int isPrime = 0; - int pm1Init = 0; + int mpInit = 0; mp_int* p = (mp_int*)&dh->key.p; mp_int* g = (mp_int*)&dh->key.g; + mp_int* q = (mp_int*)&dh->key.q; mp_int pm1; + mp_int t; - /* Explicit domain parameters: p must be prime and g in [2, p-1). */ - if (mp_init(&pm1) != 0) { + /* Explicit domain parameters: p must be large enough and prime, and + * g in [2, p-1). */ + if (mp_count_bits(p) < WP_DH_MIN_BITS) { + ERR_raise(ERR_LIB_PROV, PROV_R_KEY_SIZE_TOO_SMALL); ok = 0; } - else { - pm1Init = 1; + if (ok) { + if (mp_init_multi(&pm1, &t, NULL, NULL, NULL, NULL) != 0) { + ok = 0; + } + else { + mpInit = 1; + } } if (ok) { rc = mp_prime_is_prime(p, 8, &isPrime); @@ -1085,8 +1094,29 @@ static int wp_dh_validate(const wp_Dh* dh, int selection, int checkType) if (ok && ((mp_cmp_d(g, 2) == MP_LT) || (mp_cmp(g, &pm1) != MP_LT))) { ok = 0; } - if (pm1Init) { + /* With q present, q must be prime, divide p-1 and be the order of g. */ + if (ok && (!mp_iszero(q))) { + isPrime = 0; + rc = mp_prime_is_prime(q, 8, &isPrime); + if ((rc != 0) || (!isPrime)) { + ok = 0; + } + if (ok && (mp_mod(&pm1, q, &t) != 0)) { + ok = 0; + } + if (ok && (!mp_iszero(&t))) { + ok = 0; + } + if (ok && (mp_exptmod(g, q, p, &t) != 0)) { + ok = 0; + } + if (ok && (!mp_isone(&t))) { + ok = 0; + } + } + if (mpInit) { mp_clear(&pm1); + mp_clear(&t); } } if (ok && ((selection & OSSL_KEYMGMT_SELECT_PUBLIC_KEY) != 0)) { diff --git a/test/test_dh.c b/test/test_dh.c index da5baf82..1cf3529e 100644 --- a/test/test_dh.c +++ b/test/test_dh.c @@ -22,6 +22,7 @@ #include #include #include +#include #include #ifdef WP_HAVE_DH @@ -119,6 +120,38 @@ static const unsigned char dh_g[] = { 0x02 }; +/* A 256-bit safe prime, well below WP_DH_MIN_BITS. With g = 2 the group is + * well formed and its only defect is its size, so a rejection can only come + * from the minimum size check. */ +static const unsigned char dh_weak_p[] = { + 0xc0, 0xea, 0x51, 0x7d, 0x3c, 0x12, 0xe6, 0x36, + 0x23, 0x1e, 0xb7, 0xf0, 0x18, 0x1b, 0xd2, 0x41, + 0xd9, 0x0d, 0xec, 0xdf, 0x83, 0x4e, 0xde, 0x05, + 0x46, 0x5a, 0x83, 0xe0, 0xea, 0x66, 0xa4, 0xef +}; + +/* pub = 2^priv mod dh_weak_p. */ +static const unsigned char dh_weak_priv[] = { + 0x97, 0x92, 0x03, 0x78, 0xb5, 0xca, 0xbf, 0x30, + 0x9b, 0x4e, 0x0c, 0xb5, 0xf3, 0xf6, 0x61, 0xa8, + 0x57, 0x1e, 0x82, 0x83, 0xb5, 0x27, 0xdc, 0xef, + 0x51 +}; + +static const unsigned char dh_weak_pub[] = { + 0x77, 0x4b, 0x89, 0x5b, 0x07, 0xc8, 0x93, 0xd0, + 0xc0, 0xa4, 0x1f, 0xa6, 0xaa, 0xb2, 0x5a, 0xbe, + 0xa9, 0xa4, 0xec, 0x45, 0x7e, 0x36, 0xcf, 0x1c, + 0x42, 0x14, 0xb1, 0xb6, 0xfc, 0x62, 0x88, 0x61 +}; + +static const unsigned char dh_weak_peer_pub[] = { + 0x7d, 0x86, 0xbc, 0xc8, 0x25, 0x95, 0x6c, 0x78, + 0x87, 0xbe, 0x96, 0xdf, 0x55, 0xf4, 0xfb, 0x09, + 0x4a, 0x1e, 0x45, 0x62, 0xf8, 0x15, 0x21, 0x70, + 0x42, 0x58, 0x2b, 0xf1, 0xa1, 0xca, 0x4f, 0x52 +}; + /* Fixed param from krb5 DH generation, named "o2048" */ static const uint8_t dh_2048[] = { 0x30, 0x82, 0x02, 0x0C, 0x02, 0x82, 0x01, 0x01, @@ -897,10 +930,11 @@ static const unsigned char dh_pad_pubB[] = { 0x84, 0x04, 0x50, 0xa3, 0xfe, 0x78, 0xc7, 0x43 }; -/* Build a DH EVP_PKEY from the fixed dh_p/dh_g group with the given public and - * optional private key material. Used by the padding regression. */ -static int test_dh_key_from_fixed(EVP_PKEY **pkey, const unsigned char *pub, - size_t pubLen, const unsigned char *priv, size_t privLen) +/* Build a DH EVP_PKEY from the given prime (generator 2) with the given public + * and optional private key material. */ +static int test_dh_key_from_group(EVP_PKEY **pkey, const unsigned char *prime, + size_t primeLen, const unsigned char *pub, size_t pubLen, + const unsigned char *priv, size_t privLen) { int err = 0; DH *dh = NULL; @@ -912,7 +946,7 @@ static int test_dh_key_from_fixed(EVP_PKEY **pkey, const unsigned char *pub, dh = DH_new(); err = dh == NULL; if (err == 0) { - p = BN_bin2bn(dh_p, sizeof(dh_p), NULL); + p = BN_bin2bn(prime, (int)primeLen, NULL); g = BN_bin2bn(dh_g, sizeof(dh_g), NULL); err = (p == NULL) || (g == NULL); } @@ -956,6 +990,15 @@ static int test_dh_key_from_fixed(EVP_PKEY **pkey, const unsigned char *pub, return err; } +/* Build a DH EVP_PKEY from the fixed dh_p/dh_g group. Used by the padding + * regression. */ +static int test_dh_key_from_fixed(EVP_PKEY **pkey, const unsigned char *pub, + size_t pubLen, const unsigned char *priv, size_t privLen) +{ + return test_dh_key_from_group(pkey, dh_p, sizeof(dh_p), pub, pubLen, priv, + privLen); +} + /* Derive a shared secret between keyA (priv) and keyB (peer) using libCtx, * setting the OSSL_EXCHANGE_PARAM_PAD parameter to `pad`. The caller owns the * returned buffer and must free it with OPENSSL_free. */ @@ -1735,4 +1778,223 @@ int test_dh_pgen_min_bits(void *data) return err; } +/* Imported explicit parameters below the minimum prime size must be rejected + * by both the validation call and the derive operation. A group at the + * supported size must keep working on the same path. */ +int test_dh_weak_group_rejected(void *data) +{ + int err = 0; + EVP_PKEY *weakKey = NULL; + EVP_PKEY *weakPeer = NULL; + EVP_PKEY *goodKey = NULL; + EVP_PKEY *goodPeer = NULL; + EVP_PKEY_CTX *checkCtx = NULL; + unsigned char *secret = NULL; + size_t secretLen = 0; + + (void)data; + + PRINT_MSG("Testing DH rejection of an undersized explicit group"); + + err = test_dh_key_from_group(&weakKey, dh_weak_p, sizeof(dh_weak_p), + dh_weak_pub, sizeof(dh_weak_pub), dh_weak_priv, sizeof(dh_weak_priv)); + if (err == 0) { + checkCtx = EVP_PKEY_CTX_new_from_pkey(wpLibCtx, weakKey, NULL); + err = checkCtx == NULL; + } + if ((err == 0) && (EVP_PKEY_param_check(checkCtx) == 1)) { + PRINT_ERR_MSG("param_check accepted a %d-bit DH modulus", + (int)sizeof(dh_weak_p) * 8); + err = 1; + } + EVP_PKEY_CTX_free(checkCtx); + checkCtx = NULL; + + /* Derivation must fail even when the caller never validated. */ + if (err == 0) { + err = test_dh_key_from_group(&weakPeer, dh_weak_p, sizeof(dh_weak_p), + dh_weak_peer_pub, sizeof(dh_weak_peer_pub), NULL, 0); + } + if ((err == 0) && (test_dh_derive_with_pad(wpLibCtx, weakKey, weakPeer, 0, + &secret, &secretLen) == 0)) { + PRINT_ERR_MSG("derive succeeded with a %d-bit DH modulus", + (int)sizeof(dh_weak_p) * 8); + err = 1; + } + OPENSSL_free(secret); + secret = NULL; + + /* The 2048-bit explicit group must still validate and derive. */ + if (err == 0) { + err = test_dh_key_from_fixed(&goodKey, dh_pad_pubA, + sizeof(dh_pad_pubA), dh_pad_privA, sizeof(dh_pad_privA)); + } + if (err == 0) { + checkCtx = EVP_PKEY_CTX_new_from_pkey(wpLibCtx, goodKey, NULL); + err = checkCtx == NULL; + } + if ((err == 0) && (EVP_PKEY_param_check(checkCtx) != 1)) { + PRINT_ERR_MSG("param_check rejected a supported DH group"); + err = 1; + } + if (err == 0) { + err = test_dh_key_from_fixed(&goodPeer, dh_pad_pubB, + sizeof(dh_pad_pubB), NULL, 0); + } + if (err == 0) { + err = test_dh_derive_with_pad(wpLibCtx, goodKey, goodPeer, 0, &secret, + &secretLen); + if (err != 0) { + PRINT_ERR_MSG("derive failed with a supported DH group"); + } + } + + OPENSSL_free(secret); + EVP_PKEY_CTX_free(checkCtx); + EVP_PKEY_free(goodPeer); + EVP_PKEY_free(goodKey); + EVP_PKEY_free(weakPeer); + EVP_PKEY_free(weakKey); + return err; +} + +/* Import explicit p/q/g domain parameters and return the param_check result. + * Uses fromdata so that q reaches the provider, which DH_set0_pqg does not + * guarantee. */ +static int test_dh_param_check_pqg(const BIGNUM *p, const BIGNUM *q, + const BIGNUM *g, int *result) +{ + int err = 0; + EVP_PKEY_CTX *ctx = NULL; + EVP_PKEY_CTX *checkCtx = NULL; + EVP_PKEY *pkey = NULL; + OSSL_PARAM_BLD *bld = NULL; + OSSL_PARAM *params = NULL; + + err = (bld = OSSL_PARAM_BLD_new()) == NULL; + if (err == 0) { + err = OSSL_PARAM_BLD_push_BN(bld, OSSL_PKEY_PARAM_FFC_P, p) != 1; + } + if (err == 0) { + err = OSSL_PARAM_BLD_push_BN(bld, OSSL_PKEY_PARAM_FFC_Q, q) != 1; + } + if (err == 0) { + err = OSSL_PARAM_BLD_push_BN(bld, OSSL_PKEY_PARAM_FFC_G, g) != 1; + } + if (err == 0) { + err = (params = OSSL_PARAM_BLD_to_param(bld)) == NULL; + } + if (err == 0) { + err = (ctx = EVP_PKEY_CTX_new_from_name(wpLibCtx, "DH", NULL)) == NULL; + } + if (err == 0) { + err = EVP_PKEY_fromdata_init(ctx) != 1; + } + if (err == 0) { + err = EVP_PKEY_fromdata(ctx, &pkey, EVP_PKEY_KEY_PARAMETERS, params) + != 1; + } + if (err == 0) { + err = (checkCtx = EVP_PKEY_CTX_new_from_pkey(wpLibCtx, pkey, NULL)) + == NULL; + } + if (err == 0) { + *result = EVP_PKEY_param_check(checkCtx); + } + + EVP_PKEY_CTX_free(checkCtx); + EVP_PKEY_free(pkey); + EVP_PKEY_CTX_free(ctx); + OSSL_PARAM_free(params); + OSSL_PARAM_BLD_free(bld); + return err; +} + +/* Domain parameters carrying q must be checked for consistency: q prime, q + * dividing p-1 and g of order q. dh_p is a safe prime, so the matching q is + * (p-1)/2 and g = 2 has order q while g = 5 does not. */ +int test_dh_param_check_q(void *data) +{ + int err = 0; + int i; + int cnt; + int result; + BIGNUM *p = NULL; + BIGNUM *q = NULL; + BIGNUM *g = NULL; + BIGNUM *gBadOrder = NULL; + BIGNUM *qComposite = NULL; + BIGNUM *qNoDivide = NULL; + struct { + const char *name; + const BIGNUM *q; + const BIGNUM *g; + int expect; + } tc[4]; + + (void)data; + + PRINT_MSG("Testing DH explicit q consistency checks"); + + p = BN_bin2bn(dh_p, sizeof(dh_p), NULL); + q = BN_new(); + g = BN_new(); + gBadOrder = BN_new(); + qComposite = BN_new(); + qNoDivide = BN_new(); + err = (p == NULL) || (q == NULL) || (g == NULL) || (gBadOrder == NULL) || + (qComposite == NULL) || (qNoDivide == NULL); + if (err == 0) { + /* p is a safe prime, so (p-1)/2 is the order of the subgroup. */ + err = BN_rshift1(q, p) != 1; + } + if (err == 0) { + err = (BN_set_word(g, 2) != 1) || (BN_set_word(gBadOrder, 5) != 1) || + (BN_set_word(qComposite, 9) != 1) || + (BN_set_word(qNoDivide, 3) != 1); + } + /* A q that is not a factor of p-1 is rejected by the divisibility check, + * which runs first; the order check would also catch it. */ + if (err == 0) { + tc[0].name = "matching q"; + tc[0].q = q; + tc[0].g = g; + tc[0].expect = 1; + tc[1].name = "composite q"; + tc[1].q = qComposite; + tc[1].g = g; + tc[1].expect = 0; + tc[2].name = "q not dividing p-1"; + tc[2].q = qNoDivide; + tc[2].g = g; + tc[2].expect = 0; + tc[3].name = "g not of order q"; + tc[3].q = q; + tc[3].g = gBadOrder; + tc[3].expect = 0; + } + + cnt = (int)(sizeof(tc) / sizeof(tc[0])); + for (i = 0; (err == 0) && (i < cnt); i++) { + result = 0; + err = test_dh_param_check_pqg(p, tc[i].q, tc[i].g, &result); + if ((err == 0) && (tc[i].expect != 0) && (result != 1)) { + PRINT_ERR_MSG("param_check rejected %s", tc[i].name); + err = 1; + } + if ((err == 0) && (tc[i].expect == 0) && (result == 1)) { + PRINT_ERR_MSG("param_check accepted %s", tc[i].name); + err = 1; + } + } + + BN_free(qNoDivide); + BN_free(qComposite); + BN_free(gBadOrder); + BN_free(g); + BN_free(q); + BN_free(p); + return err; +} + #endif /* WP_HAVE_DH */ diff --git a/test/unit.c b/test/unit.c index 24692402..a426c410 100644 --- a/test/unit.c +++ b/test/unit.c @@ -343,6 +343,8 @@ TEST_CASE test_case[] = { TEST_DECL(test_dh_param_check_explicit, NULL), TEST_DECL(test_dh_import_group_no_nul, NULL), TEST_DECL(test_dh_pgen_min_bits, NULL), + TEST_DECL(test_dh_weak_group_rejected, NULL), + TEST_DECL(test_dh_param_check_q, NULL), #ifndef WOLFPROV_QUICKTEST TEST_DECL(test_dh_get_params, NULL), #endif diff --git a/test/unit.h b/test/unit.h index e7829d29..6053f44c 100644 --- a/test/unit.h +++ b/test/unit.h @@ -382,6 +382,8 @@ int test_dh_fromdata_oversize(void *data); int test_dh_param_check_explicit(void *data); int test_dh_import_group_no_nul(void *data); int test_dh_pgen_min_bits(void *data); +int test_dh_weak_group_rejected(void *data); +int test_dh_param_check_q(void *data); #endif /* WP_HAVE_DH */ #ifdef WP_HAVE_ECC