crypto/cryptosoft: Fix HMAC-SHA when a long key is used

[PruteanuVlad]

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

[xiaoxiang781216]

@ThePassionate please help review this patch.

[ThePassionate]

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]

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

[xiaoxiang781216]

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

here apache/nuttx-apps#3376, @ThePassionate

[xiaoxiang781216]

@PruteanuVlad plese apply the following suggestion:

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.

[PruteanuVlad]

Will do 😄