Skip to content

Commit

Permalink
Fix CVE-2023-5678
Browse files Browse the repository at this point in the history 
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().
  • Loading branch information
@dongbeiouba
dongbeiouba committed Nov 29, 2023
1 parent 6a4ad4d commit 1ee30a7
Show file tree
Hide file tree
Showing 9 changed files with 72 additions and 5 deletions.
2 changes: 1 addition & 1 deletion CHANGES
View file
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@

Changes between 8.3.3 and 8.3.4 [xxxx年xx月xx日]

*)
*) 修复CVE-2023-5678

Changes between 8.3.2 and 8.3.3 [2023年08月28日]

Expand Down
25 changes: 25 additions & 0 deletions crypto/dh/dh_check.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,10 @@ int DH_check_params_ex(const DH *dh)
DHerr(DH_F_DH_CHECK_PARAMS_EX, DH_R_CHECK_P_NOT_PRIME);
if ((errflags & DH_NOT_SUITABLE_GENERATOR) != 0)
DHerr(DH_F_DH_CHECK_PARAMS_EX, DH_R_NOT_SUITABLE_GENERATOR);
if ((errflags & DH_MODULUS_TOO_SMALL) != 0)
DHerr(DH_F_DH_CHECK_PARAMS_EX, DH_R_MODULUS_TOO_SMALL);
if ((errflags & DH_MODULUS_TOO_LARGE) != 0)
DHerr(DH_F_DH_CHECK_PARAMS_EX, DH_R_MODULUS_TOO_LARGE);

return errflags == 0;
}
Expand Down Expand Up @@ -58,6 +62,10 @@ int DH_check_params(const DH *dh, int *ret)
goto err;
if (BN_cmp(dh->g, tmp) >= 0)
*ret |= DH_NOT_SUITABLE_GENERATOR;
if (BN_num_bits(dh->p) < DH_MIN_MODULUS_BITS)
*ret |= DH_MODULUS_TOO_SMALL;
if (BN_num_bits(dh->p) > OPENSSL_DH_MAX_MODULUS_BITS)
*ret |= DH_MODULUS_TOO_LARGE;

ok = 1;
err:
Expand Down Expand Up @@ -91,6 +99,10 @@ int DH_check_ex(const DH *dh)
DHerr(DH_F_DH_CHECK_EX, DH_R_CHECK_P_NOT_PRIME);
if ((errflags & DH_CHECK_P_NOT_SAFE_PRIME) != 0)
DHerr(DH_F_DH_CHECK_EX, DH_R_CHECK_P_NOT_SAFE_PRIME);
if ((errflags & DH_MODULUS_TOO_SMALL) != 0)
DHerr(DH_F_DH_CHECK_EX, DH_R_MODULUS_TOO_SMALL);
if ((errflags & DH_MODULUS_TOO_LARGE) != 0)
DHerr(DH_F_DH_CHECK_EX, DH_R_MODULUS_TOO_LARGE);

return errflags == 0;
}
Expand All @@ -104,6 +116,7 @@ int DH_check(const DH *dh, int *ret)
/* 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;
return 0;
}

Expand Down Expand Up @@ -197,6 +210,18 @@ int DH_check_pub_key(const DH *dh, const BIGNUM *pub_key, int *ret)
BN_CTX *ctx = NULL;

*ret = 0;

/* Don't do any checks at all with an excessively large modulus */
if (BN_num_bits(dh->p) > OPENSSL_DH_CHECK_MAX_MODULUS_BITS) {
*ret = DH_MODULUS_TOO_LARGE | DH_CHECK_PUBKEY_INVALID;
return 0;
}

if (dh->q != NULL && BN_ucmp(dh->p, dh->q) < 0) {
*ret |= DH_CHECK_INVALID_Q_VALUE | DH_CHECK_PUBKEY_INVALID;
return 1;
}

ctx = BN_CTX_new();
if (ctx == NULL)
goto err;
Expand Down
2 changes: 2 additions & 0 deletions crypto/dh/dh_err.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -75,13 +75,15 @@ static const ERR_STRING_DATA DH_str_reasons[] = {
{ERR_PACK(ERR_LIB_DH, 0, DH_R_KEYS_NOT_SET), "keys not set"},
{ERR_PACK(ERR_LIB_DH, 0, DH_R_MISSING_PUBKEY), "missing pubkey"},
{ERR_PACK(ERR_LIB_DH, 0, DH_R_MODULUS_TOO_LARGE), "modulus too large"},
{ERR_PACK(ERR_LIB_DH, 0, DH_R_MODULUS_TOO_SMALL), "modulus too small"},
{ERR_PACK(ERR_LIB_DH, 0, DH_R_NOT_SUITABLE_GENERATOR),
"not suitable generator"},
{ERR_PACK(ERR_LIB_DH, 0, DH_R_NO_PARAMETERS_SET), "no parameters set"},
{ERR_PACK(ERR_LIB_DH, 0, DH_R_NO_PRIVATE_VALUE), "no private value"},
{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"},
Expand Down
22 changes: 22 additions & 0 deletions crypto/dh/dh_key.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -87,6 +87,17 @@ 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;
}

if (BN_num_bits(dh->p) < DH_MIN_MODULUS_BITS) {
DHerr(DH_F_GENERATE_KEY, DH_R_MODULUS_TOO_SMALL);
return 0;
}

ctx = BN_CTX_new();
if (ctx == NULL)
goto err;
Expand Down Expand Up @@ -180,6 +191,17 @@ static int compute_key(unsigned char *key, const BIGNUM *pub_key, DH *dh)
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;
}

