The issue has been discovered by libFuzzer running on provider target.

[Sashan]

There are currently three distinct reports which are addressed by code change here.

https://bugs.chromium.org/p/oss-fuzz/issues/detail?id=69236#c1
https://bugs.chromium.org/p/oss-fuzz/issues/detail?id=69243#c1
https://bugs.chromium.org/p/oss-fuzz/issues/detail?id=69261#c1

the issue has been introduced with openssl 3.0.

[kroeckx]

In which case don't we have an update function?

[Sashan]

In which case don't we have an update function?

I was trying to understand better how fuzzer constructed the context for EVP. What I could see we basically end up with cix which is a zero filled buffer. what follows are my notes from investigation for cluster fuzz issue 69243

#0  0x00005555559def0b in EVP_DigestUpdate () at crypto/evp/digest.c:428
#1  0x0000555555b35ba8 in kmac_derive () at providers/implementations/kdfs/kbkdf.c:268
#2  kbkdf_derive () at providers/implementations/kdfs/kbkdf.c:305
#3  0x00005555559471f0 in do_evp_kdf () at fuzz/provider.c:449
#4  0x000055555594470d in FuzzerTestOneInput () at fuzz/provider.c:619
#5  0x0000555555966c81 in ExecuteCallback () at /src/llvm-project/compiler-rt/lib/fuzzer/FuzzerLoop.cpp:614
#6  0x0000555555951415 in RunOneTest () at /src/llvm-project/compiler-rt/lib/fuzzer/FuzzerDriver.cpp:327
#7  0x0000555555956eab in FuzzerDriver () at /src/llvm-project/compiler-rt/lib/fuzzer/FuzzerDriver.cpp:862
#8  0x00005555559832a3 in main () at /src/llvm-project/compiler-rt/lib/fuzzer/FuzzerMain.cpp:20

We die at very last line of EVP_DigestUpdate() here at line 128:

414
415         if (ctx->digest == NULL
416                 || ctx->digest->prov == NULL
417                 || (ctx->flags & EVP_MD_CTX_FLAG_NO_INIT) != 0)
418             goto legacy;
419
420         if (ctx->digest->dupdate == NULL) {
421             ERR_raise(ERR_LIB_EVP, EVP_R_UPDATE_ERROR);
422             return 0;
423         }
424         return ctx->digest->dupdate(ctx->algctx, data, count);
425
426         /* Code below to be removed when legacy support is dropped. */
427      legacy:
428         return ctx->update(ctx, data, count);

the ctx parameter as we enter the function looks as follows:

(gdb) p *((EVP_MD_CTX *)$rdi)
$2 = {reqdigest = 0x0, digest = 0x0, engine = 0x0, flags = 0, md_data = 0x0, pctx = 0x0, update = 0x0, algctx = 0x0, 
  fetched_digest = 0x0}

