Add invalid padding_len check in get_pkcs_padding
#9082
+11
−0
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
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 - openssl/openssl@b554eef Signed-off-by: Andre Goddard Rosa <andre.goddard@gmail.com> Signed-off-by: Andre Goddard Rosa <agoddardrosa@roku.com>
Signed-off-by: Andre Goddard Rosa <andre.goddard@gmail.com> Signed-off-by: Andre Goddard Rosa <agoddardrosa@roku.com>
tgonzalezorlandoarm
added
enhancement
needs-review
…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 <andre.goddard@gmail.com> Signed-off-by: Andre Goddard Rosa <agoddardrosa@roku.com>
gilles-peskine-arm
approved these changes
May 2, 2024
tgonzalezorlandoarm
added
approved
gilles-peskine-arm
added
the
needs-backports
|
Can you please make a pull request with the same patch to the |
Please find it at #9132. Thank you! |
4 tasks
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
When trying to decrypt data with an invalid key, we found that
mbedtlsreturned0x6200(-25088), which means "CIPHER - Input data contains invalid padding and is rejected" frommbedtls_cipher_finish, but it also set the output len as18446744073709551516.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 codeMBEDTLS_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
opensslchecks the padding length:Description
Adds a missing check when doing the padding check in the case of
MBEDTLS_CIPHER_AES_128_CBCdecryption with padding. Even thoughtmbedtlswas returning an error, the output parameter was being set instead of leaving it with zero. This check exists inopenssl, so this makes the behavior closer between both libraries.Closes #9083.
PR checklist
Please tick as appropriate and edit the reasons (e.g.: "backport: not needed because this is a new feature")
padding_lencheck inget_pkcs_padding#9132