Merge pull request #1366 from gilles-peskine-arm/base64-decode-clean-3.6

Backport 3.6: mbedtls_base64_decode: fix sloppiness

ChangeLog.d/base64_decode.txt

@@ -0,0 +1,8 @@
+Bugfix
+   * Fix mbedtls_base64_decode() on inputs that did not have the correct
+     number of trailing equal signs, or had 4*k+1 digits. They were accepted
+     as long as they had at most two trailing equal signs. They are now
+     rejected. Furthermore, before, on inputs with too few equal signs, the
+     function reported the correct size in *olen when it returned
+     MBEDTLS_ERR_BASE64_BUFFER_TOO_SMALL, but truncated the output to the
+     last multiple of 3 bytes.

library/base64.c

@@ -14,6 +14,7 @@
 #include "mbedtls/base64.h"
 #include "base64_internal.h"
 #include "constant_time_internal.h"
+#include "mbedtls/error.h"
 
 #include <stdint.h>
 
@@ -183,49 +184,72 @@ int mbedtls_base64_decode(unsigned char *dst, size_t dlen, size_t *olen,
         n++;
     }
 
-    if (n == 0) {
+    /* In valid base64, the number of digits (n-equals) is always of the form
-        *olen = 0;
+     * 4*k, 4*k+2 or *4k+3. Also, the number n of digits plus the number of
-        return 0;
+     * equal signs at the end is always a multiple of 4. */
+    if ((n - equals) % 4 == 1) {
+        return MBEDTLS_ERR_BASE64_INVALID_CHARACTER;
+    }
+    if (n % 4 != 0) {
+        return MBEDTLS_ERR_BASE64_INVALID_CHARACTER;
     }
 
-    /* The following expression is to calculate the following formula without
+    /* We've determined that the input is valid, and that it contains
-     * risk of integer overflow in n:
+     * exactly k blocks of digits-or-equals, with n = 4 * k,
-     *     n = ( ( n * 6 ) + 7 ) >> 3;
+     * and equals only present at the end of the last block if at all.
+     * Now we can calculate the length of the output.
+     *
+     * Each block of 4 digits in the input map to 3 bytes of output.
+     * For the last block:
+     * - abcd (where abcd are digits) is a full 3-byte block;
+     * - abc= means 1 byte less than a full 3-byte block of output;
+     * - ab== means 2 bytes less than a full 3-byte block of output;
+     * - a==== and ==== is rejected above.
      */
-    n = (6 * (n >> 3)) + ((6 * (n & 0x7) + 7) >> 3);
+    *olen = (n / 4) * 3 - equals;
-    n -= equals;
 
-    if (dst == NULL || dlen < n) {
+    /* If the output buffer is too small, signal this and stop here.
-        *olen = n;
+     * Also, as documented, stop here if `dst` is null, independently of
+     * `dlen`.
+     *
+     * There is an edge case when the output is empty: in this case,
+     * `dlen == 0` with `dst == NULL` is valid (on some platforms,
+     * `malloc(0)` returns `NULL`). Since the call is valid, we return
+     * 0 in this case.
+     */
+    if ((*olen != 0 && dst == NULL) || dlen < *olen) {
         return MBEDTLS_ERR_BASE64_BUFFER_TOO_SMALL;
     }
 
-    equals = 0;
     for (x = 0, p = dst; i > 0; i--, src++) {
         if (*src == '\r' || *src == '\n' || *src == ' ') {
             continue;
         }
-
-        x = x << 6;
         if (*src == '=') {
-            ++equals;
+            /* We already know from the first loop that equal signs are
-        } else {
+             * only at the end. */
-            x |= mbedtls_ct_base64_dec_value(*src);
+            break;
         }
+        x = x << 6;
+        x |= mbedtls_ct_base64_dec_value(*src);
 
         if (++accumulated_digits == 4) {
             accumulated_digits = 0;
             *p++ = MBEDTLS_BYTE_2(x);
-            if (equals <= 1) {
+            *p++ = MBEDTLS_BYTE_1(x);
-                *p++ = MBEDTLS_BYTE_1(x);
+            *p++ = MBEDTLS_BYTE_0(x);
-            }
-            if (equals <= 0) {
-                *p++ = MBEDTLS_BYTE_0(x);
-            }
         }
     }
+    if (accumulated_digits == 3) {
+        *p++ = MBEDTLS_BYTE_2(x << 6);
+        *p++ = MBEDTLS_BYTE_1(x << 6);
+    } else if (accumulated_digits == 2) {
+        *p++ = MBEDTLS_BYTE_2(x << 12);
+    }
 
-    *olen = (size_t) (p - dst);
+    if (*olen != (size_t) (p - dst)) {
+        return MBEDTLS_ERR_ERROR_CORRUPTION_DETECTED;
+    }
 
     return 0;
 }

tests/suites/test_suite_base64.data

@@ -76,6 +76,107 @@ mbedtls_base64_decode:"zm=masd":"":MBEDTLS_ERR_BASE64_INVALID_CHARACTER
 Base64 decode (Space inside string)
 mbedtls_base64_decode:"zm masd":"":MBEDTLS_ERR_BASE64_INVALID_CHARACTER
 
+# The next few test cases validate systematically for short inputs that
+# we require the correct number of trailing equal signs.
+
+# 4k+1 digits is always wrong (wouldn't encode more bytes than 4k digits)
+Base64 decode: 1 digit, 0 equals (bad)
+mbedtls_base64_decode:"Y":"":MBEDTLS_ERR_BASE64_INVALID_CHARACTER
+
+Base64 decode: 1 digit, 1 equals (bad)
+mbedtls_base64_decode:"Y=":"":MBEDTLS_ERR_BASE64_INVALID_CHARACTER
+
+Base64 decode: 1 digit, 2 equals (bad)
+mbedtls_base64_decode:"Y==":"":MBEDTLS_ERR_BASE64_INVALID_CHARACTER
+
+Base64 decode: 1 digit, 3 equals (bad)
+mbedtls_base64_decode:"Y===":"":MBEDTLS_ERR_BASE64_INVALID_CHARACTER
+
+Base64 decode: 2 digits, 0 equals (bad)
+mbedtls_base64_decode:"Yw":"c":MBEDTLS_ERR_BASE64_INVALID_CHARACTER
+
+Base64 decode: 2 digits, 1 equals (bad)
+mbedtls_base64_decode:"Yw=":"c":MBEDTLS_ERR_BASE64_INVALID_CHARACTER
+
+Base64 decode: 2 digits, 2 equals (good)
+mbedtls_base64_decode:"Yw==":"c":0
+
+Base64 decode: 2 digits, 3 equals (bad)
+mbedtls_base64_decode:"Yw===":"c":MBEDTLS_ERR_BASE64_INVALID_CHARACTER
+
+Base64 decode: 3 digits, 0 equals (bad)
+mbedtls_base64_decode:"Y28":"co":MBEDTLS_ERR_BASE64_INVALID_CHARACTER
+
+Base64 decode: 3 digits, 1 equals (good)
+mbedtls_base64_decode:"Y28=":"co":0
+
+Base64 decode: 3 digits, 2 equals (bad)
+mbedtls_base64_decode:"Y28==":"co":MBEDTLS_ERR_BASE64_INVALID_CHARACTER
+
+Base64 decode: 3 digits, 3 equals (bad)
+mbedtls_base64_decode:"Y28===":"co":MBEDTLS_ERR_BASE64_INVALID_CHARACTER
+
+Base64 decode: 4 digits, 0 equals (good)
+mbedtls_base64_decode:"Y29t":"com":0
+
+Base64 decode: 4 digits, 1 equals (bad)
+mbedtls_base64_decode:"Y29t=":"com":MBEDTLS_ERR_BASE64_INVALID_CHARACTER
+
+Base64 decode: 4 digits, 2 equals (bad)
+mbedtls_base64_decode:"Y29t==":"com":MBEDTLS_ERR_BASE64_INVALID_CHARACTER
+
+Base64 decode: 4 digits, 3 equals (bad)
+mbedtls_base64_decode:"Y29t===":"com":MBEDTLS_ERR_BASE64_INVALID_CHARACTER
+
+# 4k+1 digits is always wrong (wouldn't encode more bytes than 4k digits)
+Base64 decode: 5 digits, 0 equals (bad)
+mbedtls_base64_decode:"Y29tc":"":MBEDTLS_ERR_BASE64_INVALID_CHARACTER
+
+Base64 decode: 5 digits, 1 equals (bad)
+mbedtls_base64_decode:"Y29tc=":"":MBEDTLS_ERR_BASE64_INVALID_CHARACTER
+
+Base64 decode: 5 digits, 2 equals (bad)
+mbedtls_base64_decode:"Y29tc==":"":MBEDTLS_ERR_BASE64_INVALID_CHARACTER
+
+Base64 decode: 5 digits, 3 equals (bad)
+mbedtls_base64_decode:"Y29tc===":"com":MBEDTLS_ERR_BASE64_INVALID_CHARACTER
+
+Base64 decode: 6 digits, 0 equals (bad)
+mbedtls_base64_decode:"Y29tcA":"comp":MBEDTLS_ERR_BASE64_INVALID_CHARACTER
+
+Base64 decode: 6 digits, 1 equals (bad)
+mbedtls_base64_decode:"Y29tcA=":"comp":MBEDTLS_ERR_BASE64_INVALID_CHARACTER
+
+Base64 decode: 6 digits, 2 equals (good)
+mbedtls_base64_decode:"Y29tcA==":"comp":0
+
+Base64 decode: 6 digits, 3 equals (bad)
+mbedtls_base64_decode:"Y29tcA===":"comp":MBEDTLS_ERR_BASE64_INVALID_CHARACTER
+
+Base64 decode: 7 digits, 0 equals (bad)
+mbedtls_base64_decode:"Y29tcG8":"compo":MBEDTLS_ERR_BASE64_INVALID_CHARACTER
+
+Base64 decode: 7 digits, 1 equals (good)
+mbedtls_base64_decode:"Y29tcG8=":"compo":0
+
+Base64 decode: 7 digits, 2 equals (bad)
+mbedtls_base64_decode:"Y29tcG8==":"compo":MBEDTLS_ERR_BASE64_INVALID_CHARACTER
+
+Base64 decode: 7 digits, 3 equals (bad)
+mbedtls_base64_decode:"Y29tcG8===":"compo":MBEDTLS_ERR_BASE64_INVALID_CHARACTER
+
+Base64 decode: 8 digits, 0 equals (good)
+mbedtls_base64_decode:"Y29tcG9z":"compos":0
+
+Base64 decode: 8 digits, 1 equals (bad)
+mbedtls_base64_decode:"Y29tcG9z=":"compos":MBEDTLS_ERR_BASE64_INVALID_CHARACTER
+
+Base64 decode: 8 digits, 2 equals (bad)
+mbedtls_base64_decode:"Y29tcG9z==":"compos":MBEDTLS_ERR_BASE64_INVALID_CHARACTER
+
+Base64 decode: 8 digits, 3 equals (bad)
+mbedtls_base64_decode:"Y29tcG9z===":"compos":MBEDTLS_ERR_BASE64_INVALID_CHARACTER
+
 Base64 decode "Zm9vYmFy" (no newline nor '\0' at end)
 base64_decode_hex_src:"5a6d3976596d4679":"foobar":0
 

tests/suites/test_suite_base64.function

@@ -85,20 +85,67 @@ void mbedtls_base64_encode(char *src_string, char *dst_string,
 /* BEGIN_CASE */
 void mbedtls_base64_decode(char *src_string, char *dst_string, int result)
 {
-    unsigned char src_str[1000];
+    unsigned char *src = NULL;
-    unsigned char dst_str[1000];
+    size_t src_len = strlen(src_string);
+    unsigned char *dst = NULL;
+    size_t correct_dst_len = strlen(dst_string);
+    size_t dst_size = correct_dst_len;
     size_t len;
-    int res;
 
-    memset(src_str, 0x00, 1000);
+    /* Allocate exactly the size of the input, to ensure there's no buffer
-    memset(dst_str, 0x00, 1000);
+     * overread in builds with ASan. (src_string has at least one extra null
-
+     * character at the end.) */
-    strncpy((char *) src_str, src_string, sizeof(src_str) - 1);
+    TEST_CALLOC(src, src_len);
-    res = mbedtls_base64_decode(dst_str, sizeof(dst_str), &len, src_str, strlen((char *) src_str));
+    if (src_len != 0) {
-    TEST_ASSERT(res == result);
+        memcpy(src, src_string, src_len);
-    if (result == 0) {
-        TEST_ASSERT(strcmp((char *) dst_str, dst_string) == 0);
     }
+
+    /* Allocate exactly the size of the input, to ensure there's no buffer
+     * overflow in builds with ASan. */
+    TEST_CALLOC(dst, dst_size);
+
+    /* Test normal operation */
+    TEST_EQUAL(mbedtls_base64_decode(dst, dst_size, &len,
+                                     src, src_len),
+               result);
+    if (result == 0) {
+        TEST_MEMORY_COMPARE(dst_string, correct_dst_len, dst, len);
+    }
+
+    /* Test an output buffer that's one byte too small */
+    if (result == 0 && dst_size != 0) {
+        mbedtls_free(dst);
+        dst = NULL;
+        TEST_CALLOC(dst, dst_size - 1);
+        TEST_EQUAL(mbedtls_base64_decode(dst, dst_size - 1, &len,
+                                         src, src_len),
+                   MBEDTLS_ERR_BASE64_BUFFER_TOO_SMALL);
+        TEST_EQUAL(correct_dst_len, len);
+    }
+
+    /* Test an empty output buffer. `mbedtls_base64_decode()` must return
+     * `BUFFER_TOO_SMALL` but report the correct output length.
+     * Skip this when dst_size==0 since that would be a valid call to
+     * `mbedtls_base64_decode()` which should return 0.
+     */
+    if (result == 0 && dst_size != 0) {
+        TEST_EQUAL(mbedtls_base64_decode(NULL, 0, &len,
+                                         src, src_len),
+                   MBEDTLS_ERR_BASE64_BUFFER_TOO_SMALL);
+        TEST_EQUAL(correct_dst_len, len);
+    }
+
+    /* Test dst=NULL with dlen!=0 (explicitly documented as supported) */
+    if (result == 0 && dst_size != 0) {
+        TEST_EQUAL(mbedtls_base64_decode(NULL, 42, &len,
+                                         src, src_len),
+                   MBEDTLS_ERR_BASE64_BUFFER_TOO_SMALL);
+        TEST_EQUAL(correct_dst_len, len);
+    }
+
+exit:
+    mbedtls_free(src);
+    mbedtls_free(dst);
 }
 /* END_CASE */
 

tests/suites/test_suite_pem.data

@@ -49,13 +49,21 @@ PEM read (malformed PEM DES-EDE3-CBC)
 depends_on:MBEDTLS_MD_CAN_MD5:MBEDTLS_DES_C:MBEDTLS_CIPHER_MODE_CBC
 mbedtls_pem_read_buffer:"-----BEGIN EC PRIVATE KEY-----":"-----END EC PRIVATE KEY-----":"-----BEGIN EC PRIVATE KEY-----\nProc-Type\: 4,ENCRYPTED\nDEK-Info\: DES-EDE3-CBC,AA94892A169FA426\n\nMAAA\n-----END EC PRIVATE KEY-----":"pwd":MBEDTLS_ERR_DES_INVALID_INPUT_LENGTH:""
 
-PEM read (malformed PEM AES-128-CBC)
+PEM read (malformed PEM AES-128-CBC: 3-byte ciphertext)
 depends_on:MBEDTLS_MD_CAN_MD5:MBEDTLS_AES_C:MBEDTLS_CIPHER_MODE_CBC
 mbedtls_pem_read_buffer:"-----BEGIN EC PRIVATE KEY-----":"-----END EC PRIVATE KEY-----":"-----BEGIN EC PRIVATE KEY-----\nProc-Type\: 4,ENCRYPTED\nDEK-Info\: AES-128-CBC,AA94892A169FA426AA94892A169FA426\n\nMAAA\n-----END EC PRIVATE KEY-----":"pwd":MBEDTLS_ERR_AES_INVALID_INPUT_LENGTH:""
 
-PEM read (malformed PEM AES-128-CBC with fewer than 4 base64 chars)
+PEM read (malformed PEM AES-128-CBC: 1-byte ciphertext)
 depends_on:MBEDTLS_MD_CAN_MD5:MBEDTLS_AES_C:MBEDTLS_CIPHER_MODE_CBC
-mbedtls_pem_read_buffer:"-----BEGIN EC PRIVATE KEY-----":"-----END EC PRIVATE KEY-----":"-----BEGIN EC PRIVATE KEY-----\nProc-Type\: 4,ENCRYPTED\nDEK-Info\: AES-128-CBC,7BA38DE00F67851E4207216809C3BB15\n\n8Q-----END EC PRIVATE KEY-----":"pwd":MBEDTLS_ERR_PEM_INVALID_DATA:""
+mbedtls_pem_read_buffer:"-----BEGIN EC PRIVATE KEY-----":"-----END EC PRIVATE KEY-----":"-----BEGIN EC PRIVATE KEY-----\nProc-Type\: 4,ENCRYPTED\nDEK-Info\: AES-128-CBC,7BA38DE00F67851E4207216809C3BB15\n\n8Q==-----END EC PRIVATE KEY-----":"pwd":MBEDTLS_ERR_AES_INVALID_INPUT_LENGTH:""
+
+PEM read (malformed PEM AES-128-CBC: empty ciphertext)
+depends_on:MBEDTLS_MD_CAN_MD5:MBEDTLS_AES_C:MBEDTLS_CIPHER_MODE_CBC
+mbedtls_pem_read_buffer:"-----BEGIN EC PRIVATE KEY-----":"-----END EC PRIVATE KEY-----":"-----BEGIN EC PRIVATE KEY-----\nProc-Type\: 4,ENCRYPTED\nDEK-Info\: AES-128-CBC,7BA38DE00F67851E4207216809C3BB15\n\n-----END EC PRIVATE KEY-----":"pwd":MBEDTLS_ERR_PEM_BAD_INPUT_DATA:""
+
+PEM read (malformed PEM AES-128-CBC: base64 with missing equals)
+depends_on:MBEDTLS_MD_CAN_MD5:MBEDTLS_AES_C:MBEDTLS_CIPHER_MODE_CBC
+mbedtls_pem_read_buffer:"-----BEGIN EC PRIVATE KEY-----":"-----END EC PRIVATE KEY-----":"-----BEGIN EC PRIVATE KEY-----\nProc-Type\: 4,ENCRYPTED\nDEK-Info\: AES-128-CBC,7BA38DE00F67851E4207216809C3BB15\n\n8Q-----END EC PRIVATE KEY-----":"pwd":MBEDTLS_ERR_PEM_INVALID_DATA + MBEDTLS_ERR_BASE64_INVALID_CHARACTER:""
 
 # The output sequence's length is not multiple of block size (16 bytes). This
 # proves that the pem_context->len value is properly updated based on the SEQUENCE

tests/suites/test_suite_pem.function

@@ -15,16 +15,16 @@ void mbedtls_pem_write_buffer(char *start, char *end, data_t *buf,
 
 
     ret = mbedtls_pem_write_buffer(start, end, buf->x, buf->len, NULL, 0, &olen);
-    TEST_ASSERT(ret == MBEDTLS_ERR_BASE64_BUFFER_TOO_SMALL);
+    TEST_EQUAL(ret, MBEDTLS_ERR_BASE64_BUFFER_TOO_SMALL);
 
     check_buf = (unsigned char *) mbedtls_calloc(1, olen);
     TEST_ASSERT(check_buf != NULL);
 
     ret = mbedtls_pem_write_buffer(start, end, buf->x, buf->len, check_buf, olen, &olen2);
 
-    TEST_ASSERT(olen2 <= olen);
+    TEST_LE_U(olen2, olen);
-    TEST_ASSERT(olen > strlen((char *) result_str));
+    TEST_LE_U(strlen((char *) result_str) + 1, olen);
-    TEST_ASSERT(ret == 0);
+    TEST_EQUAL(ret, 0);
     TEST_ASSERT(strncmp((char *) check_buf, (char *) result_str, olen) == 0);
 
 exit:
@@ -76,15 +76,14 @@ void mbedtls_pem_read_buffer(char *header, char *footer, char *data,
 
     ret = mbedtls_pem_read_buffer(&ctx, header, footer, (unsigned char *) data,
                                   (unsigned char *) pwd, pwd_len, &use_len);
-    TEST_ASSERT(ret == res);
+    TEST_EQUAL(ret, res);
     if (ret != 0) {
         goto exit;
     }
 
     use_len = 0;
     buf = mbedtls_pem_get_buffer(&ctx, &use_len);
-    TEST_EQUAL(use_len, out->len);
+    TEST_MEMORY_COMPARE(out->x, out->len, buf, use_len);
-    TEST_ASSERT(memcmp(out->x, buf, out->len) == 0);
 
 exit:
     mbedtls_pem_free(&ctx);