Better check of DH parameters in TLS data
authorRichard Levitte <levitte@openssl.org>
Fri, 30 Dec 2016 20:57:28 +0000 (21:57 +0100)
committerMatt Caswell <matt@openssl.org>
Thu, 26 Jan 2017 10:54:01 +0000 (10:54 +0000)
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>
crypto/dh/dh_check.c
include/openssl/dh.h
ssl/statem/statem_clnt.c
util/libcrypto.num

index b362ccf..56894e3 100644 (file)
 #include <openssl/bn.h>
 #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
index ae309e7..6d149bc 100644 (file)
@@ -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);
index 6599d43..90e4df6 100644 (file)
@@ -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);
index 917ab88..8e9b752 100644 (file)
@@ -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