Check final SHA buffer blocksize #5998
Closed
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
bandi13
approved these changes
Jan 24, 2023
dgarske
requested changes
Jan 24, 2023
| @@ -719,6 +719,14 @@ int wc_ShaFinal(wc_Sha* sha, byte* hash) | |||
| return BAD_FUNC_ARG; | |||
| } | |||
|
|
|||
| #ifndef WC_NO_HARDEN | |||
There was a problem hiding this comment.
Please move all this down to where the 0x80 is set. Use BUFFER_E as the failure. Inline with the error check on the update. Please fix other algos to use this too.
% git diff
diff --git a/wolfcrypt/src/sha.c b/wolfcrypt/src/sha.c
index bcfd1005d..3037d7cab 100644
--- a/wolfcrypt/src/sha.c
+++ b/wolfcrypt/src/sha.c
@@ -719,8 +719,6 @@ int wc_ShaFinal(wc_Sha* sha, byte* hash)
return BAD_FUNC_ARG;
}
- local = (byte*)sha->buffer;
-
#ifdef WOLF_CRYPTO_CB
if (sha->devId != INVALID_DEVID) {
ret = wc_CryptoCb_ShaHash(sha, NULL, 0, hash);
@@ -737,6 +735,11 @@ int wc_ShaFinal(wc_Sha* sha, byte* hash)
}
#endif /* WOLFSSL_ASYNC_CRYPT */
+ if (sha->buffLen > WC_SHA_BLOCK_SIZE - 1) {
+ return BUFFER_E;
+ }
+
+ local = (byte*)sha->buffer;
local[sha->buffLen++] = 0x80; /* add 1 */
/* pad with zeros */
| @@ -719,6 +719,14 @@ int wc_ShaFinal(wc_Sha* sha, byte* hash) | |||
| return BAD_FUNC_ARG; | |||
| } | |||
|
|
|||
| #ifndef WC_NO_HARDEN | |||
There was a problem hiding this comment.
Just remove the #ifndef WC_NO_HARDEN
SparkiDev
requested changes
Jan 24, 2023
| /* We'll add a 0x80 byte at the end, | ||
| ** so make sure we have appropriate buffer length. */ | ||
| if (sha->buffLen > WC_SHA_BLOCK_SIZE - 1) { | ||
| return BAD_FUNC_ARG; |
There was a problem hiding this comment.
This is an internal error.
It shouldn't happen when the implementation is working appropriately.
The best error code I've found for this kind of issue is BAD_STATE_E.
|
Can one of the admins verify this patch? |
72 tasks
|
I'm closing this lingering PR. I'd still like to do this at some point. I've added this to my Mothball section in #6234 |
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.
Description
During the final block processing of the SHA2 hash, there's an
0x80byte value added to the buffer and the remaining buffer bytes are set to zero. This PR ensures a reasonablebuffLenis passed forwc_ShaFinal(),Sha256Final(), andSha512Final().If a valid
buffLenvalue is not found, the function will return withBAD_FUNC_ARG.It should be relatively unlikely that the wrong size would be encountered. This check should have relatively small impact. However, if every possible bit of performance is desired, this feature can be disabled with
#define WC_NO_HARDEN.The length was problematic while testing a variety of scenarios related to #5948 and #5997, thus this PR was created for a more graceful handling of a bad buffer length.
Fixes zd# n/a
Testing
Confirmed wolfcrypt/test completed with success. (In my case on the ESP32, but expected to pass on other platforms as well)
Checklist