commit 899b9b19a4cd3fe526aaf5047ab9234cdca19f7d
author David Benjamin <davidben@google.com> Tue Aug 02 13:09:19 2016 -0400
committer CQ bot account: commit-bot@chromium.org <commit-bot@chromium.org> Tue Aug 02 18:53:45 2016 +0000
tree 1b13247b2f6b855f64a2a2da9e8d24a601a2f6b3
parent 875bf042372f8d090f20e9c1a476c9544d03abb2

Ensure |BN_div| never gives negative zero in the no_branch code.

Have |bn_correct_top| fix |bn->neg| if the input is zero so that we
don't have negative zeros lying around.

Thanks to Brian Smith for noticing.

Change-Id: I91bcadebc8e353bb29c81c4367e85853886c8e4e
Reviewed-on: https://boringssl-review.googlesource.com/9074
Reviewed-by: Adam Langley <agl@google.com>
Commit-Queue: Adam Langley <agl@google.com>
CQ-Verified: CQ bot account: commit-bot@chromium.org <commit-bot@chromium.org>

crypto/bn/bn.c

diff --git a/crypto/bn/bn.c b/crypto/bn/bn.c
index 0ecaf82..496266f 100644
--- a/crypto/bn/bn.c
+++ b/crypto/bn/bn.c
@@ -342,6 +342,10 @@
     }
     bn->top = tmp_top;
   }
+
+  if (bn->top == 0) {
+    bn->neg = 0;
+  }
 }
 
 int BN_get_flags(const BIGNUM *bn, int flags) {

crypto/bn/bn_test.cc

diff --git a/crypto/bn/bn_test.cc b/crypto/bn/bn_test.cc
index 554d0c6..b35e59b 100644
--- a/crypto/bn/bn_test.cc
+++ b/crypto/bn/bn_test.cc
@@ -1123,8 +1123,7 @@
   ScopedBIGNUM a(BN_new());
   ScopedBIGNUM b(BN_new());
   ScopedBIGNUM c(BN_new());
-  ScopedBIGNUM d(BN_new());
-  if (!a || !b || !c || !d) {
+  if (!a || !b || !c) {
     return false;
   }
 
@@ -1142,30 +1141,42 @@
     return false;
   }
 
-  // Test that BN_div never gives negative zero in the quotient.
-  if (!BN_set_word(a.get(), 1) ||
-      !BN_set_word(b.get(), 2)) {
-    return false;
-  }
-  BN_set_negative(a.get(), 1);
-  if (!BN_div(d.get(), c.get(), a.get(), b.get(), ctx)) {
-    return false;
-  }
-  if (!BN_is_zero(d.get()) || BN_is_negative(d.get())) {
-    fprintf(stderr, "Division test failed.\n");
-    return false;
-  }
+  for (int consttime = 0; consttime < 2; consttime++) {
+    ScopedBIGNUM numerator(BN_new()), denominator(BN_new());
+    if (!numerator || !denominator) {
+      return false;
+    }
 
-  // Test that BN_div never gives negative zero in the remainder.
-  if (!BN_set_word(b.get(), 1)) {
-    return false;
-  }
-  if (!BN_div(d.get(), c.get(), a.get(), b.get(), ctx)) {
-    return false;
-  }
-  if (!BN_is_zero(c.get()) || BN_is_negative(c.get())) {
-    fprintf(stderr, "Division test failed.\n");
-    return false;
+    if (consttime) {
+      BN_set_flags(numerator.get(), BN_FLG_CONSTTIME);
+      BN_set_flags(denominator.get(), BN_FLG_CONSTTIME);
+    }
+
+    // Test that BN_div never gives negative zero in the quotient.
+    if (!BN_set_word(numerator.get(), 1) ||
+        !BN_set_word(denominator.get(), 2)) {
+      return false;
+    }
+    BN_set_negative(numerator.get(), 1);
+    if (!BN_div(a.get(), b.get(), numerator.get(), denominator.get(), ctx)) {
+      return false;
+    }
+    if (!BN_is_zero(a.get()) || BN_is_negative(a.get())) {
+      fprintf(stderr, "Incorrect quotient (consttime = %d).\n", consttime);
+      return false;
+    }
+
+    // Test that BN_div never gives negative zero in the remainder.
+    if (!BN_set_word(denominator.get(), 1)) {
+      return false;
+    }
+    if (!BN_div(a.get(), b.get(), numerator.get(), denominator.get(), ctx)) {
+      return false;
+    }
+    if (!BN_is_zero(b.get()) || BN_is_negative(b.get())) {
+      fprintf(stderr, "Incorrect remainder (consttime = %d).\n", consttime);
+      return false;
+    }
   }
 
   // Test that BN_set_negative will not produce a negative zero.

include/openssl/bn.h

diff --git a/include/openssl/bn.h b/include/openssl/bn.h
index a8536ef..034b877 100644
--- a/include/openssl/bn.h
+++ b/include/openssl/bn.h
@@ -323,7 +323,7 @@
  * what you want before turning to these. */
 
 /* bn_correct_top decrements |bn->top| until |bn->d[top-1]| is non-zero or
- * until |top| is zero. */
+ * until |top| is zero. If |bn| is zero, |bn->neg| is set to zero. */
 OPENSSL_EXPORT void bn_correct_top(BIGNUM *bn);
 
 /* bn_wexpand ensures that |bn| has at least |words| works of space without