Skip to content

Commit 0c97337

Browse files
smittals2davidben
andauthored
Cherrypick hardening DSA param checks from BoringSSL (#2293)
### Issues: `P211191334` ### Description of changes: This PR cherrypicks 68c29a24ee6c9c70ecce56766ca70b115aad768f from BoringSSL. This change adds size checks to DSA_generate_parameters_ex and DSA_generate_key operations, completing our bounds checks for DSA operations. While this was prompted by CVE-2024-4603, BoringSSL and AWS-LC are not affected by that vulnerability. These additional checks likely have no security impact since attackers typically can't control inputs to these functions, but we're adding them for consistency with our other DSA operations. ### Call-outs: Point out areas that need special attention or support during the review process. Discuss architecture or design changes. ### Testing: How is this change tested (unit tests, fuzz tests, etc.)? Are there any testing steps to be verified by the reviewer? By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license and the ISC license. Co-authored-by: David Benjamin <[email protected]>
1 parent b0ea041 commit 0c97337

File tree

4 files changed

+36
-5
lines changed

4 files changed

+36
-5
lines changed

crypto/dsa/dsa.c

Lines changed: 10 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -241,6 +241,11 @@ int dsa_internal_paramgen(DSA *dsa, size_t bits, const EVP_MD *evpmd,
241241
int *out_counter, unsigned long *out_h,
242242
BN_GENCB *cb)
243243
{
244+
if (bits > OPENSSL_DSA_MAX_MODULUS_BITS) {
245+
OPENSSL_PUT_ERROR(DSA, DSA_R_INVALID_PARAMETERS);
246+
return 0;
247+
}
248+
244249
int ok = 0;
245250
unsigned char seed[SHA256_DIGEST_LENGTH];
246251
unsigned char md[SHA256_DIGEST_LENGTH];
@@ -510,11 +515,13 @@ DSA *DSAparams_dup(const DSA *dsa) {
510515
}
511516

512517
int DSA_generate_key(DSA *dsa) {
518+
if (!dsa_check_key(dsa)) {
519+
return 0;
520+
}
521+
513522
int ok = 0;
514-
BN_CTX *ctx = NULL;
515523
BIGNUM *pub_key = NULL, *priv_key = NULL;
516-
517-
ctx = BN_CTX_new();
524+
BN_CTX *ctx = BN_CTX_new();
518525
if (ctx == NULL) {
519526
goto err;
520527
}

crypto/dsa/dsa_asn1.c

Lines changed: 0 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -66,8 +66,6 @@
6666
#include "../crypto/internal.h"
6767

6868

69-
#define OPENSSL_DSA_MAX_MODULUS_BITS 10000
70-
7169
// This function is in dsa_asn1.c rather than dsa.c because it is reachable from
7270
// |EVP_PKEY| parsers. This makes it easier for the static linker to drop most
7371
// of the DSA implementation.

crypto/dsa/dsa_test.cc

Lines changed: 24 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -236,6 +236,30 @@ TEST(DSATest, Generate) {
236236
sig_len, dsa.get()));
237237
}
238238

239+
TEST(DSATest, GenerateParamsTooLarge) {
240+
bssl::UniquePtr<DSA> dsa(DSA_new());
241+
ASSERT_TRUE(dsa);
242+
EXPECT_FALSE(DSA_generate_parameters_ex(
243+
dsa.get(), 10001, /*seed=*/nullptr, /*seed_len=*/0,
244+
/*out_counter=*/nullptr, /*out_h=*/nullptr,
245+
/*cb=*/nullptr));
246+
}
247+
248+
TEST(DSATest, GenerateKeyTooLarge) {
249+
bssl::UniquePtr<DSA> dsa = GetFIPSDSA();
250+
ASSERT_TRUE(dsa);
251+
bssl::UniquePtr<BIGNUM> large_p(BN_new());
252+
ASSERT_TRUE(large_p);
253+
ASSERT_TRUE(BN_set_bit(large_p.get(), 10001));
254+
ASSERT_TRUE(BN_set_bit(large_p.get(), 0));
255+
ASSERT_TRUE(DSA_set0_pqg(dsa.get(), /*p=*/large_p.get(), /*q=*/nullptr,
256+
/*g=*/nullptr));
257+
large_p.release(); // |DSA_set0_pqg| takes ownership on success.
258+
259+
// Don't generate DSA keys if the group is too large.
260+
EXPECT_FALSE(DSA_generate_key(dsa.get()));
261+
}
262+
239263
TEST(DSATest, Verify) {
240264
bssl::UniquePtr<DSA> dsa = GetFIPSDSA();
241265
ASSERT_TRUE(dsa);

crypto/dsa/internal.h

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -40,6 +40,8 @@ struct dsa_st {
4040
CRYPTO_EX_DATA ex_data;
4141
};
4242

43+
#define OPENSSL_DSA_MAX_MODULUS_BITS 10000
44+
4345
// dsa_check_key performs cheap self-checks on |dsa|, and ensures it is within
4446
// DoS bounds. It returns one on success and zero on error.
4547
int dsa_check_key(const DSA *dsa);

0 commit comments

Comments
 (0)