looks like maybe you need to modify the appropriate ci builds such that they compile with a higher SCRYPT_MAX_MEMORY limit defined on the command line?

This PR is waiting for the creator to make requested changes but it has not been updated for 30 days. If you have made changes or commented to the reviewer please make sure you re-request a review (see icon in the 'reviewers' section).

Sorry about the delay, should be ready for review now

Something seems off here. #24650 cites N=1048576, r=8, p=1 tests in the RFC as evidence that the restriction is wrong. It further cites other implementations...

Golang x/crypto and BoringSSL both include the N=1048576 test from RFC 7914 (though in both cases commented out since it takes a while to run). Tarsnap/scrypt (written by the designer of scrypt and co-author of RFC 7914) also includes this test, as does the original scrypt paper (Appendix B).

However, BoringSSL includes this check:
https://github.com/google/boringssl/blob/master/crypto/evp/scrypt.c#L152-L153

And indeed, using those parameters, 2^{(128 * r / 8)} = 2¹²⁸, which is much larger than N = 1048576. So it seems this check is perfectly consistent with that tests and also with inputs that use lots of memory.

hmm, @davidben you're right. I took the pkcs8 key in #24650, and ran it through gdb:

tarting program: /home/nhorman/git/openssl/apps/openssl pkcs8 -inform PEM -in /home/nhorman/key.pem
[Thread debugging using libthread_db enabled]
Using host libthread_db library "/usr/lib64/libthread_db.so.1".
Enter Password:

Breakpoint 1, scrypt_alg (pass=0x552180 "", passlen=0, salt=0x5521a0 "", saltlen=0, N=1048576, r=8, p=1, maxmem=33554432, 
    key=0x0, keylen=0, sha256=0x552c20, libctx=0x7ffff7e0ec40 <default_context_int>, propq=0x0)
    at providers/implementations/kdfs/scrypt.c:449
449	    int rv = 0;

So we can confirm N = 1048576 , r = 8 and p =1

gdb) n
456	    if (r == 0 || p == 0 || N < 2 || (N & (N - 1)))
(gdb) n
459	    if (p > SCRYPT_PR_MAX / r) {
(gdb) n
469	    if (16 * r <= LOG2_UINT64_MAX) {
(gdb) n
483	    Blen = p * 128 * r;

So we can see that we skip the bounds check entirely, as 16 * 8 > LOG2_UINT64_MAX (63).
Continuing:

488	    if (Blen > INT_MAX) {
(gdb) 
497	    i = UINT64_MAX / (32 * sizeof(uint32_t));
(gdb) 
498	    if (N + 2 > i / r) {
(gdb) 
502	    Vlen = 32 * r * (N + 2) * sizeof(uint32_t);
(gdb) 
505	    if (Blen > UINT64_MAX - Vlen) {
(gdb) 
514	    if (Blen + Vlen > maxmem) {
(gdb) 
515	        ERR_raise(ERR_LIB_EVP, EVP_R_MEMORY_LIMIT_EXCEEDED);

We're just tripping over the maxmem sizing of 16MB that is fixed inside the scrypt code, which we had discussed here:
#24650 (comment)

It seems the test is fine regardless, we just need to settle on the best method for adjusting that sizing

though it does beg the question about what (if anything) to do with that unaddressed errata

Sounds like the erratum is invalid and should be marked as such.

Ah, nevermind, I see. The erratum wasn't filed because of this misdiagnosis but something else.

This PR is waiting for the creator to make requested changes but it has not been updated for 30 days. If you have made changes or commented to the reviewer please make sure you re-request a review (see icon in the 'reviewers' section).