kazuho / picohash

header-file-only implementation of various hash algorithms
89 stars 11 forks source link

Crash in HMAC with SHA256 and long key #10

Open hannes opened 1 year ago

hannes commented 1 year ago

First, thanks a lot for the great project, we use picohash in DuckDB and are generally very happy. We recently found what we think is a bug in pico hash though:

Here's a snippet of code we use to compute a hmac256 from a message and a secret, straight from the README:

typedef unsigned char hash_bytes[PICOHASH_SHA256_DIGEST_LENGTH];

void hmac256(const std::string &message, const char *secret, size_t secret_len, hash_bytes &out) {
    picohash_ctx_t ctx;
    picohash_init_hmac(&ctx, picohash_init_sha256, secret, secret_len);
    picohash_update(&ctx, message.c_str(), message.length());
    picohash_final(&ctx, out);
}

This works fine, unless the secret is longer than ctx->block_length, which is 64. Then, picohash_init_hmac uses another code path which first hashes the key and calls ctx->_hmac.hash_reset(ctx). The problem is that at that point ctx->_hmac.hash_reset has not been set yet, the init function (in our case picohash_init_sha256 (as it should I think) only sets ctx->_reset. picohash_init_hmac does set ctx->_hmac.hash_reset to something meaningful but only later in the function.

I think the way to fix this is to just replace the call to ctx->_hmac.hash_reset(ctx) with a call to ctx->_reset in line 739.

Happy to send a PR if I'm not wrong :)

troberti commented 9 months ago

We also just got bit by this bug. The proposed fix indeed works.