From 2650515394537ad30110f322e56d3afe710d0886 Mon Sep 17 00:00:00 2001 From: Richard Levitte Date: Fri, 30 Dec 2016 21:57:28 +0100 Subject: [PATCH] Better check of DH parameters in TLS data When the client reads DH parameters from the TLS stream, we only checked that they all are non-zero. This change updates the check to use DH_check_params() DH_check_params() is a new function for light weight checking of the p and g parameters: check that p is odd check that 1 < g < p - 1 Reviewed-by: Viktor Dukhovni --- crypto/dh/dh_check.c | 40 ++++++++++++++++++++++++++++++++++++++++ include/openssl/dh.h | 1 + ssl/statem/statem_clnt.c | 11 ++++++++++- util/libcrypto.num | 1 + 4 files changed, 52 insertions(+), 1 deletion(-) diff --git a/crypto/dh/dh_check.c b/crypto/dh/dh_check.c index b362ccfea1..56894e315b 100644 --- a/crypto/dh/dh_check.c +++ b/crypto/dh/dh_check.c @@ -12,6 +12,46 @@ #include #include "dh_locl.h" +/*- + * Check that p and g are suitable enough + * + * p is odd + * 1 < g < p - 1 + */ + +int DH_check_params(const DH *dh, int *ret) +{ + int ok = 0; + BIGNUM *tmp = NULL; + BN_CTX *ctx = NULL; + + *ret = 0; + ctx = BN_CTX_new(); + if (ctx == NULL) + goto err; + BN_CTX_start(ctx); + tmp = BN_CTX_get(ctx); + if (tmp == NULL) + goto err; + + if (!BN_is_odd(dh->p)) + *ret |= DH_CHECK_P_NOT_PRIME; + if (BN_is_negative(dh->g) || BN_is_zero(dh->g) || BN_is_one(dh->g)) + *ret |= DH_NOT_SUITABLE_GENERATOR; + if (BN_copy(tmp, dh->p) == NULL || !BN_sub_word(tmp, 1)) + goto err; + if (BN_cmp(dh->g, tmp) >= 0) + *ret |= DH_NOT_SUITABLE_GENERATOR; + + ok = 1; + err: + if (ctx != NULL) { + BN_CTX_end(ctx); + BN_CTX_free(ctx); + } + return (ok); +} + /*- * Check that p is a safe prime and * if g is 2, 3 or 5, check that it is a suitable generator diff --git a/include/openssl/dh.h b/include/openssl/dh.h index ae309e7b31..6d149bc932 100644 --- a/include/openssl/dh.h +++ b/include/openssl/dh.h @@ -124,6 +124,7 @@ DEPRECATEDIN_0_9_8(DH *DH_generate_parameters(int prime_len, int generator, int DH_generate_parameters_ex(DH *dh, int prime_len, int generator, BN_GENCB *cb); +int DH_check_params(const DH *dh, int *ret); int DH_check(const DH *dh, int *codes); int DH_check_pub_key(const DH *dh, const BIGNUM *pub_key, int *codes); int DH_generate_key(DH *dh); diff --git a/ssl/statem/statem_clnt.c b/ssl/statem/statem_clnt.c index 6599d432e6..90e4df6fda 100644 --- a/ssl/statem/statem_clnt.c +++ b/ssl/statem/statem_clnt.c @@ -1640,6 +1640,8 @@ static int tls_process_ske_dhe(SSL *s, PACKET *pkt, EVP_PKEY **pkey, int *al) DH *dh = NULL; BIGNUM *p = NULL, *g = NULL, *bnpub_key = NULL; + int check_bits = 0; + if (!PACKET_get_length_prefixed_2(pkt, &prime) || !PACKET_get_length_prefixed_2(pkt, &generator) || !PACKET_get_length_prefixed_2(pkt, &pub_key)) { @@ -1669,7 +1671,8 @@ static int tls_process_ske_dhe(SSL *s, PACKET *pkt, EVP_PKEY **pkey, int *al) goto err; } - if (BN_is_zero(p) || BN_is_zero(g) || BN_is_zero(bnpub_key)) { + /* test non-zero pupkey */ + if (BN_is_zero(bnpub_key)) { *al = SSL_AD_DECODE_ERROR; SSLerr(SSL_F_TLS_PROCESS_SKE_DHE, SSL_R_BAD_DH_VALUE); goto err; @@ -1682,6 +1685,12 @@ static int tls_process_ske_dhe(SSL *s, PACKET *pkt, EVP_PKEY **pkey, int *al) } p = g = NULL; + if (DH_check_params(dh, &check_bits) == 0 || check_bits != 0) { + *al = SSL_AD_DECODE_ERROR; + SSLerr(SSL_F_TLS_PROCESS_SKE_DHE, SSL_R_BAD_DH_VALUE); + goto err; + } + if (!DH_set0_key(dh, bnpub_key, NULL)) { *al = SSL_AD_INTERNAL_ERROR; SSLerr(SSL_F_TLS_PROCESS_SKE_DHE, ERR_R_BN_LIB); diff --git a/util/libcrypto.num b/util/libcrypto.num index 917ab888a7..8e9b752940 100644 --- a/util/libcrypto.num +++ b/util/libcrypto.num @@ -4229,3 +4229,4 @@ UI_method_get_ex_data 4179 1_1_1 EXIST::FUNCTION:UI UI_UTIL_wrap_read_pem_callback 4180 1_1_1 EXIST::FUNCTION:UI X509_VERIFY_PARAM_get_time 4181 1_1_0d EXIST::FUNCTION: EVP_PKEY_get0_poly1305 4182 1_1_1 EXIST::FUNCTION:POLY1305 +DH_check_params 4183 1_1_0d EXIST::FUNCTION:DH -- 2.25.1