From 8750fe58f4bd74a3dd1aeba47ace94907d0a7de5 Mon Sep 17 00:00:00 2001 From: Adam Langley Date: Wed, 16 Jul 2014 10:16:06 -0700 Subject: [PATCH] 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 Reviewed-by: Adam Langley --- crypto/base64/base64.c | 28 +++++++++++++++------------- crypto/x509/x509spki.c | 9 +++++++-- 2 files changed, 22 insertions(+), 15 deletions(-) diff --git a/crypto/base64/base64.c b/crypto/base64/base64.c index 94c3055c..58e6151b 100644 --- a/crypto/base64/base64.c +++ b/crypto/base64/base64.c @@ -121,7 +121,7 @@ void EVP_EncodeUpdate(EVP_ENCODE_CTX *ctx, uint8_t *out, int *out_len, 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 @@ void EVP_EncodeFinal(EVP_ENCODE_CTX *ctx, uint8_t *out, int *out_len) { } 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 @@ int EVP_DecodeFinal(EVP_ENCODE_CTX *ctx, uint8_t *out, int *outl) { 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 @@ size_t EVP_DecodeBlock(uint8_t *dst, const uint8_t *src, size_t src_len) { 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; diff --git a/crypto/x509/x509spki.c b/crypto/x509/x509spki.c index ffadcb4f..243a7d26 100644 --- a/crypto/x509/x509spki.c +++ b/crypto/x509/x509spki.c @@ -105,8 +105,13 @@ char * NETSCAPE_SPKI_b64_encode(NETSCAPE_SPKI *spki) 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; }