crypto/cryptosoft: Fix HMAC-SHA when a long key is used #18138
+28
−7
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.
Hi all, I discovered a bug within the HMAC-SHA implementation, here is my proposed fix. However, I have a couple of questions that need some clarification, apologies if this isn't the proper channel.
Summary
Using HMAC-SHA with longer keys results in a failed test.
This can be seen using the changes I made in this PR
The changes from the PR are also used to validate my fix.
The issue stems from the code explicitly refusing keys larger than
auth_hash.keysize. In reality, the RFC standard doesn't mention any key size limit. Instead, if a key is larger than the algorithm's block size, the key should first be hashed before using. (2.Definition of HMAC)The fix is fairly straightforward, it adds a key context (kctx) to be used along ictx and octx. When needed, the keys are hashed first.
Updated values for
auth_hash.keysizeare also included.Modified files:
crypto/cryptosoft.c - adds operation for hashing keys that are too long
include/crypto/cryptosoft.h - adds kctx field
crypto/xform.c - fixes keysize
Impact
The bug affects the software implementation as well as the hardware ESP32 implementation. I haven't checked the other hardware implementations. Currently, the PR only fixes the software implementation, although I plan to update it with the fix for ESP32 as well. Here, the first question appears:
As far as I can tell, the fix for ESP32 will be identical with the software one. Since I also have the hardware available I will also test the fix. If there are other platforms affected by this, and if the fix for them would be the same, should I also port the fix there, even if I can't test it on real hardware?
My second question is:
When searching for loose ends, I stumbled across this file. Best I can tell the functions inside aren't used anywhere? The init functions appear to be processing this edge case correctly, but as I've said, they aren't used. So, what to do about this file?
Testing
Development was done using ESP32 DevkitC.
Building was done on Ubuntu 24.04 VM.
For testing, I ran the official crypto HMAC test, to which I added cases for this specific case from the RFC doc.