Hey @topealabi, this looks great, thanks for all the hard work, coordinating with other groups and building this! I can't wait to integrate it into MetaMask!

Looks like you found a solution to the encryption curve solution that @chrislundkvist raised (pinging him to verify).

Is this compatible with the parity encrypt/decrypt methods?

If we have buy-in from a couple of the other encrypt-wanting developers, I'll feel very confident merging this. (I'll direct a few people here)

A few nit-picky apprehensions with this PR are formatting/stylistic things:

• There are a lot of new invisible files added to the repo
• Looks like you ran the whole project through a linter, and so basically ever line of code has changed. This isn't ideal, because it makes the PR much harder to simply review.
• You've introduced ES6 to the module, which means consumers who deliver ES5-compatible projects will no longer do so. (Solution is to either not use ES6, or include a bundle/build step to the project)

I don't want to block on any of those, because I'm very grateful for the work, but I would appreciate if you clean up as much of those issues as possible before I merge.

Sweet

For reference, here's Parity's implementation:

https://github.com/paritytech/parity/blob/bd7273061e3f13f53fdd2fa91dc641a83bcc47f0/ethcore/crypto/src/lib.rs#L211

They prepend a version byte to the ciphertext which would be useful down the road for versioning and expanding the capability. Even if 1:1 parity with parity isn't feasible, it would be nice to have the ciphertext versioned in some way in my opinion

Right now utils/ecdh.node is completely unreadable, which isn't really okay for a public project PR. It would be much better to use npm to install a well audited and reviewed encryption module that we can use for this operation.

Also, this module should work in the browser, so .node sounds like it may only work in a node environment, and we need it to be functional after being browserified.

I'd like to see this paired with an EIP, to get the larger community's opinion.

Ideally the EIP would propose a cross-client method for requesting encryption/decryption.

I would prefer if this method included a type/version parameter, so that different encryption methods can be added under the same method name. We're seeing the downside of hard-coded signature methods right now in the signTypedData discussion, where the exact implementation has changed as the discussion has gone on, and now there's no easy way for clients to iterate without breaking existing Dapps.

Example:

/**
* Decrypts some encrypted data.
* @param {Account} account - The account to decrypt with
* @param {string} version - A unique string identifying the decryption strategy.
* @param {Object} data - The data to decrypt
* @param {Function} callback - The function to call back when decryption is complete.
*/
web3.eth.decrypt = function decrypt (account, version, data, callback) { /* implementation */ }
web3.eth.decrypt(account, 'encrypt-eip-1024-v1', data, callback)

By adding this type parameter, we can implement many encryption/decryption strategies that evolve over time.

I also would really like @christianlundkvist 's take on whether this encryption strategy is sound and secure.

@danfinlay I agree. Cross-client compatibility would be great for the ecosystem.

Parity currently prepends a ‘0x04’ to the ciphertext for versioning. I think we could do something similar. Kudos to @kjbbb for pointing that out. @christianlundkvist any thoughts on the ECIES implementation described above? EC(25519) for shared secret and aes256Cbc to generate cyphertext?

General thoughts


Some of my thoughts are summarized in this comment.

In the Ethereum community people really want to use the secp256k1 curve for encryption since this curve is already used for signing and wallets mostly support handling the private keys and generating public keys. This curve is not well suited for this task according to cryptographers, so most people use modern crypto libraries like nacl which uses curve25519 which is explicitly designed with encryption in mind.

The result of this is that Ethereum devs need to use ECIES which theoretically works for any curve but has no solid/audited implementations (since most people are just using nacl anyway). So the consequence is that people end up just rolling their own ECIES implementations - Parity did this, the Whisper implementers did this and this PR is another hand-rolled implementation.

This proliferation of hand-rolled encryption libraries seems a bit worrying. For instance I don’t see any of these libraries worrying about protection against small-subgroup (“twist”) attacks which is one of the subtle gotchas of ECIES (and one of the reasons cryptographers are not fans of secp256k1 for encryption):

http://safecurves.cr.yp.to/twist.html

Specific points about this PR

I also would really like @christianlundkvist 's take on whether this encryption strategy is sound and secure.

