No support for encrypting with pbkdf2?

[michaelsbradleyjr]

I've looked at the source code, but wanted to ask in case I've misunderstood or missed something...

At present it's possible to decrypt wallets that were encrypted with pbkdf2, but it's not possible to encrypt a wallet with pbkdf2 instead of scrypt. Is that correct?

[ricmoo]

That is correct. The use of pbkdf2 wallets is highly unrecommended. In terms of protecting a wallet from a brute force attack they provide nearly no security, at least at the iteration count used today, and it provides no future-proof security, becoming less secure linearly over time. Every 12-18 months a pbkdf2 password will be twice as easy to break into.

Why? Is there a usecase where you need to generate one?

[michaelsbradleyjr]

I'm using ethers in refactors of test suites for ethereumjs-wallet and web3's wallet, i.e. to compare encryption output against a wholly independent implementation (those two share a lot of very similar code). I can just skip checking against ethers output re: pbkdf2.

[ricmoo]

Yeah, I could add it (it’s very simple and all the code is pretty much there), but I worry if it is an option, people might use it. :p

Like you said, they should decrypt properly, so maybe skipping the test case is the easiest thing. But I could certainly be convinced to include encrypting, it would just likely be lower on my backlog. :)

The test suite in ethers does include pbkdf2-based JSON wallets created using Parity from a few years ago, if you need more test cases. :)

[michaelsbradleyjr]

This may seem random, but is coming up in context of doing encrypt/decrypt comparisons across libraries. Note the kdf is pbkdf2.

const w =
      '{"crypto":{"cipher":"aes-128-ctr","cipherparams":{"iv":"6087dab2f9fdbbfaddc31a909735c1e6"},"ciphertext":"5318b4d5bcd28de64ee5559e671353e16f075ecae9f99c7a79a38af5f869aa46","kdf":"pbkdf2","kdfparams":{"c":262144,"dklen":32,"prf":"hmac-sha256","salt":"ae3cd4e7013836a3df6bd7241b12db061dbe2c6785853cce422d148a624ce0bd"},"mac":"517ead924a9d0dc3124507e3393d175ce3ff7c1e96529c6c555ce9e51205e9b2"},"id":"3198bc9c-6672-5ab3-d995-4942343ae5b6","version":3}';

ethers.Wallet.fromEncryptedJson(w, 'testpassword');

I'm getting an exception:

UnhandledPromiseRejectionWarning: Error: invalid address (arg="address", value=undefined, version=4.0.33)

Whereas:

web3.eth.accounts.decrypt(w, 'testpassword');
ethjsWallet.fromV3(w, 'testpassword');

Those both work. So I'm wondering if in this case I've found a bug in ethers.

Originally, I found bugs in web3 and ethereumjs-wallet when encrypting with scrypt and manually supplied salt, iv, uuid (has to do with strings not being normalized into buffers) — the encrypted output didn't match that of ethers.

[michaelsbradleyjr]

FWIW, I tried w3.eth.account.decrypt(w, 'testpassword') (where w is the same string as in my previous comment) using the python implementation of web3 — it worked and was in agreement with web3's wallet and ethereumjs-wallet.

[ricmoo]

All the passwords are normalized using NFKC, I believe, into bytes but I do see the problem. That wallet you provided does not have the address included in the payload.

Where did you get that file from? The Secret Storage Keystore format is supposed to contain an address key (where the address is optionally allowed to omit the 0x prefix). But if a popular piece of software is omitting the address, I can relax verifying the address in the JSON file against the address for the decrypted private key, if not present.

[michaelsbradleyjr]

I'm not sure how official it is but in: https://github.com/ethereum/wiki/wiki/Web3-Secret-Storage-Definition#alterations-from-version-1

I read: "Address unnecessary and compromises privacy"

I got that string from: https://github.com/ethereumjs/ethereumjs-wallet/blob/master/test/index.ts#L235-L236

I'm presently refactoring that test suite.

[ricmoo]

Awesome! Thanks, I’ll make address an optional property then. :)

I do recall reading that, back in the day, now that you mention it, but the examples still had address. :p

I’ll fix that, update the test cases and publish shortly.

[michaelsbradleyjr]

Great! Thanks for the quick responses.