diff --git a/crypto/fipsmodule/bn/bn_test.cc b/crypto/fipsmodule/bn/bn_test.cc index 4e35645c3..9d9e1d368 100644 --- a/crypto/fipsmodule/bn/bn_test.cc +++ b/crypto/fipsmodule/bn/bn_test.cc @@ -2775,6 +2775,27 @@ TEST_F(BNTest, ModSqrtInvalid) { BN_free(BN_mod_sqrt(nullptr, bn2140142.get(), bn4588033.get(), ctx())); } +// Test that constructing Montgomery contexts for large bignums is not possible. +// Our Montgomery reduction implementation stack-allocates temporaries, so we +// cap how large of moduli we accept. +TEST_F(BNTest, MontgomeryLarge) { + std::vector large_bignum_bytes(16 * 1024, 0xff); + bssl::UniquePtr large_bignum( + BN_bin2bn(large_bignum_bytes.data(), large_bignum_bytes.size(), nullptr)); + ASSERT_TRUE(large_bignum); + bssl::UniquePtr mont( + BN_MONT_CTX_new_for_modulus(large_bignum.get(), ctx())); + EXPECT_FALSE(mont); + + // The same limit should apply when |BN_mod_exp_mont_consttime| internally + // constructs a |BN_MONT_CTX|. + bssl::UniquePtr r(BN_new()); + ASSERT_TRUE(r); + EXPECT_FALSE(BN_mod_exp_mont_consttime(r.get(), BN_value_one(), + large_bignum.get(), large_bignum.get(), + ctx(), nullptr)); +} + #if defined(OPENSSL_BN_ASM_MONT) && defined(SUPPORTS_ABI_TEST) TEST_F(BNTest, BNMulMontABI) { for (size_t words : {4, 5, 6, 7, 8, 16, 32}) { diff --git a/crypto/fipsmodule/bn/exponentiation.c b/crypto/fipsmodule/bn/exponentiation.c index afa8e7102..859cf65b8 100644 --- a/crypto/fipsmodule/bn/exponentiation.c +++ b/crypto/fipsmodule/bn/exponentiation.c @@ -109,6 +109,7 @@ #include #include +#include #include #include @@ -868,7 +869,7 @@ static int copy_from_prebuf(BIGNUM *b, int top, const BN_ULONG *table, int idx, // be revised now that our implementation is no longer cache-time-dependent. #define BN_window_bits_for_ctime_exponent_size(b) \ ((b) > 937 ? 6 : (b) > 306 ? 5 : (b) > 89 ? 4 : (b) > 22 ? 3 : 1) -#define BN_MAX_WINDOW_BITS_FOR_CTIME_EXPONENT_SIZE (6) +#define BN_MAX_MOD_EXP_CTIME_WINDOW (6) // This variant of |BN_mod_exp_mont| uses fixed windows and fixed memory access // patterns to protect secret exponents (cf. the hyper-threading timing attacks @@ -950,6 +951,16 @@ int BN_mod_exp_mont_consttime(BIGNUM *rr, const BIGNUM *a, const BIGNUM *p, // Get the window size to use with size of p. int window = BN_window_bits_for_ctime_exponent_size(bits); + assert(window <= BN_MAX_MOD_EXP_CTIME_WINDOW); + + // Calculating |powerbuf_len| below cannot overflow because of the bound on + // Montgomery reduction. + assert((size_t)top <= BN_MONTGOMERY_MAX_WORDS); + static_assert( + BN_MONTGOMERY_MAX_WORDS <= + INT_MAX / sizeof(BN_ULONG) / ((1 << BN_MAX_MOD_EXP_CTIME_WINDOW) + 3), + "powerbuf_len may overflow"); + #if defined(OPENSSL_BN_ASM_MONT5) if (window >= 5) { window = 5; // ~5% improvement for RSA2048 sign, and even for RSA4096 diff --git a/crypto/fipsmodule/bn/internal.h b/crypto/fipsmodule/bn/internal.h index f7adfe92e..85009a925 100644 --- a/crypto/fipsmodule/bn/internal.h +++ b/crypto/fipsmodule/bn/internal.h @@ -350,6 +350,12 @@ int bn_rand_range_words(BN_ULONG *out, BN_ULONG min_inclusive, int bn_rand_secret_range(BIGNUM *r, int *out_is_uniform, BN_ULONG min_inclusive, const BIGNUM *max_exclusive); +// BN_MONTGOMERY_MAX_WORDS is the maximum numer of words allowed in a |BIGNUM| +// used with Montgomery reduction. Ideally this limit would be applied to all +// |BIGNUM|s, in |bn_wexpand|, but the exactfloat library needs to create 8 MiB +// values for other operations. +#define BN_MONTGOMERY_MAX_WORDS (8 * 1024 / sizeof(BN_ULONG)) + #if !defined(OPENSSL_NO_ASM) && \ (defined(OPENSSL_X86) || defined(OPENSSL_X86_64) || \ defined(OPENSSL_ARM) || defined(OPENSSL_AARCH64)) @@ -362,11 +368,13 @@ int bn_rand_secret_range(BIGNUM *r, int *out_is_uniform, BN_ULONG min_inclusive, // If at least one of |ap| or |bp| is fully reduced, |rp| will be fully reduced. // If neither is fully-reduced, the output may not be either. // +// This function allocates |num| words on the stack, so |num| should be at most +// |BN_MONTGOMERY_MAX_WORDS|. +// // TODO(davidben): The x86_64 implementation expects a 32-bit input and masks // off upper bits. The aarch64 implementation expects a 64-bit input and does // not. |size_t| is the safer option but not strictly correct for x86_64. But -// this function implicitly already has a bound on the size of |num| because it -// internally creates |num|-sized stack allocation. +// the |BN_MONTGOMERY_MAX_WORDS| bound makes this moot. // // See also discussion in |ToWord| in abi_test.h for notes on smaller-than-word // inputs. diff --git a/crypto/fipsmodule/bn/montgomery.c b/crypto/fipsmodule/bn/montgomery.c index d04e91a13..92bfa5e32 100644 --- a/crypto/fipsmodule/bn/montgomery.c +++ b/crypto/fipsmodule/bn/montgomery.c @@ -172,6 +172,10 @@ static int bn_mont_ctx_set_N_and_n0(BN_MONT_CTX *mont, const BIGNUM *mod) { OPENSSL_PUT_ERROR(BN, BN_R_NEGATIVE_NUMBER); return 0; } + if (!bn_fits_in_words(mod, BN_MONTGOMERY_MAX_WORDS)) { + OPENSSL_PUT_ERROR(BN, BN_R_BIGNUM_TOO_LONG); + return 0; + } // Save the modulus. if (!BN_copy(&mont->N, mod)) { @@ -428,6 +432,9 @@ int BN_mod_mul_montgomery(BIGNUM *r, const BIGNUM *a, const BIGNUM *b, if (!bn_wexpand(r, num)) { return 0; } + // This bound is implied by |bn_mont_ctx_set_N_and_n0|. |bn_mul_mont| + // allocates |num| words on the stack, so |num| cannot be too large. + assert((size_t)num <= BN_MONTGOMERY_MAX_WORDS); if (!bn_mul_mont(r->d, a->d, b->d, mont->N.d, mont->n0, num)) { // The check above ensures this won't happen. assert(0);