Hi Philippe,
On Mon, 9 Dec 2024 at 04:42, Philippe Reynes <philippe.rey...@softathome.com>
wrote:
> Adds the support of the hmac based on sha256.
> This implementation is based on rfc2104.
>
> Signed-off-by: Philippe Reynes <philippe.rey...@softathome.com>
> ---
> include/u-boot/sha256.h | 4 ++++
> lib/mbedtls/sha256.c | 38 ++++++++++++++++++++++++++++++++++++++
> lib/sha256.c | 37 +++++++++++++++++++++++++++++++++++++
> 3 files changed, 79 insertions(+)
>
> diff --git a/include/u-boot/sha256.h b/include/u-boot/sha256.h
> index 44a9b528b48..2f12275b703 100644
> --- a/include/u-boot/sha256.h
> +++ b/include/u-boot/sha256.h
> @@ -45,4 +45,8 @@ void sha256_finish(sha256_context * ctx, uint8_t
> digest[SHA256_SUM_LEN]);
> void sha256_csum_wd(const unsigned char *input, unsigned int ilen,
> unsigned char *output, unsigned int chunk_sz);
>
> +void sha256_hmac(const unsigned char *key, int keylen,
> + const unsigned char *input, unsigned int ilen,
> + unsigned char *output);
> +
> #endif /* _SHA256_H */
> diff --git a/lib/mbedtls/sha256.c b/lib/mbedtls/sha256.c
> index 24aa58fa674..1b9fc1a8503 100644
> --- a/lib/mbedtls/sha256.c
> +++ b/lib/mbedtls/sha256.c
> @@ -8,6 +8,7 @@
> #ifndef USE_HOSTCC
> #include <cyclic.h>
> #endif /* USE_HOSTCC */
> +#include <string.h>
> #include <u-boot/sha256.h>
>
> const u8 sha256_der_prefix[SHA256_DER_LEN] = {
> @@ -60,3 +61,40 @@ void sha256_csum_wd(const unsigned char *input,
> unsigned int ilen,
>
> sha256_finish(&ctx, output);
> }
> +
> +void sha256_hmac(const unsigned char *key, int keylen,
> + const unsigned char *input, unsigned int ilen,
> + unsigned char *output)
> +{
> + int i;
> + sha256_context ctx;
> + unsigned char k_ipad[64];
> + unsigned char k_opad[64];
> + unsigned char tmpbuf[32];
> +
> + memset(k_ipad, 0x36, 64);
> + memset(k_opad, 0x5C, 64);
> +
> + for (i = 0; i < keylen; i++) {
> + if (i >= 64)
> + break;
> +
> + k_ipad[i] ^= key[i];
> + k_opad[i] ^= key[i];
> + }
> +
>
You are assuming that the key length is at most 64 bytes.
But according to the HMAC specification:
If the key is longer than the hash block size (64 bytes for SHA256),
it should be hashed first to produce a shorter key.
If the key is shorter than 64 bytes, it should be zero-padded to 64 bytes.
> + sha256_starts(&ctx);
> + sha256_update(&ctx, k_ipad, sizeof(k_ipad));
> + sha256_update(&ctx, input, ilen);
> + sha256_finish(&ctx, tmpbuf);
> +
> + sha256_starts(&ctx);
> + sha256_update(&ctx, k_opad, sizeof(k_opad));
> + sha256_update(&ctx, tmpbuf, sizeof(tmpbuf));
> + sha256_finish(&ctx, output);
> +
> + memset(k_ipad, 0, sizeof(k_ipad));
> + memset(k_opad, 0, sizeof(k_opad));
> + memset(tmpbuf, 0, sizeof(tmpbuf));
> + memset(&ctx, 0, sizeof(sha256_context));
> +}
> diff --git a/lib/sha256.c b/lib/sha256.c
> index fb195d988f1..66224c92dd9 100644
> --- a/lib/sha256.c
> +++ b/lib/sha256.c
> @@ -300,3 +300,40 @@ void sha256_csum_wd(const unsigned char *input,
> unsigned int ilen,
>
> sha256_finish(&ctx, output);
> }
> +
> +void sha256_hmac(const unsigned char *key, int keylen,
> + const unsigned char *input, unsigned int ilen,
> + unsigned char *output)
> +{
> + int i;
> + sha256_context ctx;
> + unsigned char k_ipad[64];
> + unsigned char k_opad[64];
> + unsigned char tmpbuf[32];
> +
> + memset(k_ipad, 0x36, 64);
> + memset(k_opad, 0x5C, 64);
> +
> + for (i = 0; i < keylen; i++) {
> + if (i >= 64)
> + break;
> +
> + k_ipad[i] ^= key[i];
> + k_opad[i] ^= key[i];
> + }
> +
> + sha256_starts(&ctx);
> + sha256_update(&ctx, k_ipad, sizeof(k_ipad));
> + sha256_update(&ctx, input, ilen);
> + sha256_finish(&ctx, tmpbuf);
> +
> + sha256_starts(&ctx);
> + sha256_update(&ctx, k_opad, sizeof(k_opad));
> + sha256_update(&ctx, tmpbuf, sizeof(tmpbuf));
> + sha256_finish(&ctx, output);
> +
> + memset(k_ipad, 0, sizeof(k_ipad));
> + memset(k_opad, 0, sizeof(k_opad));
> + memset(tmpbuf, 0, sizeof(tmpbuf));
> + memset(&ctx, 0, sizeof(sha256_context));
> +}
>
I understand now we have duplicated 'shaX_csum_wd()' under lib and
lib/mbedtls.
That is because at the time when MbedTLS was added in, _csum_wd() is the
only
duplicated function and it is a trade-off comparing with creating a new
common
file for each algorithm.
But it is not the case if the number of these duplicated functions is
increasing.
Do you mind moving them into a common file with the one you created?
At least for sha256 with this patch I think.
Regards,
Raymond
> --
> 2.25.1
>
>
