From 76ce04bec8162a866ce42cc27ed792c48484908a Mon Sep 17 00:00:00 2001 From: David Benjamin Date: Tue, 23 Jan 2018 16:17:55 -0500 Subject: [PATCH] Fix up BN_MONT_CTX_set with non-minimal values. Give a non-minimal modulus, there are two possible values of R we might pick: 2^(BN_BITS2 * width) or 2^(BN_BITS2 * bn_minimal_width). Potentially secret moduli would make the former attractive and things might even work, but our only secret moduli (RSA) have public bit widths. It's more cases to test and the usual BIGNUM invariant is that widths do not affect numerical output. Thus, settle on minimizing mont->N for now. With the top explicitly made minimal, computing |lgBigR| is also a little simpler. This CL also abstracts out the < R check in the RSA code, and implements it in a width-agnostic way. Bug: 232 Change-Id: I354643df30530db7866bb7820e34241d7614f3c2 Reviewed-on: https://boringssl-review.googlesource.com/25250 Reviewed-by: Adam Langley --- crypto/fipsmodule/bn/bn.c | 2 +- crypto/fipsmodule/bn/bn_test.cc | 22 ++++++++++++++++++++++ crypto/fipsmodule/bn/internal.h | 8 ++++++++ crypto/fipsmodule/bn/montgomery.c | 13 +++++++++++-- crypto/fipsmodule/rsa/rsa_impl.c | 11 +++++------ include/openssl/bn.h | 7 +++++-- 6 files changed, 52 insertions(+), 11 deletions(-) diff --git a/crypto/fipsmodule/bn/bn.c b/crypto/fipsmodule/bn/bn.c index aa0c619a..520ca27d 100644 --- a/crypto/fipsmodule/bn/bn.c +++ b/crypto/fipsmodule/bn/bn.c @@ -297,7 +297,7 @@ int bn_set_words(BIGNUM *bn, const BN_ULONG *words, size_t num) { return 1; } -static int bn_fits_in_words(const BIGNUM *bn, size_t num) { +int bn_fits_in_words(const BIGNUM *bn, size_t num) { // All words beyond |num| must be zero. BN_ULONG mask = 0; for (size_t i = num; i < (size_t)bn->top; i++) { diff --git a/crypto/fipsmodule/bn/bn_test.cc b/crypto/fipsmodule/bn/bn_test.cc index 707d5f58..89db4ad1 100644 --- a/crypto/fipsmodule/bn/bn_test.cc +++ b/crypto/fipsmodule/bn/bn_test.cc @@ -1957,6 +1957,28 @@ TEST_F(BNTest, NonMinimal) { EXPECT_TRUE(bn_resize_words(eight.get(), 4)); EXPECT_EQ(4, eight->top); EXPECT_TRUE(BN_is_pow2(eight.get())); + + // |BN_MONT_CTX| is always stored minimally and uses the same R independent of + // input width. + static const uint8_t kP[] = { + 0xff, 0xff, 0xff, 0xff, 0x00, 0x00, 0x00, 0x01, 0x00, 0x00, 0x00, + 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0xff, 0xff, + 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, + }; + bssl::UniquePtr p(BN_bin2bn(kP, sizeof(kP), nullptr)); + ASSERT_TRUE(p); + + bssl::UniquePtr mont(BN_MONT_CTX_new()); + ASSERT_TRUE(mont); + ASSERT_TRUE(BN_MONT_CTX_set(mont.get(), p.get(), ctx())); + + ASSERT_TRUE(bn_resize_words(p.get(), 32)); + bssl::UniquePtr mont2(BN_MONT_CTX_new()); + ASSERT_TRUE(mont2); + ASSERT_TRUE(BN_MONT_CTX_set(mont2.get(), p.get(), ctx())); + + EXPECT_EQ(mont->N.top, mont2->N.top); + EXPECT_EQ(0, BN_cmp(&mont->RR, &mont2->RR)); } #endif // !BORINGSSL_SHARED_LIBRARY diff --git a/crypto/fipsmodule/bn/internal.h b/crypto/fipsmodule/bn/internal.h index 83102d8d..f3b8d8ad 100644 --- a/crypto/fipsmodule/bn/internal.h +++ b/crypto/fipsmodule/bn/internal.h @@ -226,6 +226,10 @@ int bn_resize_words(BIGNUM *bn, size_t words); // least significant word first. int bn_set_words(BIGNUM *bn, const BN_ULONG *words, size_t num); +// bn_fits_in_words returns one if |bn| may be represented in |num| words, plus +// a sign bit, and zero otherwise. +int bn_fits_in_words(const BIGNUM *bn, size_t num); + // bn_copy_words copies the value of |bn| to |out| and returns one if the value // is representable in |num| words. Otherwise, it returns zero. int bn_copy_words(BN_ULONG *out, size_t num, const BIGNUM *bn); @@ -347,6 +351,10 @@ int bn_is_bit_set_words(const BN_ULONG *a, size_t num, unsigned bit); // as public. int bn_one_to_montgomery(BIGNUM *r, const BN_MONT_CTX *mont, BN_CTX *ctx); +// bn_less_than_montgomery_R returns one if |bn| is less than the Montgomery R +// value for |mont| and zero otherwise. +int bn_less_than_montgomery_R(const BIGNUM *bn, const BN_MONT_CTX *mont); + // Low-level operations for small numbers. // diff --git a/crypto/fipsmodule/bn/montgomery.c b/crypto/fipsmodule/bn/montgomery.c index 49979fdb..624ab5fc 100644 --- a/crypto/fipsmodule/bn/montgomery.c +++ b/crypto/fipsmodule/bn/montgomery.c @@ -189,6 +189,10 @@ int BN_MONT_CTX_set(BN_MONT_CTX *mont, const BIGNUM *mod, BN_CTX *ctx) { OPENSSL_PUT_ERROR(BN, ERR_R_INTERNAL_ERROR); return 0; } + // |mont->N| is always stored minimally. Computing RR efficiently leaks the + // size of the modulus. While the modulus may be private in RSA (one of the + // primes), their sizes are public, so this is fine. + bn_correct_top(&mont->N); // Find n0 such that n0 * N == -1 (mod r). // @@ -196,7 +200,7 @@ int BN_MONT_CTX_set(BN_MONT_CTX *mont, const BIGNUM *mod, BN_CTX *ctx) { // others, we could use a shorter R value and use faster |BN_ULONG|-based // math instead of |uint64_t|-based math, which would be double-precision. // However, currently only the assembler files know which is which. - uint64_t n0 = bn_mont_n0(mod); + uint64_t n0 = bn_mont_n0(&mont->N); mont->n0[0] = (BN_ULONG)n0; #if BN_MONT_CTX_N0_LIMBS == 2 mont->n0[1] = (BN_ULONG)(n0 >> BN_BITS2); @@ -211,7 +215,7 @@ int BN_MONT_CTX_set(BN_MONT_CTX *mont, const BIGNUM *mod, BN_CTX *ctx) { // as |BN_MONT_CTX_N0_LIMBS| is either one or two. // // XXX: This is not constant time with respect to |mont->N|, but it should be. - unsigned lgBigR = (BN_num_bits(mod) + (BN_BITS2 - 1)) / BN_BITS2 * BN_BITS2; + unsigned lgBigR = mont->N.top * BN_BITS2; if (!bn_mod_exp_base_2_vartime(&mont->RR, lgBigR * 2, &mont->N)) { return 0; } @@ -443,6 +447,11 @@ int BN_mod_mul_montgomery(BIGNUM *r, const BIGNUM *a, const BIGNUM *b, return bn_mod_mul_montgomery_fallback(r, a, b, mont, ctx); } +int bn_less_than_montgomery_R(const BIGNUM *bn, const BN_MONT_CTX *mont) { + return !BN_is_negative(bn) && + bn_fits_in_words(bn, mont->N.top); +} + int bn_to_montgomery_small(BN_ULONG *r, size_t num_r, const BN_ULONG *a, size_t num_a, const BN_MONT_CTX *mont) { return bn_mod_mul_montgomery_small(r, num_r, a, num_a, mont->RR.d, diff --git a/crypto/fipsmodule/rsa/rsa_impl.c b/crypto/fipsmodule/rsa/rsa_impl.c index 1c7bcefa..626bbe85 100644 --- a/crypto/fipsmodule/rsa/rsa_impl.c +++ b/crypto/fipsmodule/rsa/rsa_impl.c @@ -646,12 +646,11 @@ err: static int mod_montgomery(BIGNUM *r, const BIGNUM *I, const BIGNUM *p, const BN_MONT_CTX *mont_p, const BIGNUM *q, BN_CTX *ctx) { - // Reduce in constant time with Montgomery reduction, which requires I <= p * - // R. If p and q are the same size, which is true for any RSA keys we or - // anyone sane generates, we have q < R and I < p * q, so this holds. - // - // If q is too big, fall back to |BN_mod|. - if (q->top > p->top) { + // Reducing in constant-time with Montgomery reduction requires I <= p * R. We + // have I < p * q, so this follows if q < R. In particular, this always holds + // if p and q are the same size, which is true for any RSA keys we or anyone + // sane generates. For other keys, we fall back to |BN_mod|. + if (!bn_less_than_montgomery_R(q, mont_p)) { return BN_mod(r, I, p, ctx); } diff --git a/include/openssl/bn.h b/include/openssl/bn.h index becc9553..d9a7f082 100644 --- a/include/openssl/bn.h +++ b/include/openssl/bn.h @@ -909,8 +909,11 @@ struct bignum_st { }; struct bn_mont_ctx_st { - BIGNUM RR; // used to convert to montgomery form - BIGNUM N; // The modulus + // RR is R^2, reduced modulo |N|. It is used to convert to Montgomery form. + BIGNUM RR; + // N is the modulus. It is always stored in minimal form, so |N.top| + // determines R. + BIGNUM N; BN_ULONG n0[2]; // least significant words of (R*Ri-1)/N };