Check DSA size limits in a couple more places
This change was prompted by CVE-2024-4603, but is *not* part of it.
BoringSSL is not affected by CVE-2024-4603.
When applications use DSA they are expected to use known-valid domain
parameters (as required by FIPS 186-4, section 4.3). If the application
instead allows the attacker to supply domain parameters, DSA is no
longer cryptographically sound, and algorithms that were otherwise
correct now become DoS attack surfaces.
Alas, incorrect uses of DSA, like Diffie-Hellman (see CVE-2023-3446 and
CVE-2023-3817) are rampant, so it is useful to bound our operations, so
we at least can say something coherent about DoS in this scenario, even
though we cannot avoid the application being cryptographically unsound.
To that end, we already check DSA sizes before using the key, however,
we missed a couple of operations:
- DSA_generate_parameters_ex
- DSA_generate_key
The CL adds those too. I would not expect either to have any security
impact as it's quite unlikely for an application to allow attacker
control of the inputs to either function. Still, we ought to check for
completeness' sake. There's no sense in generating parameters or a key
that we know we'll reject.
Ultimately, the problem here is that DSA was a badly-designed primitive.
with a continuum of domain parameters. Experience has shown that such
systems are confusing and applications will often incorrectly treat
domain parameters as part of the key. Modern primitives use a small set
of discrete, named parameter sets, which is far easier to get right. For
this reason, we consider DSA a deprecated, legacy algorithm.
Change-Id: Ib3b5dece32bcb0ac9a795f8222c1c530d9dd91a0
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/68707
Reviewed-by: Adam Langley <agl@google.com>
Commit-Queue: Adam Langley <agl@google.com>
Auto-Submit: David Benjamin <davidben@google.com>
diff --git a/crypto/dsa/dsa.c b/crypto/dsa/dsa.c
index a35b384..b2eda20 100644
--- a/crypto/dsa/dsa.c
+++ b/crypto/dsa/dsa.c
@@ -208,6 +208,11 @@
int DSA_generate_parameters_ex(DSA *dsa, unsigned bits, const uint8_t *seed_in,
size_t seed_len, int *out_counter,
unsigned long *out_h, BN_GENCB *cb) {
+ if (bits > OPENSSL_DSA_MAX_MODULUS_BITS) {
+ OPENSSL_PUT_ERROR(DSA, DSA_R_INVALID_PARAMETERS);
+ return 0;
+ }
+
int ok = 0;
unsigned char seed[SHA256_DIGEST_LENGTH];
unsigned char md[SHA256_DIGEST_LENGTH];
@@ -479,11 +484,13 @@
}
int DSA_generate_key(DSA *dsa) {
- int ok = 0;
- BN_CTX *ctx = NULL;
- BIGNUM *pub_key = NULL, *priv_key = NULL;
+ if (!dsa_check_key(dsa)) {
+ return 0;
+ }
- ctx = BN_CTX_new();
+ int ok = 0;
+ BIGNUM *pub_key = NULL, *priv_key = NULL;
+ BN_CTX *ctx = BN_CTX_new();
if (ctx == NULL) {
goto err;
}
diff --git a/crypto/dsa/dsa_asn1.c b/crypto/dsa/dsa_asn1.c
index 1caf4ae..8ec55dd 100644
--- a/crypto/dsa/dsa_asn1.c
+++ b/crypto/dsa/dsa_asn1.c
@@ -65,8 +65,6 @@
#include "../bytestring/internal.h"
-#define OPENSSL_DSA_MAX_MODULUS_BITS 10000
-
// This function is in dsa_asn1.c rather than dsa.c because it is reachable from
// |EVP_PKEY| parsers. This makes it easier for the static linker to drop most
// of the DSA implementation.
diff --git a/crypto/dsa/dsa_test.cc b/crypto/dsa/dsa_test.cc
index 3b83e18..f90c009 100644
--- a/crypto/dsa/dsa_test.cc
+++ b/crypto/dsa/dsa_test.cc
@@ -236,6 +236,30 @@
sig_len, dsa.get()));
}
+TEST(DSATest, GenerateParamsTooLarge) {
+ bssl::UniquePtr<DSA> dsa(DSA_new());
+ ASSERT_TRUE(dsa);
+ EXPECT_FALSE(DSA_generate_parameters_ex(
+ dsa.get(), 10001, /*seed=*/nullptr, /*seed_len=*/0,
+ /*out_counter=*/nullptr, /*out_h=*/nullptr,
+ /*cb=*/nullptr));
+}
+
+TEST(DSATest, GenerateKeyTooLarge) {
+ bssl::UniquePtr<DSA> dsa = GetFIPSDSA();
+ ASSERT_TRUE(dsa);
+ bssl::UniquePtr<BIGNUM> large_p(BN_new());
+ ASSERT_TRUE(large_p);
+ ASSERT_TRUE(BN_set_bit(large_p.get(), 10001));
+ ASSERT_TRUE(BN_set_bit(large_p.get(), 0));
+ ASSERT_TRUE(DSA_set0_pqg(dsa.get(), /*p=*/large_p.get(), /*q=*/nullptr,
+ /*g=*/nullptr));
+ large_p.release(); // |DSA_set0_pqg| takes ownership on success.
+
+ // Don't generate DSA keys if the group is too large.
+ EXPECT_FALSE(DSA_generate_key(dsa.get()));
+}
+
TEST(DSATest, Verify) {
bssl::UniquePtr<DSA> dsa = GetFIPSDSA();
ASSERT_TRUE(dsa);
diff --git a/crypto/dsa/internal.h b/crypto/dsa/internal.h
index 61cf9a6..9cceeb1 100644
--- a/crypto/dsa/internal.h
+++ b/crypto/dsa/internal.h
@@ -42,6 +42,8 @@
CRYPTO_EX_DATA ex_data;
};
+#define OPENSSL_DSA_MAX_MODULUS_BITS 10000
+
// dsa_check_key performs cheap self-checks on |dsa|, and ensures it is within
// DoS bounds. It returns one on success and zero on error.
int dsa_check_key(const DSA *dsa);
