commit 3711734a0a078f200fbb02edce46ee7f4fd16cf4
author Andre Goddard Rosa <agoddardrosa@roku.com> Wed May 01 11:47:12 2024 -0500
committer Andre Goddard Rosa <agoddardrosa@roku.com> Mon May 13 09:27:57 2024 -0500
tree e796174b278bc5ccc8ffb1f6e4074498d87c365e
parent f1415b65d006eae3679f46f079c09293fcfc118e

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`.

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

So add a check ensuring output is set to the least-harmful value in the
error cases.

With the robustness fix:
`PASSED (125 suites, 26644 tests run)`

Without the robustness fix:
`FAILED (125 suites, 26644 tests run)`

Signed-off-by: Andre Goddard Rosa <andre.goddard@gmail.com>
Signed-off-by: Andre Goddard Rosa <agoddardrosa@roku.com>

library/cipher.c

diff --git a/library/cipher.c b/library/cipher.c
index 0683677..7f4c121 100644
--- a/library/cipher.c
+++ b/library/cipher.c
@@ -849,6 +849,9 @@
     }
 
     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);

tests/suites/test_suite_cipher.function

diff --git a/tests/suites/test_suite_cipher.function b/tests/suites/test_suite_cipher.function
index aca4150..8e49d2d 100644
--- a/tests/suites/test_suite_cipher.function
+++ b/tests/suites/test_suite_cipher.function
@@ -549,6 +549,10 @@
     /* 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 @@
     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 ||