if (BN_num_bits(dh->p) < DH_MIN_MODULUS_BITS) {
DHerr(DH_F_COMPUTE_KEY, DH_R_MODULUS_TOO_SMALL);
goto err;
}

ctx = BN_CTX_new();
if (ctx == NULL)
goto err;
Expand Down
2 changes: 2 additions & 0 deletions crypto/dh/dh_local.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,8 @@
#include <openssl/dh.h>
#include "internal/refcount.h"

#define DH_MIN_MODULUS_BITS 512

struct dh_st {
/*
* This first argument is used to pick up errors when a DH is passed
Expand Down
2 changes: 2 additions & 0 deletions crypto/err/openssl.txt
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2466,11 +2466,13 @@ DH_R_KDF_PARAMETER_ERROR:112:kdf parameter error
DH_R_KEYS_NOT_SET:108:keys not set
DH_R_MISSING_PUBKEY:125:missing pubkey
DH_R_MODULUS_TOO_LARGE:103:modulus too large
DH_R_MODULUS_TOO_SMALL:126:modulus too small
DH_R_NOT_SUITABLE_GENERATOR:120:not suitable generator
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
Expand Down
7 changes: 5 additions & 2 deletions include/openssl/dh.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -71,14 +71,17 @@ 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
# define DH_MODULUS_TOO_SMALL 0x80
# define DH_MODULUS_TOO_LARGE 0x100 /* +DH_check_pub_key */


/* DH_check_pub_key error codes */
# define DH_CHECK_PUBKEY_TOO_SMALL 0x01
Expand Down
2 changes: 2 additions & 0 deletions include/openssl/dherr.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -75,11 +75,13 @@ int ERR_load_DH_strings(void);
# define DH_R_KEYS_NOT_SET 108
# define DH_R_MISSING_PUBKEY 125
# define DH_R_MODULUS_TOO_LARGE 103
# define DH_R_MODULUS_TOO_SMALL 126
# define DH_R_NOT_SUITABLE_GENERATOR 120
# define DH_R_NO_PARAMETERS_SET 107
# define DH_R_NO_PRIVATE_VALUE 100
# 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

Expand Down
13 changes: 11 additions & 2 deletions test/dhtest.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -65,12 +65,16 @@ static int dh_test(void)

if (!TEST_true(DH_check(dh, &i)))
goto err2;
i ^= DH_MODULUS_TOO_SMALL;
if (!TEST_false(i & DH_CHECK_P_NOT_PRIME)
|| !TEST_false(i & DH_CHECK_P_NOT_SAFE_PRIME)
|| !TEST_false(i & DH_CHECK_INVALID_Q_VALUE)
|| !TEST_false(i & DH_CHECK_Q_NOT_PRIME)
|| !TEST_false(i & DH_UNABLE_TO_CHECK_GENERATOR)
|| !TEST_false(i & DH_NOT_SUITABLE_GENERATOR)
|| !TEST_false(i & DH_CHECK_Q_NOT_PRIME)
|| !TEST_false(i & DH_CHECK_INVALID_Q_VALUE)
|| !TEST_false(i & DH_CHECK_INVALID_J_VALUE)
|| !TEST_false(i & DH_MODULUS_TOO_SMALL)
|| !TEST_false(i & DH_MODULUS_TOO_LARGE)
|| !TEST_false(i))
goto err2;

Expand Down Expand Up @@ -166,6 +170,11 @@ static int dh_test(void)
|| !TEST_false(i & DH_CHECK_P_NOT_SAFE_PRIME)
|| !TEST_false(i & DH_UNABLE_TO_CHECK_GENERATOR)
|| !TEST_false(i & DH_NOT_SUITABLE_GENERATOR)
|| !TEST_false(i & DH_CHECK_Q_NOT_PRIME)
|| !TEST_false(i & DH_CHECK_INVALID_Q_VALUE)
|| !TEST_false(i & DH_CHECK_INVALID_J_VALUE)
|| !TEST_false(i & DH_MODULUS_TOO_SMALL)
|| !TEST_false(i & DH_MODULUS_TOO_LARGE)
|| !TEST_false(i))
goto err3;

Expand Down

0 comments on commit 1ee30a7

Please sign in to comment.