[PATCH v3 2/5] lib: sha256: add feature sha256_hmac

[Philippe REYNES]

Hi Raymond,


Le 09/12/2024 à 17:06, Raymond Mao a écrit :
>
> *This Mail comes from Outside of SoftAtHome: *Do not answer, click 
> links or open attachments unless you recognize the sender and know the 
> content is safe.**
>
> Hi Philippe,
>
> On Mon, 9 Dec 2024 at 04:42, Philippe Reynes 
> <philippe.reynes at softathome.com> wrote:
>
>     Adds the support of the hmac based on sha256.
>     This implementation is based on rfc2104.
>
>     Signed-off-by: Philippe Reynes <philippe.reynes at 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.


good catch,  you're right, I've missed this part.
I fix it in v4, and I also have added more test unit.
btw: the sha1_hmac has the same issue.


>     +       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.
>

In the v4, I have moved the function sha256_csum_wd() to the file 
sha256_common.c

I plan to do another serie for sha1 and sha512.


> Regards,
> Raymond
>
>     -- 
>     2.25.1
>

Regards,
Philippe