From d37e0c4639c66f7bb4b6e2c039c96304b7536185 Mon Sep 17 00:00:00 2001 From: David Horstmann Date: Thu, 16 Jan 2025 16:24:35 +0000 Subject: [PATCH 01/52] Add constant-flow testing for PKCS7 padding Signed-off-by: David Horstmann --- library/cipher.c | 6 +++-- library/cipher_invasive.h | 27 +++++++++++++++++++++ tests/suites/test_suite_cipher.function | 22 +++++++++++++++++ tests/suites/test_suite_cipher.padding.data | 12 +++++++++ 4 files changed, 65 insertions(+), 2 deletions(-) create mode 100644 library/cipher_invasive.h diff --git a/library/cipher.c b/library/cipher.c index 7f4c121492..5e14e1eab6 100644 --- a/library/cipher.c +++ b/library/cipher.c @@ -14,6 +14,7 @@ #if defined(MBEDTLS_CIPHER_C) #include "mbedtls/cipher.h" +#include "cipher_invasive.h" #include "cipher_wrap.h" #include "mbedtls/platform_util.h" #include "mbedtls/error.h" @@ -838,8 +839,9 @@ static void add_pkcs_padding(unsigned char *output, size_t output_len, } } -static int get_pkcs_padding(unsigned char *input, size_t input_len, - size_t *data_len) +MBEDTLS_STATIC_TESTABLE int get_pkcs_padding(unsigned char *input, + size_t input_len, + size_t *data_len) { size_t i, pad_idx; unsigned char padding_len; diff --git a/library/cipher_invasive.h b/library/cipher_invasive.h new file mode 100644 index 0000000000..2dbd1d2c7b --- /dev/null +++ b/library/cipher_invasive.h @@ -0,0 +1,27 @@ +/** + * \file cipher_invasive.h + * + * \brief Cipher module: interfaces for invasive testing only. + * + * The interfaces in this file are intended for testing purposes only. + * They SHOULD NOT be made available in library integrations except when + * building the library for testing. + */ +/* + * Copyright The Mbed TLS Contributors + * SPDX-License-Identifier: Apache-2.0 OR GPL-2.0-or-later + */ +#ifndef MBEDTLS_CIPHER_INVASIVE_H +#define MBEDTLS_CIPHER_INVASIVE_H + +#include "common.h" + +#if defined(MBEDTLS_TEST_HOOKS) && defined(MBEDTLS_CIPHER_C) + +MBEDTLS_STATIC_TESTABLE int get_pkcs_padding(unsigned char *input, + size_t input_len, + size_t *data_len); + +#endif + +#endif /* MBEDTLS_CIPHER_INVASIVE_H */ diff --git a/tests/suites/test_suite_cipher.function b/tests/suites/test_suite_cipher.function index 040c35ca58..2444ef9d8f 100644 --- a/tests/suites/test_suite_cipher.function +++ b/tests/suites/test_suite_cipher.function @@ -6,6 +6,10 @@ #include "mbedtls/gcm.h" #endif +#include "cipher_invasive.h" + +#include "test/constant_flow.h" + #if defined(MBEDTLS_CIPHER_HAVE_SOME_AEAD_VIA_LEGACY_OR_USE_PSA) || defined(MBEDTLS_NIST_KW_C) #define MBEDTLS_CIPHER_AUTH_CRYPT #endif @@ -1260,3 +1264,21 @@ exit: mbedtls_free(key); } /* END_CASE */ + +/* BEGIN_CASE depends_on:MBEDTLS_TEST_HOOKS */ +void get_pkcs_padding(data_t *decrypted_block, int exp_ret, int exp_len) +{ + int ret; + size_t calculated_len; + + TEST_CF_SECRET(decrypted_block->x, decrypted_block->len); + ret = get_pkcs_padding(decrypted_block->x, decrypted_block->len, + &calculated_len); + TEST_CF_PUBLIC(decrypted_block->x, decrypted_block->len); + + TEST_EQUAL(ret, exp_ret); + if (exp_ret == 0) { + TEST_EQUAL(calculated_len, exp_len); + } +} +/* END_CASE */ diff --git a/tests/suites/test_suite_cipher.padding.data b/tests/suites/test_suite_cipher.padding.data index 0370fb3d28..bd04a50882 100644 --- a/tests/suites/test_suite_cipher.padding.data +++ b/tests/suites/test_suite_cipher.padding.data @@ -217,3 +217,15 @@ check_padding:MBEDTLS_PADDING_NONE:"DABBAD0001":0:5 Check no padding #3 (correct by definition) check_padding:MBEDTLS_PADDING_NONE:"":0:0 + +Constant-time PKCS7 padding, valid #1 +get_pkcs_padding:"00112233445566778899AABBCCDDEE01":0:15 + +Constant-time PKCS7 padding, valid #2 +get_pkcs_padding:"00112233445566778899AA0505050505":0:11 + +Constant-time PKCS7 padding, invalid zero +get_pkcs_padding:"00112233445566778899AABBCCDDEE00":MBEDTLS_ERR_CIPHER_INVALID_PADDING:0 + +Constant-time PKCS7 padding, invalid > 16 +get_pkcs_padding:"00112233445566778899AABBCCDDEE11":MBEDTLS_ERR_CIPHER_INVALID_PADDING:0 From 652ea21737fcc6b2fdcc0be24e2462c17f16d67c Mon Sep 17 00:00:00 2001 From: David Horstmann Date: Mon, 20 Jan 2025 17:44:00 +0000 Subject: [PATCH 02/52] Fix timing side-channel in PKCS7 padding Previously, we were checking if the last padding byte was in the range 1-16 and returning early if not. This was to prevent an integer overflow in the output length. Instead, do the checks in constant-time and conditionally set the output length based on whether the padding is correct or not, preventing both the side-channel and the integer overflow. Signed-off-by: David Horstmann --- library/cipher.c | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/library/cipher.c b/library/cipher.c index 5e14e1eab6..9be53b7427 100644 --- a/library/cipher.c +++ b/library/cipher.c @@ -851,10 +851,6 @@ MBEDTLS_STATIC_TESTABLE int get_pkcs_padding(unsigned char *input, } padding_len = input[input_len - 1]; - if (padding_len == 0 || padding_len > input_len) { - return MBEDTLS_ERR_CIPHER_INVALID_PADDING; - } - *data_len = input_len - padding_len; mbedtls_ct_condition_t bad = mbedtls_ct_uint_gt(padding_len, input_len); bad = mbedtls_ct_bool_or(bad, mbedtls_ct_uint_eq(padding_len, 0)); @@ -868,6 +864,9 @@ MBEDTLS_STATIC_TESTABLE int get_pkcs_padding(unsigned char *input, bad = mbedtls_ct_bool_or(bad, mbedtls_ct_bool_and(in_padding, different)); } + /* If the padding is invalid, set the output length to 0 */ + *data_len = mbedtls_ct_if(bad, 0, input_len - padding_len); + return mbedtls_ct_error_if_else_0(bad, MBEDTLS_ERR_CIPHER_INVALID_PADDING); } #endif /* MBEDTLS_CIPHER_PADDING_PKCS7 */ From 16674559929f6b37aee738953151b2b67eeeade9 Mon Sep 17 00:00:00 2001 From: David Horstmann Date: Wed, 22 Jan 2025 11:18:14 +0000 Subject: [PATCH 03/52] Add ChangeLog entry for PKCS#7 side channel fix Signed-off-by: David Horstmann --- ChangeLog.d/pkcs7-padding-side-channel-fix.txt | 4 ++++ 1 file changed, 4 insertions(+) create mode 100644 ChangeLog.d/pkcs7-padding-side-channel-fix.txt diff --git a/ChangeLog.d/pkcs7-padding-side-channel-fix.txt b/ChangeLog.d/pkcs7-padding-side-channel-fix.txt new file mode 100644 index 0000000000..f34c095056 --- /dev/null +++ b/ChangeLog.d/pkcs7-padding-side-channel-fix.txt @@ -0,0 +1,4 @@ +Security + * Fix a timing side channel in the implementation of PKCS#7 padding + which would allow an attacker who can request decryption of arbitrary + ciphertexts to recover the last byte of each block of the plaintext. From 32d8c9052815e51202759cbc11cfea7499db2470 Mon Sep 17 00:00:00 2001 From: David Horstmann Date: Wed, 22 Jan 2025 11:26:00 +0000 Subject: [PATCH 04/52] Disable check-names for static padding function Check names is intended for public APIs. It doesn't matter if we call a static function a non-namespaced name, so add an exception in the invasive testing header file. Signed-off-by: David Horstmann --- library/cipher_invasive.h | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/library/cipher_invasive.h b/library/cipher_invasive.h index 2dbd1d2c7b..f1584007b8 100644 --- a/library/cipher_invasive.h +++ b/library/cipher_invasive.h @@ -18,9 +18,10 @@ #if defined(MBEDTLS_TEST_HOOKS) && defined(MBEDTLS_CIPHER_C) -MBEDTLS_STATIC_TESTABLE int get_pkcs_padding(unsigned char *input, - size_t input_len, - size_t *data_len); +MBEDTLS_STATIC_TESTABLE +int get_pkcs_padding(unsigned char *input, //no-check-names + size_t input_len, + size_t *data_len); #endif From ab7bb5734d26e29faee843a1fc7cabdfd8805f4a Mon Sep 17 00:00:00 2001 From: David Horstmann Date: Wed, 5 Mar 2025 18:05:04 +0000 Subject: [PATCH 05/52] Appease check-names with prefix Adding an mbedtls_prefix is preferred to having '//no-check-names' littered about. Signed-off-by: David Horstmann --- library/cipher.c | 6 +++--- library/cipher_invasive.h | 7 +++---- 2 files changed, 6 insertions(+), 7 deletions(-) diff --git a/library/cipher.c b/library/cipher.c index 9be53b7427..ebb03f464e 100644 --- a/library/cipher.c +++ b/library/cipher.c @@ -839,9 +839,9 @@ static void add_pkcs_padding(unsigned char *output, size_t output_len, } } -MBEDTLS_STATIC_TESTABLE int get_pkcs_padding(unsigned char *input, - size_t input_len, - size_t *data_len) +MBEDTLS_STATIC_TESTABLE int mbedtls_get_pkcs_padding(unsigned char *input, + size_t input_len, + size_t *data_len) { size_t i, pad_idx; unsigned char padding_len; diff --git a/library/cipher_invasive.h b/library/cipher_invasive.h index f1584007b8..702f8f73e9 100644 --- a/library/cipher_invasive.h +++ b/library/cipher_invasive.h @@ -18,10 +18,9 @@ #if defined(MBEDTLS_TEST_HOOKS) && defined(MBEDTLS_CIPHER_C) -MBEDTLS_STATIC_TESTABLE -int get_pkcs_padding(unsigned char *input, //no-check-names - size_t input_len, - size_t *data_len); +MBEDTLS_STATIC_TESTABLE int mbedtls_get_pkcs_padding(unsigned char *input, + size_t input_len, + size_t *data_len); #endif From 5a5440e63af436b106a01aa4ba314b0cb8fd3129 Mon Sep 17 00:00:00 2001 From: David Horstmann Date: Fri, 28 Mar 2025 10:57:45 +0000 Subject: [PATCH 06/52] Update to the new name in usages as well Somehow the uses of the function were missed when renaming. Signed-off-by: David Horstmann --- library/cipher.c | 2 +- tests/suites/test_suite_cipher.function | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/library/cipher.c b/library/cipher.c index ebb03f464e..d0ba01ea01 100644 --- a/library/cipher.c +++ b/library/cipher.c @@ -1145,7 +1145,7 @@ int mbedtls_cipher_set_padding_mode(mbedtls_cipher_context_t *ctx, #if defined(MBEDTLS_CIPHER_PADDING_PKCS7) case MBEDTLS_PADDING_PKCS7: ctx->add_padding = add_pkcs_padding; - ctx->get_padding = get_pkcs_padding; + ctx->get_padding = mbedtls_get_pkcs_padding; break; #endif #if defined(MBEDTLS_CIPHER_PADDING_ONE_AND_ZEROS) diff --git a/tests/suites/test_suite_cipher.function b/tests/suites/test_suite_cipher.function index 2444ef9d8f..3eb46257df 100644 --- a/tests/suites/test_suite_cipher.function +++ b/tests/suites/test_suite_cipher.function @@ -1272,8 +1272,8 @@ void get_pkcs_padding(data_t *decrypted_block, int exp_ret, int exp_len) size_t calculated_len; TEST_CF_SECRET(decrypted_block->x, decrypted_block->len); - ret = get_pkcs_padding(decrypted_block->x, decrypted_block->len, - &calculated_len); + ret = mbedtls_get_pkcs_padding(decrypted_block->x, decrypted_block->len, + &calculated_len); TEST_CF_PUBLIC(decrypted_block->x, decrypted_block->len); TEST_EQUAL(ret, exp_ret); From d75c4c23818c0221cb8adba705b3dcff9a65c324 Mon Sep 17 00:00:00 2001 From: David Horstmann Date: Fri, 28 Mar 2025 17:16:27 +0000 Subject: [PATCH 07/52] Remove unnecessary TEST_CF_PUBLIC macro call Signed-off-by: David Horstmann --- tests/suites/test_suite_cipher.function | 1 - 1 file changed, 1 deletion(-) diff --git a/tests/suites/test_suite_cipher.function b/tests/suites/test_suite_cipher.function index 3eb46257df..2856ae5699 100644 --- a/tests/suites/test_suite_cipher.function +++ b/tests/suites/test_suite_cipher.function @@ -1274,7 +1274,6 @@ void get_pkcs_padding(data_t *decrypted_block, int exp_ret, int exp_len) TEST_CF_SECRET(decrypted_block->x, decrypted_block->len); ret = mbedtls_get_pkcs_padding(decrypted_block->x, decrypted_block->len, &calculated_len); - TEST_CF_PUBLIC(decrypted_block->x, decrypted_block->len); TEST_EQUAL(ret, exp_ret); if (exp_ret == 0) { From b2b1c3bb8193dd8965b14b3d8bf889e0c271fde5 Mon Sep 17 00:00:00 2001 From: David Horstmann Date: Fri, 28 Mar 2025 17:24:45 +0000 Subject: [PATCH 08/52] Add testcase for maximum padding length Signed-off-by: David Horstmann --- tests/suites/test_suite_cipher.padding.data | 3 +++ 1 file changed, 3 insertions(+) diff --git a/tests/suites/test_suite_cipher.padding.data b/tests/suites/test_suite_cipher.padding.data index bd04a50882..85b14c1f2f 100644 --- a/tests/suites/test_suite_cipher.padding.data +++ b/tests/suites/test_suite_cipher.padding.data @@ -224,6 +224,9 @@ get_pkcs_padding:"00112233445566778899AABBCCDDEE01":0:15 Constant-time PKCS7 padding, valid #2 get_pkcs_padding:"00112233445566778899AA0505050505":0:11 +Constant-time PKCS7 padding, valid #3 +get_pkcs_padding:"10101010101010101010101010101010":0:0 + Constant-time PKCS7 padding, invalid zero get_pkcs_padding:"00112233445566778899AABBCCDDEE00":MBEDTLS_ERR_CIPHER_INVALID_PADDING:0 From bbf1a015036a4fbcf713dc06fc6dd24726db59ec Mon Sep 17 00:00:00 2001 From: David Horstmann Date: Fri, 28 Mar 2025 17:31:15 +0000 Subject: [PATCH 09/52] Modify ChangeLog entry to full plaintext recovery Signed-off-by: David Horstmann --- ChangeLog.d/pkcs7-padding-side-channel-fix.txt | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/ChangeLog.d/pkcs7-padding-side-channel-fix.txt b/ChangeLog.d/pkcs7-padding-side-channel-fix.txt index f34c095056..b813b84ce8 100644 --- a/ChangeLog.d/pkcs7-padding-side-channel-fix.txt +++ b/ChangeLog.d/pkcs7-padding-side-channel-fix.txt @@ -1,4 +1,4 @@ Security * Fix a timing side channel in the implementation of PKCS#7 padding which would allow an attacker who can request decryption of arbitrary - ciphertexts to recover the last byte of each block of the plaintext. + ciphertexts to recover the plaintext through a timing oracle attack. From 850e5b317d17b3fe965ad172b5799e60661a144c Mon Sep 17 00:00:00 2001 From: David Horstmann Date: Fri, 28 Mar 2025 17:33:45 +0000 Subject: [PATCH 10/52] Document assumption of mbedtls_get_pkcs_padding Point out that the input length must be the same as the cipher's block size. Signed-off-by: David Horstmann --- library/cipher.c | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/library/cipher.c b/library/cipher.c index d0ba01ea01..2ae01dd84d 100644 --- a/library/cipher.c +++ b/library/cipher.c @@ -839,6 +839,11 @@ static void add_pkcs_padding(unsigned char *output, size_t output_len, } } +/* + * Get the length of the PKCS7 padding. + * + * Note: input_len must be the block size of the cipher. + */ MBEDTLS_STATIC_TESTABLE int mbedtls_get_pkcs_padding(unsigned char *input, size_t input_len, size_t *data_len) From 19d2c9165a13decf754177adda2bf59fd0e32aa1 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Manuel=20P=C3=A9gouri=C3=A9-Gonnard?= Date: Mon, 5 May 2025 16:41:52 +0200 Subject: [PATCH 11/52] Fix undocumented free() in x509_string_to_names() MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Now programs/x509/cert_write san="DN:CN=#0000;DN:CN=#0000" is no longer crashing with use-after-free, instead it's now failing cleanly: failed ! mbedtls_x509_string_to_names returned -0x2800 - X509 - Input invalid That's better of course but still not great, will be fixed by future commits. Signed-off-by: Manuel Pégourié-Gonnard --- .../fix-string-to-names-memory-management.txt | 18 ++++++++++++++++++ include/mbedtls/x509.h | 3 ++- library/x509_create.c | 8 ++++++-- 3 files changed, 26 insertions(+), 3 deletions(-) create mode 100644 ChangeLog.d/fix-string-to-names-memory-management.txt diff --git a/ChangeLog.d/fix-string-to-names-memory-management.txt b/ChangeLog.d/fix-string-to-names-memory-management.txt new file mode 100644 index 0000000000..1b2198287d --- /dev/null +++ b/ChangeLog.d/fix-string-to-names-memory-management.txt @@ -0,0 +1,18 @@ +Security + * Fix possible use-after-free or double-free in code calling + mbedtls_x509_string_to_names(). This was caused by the function calling + mbedtls_asn1_free_named_data_list() on its head argument, while the + documentation did no suggest it did, making it likely for callers relying + on the documented behaviour to still hold pointers to memory blocks after + they were free()d, resulting in high risk of use-after-free or double-free, + with consequences ranging up to arbitrary code execution. + In particular, the two sample programs x509/cert_write and x509/cert_req + were affected (use-after-free if the san string contains more than one DN). + Code that does not call mbedtls_string_to_names() directly is not affected. + Found by Linh Le and Ngan Nguyen from Calif. + +Changes + * The function mbedtls_x509_string_to_names() now requires its head argument + to point to NULL on entry. This make it likely that existing risky uses of + this function (see the entry in the Security section) will be detected and + fixed. diff --git a/include/mbedtls/x509.h b/include/mbedtls/x509.h index 453f598c74..6b104613d7 100644 --- a/include/mbedtls/x509.h +++ b/include/mbedtls/x509.h @@ -332,7 +332,8 @@ int mbedtls_x509_dn_gets(char *buf, size_t size, const mbedtls_x509_name *dn); * call to mbedtls_asn1_free_named_data_list(). * * \param[out] head Address in which to store the pointer to the head of the - * allocated list of mbedtls_x509_name + * allocated list of mbedtls_x509_name. Must point to NULL on + * entry. * \param[in] name The string representation of a DN to convert * * \return 0 on success, or a negative error code. diff --git a/library/x509_create.c b/library/x509_create.c index 839b5df226..420e36b81b 100644 --- a/library/x509_create.c +++ b/library/x509_create.c @@ -292,8 +292,12 @@ int mbedtls_x509_string_to_names(mbedtls_asn1_named_data **head, const char *nam unsigned char data[MBEDTLS_X509_MAX_DN_NAME_SIZE]; size_t data_len = 0; - /* Clear existing chain if present */ - mbedtls_asn1_free_named_data_list(head); + /* Ensure the output parameter is not already populated. + * (If it were, overwriting it would likely cause a memory leak.) + */ + if (*head != NULL) { + return MBEDTLS_ERR_X509_BAD_INPUT_DATA; + } while (c <= end) { if (in_attr_type && *c == '=') { From acdcb7fcd1aaca3050431b9034cc4ec3f8f7c7de Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Manuel=20P=C3=A9gouri=C3=A9-Gonnard?= Date: Mon, 5 May 2025 16:49:45 +0200 Subject: [PATCH 12/52] Restore behaviour of mbedtls_x509write_set_foo_name() MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The documentation doesn't say you can't call these functions more than once on the same context, and if you do it shouldn't result in a memory leak. Historically, the call to mbedtls_asn1_free_named_data_list() in mbedtls_x509_string_to_names() (that was removed in the previous commit) was ensuring that. Let's restore it where it makes sense. (These are the only 3 places calling mbedtls_x509_string_to_names() in the library.) Signed-off-by: Manuel Pégourié-Gonnard --- library/x509write_crt.c | 2 ++ library/x509write_csr.c | 1 + 2 files changed, 3 insertions(+) diff --git a/library/x509write_crt.c b/library/x509write_crt.c index 56f23c9fab..d829ffe602 100644 --- a/library/x509write_crt.c +++ b/library/x509write_crt.c @@ -84,12 +84,14 @@ void mbedtls_x509write_crt_set_issuer_key(mbedtls_x509write_cert *ctx, int mbedtls_x509write_crt_set_subject_name(mbedtls_x509write_cert *ctx, const char *subject_name) { + mbedtls_asn1_free_named_data_list(&ctx->subject); return mbedtls_x509_string_to_names(&ctx->subject, subject_name); } int mbedtls_x509write_crt_set_issuer_name(mbedtls_x509write_cert *ctx, const char *issuer_name) { + mbedtls_asn1_free_named_data_list(&ctx->issuer); return mbedtls_x509_string_to_names(&ctx->issuer, issuer_name); } diff --git a/library/x509write_csr.c b/library/x509write_csr.c index 0d6f6bb1d3..75bd8f044e 100644 --- a/library/x509write_csr.c +++ b/library/x509write_csr.c @@ -66,6 +66,7 @@ void mbedtls_x509write_csr_set_key(mbedtls_x509write_csr *ctx, mbedtls_pk_contex int mbedtls_x509write_csr_set_subject_name(mbedtls_x509write_csr *ctx, const char *subject_name) { + mbedtls_asn1_free_named_data_list(&ctx->subject); return mbedtls_x509_string_to_names(&ctx->subject, subject_name); } From 4dd52b7cfedc9d42aa144d0f5e4350fa07001226 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Manuel=20P=C3=A9gouri=C3=A9-Gonnard?= Date: Mon, 5 May 2025 17:09:14 +0200 Subject: [PATCH 13/52] Fix runtime error in cert_write & cert_req MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The runtime error was introduced two commits ago (while avoiding a use-after-free). Now the programs run cleanly but still leak memory. The memory leak is long pre-existing and larger than just DN components (which are made temporarily slightly worse by this commit) and will be fixed properly in the next commit. Signed-off-by: Manuel Pégourié-Gonnard --- programs/x509/cert_req.c | 13 +++++++++---- programs/x509/cert_write.c | 13 +++++++++---- 2 files changed, 18 insertions(+), 8 deletions(-) diff --git a/programs/x509/cert_req.c b/programs/x509/cert_req.c index 995ee499d5..c399021916 100644 --- a/programs/x509/cert_req.c +++ b/programs/x509/cert_req.c @@ -150,7 +150,6 @@ int main(int argc, char *argv[]) mbedtls_ctr_drbg_context ctr_drbg; const char *pers = "csr example app"; mbedtls_x509_san_list *cur, *prev; - mbedtls_asn1_named_data *ext_san_dirname = NULL; #if defined(MBEDTLS_X509_CRT_PARSE_C) uint8_t ip[4] = { 0 }; #endif @@ -274,7 +273,12 @@ usage: cur->node.san.unstructured_name.len = sizeof(ip); } else if (strcmp(q, "DN") == 0) { cur->node.type = MBEDTLS_X509_SAN_DIRECTORY_NAME; - if ((ret = mbedtls_x509_string_to_names(&ext_san_dirname, + /* Work around an API mismatch between string_to_names() and + * mbedtls_x509_subject_alternative_name, which holds an + * actual mbedtls_x509_name while a pointer to one would be + * more convenient here. */ + mbedtls_asn1_named_data *tmp_san_dirname = NULL; + if ((ret = mbedtls_x509_string_to_names(&tmp_san_dirname, subtype_value)) != 0) { mbedtls_strerror(ret, buf, sizeof(buf)); mbedtls_printf( @@ -283,7 +287,9 @@ usage: (unsigned int) -ret, buf); goto exit; } - cur->node.san.directory_name = *ext_san_dirname; + cur->node.san.directory_name = *tmp_san_dirname; + mbedtls_free(tmp_san_dirname); + tmp_san_dirname = NULL; } else { mbedtls_free(cur); goto usage; @@ -492,7 +498,6 @@ exit: } mbedtls_x509write_csr_free(&req); - mbedtls_asn1_free_named_data_list(&ext_san_dirname); mbedtls_pk_free(&key); mbedtls_ctr_drbg_free(&ctr_drbg); mbedtls_entropy_free(&entropy); diff --git a/programs/x509/cert_write.c b/programs/x509/cert_write.c index 6fd1dce1fc..63872a953f 100644 --- a/programs/x509/cert_write.c +++ b/programs/x509/cert_write.c @@ -312,7 +312,6 @@ int main(int argc, char *argv[]) mbedtls_ctr_drbg_context ctr_drbg; const char *pers = "crt example app"; mbedtls_x509_san_list *cur, *prev; - mbedtls_asn1_named_data *ext_san_dirname = NULL; uint8_t ip[4] = { 0 }; /* * Set to sane values @@ -595,7 +594,12 @@ usage: cur->node.san.unstructured_name.len = sizeof(ip); } else if (strcmp(q, "DN") == 0) { cur->node.type = MBEDTLS_X509_SAN_DIRECTORY_NAME; - if ((ret = mbedtls_x509_string_to_names(&ext_san_dirname, + /* Work around an API mismatch between string_to_names() and + * mbedtls_x509_subject_alternative_name, which holds an + * actual mbedtls_x509_name while a pointer to one would be + * more convenient here. */ + mbedtls_asn1_named_data *tmp_san_dirname = NULL; + if ((ret = mbedtls_x509_string_to_names(&tmp_san_dirname, subtype_value)) != 0) { mbedtls_strerror(ret, buf, sizeof(buf)); mbedtls_printf( @@ -604,7 +608,9 @@ usage: (unsigned int) -ret, buf); goto exit; } - cur->node.san.directory_name = *ext_san_dirname; + cur->node.san.directory_name = *tmp_san_dirname; + mbedtls_free(tmp_san_dirname); + tmp_san_dirname = NULL; } else { mbedtls_free(cur); goto usage; @@ -998,7 +1004,6 @@ exit: #if defined(MBEDTLS_X509_CSR_PARSE_C) mbedtls_x509_csr_free(&csr); #endif /* MBEDTLS_X509_CSR_PARSE_C */ - mbedtls_asn1_free_named_data_list(&ext_san_dirname); mbedtls_x509_crt_free(&issuer_crt); mbedtls_x509write_crt_free(&crt); mbedtls_pk_free(&loaded_subject_key); From 0803df29fc5f69628eac03ffc7b2274542b6bcd3 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Manuel=20P=C3=A9gouri=C3=A9-Gonnard?= Date: Mon, 5 May 2025 17:31:35 +0200 Subject: [PATCH 14/52] Fix memory leak in cert_write & cert_req MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit That memory leak had been present ever since the san command-line argument has been added. Tested that the following invocation is now fully valgrind clean: programs/x509/cert_write san=DN:C=NL,CN=#0000,CN=foo;DN:CN=#0000,O=foo,OU=bar,C=UK;IP:1.2.3.4;IP:4.3.2.1;URI:http\\://example.org/;URI:foo;DNS:foo.example.org;DNS:bar.example.org Signed-off-by: Manuel Pégourié-Gonnard --- programs/x509/cert_req.c | 17 +++++++++++++++++ programs/x509/cert_write.c | 17 +++++++++++++++++ 2 files changed, 34 insertions(+) diff --git a/programs/x509/cert_req.c b/programs/x509/cert_req.c index c399021916..26aed93e4f 100644 --- a/programs/x509/cert_req.c +++ b/programs/x509/cert_req.c @@ -497,6 +497,23 @@ exit: #endif } + cur = opt.san_list; + while (cur != NULL) { + mbedtls_x509_san_list *next = cur->next; + /* Note: mbedtls_x509_free_subject_alt_name() is not what we want here. + * It's the right thing for entries that were parsed from a certificate, + * where pointers are to the raw certificate, but here all the + * pointers were allocated while parsing from a user-provided string. */ + if (cur->node.type == MBEDTLS_X509_SAN_DIRECTORY_NAME) { + mbedtls_x509_name dn = cur->node.san.directory_name; + mbedtls_free(dn.oid.p); + mbedtls_free(dn.val.p); + mbedtls_asn1_free_named_data_list(&dn.next); + } + mbedtls_free(cur); + cur = next; + } + mbedtls_x509write_csr_free(&req); mbedtls_pk_free(&key); mbedtls_ctr_drbg_free(&ctr_drbg); diff --git a/programs/x509/cert_write.c b/programs/x509/cert_write.c index 63872a953f..d46470274a 100644 --- a/programs/x509/cert_write.c +++ b/programs/x509/cert_write.c @@ -1001,6 +1001,23 @@ usage: exit_code = MBEDTLS_EXIT_SUCCESS; exit: + cur = opt.san_list; + while (cur != NULL) { + mbedtls_x509_san_list *next = cur->next; + /* Note: mbedtls_x509_free_subject_alt_name() is not what we want here. + * It's the right thing for entries that were parsed from a certificate, + * where pointers are to the raw certificate, but here all the + * pointers were allocated while parsing from a user-provided string. */ + if (cur->node.type == MBEDTLS_X509_SAN_DIRECTORY_NAME) { + mbedtls_x509_name dn = cur->node.san.directory_name; + mbedtls_free(dn.oid.p); + mbedtls_free(dn.val.p); + mbedtls_asn1_free_named_data_list(&dn.next); + } + mbedtls_free(cur); + cur = next; + } + #if defined(MBEDTLS_X509_CSR_PARSE_C) mbedtls_x509_csr_free(&csr); #endif /* MBEDTLS_X509_CSR_PARSE_C */ From f9ac5e772832010aad14e7ed0f7da7dd515002e9 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Manuel=20P=C3=A9gouri=C3=A9-Gonnard?= Date: Mon, 5 May 2025 18:25:26 +0200 Subject: [PATCH 15/52] Add unit test for new behaviour of string_to_names() MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Manuel Pégourié-Gonnard --- tests/suites/test_suite_x509write.function | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/tests/suites/test_suite_x509write.function b/tests/suites/test_suite_x509write.function index 81816fe63f..a22c486def 100644 --- a/tests/suites/test_suite_x509write.function +++ b/tests/suites/test_suite_x509write.function @@ -731,6 +731,11 @@ void mbedtls_x509_string_to_names(char *name, char *parsed_name, TEST_LE_S(1, ret); TEST_ASSERT(strcmp((char *) out, parsed_name) == 0); + /* Check that calling a 2nd time with the same param (now non-NULL) + * returns an error as expected. */ + ret = mbedtls_x509_string_to_names(&names, name); + TEST_EQUAL(ret, MBEDTLS_ERR_X509_BAD_INPUT_DATA); + exit: mbedtls_asn1_free_named_data_list(&names); From ddbf8d030a32c5b59afc9975bca180fd9ce28d8e Mon Sep 17 00:00:00 2001 From: David Horstmann Date: Wed, 14 May 2025 15:45:00 +0100 Subject: [PATCH 16/52] Add credit to the reporters of the PKCS7 issue Signed-off-by: David Horstmann --- ChangeLog.d/pkcs7-padding-side-channel-fix.txt | 2 ++ 1 file changed, 2 insertions(+) diff --git a/ChangeLog.d/pkcs7-padding-side-channel-fix.txt b/ChangeLog.d/pkcs7-padding-side-channel-fix.txt index b813b84ce8..c5cbc75353 100644 --- a/ChangeLog.d/pkcs7-padding-side-channel-fix.txt +++ b/ChangeLog.d/pkcs7-padding-side-channel-fix.txt @@ -2,3 +2,5 @@ Security * Fix a timing side channel in the implementation of PKCS#7 padding which would allow an attacker who can request decryption of arbitrary ciphertexts to recover the plaintext through a timing oracle attack. + Reported by Ka Lok Wu from Stony Brook University and Doria Tang from + The Chinese University of Hong Kong. From 35f2220e37cce029fa7f22f5dd579bd61b2a59b0 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Manuel=20P=C3=A9gouri=C3=A9-Gonnard?= Date: Mon, 19 May 2025 12:21:32 +0200 Subject: [PATCH 17/52] Remove redundant free loop MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This version is incomplete. I failed to noticed it when adding a more complete version, making the existing one redundant. Signed-off-by: Manuel Pégourié-Gonnard --- programs/x509/cert_req.c | 24 ++++++++---------------- 1 file changed, 8 insertions(+), 16 deletions(-) diff --git a/programs/x509/cert_req.c b/programs/x509/cert_req.c index 26aed93e4f..97b2cccf2d 100644 --- a/programs/x509/cert_req.c +++ b/programs/x509/cert_req.c @@ -497,6 +497,14 @@ exit: #endif } + mbedtls_x509write_csr_free(&req); + mbedtls_pk_free(&key); + mbedtls_ctr_drbg_free(&ctr_drbg); + mbedtls_entropy_free(&entropy); +#if defined(MBEDTLS_USE_PSA_CRYPTO) + mbedtls_psa_crypto_free(); +#endif /* MBEDTLS_USE_PSA_CRYPTO */ + cur = opt.san_list; while (cur != NULL) { mbedtls_x509_san_list *next = cur->next; @@ -514,22 +522,6 @@ exit: cur = next; } - mbedtls_x509write_csr_free(&req); - mbedtls_pk_free(&key); - mbedtls_ctr_drbg_free(&ctr_drbg); - mbedtls_entropy_free(&entropy); -#if defined(MBEDTLS_USE_PSA_CRYPTO) - mbedtls_psa_crypto_free(); -#endif /* MBEDTLS_USE_PSA_CRYPTO */ - - cur = opt.san_list; - while (cur != NULL) { - prev = cur; - cur = cur->next; - mbedtls_free(prev); - } - - mbedtls_exit(exit_code); } #endif /* MBEDTLS_X509_CSR_WRITE_C && MBEDTLS_PK_PARSE_C && MBEDTLS_FS_IO && From 8a6fc08607f538c5a753ab6b58b228264fe92a73 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Manuel=20P=C3=A9gouri=C3=A9-Gonnard?= Date: Mon, 19 May 2025 12:28:42 +0200 Subject: [PATCH 18/52] Add comment on apparent type mismatch MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Manuel Pégourié-Gonnard --- programs/x509/cert_req.c | 5 ++++- programs/x509/cert_write.c | 5 ++++- 2 files changed, 8 insertions(+), 2 deletions(-) diff --git a/programs/x509/cert_req.c b/programs/x509/cert_req.c index 97b2cccf2d..cefdf179a9 100644 --- a/programs/x509/cert_req.c +++ b/programs/x509/cert_req.c @@ -276,7 +276,10 @@ usage: /* Work around an API mismatch between string_to_names() and * mbedtls_x509_subject_alternative_name, which holds an * actual mbedtls_x509_name while a pointer to one would be - * more convenient here. */ + * more convenient here. (Note mbedtls_x509_name and + * mbedtls_asn1_named_data are synonymous, again + * string_to_names() uses one while + * cur->node.san.directory_name is nominally the other.) */ mbedtls_asn1_named_data *tmp_san_dirname = NULL; if ((ret = mbedtls_x509_string_to_names(&tmp_san_dirname, subtype_value)) != 0) { diff --git a/programs/x509/cert_write.c b/programs/x509/cert_write.c index d46470274a..478dab71d6 100644 --- a/programs/x509/cert_write.c +++ b/programs/x509/cert_write.c @@ -597,7 +597,10 @@ usage: /* Work around an API mismatch between string_to_names() and * mbedtls_x509_subject_alternative_name, which holds an * actual mbedtls_x509_name while a pointer to one would be - * more convenient here. */ + * more convenient here. (Note mbedtls_x509_name and + * mbedtls_asn1_named_data are synonymous, again + * string_to_names() uses one while + * cur->node.san.directory_name is nominally the other.) */ mbedtls_asn1_named_data *tmp_san_dirname = NULL; if ((ret = mbedtls_x509_string_to_names(&tmp_san_dirname, subtype_value)) != 0) { From 8429619a921e3ea2e074015009ac153b03724681 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Manuel=20P=C3=A9gouri=C3=A9-Gonnard?= Date: Mon, 19 May 2025 12:29:11 +0200 Subject: [PATCH 19/52] Fix type in ChangeLog MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Manuel Pégourié-Gonnard --- ChangeLog.d/fix-string-to-names-memory-management.txt | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/ChangeLog.d/fix-string-to-names-memory-management.txt b/ChangeLog.d/fix-string-to-names-memory-management.txt index 1b2198287d..87bc59694f 100644 --- a/ChangeLog.d/fix-string-to-names-memory-management.txt +++ b/ChangeLog.d/fix-string-to-names-memory-management.txt @@ -13,6 +13,6 @@ Security Changes * The function mbedtls_x509_string_to_names() now requires its head argument - to point to NULL on entry. This make it likely that existing risky uses of + to point to NULL on entry. This makes it likely that existing risky uses of this function (see the entry in the Security section) will be detected and fixed. From 8ac3eb9833f4447489b77c92d7f995a63739f6f4 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Manuel=20P=C3=A9gouri=C3=A9-Gonnard?= Date: Wed, 21 May 2025 11:17:39 +0200 Subject: [PATCH 20/52] Avoid a useless copy in cert_{req,write} MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit I'm just trying to have a shorter name to avoid repeating a long expression. This is a job for a pointer, not copying a struct. Signed-off-by: Manuel Pégourié-Gonnard --- programs/x509/cert_req.c | 8 ++++---- programs/x509/cert_write.c | 8 ++++---- 2 files changed, 8 insertions(+), 8 deletions(-) diff --git a/programs/x509/cert_req.c b/programs/x509/cert_req.c index cefdf179a9..38b7af0925 100644 --- a/programs/x509/cert_req.c +++ b/programs/x509/cert_req.c @@ -516,10 +516,10 @@ exit: * where pointers are to the raw certificate, but here all the * pointers were allocated while parsing from a user-provided string. */ if (cur->node.type == MBEDTLS_X509_SAN_DIRECTORY_NAME) { - mbedtls_x509_name dn = cur->node.san.directory_name; - mbedtls_free(dn.oid.p); - mbedtls_free(dn.val.p); - mbedtls_asn1_free_named_data_list(&dn.next); + mbedtls_x509_name *dn = &cur->node.san.directory_name; + mbedtls_free(dn->oid.p); + mbedtls_free(dn->val.p); + mbedtls_asn1_free_named_data_list(&dn->next); } mbedtls_free(cur); cur = next; diff --git a/programs/x509/cert_write.c b/programs/x509/cert_write.c index 478dab71d6..95f08b9127 100644 --- a/programs/x509/cert_write.c +++ b/programs/x509/cert_write.c @@ -1012,10 +1012,10 @@ exit: * where pointers are to the raw certificate, but here all the * pointers were allocated while parsing from a user-provided string. */ if (cur->node.type == MBEDTLS_X509_SAN_DIRECTORY_NAME) { - mbedtls_x509_name dn = cur->node.san.directory_name; - mbedtls_free(dn.oid.p); - mbedtls_free(dn.val.p); - mbedtls_asn1_free_named_data_list(&dn.next); + mbedtls_x509_name *dn = &cur->node.san.directory_name; + mbedtls_free(dn->oid.p); + mbedtls_free(dn->val.p); + mbedtls_asn1_free_named_data_list(&dn->next); } mbedtls_free(cur); cur = next; From 13f86e689e64cc121b469822ea957d5f61c31900 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Manuel=20P=C3=A9gouri=C3=A9-Gonnard?= Date: Wed, 21 May 2025 14:35:42 +0200 Subject: [PATCH 21/52] Add tests for bug in mbedtls_x509_string_to_names() MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The commented out tests cause crashes (in different ways) until the bug is fixed; the first two test are passing already and are here mostly to provide a reference point. The bug report was using programs/x509/cert_write, but string_to_names() is what it was really targetting, which is better for automated tests. The strings used are a minor adapation of those from the report. Signed-off-by: Manuel Pégourié-Gonnard --- tests/suites/test_suite_x509write.data | 21 +++++++++++++++++++++ 1 file changed, 21 insertions(+) diff --git a/tests/suites/test_suite_x509write.data b/tests/suites/test_suite_x509write.data index 0cbad4bcee..7bd5e74e1f 100644 --- a/tests/suites/test_suite_x509write.data +++ b/tests/suites/test_suite_x509write.data @@ -254,6 +254,27 @@ mbedtls_x509_string_to_names:"C=NL, O=Of\\CCspark, OU=PolarSSL":"C=NL, O=Of\\CCs X509 String to Names #20 (Reject empty AttributeValue) mbedtls_x509_string_to_names:"C=NL, O=, OU=PolarSSL":"":MBEDTLS_ERR_X509_INVALID_NAME:0 +# Note: the behaviour is incorrect, output from string->names->string should be +# the same as the input, rather than just the last component, see +# https://github.com/Mbed-TLS/mbedtls/issues/10189 +# Still including tests for the current incorrect behaviour because of the +# variants below where we want to ensure at least that no memory corruption +# happens (which would be a lot worse than just a functional bug). +X509 String to Names (repeated OID) +mbedtls_x509_string_to_names:"CN=ab,CN=cd,CN=ef":"CN=ef":0:0 + +# Note: when a value starts with a # sign, it's treated as the hex encoding of +# the DER encoding of the value. Here, 0400 is a zero-length OCTET STRING. +# The tag actually doesn't matter for our purposes, only the length. +X509 String to Names (repeated OID, 1st is zero-length) +mbedtls_x509_string_to_names:"CN=#0400,CN=cd,CN=ef":"CN=ef":0:0 + +#X509 String to Names (repeated OID, middle is zero-length) +#mbedtls_x509_string_to_names:"CN=ab,CN=#0400,CN=ef":"CN=ef":0:0 + +#X509 String to Names (repeated OID, last is zero-length) +#mbedtls_x509_string_to_names:"CN=ab,CN=cd,CN=#0400":"CN=ef":0:0 + X509 Round trip test (Escaped characters) mbedtls_x509_string_to_names:"CN=Lu\\C4\\8Di\\C4\\87, O=Offspark, OU=PolarSSL":"CN=Lu\\C4\\8Di\\C4\\87, O=Offspark, OU=PolarSSL":0:0 From 2df7ab7c0c3d5bb8a31481073c494521d10d4eba Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Manuel=20P=C3=A9gouri=C3=A9-Gonnard?= Date: Mon, 26 May 2025 10:42:14 +0200 Subject: [PATCH 22/52] Fix bug in mbedtls_asn1_store_named_data() MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit When passed a zero-length val, the function was free-ing the buffer as the documentation suggests: * \param val_len The minimum length of the data buffer needed. * If this is 0, do not allocate a buffer for the associated * data. * If the OID was already present, enlarge, shrink or free * the existing buffer to fit \p val_len. However it kept the previous length, leaving the val structure in the corresponding item in the output list in an inconsistent state: p == NULL but len != 0 As a result, functions that would try using this item in the list (including the same function!) afterwards would trip an dereference the NULL pointer. Signed-off-by: Manuel Pégourié-Gonnard --- library/asn1write.c | 1 + tests/suites/test_suite_x509write.data | 8 ++++---- 2 files changed, 5 insertions(+), 4 deletions(-) diff --git a/library/asn1write.c b/library/asn1write.c index 775a9ef530..415357b7b5 100644 --- a/library/asn1write.c +++ b/library/asn1write.c @@ -412,6 +412,7 @@ mbedtls_asn1_named_data *mbedtls_asn1_store_named_data( } else if (val_len == 0) { mbedtls_free(cur->val.p); cur->val.p = NULL; + cur->val.len = 0; } else if (cur->val.len != val_len) { /* * Enlarge existing value buffer if needed diff --git a/tests/suites/test_suite_x509write.data b/tests/suites/test_suite_x509write.data index 7bd5e74e1f..56af0ce8a8 100644 --- a/tests/suites/test_suite_x509write.data +++ b/tests/suites/test_suite_x509write.data @@ -269,11 +269,11 @@ mbedtls_x509_string_to_names:"CN=ab,CN=cd,CN=ef":"CN=ef":0:0 X509 String to Names (repeated OID, 1st is zero-length) mbedtls_x509_string_to_names:"CN=#0400,CN=cd,CN=ef":"CN=ef":0:0 -#X509 String to Names (repeated OID, middle is zero-length) -#mbedtls_x509_string_to_names:"CN=ab,CN=#0400,CN=ef":"CN=ef":0:0 +X509 String to Names (repeated OID, middle is zero-length) +mbedtls_x509_string_to_names:"CN=ab,CN=#0400,CN=ef":"CN=ef":0:0 -#X509 String to Names (repeated OID, last is zero-length) -#mbedtls_x509_string_to_names:"CN=ab,CN=cd,CN=#0400":"CN=ef":0:0 +X509 String to Names (repeated OID, last is zero-length) +mbedtls_x509_string_to_names:"CN=ab,CN=cd,CN=#0400":"CN=#0000":0:MAY_FAIL_GET_NAME X509 Round trip test (Escaped characters) mbedtls_x509_string_to_names:"CN=Lu\\C4\\8Di\\C4\\87, O=Offspark, OU=PolarSSL":"CN=Lu\\C4\\8Di\\C4\\87, O=Offspark, OU=PolarSSL":0:0 From 12df5f3a16a2bc53881b9d88556192dcbc2f6774 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Manuel=20P=C3=A9gouri=C3=A9-Gonnard?= Date: Mon, 26 May 2025 10:55:59 +0200 Subject: [PATCH 23/52] Improve unit tests for mbedtls_asn1_store_named_data MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Every time we check found->val.p we should also check found->val.len. Signed-off-by: Manuel Pégourié-Gonnard --- tests/suites/test_suite_asn1write.function | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/tests/suites/test_suite_asn1write.function b/tests/suites/test_suite_asn1write.function index f5fc025f7d..b1e66ed901 100644 --- a/tests/suites/test_suite_asn1write.function +++ b/tests/suites/test_suite_asn1write.function @@ -550,6 +550,9 @@ void store_named_data_val_found(int old_len, int new_len) } if (new_len == 0) { TEST_ASSERT(found->val.p == NULL); + /* If new_len != 0, then new_val != NULL and the length has been checked + * above by TEST_MEMORY_COMPARE(). But not here, so we need to check. */ + TEST_EQUAL(found->val.len, 0); } else if (new_len == old_len) { TEST_ASSERT(found->val.p == old_val); } else { @@ -583,8 +586,10 @@ void store_named_data_val_new(int new_len, int set_new_val) TEST_MEMORY_COMPARE(found->oid.p, found->oid.len, oid, oid_len); if (new_len == 0) { TEST_ASSERT(found->val.p == NULL); + TEST_EQUAL(found->val.len, 0); } else if (new_val == NULL) { TEST_ASSERT(found->val.p != NULL); + TEST_EQUAL(found->val.len, new_len); } else { TEST_ASSERT(found->val.p != new_val); TEST_MEMORY_COMPARE(found->val.p, found->val.len, From 04fe95d95b1609b7a28e5665bd86066f58f00484 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Manuel=20P=C3=A9gouri=C3=A9-Gonnard?= Date: Mon, 26 May 2025 12:38:52 +0200 Subject: [PATCH 24/52] Add ChangeLog entry MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Manuel Pégourié-Gonnard --- ChangeLog.d/fix-string-to-names-store-named-data.txt | 12 ++++++++++++ 1 file changed, 12 insertions(+) create mode 100644 ChangeLog.d/fix-string-to-names-store-named-data.txt diff --git a/ChangeLog.d/fix-string-to-names-store-named-data.txt b/ChangeLog.d/fix-string-to-names-store-named-data.txt new file mode 100644 index 0000000000..422ce07f85 --- /dev/null +++ b/ChangeLog.d/fix-string-to-names-store-named-data.txt @@ -0,0 +1,12 @@ +Security + * Fix a bug in mbedtls_asn1_store_named_data() where it would sometimes leave + an item in the output list in an inconsistent state with val.p == NULL but + val.len > 0. This impacts applications that call this function directly, + or indirectly via mbedtls_x509_string_to_names() or one of the + mbedtls_x509write_{crt,csr}_set_{subject,issuer}_name() functions. The + inconsistent state of the output could then cause a NULL dereference either + inside the same call to mbedtls_x509_string_to_names(), or in subsequent + users of the output structure, such as mbedtls_x509_write_names(). This + only affects applications that create (as opposed to consume) X.509 + certificates, CSRs or CRLS, or that call mbedtls_asn1_store_named_data() + directly. Found by Linh Le and Ngan Nguyen from Calif. From e51bde06daff0ac92f0545ebe4406bda260542e5 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Manuel=20P=C3=A9gouri=C3=A9-Gonnard?= Date: Tue, 3 Jun 2025 11:22:55 +0200 Subject: [PATCH 25/52] Fix possible UB in mbedtls_asn1_write_raw_buffer() MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This is mostly unrelated to other commits in this PR, except for the fact that one of the added X.509 tests revealed that with UBSan. Signed-off-by: Manuel Pégourié-Gonnard --- ChangeLog.d/fix-asn1write-raw-buffer.txt | 5 +++++ library/asn1write.c | 4 +++- 2 files changed, 8 insertions(+), 1 deletion(-) create mode 100644 ChangeLog.d/fix-asn1write-raw-buffer.txt diff --git a/ChangeLog.d/fix-asn1write-raw-buffer.txt b/ChangeLog.d/fix-asn1write-raw-buffer.txt new file mode 100644 index 0000000000..292631aabc --- /dev/null +++ b/ChangeLog.d/fix-asn1write-raw-buffer.txt @@ -0,0 +1,5 @@ +Bugfix + * When calling mbedtls_asn1_write_raw_buffer() with NULL, 0 as the last two + arguments, undefined behaviour would be triggered, in the form of a call to + memcpy(..., NULL, 0). This was harmless in practice, but could trigger + complains from sanitizers or static analyzers. diff --git a/library/asn1write.c b/library/asn1write.c index 415357b7b5..97f9db039b 100644 --- a/library/asn1write.c +++ b/library/asn1write.c @@ -90,7 +90,9 @@ int mbedtls_asn1_write_raw_buffer(unsigned char **p, const unsigned char *start, len = size; (*p) -= len; - memcpy(*p, buf, len); + if (len != 0) { + memcpy(*p, buf, len); + } return (int) len; } From 9325883d9fb270cae63af5a254eb6a855813a189 Mon Sep 17 00:00:00 2001 From: Felix Conway Date: Tue, 27 May 2025 14:54:07 +0100 Subject: [PATCH 26/52] Add test using underflow-causing PEM keyfile Signed-off-by: Felix Conway --- tests/suites/test_suite_pem.data | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/tests/suites/test_suite_pem.data b/tests/suites/test_suite_pem.data index 007ba104a9..1df9645650 100644 --- a/tests/suites/test_suite_pem.data +++ b/tests/suites/test_suite_pem.data @@ -53,6 +53,10 @@ PEM read (malformed PEM AES-128-CBC) 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) +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:"" + # 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 # length read from the decoded ASN.1 data (i.e. extra padding, if any, is ignored). From 6165e715899a9b370851e2868fe312d7e0a2cb83 Mon Sep 17 00:00:00 2001 From: Felix Conway Date: Tue, 27 May 2025 16:00:48 +0100 Subject: [PATCH 27/52] Add fix for PEM underflow Signed-off-by: Felix Conway --- library/pem.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/library/pem.c b/library/pem.c index 0207601456..119fd59e12 100644 --- a/library/pem.c +++ b/library/pem.c @@ -243,7 +243,10 @@ exit: #if defined(MBEDTLS_DES_C) || defined(MBEDTLS_AES_C) static int pem_check_pkcs_padding(unsigned char *input, size_t input_len, size_t *data_len) { - /* input_len > 0 is guaranteed by mbedtls_pem_read_buffer(). */ + /* input_len > 0 is not guaranteed by mbedtls_pem_read_buffer(). */ + if (input_len < 1) { + return MBEDTLS_ERR_PEM_INVALID_DATA; + } size_t pad_len = input[input_len - 1]; size_t i; From 42323eacc9ca7ca8c6f14bb2e5a8b34349f29c6a Mon Sep 17 00:00:00 2001 From: Felix Conway Date: Tue, 27 May 2025 16:01:07 +0100 Subject: [PATCH 28/52] Add changelog Signed-off-by: Felix Conway --- ChangeLog.d/pem-integer-underflow.txt | 5 +++++ 1 file changed, 5 insertions(+) create mode 100644 ChangeLog.d/pem-integer-underflow.txt diff --git a/ChangeLog.d/pem-integer-underflow.txt b/ChangeLog.d/pem-integer-underflow.txt new file mode 100644 index 0000000000..77274aa279 --- /dev/null +++ b/ChangeLog.d/pem-integer-underflow.txt @@ -0,0 +1,5 @@ +Security + * Fix an integer underflow that could occur when parsing malformed PEM + keys, which could be used by an attacker capable of feeding encrypted + PEM keys to a user. This could cause a crash or information disclosure. + Found and reported by Linh Le and Ngan Nguyen from Calif. From 548e2dbf6511cc004c647657e43e848b83a19d2b Mon Sep 17 00:00:00 2001 From: Minos Galanakis Date: Mon, 2 Jun 2025 14:17:38 +0100 Subject: [PATCH 29/52] Built-in lms driver: Added input guard Signed-off-by: Minos Galanakis --- library/lms.c | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/library/lms.c b/library/lms.c index 7f7bec068b..4bdfd434ad 100644 --- a/library/lms.c +++ b/library/lms.c @@ -242,6 +242,10 @@ int mbedtls_lms_import_public_key(mbedtls_lms_public_t *ctx, mbedtls_lms_algorithm_type_t type; mbedtls_lmots_algorithm_type_t otstype; + if (key_size < 4) { + return MBEDTLS_ERR_LMS_BAD_INPUT_DATA; + } + type = (mbedtls_lms_algorithm_type_t) MBEDTLS_GET_UINT32_BE(key, PUBLIC_KEY_TYPE_OFFSET); if (type != MBEDTLS_LMS_SHA256_M32_H10) { return MBEDTLS_ERR_LMS_BAD_INPUT_DATA; From caaffc1e7ec1a120821dea1f00a441e7b271c63d Mon Sep 17 00:00:00 2001 From: Minos Galanakis Date: Mon, 2 Jun 2025 14:28:31 +0100 Subject: [PATCH 30/52] Built-in lms/lmots driver: Harden public key import against enum truncation Signed-off-by: Minos Galanakis --- library/lmots.c | 7 +++++-- library/lms.c | 16 ++++++---------- 2 files changed, 11 insertions(+), 12 deletions(-) diff --git a/library/lmots.c b/library/lmots.c index c51cb41ece..404aa80da6 100644 --- a/library/lmots.c +++ b/library/lmots.c @@ -401,8 +401,11 @@ int mbedtls_lmots_import_public_key(mbedtls_lmots_public_t *ctx, return MBEDTLS_ERR_LMS_BAD_INPUT_DATA; } - ctx->params.type = (mbedtls_lmots_algorithm_type_t) - MBEDTLS_GET_UINT32_BE(key, MBEDTLS_LMOTS_SIG_TYPE_OFFSET); + uint32_t type = MBEDTLS_GET_UINT32_BE(key, MBEDTLS_LMOTS_SIG_TYPE_OFFSET); + if (type != (uint32_t) MBEDTLS_LMOTS_SHA256_N32_W8) { + return MBEDTLS_ERR_LMS_BAD_INPUT_DATA; + } + ctx->params.type = (mbedtls_lmots_algorithm_type_t) type; if (key_len != MBEDTLS_LMOTS_PUBLIC_KEY_LEN(ctx->params.type)) { return MBEDTLS_ERR_LMS_BAD_INPUT_DATA; diff --git a/library/lms.c b/library/lms.c index 4bdfd434ad..8d1840cabf 100644 --- a/library/lms.c +++ b/library/lms.c @@ -239,29 +239,25 @@ void mbedtls_lms_public_free(mbedtls_lms_public_t *ctx) int mbedtls_lms_import_public_key(mbedtls_lms_public_t *ctx, const unsigned char *key, size_t key_size) { - mbedtls_lms_algorithm_type_t type; - mbedtls_lmots_algorithm_type_t otstype; - if (key_size < 4) { return MBEDTLS_ERR_LMS_BAD_INPUT_DATA; } - type = (mbedtls_lms_algorithm_type_t) MBEDTLS_GET_UINT32_BE(key, PUBLIC_KEY_TYPE_OFFSET); - if (type != MBEDTLS_LMS_SHA256_M32_H10) { + uint32_t type = MBEDTLS_GET_UINT32_BE(key, PUBLIC_KEY_TYPE_OFFSET); + if (type != (uint32_t) MBEDTLS_LMS_SHA256_M32_H10) { return MBEDTLS_ERR_LMS_BAD_INPUT_DATA; } - ctx->params.type = type; + ctx->params.type = (mbedtls_lms_algorithm_type_t) type; if (key_size != MBEDTLS_LMS_PUBLIC_KEY_LEN(ctx->params.type)) { return MBEDTLS_ERR_LMS_BAD_INPUT_DATA; } - otstype = (mbedtls_lmots_algorithm_type_t) - MBEDTLS_GET_UINT32_BE(key, PUBLIC_KEY_OTSTYPE_OFFSET); - if (otstype != MBEDTLS_LMOTS_SHA256_N32_W8) { + uint32_t otstype = MBEDTLS_GET_UINT32_BE(key, PUBLIC_KEY_OTSTYPE_OFFSET); + if (otstype != (uint32_t) MBEDTLS_LMOTS_SHA256_N32_W8) { return MBEDTLS_ERR_LMS_BAD_INPUT_DATA; } - ctx->params.otstype = otstype; + ctx->params.otstype = (mbedtls_lmots_algorithm_type_t) otstype; memcpy(ctx->params.I_key_identifier, key + PUBLIC_KEY_I_KEY_ID_OFFSET, From ae449bfca5e9a9a7b1cf1ec777755e33c79488e4 Mon Sep 17 00:00:00 2001 From: Minos Galanakis Date: Mon, 2 Jun 2025 14:33:47 +0100 Subject: [PATCH 31/52] Built-in lms driver:Check return values of Merkle node creation Signed-off-by: Minos Galanakis --- library/lms.c | 14 ++++++++++---- 1 file changed, 10 insertions(+), 4 deletions(-) diff --git a/library/lms.c b/library/lms.c index 8d1840cabf..78886ab78d 100644 --- a/library/lms.c +++ b/library/lms.c @@ -374,12 +374,16 @@ int mbedtls_lms_verify(const mbedtls_lms_public_t *ctx, return MBEDTLS_ERR_LMS_VERIFY_FAILED; } - create_merkle_leaf_value( + ret = create_merkle_leaf_value( &ctx->params, Kc_candidate_ots_pub_key, MERKLE_TREE_INTERNAL_NODE_AM(ctx->params.type) + q_leaf_identifier, Tc_candidate_root_node); + if (ret != 0) { + return MBEDTLS_ERR_LMS_VERIFY_FAILED; + } + curr_node_id = MERKLE_TREE_INTERNAL_NODE_AM(ctx->params.type) + q_leaf_identifier; @@ -398,9 +402,11 @@ int mbedtls_lms_verify(const mbedtls_lms_public_t *ctx, height * MBEDTLS_LMS_M_NODE_BYTES(ctx->params.type); } - create_merkle_internal_value(&ctx->params, left_node, right_node, - parent_node_id, Tc_candidate_root_node); - + ret = create_merkle_internal_value(&ctx->params, left_node, right_node, + parent_node_id, Tc_candidate_root_node); + if (ret != 0) { + return MBEDTLS_ERR_LMS_VERIFY_FAILED; + } curr_node_id /= 2; } From 9b3051fb1060e3d783bf9f900295eace740b1c66 Mon Sep 17 00:00:00 2001 From: Minos Galanakis Date: Mon, 2 Jun 2025 14:36:25 +0100 Subject: [PATCH 32/52] Built-in lms driver: always zeroize output-buffer in create_merkle_leaf_value Signed-off-by: Minos Galanakis --- library/lms.c | 3 +++ 1 file changed, 3 insertions(+) diff --git a/library/lms.c b/library/lms.c index 78886ab78d..11e2508954 100644 --- a/library/lms.c +++ b/library/lms.c @@ -101,6 +101,9 @@ static int create_merkle_leaf_value(const mbedtls_lms_parameters_t *params, size_t output_hash_len; unsigned char r_node_idx_bytes[4]; + /* Always zeroize the output buffer to avoid undefined behavior at an early exit */ + memset(out, 0, MBEDTLS_LMS_M_NODE_BYTES(params->type)); + op = psa_hash_operation_init(); status = psa_hash_setup(&op, PSA_ALG_SHA_256); if (status != PSA_SUCCESS) { From 3b392af70d9766425b8592b567a4a79914901203 Mon Sep 17 00:00:00 2001 From: Minos Galanakis Date: Mon, 2 Jun 2025 14:37:54 +0100 Subject: [PATCH 33/52] Added changelog for lms overread Signed-off-by: Minos Galanakis --- ChangeLog.d/1351_lms_overread.txt | 3 +++ 1 file changed, 3 insertions(+) create mode 100644 ChangeLog.d/1351_lms_overread.txt diff --git a/ChangeLog.d/1351_lms_overread.txt b/ChangeLog.d/1351_lms_overread.txt new file mode 100644 index 0000000000..43249f70a5 --- /dev/null +++ b/ChangeLog.d/1351_lms_overread.txt @@ -0,0 +1,3 @@ +Security + * Fix a buffer overread in mbedtls_lms_import_public_key() when the input is + less than 3 bytes. Reported by Linh Le and Ngan Nguyen from Calif. From 3444757ac475cdd2eddc8e08154d0df71ad488d0 Mon Sep 17 00:00:00 2001 From: Minos Galanakis Date: Mon, 2 Jun 2025 14:38:26 +0100 Subject: [PATCH 34/52] Added changelog for lms enum casting Signed-off-by: Minos Galanakis --- ChangeLog.d/1352_lms_enum_casting.txt | 6 ++++++ 1 file changed, 6 insertions(+) create mode 100644 ChangeLog.d/1352_lms_enum_casting.txt diff --git a/ChangeLog.d/1352_lms_enum_casting.txt b/ChangeLog.d/1352_lms_enum_casting.txt new file mode 100644 index 0000000000..de66d2854c --- /dev/null +++ b/ChangeLog.d/1352_lms_enum_casting.txt @@ -0,0 +1,6 @@ +Bugfix + * Fix a sloppy check in LMS public key import, which could lead to accepting + keys with a different LMS or LM-OTS types on some platforms. Specifically, + this could happen on platforms where enum types are smaller than 32 bits + and compiler optimization is enabled. Found and reported by Linh Le and + Ngan Nguyen from Calif. From f84bc3f59246c6077664d7a32278836103dccb80 Mon Sep 17 00:00:00 2001 From: Minos Galanakis Date: Mon, 2 Jun 2025 14:38:55 +0100 Subject: [PATCH 35/52] Added changelog for check return of merkle leaf Signed-off-by: Minos Galanakis --- ChangeLog.d/1353_lms_check_return_of_merkle_leaf.txt | 4 ++++ 1 file changed, 4 insertions(+) create mode 100644 ChangeLog.d/1353_lms_check_return_of_merkle_leaf.txt diff --git a/ChangeLog.d/1353_lms_check_return_of_merkle_leaf.txt b/ChangeLog.d/1353_lms_check_return_of_merkle_leaf.txt new file mode 100644 index 0000000000..9feca99ba7 --- /dev/null +++ b/ChangeLog.d/1353_lms_check_return_of_merkle_leaf.txt @@ -0,0 +1,4 @@ +Security + * Fix a vulnerability in LMS verification through which an adversary could + get an invalid signature accepted if they could cause a hash accelerator + to fail. Found and reported by Linh Le and Ngan Nguyen from Calif. From b72573853a68e454dd490fd568e28b54efa09e11 Mon Sep 17 00:00:00 2001 From: Minos Galanakis Date: Mon, 2 Jun 2025 15:00:47 +0100 Subject: [PATCH 36/52] test_suite_lms: Added a test for importing invalid sized key Signed-off-by: Minos Galanakis --- tests/suites/test_suite_lms.data | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/tests/suites/test_suite_lms.data b/tests/suites/test_suite_lms.data index 16ebd1d217..9ce99ae86f 100644 --- a/tests/suites/test_suite_lms.data +++ b/tests/suites/test_suite_lms.data @@ -212,6 +212,12 @@ LMS negative test (invalid leaf ID) # test should fail to verify the signature. lms_verify_test:"bfff9cd687351db88a98c71fd2f9b927a0ee600130a112533b791041d30cb91665fc369a5ac7cc9a04547414ac45288081d19d4a600579c73ac4bc953de03ad6":"0000040000000004e474c96c496b231ecedcd92566ab3e1cdfac83f7e56abaae6571401e212e59bdbcc18105e0709249510d13d7ae1091558c217033316ac70a36aae764bd0f4032e369453c634b81061079d216d03d3c55976a1aabc00287b307297ae03587ba20839460daae85d26dfcef7638c10a1d8559612e5e9ced1a4205a52ca0ce88e58602e59cf9ab34adf2e958e56570573297b99f733fbbf58d2440526fd4dc15c5509e8a11405f6c08772abcf58731fbf9a73642670e3247c5f70905f0fa81f6174bf32977209923507525a170fcd260e81f04193583fbcd305ac245c80eb337ca326fd1105e73748fab8a1f0c8d8a99f011718e7aae027a34d2a85ff18769fa277810126a86b51b096a04d8e28a4fb8c5e14e50a67cb1ee88e43e5cc077902442f5d9c55ac2b8acdd76c67bc740c6083aba4e3cb404c23f1f3118337563fef6a4b01fb476810c5b5682d0aecdccd55c85a4af50e9150f7d83dcccd8e822a302e6e5a52e00505e6e65338dcfb9cfbe22594e9e18ebde36af29450c5ea31523019cf64fd6eca8c77d98c2a146dcedd51bf6c61c1f7cacbce3ab20c8606930cc42737e17f2703cf0980aef560768d1ac5585c05a60a5f94db15f5ac4d4df5cbd81430878d4e9b77346e3a6538b80b80873e3e6c37860470091979296631440adb8cc71991aba2a4dd2884764878306fe774a25512cfbf080f2829ea2903ffa748dd187c21aef918ee3436a1bd336c3d09cc1f748d7528db90a98f69078b82c4d23de7bcec092a292d2b8cac71a5c87d008f128b89a5e608a4501aef41e9f17e4056ed4767957188f780159daebf327751386980b0fca5a2d36b141acff957f46ce2381897099619475db9d3a613e7ef98b056f42b4d6eafb1d62eebbe46a7502f893fbd36ccfb12a280f45ffb93f050eb280bf0a6cd640abdea8590bffb98bdb29ee3a31daa0fa3ab35fee11dc1b7d1fcea82b0e284b2e35b34e77c3f401ed887e7fc6c97b327f76f99caca2f355afa2753a8923bfb06fb2a9df08d31c93882e34ef5a3cccc9d078855334bdf909ae418b177724c42fb1d586d212c4474932acce295236030f4379158957300fe9fdc5cc9145e3ded50cf9f5a8e19321961536c4a47fffc3eb4383fb78a5d2aeed5b45b92132b5e2a53e3b67841fa2e1bd217ee2c30812c4eb1bd4f8c85b328e23d27f12a2fad5c6b236c87f8fd1aad441416e53ebd4d45d4bf601b94eb37dc9a065218ae58e69dba1250bb20626baeda961b3ef11d217697e73f41fa3870d726a032bc4a388fb12c023822945df058e22f54e5f6377eab34297c57883515204fc189d0d4b0ad9bacb24acf7f9d55e7c6368bb8ababd7622f586ec22683306c5d88d5244219a3952adbd85c89893a441a58b532e15600cd5afdbb5b441e1670b72656c7995189bdf993154e09912db8c4ddaff0e75591720230cf99c8b71cd841dffc4372c5e0f9ff906a379d28d6884351609bf7c849ebfabfb049ae986bbb8467251dbf5ccdd05a86ff6ce1392f7ca1bd49595ad9ad805d71b389afab1865f7f69dc24662af19934f025ced212536509500c132aec000000068b991bed50319a6cb9ff040b92f1563889b3787c37145fc9737d4643f66ade33ebd85a2c29b8c64a581cff01b89d59807d6fade2d2c88872f77d0ed83d97c4b5438681d0b95feb973125e4ee70ebe11699290b831e86571e36513a71f159d48ce563f6814cc2a89851d2520c5275b34cc83614cab14c0d197166580d800ee6b9004b8fd72daac8d73c36c1623c37be93ba49a06c4efde238a3a10a05daba5d4942f7de52648af2be31f33e723b3605346282f5d5e356c5d0004eea40fe0b80abf658f6c96c56319ab53d7fefb5879e0136d1cf320973a2f47c1ee3d21554910f09d17afac4607657c4309890957a4954bf86e2a8491ba37dd88b2c3fe3c7edebd767c400bc23e40d165b27133c726b90bc26cbb2a86a6aa400c47aa7ffc538388de8490b9349afa53b814ffe2a56ff16a496e9648284193754f989f6f12aeb6e":"0000000600000004d96bb26744d99ef624e32161c36d3d6efcdd0484e2b17a6dd183125be4b1af1cda931a91a3acb1151877c174f7943fd9":MBEDTLS_ERR_LMS_VERIFY_FAILED +LMS key import out-of-bounds sized key test +# This test cuts the input data it down so it is MBEDTLS_LMS_TYPE_LEN -1 +# and imports it. This should fail, and not attempt to read invalidly +# outside the buffer. +lms_import_export_test:"000000":MBEDTLS_ERR_LMS_BAD_INPUT_DATA + LMS import/export test # This test uses the key from hsslms interop test 1, imports it, exports it and # tests that it is the same. It also checks if the export correctly fail when From c7beb847c918f045c1fcaf109119b794798fdf2d Mon Sep 17 00:00:00 2001 From: Minos Galanakis Date: Mon, 2 Jun 2025 15:52:14 +0100 Subject: [PATCH 37/52] test_suite_lms: Added negative test for corrupted Merkle path Signed-off-by: Minos Galanakis --- tests/suites/test_suite_lms.data | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/tests/suites/test_suite_lms.data b/tests/suites/test_suite_lms.data index 9ce99ae86f..51578de078 100644 --- a/tests/suites/test_suite_lms.data +++ b/tests/suites/test_suite_lms.data @@ -186,6 +186,10 @@ LMS hsslms interop test #2 # print('lms_verify_test:"{}":"{}":"{}":0'.format(message.hex(), sig.hex(), public_key.get_pubkey().hex())) lms_verify_test:"92d036bde8c45b8bb5dea2a072560b1e29fc4bb7dc4549ce90bccee8a6e962a1":"00000001000000042e758f2a0e8f301af58847b6973a15fe4856b91af88a1ff601e2f0e87bde33afbc39202a66e38931fbecd7d493cf957b37eeb57ed2e4d8f693b97adafa8241746d775cfb9471688d935e090581eb8586e7004d6b8855963d82ccb6f79df2d93dd35848556da6735def0f0c6c8fc969c1df692341f6a99626eff37d20226cef361c8802a995fa43535fe2336d8ae468c78eb6a7082e27c2c6317c034369b588e3d65a2eeac9804b427702dc49f681a841076813ed407399aa778259e7c34c750baa6d296a384e1274facaba9e2214d197628f5df7b2bf3896fedeab377b8edb775d6e8d67f1474ba3066794447f8f8e0c13552a007557a1f1c3b9bd2b41d9b446c6bcf36c828fd4c80850a31ee603065f5cc90d198df03835195b14e27da7bf727a16081fcc787f1dc7fa6da8b9ff908fb2c02d6f2a183486de0e39cd7da7fcdee0c8e96876c56ad9b0b18e4e4999e2c81a618aa4b27e050ce488dbb1e79089131afacc446cdf15b625f4e011f8d8160bb93f326bca3bb56fa41e34893d55f17d746fc142297997c5dbbba8f6b6c80678168ba455f12bac6982e5192de5462a46e14a45a01ce9e07279aa301dfd0fa9a12c6a55059b19a19d7afbe99779ea130ddeeb5ecb67d2ddb6c1c5d198e421b78091efa5aa429e1eb052760c0d8e2eb0c0ced000e93f7f265611a385f77c0cece0496eb29010f710e70a768d3713f0b7fc60c8ce372dc3234f27c7a1c2776a939ef70c7be869337b967df2223d4f20dca697e3bb6d0e53bbad153ff08d579f60c8535710f253b90e73ee9a19e1e57df66ec6c85ad1b4cea28a9d62fc5a4cf130f70b910dbc7e6f0e6b0cce1a1b5ff106b7f0b101405c0989084b2c94977116b98d15d6062a8d77d660aa813d432cf3338484308b7beed10236081f52da44eb807f9a75fd4cc1ba998ef3fc2e4791712597c786dd46431468bb4a1975a6cd854a1da23912fc99160f51df484efc9371c2d8e028d9468635cf93226f5a8834d14cead59e5d2a61dd6440d7b91c903ae8823907b75595c4828c7710036b347dcfb67f8561e835a53f569c8b3a1cd4317b2a6b2243100ee3d9468f9191acf2276d18dde9ebf2e11a48ba1fc1a15dc51091d3358d8d1f65ec7d84b97bb1669a9141f74065454f08e5ef25432b7635b8ec673ca70e4b3c25d07975a6fb725a56f28c1b5a81a6da2fe0a2c3474275926f9819a25b942462a68097e1cf6d9ae94f6b1f76b54addaeda04f9fc8db025fd6c453e1ad928f9323bf1381fce1893938828612728185d22a3d45d21ce762c066ab53a582c487d76d431e5b8f65a382142dd823d4620931e5572a4e6aee69986421afa119634bc8ea88aa6535c4d619ca0e0af94934637bc0c834e5e2a7a2853fa73835d00e13e5f26ad085ef66c8efb60097860cb199e03596a3b8f0ec78690d527bbc9363dd9702226788b1529871df74918ae2a4e02745043bd5ee8ab027826fb4cd54b0c27d99076757a1b41e2725ec02adc7926e8213796a8aa1740a2dc675437771e0364a83b0bd64c9620f6c203d92626ff29ef736eac0e13c71fd1957333ee0048000000061f7b7d6f916710efe9ed625ae689c67b3cc1cdf0d672e58c0b86b3839bbba2c243dcda2e63b26f5efa39ced6095a54625e67ab25d3df068e903eaaee894ac0f1fdeb4a2f1390f655db3608583eacfb0be4282f7bd1c42c5d748d524d7cdcd45878dea56cbc11a63bebbd74a5413ce72a931b1d4794c78c4cf16315bf2e055bb3305fe0272c8b916856cc27aa7a773ddce62afa7bb4da76c287e0ed3ed10452512de82c051f17b49c608b1a259e16a3812c0de684f2cb1ee59296c375376f146e2b0cc299ef41ed8e6fdf0557ec8d95fa026970f8d47c8347fed1e37e018413c5e813d1726ea18bc926ed02840349ab3b2adc8758a9cd57be38e9e76869762a81bb79721ca1c031c9dfdc3735fe9318064b62c2a7e8e2ec099963257b0705aac812dbc8cc3fbeea81af7c0d592c7e2ad1c21e877d4ae392b13ac1b57a8311d406":"000000060000000447cc5b29dd0cecd01c382434a6d16864d51b60cdb2a9eed2419015d8524c717ce38a865d7a37da6c84f94621ad595f5d":0 +LMS negative test (corrupt Merkle path byte) +# Corrupts one byte in the Merkle authentication path to force internal failure. +lms_verify_test:"92d036bde8c45b8bb5dea2a072560b1e29fc4bb7dc4549ce90bccee8a6e962a1":"00000001000000042e758f2a0e8f301af58847b6973a15fe4856b91af88a1ff601e2f0e87bde33afbc39202a66e38931fbecd7d493cf957b37eeb57ed2e4d8f693b97adafa8241746d775cfb9471688d935e090581eb8586e7004d6b8855963d82ccb6f79df2d93dd35848556da6735def0f0c6c8fc969c1df692341f6a99626eff37d20226cef361c8802a995fa43535fe2336d8ae468c78eb6a7082e27c2c6317c034369b588e3d65a2eeac9804b427702dc49f681a841076813ed407399aa778259e7c34c750b556d296a384e1274facaba9e2214d197628f5df7b2bf3896fedeab377b8edb775d6e8d67f1474ba3066794447f8f8e0c13552a007557a1f1c3b9bd2b41d9b446c6bcf36c828fd4c80850a31ee603065f5cc90d198df03835195b14e27da7bf727a16081fcc787f1dc7fa6da8b9ff908fb2c02d6f2a183486de0e39cd7da7fcdee0c8e96876c56ad9b0b18e4e4999e2c81a618aa4b27e050ce488dbb1e79089131afacc446cdf15b625f4e011f8d8160bb93f326bca3bb56fa41e34893d55f17d746fc142297997c5dbbba8f6b6c80678168ba455f12bac6982e5192de5462a46e14a45a01ce9e07279aa301dfd0fa9a12c6a55059b19a19d7afbe99779ea130ddeeb5ecb67d2ddb6c1c5d198e421b78091efa5aa429e1eb052760c0d8e2eb0c0ced000e93f7f265611a385f77c0cece0496eb29010f710e70a768d3713f0b7fc60c8ce372dc3234f27c7a1c2776a939ef70c7be869337b967df2223d4f20dca697e3bb6d0e53bbad153ff08d579f60c8535710f253b90e73ee9a19e1e57df66ec6c85ad1b4cea28a9d62fc5a4cf130f70b910dbc7e6f0e6b0cce1a1b5ff106b7f0b101405c0989084b2c94977116b98d15d6062a8d77d660aa813d432cf3338484308b7beed10236081f52da44eb807f9a75fd4cc1ba998ef3fc2e4791712597c786dd46431468bb4a1975a6cd854a1da23912fc99160f51df484efc9371c2d8e028d9468635cf93226f5a8834d14cead59e5d2a61dd6440d7b91c903ae8823907b75595c4828c7710036b347dcfb67f8561e835a53f569c8b3a1cd4317b2a6b2243100ee3d9468f9191acf2276d18dde9ebf2e11a48ba1fc1a15dc51091d3358d8d1f65ec7d84b97bb1669a9141f74065454f08e5ef25432b7635b8ec673ca70e4b3c25d07975a6fb725a56f28c1b5a81a6da2fe0a2c3474275926f9819a25b942462a68097e1cf6d9ae94f6b1f76b54addaeda04f9fc8db025fd6c453e1ad928f9323bf1381fce1893938828612728185d22a3d45d21ce762c066ab53a582c487d76d431e5b8f65a382142dd823d4620931e5572a4e6aee69986421afa119634bc8ea88aa6535c4d619ca0e0af94934637bc0c834e5e2a7a2853fa73835d00e13e5f26ad085ef66c8efb60097860cb199e03596a3b8f0ec78690d527bbc9363dd9702226788b1529871df74918ae2a4e02745043bd5ee8ab027826fb4cd54b0c27d99076757a1b41e2725ec02adc7926e8213796a8aa1740a2dc675437771e0364a83b0bd64c9620f6c203d92626ff29ef736eac0e13c71fd1957333ee0048000000061f7b7d6f916710efe9ed625ae689c67b3cc1cdf0d672e58c0b86b3839bbba2c243dcda2e63b26f5efa39ced6095a54625e67ab25d3df068e903eaaee894ac0f1fdeb4a2f1390f655db3608583eacfb0be4282f7bd1c42c5d748d524d7cdcd45878dea56cbc11a63bebbd74a5413ce72a931b1d4794c78c4cf16315bf2e055bb3305fe0272c8b916856cc27aa7a773ddce62afa7bb4da76c287e0ed3ed10452512de82c051f17b49c608b1a259e16a3812c0de684f2cb1ee59296c375376f146e2b0cc299ef41ed8e6fdf0557ec8d95fa026970f8d47c8347fed1e37e018413c5e813d1726ea18bc926ed02840349ab3b2adc8758a9cd57be38e9e76869762a81bb79721ca1c031c9dfdc3735fe9318064b62c2a7e8e2ec099963257b0705aac812dbc8cc3fbeea81af7c0d592c7e2ad1c21e877d4ae392b13ac1b57a8311d406":"000000060000000447cc5b29dd0cecd01c382434a6d16864d51b60cdb2a9eed2419015d8524c717ce38a865d7a37da6c84f94621ad595f5d":MBEDTLS_ERR_LMS_VERIFY_FAILED + LMS negative test (invalid lms type) #1 # This test uses the data from hash-sigs interop test #1. This test has a valid # LMOTS type (0x4) but an invalid LMS type (0x5), and should fail. From 715bbf3e0c627850428643fe831c0562bc588dfc Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Tue, 3 Jun 2025 22:00:58 +0200 Subject: [PATCH 38/52] 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 39/52] 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 40/52] 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 58a4479ace9931a8e6688c024d5bac692b60674e Mon Sep 17 00:00:00 2001 From: Minos Galanakis Date: Fri, 6 Jun 2025 10:58:20 +0100 Subject: [PATCH 41/52] test_suite_lms.data: Updated comments Signed-off-by: Minos Galanakis --- tests/suites/test_suite_lms.data | 13 ++++++++----- 1 file changed, 8 insertions(+), 5 deletions(-) diff --git a/tests/suites/test_suite_lms.data b/tests/suites/test_suite_lms.data index 51578de078..5601cce92c 100644 --- a/tests/suites/test_suite_lms.data +++ b/tests/suites/test_suite_lms.data @@ -187,7 +187,10 @@ LMS hsslms interop test #2 lms_verify_test:"92d036bde8c45b8bb5dea2a072560b1e29fc4bb7dc4549ce90bccee8a6e962a1":"00000001000000042e758f2a0e8f301af58847b6973a15fe4856b91af88a1ff601e2f0e87bde33afbc39202a66e38931fbecd7d493cf957b37eeb57ed2e4d8f693b97adafa8241746d775cfb9471688d935e090581eb8586e7004d6b8855963d82ccb6f79df2d93dd35848556da6735def0f0c6c8fc969c1df692341f6a99626eff37d20226cef361c8802a995fa43535fe2336d8ae468c78eb6a7082e27c2c6317c034369b588e3d65a2eeac9804b427702dc49f681a841076813ed407399aa778259e7c34c750baa6d296a384e1274facaba9e2214d197628f5df7b2bf3896fedeab377b8edb775d6e8d67f1474ba3066794447f8f8e0c13552a007557a1f1c3b9bd2b41d9b446c6bcf36c828fd4c80850a31ee603065f5cc90d198df03835195b14e27da7bf727a16081fcc787f1dc7fa6da8b9ff908fb2c02d6f2a183486de0e39cd7da7fcdee0c8e96876c56ad9b0b18e4e4999e2c81a618aa4b27e050ce488dbb1e79089131afacc446cdf15b625f4e011f8d8160bb93f326bca3bb56fa41e34893d55f17d746fc142297997c5dbbba8f6b6c80678168ba455f12bac6982e5192de5462a46e14a45a01ce9e07279aa301dfd0fa9a12c6a55059b19a19d7afbe99779ea130ddeeb5ecb67d2ddb6c1c5d198e421b78091efa5aa429e1eb052760c0d8e2eb0c0ced000e93f7f265611a385f77c0cece0496eb29010f710e70a768d3713f0b7fc60c8ce372dc3234f27c7a1c2776a939ef70c7be869337b967df2223d4f20dca697e3bb6d0e53bbad153ff08d579f60c8535710f253b90e73ee9a19e1e57df66ec6c85ad1b4cea28a9d62fc5a4cf130f70b910dbc7e6f0e6b0cce1a1b5ff106b7f0b101405c0989084b2c94977116b98d15d6062a8d77d660aa813d432cf3338484308b7beed10236081f52da44eb807f9a75fd4cc1ba998ef3fc2e4791712597c786dd46431468bb4a1975a6cd854a1da23912fc99160f51df484efc9371c2d8e028d9468635cf93226f5a8834d14cead59e5d2a61dd6440d7b91c903ae8823907b75595c4828c7710036b347dcfb67f8561e835a53f569c8b3a1cd4317b2a6b2243100ee3d9468f9191acf2276d18dde9ebf2e11a48ba1fc1a15dc51091d3358d8d1f65ec7d84b97bb1669a9141f74065454f08e5ef25432b7635b8ec673ca70e4b3c25d07975a6fb725a56f28c1b5a81a6da2fe0a2c3474275926f9819a25b942462a68097e1cf6d9ae94f6b1f76b54addaeda04f9fc8db025fd6c453e1ad928f9323bf1381fce1893938828612728185d22a3d45d21ce762c066ab53a582c487d76d431e5b8f65a382142dd823d4620931e5572a4e6aee69986421afa119634bc8ea88aa6535c4d619ca0e0af94934637bc0c834e5e2a7a2853fa73835d00e13e5f26ad085ef66c8efb60097860cb199e03596a3b8f0ec78690d527bbc9363dd9702226788b1529871df74918ae2a4e02745043bd5ee8ab027826fb4cd54b0c27d99076757a1b41e2725ec02adc7926e8213796a8aa1740a2dc675437771e0364a83b0bd64c9620f6c203d92626ff29ef736eac0e13c71fd1957333ee0048000000061f7b7d6f916710efe9ed625ae689c67b3cc1cdf0d672e58c0b86b3839bbba2c243dcda2e63b26f5efa39ced6095a54625e67ab25d3df068e903eaaee894ac0f1fdeb4a2f1390f655db3608583eacfb0be4282f7bd1c42c5d748d524d7cdcd45878dea56cbc11a63bebbd74a5413ce72a931b1d4794c78c4cf16315bf2e055bb3305fe0272c8b916856cc27aa7a773ddce62afa7bb4da76c287e0ed3ed10452512de82c051f17b49c608b1a259e16a3812c0de684f2cb1ee59296c375376f146e2b0cc299ef41ed8e6fdf0557ec8d95fa026970f8d47c8347fed1e37e018413c5e813d1726ea18bc926ed02840349ab3b2adc8758a9cd57be38e9e76869762a81bb79721ca1c031c9dfdc3735fe9318064b62c2a7e8e2ec099963257b0705aac812dbc8cc3fbeea81af7c0d592c7e2ad1c21e877d4ae392b13ac1b57a8311d406":"000000060000000447cc5b29dd0cecd01c382434a6d16864d51b60cdb2a9eed2419015d8524c717ce38a865d7a37da6c84f94621ad595f5d":0 LMS negative test (corrupt Merkle path byte) -# Corrupts one byte in the Merkle authentication path to force internal failure. +# Corrupt one byte at index 200 in the Merkle authentication path to force internal failure. +# The test modifies a valid test's data so that the left_node parameter of create_merkle_internal_value becomes invalid. +# Since that corrupted data is not used by mbedtls_lms_verify before that invocation, +# the test targets the check of the value returned by create_merkle_internal_value. lms_verify_test:"92d036bde8c45b8bb5dea2a072560b1e29fc4bb7dc4549ce90bccee8a6e962a1":"00000001000000042e758f2a0e8f301af58847b6973a15fe4856b91af88a1ff601e2f0e87bde33afbc39202a66e38931fbecd7d493cf957b37eeb57ed2e4d8f693b97adafa8241746d775cfb9471688d935e090581eb8586e7004d6b8855963d82ccb6f79df2d93dd35848556da6735def0f0c6c8fc969c1df692341f6a99626eff37d20226cef361c8802a995fa43535fe2336d8ae468c78eb6a7082e27c2c6317c034369b588e3d65a2eeac9804b427702dc49f681a841076813ed407399aa778259e7c34c750b556d296a384e1274facaba9e2214d197628f5df7b2bf3896fedeab377b8edb775d6e8d67f1474ba3066794447f8f8e0c13552a007557a1f1c3b9bd2b41d9b446c6bcf36c828fd4c80850a31ee603065f5cc90d198df03835195b14e27da7bf727a16081fcc787f1dc7fa6da8b9ff908fb2c02d6f2a183486de0e39cd7da7fcdee0c8e96876c56ad9b0b18e4e4999e2c81a618aa4b27e050ce488dbb1e79089131afacc446cdf15b625f4e011f8d8160bb93f326bca3bb56fa41e34893d55f17d746fc142297997c5dbbba8f6b6c80678168ba455f12bac6982e5192de5462a46e14a45a01ce9e07279aa301dfd0fa9a12c6a55059b19a19d7afbe99779ea130ddeeb5ecb67d2ddb6c1c5d198e421b78091efa5aa429e1eb052760c0d8e2eb0c0ced000e93f7f265611a385f77c0cece0496eb29010f710e70a768d3713f0b7fc60c8ce372dc3234f27c7a1c2776a939ef70c7be869337b967df2223d4f20dca697e3bb6d0e53bbad153ff08d579f60c8535710f253b90e73ee9a19e1e57df66ec6c85ad1b4cea28a9d62fc5a4cf130f70b910dbc7e6f0e6b0cce1a1b5ff106b7f0b101405c0989084b2c94977116b98d15d6062a8d77d660aa813d432cf3338484308b7beed10236081f52da44eb807f9a75fd4cc1ba998ef3fc2e4791712597c786dd46431468bb4a1975a6cd854a1da23912fc99160f51df484efc9371c2d8e028d9468635cf93226f5a8834d14cead59e5d2a61dd6440d7b91c903ae8823907b75595c4828c7710036b347dcfb67f8561e835a53f569c8b3a1cd4317b2a6b2243100ee3d9468f9191acf2276d18dde9ebf2e11a48ba1fc1a15dc51091d3358d8d1f65ec7d84b97bb1669a9141f74065454f08e5ef25432b7635b8ec673ca70e4b3c25d07975a6fb725a56f28c1b5a81a6da2fe0a2c3474275926f9819a25b942462a68097e1cf6d9ae94f6b1f76b54addaeda04f9fc8db025fd6c453e1ad928f9323bf1381fce1893938828612728185d22a3d45d21ce762c066ab53a582c487d76d431e5b8f65a382142dd823d4620931e5572a4e6aee69986421afa119634bc8ea88aa6535c4d619ca0e0af94934637bc0c834e5e2a7a2853fa73835d00e13e5f26ad085ef66c8efb60097860cb199e03596a3b8f0ec78690d527bbc9363dd9702226788b1529871df74918ae2a4e02745043bd5ee8ab027826fb4cd54b0c27d99076757a1b41e2725ec02adc7926e8213796a8aa1740a2dc675437771e0364a83b0bd64c9620f6c203d92626ff29ef736eac0e13c71fd1957333ee0048000000061f7b7d6f916710efe9ed625ae689c67b3cc1cdf0d672e58c0b86b3839bbba2c243dcda2e63b26f5efa39ced6095a54625e67ab25d3df068e903eaaee894ac0f1fdeb4a2f1390f655db3608583eacfb0be4282f7bd1c42c5d748d524d7cdcd45878dea56cbc11a63bebbd74a5413ce72a931b1d4794c78c4cf16315bf2e055bb3305fe0272c8b916856cc27aa7a773ddce62afa7bb4da76c287e0ed3ed10452512de82c051f17b49c608b1a259e16a3812c0de684f2cb1ee59296c375376f146e2b0cc299ef41ed8e6fdf0557ec8d95fa026970f8d47c8347fed1e37e018413c5e813d1726ea18bc926ed02840349ab3b2adc8758a9cd57be38e9e76869762a81bb79721ca1c031c9dfdc3735fe9318064b62c2a7e8e2ec099963257b0705aac812dbc8cc3fbeea81af7c0d592c7e2ad1c21e877d4ae392b13ac1b57a8311d406":"000000060000000447cc5b29dd0cecd01c382434a6d16864d51b60cdb2a9eed2419015d8524c717ce38a865d7a37da6c84f94621ad595f5d":MBEDTLS_ERR_LMS_VERIFY_FAILED LMS negative test (invalid lms type) #1 @@ -216,10 +219,10 @@ LMS negative test (invalid leaf ID) # test should fail to verify the signature. lms_verify_test:"bfff9cd687351db88a98c71fd2f9b927a0ee600130a112533b791041d30cb91665fc369a5ac7cc9a04547414ac45288081d19d4a600579c73ac4bc953de03ad6":"0000040000000004e474c96c496b231ecedcd92566ab3e1cdfac83f7e56abaae6571401e212e59bdbcc18105e0709249510d13d7ae1091558c217033316ac70a36aae764bd0f4032e369453c634b81061079d216d03d3c55976a1aabc00287b307297ae03587ba20839460daae85d26dfcef7638c10a1d8559612e5e9ced1a4205a52ca0ce88e58602e59cf9ab34adf2e958e56570573297b99f733fbbf58d2440526fd4dc15c5509e8a11405f6c08772abcf58731fbf9a73642670e3247c5f70905f0fa81f6174bf32977209923507525a170fcd260e81f04193583fbcd305ac245c80eb337ca326fd1105e73748fab8a1f0c8d8a99f011718e7aae027a34d2a85ff18769fa277810126a86b51b096a04d8e28a4fb8c5e14e50a67cb1ee88e43e5cc077902442f5d9c55ac2b8acdd76c67bc740c6083aba4e3cb404c23f1f3118337563fef6a4b01fb476810c5b5682d0aecdccd55c85a4af50e9150f7d83dcccd8e822a302e6e5a52e00505e6e65338dcfb9cfbe22594e9e18ebde36af29450c5ea31523019cf64fd6eca8c77d98c2a146dcedd51bf6c61c1f7cacbce3ab20c8606930cc42737e17f2703cf0980aef560768d1ac5585c05a60a5f94db15f5ac4d4df5cbd81430878d4e9b77346e3a6538b80b80873e3e6c37860470091979296631440adb8cc71991aba2a4dd2884764878306fe774a25512cfbf080f2829ea2903ffa748dd187c21aef918ee3436a1bd336c3d09cc1f748d7528db90a98f69078b82c4d23de7bcec092a292d2b8cac71a5c87d008f128b89a5e608a4501aef41e9f17e4056ed4767957188f780159daebf327751386980b0fca5a2d36b141acff957f46ce2381897099619475db9d3a613e7ef98b056f42b4d6eafb1d62eebbe46a7502f893fbd36ccfb12a280f45ffb93f050eb280bf0a6cd640abdea8590bffb98bdb29ee3a31daa0fa3ab35fee11dc1b7d1fcea82b0e284b2e35b34e77c3f401ed887e7fc6c97b327f76f99caca2f355afa2753a8923bfb06fb2a9df08d31c93882e34ef5a3cccc9d078855334bdf909ae418b177724c42fb1d586d212c4474932acce295236030f4379158957300fe9fdc5cc9145e3ded50cf9f5a8e19321961536c4a47fffc3eb4383fb78a5d2aeed5b45b92132b5e2a53e3b67841fa2e1bd217ee2c30812c4eb1bd4f8c85b328e23d27f12a2fad5c6b236c87f8fd1aad441416e53ebd4d45d4bf601b94eb37dc9a065218ae58e69dba1250bb20626baeda961b3ef11d217697e73f41fa3870d726a032bc4a388fb12c023822945df058e22f54e5f6377eab34297c57883515204fc189d0d4b0ad9bacb24acf7f9d55e7c6368bb8ababd7622f586ec22683306c5d88d5244219a3952adbd85c89893a441a58b532e15600cd5afdbb5b441e1670b72656c7995189bdf993154e09912db8c4ddaff0e75591720230cf99c8b71cd841dffc4372c5e0f9ff906a379d28d6884351609bf7c849ebfabfb049ae986bbb8467251dbf5ccdd05a86ff6ce1392f7ca1bd49595ad9ad805d71b389afab1865f7f69dc24662af19934f025ced212536509500c132aec000000068b991bed50319a6cb9ff040b92f1563889b3787c37145fc9737d4643f66ade33ebd85a2c29b8c64a581cff01b89d59807d6fade2d2c88872f77d0ed83d97c4b5438681d0b95feb973125e4ee70ebe11699290b831e86571e36513a71f159d48ce563f6814cc2a89851d2520c5275b34cc83614cab14c0d197166580d800ee6b9004b8fd72daac8d73c36c1623c37be93ba49a06c4efde238a3a10a05daba5d4942f7de52648af2be31f33e723b3605346282f5d5e356c5d0004eea40fe0b80abf658f6c96c56319ab53d7fefb5879e0136d1cf320973a2f47c1ee3d21554910f09d17afac4607657c4309890957a4954bf86e2a8491ba37dd88b2c3fe3c7edebd767c400bc23e40d165b27133c726b90bc26cbb2a86a6aa400c47aa7ffc538388de8490b9349afa53b814ffe2a56ff16a496e9648284193754f989f6f12aeb6e":"0000000600000004d96bb26744d99ef624e32161c36d3d6efcdd0484e2b17a6dd183125be4b1af1cda931a91a3acb1151877c174f7943fd9":MBEDTLS_ERR_LMS_VERIFY_FAILED -LMS key import out-of-bounds sized key test -# This test cuts the input data it down so it is MBEDTLS_LMS_TYPE_LEN -1 -# and imports it. This should fail, and not attempt to read invalidly -# outside the buffer. +LMS public key import: truncated input below minimum valid size +# This test provides an input buffer that is one byte shorter than the minimum required +# size (MBEDTLS_LMS_TYPE_LEN - 1). The import operation is expected to fail gracefully, +# without attempting to read beyond the end of the buffer. lms_import_export_test:"000000":MBEDTLS_ERR_LMS_BAD_INPUT_DATA LMS import/export test From df2f0aae813c037bd9d5c3468a96ff928461cce5 Mon Sep 17 00:00:00 2001 From: Minos Galanakis Date: Fri, 6 Jun 2025 14:34:54 +0100 Subject: [PATCH 42/52] lms.c: Updated documentation Signed-off-by: Minos Galanakis --- library/lms.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/library/lms.c b/library/lms.c index 11e2508954..41d34bfe16 100644 --- a/library/lms.c +++ b/library/lms.c @@ -101,7 +101,7 @@ static int create_merkle_leaf_value(const mbedtls_lms_parameters_t *params, size_t output_hash_len; unsigned char r_node_idx_bytes[4]; - /* Always zeroize the output buffer to avoid undefined behavior at an early exit */ + /* Always zeroize the output buffer because it may contain data from the previous invocation */ memset(out, 0, MBEDTLS_LMS_M_NODE_BYTES(params->type)); op = psa_hash_operation_init(); From 255c492dab881e0f8390ecb5f1eb948def15c1e9 Mon Sep 17 00:00:00 2001 From: Minos Galanakis Date: Sun, 8 Jun 2025 23:10:58 +0100 Subject: [PATCH 43/52] Added CVE's to ChangeLogs Signed-off-by: Minos Galanakis --- ChangeLog.d/1351_lms_overread.txt | 1 + ChangeLog.d/1353_lms_check_return_of_merkle_leaf.txt | 1 + 2 files changed, 2 insertions(+) diff --git a/ChangeLog.d/1351_lms_overread.txt b/ChangeLog.d/1351_lms_overread.txt index 43249f70a5..c6ad77227c 100644 --- a/ChangeLog.d/1351_lms_overread.txt +++ b/ChangeLog.d/1351_lms_overread.txt @@ -1,3 +1,4 @@ Security * Fix a buffer overread in mbedtls_lms_import_public_key() when the input is less than 3 bytes. Reported by Linh Le and Ngan Nguyen from Calif. + CVE-2025-49601 diff --git a/ChangeLog.d/1353_lms_check_return_of_merkle_leaf.txt b/ChangeLog.d/1353_lms_check_return_of_merkle_leaf.txt index 9feca99ba7..4d8bd8a1c3 100644 --- a/ChangeLog.d/1353_lms_check_return_of_merkle_leaf.txt +++ b/ChangeLog.d/1353_lms_check_return_of_merkle_leaf.txt @@ -2,3 +2,4 @@ Security * Fix a vulnerability in LMS verification through which an adversary could get an invalid signature accepted if they could cause a hash accelerator to fail. Found and reported by Linh Le and Ngan Nguyen from Calif. + CVE-2025-49600 From 2b3d6a8f288411187cfc4441a893b0f5a5a163f7 Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Wed, 4 Jun 2025 11:22:25 +0200 Subject: [PATCH 44/52] 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 45/52] 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 46/52] 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 8c67ac0f7f8863e60714f16513b4d420c764c093 Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Mon, 9 Jun 2025 23:34:59 +0200 Subject: [PATCH 47/52] Fix race condition in mbedtls_aesni_has_support Fix a race condition in `mbedtls_aes_ni_has_support()` with some compilers. A compiler could hoist the assignment `done = 1` above the assignment to `c`, in which case if two threads call `mbedtls_aes_ni_has_support()` at almost the same time, they could be interleaved as follows: Initially: done = 0, c = 0 thread A thread B if (!done) done = 1; # hoisted if (!done) return c & what; # wrong! c = cpuid(); return c & what This would lead to thread B using software AES even though AESNI was available. This is a very minor performance bug. But also, given a very powerful adversary who can block thread A indefinitely (which may be possible when attacking an SGX enclave), thread B could use software AES for a long time, opening the way to a timing side channel attack. Signed-off-by: Gilles Peskine --- ChangeLog.d/aesni_has_support.txt | 15 +++++++++++++++ library/aesni.c | 8 ++++++-- 2 files changed, 21 insertions(+), 2 deletions(-) create mode 100644 ChangeLog.d/aesni_has_support.txt diff --git a/ChangeLog.d/aesni_has_support.txt b/ChangeLog.d/aesni_has_support.txt new file mode 100644 index 0000000000..6ae0a56584 --- /dev/null +++ b/ChangeLog.d/aesni_has_support.txt @@ -0,0 +1,15 @@ +Bugfix + * Fix a race condition on x86/amd64 platforms in AESNI support detection + that could lead to using software AES in some threads at the very + beginning of a multithreaded program. Reported by Solar Designer. + Fixes #9840. + +Security + * On x86/amd64 platforms, with some compilers, when the library is + compiled with support for both AESNI and software AES and AESNI is + available in hardware, an adversary with fine control over which + threads make progress in a multithreaded program could force software + AES to be used for some time when the program starts. This could allow + the adversary to conduct timing attacks and potentially recover the + key. In particular, this attacker model may be possible against an SGX + enclave. diff --git a/library/aesni.c b/library/aesni.c index 4fc1cb918b..c51957f6ef 100644 --- a/library/aesni.c +++ b/library/aesni.c @@ -48,8 +48,12 @@ */ int mbedtls_aesni_has_support(unsigned int what) { - static int done = 0; - static unsigned int c = 0; + /* To avoid a race condition, tell the compiler that the assignment + * `done = 1` and the assignment to `c` may not be reordered. + * https://github.com/Mbed-TLS/mbedtls/issues/9840 + */ + static volatile int done = 0; + static volatile unsigned int c = 0; if (!done) { #if MBEDTLS_AESNI_HAVE_CODE == 2 From 55d211388af1f5717f971d23e09837b93469c54d Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Tue, 10 Jun 2025 09:42:03 +0200 Subject: [PATCH 48/52] 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 f5db3e9436cc004abcc22de3b35ee02405d2ffb7 Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Wed, 11 Jun 2025 10:45:41 +0200 Subject: [PATCH 49/52] Note that GCM is also impacted Signed-off-by: Gilles Peskine --- ChangeLog.d/aesni_has_support.txt | 2 ++ 1 file changed, 2 insertions(+) diff --git a/ChangeLog.d/aesni_has_support.txt b/ChangeLog.d/aesni_has_support.txt index 6ae0a56584..26b7c2c59b 100644 --- a/ChangeLog.d/aesni_has_support.txt +++ b/ChangeLog.d/aesni_has_support.txt @@ -13,3 +13,5 @@ Security the adversary to conduct timing attacks and potentially recover the key. In particular, this attacker model may be possible against an SGX enclave. + The same vulnerability affects GCM acceleration, which could allow + a similarly powerful adversary to craft GCM forgeries. From 03303d88fb1f8654a95ae7313eb237fc3ab611fd Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Wed, 11 Jun 2025 18:24:26 +0200 Subject: [PATCH 50/52] 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 51/52] 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), From 853cfbdcedece871c8f5ac4bfd239d2d7b8b7899 Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Thu, 12 Jun 2025 18:30:45 +0200 Subject: [PATCH 52/52] Add a note about processor memory reordering Signed-off-by: Gilles Peskine --- library/aesni.c | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/library/aesni.c b/library/aesni.c index c51957f6ef..2857068244 100644 --- a/library/aesni.c +++ b/library/aesni.c @@ -51,6 +51,13 @@ int mbedtls_aesni_has_support(unsigned int what) /* To avoid a race condition, tell the compiler that the assignment * `done = 1` and the assignment to `c` may not be reordered. * https://github.com/Mbed-TLS/mbedtls/issues/9840 + * + * Note that we may also be worried about memory access reordering, + * but fortunately the x86 memory model is not too wild: stores + * from the same thread are observed consistently by other threads. + * (See example 8-1 in Sewell et al., "x86-TSO: A Rigorous and Usable + * Programmer’s Model for x86 Multiprocessors", CACM, 2010, + * https://www.cl.cam.ac.uk/~pes20/weakmemory/cacm.pdf) */ static volatile int done = 0; static volatile unsigned int c = 0;