So the code starts to move sideways at line 415 because member digest is NULL. The function assumes we deal with legacy provider and jumps to legacy target, where we die immediately on NULL pointer dereference. I`m not expert here so can not how fuzzer managed to get us that far. Revealing parameters generated by fuzzer is hard because I need to do manual stack unwinding. jumping to frames in debugger does not seem to work for fuzzer binaries, the gdb is no able to extract local variables in frames. not sure why perhaps binary got linked with static crypto is to blame here. anyway this simple tweak will make fuzzer happy:

--- a/crypto/evp/digest.c
+++ b/crypto/evp/digest.c
@@ -425,7 +425,7 @@ int EVP_DigestUpdate(EVP_MD_CTX *ctx, const void *data, size_t count)
 
     /* Code below to be removed when legacy support is dropped. */
  legacy:
-    return ctx->update(ctx, data, count);
+    return ctx->update != NULL ? ctx->update(ctx, data, count) : 0;
 }
 
 /* The caller can assume that this removes any secret data from the context */

And here are the notes for issue 69236

To reveal the stack one has to step through gdb to discover we die in EVP_DigestUpdate()again

#0  EVP_DigestUpdate () at crypto/evp/digest.c:387
#1  0x0000555555b65070 in kmac_final () at providers/implementations/macs/kmac_prov.c:353
#2  0x0000555555a0e985 in evp_mac_final () at crypto/evp/mac_lib.c:165
#3  0x0000555555b35bed in kmac_derive () at providers/implementations/kdfs/kbkdf.c:269
#4  kbkdf_derive () at providers/implementations/kdfs/kbkdf.c:305
#5  0x00005555559471f0 in do_evp_kdf () at fuzz/provider.c:449
#6  0x000055555594470d in FuzzerTestOneInput () at fuzz/provider.c:619
#7  0x0000555555966c81 in ExecuteCallback () at /src/llvm-project/compiler-rt/lib/fuzzer/FuzzerLoop.cpp:614
#8  0x0000555555951415 in RunOneTest () at /src/llvm-project/compiler-rt/lib/fuzzer/FuzzerDriver.cpp:327
#9  0x0000555555956eab in FuzzerDriver () at /src/llvm-project/compiler-rt/lib/fuzzer/FuzzerDriver.cpp:862
#10 0x00005555559832a3 in main () at /src/llvm-project/compiler-rt/lib/fuzzer/FuzzerMain.cpp:20

In EVP_DigestUpdate() we arrive to line 428 where we die instantly by jumping to NULL.

the kctx/vmacctx argument passed to kmac_final() function reads as follows:

(gdb) p *((struct kmac_data_st *)0x51a000000080)
$1 = {provctx = 0x50300000d060, ctx = 0x507000000790, digest = {md = 0x511000001940, alloc_md = 0x511000001940, 
    engine = 0x0}, out_len = 32, key_len = 0, custom_len = 0, xof_mode = 0, key = '\000' <repeats 671 times>, 
  custom = '\000' <repeats 515 times>}

We can just grab the ctx member and try to dereference it:

(gdb) p *((EVP_MD_CTX *)0x507000000790)
$2 = {reqdigest = 0x0, digest = 0x0, engine = 0x0, flags = 0, md_data = 0x0, pctx = 0x0, update = 0x0, algctx = 0x0, 
  fetched_digest = 0x0}

yup, this is zero-fill buffer. the EVP_DigestUpdate() gets called from here in kmac_final():

351
352     ok = right_encode(encoded_outlen, sizeof(encoded_outlen), &len, lbits)
353         && EVP_DigestUpdate(ctx, encoded_outlen, len)
354         && EVP_DigestFinalXOF(ctx, out, kctx->out_len);

according to gdb the len is 3 and ctx is buffer of zeros which is a lethal to EVP_DigestUpdate().

[t8m]

So although IMO adding the NULL check to the EVP_DigestUpdate makes sense, we should also fix the KBKDF provider implementation to avoid calling EVP_MAC_update() and EVP_MAC_final() before previous call to EVP_MAC_init().

And we should probably also fix KMAC provider to report error if mac_update or mac_final is called before init.

[t8m]

In which case don't we have an update function?

In case you call EVP_DigestUpdate on MD_CTX that is just new and not properly initialized with EVP_DigestInit.

[t8m]

We could have a test for this but it would have to be run only with default provider or skipped if the FIPS version was old.

[kroeckx]

To me it sounds like we need an assert, instead of ignoring the error. We seem to be coming there because of bugs in other places.

[t8m]

This PR definitely needs a test case

We could add one from the oss-fuzz to the fuzz-corpora. I do not think we are running the fuzz tests against the fips provider.

[Sashan]

rebased, added a test-case which is based on reproducer found by fuzzer. @mattcaswell suggested to put this test into evp_extra_test.c

[t8m]

@bernd-edlinger please reconfirm

[nhorman]

ack, approval holds

[openssl-machine]

24 hours has passed since 'approval: done' was set, but as this PR has been updated in that time the label 'approval: ready to merge' is not being automatically set. Please review the updates and set the label manually.

[mattcaswell]

This PR appears to have already been merged to the master, 3.3, 3.2, 3.1 and 3.0 branches. Closing.