From d279a21d8c7c39e603dd3d7922afa219fbbc713b Mon Sep 17 00:00:00 2001
From: Brian Smith <brian@briansmith.org>
Date: Tue, 8 Mar 2016 17:09:40 -1000
Subject: [PATCH] Avoid potential uninitialized memory read in
 crypto/ec/p256-x86_64.c.

If the function returns early due to an error, then the coordinates of the
result will have their |top| value set to a value beyond what has actually
been been written. Fix that, and make it easier to avoid such issues in the
future by refactoring the code.

As a bonus, avoid a false positive MSVC 64-bit opt build "potentially
uninitialized value used" warning.

Change-Id: I8c48deb63163a27f739c8797962414f8ca2588cd
Reviewed-on: https://boringssl-review.googlesource.com/6579
Reviewed-by: David Benjamin <davidben@google.com>
---
 crypto/bn/bn.c          | 11 +++++++++++
 crypto/bn/internal.h    |  5 +++++
 crypto/ec/p256-x86_64.c | 23 +++++------------------
 3 files changed, 21 insertions(+), 18 deletions(-)

diff --git a/crypto/bn/bn.c b/crypto/bn/bn.c
index 2263701f..d960f121 100644
--- a/crypto/bn/bn.c
+++ b/crypto/bn/bn.c
@@ -266,6 +266,17 @@ int BN_set_word(BIGNUM *bn, BN_ULONG value) {
   return 1;
 }
 
+int bn_set_words(BIGNUM *bn, const BN_ULONG *words, size_t num) {
+  if (bn_wexpand(bn, num) == NULL) {
+    return 0;
+  }
+  memmove(bn->d, words, num * sizeof(BN_ULONG));
+  /* |bn_wexpand| verified that |num| isn't too large. */
+  bn->top = (int)num;
+  bn_correct_top(bn);
+  return 1;
+}
+
 int BN_is_negative(const BIGNUM *bn) {
   return bn->neg != 0;
 }
diff --git a/crypto/bn/internal.h b/crypto/bn/internal.h
index 1e78dc2c..0a2982c7 100644
--- a/crypto/bn/internal.h
+++ b/crypto/bn/internal.h
@@ -192,6 +192,11 @@ BIGNUM *bn_expand(BIGNUM *bn, size_t bits);
 #define Hw(t) (((BN_ULONG)((t)>>BN_BITS2))&BN_MASK2)
 #endif
 
+
+/* bn_set_words sets |bn| to the value encoded in the |num| words in |words|,
+ * least significant word first. */
+int bn_set_words(BIGNUM *bn, const BN_ULONG *words, size_t num);
+
 BN_ULONG bn_mul_add_words(BN_ULONG *rp, const BN_ULONG *ap, int num, BN_ULONG w);
 BN_ULONG bn_mul_words(BN_ULONG *rp, const BN_ULONG *ap, int num, BN_ULONG w);
 void     bn_sqr_words(BN_ULONG *rp, const BN_ULONG *ap, int num);
diff --git a/crypto/ec/p256-x86_64.c b/crypto/ec/p256-x86_64.c
index acbf4550..f82830f6 100644
--- a/crypto/ec/p256-x86_64.c
+++ b/crypto/ec/p256-x86_64.c
@@ -390,17 +390,6 @@ static int ecp_nistz256_points_mul(
   BN_CTX *new_ctx = NULL;
   int ctx_started = 0;
 
-  /* Need 256 bits for space for all coordinates. */
-  if (bn_wexpand(&r->X, P256_LIMBS) == NULL ||
-      bn_wexpand(&r->Y, P256_LIMBS) == NULL ||
-      bn_wexpand(&r->Z, P256_LIMBS) == NULL) {
-    OPENSSL_PUT_ERROR(EC, ERR_R_MALLOC_FAILURE);
-    goto err;
-  }
-  r->X.top = P256_LIMBS;
-  r->Y.top = P256_LIMBS;
-  r->Z.top = P256_LIMBS;
-
   if (g_scalar != NULL) {
     if (BN_num_bits(g_scalar) > 256 || BN_is_negative(g_scalar)) {
       if (ctx == NULL) {
@@ -494,14 +483,12 @@ static int ecp_nistz256_points_mul(
     }
   }
 
-  memcpy(r->X.d, p.p.X, sizeof(p.p.X));
-  memcpy(r->Y.d, p.p.Y, sizeof(p.p.Y));
-  memcpy(r->Z.d, p.p.Z, sizeof(p.p.Z));
-
   /* Not constant-time, but we're only operating on the public output. */
-  bn_correct_top(&r->X);
-  bn_correct_top(&r->Y);
-  bn_correct_top(&r->Z);
+  if (!bn_set_words(&r->X, p.p.X, P256_LIMBS) ||
+      !bn_set_words(&r->Y, p.p.Y, P256_LIMBS) ||
+      !bn_set_words(&r->Z, p.p.Z, P256_LIMBS)) {
+    return 0;
+  }
 
   ret = 1;