Consistently reject large p and large q in DH

When applications use Diffie-Hellman incorrectly, and use
attacker-supplied domain parameters, rather than known-valid ones (as
required by SP 800-56A, 5.5.2), algorithms that aren't designed with
attacker-supplied parameters in mind become attack surfaces.

CVE-2023-3446 and CVE-2023-3817 in OpenSSL cover problems with the
DH_check function given large p and large q. This CL adds some fast
validity checks to the DH parameters before running any operation. This
differs from upstream in a few ways:

- Upstream only addressed issues with DH_check. We also check in
  DH_generate_key and DH_check_pub_key.

- For a more consistent invariant, reuse the existing DH modulus limit.
  Ideally we'd enforce these invariants on DH creation, but this is not
  possible due to OpenSSL's API. We additionally check some other
  cheap invariants.

This does not impact TLS, or any applications that used Diffie-Hellman
correctly, with trusted, well-known domain parameters.

Ultimately, that this comes up at all is a flaw in how DH was specified.
This is analogous to the issues with ECC with arbitrary groups and DSA,
which led to https://github.com/openssl/openssl/issues/20268
CVE-2022-0778, CVE-2020-0601, and likely others. Cryptographic
primitives should be limited to a small set of named, well-known domain
parameters.

Update-Note: Egregiously large or invalid DH p, q, or g values will be
more consistently rejected in DH operations. This does not impact TLS.
Applications should switch to modern primitives such as X25519 or ECDH
with P-256.

Change-Id: I666fe0b9f8b71632f6cf8064c8ea0251e5c286bb
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/62226
Reviewed-by: Adam Langley <agl@google.com>
Commit-Queue: David Benjamin <davidben@google.com>

crypto/dh_extra/dh_asn1.c

@@ -110,6 +110,10 @@ DH *DH_parse_parameters(CBS *cbs) {
     goto err;
   }
 
+  if (!dh_check_params_fast(ret)) {
+    goto err;
+  }
+
   return ret;
 
 err:

crypto/dh_extra/dh_test.cc

@@ -71,7 +71,6 @@
 #include <openssl/mem.h>
 
 #include "../fipsmodule/dh/internal.h"
-#include "../internal.h"
 #include "../test/test_util.h"
 
 
@@ -195,15 +194,35 @@ static const uint8_t kRFC5114_2048_224BadY[] = {
     0x93, 0x74, 0x89, 0x59,
 };
 
