summaryrefslogtreecommitdiffstats
diff options
context:
space:
mode:
authorDavid Benjamin <davidben@google.com>2016-08-22 22:19:01 -0700
committerSean McCreary <mccreary@mcwest.org>2017-03-22 12:17:27 -0600
commit70920e0bef6d67c9c48246347a29722af7161542 (patch)
tree8d56a728e7dd30251707bbdba092771b850e2dd8
parent1c725f9b6cce4af300ac28b902d186e8053c2f97 (diff)
downloadexternal_boringssl-70920e0bef6d67c9c48246347a29722af7161542.tar.gz
external_boringssl-70920e0bef6d67c9c48246347a29722af7161542.tar.bz2
external_boringssl-70920e0bef6d67c9c48246347a29722af7161542.zip
Rewrite BN_bn2dec.
This is a more complete fix for CVE-2016-2182. The original commit message was: "If an oversize BIGNUM is presented to BN_bn2dec() it can cause BN_div_word() to fail and not reduce the value of 't' resulting in OOB writes to the bn_data buffer and eventually crashing. Fix by checking return value of BN_div_word() and checking writes don't overflow buffer. Thanks to Shi Lei for reporting this bug." BoringSSL's rewrite commit message: "958aaf1ea1b481e8ef32970d5b0add80504be4b2, imported from upstream, had an off-by-one error. Reproducing the failure is fairly easy as it can't even serialize 1. See also upstream's 099e2968ed3c7d256cda048995626664082b1b30. Rewrite the function completely with CBB and add a basic test. BUG=chromium:639740" CVE-2016-2182 Change-Id: I41a91514c4bb9e83854824ed5258ffe4e49d9491 Bug: 32096880 (cherry picked from commit 29b92ab938c1a17d4d1b3b039042a0f499f58b5d) (cherry picked from commit 54bf62a81586d99d0a951ca3342d569b59e69b80 with adaptations from <sultanxda@gmail.com>)
-rw-r--r--src/crypto/bn/bn_test.cc42
-rw-r--r--src/crypto/bn/convert.c116
2 files changed, 100 insertions, 58 deletions
diff --git a/src/crypto/bn/bn_test.cc b/src/crypto/bn/bn_test.cc
index 6a7d48c..95b7bbb 100644
--- a/src/crypto/bn/bn_test.cc
+++ b/src/crypto/bn/bn_test.cc
@@ -122,6 +122,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();
+static bool TestBN2Dec();
static const uint8_t kSample[] =
"\xC6\x4F\x43\x04\x2A\xEA\xCA\x6E\x58\x36\x80\x5B\xE8\xC9"
@@ -341,6 +342,12 @@ int main(int argc, char *argv[]) {
}
flush_fp(bc_file.get());
+ message(bc_file.get(), "BN_bn2dec");
+ if (!TestBN2Dec()) {
+ return 1;
+ }
+ flush_fp(bc_file.get());
+
printf("PASS\n");
return 0;
}
@@ -1628,3 +1635,38 @@ static bool test_rand() {
return true;
}
+
+static bool TestBN2Dec() {
+ static const char *kBN2DecTests[] = {
+ "0",
+ "1",
+ "-1",
+ "100",
+ "-100",
+ "123456789012345678901234567890",
+ "-123456789012345678901234567890",
+ "123456789012345678901234567890123456789012345678901234567890",
+ "-123456789012345678901234567890123456789012345678901234567890",
+ };
+
+ for (const char *test : kBN2DecTests) {
+ ScopedBIGNUM bn;
+ int ret = DecimalToBIGNUM(&bn, test);
+ if (ret == 0) {
+ return false;
+ }
+
+ ScopedOpenSSLString dec(BN_bn2dec(bn.get()));
+ if (!dec) {
+ fprintf(stderr, "BN_bn2dec failed on %s.\n", test);
+ return false;
+ }
+
+ if (strcmp(dec.get(), test) != 0) {
+ fprintf(stderr, "BN_bn2dec gave %s, wanted %s.\n", dec.get(), test);
+ return false;
+ }
+ }
+
+ return true;
+}
diff --git a/src/crypto/bn/convert.c b/src/crypto/bn/convert.c
index 531b661..2f0ff8b 100644
--- a/src/crypto/bn/convert.c
+++ b/src/crypto/bn/convert.c
@@ -56,11 +56,13 @@
#include <openssl/bn.h>
+#include <assert.h>
#include <ctype.h>
#include <stdio.h>
#include <string.h>
#include <openssl/bio.h>
+#include <openssl/bytestring.h>
#include <openssl/err.h>
#include <openssl/mem.h>
@@ -348,73 +350,71 @@ int BN_hex2bn(BIGNUM **outp, const char *in) {
}
char *BN_bn2dec(const BIGNUM *a) {
- int i = 0, num, ok = 0;
- char *buf = NULL;
- char *p;
- BIGNUM *t = NULL;
- BN_ULONG *bn_data = NULL, *lp;
-
- /* get an upper bound for the length of the decimal integer
- * num <= (BN_num_bits(a) + 1) * log(2)
- * <= 3 * BN_num_bits(a) * 0.1001 + log(2) + 1 (rounding error)
- * <= BN_num_bits(a)/10 + BN_num_bits/1000 + 1 + 1
- */
- i = BN_num_bits(a) * 3;
- num = i / 10 + i / 1000 + 1 + 1;
- bn_data =
- (BN_ULONG *)OPENSSL_malloc((num / BN_DEC_NUM + 1) * sizeof(BN_ULONG));
- buf = (char *)OPENSSL_malloc(num + 3);
- if ((buf == NULL) || (bn_data == NULL)) {
- OPENSSL_PUT_ERROR(BN, BN_bn2dec, ERR_R_MALLOC_FAILURE);
- goto err;
- }
- t = BN_dup(a);
- if (t == NULL) {
- goto err;
- }
-
-#define BUF_REMAIN (num + 3 - (size_t)(p - buf))
- p = buf;
- lp = bn_data;
- if (BN_is_zero(t)) {
- *(p++) = '0';
- *(p++) = '\0';
+ /* It is easier to print strings little-endian, so we assemble it in reverse
+ * and fix at the end. */
+ BIGNUM *copy = NULL;
+ CBB cbb;
+ if (!CBB_init(&cbb, 16) ||
+ !CBB_add_u8(&cbb, 0 /* trailing NUL */)) {
+ goto cbb_err;
+ }
+
+ if (BN_is_zero(a)) {
+ if (!CBB_add_u8(&cbb, '0')) {
+ goto cbb_err;
+ }
} else {
- if (BN_is_negative(t)) {
- *p++ = '-';
+ copy = BN_dup(a);
+ if (copy == NULL) {
+ goto err;
}
- while (!BN_is_zero(t)) {
- *lp = BN_div_word(t, BN_DEC_CONV);
- lp++;
- }
- lp--;
- /* We now have a series of blocks, BN_DEC_NUM chars
- * in length, where the last one needs truncation.
- * The blocks need to be reversed in order. */
- BIO_snprintf(p, BUF_REMAIN, BN_DEC_FMT1, *lp);
- while (*p) {
- p++;
- }
- while (lp != bn_data) {
- lp--;
- BIO_snprintf(p, BUF_REMAIN, BN_DEC_FMT2, *lp);
- while (*p) {
- p++;
+ while (!BN_is_zero(copy)) {
+ BN_ULONG word = BN_div_word(copy, BN_DEC_CONV);
+ if (word == (BN_ULONG)-1) {
+ goto err;
}
+
+ const int add_leading_zeros = !BN_is_zero(copy);
+ int i;
+ for (i = 0; i < BN_DEC_NUM && (add_leading_zeros || word != 0); i++) {
+ if (!CBB_add_u8(&cbb, '0' + word % 10)) {
+ goto cbb_err;
+ }
+ word /= 10;
+ }
+ assert(word == 0);
}
}
- ok = 1;
-err:
- OPENSSL_free(bn_data);
- BN_free(t);
- if (!ok) {
- OPENSSL_free(buf);
- buf = NULL;
+ if (BN_is_negative(a) &&
+ !CBB_add_u8(&cbb, '-')) {
+ goto cbb_err;
}
- return buf;
+ uint8_t *data;
+ size_t len;
+ if (!CBB_finish(&cbb, &data, &len)) {
+ goto cbb_err;
+ }
+
+ /* Reverse the buffer. */
+ size_t i;
+ for (i = 0; i < len/2; i++) {
+ uint8_t tmp = data[i];
+ data[i] = data[len - 1 - i];
+ data[len - 1 - i] = tmp;
+ }
+
+ BN_free(copy);
+ return (char *)data;
+
+cbb_err:
+ OPENSSL_PUT_ERROR(BN, BN_bn2dec, ERR_R_MALLOC_FAILURE);
+err:
+ BN_free(copy);
+ CBB_cleanup(&cbb);
+ return NULL;
}
int BN_dec2bn(BIGNUM **outp, const char *in) {