Can the max value for the maxmem option of crypto.scrypt/Sync be reconsidered?

[michaelsbradleyjr]

Is your feature request related to a problem? Please describe.

The Ethereum community includes a sizable community of developers and users who make heavy use of Node and the npm ecosystem. The scrypt key derivation function is a core component of Ethereum's cryptopgraphy. Within that community there is presently interest in moving as much as possible to Node's crypto built-ins (and so to more recent versions of Node) and away from 3rd party modules, e.g. the deprecated scrypt package.

However, the way that Node's built-in crypto.scrypt currently implements its memory limit is incompatible with options N, r, p value-combinations that are commonly seen in sample/test data for Ethereum and real-world usage as well. Examples:

https://github.com/ethereum/go-ethereum/blob/master/accounts/keystore/testdata/v3_test_vector.json

https://github.com/paritytech/parity-ethereum/blob/7c335e87642b848f61ab3ee7bbff5964b53d6f11/accounts/ethstore/src/json/key_file.rs#L201-L208

https://github.com/ethereumjs/ethereumjs-wallet/blob/master/src/test/index.js

Even if the maxmem option for Node's crypto.scrypt is set to the max value allowed (4294967295), there will be an exception such as:

Error: error:060B50AC:digital envelope routines:EVP_PBE_scrypt:memory limit exceeded
      at handleError (internal/crypto/scrypt.js:62:14)
      at Object.scrypt (internal/crypto/scrypt.js:47:3)
      ...

Describe the solution you'd like

Node's crypto.scrypt and crypto.scryptSync are revised to allow for a maxmem value corresponding to options N, r, p value-combinations that are in wide use with other scrypt implementations, i.e. in JS but also other programming languages (golang example, pure-JS example, another pure-JS example). Alternatively, the way that Node checks for too large N, r, p value-combinations is revised to the same end.

Describe alternatives you've considered

(a) A solution exists for Node >=10.5.0 <12.0.0 whereby, via an scrypt function shim that has fallback logic for memory limit exceptions (example), it's suggested to users that the scrypt package be installed in their own projects. This has obvious downsides:

1. It's a deprecated package.
2. That deprecated package is a 3rd party package vs. a Node built-in.
3. That deprecated package isn't compatible with Node >=12.0.0.
4. If the deprecated package isn't installed then the shim must fall back to a pure-JS implementation, see downsides of (b) below.

(b) For Node >=12.0.0 the only solution presently is to implement an scrypt function shim with automatic fallback to a pure-JS implementation. Downsides:

1. Performance will greatly suffer for widely used N, r, p value-combinations.
2. The pure-JS implementation will come from a 3rd party package vs. a Node built-in.

[bnoordhuis]

I think that would be acceptable. EVP_PBE_scrypt() takes uint64s for N, r, p and maxmem but crypto.scrypt() currently restricts them to uint32s. I don't see a problem with widening at least maxmem to uint64.

Implementation-wise, lib/internal/crypto/scrypt.js would have to check that it's 0 <= maxmem <= Number.MAX_SAFE_INTEGER and then src/node_crypto.cc can just check that args[6]->IsNumber() and call static_cast<uint64_t>(args[6]->IntegerValue(context).ToChecked()).

I suppose it could also be overloaded to accept a BigInt but then you need to check whether it fits in an uint64.

[tniessen]

Implementation without BigInt support in #28799.