From 715bbf3e0c627850428643fe831c0562bc588dfc Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Tue, 3 Jun 2025 22:00:58 +0200 Subject: [PATCH 1/9] mbedtls_base64_decode: test the reported output length Reinforce the unit test for `mbedtls_base64_decode()` with valid inputs to systematically call the function with a smaller output buffer and with an empty output buffer. Assert the reported necessary output length in those cases. Signed-off-by: Gilles Peskine --- tests/suites/test_suite_base64.function | 55 ++++++++++++++++++++----- 1 file changed, 44 insertions(+), 11 deletions(-) diff --git a/tests/suites/test_suite_base64.function b/tests/suites/test_suite_base64.function index e351ad8a25..1797049a08 100644 --- a/tests/suites/test_suite_base64.function +++ b/tests/suites/test_suite_base64.function @@ -85,20 +85,53 @@ 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); + TEST_CALLOC(src, src_len); + if (src_len != 0) { + memcpy(src, src_string, src_len); } + + TEST_CALLOC(dst, dst_size); + + size_t expected_dst_len = correct_dst_len; + + /* Test normal operation */ + TEST_EQUAL(mbedtls_base64_decode(dst, dst_size, &len, + src, src_len), + result); + if (result == 0) { + TEST_MEMORY_COMPARE(dst_string, expected_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; + dst_size -= 1; + TEST_CALLOC(dst, dst_size); + TEST_EQUAL(mbedtls_base64_decode(dst, dst_size, &len, + src, src_len), + MBEDTLS_ERR_BASE64_BUFFER_TOO_SMALL); + TEST_EQUAL(correct_dst_len, len); + } + + /* Test an empty output buffer */ + 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); + } + +exit: + mbedtls_free(src); + mbedtls_free(dst); } /* END_CASE */ From 683a46e6c1265e2028212c03d2e4c47602b3297f Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Tue, 3 Jun 2025 22:01:33 +0200 Subject: [PATCH 2/9] mbedtls_base64_decode: assert sloppy behavior with bad number of = Add unit tests covering cases where the number of digits plus equal signs is not a multiple of 4. These are invalid inputs, but they are currently accepted as long as the number of equal signs is at most 2. The tests assert the current behavior, not behavior that is desirable. Signed-off-by: Gilles Peskine --- tests/suites/test_suite_base64.data | 104 ++++++++++++++++++++++++ tests/suites/test_suite_base64.function | 11 +++ 2 files changed, 115 insertions(+) diff --git a/tests/suites/test_suite_base64.data b/tests/suites/test_suite_base64.data index 3999e73bf9..9488df3a21 100644 --- a/tests/suites/test_suite_base64.data +++ b/tests/suites/test_suite_base64.data @@ -76,6 +76,110 @@ 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 +# Validate the weird behavior of mbedtls_base64_decode() on some +# invalid inputs (number of digts + equals not a multiple of 4). +# In the reference output, "!" characters at the end are needed to +# pad the output buffer, but the actual output omits those. E.g. if +# dst_string is "ab!" then mbedtls_base64_decode() reports a 3-byte +# output when dlen < 3, but actually outputs 2 bytes if given a +# buffer of 3 bytes or more. + +Base64 decode: 1 digit, 0 equals (sloppily accepted) +mbedtls_base64_decode:"Y":"!":0 + +Base64 decode: 1 digit, 1 equals (sloppily accepted) +mbedtls_base64_decode:"Y":"!":0 + +Base64 decode: 1 digit, 2 equals (sloppily accepted) +mbedtls_base64_decode:"Y==":"!":0 + +Base64 decode: 1 digit, 3 equals (bad) +mbedtls_base64_decode:"Y===":"!":MBEDTLS_ERR_BASE64_INVALID_CHARACTER + +Base64 decode: 2 digits, 0 equals (sloppily accepted) +mbedtls_base64_decode:"Yw":"!!":0 + +Base64 decode: 2 digits, 1 equals (sloppily accepted) +mbedtls_base64_decode:"Yw=":"!!":0 + +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 (sloppily accepted) +mbedtls_base64_decode:"Y28":"!!!":0 + +Base64 decode: 3 digits, 1 equals (good) +mbedtls_base64_decode:"Y28=":"co":0 + +Base64 decode: 3 digits, 2 equals (sloppily accepted) +mbedtls_base64_decode:"Y28==":"co":0 + +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 (sloppily accepted) +mbedtls_base64_decode:"Y29t=":"com":0 + +Base64 decode: 4 digits, 2 equals (sloppily accepted) +mbedtls_base64_decode:"Y29t==":"com":0 + +Base64 decode: 4 digits, 3 equals (bad) +mbedtls_base64_decode:"Y29t===":"com":MBEDTLS_ERR_BASE64_INVALID_CHARACTER + +Base64 decode: 5 digits, 0 equals (sloppily accepted) +mbedtls_base64_decode:"Y29tc":"com!":0 + +Base64 decode: 5 digits, 1 equals (sloppily accepted) +mbedtls_base64_decode:"Y29tc=":"com!":0 + +Base64 decode: 5 digits, 2 equals (sloppily accepted) +mbedtls_base64_decode:"Y29tc==":"com!":0 + +Base64 decode: 5 digits, 3 equals (bad) +mbedtls_base64_decode:"Y29tc===":"com!":MBEDTLS_ERR_BASE64_INVALID_CHARACTER + +Base64 decode: 6 digits, 0 equals (sloppily accepted) +mbedtls_base64_decode:"Y29tcA":"com!!":0 + +Base64 decode: 6 digits, 1 equals (sloppily accepted) +mbedtls_base64_decode:"Y29tcA=":"com!!":0 + +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 (sloppily accepted) +mbedtls_base64_decode:"Y29tcG8":"com!!!":0 + +Base64 decode: 7 digits, 1 equals (good) +mbedtls_base64_decode:"Y29tcG8=":"compo":0 + +Base64 decode: 7 digits, 2 equals (sloppily accepted) +mbedtls_base64_decode:"Y29tcG8==":"compo":0 + +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 (sloppily accepted) +mbedtls_base64_decode:"Y29tcG9z=":"compos":0 + +Base64 decode: 8 digits, 2 equals (sloppily accepted) +mbedtls_base64_decode:"Y29tcG9z==":"compos":0 + +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 1797049a08..182be2916f 100644 --- a/tests/suites/test_suite_base64.function +++ b/tests/suites/test_suite_base64.function @@ -99,7 +99,18 @@ void mbedtls_base64_decode(char *src_string, char *dst_string, int result) TEST_CALLOC(dst, dst_size); + /* Validate broken behavior observed on Mbed TLS 3.6.3: + * some invalid inputs are accepted, and asking for the decoded length + * gives a figure that's longer than the decoded output. + * In the test data, trailing "!" characters in dst_string indicate + * padding that must be present in the output buffer length, but + * will not be present in the actual output when the output buffer + * is large enough. + */ size_t expected_dst_len = correct_dst_len; + while (expected_dst_len > 0 && dst_string[expected_dst_len - 1] == '!') { + --expected_dst_len; + } /* Test normal operation */ TEST_EQUAL(mbedtls_base64_decode(dst, dst_size, &len, From 84999d1a7bd0068c08e623a17cddb11674312f61 Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Wed, 4 Jun 2025 10:33:31 +0200 Subject: [PATCH 3/9] Fix mbedtls_base64_decode() accepting invalid inputs with 4n+1 digits The last digit was ignored. Signed-off-by: Gilles Peskine --- ChangeLog.d/base64_decode.txt | 3 +++ library/base64.c | 6 ++++++ tests/suites/test_suite_base64.data | 24 ++++++++++++------------ 3 files changed, 21 insertions(+), 12 deletions(-) create mode 100644 ChangeLog.d/base64_decode.txt diff --git a/ChangeLog.d/base64_decode.txt b/ChangeLog.d/base64_decode.txt new file mode 100644 index 0000000000..93dfef12ff --- /dev/null +++ b/ChangeLog.d/base64_decode.txt @@ -0,0 +1,3 @@ +Bugfix + * Fix mbedtls_base64_decode() accepting invalid inputs with 4n+1 digits + (the last digit was ignored). diff --git a/library/base64.c b/library/base64.c index 9677dee5b2..bff9123398 100644 --- a/library/base64.c +++ b/library/base64.c @@ -183,6 +183,12 @@ int mbedtls_base64_decode(unsigned char *dst, size_t dlen, size_t *olen, n++; } + /* In valid base64, the number of digits is always of the form + * 4n, 4n+2 or 4n+3. */ + if ((n - equals) % 4 == 1) { + return MBEDTLS_ERR_BASE64_INVALID_CHARACTER; + } + if (n == 0) { *olen = 0; return 0; diff --git a/tests/suites/test_suite_base64.data b/tests/suites/test_suite_base64.data index 9488df3a21..4d3b5b9a1a 100644 --- a/tests/suites/test_suite_base64.data +++ b/tests/suites/test_suite_base64.data @@ -84,14 +84,14 @@ mbedtls_base64_decode:"zm masd":"":MBEDTLS_ERR_BASE64_INVALID_CHARACTER # output when dlen < 3, but actually outputs 2 bytes if given a # buffer of 3 bytes or more. -Base64 decode: 1 digit, 0 equals (sloppily accepted) -mbedtls_base64_decode:"Y":"!":0 +Base64 decode: 1 digit, 0 equals (bad) +mbedtls_base64_decode:"Y":"":MBEDTLS_ERR_BASE64_INVALID_CHARACTER -Base64 decode: 1 digit, 1 equals (sloppily accepted) -mbedtls_base64_decode:"Y":"!":0 +Base64 decode: 1 digit, 1 equals (bad) +mbedtls_base64_decode:"Y":"":MBEDTLS_ERR_BASE64_INVALID_CHARACTER -Base64 decode: 1 digit, 2 equals (sloppily accepted) -mbedtls_base64_decode:"Y==":"!":0 +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 @@ -132,14 +132,14 @@ mbedtls_base64_decode:"Y29t==":"com":0 Base64 decode: 4 digits, 3 equals (bad) mbedtls_base64_decode:"Y29t===":"com":MBEDTLS_ERR_BASE64_INVALID_CHARACTER -Base64 decode: 5 digits, 0 equals (sloppily accepted) -mbedtls_base64_decode:"Y29tc":"com!":0 +Base64 decode: 5 digits, 0 equals (bad) +mbedtls_base64_decode:"Y29tc":"":MBEDTLS_ERR_BASE64_INVALID_CHARACTER -Base64 decode: 5 digits, 1 equals (sloppily accepted) -mbedtls_base64_decode:"Y29tc=":"com!":0 +Base64 decode: 5 digits, 1 equals (bad) +mbedtls_base64_decode:"Y29tc=":"":MBEDTLS_ERR_BASE64_INVALID_CHARACTER -Base64 decode: 5 digits, 2 equals (sloppily accepted) -mbedtls_base64_decode:"Y29tc==":"com!":0 +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 From 2b3d6a8f288411187cfc4441a893b0f5a5a163f7 Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Wed, 4 Jun 2025 11:22:25 +0200 Subject: [PATCH 4/9] mbedtls_base64_decode: insist on correct padding Correct base64 input (excluding ignored characters such as spaces) consists of exactly 4*k, 4*k-1 or 4*k-2 digits, followed by 0, 1 or 2 equal signs respectively. Previously, any number of trailing equal signs up to 2 was accepted, but if there fewer than 4*k digits-or-equals, the last partial block was counted in `*olen` in buffer-too-small mode, but was not output despite returning 0. Now `mbedtls_base64_decode()` insists on correct padding. This is backward-compatible since the only plausible useful inputs that used to be accepted were inputs with 4*k-1 or 4*k-2 digits and no trailing equal signs, and those led to invalid (truncated) output. Furthermore the function now always reports the exact output size in buffer-too-small mode. Signed-off-by: Gilles Peskine --- ChangeLog.d/base64_decode.txt | 9 +++- library/base64.c | 57 +++++++++++----------- tests/suites/test_suite_base64.data | 64 +++++++++++-------------- tests/suites/test_suite_base64.function | 15 +----- 4 files changed, 67 insertions(+), 78 deletions(-) diff --git a/ChangeLog.d/base64_decode.txt b/ChangeLog.d/base64_decode.txt index 93dfef12ff..2cd2c598aa 100644 --- a/ChangeLog.d/base64_decode.txt +++ b/ChangeLog.d/base64_decode.txt @@ -1,3 +1,8 @@ Bugfix - * Fix mbedtls_base64_decode() accepting invalid inputs with 4n+1 digits - (the last digit was ignored). + * 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 bff9123398..cc6a73d150 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,55 +184,57 @@ int mbedtls_base64_decode(unsigned char *dst, size_t dlen, size_t *olen, n++; } - /* In valid base64, the number of digits is always of the form - * 4n, 4n+2 or 4n+3. */ + /* 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 == 0) { - *olen = 0; - return 0; + 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; - */ - n = (6 * (n >> 3)) + ((6 * (n & 0x7) + 7) >> 3); - n -= equals; + /* We've determined that the input is valid, and that it contains + * n digits-plus-trailing-equal-signs, which means (n - equals) digits. + * Now set *olen to the exact length of the output. */ + /* Each block of 4 digits in the input map to 3 bytes of output. + * The last block can have one or two equal signs, in which case + * there are that many fewer output bytes. */ + *olen = (n / 4) * 3 - equals; - if (dst == NULL || dlen < n) { - *olen = n; + 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 4d3b5b9a1a..547b9fd127 100644 --- a/tests/suites/test_suite_base64.data +++ b/tests/suites/test_suite_base64.data @@ -76,31 +76,25 @@ 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 -# Validate the weird behavior of mbedtls_base64_decode() on some -# invalid inputs (number of digts + equals not a multiple of 4). -# In the reference output, "!" characters at the end are needed to -# pad the output buffer, but the actual output omits those. E.g. if -# dst_string is "ab!" then mbedtls_base64_decode() reports a 3-byte -# output when dlen < 3, but actually outputs 2 bytes if given a -# buffer of 3 bytes or more. - +# The next few test cases validate systematically for short inputs that +# we require the correct number of trailing equal signs. 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 +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 +mbedtls_base64_decode:"Y===":"":MBEDTLS_ERR_BASE64_INVALID_CHARACTER -Base64 decode: 2 digits, 0 equals (sloppily accepted) -mbedtls_base64_decode:"Yw":"!!":0 +Base64 decode: 2 digits, 0 equals (bad) +mbedtls_base64_decode:"Yw":"c":MBEDTLS_ERR_BASE64_INVALID_CHARACTER -Base64 decode: 2 digits, 1 equals (sloppily accepted) -mbedtls_base64_decode:"Yw=":"!!":0 +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 @@ -108,14 +102,14 @@ 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 (sloppily accepted) -mbedtls_base64_decode:"Y28":"!!!":0 +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 (sloppily accepted) -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 @@ -123,11 +117,11 @@ 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 (sloppily accepted) -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 (sloppily accepted) -mbedtls_base64_decode:"Y29t==":"com":0 +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 @@ -142,13 +136,13 @@ 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 +mbedtls_base64_decode:"Y29tc===":"com":MBEDTLS_ERR_BASE64_INVALID_CHARACTER -Base64 decode: 6 digits, 0 equals (sloppily accepted) -mbedtls_base64_decode:"Y29tcA":"com!!":0 +Base64 decode: 6 digits, 0 equals (bad) +mbedtls_base64_decode:"Y29tcA":"comp":MBEDTLS_ERR_BASE64_INVALID_CHARACTER -Base64 decode: 6 digits, 1 equals (sloppily accepted) -mbedtls_base64_decode:"Y29tcA=":"com!!":0 +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 @@ -156,14 +150,14 @@ 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 (sloppily accepted) -mbedtls_base64_decode:"Y29tcG8":"com!!!":0 +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 (sloppily accepted) -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 @@ -171,11 +165,11 @@ 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 (sloppily accepted) -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 (sloppily accepted) -mbedtls_base64_decode:"Y29tcG9z==":"compos":0 +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 diff --git a/tests/suites/test_suite_base64.function b/tests/suites/test_suite_base64.function index 182be2916f..8c948b4b57 100644 --- a/tests/suites/test_suite_base64.function +++ b/tests/suites/test_suite_base64.function @@ -99,25 +99,12 @@ void mbedtls_base64_decode(char *src_string, char *dst_string, int result) TEST_CALLOC(dst, dst_size); - /* Validate broken behavior observed on Mbed TLS 3.6.3: - * some invalid inputs are accepted, and asking for the decoded length - * gives a figure that's longer than the decoded output. - * In the test data, trailing "!" characters in dst_string indicate - * padding that must be present in the output buffer length, but - * will not be present in the actual output when the output buffer - * is large enough. - */ - size_t expected_dst_len = correct_dst_len; - while (expected_dst_len > 0 && dst_string[expected_dst_len - 1] == '!') { - --expected_dst_len; - } - /* Test normal operation */ TEST_EQUAL(mbedtls_base64_decode(dst, dst_size, &len, src, src_len), result); if (result == 0) { - TEST_MEMORY_COMPARE(dst_string, expected_dst_len, dst, len); + TEST_MEMORY_COMPARE(dst_string, correct_dst_len, dst, len); } /* Test an output buffer that's one byte too small */ From e7ed8c4c2f7448ea00d67679354e7303830c9434 Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Thu, 5 Jun 2025 16:00:27 +0200 Subject: [PATCH 5/9] Explain some aspects of the tests Signed-off-by: Gilles Peskine --- tests/suites/test_suite_base64.data | 3 +++ tests/suites/test_suite_base64.function | 5 +++++ 2 files changed, 8 insertions(+) diff --git a/tests/suites/test_suite_base64.data b/tests/suites/test_suite_base64.data index 547b9fd127..feed172d9c 100644 --- a/tests/suites/test_suite_base64.data +++ b/tests/suites/test_suite_base64.data @@ -78,6 +78,8 @@ 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 @@ -126,6 +128,7 @@ 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 diff --git a/tests/suites/test_suite_base64.function b/tests/suites/test_suite_base64.function index 8c948b4b57..df63aea06f 100644 --- a/tests/suites/test_suite_base64.function +++ b/tests/suites/test_suite_base64.function @@ -92,11 +92,16 @@ void mbedtls_base64_decode(char *src_string, char *dst_string, int result) size_t dst_size = correct_dst_len; size_t len; + /* 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 */ From 13cc0c2122acdcbf9309ef0762c891b4a86c640b Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Thu, 5 Jun 2025 16:02:55 +0200 Subject: [PATCH 6/9] mbedtls_base64_decode: test dst=NULL with dlen>0 The documentation explicitly says that `*dst = NULL` **or** `dlen = 0` triggers tell-me-the-output-length mode. Signed-off-by: Gilles Peskine --- tests/suites/test_suite_base64.function | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/tests/suites/test_suite_base64.function b/tests/suites/test_suite_base64.function index df63aea06f..5d8ed9bf9f 100644 --- a/tests/suites/test_suite_base64.function +++ b/tests/suites/test_suite_base64.function @@ -132,6 +132,14 @@ void mbedtls_base64_decode(char *src_string, char *dst_string, int result) 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); From 55d211388af1f5717f971d23e09837b93469c54d Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Tue, 10 Jun 2025 09:42:03 +0200 Subject: [PATCH 7/9] Adjust test case with invalid base64 Now that Base64 validates the number of trailing equals, adjust the PEM test case that has a Base64 payload with a wrong number of trailing equals, where `mbedtls_pem_read_buffer()` now returns a different error code. I'm not sure what the exact intent of the test was, so add a variant with trailing equals as well. Signed-off-by: Gilles Peskine --- tests/suites/test_suite_pem.data | 14 +++++++++++--- tests/suites/test_suite_pem.function | 13 ++++++------- 2 files changed, 17 insertions(+), 10 deletions(-) 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); From 03303d88fb1f8654a95ae7313eb237fc3ab611fd Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Wed, 11 Jun 2025 18:24:26 +0200 Subject: [PATCH 8/9] Don't mutate dst_size This lead to `dst_size` not having the intended value in subsequent code. Signed-off-by: Gilles Peskine --- tests/suites/test_suite_base64.function | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/tests/suites/test_suite_base64.function b/tests/suites/test_suite_base64.function index 5d8ed9bf9f..3bd9932408 100644 --- a/tests/suites/test_suite_base64.function +++ b/tests/suites/test_suite_base64.function @@ -116,9 +116,8 @@ void mbedtls_base64_decode(char *src_string, char *dst_string, int result) if (result == 0 && dst_size != 0) { mbedtls_free(dst); dst = NULL; - dst_size -= 1; - TEST_CALLOC(dst, dst_size); - TEST_EQUAL(mbedtls_base64_decode(dst, dst_size, &len, + 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); From 51dccfb2a6edbebce791387041c65071b29545ca Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Wed, 11 Jun 2025 18:47:31 +0200 Subject: [PATCH 9/9] Improve some explanations Signed-off-by: Gilles Peskine --- library/base64.c | 25 ++++++++++++++++++++----- tests/suites/test_suite_base64.function | 6 +++++- 2 files changed, 25 insertions(+), 6 deletions(-) diff --git a/library/base64.c b/library/base64.c index cc6a73d150..388fa9f388 100644 --- a/library/base64.c +++ b/library/base64.c @@ -195,13 +195,28 @@ int mbedtls_base64_decode(unsigned char *dst, size_t dlen, size_t *olen, } /* We've determined that the input is valid, and that it contains - * n digits-plus-trailing-equal-signs, which means (n - equals) digits. - * Now set *olen to the exact length of the output. */ - /* Each block of 4 digits in the input map to 3 bytes of output. - * The last block can have one or two equal signs, in which case - * there are that many fewer output bytes. */ + * 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. + */ *olen = (n / 4) * 3 - equals; + /* 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; } diff --git a/tests/suites/test_suite_base64.function b/tests/suites/test_suite_base64.function index 3bd9932408..095f980f3f 100644 --- a/tests/suites/test_suite_base64.function +++ b/tests/suites/test_suite_base64.function @@ -123,7 +123,11 @@ void mbedtls_base64_decode(char *src_string, char *dst_string, int result) TEST_EQUAL(correct_dst_len, len); } - /* Test an empty output buffer */ + /* 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),