Skip to content

Conversation

@PruteanuVlad
Copy link
Contributor

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.keysize are 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.

nsh> hmac
hmac md5 success
hmac md5 success
hmac md5 success
hmac md5 success
hmac md5 success
hmac sha1 success
hmac sha1 success
hmac sha1 success
hmac sha1 success
hmac sha1 success
hmac sha256 success
hmac sha256 success
hmac sha256 success
hmac sha256 success
hmac sha256 success

When using a key that is longer than the block size of the hashing
algorithm used, the key must be hashed before it is used.

Signed-off-by: Vlad Pruteanu <pruteanuvlad1611@yahoo.com>
@xiaoxiang781216 xiaoxiang781216 marked this pull request as ready for review January 25, 2026 02:44
@xiaoxiang781216
Copy link
Contributor

@ThePassionate please help review this patch.

@ThePassionate
Copy link
Contributor

Good point! I notice that kctx and ictx have the same size (axf->ctxsize),
so we could actually reuse ictx for hashing the long key, then reinitialize it
for computing the ipad. This would save one memory allocation and make the code
more elegant:

/* If the key is too long, hash it first using ictx */
if (cri->cri_klen / 8 > axf->keysize) {
    axf->init((*swd)->sw_ictx);              // Reuse ictx
    axf->update((*swd)->sw_ictx, 
                (FAR uint8_t *)cri->cri_key, 
                cri->cri_klen / 8);
    axf->final((unsigned char *)cri->cri_key, 
               (*swd)->sw_ictx);
    cri->cri_klen = axf->hashsize * 8;
}

This way we avoid allocating a separate kctx while still correctly implementing the RFC 2104 requirement to hash long keys before use.

@ThePassionate
Copy link
Contributor

By the way could you consider adding test cases for the long key cases?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Area: Crypto Size: S The size of the change in this PR is small

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants