|  | 
|  | 1 | +Backport of: | 
|  | 2 | + | 
|  | 3 | +From db925ae2e65d0d925adef429afc37f75bd1c2017 Mon Sep 17 00:00:00 2001 | 
|  | 4 | +From: Richard Levitte <[email protected] > | 
|  | 5 | +Date: Fri, 20 Oct 2023 09:18:19 +0200 | 
|  | 6 | +Subject: [PATCH] Make DH_check_pub_key() and DH_generate_key() safer yet | 
|  | 7 | + | 
|  | 8 | +We already check for an excessively large P in DH_generate_key(), but not in | 
|  | 9 | +DH_check_pub_key(), and none of them check for an excessively large Q. | 
|  | 10 | + | 
|  | 11 | +This change adds all the missing excessive size checks of P and Q. | 
|  | 12 | + | 
|  | 13 | +It's to be noted that behaviours surrounding excessively sized P and Q | 
|  | 14 | +differ.  DH_check() raises an error on the excessively sized P, but only | 
|  | 15 | +sets a flag for the excessively sized Q.  This behaviour is mimicked in | 
|  | 16 | +DH_check_pub_key(). | 
|  | 17 | + | 
|  | 18 | +Reviewed-by: Tomas Mraz <[email protected] > | 
|  | 19 | +Reviewed-by: Matt Caswell <[email protected] > | 
|  | 20 | +Reviewed-by: Hugo Landau <[email protected] > | 
|  | 21 | +(Merged from https://github.com/openssl/openssl/pull/22518) | 
|  | 22 | + | 
|  | 23 | +(cherry picked from commit ddeb4b6c6d527e54ce9a99cba785c0f7776e54b6) | 
|  | 24 | +--- | 
|  | 25 | + crypto/dh/dh_check.c    | 12 ++++++++++++ | 
|  | 26 | + crypto/dh/dh_err.c      |  3 ++- | 
|  | 27 | + crypto/dh/dh_key.c      | 12 ++++++++++++ | 
|  | 28 | + crypto/err/openssl.txt  |  1 + | 
|  | 29 | + include/crypto/dherr.h  |  2 +- | 
|  | 30 | + include/openssl/dh.h    |  6 +++--- | 
|  | 31 | + include/openssl/dherr.h |  3 ++- | 
|  | 32 | + 7 files changed, 33 insertions(+), 6 deletions(-) | 
|  | 33 | + | 
|  | 34 | +--- a/crypto/dh/dh_check.c | 
|  | 35 | ++++ b/crypto/dh/dh_check.c | 
|  | 36 | +@@ -201,6 +201,19 @@ int DH_check_pub_key(const DH *dh, const | 
|  | 37 | +     if (ctx == NULL) | 
|  | 38 | +         goto err; | 
|  | 39 | +     BN_CTX_start(ctx); | 
|  | 40 | ++ | 
|  | 41 | ++    /* Don't do any checks at all with an excessively large modulus */ | 
|  | 42 | ++    if (BN_num_bits(dh->p) > OPENSSL_DH_CHECK_MAX_MODULUS_BITS) { | 
|  | 43 | ++        DHerr(DH_F_DH_CHECK, DH_R_MODULUS_TOO_LARGE); | 
|  | 44 | ++        *ret = DH_MODULUS_TOO_LARGE | DH_CHECK_PUBKEY_INVALID; | 
|  | 45 | ++        goto err; | 
|  | 46 | ++    } | 
|  | 47 | ++ | 
|  | 48 | ++    if (dh->q != NULL && BN_ucmp(dh->p, dh->q) < 0) { | 
|  | 49 | ++        *ret |= DH_CHECK_INVALID_Q_VALUE | DH_CHECK_PUBKEY_INVALID; | 
|  | 50 | ++        goto out; | 
|  | 51 | ++    } | 
|  | 52 | ++ | 
|  | 53 | +     tmp = BN_CTX_get(ctx); | 
|  | 54 | +     if (tmp == NULL || !BN_set_word(tmp, 1)) | 
|  | 55 | +         goto err; | 
|  | 56 | +@@ -219,6 +232,7 @@ int DH_check_pub_key(const DH *dh, const | 
|  | 57 | +             *ret |= DH_CHECK_PUBKEY_INVALID; | 
|  | 58 | +     } | 
|  | 59 | +  | 
|  | 60 | ++ out: | 
|  | 61 | +     ok = 1; | 
|  | 62 | +  err: | 
|  | 63 | +     BN_CTX_end(ctx); | 
|  | 64 | +--- a/crypto/dh/dh_err.c | 
|  | 65 | ++++ b/crypto/dh/dh_err.c | 
|  | 66 | +@@ -82,6 +82,7 @@ static const ERR_STRING_DATA DH_str_reas | 
|  | 67 | +     {ERR_PACK(ERR_LIB_DH, 0, DH_R_PARAMETER_ENCODING_ERROR), | 
|  | 68 | +     "parameter encoding error"}, | 
|  | 69 | +     {ERR_PACK(ERR_LIB_DH, 0, DH_R_PEER_KEY_ERROR), "peer key error"}, | 
|  | 70 | ++    {ERR_PACK(ERR_LIB_DH, 0, DH_R_Q_TOO_LARGE), "q too large"}, | 
|  | 71 | +     {ERR_PACK(ERR_LIB_DH, 0, DH_R_SHARED_INFO_ERROR), "shared info error"}, | 
|  | 72 | +     {ERR_PACK(ERR_LIB_DH, 0, DH_R_UNABLE_TO_CHECK_GENERATOR), | 
|  | 73 | +     "unable to check generator"}, | 
|  | 74 | +--- a/crypto/dh/dh_key.c | 
|  | 75 | ++++ b/crypto/dh/dh_key.c | 
|  | 76 | +@@ -87,6 +87,12 @@ static int generate_key(DH *dh) | 
|  | 77 | +         return 0; | 
|  | 78 | +     } | 
|  | 79 | +  | 
|  | 80 | ++    if (dh->q != NULL | 
|  | 81 | ++        && BN_num_bits(dh->q) > OPENSSL_DH_MAX_MODULUS_BITS) { | 
|  | 82 | ++        DHerr(DH_F_GENERATE_KEY, DH_R_Q_TOO_LARGE); | 
|  | 83 | ++        return 0; | 
|  | 84 | ++    } | 
|  | 85 | ++ | 
|  | 86 | +     ctx = BN_CTX_new(); | 
|  | 87 | +     if (ctx == NULL) | 
|  | 88 | +         goto err; | 
|  | 89 | +@@ -180,6 +186,12 @@ static int compute_key(unsigned char *ke | 
|  | 90 | +         goto err; | 
|  | 91 | +     } | 
|  | 92 | +  | 
|  | 93 | ++    if (dh->q != NULL | 
|  | 94 | ++        && BN_num_bits(dh->q) > OPENSSL_DH_MAX_MODULUS_BITS) { | 
|  | 95 | ++        DHerr(DH_F_COMPUTE_KEY, DH_R_Q_TOO_LARGE); | 
|  | 96 | ++        goto err; | 
|  | 97 | ++    } | 
|  | 98 | ++ | 
|  | 99 | +     ctx = BN_CTX_new(); | 
|  | 100 | +     if (ctx == NULL) | 
|  | 101 | +         goto err; | 
|  | 102 | +--- a/crypto/err/openssl.txt | 
|  | 103 | ++++ b/crypto/err/openssl.txt | 
|  | 104 | +@@ -2110,6 +2110,7 @@ DH_R_NO_PARAMETERS_SET:107:no parameters | 
|  | 105 | + DH_R_NO_PRIVATE_VALUE:100:no private value | 
|  | 106 | + DH_R_PARAMETER_ENCODING_ERROR:105:parameter encoding error | 
|  | 107 | + DH_R_PEER_KEY_ERROR:111:peer key error | 
|  | 108 | ++DH_R_Q_TOO_LARGE:130:q too large | 
|  | 109 | + DH_R_SHARED_INFO_ERROR:113:shared info error | 
|  | 110 | + DH_R_UNABLE_TO_CHECK_GENERATOR:121:unable to check generator | 
|  | 111 | + DSA_R_BAD_Q_VALUE:102:bad q value | 
|  | 112 | +--- a/include/openssl/dh.h | 
|  | 113 | ++++ b/include/openssl/dh.h | 
|  | 114 | +@@ -71,14 +71,16 @@ DECLARE_ASN1_ITEM(DHparams) | 
|  | 115 | + /* #define DH_GENERATOR_3       3 */ | 
|  | 116 | + # define DH_GENERATOR_5          5 | 
|  | 117 | +  | 
|  | 118 | +-/* DH_check error codes */ | 
|  | 119 | ++/* DH_check error codes, some of them shared with DH_check_pub_key */ | 
|  | 120 | + # define DH_CHECK_P_NOT_PRIME            0x01 | 
|  | 121 | + # define DH_CHECK_P_NOT_SAFE_PRIME       0x02 | 
|  | 122 | + # define DH_UNABLE_TO_CHECK_GENERATOR    0x04 | 
|  | 123 | + # define DH_NOT_SUITABLE_GENERATOR       0x08 | 
|  | 124 | + # define DH_CHECK_Q_NOT_PRIME            0x10 | 
|  | 125 | +-# define DH_CHECK_INVALID_Q_VALUE        0x20 | 
|  | 126 | ++# define DH_CHECK_INVALID_Q_VALUE        0x20 /* +DH_check_pub_key */ | 
|  | 127 | + # define DH_CHECK_INVALID_J_VALUE        0x40 | 
|  | 128 | ++# define DH_MODULUS_TOO_SMALL            0x80 | 
|  | 129 | ++# define DH_MODULUS_TOO_LARGE            0x100 /* +DH_check_pub_key */ | 
|  | 130 | +  | 
|  | 131 | + /* DH_check_pub_key error codes */ | 
|  | 132 | + # define DH_CHECK_PUBKEY_TOO_SMALL       0x01 | 
|  | 133 | +--- a/include/openssl/dherr.h | 
|  | 134 | ++++ b/include/openssl/dherr.h | 
|  | 135 | +@@ -82,6 +82,7 @@ int ERR_load_DH_strings(void); | 
|  | 136 | + #  define DH_R_NO_PRIVATE_VALUE                            100 | 
|  | 137 | + #  define DH_R_PARAMETER_ENCODING_ERROR                    105 | 
|  | 138 | + #  define DH_R_PEER_KEY_ERROR                              111 | 
|  | 139 | ++#  define DH_R_Q_TOO_LARGE                                 130 | 
|  | 140 | + #  define DH_R_SHARED_INFO_ERROR                           113 | 
|  | 141 | + #  define DH_R_UNABLE_TO_CHECK_GENERATOR                   121 | 
|  | 142 | +  | 
0 commit comments