From 30666d478b44082744c0a1832b68f98ad69a0c9f Mon Sep 17 00:00:00 2001 From: Andre Goddard Rosa Date: Wed, 1 May 2024 11:47:12 -0500 Subject: [PATCH 1/3] Add invalid `padding_len` check in `get_pkcs_padding` When trying to decrypt data with an invalid key, we found that `mbedtls` returned `0x6200` (`-25088`), which means "_CIPHER - Input data contains invalid padding and is rejected_" from `mbedtls_cipher_finish`, but it also set the output len as `18446744073709551516`. In case we detect an error with padding, we leave the output len zero'ed and return `MBEDTLS_ERR_CIPHER_INVALID_PADDING`. I believe that the current test cases are sufficient, as they fail if I return the alternative code `MBEDTLS_ERR_CIPHER_BAD_INPUT_DATA`, so they do already expect a padding failure, but now we don't change the output len in the error case. Here's a reference for the way `openssl` checks the padding length: - https://github.com/openssl/openssl/blob/1848c561ec39a9ea91ff1bf740a554be274f98b0/crypto/evp/evp_enc.c#L1023 - https://github.com/openssl/openssl/commit/b554eef43b9ac5b92f590da6a120dbfd9ca0582e Signed-off-by: Andre Goddard Rosa Signed-off-by: Andre Goddard Rosa --- library/cipher.c | 3 +++ 1 file changed, 3 insertions(+) diff --git a/library/cipher.c b/library/cipher.c index 0683677eda..f883171921 100644 --- a/library/cipher.c +++ b/library/cipher.c @@ -849,6 +849,9 @@ static int get_pkcs_padding(unsigned char *input, size_t input_len, } padding_len = input[input_len - 1]; + if (padding_len == 0 || padding_len > (int)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); From d0a1691b99ef2ad0ed9352ee4feaca6499be95b5 Mon Sep 17 00:00:00 2001 From: Andre Goddard Rosa Date: Wed, 1 May 2024 12:44:02 -0500 Subject: [PATCH 2/3] Remove unnecessary cast Signed-off-by: Andre Goddard Rosa Signed-off-by: Andre Goddard Rosa --- library/cipher.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/library/cipher.c b/library/cipher.c index f883171921..7f4c121492 100644 --- a/library/cipher.c +++ b/library/cipher.c @@ -849,7 +849,7 @@ static int get_pkcs_padding(unsigned char *input, size_t input_len, } padding_len = input[input_len - 1]; - if (padding_len == 0 || padding_len > (int)input_len) { + if (padding_len == 0 || padding_len > input_len) { return MBEDTLS_ERR_CIPHER_INVALID_PADDING; } *data_len = input_len - padding_len; From 043aa9e2a2013b1a9472420f75b616b98f494777 Mon Sep 17 00:00:00 2001 From: Andre Goddard Rosa Date: Thu, 2 May 2024 09:51:49 -0500 Subject: [PATCH 3/3] Add check ensuring output is set to the least-harmful value in error cases With the robustness fix: `PASSED (125 suites, 26639 tests run)` Without the robustness fix: `FAILED (125 suites, 26639 tests run)` Signed-off-by: Andre Goddard Rosa Signed-off-by: Andre Goddard Rosa --- tests/suites/test_suite_cipher.function | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/tests/suites/test_suite_cipher.function b/tests/suites/test_suite_cipher.function index aca415095f..8e49d2d3b5 100644 --- a/tests/suites/test_suite_cipher.function +++ b/tests/suites/test_suite_cipher.function @@ -549,6 +549,10 @@ void enc_fail(int cipher_id, int pad_mode, int key_len, int length_val, /* encode length number of bytes from inbuf */ TEST_ASSERT(0 == mbedtls_cipher_update(&ctx, inbuf, length, encbuf, &outlen)); TEST_ASSERT(ret == mbedtls_cipher_finish(&ctx, encbuf + outlen, &outlen)); + if (0 != ret) { + /* Check output parameter is set to the least-harmful value on error */ + TEST_ASSERT(0 == outlen); + } /* done */ exit: @@ -826,6 +830,10 @@ void decrypt_test_vec(int cipher_id, int pad_mode, data_t *key, total_len += outlen; TEST_ASSERT(finish_result == mbedtls_cipher_finish(&ctx, output + outlen, &outlen)); + if (0 != finish_result) { + /* Check output parameter is set to the least-harmful value on error */ + TEST_ASSERT(0 == outlen); + } total_len += outlen; #if defined(MBEDTLS_GCM_C) || defined(MBEDTLS_CHACHAPOLY_C) int tag_expected = (ctx.cipher_info->mode == MBEDTLS_MODE_GCM ||