HMAC with SHAKE128 via EVP interface crashes on EVP_DigestSignUpdate

[guidovranken]

Version: git master

The following results in a null pointer dereference. As far as I could tell this only happens with SHAKE128.

#include <openssl/evp.h>

#define CF_CHECK_EQ(expr, res) if ( (expr) != (res) ) { goto end; }
#define CF_CHECK_NE(expr, res) if ( (expr) == (res) ) { goto end; }

int main(void)
{
    EVP_MD_CTX* ctx = NULL;
    const EVP_MD* md = NULL;
    EVP_PKEY *pkey = NULL;

    const unsigned char key[16] = { 0 };
    const unsigned char input[16] = { 0 };

    OPENSSL_init_crypto(OPENSSL_INIT_LOAD_CRYPTO_STRINGS, NULL);

    CF_CHECK_NE(ctx = EVP_MD_CTX_create(), NULL);
    CF_CHECK_NE(md = EVP_shake128(), NULL);
    CF_CHECK_NE(pkey = EVP_PKEY_new_mac_key(EVP_PKEY_HMAC, NULL, key, sizeof(key)), NULL);
    CF_CHECK_EQ(EVP_DigestSignInit(ctx, NULL, md, NULL, pkey), 1);

    /* Crashes */
    CF_CHECK_EQ(EVP_DigestSignUpdate(ctx, input, sizeof(input)), 1);

end:
    EVP_MD_CTX_destroy(ctx);
    EVP_PKEY_free(pkey);

    return 0;
}

[mspncp]

I just had a quick look at your example and can confirm that it crashes with 9c0cf21. Note that in the debug version the crash occurs earlier (in EVP_DigestSignInit), where it aborts after the following assertion fails

openssl/crypto/hmac/hmac.c

Line 41 in 9c0cf21

if (!ossl_assert(j <= (int)sizeof(ctx->key)))

crypto/hmac/hmac.c:41: OpenSSL internal error: Assertion failed: j <= (int)sizeof(ctx->key)
Aborted (core dumped)

This is the callstack:

#0  __GI_raise (sig=sig@entry=6) at ../sysdeps/unix/sysv/linux/raise.c:51
#1  0x00007ffff74d6e41 in __GI_abort () at abort.c:79
#2  0x00007ffff7a371d4 in OPENSSL_die (message=0x7ffff7b343c0 "Assertion failed: j <= (int)sizeof(ctx->key)", file=0x7ffff7b343a8 "crypto/hmac/hmac.c", line=41) at crypto/cryptlib.c:421
#3  0x00007ffff7a2bd1c in ossl_assert_int (expr=0, exprstr=0x7ffff7b343c0 "Assertion failed: j <= (int)sizeof(ctx->key)", file=0x7ffff7b343a8 "crypto/hmac/hmac.c", line=41)
    at include/internal/cryptlib.h:35
#4  0x00007ffff7a2be55 in HMAC_Init_ex (ctx=0x55555576f050, key=0x55555576f2d0, len=16, md=0x7ffff7db0f60 <shake128_md>, impl=0x0) at crypto/hmac/hmac.c:41
#5  0x00007ffff7a2ba1c in hmac_ctrl (hctx=0x55555576f030, cmd=1, args=0x7fffffffd720) at crypto/hmac/hm_meth.c:103
#6  0x00007ffff7a23b88 in EVP_MAC_vctrl (ctx=0x55555576edd0, cmd=1, args=0x7fffffffd720) at crypto/evp/mac_lib.c:132
#7  0x00007ffff7a23ad6 in EVP_MAC_ctrl (ctx=0x55555576edd0, cmd=1) at crypto/evp/mac_lib.c:107
#8  0x00007ffff7a27edb in pkey_mac_ctrl (ctx=0x55555576ebb0, type=7, p1=0, p2=0x55555576eb60) at crypto/evp/pkey_mac.c:302
#9  0x00007ffff7a2a268 in EVP_PKEY_CTX_ctrl (ctx=0x55555576ebb0, keytype=-1, optype=248, cmd=7, p1=0, p2=0x55555576eb60) at crypto/evp/pmeth_lib.c:393
#10 0x00007ffff7a05be9 in EVP_DigestInit_ex (ctx=0x55555576eb60, type=0x7ffff7db0f60 <shake128_md>, impl=0x0) at crypto/evp/digest.c:250
#11 0x00007ffff7a22f25 in do_sigver_init (ctx=0x55555576eb60, pctx=0x0, type=0x7ffff7db0f60 <shake128_md>, e=0x0, pkey=0x55555576f220, ver=0) at crypto/evp/m_sigver.c:76
#12 0x00007ffff7a22fcb in EVP_DigestSignInit (ctx=0x55555576eb60, pctx=0x0, type=0x7ffff7db0f60 <shake128_md>, e=0x0, pkey=0x55555576f220) at crypto/evp/m_sigver.c:91
#13 0x00005555555549ef in main () at test.c:20

[mspncp]

I forgot to mention: j == 168 and sizeof(ctx->key) == 144.

[guidovranken]

Assert failure also happens with the HMAC_* functions for SHAKE128, but it doesn't crash.

[mspncp]

Did this test ever work for you? I was able to trace the assertion failure(*) back to commit 91ce87c by @dot-asm which introduced EVP_shake128() and EVP_shake256().

@dot-asm would you mind taking a look at this issue?

(*) Side Note, FWIW: somewhere between 91ce87c and current master, sizeof(ctx->key) changed from 128 to 144.

[dot-asm]

I fail to see that it has something to do with quoted m_sha3.c. It's all HMAC thing (which I didn't touch). On related note HMAC is not actually defined with SHAKEs, only with SHA's. Which is why sizeof(key) is limited 144, value specific to most "demanding" SHA3-224. This is not really an excuse for not handling the error condition, just an explanation where does 144 come from. Why not SHAKE128? Well, ask NIST. Again, not an excuse for not handling error condition, just explanation for why you shouln't actually expect HMAC work with SHAKEs, be it 128 or 256, it won't be interoperable with anything else.

[mspncp]

Ok, thanks for your quick reply @dot-asm. I'd like to emphasize that the reason why I adressed you in particular was not to blame you for anything, it was just the hope that you could shed some light on this issue (which you did), since git told me you were involved somehow. :-)

If HMAC and SHAKE are not meant to be used together, then OpenSSL should error out gracefully instead of crashing. Since I am not really familiar with the subject, somebody else from @openssl with more expertise would have to take a look at this issue.

[paulidale]

I agree things should be handled better. Could the HMAC code check for a variable length digest and fail if so?

The algorithm should work with any fixed length digest.

[mattcaswell]

There seems to be two separate issues here:

1. HMAC should not attempt to use shake* if it is not supported. Probably we could just check in HMAC_Init to see if the EVP_MD_FLAG_XOF flag is set for the md
2. This code should still have failed gracefully in a non-debug build except for another bug, this time in pkey_mac_ctrl which incorrectly checks for < 0 as an error return from EVP_MAC_ctrl but takes 0 as a success. The documentation says EVP_MAC_ctrl returns 0 or a negative number on failure. If it wasn't for this bug the EVP_DigestSignInit call would fail gracefully.

Not yet investigated what happens in 1.1.1 wrt to issue (2).

[mattcaswell]

(2) does not appear to be an issue in 1.1.1, and the code behaves correctly in a non-debug build, i.e. the EVP_DigestSignInit call fails as expected. In a debug build the assertion is hit and therefore a crash results.

[mattcaswell]

Fix in #8584 (master) and #8585 (1.1.1).