From 70920e0bef6d67c9c48246347a29722af7161542 Mon Sep 17 00:00:00 2001 From: David Benjamin Date: Mon, 22 Aug 2016 22:19:01 -0700 Subject: 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 ) --- src/crypto/bn/bn_test.cc | 42 +++++++++++++++++ src/crypto/bn/convert.c | 116 +++++++++++++++++++++++------------------------ 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 +#include #include #include #include #include +#include #include #include @@ -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) { -- cgit v1.1