From 0814467cc1b6a2839877277d3efa69cdd4582dd7 Mon Sep 17 00:00:00 2001 From: Richard Levitte Date: Fri, 20 Oct 2023 09:18:19 +0200 Subject: [PATCH] Make DH_check_pub_key() and DH_generate_key() safer yet We already check for an excessively large P in DH_generate_key(), but not in DH_check_pub_key(), and none of them check for an excessively large Q. This change adds all the missing excessive size checks of P and Q. It's to be noted that behaviours surrounding excessively sized P and Q differ. DH_check() raises an error on the excessively sized P, but only sets a flag for the excessively sized Q. This behaviour is mimicked in DH_check_pub_key(). Reviewed-by: Tomas Mraz Reviewed-by: Matt Caswell Reviewed-by: Hugo Landau (Merged from https://github.com/openssl/openssl/pull/22518) (cherry picked from commit ddeb4b6c6d527e54ce9a99cba785c0f7776e54b6) Backported-by: Clemens Lang --- crypto/dh/dh_check.c | 17 +++++++++++++++++ crypto/dh/dh_err.c | 1 + crypto/dh/dh_key.c | 10 ++++++++++ crypto/err/openssl.txt | 1 + include/openssl/dh.h | 6 ++++-- include/openssl/dherr.h | 1 + 6 files changed, 34 insertions(+), 2 deletions(-) diff --git a/crypto/dh/dh_check.c b/crypto/dh/dh_check.c index ae1b03bc92..424a3bb4cd 100644 --- a/crypto/dh/dh_check.c +++ b/crypto/dh/dh_check.c @@ -198,10 +198,27 @@ int DH_check_pub_key(const DH *dh, const BIGNUM *pub_key, int *ret) BN_CTX *ctx = NULL; *ret = 0; + ctx = BN_CTX_new(); if (ctx == NULL) goto err; BN_CTX_start(ctx); + + /* Don't do any checks at all with an excessively large modulus */ + if (BN_num_bits(dh->p) > OPENSSL_DH_CHECK_MAX_MODULUS_BITS) { + DHerr(DH_F_DH_CHECK, DH_R_MODULUS_TOO_LARGE); + *ret = DH_MODULUS_TOO_LARGE | DH_CHECK_PUBKEY_INVALID; + goto err; + } + if (dh->q != NULL && BN_ucmp(dh->p, dh->q) < 0) { + *ret |= DH_CHECK_INVALID_Q_VALUE | DH_CHECK_PUBKEY_INVALID; + /* This may look strange here, but returning 1 after setting ret is + * correct. See also the behavior of the pub_key^q == 1 mod p check + * further down, which behaves in the same way. */ + ok = 1; + goto err; + } + tmp = BN_CTX_get(ctx); if (tmp == NULL || !BN_set_word(tmp, 1)) goto err; diff --git a/crypto/dh/dh_err.c b/crypto/dh/dh_err.c index 92800d3fcc..b3b1e7a706 100644 --- a/crypto/dh/dh_err.c +++ b/crypto/dh/dh_err.c @@ -87,6 +87,7 @@ static const ERR_STRING_DATA DH_str_reasons[] = { {ERR_PACK(ERR_LIB_DH, 0, DH_R_PARAMETER_ENCODING_ERROR), "parameter encoding error"}, {ERR_PACK(ERR_LIB_DH, 0, DH_R_PEER_KEY_ERROR), "peer key error"}, + {ERR_PACK(ERR_LIB_DH, 0, DH_R_Q_TOO_LARGE), "q too large"}, {ERR_PACK(ERR_LIB_DH, 0, DH_R_SHARED_INFO_ERROR), "shared info error"}, {ERR_PACK(ERR_LIB_DH, 0, DH_R_UNABLE_TO_CHECK_GENERATOR), "unable to check generator"}, diff --git a/crypto/dh/dh_key.c b/crypto/dh/dh_key.c index 117f2fa883..9f5e6f6d4c 100644 --- a/crypto/dh/dh_key.c +++ b/crypto/dh/dh_key.c @@ -140,6 +140,11 @@ static int generate_key(DH *dh) return 0; } + if (dh->q != NULL && BN_num_bits(dh->q) > OPENSSL_DH_MAX_MODULUS_BITS) { + DHerr(DH_F_GENERATE_KEY, DH_R_Q_TOO_LARGE); + return 0; + } + ctx = BN_CTX_new(); if (ctx == NULL) goto err; @@ -250,6 +255,12 @@ static int compute_key(unsigned char *key, const BIGNUM *pub_key, DH *dh) DHerr(DH_F_COMPUTE_KEY, DH_R_MODULUS_TOO_LARGE); goto err; } + + if (dh->q != NULL && BN_num_bits(dh->q) > OPENSSL_DH_MAX_MODULUS_BITS) { + DHerr(DH_F_COMPUTE_KEY, DH_R_Q_TOO_LARGE); + goto err; + } + #ifdef OPENSSL_FIPS if (FIPS_mode() && (BN_num_bits(dh->p) < OPENSSL_DH_FIPS_MIN_MODULUS_BITS)) { diff --git a/crypto/err/openssl.txt b/crypto/err/openssl.txt index c0a3cd720b..5e0ff47516 100644 --- a/crypto/err/openssl.txt +++ b/crypto/err/openssl.txt @@ -2151,6 +2151,7 @@DH_R_NO_PARAMETERS_SET:107:no parameters set DH_R_NO_PRIVATE_VALUE:100:no private value DH_R_PARAMETER_ENCODING_ERROR:105:parameter encoding error DH_R_PEER_KEY_ERROR:111:peer key error +DH_R_Q_TOO_LARGE:130:q too large DH_R_SHARED_INFO_ERROR:113:shared info error DH_R_UNABLE_TO_CHECK_GENERATOR:121:unable to check generator DSA_R_BAD_Q_VALUE:102:bad q value diff --git a/include/openssl/dh.h b/include/openssl/dh.h index 6c6ff3636a..b7df43b44f 100644 --- a/include/openssl/dh.h +++ b/include/openssl/dh.h @@ -72,14 +72,16 @@ DECLARE_ASN1_ITEM(DHparams) /* #define DH_GENERATOR_3 3 */ # define DH_GENERATOR_5 5 -/* DH_check error codes */ +/* DH_check error codes, some of them shared with DH_check_pub_key */ # define DH_CHECK_P_NOT_PRIME 0x01 # define DH_CHECK_P_NOT_SAFE_PRIME 0x02 # define DH_UNABLE_TO_CHECK_GENERATOR 0x04 # define DH_NOT_SUITABLE_GENERATOR 0x08 # define DH_CHECK_Q_NOT_PRIME 0x10 -# define DH_CHECK_INVALID_Q_VALUE 0x20 +# define DH_CHECK_INVALID_Q_VALUE 0x20 /* +DH_check_pub_key */ # define DH_CHECK_INVALID_J_VALUE 0x40 +/* DH_MODULUS_TOO_SMALL is 0x80 upstream */ +# define DH_MODULUS_TOO_LARGE 0x100 /* +DH_check_pub_key */ /* DH_check_pub_key error codes */ # define DH_CHECK_PUBKEY_TOO_SMALL 0x01 diff --git a/include/openssl/dherr.h b/include/openssl/dherr.h index 528c819856..d66c35aa8e 100644 --- a/include/openssl/dherr.h +++ b/include/openssl/dherr.h @@ -87,6 +87,7 @@ int ERR_load_DH_strings(void); # define DH_R_NON_FIPS_METHOD 202 # define DH_R_PARAMETER_ENCODING_ERROR 105 # define DH_R_PEER_KEY_ERROR 111 +# define DH_R_Q_TOO_LARGE 130 # define DH_R_SHARED_INFO_ERROR 113 # define DH_R_UNABLE_TO_CHECK_GENERATOR 121 -- 2.41.0