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 54bf62a81586d99d0a951ca3342d569b59e69b80)

Author: davidben

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;
+}

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) {