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>
diff --git a/crypto/bn/bn.c b/crypto/bn/bn.c
index 2263701..d960f12 100644
--- a/crypto/bn/bn.c
+++ b/crypto/bn/bn.c
@@ -266,6 +266,17 @@
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 1e78dc2..0a2982c 100644
--- a/crypto/bn/internal.h
+++ b/crypto/bn/internal.h
@@ -192,6 +192,11 @@
#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 acbf455..f82830f 100644
--- a/crypto/ec/p256-x86_64.c
+++ b/crypto/ec/p256-x86_64.c
@@ -390,17 +390,6 @@
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 @@
}
}
- 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;