commit 20b6a4e2a18e6a2c961ae0cd9945b69ad42dc64e
author David Benjamin <davidben@google.com> Fri Jul 20 19:29:14 2018 -0400
committer CQ bot account: commit-bot@chromium.org <commit-bot@chromium.org> Fri Jul 20 23:45:06 2018 +0000
tree b61582514826d5c24c368155964579b35e7ec3c6
parent d154c7ccbc133c5e69a3a5281409ee50754eebe0

Clear r->neg in bn_mod_{add,sub}_consttime.

Otherwise, if the output BIGNUM was previously negative, we'd incorrectly give
a negative result. Thanks to Guide Vranken for reporting this issue!

Fortunately, this does not appear to come up in any existing caller. This isn't
all that surprising as negative numbers never really come up in cryptography.
Were it not for OpenSSL historically designing a calculator API, we'd just
delete the bit altogether. :-(

Bug: chromium:865924
Change-Id: I28fdc986dfaba3e38435b14ebf07453d537cc60a
Reviewed-on: https://boringssl-review.googlesource.com/29944
Commit-Queue: David Benjamin <davidben@google.com>
Commit-Queue: Adam Langley <agl@google.com>
Reviewed-by: Adam Langley <agl@google.com>
CQ-Verified: CQ bot account: commit-bot@chromium.org <commit-bot@chromium.org>

crypto/fipsmodule/bn/bn_test.cc

diff --git a/crypto/fipsmodule/bn/bn_test.cc b/crypto/fipsmodule/bn/bn_test.cc
index a25d487..27fd5c7 100644
--- a/crypto/fipsmodule/bn/bn_test.cc
+++ b/crypto/fipsmodule/bn/bn_test.cc
@@ -2346,3 +2346,28 @@
   ASSERT_TRUE(bn_resize_words(bn.get(), 16));
   EXPECT_EQ(0, BN_count_low_zero_bits(bn.get()));
 }
+
+TEST_F(BNTest, WriteIntoNegative) {
+  bssl::UniquePtr<BIGNUM> r(BN_new());
+  ASSERT_TRUE(r);
+  bssl::UniquePtr<BIGNUM> two(BN_new());
+  ASSERT_TRUE(two);
+  ASSERT_TRUE(BN_set_word(two.get(), 2));
+  bssl::UniquePtr<BIGNUM> three(BN_new());
+  ASSERT_TRUE(three);
+  ASSERT_TRUE(BN_set_word(three.get(), 3));
+  bssl::UniquePtr<BIGNUM> seven(BN_new());
+  ASSERT_TRUE(seven);
+  ASSERT_TRUE(BN_set_word(seven.get(), 7));
+
+  ASSERT_TRUE(BN_set_word(r.get(), 1));
+  BN_set_negative(r.get(), 1);
+  ASSERT_TRUE(BN_mod_add_quick(r.get(), two.get(), three.get(), seven.get()));
+  EXPECT_TRUE(BN_is_word(r.get(), 5));
+  EXPECT_FALSE(BN_is_negative(r.get()));
+
+  BN_set_negative(r.get(), 1);
+  ASSERT_TRUE(BN_mod_sub_quick(r.get(), two.get(), three.get(), seven.get()));
+  EXPECT_TRUE(BN_is_word(r.get(), 6));
+  EXPECT_FALSE(BN_is_negative(r.get()));
+}

crypto/fipsmodule/bn/div.c

diff --git a/crypto/fipsmodule/bn/div.c b/crypto/fipsmodule/bn/div.c
index a350fbf..57485bd 100644
--- a/crypto/fipsmodule/bn/div.c
+++ b/crypto/fipsmodule/bn/div.c
@@ -591,6 +591,7 @@
   if (ok) {
     bn_mod_add_words(r->d, a->d, b->d, m->d, tmp->d, m->width);
     r->width = m->width;
+    r->neg = 0;
   }
   BN_CTX_end(ctx);
   return ok;
@@ -615,6 +616,7 @@
   if (ok) {
     bn_mod_sub_words(r->d, a->d, b->d, m->d, tmp->d, m->width);
     r->width = m->width;
+    r->neg = 0;
   }
   BN_CTX_end(ctx);
   return ok;