diff --git a/crypto/fipsmodule/bn/bn.c b/crypto/fipsmodule/bn/bn.c index 70f98c8a4..aa0c619a2 100644 --- a/crypto/fipsmodule/bn/bn.c +++ b/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; } diff --git a/crypto/fipsmodule/bn/bn_test.cc b/crypto/fipsmodule/bn/bn_test.cc index 8e6f4eb4a..a9442cdab 100644 --- a/crypto/fipsmodule/bn/bn_test.cc +++ b/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(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 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(a->top) <= BN_SMALL_MAX_WORDS && - static_cast(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(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(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 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 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(bn_minimal_width(m.get())); + if (m_width <= BN_SMALL_MAX_WORDS) { + std::unique_ptr 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 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(bn_minimal_width(m.get())); + if (m_width <= BN_SMALL_MAX_WORDS) { + std::unique_ptr 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(bn_minimal_width(m.get())); + if (m_width <= BN_SMALL_MAX_WORDS) { bssl::UniquePtr 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 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 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()); } diff --git a/crypto/fipsmodule/bn/internal.h b/crypto/fipsmodule/bn/internal.h index 1a5dc1b38..140353fcf 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_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 diff --git a/crypto/fipsmodule/ec/ec.c b/crypto/fipsmodule/ec/ec.c index c9687a618..f002ccd66 100644 --- a/crypto/fipsmodule/ec/ec.c +++ b/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; } diff --git a/crypto/fipsmodule/ec/ec_test.cc b/crypto/fipsmodule/ec/ec_test.cc index e69f8d720..85dc8f28c 100644 --- a/crypto/fipsmodule/ec/ec_test.cc +++ b/crypto/fipsmodule/ec/ec_test.cc @@ -28,6 +28,7 @@ #include #include +#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 group(EC_GROUP_new_by_curve_name(GetParam().nid)); + ASSERT_TRUE(group); + + bssl::UniquePtr forty_two(BN_new()); + ASSERT_TRUE(forty_two); + ASSERT_TRUE(BN_set_word(forty_two.get(), 42)); + + // Compute g × 42. + bssl::UniquePtr 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 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 key(EC_KEY_new_by_curve_name(GetParam().nid)); diff --git a/crypto/fipsmodule/ec/p256-x86_64.c b/crypto/fipsmodule/ec/p256-x86_64.c index 0e79b6dc8..ec371bf0b 100644 --- a/crypto/fipsmodule/ec/p256-x86_64.c +++ b/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 diff --git a/crypto/fipsmodule/ec/p256-x86_64_test.cc b/crypto/fipsmodule/ec/p256-x86_64_test.cc index a802bfb59..4f33de04d 100644 --- a/crypto/fipsmodule/ec/p256-x86_64_test.cc +++ b/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; }