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 <viktor@openssl.org>

4 files changed, 52 insertions, 1 deletions

diff --git a/crypto/dh/dh_check.c b/crypto/dh/dh_check.c
index fcc1d99ad7..3b0fa5903e 100644
--- a/crypto/dh/dh_check.c
+++ b/crypto/dh/dh_check.c
@@ -13,6 +13,46 @@
 #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
  * where
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 a7cf227ce4..dc6443ddaf 100644
--- a/ssl/statem/statem_clnt.c
+++ b/ssl/statem/statem_clnt.c
@@ -1414,6 +1414,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)) {
@@ -1441,7 +1443,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;
@@ -1454,6 +1457,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 b0de30af0e..c1a09455ef 100644
--- a/util/libcrypto.num
+++ b/util/libcrypto.num
@@ -4213,3 +4213,4 @@ CT_POLICY_EVAL_CTX_set_time             4173	1_1_0d	EXIST::FUNCTION:CT
 X509_VERIFY_PARAM_set_inh_flags         4174	1_1_0d	EXIST::FUNCTION:
 X509_VERIFY_PARAM_get_inh_flags         4175	1_1_0d	EXIST::FUNCTION:
 X509_VERIFY_PARAM_get_time              4181	1_1_0d	EXIST::FUNCTION:
+DH_check_params                         4183	1_1_0d	EXIST::FUNCTION:DH