Fix off-by-one in BN_rand
If BN_rand is called with |bits| set to 1 and |top| set to 1 then a 1 byte
buffer overflow can occur.
See also upstream's efee575ad464bfb60bf72dcb73f9b51768f4b1a1. But rather than
making |BN_rand| fail, be consistent with the |bits| = 0 case and just don't
set the bits that don't exist. Add tests to ensure the degenerate cases behave.
Change-Id: I5e9fbe6fd8f7f7b2e011a680f2fbe6d7ed4dab65
Reviewed-on: https://boringssl-review.googlesource.com/4893
Reviewed-by: Adam Langley <agl@google.com>
diff --git a/crypto/bn/bn_test.cc b/crypto/bn/bn_test.cc
index 9aa2bf5..01460ab 100644
--- a/crypto/bn/bn_test.cc
+++ b/crypto/bn/bn_test.cc
@@ -114,6 +114,7 @@
static bool test_dec2bn(FILE *fp, BN_CTX *ctx);
static bool test_hex2bn(FILE *fp, BN_CTX *ctx);
static bool test_asc2bn(FILE *fp, BN_CTX *ctx);
+static bool test_rand();
// g_results can be set to true to cause the result of each computation to be
// printed.
@@ -308,6 +309,12 @@
}
fflush(stdout);
+ message(stdout, "BN_rand");
+ if (!test_rand()) {
+ return 1;
+ }
+ fflush(stdout);
+
printf("PASS\n");
return 0;
}
@@ -1617,3 +1624,47 @@
return true;
}
+
+static bool test_rand() {
+ ScopedBIGNUM bn(BN_new());
+ if (!bn) {
+ return false;
+ }
+
+ // Test BN_rand accounts for degenerate cases with |top| and |bottom|
+ // parameters.
+ if (!BN_rand(bn.get(), 0, 0 /* top */, 0 /* bottom */) ||
+ !BN_is_zero(bn.get())) {
+ fprintf(stderr, "BN_rand gave a bad result.\n");
+ return false;
+ }
+ if (!BN_rand(bn.get(), 0, 1 /* top */, 1 /* bottom */) ||
+ !BN_is_zero(bn.get())) {
+ fprintf(stderr, "BN_rand gave a bad result.\n");
+ return false;
+ }
+
+ if (!BN_rand(bn.get(), 1, 0 /* top */, 0 /* bottom */) ||
+ !BN_is_word(bn.get(), 1)) {
+ fprintf(stderr, "BN_rand gave a bad result.\n");
+ return false;
+ }
+ if (!BN_rand(bn.get(), 1, 1 /* top */, 0 /* bottom */) ||
+ !BN_is_word(bn.get(), 1)) {
+ fprintf(stderr, "BN_rand gave a bad result.\n");
+ return false;
+ }
+ if (!BN_rand(bn.get(), 1, -1 /* top */, 1 /* bottom */) ||
+ !BN_is_word(bn.get(), 1)) {
+ fprintf(stderr, "BN_rand gave a bad result.\n");
+ return false;
+ }
+
+ if (!BN_rand(bn.get(), 2, 1 /* top */, 0 /* bottom */) ||
+ !BN_is_word(bn.get(), 3)) {
+ fprintf(stderr, "BN_rand gave a bad result.\n");
+ return false;
+ }
+
+ return true;
+}
diff --git a/crypto/bn/random.c b/crypto/bn/random.c
index 3be7510..549ac48 100644
--- a/crypto/bn/random.c
+++ b/crypto/bn/random.c
@@ -144,7 +144,7 @@
}
if (top != -1) {
- if (top) {
+ if (top && bits > 1) {
if (bit == 0) {
buf[0] = 1;
buf[1] |= 0x80;
diff --git a/include/openssl/bn.h b/include/openssl/bn.h
index d4c85df..ec1c8ff 100644
--- a/include/openssl/bn.h
+++ b/include/openssl/bn.h
@@ -548,15 +548,15 @@
/* Random and prime number generation. */
-/* BN_rand sets |rnd| to a random number of length |bits|. If |top| is zero,
- * the most-significant bit will be set. If |top| is one, the two most
- * significant bits will be set.
+/* BN_rand sets |rnd| to a random number of length |bits|. If |top| is zero, the
+ * most-significant bit, if any, will be set. If |top| is one, the two most
+ * significant bits, if any, will be set.
*
* If |top| is -1 then no extra action will be taken and |BN_num_bits(rnd)| may
* not equal |bits| if the most significant bits randomly ended up as zeros.
*
- * If |bottom| is non-zero, the least-significant bit will be set. The function
- * returns one on success or zero otherwise. */
+ * If |bottom| is non-zero, the least-significant bit, if any, will be set. The
+ * function returns one on success or zero otherwise. */
OPENSSL_EXPORT int BN_rand(BIGNUM *rnd, int bits, int top, int bottom);
/* BN_pseudo_rand is an alias for |BN_rand|. */
