Add bn_copy_words.

This makes it easier going to and from non-minimal BIGNUMs and words
without worrying about the widths which are ultimately to become less
friendly.

Bug: 232
Change-Id: Ia57cb29164c560b600573c27b112ad9375a86aad
Reviewed-on: https://boringssl-review.googlesource.com/25245
Reviewed-by: Adam Langley <agl@google.com>

crypto/fipsmodule/bn/bn.c

@@ -297,6 +297,35 @@ 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) {
+  // All words beyond |num| must be zero.
+  BN_ULONG mask = 0;
+  for (size_t i = num; i < (size_t)bn->top; i++) {
+    mask |= bn->d[i];
+  }
+  return mask == 0;
+}
+
+int bn_copy_words(BN_ULONG *out, size_t num, const BIGNUM *bn) {
+  if (bn->neg) {
+    OPENSSL_PUT_ERROR(BN, BN_R_NEGATIVE_NUMBER);
+    return 0;
+  }
+
+  size_t width = (size_t)bn->top;
+  if (width > num) {
+    if (!bn_fits_in_words(bn, num)) {
+      OPENSSL_PUT_ERROR(BN, BN_R_BIGNUM_TOO_LONG);
+      return 0;
+    }
+    width = num;
+  }
+
+  OPENSSL_memset(out, 0, sizeof(BN_ULONG) * num);
+  OPENSSL_memcpy(out, bn->d, sizeof(BN_ULONG) * width);
+  return 1;
+}
+
 int BN_is_negative(const BIGNUM *bn) {
   return bn->neg != 0;
 }
@@ -360,11 +389,7 @@ int bn_resize_words(BIGNUM *bn, size_t words) {
   }
 
   // All words beyond the new width must be zero.
