SDK: HMAC-SHA-256 implementation assumes a 32-byte key

[str4d]

Describe the bug.

The hmac_sha256_init function exposed in the SDK is undocumented, but internally calls this helper function:

https://github.com/flipperdevices/flipperzero-firmware/blob/b698126c36a6b491420b673683333102a242455d/lib/toolbox/hmac_sha256.c#L49-L55

The internal documentation and code make clear that this is assuming a key length of 32 bytes, matching the output size of SHA-256.

The reference to RFC 6979 is somewhat misleading here; the more pertinent reference is RFC 2104 which specifies HMAC, and states:

We denote by B the byte-length of such blocks (B=64 for all the above mentioned examples of hash functions), and by L the byte-length of hash outputs (L=16 for MD5, L=20 for SHA-1). The authentication key K can be of any length up to B, the block length of the hash function. Applications that use keys longer than B bytes will first hash the key using H and then use the resultant L byte string as the actual key to HMAC. In any case the minimal recommended length for K is L bytes (as the hash output length).

Put in RFC 2104 terms, hmac_sha256_init is internally assuming that the provided key is always exactly L bytes. However, applications may use a key of any length, and hmac_sha256_init only takes a pointer to the key bytes, not an array indicating the length required for the internal implementation assumptions. This leads to two bugs:

• If an application provides a pointer to a key shorter than 32 bytes, the HMAC implementation will extend the key with an out-of-bounds read. This introduces non-determinism and likely means that a different HMAC output will be produced on every invocation.
• If an application provides a pointer to a key longer than 32 bytes, the HMAC implementation will truncate the key to 32 bytes. This will deterministically produce an invalid HMAC output.

It appears that this HMAC-SHA-256 implementation was introduced as part of the U2F implementation (#879). And indeed, both CTAP1 and CTAP2 only ever call HMAC-SHA-256 with a sharedSecret that is exactly 32 bytes. However, CTAP1 (which is what U2F has been relabeled to) also calls it with a pinUvAuthToken that can be either 16 or 32 bytes; the 16-byte case will trigger the non-determinism bug. (In CTAP2, pinUvAuthToken is always 32 bytes.)

If the HMAC-SHA-256 implementation is intended to be exposed as a general part of the Flipper Zero SDK, then it should be fixed to be compatible with RFC 2104. If it is only intended to be used as part of the U2F implementation, then it should either be removed from the SDK API, or have its 32-byte key requirement clearly documented or enforced.

Reproduction

1. Call hmac_sha256_init with a key that is either shorter or longer than 32 bytes.
2. Compare the output to an RFC 2104-compliant implementation of HMAC-SHA-256.

Target

No response

Logs

No response

Anything else?

No response

[str4d]

However, CTAP1 (which is what U2F has been relabeled to) also calls it with a pinUvAuthToken that can be either 16 or 32 bytes; the 16-byte case will trigger the non-determinism bug.

Note that a 16-byte pinUvAuthToken can still be used with the current API (as can any key with length shorter than 32 bytes), but the caller needs to be aware that they need to pad with trailing zero bytes. An API that was aware of the caller's key length would be nicer.

[nminaylov]

Yes, this version of HMAC is used by U2F app only. Our U2F implementation uses fixed-size 32-byte key, so there was no need in support for other key lengths. We'll remove it from public API on the next API version update

[nminaylov]

Also we have mbedtls library with full HMAC support.

[GMMan]

Slight issue migrating to Mbed TLS's HMAC implementation is it includes a ton of hashing functions by default, and they're all referenced by md.c. A custom config should be created to enable a small subset of features, but where to put that and who's deciding what goes in?

[skotopes]

fixed by migrating to mbedtls, its configuration updated to included more options by default