-TEST(DHTest, BadY) {
+static bssl::UniquePtr<DH> NewDHGroup(const BIGNUM *p, const BIGNUM *q,
+                                      const BIGNUM *g) {
+  bssl::UniquePtr<BIGNUM> p_copy(BN_dup(p));
+  bssl::UniquePtr<BIGNUM> q_copy(q != nullptr ? BN_dup(q) : nullptr);
+  bssl::UniquePtr<BIGNUM> g_copy(BN_dup(g));
   bssl::UniquePtr<DH> dh(DH_new());
+  if (p_copy == nullptr || (q != nullptr && q_copy == nullptr) ||
+      g_copy == nullptr || dh == nullptr ||
+      !DH_set0_pqg(dh.get(), p_copy.get(), q_copy.get(), g_copy.get())) {
+    return nullptr;
+  }
+  p_copy.release();
+  q_copy.release();
+  g_copy.release();
+  return dh;
+}
+
+TEST(DHTest, BadY) {
+  bssl::UniquePtr<BIGNUM> p(
+      BN_bin2bn(kRFC5114_2048_224P, sizeof(kRFC5114_2048_224P), nullptr));
+  bssl::UniquePtr<BIGNUM> q(
+      BN_bin2bn(kRFC5114_2048_224Q, sizeof(kRFC5114_2048_224Q), nullptr));
+  bssl::UniquePtr<BIGNUM> g(
+      BN_bin2bn(kRFC5114_2048_224G, sizeof(kRFC5114_2048_224G), nullptr));
+  ASSERT_TRUE(p);
+  ASSERT_TRUE(q);
+  ASSERT_TRUE(g);
+  bssl::UniquePtr<DH> dh = NewDHGroup(p.get(), q.get(), g.get());
   ASSERT_TRUE(dh);
-  dh->p = BN_bin2bn(kRFC5114_2048_224P, sizeof(kRFC5114_2048_224P), nullptr);
-  dh->g = BN_bin2bn(kRFC5114_2048_224G, sizeof(kRFC5114_2048_224G), nullptr);
-  dh->q = BN_bin2bn(kRFC5114_2048_224Q, sizeof(kRFC5114_2048_224Q), nullptr);
-  ASSERT_TRUE(dh->p);
-  ASSERT_TRUE(dh->g);
-  ASSERT_TRUE(dh->q);
 
   bssl::UniquePtr<BIGNUM> pub_key(
       BN_bin2bn(kRFC5114_2048_224BadY, sizeof(kRFC5114_2048_224BadY), nullptr));
@@ -336,11 +355,8 @@ TEST(DHTest, LeadingZeros) {
   ASSERT_TRUE(g);
   ASSERT_TRUE(BN_set_word(g.get(), 2));
 
-  bssl::UniquePtr<DH> dh(DH_new());
+  bssl::UniquePtr<DH> dh = NewDHGroup(p.get(), /*q=*/nullptr, g.get());
   ASSERT_TRUE(dh);
-  ASSERT_TRUE(DH_set0_pqg(dh.get(), p.get(), /*q=*/nullptr, g.get()));
-  p.release();
-  g.release();
 
   // These values are far too small to be reasonable Diffie-Hellman keys, but
   // they are an easy way to get a shared secret with leading zeros.
@@ -375,11 +391,8 @@ TEST(DHTest, Overwrite) {
   ASSERT_TRUE(g);
   ASSERT_TRUE(BN_set_word(g.get(), 2));
 
-  bssl::UniquePtr<DH> key1(DH_new());
+  bssl::UniquePtr<DH> key1 = NewDHGroup(p.get(), /*q=*/nullptr, g.get());
   ASSERT_TRUE(key1);
-  ASSERT_TRUE(DH_set0_pqg(key1.get(), p.get(), /*q=*/nullptr, g.get()));
-  p.release();
-  g.release();
   ASSERT_TRUE(DH_generate_key(key1.get()));
 
   bssl::UniquePtr<BIGNUM> peer_key(BN_new());
@@ -393,15 +406,8 @@ TEST(DHTest, Overwrite) {
   // Generate a different key with a different group.
   p.reset(BN_get_rfc3526_prime_2048(nullptr));
   ASSERT_TRUE(p);
-  g.reset(BN_new());
-  ASSERT_TRUE(g);
-  ASSERT_TRUE(BN_set_word(g.get(), 2));
-
-  bssl::UniquePtr<DH> key2(DH_new());
+  bssl::UniquePtr<DH> key2 = NewDHGroup(p.get(), /*q=*/nullptr, g.get());
   ASSERT_TRUE(key2);
-  ASSERT_TRUE(DH_set0_pqg(key2.get(), p.get(), /*q=*/nullptr, g.get()));
-  p.release();
-  g.release();
   ASSERT_TRUE(DH_generate_key(key2.get()));
 
   // Overwrite |key1|'s contents with |key2|.
@@ -434,11 +440,8 @@ TEST(DHTest, GenerateKeyTwice) {
   bssl::UniquePtr<BIGNUM> g(BN_new());
   ASSERT_TRUE(g);
   ASSERT_TRUE(BN_set_word(g.get(), 2));
-  bssl::UniquePtr<DH> key1(DH_new());
+  bssl::UniquePtr<DH> key1 = NewDHGroup(p.get(), /*q=*/nullptr, g.get());
   ASSERT_TRUE(key1);
-  ASSERT_TRUE(DH_set0_pqg(key1.get(), p.get(), /*q=*/nullptr, g.get()));
-  p.release();
-  g.release();
   ASSERT_TRUE(DH_generate_key(key1.get()));
 
   // Copy the parameters and private key to a new DH object.
@@ -456,3 +459,65 @@ TEST(DHTest, GenerateKeyTwice) {
   EXPECT_EQ(BN_cmp(DH_get0_pub_key(key1.get()), DH_get0_pub_key(key2.get())),
             0);
 }
+
+// Bad parameters should be rejected, rather than cause a DoS risk in the
+// event that an application uses Diffie-Hellman incorrectly, with untrusted
+// domain parameters.
+TEST(DHTest, InvalidParameters) {
+  auto check_invalid_group = [](DH *dh) {
+    // All operations on egregiously invalid groups should fail.
+    EXPECT_FALSE(DH_generate_key(dh));
+    int check_result;
+    EXPECT_FALSE(DH_check(dh, &check_result));
+    bssl::UniquePtr<BIGNUM> pub_key(BN_new());
+    ASSERT_TRUE(pub_key);
+    ASSERT_TRUE(BN_set_u64(pub_key.get(), 42));
+    EXPECT_FALSE(DH_check_pub_key(dh, pub_key.get(), &check_result));
+    uint8_t buf[1024];
+    EXPECT_EQ(DH_compute_key(buf, pub_key.get(), dh), -1);
+    EXPECT_EQ(DH_compute_key_padded(buf, pub_key.get(), dh), -1);
+  };
+
+  bssl::UniquePtr<BIGNUM> p(BN_get_rfc3526_prime_2048(nullptr));
+  ASSERT_TRUE(p);
+  bssl::UniquePtr<BIGNUM> g(BN_new());
+  ASSERT_TRUE(g);
+  ASSERT_TRUE(BN_set_word(g.get(), 2));
+
+  // p is negative.
+  BN_set_negative(p.get(), 1);
+  bssl::UniquePtr<DH> dh = NewDHGroup(p.get(), /*q=*/nullptr, g.get());
+  ASSERT_TRUE(dh);
+  BN_set_negative(p.get(), 0);
+  check_invalid_group(dh.get());
+
+  // g is negative.
+  BN_set_negative(g.get(), 1);
+  dh = NewDHGroup(p.get(), /*q=*/nullptr, g.get());
+  ASSERT_TRUE(dh);
+  BN_set_negative(g.get(), 0);
+  check_invalid_group(dh.get());
+
+  // g is not reduced mod p.
+  dh = NewDHGroup(p.get(), /*q=*/nullptr, p.get());
+  ASSERT_TRUE(dh);
+  BN_set_negative(g.get(), 0);
+  check_invalid_group(dh.get());
+
+  // p is too large.
+  bssl::UniquePtr<BIGNUM> large(BN_new());
+  ASSERT_TRUE(BN_set_bit(large.get(), 0));
+  ASSERT_TRUE(BN_set_bit(large.get(), 10000000));
+  dh = NewDHGroup(large.get(), /*q=*/nullptr, g.get());
+  ASSERT_TRUE(dh);
+  check_invalid_group(dh.get());
+
+  // q is too large.
+  dh = NewDHGroup(p.get(), large.get(), g.get());
+  ASSERT_TRUE(dh);
+  check_invalid_group(dh.get());
+
+  // Attempting to generate too large of a Diffie-Hellman group should fail.
+  EXPECT_FALSE(
+      DH_generate_parameters_ex(dh.get(), 20000, DH_GENERATOR_5, nullptr));
+}

crypto/dh_extra/params.c

@@ -337,6 +337,11 @@ int DH_generate_parameters_ex(DH *dh, int prime_bits, int generator,
   // It's just as OK (and in some sense better) to use a generator of the
   // order-q subgroup.
 
+  if (prime_bits <= 0 || prime_bits > OPENSSL_DH_MAX_MODULUS_BITS) {
+    OPENSSL_PUT_ERROR(DH, DH_R_MODULUS_TOO_LARGE);
+    return 0;
+  }
+
   BIGNUM *t1, *t2;
   int g, ok = 0;
   BN_CTX *ctx = NULL;

crypto/err/dh.errordata

@@ -1,6 +1,7 @@
 DH,100,BAD_GENERATOR
 DH,104,DECODE_ERROR
 DH,105,ENCODE_ERROR
+DH,106,INVALID_PARAMETERS
 DH,101,INVALID_PUBKEY
 DH,102,MODULUS_TOO_LARGE
 DH,103,NO_PRIVATE_VALUE

crypto/fipsmodule/dh/check.c

@@ -57,12 +57,40 @@
 #include <openssl/dh.h>
 
 #include <openssl/bn.h>
+#include <openssl/err.h>
 
 #include "internal.h"
 
 
+int dh_check_params_fast(const DH *dh) {
+  // Most operations scale with p and q.
+  if (BN_is_negative(dh->p) || !BN_is_odd(dh->p) ||
+      BN_num_bits(dh->p) > OPENSSL_DH_MAX_MODULUS_BITS) {
+    OPENSSL_PUT_ERROR(DH, DH_R_INVALID_PARAMETERS);
+    return 0;
+  }
+
+  // q must be bounded by p.
+  if (dh->q != NULL && (BN_is_negative(dh->q) || BN_ucmp(dh->q, dh->p) > 0)) {
+    OPENSSL_PUT_ERROR(DH, DH_R_INVALID_PARAMETERS);
+    return 0;
+  }
+
+  // g must be an element of p's multiplicative group.
+  if (BN_is_negative(dh->g) || BN_is_zero(dh->g) ||
+      BN_ucmp(dh->g, dh->p) >= 0) {
+    OPENSSL_PUT_ERROR(DH, DH_R_INVALID_PARAMETERS);
+    return 0;
+  }
+
+  return 1;
+}
+
 int DH_check_pub_key(const DH *dh, const BIGNUM *pub_key, int *out_flags) {
   *out_flags = 0;
+  if (!dh_check_params_fast(dh)) {
+    return 0;
+  }
 
   BN_CTX *ctx = BN_CTX_new();
   if (ctx == NULL) {
@@ -73,17 +101,14 @@ int DH_check_pub_key(const DH *dh, const BIGNUM *pub_key, int *out_flags) {
   int ok = 0;
 
   // Check |pub_key| is greater than 1.
-  BIGNUM *tmp = BN_CTX_get(ctx);
-  if (tmp == NULL ||
-      !BN_set_word(tmp, 1)) {
-    goto err;
-  }
-  if (BN_cmp(pub_key, tmp) <= 0) {
+  if (BN_cmp(pub_key, BN_value_one()) <= 0) {
     *out_flags |= DH_CHECK_PUBKEY_TOO_SMALL;
   }
 
   // Check |pub_key| is less than |dh->p| - 1.
-  if (!BN_copy(tmp, dh->p) ||
+  BIGNUM *tmp = BN_CTX_get(ctx);
+  if (tmp == NULL ||
+      !BN_copy(tmp, dh->p) ||
       !BN_sub_word(tmp, 1)) {
     goto err;
   }
@@ -113,6 +138,11 @@ err:
 
 
 int DH_check(const DH *dh, int *out_flags) {
+  *out_flags = 0;
+  if (!dh_check_params_fast(dh)) {
+    return 0;
+  }
+
   // Check that p is a safe prime and if g is 2, 3 or 5, check that it is a
   // suitable generator where:
   //   for 2, p mod 24 == 11
@@ -124,7 +154,6 @@ int DH_check(const DH *dh, int *out_flags) {
   BN_ULONG l;
   BIGNUM *t1 = NULL, *t2 = NULL;
 
-  *out_flags = 0;
   ctx = BN_CTX_new();
   if (ctx == NULL) {
     goto err;

crypto/fipsmodule/dh/dh.c

@@ -70,8 +70,6 @@
 #include "internal.h"
 
 
-#define OPENSSL_DH_MAX_MODULUS_BITS 10000
-
 DH *DH_new(void) {
   DH *dh = OPENSSL_malloc(sizeof(DH));
   if (dh == NULL) {
@@ -191,16 +189,15 @@ int DH_set_length(DH *dh, unsigned priv_length) {
 int DH_generate_key(DH *dh) {
   boringssl_ensure_ffdh_self_test();
 
+  if (!dh_check_params_fast(dh)) {
+    return 0;
+  }
+
   int ok = 0;
   int generate_new_key = 0;
   BN_CTX *ctx = NULL;
   BIGNUM *pub_key = NULL, *priv_key = NULL;
 
-  if (BN_num_bits(dh->p) > OPENSSL_DH_MAX_MODULUS_BITS) {
-    OPENSSL_PUT_ERROR(DH, DH_R_MODULUS_TOO_LARGE);
-    goto err;
-  }
-
   ctx = BN_CTX_new();
   if (ctx == NULL) {
     goto err;
@@ -279,8 +276,7 @@ err:
 
 static int dh_compute_key(DH *dh, BIGNUM *out_shared_key,
                           const BIGNUM *peers_key, BN_CTX *ctx) {
-  if (BN_num_bits(dh->p) > OPENSSL_DH_MAX_MODULUS_BITS) {
-    OPENSSL_PUT_ERROR(DH, DH_R_MODULUS_TOO_LARGE);
+  if (!dh_check_params_fast(dh)) {
     return 0;
   }
 

crypto/fipsmodule/dh/internal.h

@@ -26,6 +26,8 @@ extern "C" {
 #endif
 
 
+#define OPENSSL_DH_MAX_MODULUS_BITS 10000
+
 struct dh_st {
   BIGNUM *p;
   BIGNUM *g;
@@ -44,6 +46,11 @@ struct dh_st {
   CRYPTO_refcount_t references;
 };
 
+// dh_check_params_fast checks basic invariants on |dh|'s domain parameters. It
+// does not check that |dh| forms a valid group, only that the sizes are within
+// DoS bounds.
+int dh_check_params_fast(const DH *dh);
+
 // dh_compute_key_padded_no_self_test does the same as |DH_compute_key_padded|,
 // but doesn't try to run the self-test first. This is for use in the self tests
 // themselves, to prevent an infinite loop.

include/openssl/dh.h

@@ -353,5 +353,6 @@ BSSL_NAMESPACE_END
 #define DH_R_NO_PRIVATE_VALUE 103
 #define DH_R_DECODE_ERROR 104
 #define DH_R_ENCODE_ERROR 105
+#define DH_R_INVALID_PARAMETERS 106
 
 #endif  // OPENSSL_HEADER_DH_H