bootutil: Fix AES and SHA-256 contexts not zeroized (mbedTLS) #2060
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
For some reason, the calls to mbedtls_aes_free, mbedtls_nist_kw_free and mbedtls_sha256_free_drop were commented out which means the AES and SHA-256 contexts were not properly de-initialized after usage when mbedTLS is used. In the case of AES-KW it seems that might lead to a memory leak depending on the mbedTLS configuration, but in any case and independently of the mbedTLS configuration, this leads to the contexts not be zeroized after usage. Not zeroizing a context means it stays in RAM an undefined amount of time, which might enable an attacker to access it and to dump the sensitive data it contains. Signed-off-by: Thomas Altenbach <thomas.altenbach@legrand.com>
tomi-font
approved these changes
Sep 12, 2024
There was a problem hiding this comment.
Makes sense to me. FYI, MBEDTLS_PLATFORM_NO_STD_FUNCTIONS was removed from Zephyr even before the commit that commented out the code that you just restored (zephyrproject-rtos/zephyr#16303). So that's a bit weird.
@valeriosetti Can you review as well?
de-nordic
approved these changes
Sep 12, 2024
valeriosetti
approved these changes
Sep 12, 2024
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.
For some reason, the calls to
mbedtls_aes_free,mbedtls_nist_kw_freeandmbedtls_sha256_free_dropwere commented out which means the AES and SHA-256 contexts were not properly de-initialized after usage when mbedTLS is used. In the case of AES-KW it seems that might lead to a memory leak depending on the mbedTLS configuration, but in any case and independently of the mbedTLS configuration, this leads to the contexts not be zeroized after usage.Not zeroizing a context means it stays in RAM an undefined amount of time, which might enable an attacker to access it and to dump the sensitive data it contains. For example, for SHA-256, knowing the value of the context might make possible to obtain part of sensitive data that was hashed. For AES, it might make possible to infer the AES master key (or at least part of it).
The commit adding the commented out code is here. My understanding of the commit message is that the de-initialization functions weren't called because this was not needed on Zephyr which defines
MBEDTLS_PLATFORM_NO_STD_FUNCTIONS:To me, this seems to be an incorrect statement even on Zephyr when
MBEDTLS_PLATFORM_NO_STD_FUNCTIONSis defined as zeroization is always needed (or at least highly recommended) for security purposes.