scrypt usage not compliant with RFC 7914?

[michaelsbradleyjr]

See: https://tools.ietf.org/html/rfc7914. In particular, Section 2: scrypt Parameters:

The CPU/Memory cost parameter N ("costParameter") must be larger than 1, a power of 2, and less than 2^(128 * r / 8). The parallelization parameter p ("parallelizationParameter") is a positive integer less than or equal to ((2^32-1) * 32) / (128 * r).

go-ethereum test data: https://github.com/ethereum/go-ethereum/blob/master/accounts/keystore/testdata/v3_test_vector.json#L13-L15.

Parameters {N: 262144, r: 1, p: 8} are not valid for an RFC-compliant implementation of scrypt.

Can someone on the geth team shed light on why geth's usage of scrypt is not compliant with the RFC? Was/is it a deliberate decision or an accident?

Context re: why I'm filing this issue: nodejs/node#28799 (comment).

geth's test data has long been adapted for use in test suite's of tools for other runtimes. For example, see: https://github.com/ethereum/web3.js/blob/1.x/test/eth.accounts.encrypt-decrypt.js#L32-L34. The PR that landed that test script/data was made in mid 2017. Likewise, implementations of scrypt (for those other runtimes; example: scryptsy) that are used to build Ethereum tooling don't adhere to the RFC.

In the case of Node.js, per the GitHub comment linked above, the built-in RFC-compliant scrypt can't handle the N, r, p combination in the test data.

[michaelsbradleyjr]

It seems that Go's implementation of scrypt, used by geth, doesn't adhere to the RFC:

https://github.com/golang/crypto/blob/master/scrypt/scrypt.go#L200

That is, it doesn't enforce N < 2^(128 * r / 8) as far as I can tell (I'm not fluent in Go). In the Go playground I find that N's upper limit when r is 1 is 16777215 such that {N: 262144, r: 1, p: 8} won't cause scrypt to choke.

I suspect the answer to my question ("Can someone shed light...?") boils down to: "geth's test data reflects what Go's scrypt allows for."

On the other hand, the Rust implementation of scrypt used by parity does seem to restrict N according to the RFC:

https://docs.rs/rust-crypto/0.2.36/src/crypto/scrypt.rs.html#193-196

I don't know Rust, so I can't speak to whether those lines are an exact match for N < 2^(128 * r / 8) but I will try geth's test data with parity and see what happens.

[michaelsbradleyjr]

I conducted the experiment with parity vs. geth:

With a wallet based on geth's test data, but regen'd so as to include the address (otherwise an exact match):

{
   "version":3,
   "id":"3198bc9c-6672-4ab3-9995-4942343ae5b6",
   "address":"008aeeda4d805471df9b2a5b0f38a0c3bcba786b",
   "crypto":{
      "ciphertext":"d172bf743a674da9cdad04534d56926ef8358534d458fffccd4e6ad2fbde479c",
      "cipherparams":{
         "iv":"83dbcc02d8ccb40e466191a123791e0e"
      },
      "cipher":"aes-128-ctr",
      "kdf":"scrypt",
      "kdfparams":{
         "dklen":32,
         "salt":"ab0c7876052600dd703518d6fc3fe8984592145b591fc8fb5c6d43190334ba19",
         "n":262144,
         "r":1,
         "p":8
      },
      "mac":"2103ac29920d71da29f15d75b4a16dbe95cfd7ff8faea1056c33131d846e3097"
   }
}

With that wallet in the keystore and web3.js connected to geth, this works in a Node.js REPL:

> web3.eth.personal.unlockAccount('0x008AeEda4D805471dF9b2A5B0f38A0C3bCBA786b', 'testpassword', null).then(v => console.log(v)).catch(err => console.error(err))
// prints true

But it does not work when web3.js is connected to a parity client that has loaded the same wallet. When using --logging=debug with parity client, I see this in the output when attempting to unlock:

2019-08-18 10:48:24  jsonrpc-eventloop-0 DEBUG rpc  Response: {"jsonrpc":"2.0","error":{"code":-32023,"message":"Unable to unlock the account.","data":"EthCrypto(Scrypt(InvalidN))"},"id":42}.

---

I think the right thing would be for geth to start checking the kdf params for correctness re: the scrypt RFC before calling golang's scrypt implementation; and that geth's test wallets should be refactored so that they're all valid with respect to the RFC.

[karalabe]

I think the right thing would be for geth to start checking the kdf params for correctness re: the scrypt RFC before calling golang's scrypt implementation; and that geth's test wallets should be refactored so that they're all valid with respect to the RFC.

Geth uses {N=262144, R=8, P=1} for wallets, optionally supporting {N=4096, R=8, P=6} for low powered devices if explicitly requested. Geth will never generate a wallet other than these two, so I don't think it's too big of an issue that we can handle more keys than the IETF standard specs, as long as they are cryptographically correct.

As for invalidating the tests, we don't plan on overriding Go's crypto constraints. I'm not saying that those should not be updated, but please open an issue against upstream Go. That is the place to fix standardization issues, not downstream packages. If they accept the proposal, we'll gladly pull in the changes.

[michaelsbradleyjr]

I opened an issue upstream a few minutes after I opened this one. See #33703.

I also opened an upstream issue, of sorts, with ethereum/wiki, see #674.

As I mentioned in the latter issue, portability seems to be a concern. A wallet with {N=262144, R=1, P=8} as found in geth's tests won't work with parity nor with wallet implementations in JavaScript that rely on Node's built-in scrypt, i.e. without a clumsy fallback to an scrypt implementation that's non-compliant in the same way as Go's.

As you note, in practice, because geth only generates wallets with valid params it's probably not a huge issue for geth users. I also understand your decision to wait on an upstream change. But this was a rather confusing puzzle to piece together, and it already has implications for JS libs such as web3.js and ethereumjs-wallet.

[karalabe]

The RFC's author says that's an error in the spec golang/go#33703 (comment) and the code is actually correct. They'll probably fix up the spec whenever.

[tniessen]

They'll probably fix up the spec whenever.

I found the mistake in the RFC and filed errata a few months ago. See 5971, 5972, and 5973.

[karalabe]

@tniessen Thank you for the corrections and the confirmation!