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 <agl@google.com>

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++) {

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<BIGNUM> p(BN_bin2bn(kP, sizeof(kP), nullptr));
+  ASSERT_TRUE(p);
+
+  bssl::UniquePtr<BN_MONT_CTX> 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<BN_MONT_CTX> 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

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.
 //

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,

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);
   }
 

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
 };