`crypto_kdf_hkdf_sha{256,512}_extract_final` use `sizeof` on a pointer (in code which currently is actually not buggy)

[niooss-ledger]

Hello,

In current git master, here are the definitions of functions crypto_kdf_hkdf_sha256_extract_final and crypto_kdf_hkdf_sha512_extract_final: https://github.com/jedisct1/libsodium/blob/7978205916784fea4db9f8f540be7bd87043425f/src/libsodium/crypto_kdf/hkdf/kdf_hkdf_sha256.c#L24-L32 and https://github.com/jedisct1/libsodium/blob/7978205916784fea4db9f8f540be7bd87043425f/src/libsodium/crypto_kdf/hkdf/kdf_hkdf_sha512.c#L24-L32

In both function, sizeof state returns the size of the pointer, not the size of the actual structure (which is much larger than 32 or 64 bits). So sodium_memzero only resets the start of the state structure.

Nonetheless, in practice, the state is actually always cleared! For example this small program displays only zeros in the state:

#include <assert.h>
#include <stdio.h>
#include <sodium/crypto_kdf_hkdf_sha256.h>

int main(void) {
    crypto_kdf_hkdf_sha256_state st = {};
    unsigned char output[32];
    int ret;
    size_t i;

    ret = crypto_kdf_hkdf_sha256_extract_init(&st, NULL, 0);
    assert(ret == 0);
    ret = crypto_kdf_hkdf_sha256_extract_update(&st, (const unsigned char *)"some string", 10);
    assert(ret == 0);
    ret = crypto_kdf_hkdf_sha256_extract_final(&st, output);
    assert(ret == 0);

    printf("State after extract_final:");
    for (i = 0; i < sizeof(st); i++) {
        printf(" %02x", ((unsigned char *)&st)[i]);
    }
    printf("\n");
    return 0;
}

This is because the KDF state uses HMAC states, defined for SHA256 as https://github.com/jedisct1/libsodium/blob/7978205916784fea4db9f8f540be7bd87043425f/src/libsodium/include/sodium/crypto_auth_hmacsha256.h#L38-L41 ... and function crypto_hash_sha256_final correctly clears the SHA256 state (by using sizeof *state): https://github.com/jedisct1/libsodium/blob/7978205916784fea4db9f8f540be7bd87043425f/src/libsodium/crypto_hash/sha256/cp/hash_sha256_cp.c#L232-L240

TL;DR (summary)

crypto_kdf_hkdf_sha256_extract_final and crypto_kdf_hkdf_sha512_extract_final call functions which actually clear all fields in the KDF state. These functions also call sodium_memzero(state, sizeof state);, which currently does nothing more (as the KDF state only contains already-cleared hash states). This call to sodium_memzero is nonetheless fragile (sizeof takes the size of the pointer) and to make the code more robust, I suggest adding a star: sodium_memzero(state, sizeof *state);.

What do you think?