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>
diff --git a/crypto/dh_extra/dh_asn1.c b/crypto/dh_extra/dh_asn1.c
index de01077..4e2e2c4 100644
--- a/crypto/dh_extra/dh_asn1.c
+++ b/crypto/dh_extra/dh_asn1.c
@@ -110,6 +110,10 @@
goto err;
}
+ if (!dh_check_params_fast(ret)) {
+ goto err;
+ }
+
return ret;
err:
diff --git a/crypto/dh_extra/dh_test.cc b/crypto/dh_extra/dh_test.cc
index f27c8f0..881f72d 100644
--- a/crypto/dh_extra/dh_test.cc
+++ b/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 @@
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 @@
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 @@
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 @@
// 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 @@
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 @@
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));
+}
diff --git a/crypto/dh_extra/params.c b/crypto/dh_extra/params.c
index 0e76747..548c4c8 100644
--- a/crypto/dh_extra/params.c
+++ b/crypto/dh_extra/params.c
@@ -337,6 +337,11 @@
// 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;
diff --git a/crypto/err/dh.errordata b/crypto/err/dh.errordata
index 9e1b87d..09053ae 100644
--- a/crypto/err/dh.errordata
+++ b/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
diff --git a/crypto/fipsmodule/dh/check.c b/crypto/fipsmodule/dh/check.c
index 0c82c17..b92b700 100644
--- a/crypto/fipsmodule/dh/check.c
+++ b/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 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 @@
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 @@
BN_ULONG l;
BIGNUM *t1 = NULL, *t2 = NULL;
- *out_flags = 0;
ctx = BN_CTX_new();
if (ctx == NULL) {
goto err;
diff --git a/crypto/fipsmodule/dh/dh.c b/crypto/fipsmodule/dh/dh.c
index 80940fd..1e8971a 100644
--- a/crypto/fipsmodule/dh/dh.c
+++ b/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_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 @@
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;
}
diff --git a/crypto/fipsmodule/dh/internal.h b/crypto/fipsmodule/dh/internal.h
index fe7fda4..d11e59b 100644
--- a/crypto/fipsmodule/dh/internal.h
+++ b/crypto/fipsmodule/dh/internal.h
@@ -26,6 +26,8 @@
#endif
+#define OPENSSL_DH_MAX_MODULUS_BITS 10000
+
struct dh_st {
BIGNUM *p;
BIGNUM *g;
@@ -44,6 +46,11 @@
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.
diff --git a/include/openssl/dh.h b/include/openssl/dh.h
index b83fb5e..a3094d8 100644
--- a/include/openssl/dh.h
+++ b/include/openssl/dh.h
@@ -353,5 +353,6 @@
#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
