diff --git a/ChangeLog.d/base64_decode.txt b/ChangeLog.d/base64_decode.txt new file mode 100644 index 0000000000..2cd2c598aa --- /dev/null +++ b/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. diff --git a/library/base64.c b/library/base64.c index 9677dee5b2..388fa9f388 100644 --- a/library/base64.c +++ b/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 @@ -183,49 +184,72 @@ int mbedtls_base64_decode(unsigned char *dst, size_t dlen, size_t *olen, n++; } - if (n == 0) { - *olen = 0; - return 0; + /* In valid base64, the number of digits (n-equals) is always of the form + * 4*k, 4*k+2 or *4k+3. Also, the number n of digits plus the number of + * 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 - * risk of integer overflow in n: - * n = ( ( n * 6 ) + 7 ) >> 3; + /* We've determined that the input is valid, and that it contains + * exactly k blocks of digits-or-equals, with n = 4 * k, + * 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); - n -= equals; + *olen = (n / 4) * 3 - equals; - if (dst == NULL || dlen < n) { - *olen = n; + /* If the output buffer is too small, signal this and stop here. + * 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; - } else { - x |= mbedtls_ct_base64_dec_value(*src); + /* We already know from the first loop that equal signs are + * only at the end. */ + 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); - } - if (equals <= 0) { - *p++ = MBEDTLS_BYTE_0(x); - } + *p++ = MBEDTLS_BYTE_1(x); + *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; } diff --git a/tests/suites/test_suite_base64.data b/tests/suites/test_suite_base64.data index 3999e73bf9..feed172d9c 100644 --- a/tests/suites/test_suite_base64.data +++ b/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 diff --git a/tests/suites/test_suite_base64.function b/tests/suites/test_suite_base64.function index e351ad8a25..095f980f3f 100644 --- a/tests/suites/test_suite_base64.function +++ b/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 dst_str[1000]; + unsigned char *src = NULL; + 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); - memset(dst_str, 0x00, 1000); - - strncpy((char *) src_str, src_string, sizeof(src_str) - 1); - res = mbedtls_base64_decode(dst_str, sizeof(dst_str), &len, src_str, strlen((char *) src_str)); - TEST_ASSERT(res == result); - 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 + * overread in builds with ASan. (src_string has at least one extra null + * character at the end.) */ + TEST_CALLOC(src, src_len); + if (src_len != 0) { + memcpy(src, src_string, src_len); } + + /* 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 */ diff --git a/tests/suites/test_suite_pem.data b/tests/suites/test_suite_pem.data index 1df9645650..11c54b7cfc 100644 --- a/tests/suites/test_suite_pem.data +++ b/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 diff --git a/tests/suites/test_suite_pem.function b/tests/suites/test_suite_pem.function index 091cbd15e5..342ca52f22 100644 --- a/tests/suites/test_suite_pem.function +++ b/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_ASSERT(olen > strlen((char *) result_str)); - TEST_ASSERT(ret == 0); + TEST_LE_U(olen2, olen); + TEST_LE_U(strlen((char *) result_str) + 1, olen); + 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_ASSERT(memcmp(out->x, buf, out->len) == 0); + TEST_MEMORY_COMPARE(out->x, out->len, buf, use_len); exit: mbedtls_pem_free(&ctx);