commit 8750fe58f4bd74a3dd1aeba47ace94907d0a7de5
author Adam Langley <agl@chromium.org> Wed Jul 16 10:16:06 2014 -0700
committer Adam Langley <agl@google.com> Wed Jul 16 18:14:32 2014 +0000
tree 2f1603f6ec6e89c16c2755e16e6bfc01a41c44d5
parent 8f3234b2c8efc4aea4e49e0b7e03b29131c99662

base64: fix underflow in EVP_EncodeBlock.

When I switched the base64 code to use size_t, I missed that one of the
loops was counting down, not up, and depended on the loop variable going
negative.

Additionally this change fixes a bug in NETSCAPE_SPKI_b64_encode where
the size of the result buffer was incorrectly calculated and a possible
memory leak.

Change-Id: Ibdf644244291274f50b314f3bb13a61b46858ca1
Reviewed-on: https://boringssl-review.googlesource.com/1220
Reviewed-by: David Benjamin <davidben@chromium.org>
Reviewed-by: Adam Langley <agl@google.com>

crypto/base64/base64.c

diff --git a/crypto/base64/base64.c b/crypto/base64/base64.c
index 94c3055..58e6151 100644
--- a/crypto/base64/base64.c
+++ b/crypto/base64/base64.c
@@ -121,7 +121,7 @@
 
   assert(ctx->length <= sizeof(ctx->enc_data));
 
-  if ((ctx->num + in_len) < ctx->length) {
+  if (ctx->num + in_len < ctx->length) {
     memcpy(&ctx->enc_data[ctx->num], in, in_len);
     ctx->num += in_len;
     return;
@@ -167,26 +167,28 @@
 }
 
 size_t EVP_EncodeBlock(uint8_t *dst, const uint8_t *src, size_t src_len) {
-  unsigned long l;
-  size_t i, ret = 0;
+  uint32_t l;
+  size_t remaining = src_len, ret = 0;
 
-  for (i = src_len; i > 0; i -= 3) {
-    if (i >= 3) {
-      l = (((unsigned long)src[0]) << 16L) | (((unsigned long)src[1]) << 8L) | src[2];
+  while (remaining) {
+    if (remaining >= 3) {
+      l = (((uint32_t)src[0]) << 16L) | (((uint32_t)src[1]) << 8L) | src[2];
       *(dst++) = conv_bin2ascii(l >> 18L);
       *(dst++) = conv_bin2ascii(l >> 12L);
       *(dst++) = conv_bin2ascii(l >> 6L);
       *(dst++) = conv_bin2ascii(l);
+      remaining -= 3;
     } else {
-      l = ((unsigned long)src[0]) << 16L;
-      if (i == 2) {
-        l |= ((unsigned long)src[1] << 8L);
+      l = ((uint32_t)src[0]) << 16L;
+      if (remaining == 2) {
+        l |= ((uint32_t)src[1] << 8L);
       }
 
       *(dst++) = conv_bin2ascii(l >> 18L);
       *(dst++) = conv_bin2ascii(l >> 12L);
-      *(dst++) = (i == 1) ? '=' : conv_bin2ascii(l >> 6L);
+      *(dst++) = (remaining == 1) ? '=' : conv_bin2ascii(l >> 6L);
       *(dst++) = '=';
+      remaining = 0;
     }
     ret += 4;
     src += 3;
@@ -357,7 +359,7 @@
 
 size_t EVP_DecodeBlock(uint8_t *dst, const uint8_t *src, size_t src_len) {
   int a, b, c, d;
-  unsigned long l;
+  uint32_t l;
   size_t i, ret = 0;
 
   /* trim white space from the start of the line. */
@@ -384,8 +386,8 @@
     if ((a & 0x80) || (b & 0x80) || (c & 0x80) || (d & 0x80)) {
       return -1;
     }
-    l = ((((unsigned long)a) << 18L) | (((unsigned long)b) << 12L) |
-         (((unsigned long)c) << 6L) | (((unsigned long)d)));
+    l = ((((uint32_t)a) << 18L) | (((uint32_t)b) << 12L) |
+         (((uint32_t)c) << 6L) | (((uint32_t)d)));
     *(dst++) = (uint8_t)(l >> 16L) & 0xff;
     *(dst++) = (uint8_t)(l >> 8L) & 0xff;
     *(dst++) = (uint8_t)(l) & 0xff;

crypto/x509/x509spki.c

diff --git a/crypto/x509/x509spki.c b/crypto/x509/x509spki.c
index ffadcb4..243a7d2 100644
--- a/crypto/x509/x509spki.c
+++ b/crypto/x509/x509spki.c
@@ -105,8 +105,13 @@
 	int der_len;
 	der_len = i2d_NETSCAPE_SPKI(spki, NULL);
 	der_spki = OPENSSL_malloc(der_len);
-	b64_str = OPENSSL_malloc(der_len * 2);
-	if(!der_spki || !b64_str) {
+	if (der_spki == NULL) {
+		OPENSSL_PUT_ERROR(X509, NETSCAPE_SPKI_b64_encode, ERR_R_MALLOC_FAILURE);
+		return NULL;
+	}
+	b64_str = OPENSSL_malloc((der_len + 2) / 3 * 4 + 1);
+	if (b64_str == NULL) {
+		OPENSSL_free(der_spki);
 		OPENSSL_PUT_ERROR(X509, NETSCAPE_SPKI_b64_encode, ERR_R_MALLOC_FAILURE);
 		return NULL;
 	}