Potential UB in cpuperf: passing NULL to memcpy()

[bobjansen]

This issue was found by automated checks on the R-package scrypt which I maintain. That package is based on scrypt 1.1.6 but I believe the issue could still be present in the current version of scrypt. The offending block in _SHA256_Update() is:

/* Handle the case where we don't need to perform any transforms. */
if (len < 64 - r) {
    memcpy(&ctx->buf[r], src, len);
    return;
}

The function scryptenc_cpuperf() has the following call:

crypto_scrypt(NULL, 0, NULL, 0, 16, 1, 1, NULL, 0)

where the third argument represents the salt. crypto_scrypt() passes the salt to PBKDF2_SHA256() which passes to _HMAC_SHA256_Update(), to _SHA256_Update() which then goes by the name src.

[gperciva]

Thanks for the report!

I think that we're fine, though. If buflen == 0 (the final parameter of the quoted call), then I think[1] that the _SHA256_Update len would be 0, so we would return when we hit

/* Return immediately if we have nothing to do. */
if (len == 0)
	return;

on line 390 of that file (whereas the quoted section is line 400).

[1] I haven't traced through every step between crypto_scrypt and _SHA256_Update yet because I'm just about to head out for an appointment.

[bobjansen]

I did some further digging, in the R package with the old lib, the UB was triggered but I didn't manage to get it NULL in the latest version here. With this code I do see len=64 and r=0 so the the copy is guarded by the if.

You have far more expertise though, so I'll let you close if it's indeed nothing.

[cperciva]

The check for len == 0 was added to scrypt in 719e3fb, which was after version 1.1.6 was released. So the automated checks were right, and @gperciva is also right that we don't need to fix anything. ;-)

@bobjansen I highly recommend updating your package to use a newer version of scrypt though, 1.1.6 is 12 years old...

[bobjansen]

Thanks for the clarification and that's a fair point, the package was very much in maintenance mode but it might be good to consider.