In general the code seems to be well written and I don’t see any obvious problems apart from the twist protection (which I believe all of these implementations lack). I’m not comfortable giving a thorough cryptographic review or “sign off” since I’m not a cryptographer. There may be other cryptography gotchas that I’ve missed (another reason I prefer complete, audited libraries like nacl.)

We use a curve25519 ECDH algorithm



What does this mean? As far as I can tell you are using a standard ECDH algorithm (pure EC multiplication from the elliptic library) using the secp256k1 curve, I don’t see curve25519 used anywhere. Am I missing something?

We generate a cipher text with the shared secret using the aes256CbcEncrypt algorithm



I personally much prefer the secretbox function from nacl for symmetric encryption. For AES we always have a lot of choices like “oh yeah one of those modes were horribly broken, which one was it?”, “what to do with the IV”, “need to make sure to use an HMAC on top to get authenticated encryption, what hash function to use?”. See this article for instance for some pitfalls:


https://tonyarcieri.com/all-the-crypto-code-youve-ever-written-is-probably-broken

That being said as far as I see this code follows best practices around AES (i.e. CBC with random IV, MAC using sha256). What I like about secretbox is that all you need to do is input message, key and random nonce and you’ve got authenticated encryption out of the (secret)box.

Possible alternative encryption/decryption implementations

Another way to implement encryption/decryption is the following: Add a method like getEncryptionPubKey(address) which computes the corresponding curve25519 point for the private key corresponding to the specified Ethereum address. This curve point will act as the public encryption key corresponding to the address. Now the encrypt function can use the nacl.box function directly with this public key (along with an ephemeral key pair). This function incorporates all the ECDH operations as well as authenticated symmetric encryption. The decrypt function would use nacl.box.open.

Upsides to this method:



• Based on modern, audited crypto library nacl with solid implementation (tweet-nacl)
• Minimal wrapper around the above

Downsides:



• No way to extract the public key from an ETH transaction or signature (which I see suggested in some places)
• No way to use BIP32 methods to derive public keys from master public keys

Conclusions

The code looks ok and I don’t see any major issues but I’m in general wary of the proliferation of hand-rolled secp256k1 ECIES implementations in the Ethereum space, and I personally much prefer the curve25519 based encryption algorithms.


I agree with @FlySwatter that it’s important to have these discussions occur in an EIP so we can have input from professional cryptographers, and ideally to have client independent protocols.

I also fully support the ability to support different versions or types. That way other encryption methods can be supported like the curve25519 one described above. 

I don’t think it’s enough to just have a version byte in the cipher text. The whole structure needs to have a version or type field to distinguish between different encryption methods.

Made some updates!

Also maybe add a couple tests to check that the error handling works as expected?

Another thing that we need to discuss is padding. nacl.box and nacl.secretbox do not take padding into account at all. If the cleartext message is of length N, then the encrypted message will always be of length N + nacl.box.overheadLength. So we need to be very clear that implementers will need to take padding into account when encrypting.

If you don't take padding into account it could break the whole security of the system being built. Imagine a voting app where a user submits an encrypted string which is either "Yes" or "No". If the implementer forgets about padding anyone can compute if the length of the cleartext string is either 3 or 2 and thus figure out which string is the cleartext one.

So in this example the implementer would need to have the user sign either "Yes" or "No" which are both of length 3.

Another example is encrypting a mnemonic seed. If you reveal the length of the seed you reveal a lot about which words may be included in the seed. So you'd need to add padding in this case as well, see for instance here in lightwallet:

https://github.com/ConsenSys/eth-lightwallet/blob/master/lib/keystore.js#L78

Any comments on padding? Add a padding parameter or just don’t do padding and leave it to implementers? I’m ok with either but like some other inputs/opinions.

Discussed this with Christian. The padding is used to obscure inputs that can be different lengths. Since inputs can vary widely in their lengths, we are going to leave padding outside of this API, and simply make it the responsibility of documentation to communicate the importance of properly padding inputs.

(ie, a two-letter ciphertext that can be either yes or no is obviously yes).

Aside from that documentation caveat, we're ready to merge!

Closing in favor of #29 which can run the tests & fixes merge conflicts.