From 4c2818ea83f5f96c552a43ee6c72a10568c2243f Mon Sep 17 00:00:00 2001
From: Sean McGrail <549813+skmcgrail@users.noreply.github.com>
Date: Wed, 2 Aug 2023 07:48:20 -0700
Subject: [PATCH] [fips-2021-10-20] Backport CVE-2023-3446, CVE-2023-3817 fixes
 for DH_check (#1127)

* Fix DH_check() excessive time with oversized modulus (#1109)

(cherry picked from commit 9545d9de6059a94a7fd0e49a39b32905a7dd2f74)

* Fix Excessive time spent checking DH q parameter value (#1121)

(cherry picked from commit 1bb574f3f2e758124b2a7acac6550ec0c63d9970)

* Add GitHub CODEOWNERS
---
 .github/CODEOWNERS           |  2 ++
 crypto/dh_extra/dh_test.cc   | 48 ++++++++++++++++++++++++++++++++++++
 crypto/fipsmodule/dh/check.c | 18 +++++++++++++-
 3 files changed, 67 insertions(+), 1 deletion(-)
 create mode 100644 .github/CODEOWNERS

diff --git a/.github/CODEOWNERS b/.github/CODEOWNERS
new file mode 100644
index 0000000000..91c2eec81f
--- /dev/null
+++ b/.github/CODEOWNERS
@@ -0,0 +1,2 @@
+# Add core contributors to all PRs by default
+* @aws/aws-lc-dev @aws/aws-lc-fips-dev
diff --git a/crypto/dh_extra/dh_test.cc b/crypto/dh_extra/dh_test.cc
index 7933a8cc5a..a81aa834cd 100644
--- a/crypto/dh_extra/dh_test.cc
+++ b/crypto/dh_extra/dh_test.cc
@@ -202,6 +202,54 @@ static bool RunBasicTests() {
   return true;
 }
 
+TEST(DHTest, OversizedModulus) {
+  bssl::UniquePtr<DH> a(DH_new());
+  ASSERT_TRUE(a);
+
+  const size_t LARGE_MOD_P = 4097;  // OPENSSL_DH_CHECK_MAX_MODULUS_BITS / 8 + 1
+
+  // Create a BigNumber which will be interpreted as a big-endian value
+  auto number = std::unique_ptr<uint8_t[], std::default_delete<uint8_t[]>>(
+      new uint8_t[LARGE_MOD_P]);
+  for (size_t i = 0; i < LARGE_MOD_P; i++) {
+    number[i] = 255;
+  }
+
+  bssl::UniquePtr<BIGNUM> p(BN_bin2bn(number.get(), LARGE_MOD_P, nullptr));
+  bssl::UniquePtr<BIGNUM> q(BN_new());
+  bssl::UniquePtr<BIGNUM> g(BN_new());
+
+  // Q and G don't matter for this test, they just can't be null
+  ASSERT_TRUE(DH_set0_pqg(a.get(), p.release(), q.release(), g.release()));
+
+  int check_result;
+  ASSERT_FALSE(DH_check(a.get(), &check_result));
+  uint32_t error = ERR_get_error();
+  ASSERT_EQ(ERR_LIB_DH, ERR_GET_LIB(error));
+  ASSERT_EQ(DH_R_MODULUS_TOO_LARGE, ERR_GET_REASON(error));
+}
+
+TEST(DHTest, LargeQ) {
+  bssl::UniquePtr<DH> a(DH_new());
+  ASSERT_TRUE(a);
+  ASSERT_TRUE(DH_generate_parameters_ex(a.get(), 64, DH_GENERATOR_5, nullptr));
+
+  bssl::UniquePtr<BIGNUM> q(BN_new());
+  ASSERT_TRUE(q);
+  BN_set_word(q.get(), 2039L);
+
+  a.get()->q = q.release();
+
+  ASSERT_TRUE(DH_generate_key(a.get()));
+
+  ASSERT_TRUE(BN_copy(a.get()->q, a.get()->p));
+  ASSERT_TRUE(BN_add(a.get()->q, a.get()->q, BN_value_one()));
+
+  int check_result;
+  ASSERT_TRUE(DH_check(a.get(), &check_result));
+  ASSERT_TRUE(check_result & DH_CHECK_INVALID_Q_VALUE);
+}
+
 // The following parameters are taken from RFC 5114, section 2.2. This is not a
 // safe prime. Do not use these parameters.
 static const uint8_t kRFC5114_2048_224P[] = {
diff --git a/crypto/fipsmodule/dh/check.c b/crypto/fipsmodule/dh/check.c
index 5b6e03a517..5135d1c272 100644
--- a/crypto/fipsmodule/dh/check.c
+++ b/crypto/fipsmodule/dh/check.c
@@ -58,6 +58,7 @@
 
 #include <openssl/bn.h>
 
+#define OPENSSL_DH_CHECK_MAX_MODULUS_BITS  32768
 
 int DH_check_pub_key(const DH *dh, const BIGNUM *pub_key, int *out_flags) {
   *out_flags = 0;
@@ -117,12 +118,19 @@ int DH_check(const DH *dh, int *out_flags) {
   //   for 3, p mod 12 == 5
   //   for 5, p mod 10 == 3 or 7
   // should hold.
-  int ok = 0, r;
+  int ok = 0, r, q_good = 0;
   BN_CTX *ctx = NULL;
   BN_ULONG l;
   BIGNUM *t1 = NULL, *t2 = NULL;
 
   *out_flags = 0;
+
+  /* Don't do any checks at all with an excessively large modulus */
+  if (BN_num_bits(dh->p) > OPENSSL_DH_CHECK_MAX_MODULUS_BITS) {
+    OPENSSL_PUT_ERROR(DH, DH_R_MODULUS_TOO_LARGE);
+    return 0;
+  }
+
   ctx = BN_CTX_new();
   if (ctx == NULL) {
     goto err;
@@ -138,6 +146,14 @@ int DH_check(const DH *dh, int *out_flags) {
   }
 
   if (dh->q) {
+    if (BN_ucmp(dh->p, dh->q) > 0) {
+      q_good = 1;
+    } else {
+      *out_flags |= DH_CHECK_INVALID_Q_VALUE;
+    }
+  }
+
+  if (q_good) {
     if (BN_cmp(dh->g, BN_value_one()) <= 0) {
       *out_flags |= DH_CHECK_NOT_SUITABLE_GENERATOR;
     } else if (BN_cmp(dh->g, dh->p) >= 0) {