-  BN_ULONG mask = 0;
-  for (size_t i = words; i < (size_t)bn->top; i++) {
-    mask |= bn->d[i];
-  }
-  if (mask != 0) {
+  if (!bn_fits_in_words(bn, words)) {
     OPENSSL_PUT_ERROR(BN, BN_R_BIGNUM_TOO_LONG);
     return 0;
   }

crypto/fipsmodule/bn/bn_test.cc

@@ -387,15 +387,15 @@ static void TestSquare(FileTest *t, BN_CTX *ctx) {
   }
 
 #if !defined(BORINGSSL_SHARED_LIBRARY)
-  if (static_cast<size_t>(a->top) <= BN_SMALL_MAX_WORDS) {
-    for (size_t num_a = a->top; num_a <= BN_SMALL_MAX_WORDS; num_a++) {
+  int a_width = bn_minimal_width(a.get());
+  if (a_width <= BN_SMALL_MAX_WORDS) {
+    for (size_t num_a = a_width; num_a <= BN_SMALL_MAX_WORDS; num_a++) {
       SCOPED_TRACE(num_a);
       size_t num_r = 2 * num_a;
       // Use newly-allocated buffers so ASan will catch out-of-bounds writes.
       std::unique_ptr<BN_ULONG[]> a_words(new BN_ULONG[num_a]),
           r_words(new BN_ULONG[num_r]);
-      OPENSSL_memset(a_words.get(), 0, num_a * sizeof(BN_ULONG));
-      OPENSSL_memcpy(a_words.get(), a->d, a->top * sizeof(BN_ULONG));
+      ASSERT_TRUE(bn_copy_words(a_words.get(), num_a, a.get()));
 
       ASSERT_TRUE(bn_mul_small(r_words.get(), num_r, a_words.get(), num_a,
                                a_words.get(), num_a));
@@ -445,22 +445,25 @@ static void TestProduct(FileTest *t, BN_CTX *ctx) {
   }
 
 #if !defined(BORINGSSL_SHARED_LIBRARY)
-  if (!BN_is_negative(product.get()) &&
-      static_cast<size_t>(a->top) <= BN_SMALL_MAX_WORDS &&
-      static_cast<size_t>(b->top) <= BN_SMALL_MAX_WORDS) {
-    for (size_t num_a = a->top; num_a <= BN_SMALL_MAX_WORDS; num_a++) {
+  BN_set_negative(a.get(), 0);
+  BN_set_negative(b.get(), 0);
+  BN_set_negative(product.get(), 0);
+
+  int a_width = bn_minimal_width(a.get());
+  int b_width = bn_minimal_width(b.get());
+  if (a_width <= BN_SMALL_MAX_WORDS && b_width <= BN_SMALL_MAX_WORDS) {
+    for (size_t num_a = static_cast<size_t>(a_width);
+         num_a <= BN_SMALL_MAX_WORDS; num_a++) {
       SCOPED_TRACE(num_a);
-      for (size_t num_b = b->top; num_b <= BN_SMALL_MAX_WORDS; num_b++) {
+      for (size_t num_b = static_cast<size_t>(b_width);
+           num_b <= BN_SMALL_MAX_WORDS; num_b++) {
         SCOPED_TRACE(num_b);
         size_t num_r = num_a + num_b;
         // Use newly-allocated buffers so ASan will catch out-of-bounds writes.
         std::unique_ptr<BN_ULONG[]> a_words(new BN_ULONG[num_a]),
             b_words(new BN_ULONG[num_b]), r_words(new BN_ULONG[num_r]);
-        OPENSSL_memset(a_words.get(), 0, num_a * sizeof(BN_ULONG));
-        OPENSSL_memcpy(a_words.get(), a->d, a->top * sizeof(BN_ULONG));
-
-        OPENSSL_memset(b_words.get(), 0, num_b * sizeof(BN_ULONG));
-        OPENSSL_memcpy(b_words.get(), b->d, b->top * sizeof(BN_ULONG));
+        ASSERT_TRUE(bn_copy_words(a_words.get(), num_a, a.get()));
+        ASSERT_TRUE(bn_copy_words(b_words.get(), num_b, b.get()));
 
         ASSERT_TRUE(bn_mul_small(r_words.get(), num_r, a_words.get(), num_a,
                                  b_words.get(), num_b));
@@ -554,24 +557,23 @@ static void TestModMul(FileTest *t, BN_CTX *ctx) {
                          ret.get());
 
 #if !defined(BORINGSSL_SHARED_LIBRARY)
-    if (m->top <= BN_SMALL_MAX_WORDS) {
-      std::unique_ptr<BN_ULONG[]> a_words(new BN_ULONG[m->top]),
-          b_words(new BN_ULONG[m->top]), r_words(new BN_ULONG[m->top]);
-      OPENSSL_memset(a_words.get(), 0, m->top * sizeof(BN_ULONG));
-      OPENSSL_memcpy(a_words.get(), a->d, a->top * sizeof(BN_ULONG));
-      OPENSSL_memset(b_words.get(), 0, m->top * sizeof(BN_ULONG));
-      OPENSSL_memcpy(b_words.get(), b->d, b->top * sizeof(BN_ULONG));
-      ASSERT_TRUE(bn_to_montgomery_small(a_words.get(), m->top, a_words.get(),
-                                         m->top, mont.get()));
-      ASSERT_TRUE(bn_to_montgomery_small(b_words.get(), m->top, b_words.get(),
-                                         m->top, mont.get()));
+    size_t m_width = static_cast<size_t>(bn_minimal_width(m.get()));
+    if (m_width <= BN_SMALL_MAX_WORDS) {
+      std::unique_ptr<BN_ULONG[]> a_words(new BN_ULONG[m_width]),
+          b_words(new BN_ULONG[m_width]), r_words(new BN_ULONG[m_width]);
+      ASSERT_TRUE(bn_copy_words(a_words.get(), m_width, a.get()));
+      ASSERT_TRUE(bn_copy_words(b_words.get(), m_width, b.get()));
+      ASSERT_TRUE(bn_to_montgomery_small(a_words.get(), m_width, a_words.get(),
+                                         m_width, mont.get()));
+      ASSERT_TRUE(bn_to_montgomery_small(b_words.get(), m_width, b_words.get(),
+                                         m_width, mont.get()));
       ASSERT_TRUE(bn_mod_mul_montgomery_small(
-          r_words.get(), m->top, a_words.get(), m->top, b_words.get(), m->top,
+          r_words.get(), m_width, a_words.get(), m_width, b_words.get(), m_width,
           mont.get()));
       // Use the second half of |tmp| so ASan will catch out-of-bounds writes.
-      ASSERT_TRUE(bn_from_montgomery_small(r_words.get(), m->top, r_words.get(),
-                                           m->top, mont.get()));
-      ASSERT_TRUE(bn_set_words(ret.get(), r_words.get(), m->top));
+      ASSERT_TRUE(bn_from_montgomery_small(r_words.get(), m_width, r_words.get(),
+                                           m_width, mont.get()));
+      ASSERT_TRUE(bn_set_words(ret.get(), r_words.get(), m_width));
       EXPECT_BIGNUMS_EQUAL("A * B (mod M) (Montgomery, words)", mod_mul.get(),
                            ret.get());
     }
@@ -623,32 +625,32 @@ static void TestModSquare(FileTest *t, BN_CTX *ctx) {
                          ret.get());
 
 #if !defined(BORINGSSL_SHARED_LIBRARY)
-    if (m->top <= BN_SMALL_MAX_WORDS) {
-      std::unique_ptr<BN_ULONG[]> a_words(new BN_ULONG[m->top]),
-          a_copy_words(new BN_ULONG[m->top]), r_words(new BN_ULONG[m->top]);
-      OPENSSL_memset(a_words.get(), 0, m->top * sizeof(BN_ULONG));
-      OPENSSL_memcpy(a_words.get(), a->d, a->top * sizeof(BN_ULONG));
-      ASSERT_TRUE(bn_to_montgomery_small(a_words.get(), m->top, a_words.get(),
-                                         m->top, mont.get()));
+    size_t m_width = static_cast<size_t>(bn_minimal_width(m.get()));
+    if (m_width <= BN_SMALL_MAX_WORDS) {
+      std::unique_ptr<BN_ULONG[]> a_words(new BN_ULONG[m_width]),
+          a_copy_words(new BN_ULONG[m_width]), r_words(new BN_ULONG[m_width]);
+      ASSERT_TRUE(bn_copy_words(a_words.get(), m_width, a.get()));
+      ASSERT_TRUE(bn_to_montgomery_small(a_words.get(), m_width, a_words.get(),
+                                         m_width, mont.get()));
       ASSERT_TRUE(bn_mod_mul_montgomery_small(
-          r_words.get(), m->top, a_words.get(), m->top, a_words.get(), m->top,
-          mont.get()));
-      ASSERT_TRUE(bn_from_montgomery_small(r_words.get(), m->top, r_words.get(),
-                                           m->top, mont.get()));
-      ASSERT_TRUE(bn_set_words(ret.get(), r_words.get(), m->top));
+          r_words.get(), m_width, a_words.get(), m_width, a_words.get(),
+          m_width, mont.get()));
+      ASSERT_TRUE(bn_from_montgomery_small(r_words.get(), m_width,
+                                           r_words.get(), m_width, mont.get()));
+      ASSERT_TRUE(bn_set_words(ret.get(), r_words.get(), m_width));
       EXPECT_BIGNUMS_EQUAL("A * A (mod M) (Montgomery, words)",
                            mod_square.get(), ret.get());
 
       // Repeat the operation with |a_copy_words|.
       OPENSSL_memcpy(a_copy_words.get(), a_words.get(),
-                     m->top * sizeof(BN_ULONG));
+                     m_width * sizeof(BN_ULONG));
       ASSERT_TRUE(bn_mod_mul_montgomery_small(
-          r_words.get(), m->top, a_words.get(), m->top, a_copy_words.get(),
-          m->top, mont.get()));
+          r_words.get(), m_width, a_words.get(), m_width, a_copy_words.get(),
+          m_width, mont.get()));
       // Use the second half of |tmp| so ASan will catch out-of-bounds writes.
-      ASSERT_TRUE(bn_from_montgomery_small(r_words.get(), m->top, r_words.get(),
-                                           m->top, mont.get()));
-      ASSERT_TRUE(bn_set_words(ret.get(), r_words.get(), m->top));
+      ASSERT_TRUE(bn_from_montgomery_small(r_words.get(), m_width,
+                                           r_words.get(), m_width, mont.get()));
+      ASSERT_TRUE(bn_set_words(ret.get(), r_words.get(), m_width));
       EXPECT_BIGNUMS_EQUAL("A * A_copy (mod M) (Montgomery, words)",
                            mod_square.get(), ret.get());
     }
@@ -683,22 +685,22 @@ static void TestModExp(FileTest *t, BN_CTX *ctx) {
                          ret.get());
 
 #if !defined(BORINGSSL_SHARED_LIBRARY)
-    if (m->top <= BN_SMALL_MAX_WORDS) {
+    size_t m_width = static_cast<size_t>(bn_minimal_width(m.get()));
+    if (m_width <= BN_SMALL_MAX_WORDS) {
       bssl::UniquePtr<BN_MONT_CTX> mont(BN_MONT_CTX_new());
       ASSERT_TRUE(mont.get());
       ASSERT_TRUE(BN_MONT_CTX_set(mont.get(), m.get(), ctx));
       ASSERT_TRUE(BN_nnmod(a.get(), a.get(), m.get(), ctx));
-      std::unique_ptr<BN_ULONG[]> r_words(new BN_ULONG[m->top]),
-          a_words(new BN_ULONG[m->top]);
-      OPENSSL_memset(a_words.get(), 0, m->top * sizeof(BN_ULONG));
-      OPENSSL_memcpy(a_words.get(), a->d, a->top * sizeof(BN_ULONG));
-      ASSERT_TRUE(bn_to_montgomery_small(a_words.get(), m->top, a_words.get(),
-                                         m->top, mont.get()));
-      ASSERT_TRUE(bn_mod_exp_mont_small(r_words.get(), m->top, a_words.get(),
-                                        m->top, e->d, e->top, mont.get()));
-      ASSERT_TRUE(bn_from_montgomery_small(r_words.get(), m->top, r_words.get(),
-                                           m->top, mont.get()));
-      ASSERT_TRUE(bn_set_words(ret.get(), r_words.get(), m->top));
+      std::unique_ptr<BN_ULONG[]> r_words(new BN_ULONG[m_width]),
+          a_words(new BN_ULONG[m_width]);
+      ASSERT_TRUE(bn_copy_words(a_words.get(), m_width, a.get()));
+      ASSERT_TRUE(bn_to_montgomery_small(a_words.get(), m_width, a_words.get(),
+                                         m_width, mont.get()));
+      ASSERT_TRUE(bn_mod_exp_mont_small(r_words.get(), m_width, a_words.get(),
+                                        m_width, e->d, e->top, mont.get()));
+      ASSERT_TRUE(bn_from_montgomery_small(r_words.get(), m_width,
+                                           r_words.get(), m_width, mont.get()));
+      ASSERT_TRUE(bn_set_words(ret.get(), r_words.get(), m_width));
       EXPECT_BIGNUMS_EQUAL("A ^ E (mod M) (Montgomery, words)", mod_exp.get(),
                            ret.get());
     }

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_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);
+
 // bn_mul_add_words multiples |ap| by |w|, adds the result to |rp|, and places
 // the result in |rp|. |ap| and |rp| must both be |num| words long. It returns
 // the carry word of the operation. |ap| and |rp| may be equal but otherwise may

crypto/fipsmodule/ec/ec.c

@@ -952,12 +952,10 @@ int ec_bignum_to_scalar(const EC_GROUP *group, EC_SCALAR *out,
 
 int ec_bignum_to_scalar_unchecked(const EC_GROUP *group, EC_SCALAR *out,
                                   const BIGNUM *in) {
-  if (BN_is_negative(in) || in->top > group->order.top) {
+  if (!bn_copy_words(out->words, group->order.top, in)) {
     OPENSSL_PUT_ERROR(EC, EC_R_INVALID_SCALAR);
     return 0;
   }
-  OPENSSL_memset(out->words, 0, group->order.top * sizeof(BN_ULONG));
-  OPENSSL_memcpy(out->words, in->d, in->top * sizeof(BN_ULONG));
   return 1;
 }
 

crypto/fipsmodule/ec/ec_test.cc

@@ -28,6 +28,7 @@
 #include <openssl/nid.h>
 #include <openssl/obj.h>
 
+#include "../bn/internal.h"
 #include "../../test/test_util.h"
 
 
@@ -553,6 +554,32 @@ TEST_P(ECCurveTest, Mul) {
   EXPECT_EQ(0, EC_POINT_cmp(group.get(), result.get(), generator, nullptr));
 }
 
+#if !defined(BORINGSSL_SHARED_LIBRARY)
+TEST_P(ECCurveTest, MulNonMinimal) {
+  bssl::UniquePtr<EC_GROUP> group(EC_GROUP_new_by_curve_name(GetParam().nid));
+  ASSERT_TRUE(group);
+
+  bssl::UniquePtr<BIGNUM> forty_two(BN_new());
+  ASSERT_TRUE(forty_two);
+  ASSERT_TRUE(BN_set_word(forty_two.get(), 42));
+
+  // Compute g × 42.
+  bssl::UniquePtr<EC_POINT> point(EC_POINT_new(group.get()));
+  ASSERT_TRUE(point);
+  ASSERT_TRUE(EC_POINT_mul(group.get(), point.get(), forty_two.get(), nullptr,
+                           nullptr, nullptr));
+
+  // Compute it again with a non-minimal 42, much larger than the scalar.
+  ASSERT_TRUE(bn_resize_words(forty_two.get(), 64));
+
+  bssl::UniquePtr<EC_POINT> point2(EC_POINT_new(group.get()));
+  ASSERT_TRUE(point2);
+  ASSERT_TRUE(EC_POINT_mul(group.get(), point2.get(), forty_two.get(), nullptr,
+                           nullptr, nullptr));
+  EXPECT_EQ(0, EC_POINT_cmp(group.get(), point.get(), point2.get(), nullptr));
+}
+#endif  // BORINGSSL_SHARED_LIBRARY
+
 // Test that EC_KEY_set_private_key rejects invalid values.
 TEST_P(ECCurveTest, SetInvalidPrivateKey) {
   bssl::UniquePtr<EC_KEY> key(EC_KEY_new_by_curve_name(GetParam().nid));

crypto/fipsmodule/ec/p256-x86_64.c

@@ -205,13 +205,7 @@ static void ecp_nistz256_mod_inverse_mont(BN_ULONG r[P256_LIMBS],
 // returns one if it fits. Otherwise it returns zero.
 static int ecp_nistz256_bignum_to_field_elem(BN_ULONG out[P256_LIMBS],
                                              const BIGNUM *in) {
-  if (in->top > P256_LIMBS) {
-    return 0;
-  }
-
-  OPENSSL_memset(out, 0, sizeof(BN_ULONG) * P256_LIMBS);
-  OPENSSL_memcpy(out, in->d, sizeof(BN_ULONG) * in->top);
-  return 1;
+  return bn_copy_words(out, P256_LIMBS, in);
 }
 
 // r = p * p_scalar

crypto/fipsmodule/ec/p256-x86_64_test.cc

@@ -160,10 +160,9 @@ static bool PointToAffine(P256_POINT_AFFINE *out, const P256_POINT *in) {
     return false;
   }
 
-  OPENSSL_memset(out, 0, sizeof(P256_POINT_AFFINE));
-
   if (BN_is_zero(z.get())) {
     // The point at infinity is represented as (0, 0).
+    OPENSSL_memset(out, 0, sizeof(P256_POINT_AFFINE));
     return true;
   }
 
@@ -185,12 +184,11 @@ static bool PointToAffine(P256_POINT_AFFINE *out, const P256_POINT *in) {
       !BN_mod_mul_montgomery(y.get(), y.get(), z.get(), mont.get(),
                              ctx.get()) ||
       !BN_mod_mul_montgomery(y.get(), y.get(), z.get(), mont.get(),
-                             ctx.get())) {
+                             ctx.get()) ||
+      !bn_copy_words(out->X, P256_LIMBS, x.get()) ||
+      !bn_copy_words(out->Y, P256_LIMBS, y.get())) {
     return false;
   }
-
-  OPENSSL_memcpy(out->X, x->d, sizeof(BN_ULONG) * x->top);
-  OPENSSL_memcpy(out->Y, y->d, sizeof(BN_ULONG) * y->top);
   return true;
 }