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"
Change-Id: I41a91514c4bb9e83854824ed5258ffe4e49d9491
Bug: 32096880
(cherry picked from commit 29b92ab938c1a17d4d1b3b039042a0f499f58b5d)
diff --git a/src/crypto/bn/bn_test.cc b/src/crypto/bn/bn_test.cc
index e7e04f1..b00b420 100644
--- a/src/crypto/bn/bn_test.cc
+++ b/src/crypto/bn/bn_test.cc
@@ -127,6 +127,7 @@
static bool test_mpi();
static bool test_rand();
static bool test_asn1();
+static bool TestBN2Dec();
static const uint8_t kSample[] =
"\xC6\x4F\x43\x04\x2A\xEA\xCA\x6E\x58\x36\x80\x5B\xE8\xC9"
@@ -326,6 +327,12 @@
return 1;
}
+ message(bc_file.get(), "BN_bn2dec");
+ if (!TestBN2Dec()) {
+ return 1;
+ }
+ flush_fp(bc_file.get());
+
printf("PASS\n");
return 0;
}
@@ -1935,3 +1942,38 @@
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 1f7af64..8646e59 100644
--- a/src/crypto/bn/convert.c
+++ b/src/crypto/bn/convert.c
@@ -372,73 +372,71 @@
}
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, ERR_R_MALLOC_FAILURE);
- goto err;
- }
- t = BN_dup(a);
- if (t == NULL) {
- goto err;
+ /* 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;
}
-#define BUF_REMAIN (num + 3 - (size_t)(p - buf))
- p = buf;
- lp = bn_data;
- if (BN_is_zero(t)) {
- *(p++) = '0';
- *(p++) = '\0';
+ 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, ERR_R_MALLOC_FAILURE);
+err:
+ BN_free(copy);
+ CBB_cleanup(&cbb);
+ return NULL;
}
int BN_dec2bn(BIGNUM **outp, const char *in) {
