Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use proper authenticated encryption instead of insecure self-baked scheme #61

Open
wants to merge 19 commits into
base: master
Choose a base branch
from

Conversation

angelol
Copy link

@angelol angelol commented Jul 23, 2019

We at privEOS are using eosjs-ecc and found a number of issues that this pull request fixes:

  1. Replace insecure use of AES-256-CBC, which is an unauthenticated mode, with tweetnacl. Tweetnacl has been audited and found to be secure and it implements proper authenticated encryption.

  2. Removes insecure use of checksum that leaks a hash of the private key to the public.

  3. Ability to encrypt data using a pre-existing shared secret. This adds interoperability with a huge number of tools, including hardware security modules and Scatter (@nsjames).

package.json Outdated
@@ -1,5 +1,5 @@
{
"name": "eosjs-ecc",
"name": "eosjs-ecc-priveos",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should remain eosjs-ecc

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Of course, my bad

@jcalfee
Copy link
Contributor

jcalfee commented Sep 10, 2019

From what I see, there are several different encrypted message schemes. How about deprecating this in eosjs-ecc then removing it? The shares secret can still come from eoejs-ecc.

@angelol
Copy link
Author

angelol commented Sep 10, 2019

From what I see, there are several different encrypted message schemes. How about deprecating this in eosjs-ecc then removing it? The shares secret can still come from eoejs-ecc.

Suggestion:

For compatibility with messages that were encrypted with the old version, it may make sense to detect the old format end decrypt it using the old algorithm. New messages should be encrypted using the new algorithm. That way, the change would be backwards compatible.

@jcalfee
Copy link
Contributor

jcalfee commented Sep 10, 2019

TweetNaCl has its own signing, sha implementation, keypair object, etc.. so that is all code that eosjs-ecc has already. Seems like you just need the well-known Encrypt-then-MAC code for message encryption (I assume that is what TweetNaCl is doing: Encrypt-then-MAC).

@angelol
Copy link
Author

angelol commented Sep 10, 2019

TweetNaCl has its own signing, sha implementation, keypair object, etc.. so that is all code that eosjs-ecc has already. Seems like you just need the well-known Encrypt-then-MAC code for message encryption (I assume that is what TweetNaCl is doing: Encrypt-then-MAC).

No, it's a streaming cypher with built-in authentication. The tweetnacl-js library has already been audited (report here). That's why it would be much more secure than a self-made encrypt-then-mac implementation. Such a self-made implementation would have to be audited again.

@jcalfee
Copy link
Contributor

jcalfee commented Sep 10, 2019

Agreed, a streaming cipher is better. This library and its direction is out of my hands. I helped to bring it to b1's attention though. Let see what they say.

For my own curiosity: did you look comparability for other platforms like and Java/Android and iOS?

@angelol
Copy link
Author

angelol commented Sep 10, 2019

Agreed, a streaming cipher is better. This library and its direction is out of my hands. I helped to bring it to b1's attention though. Let see what they say.

Thank you!

For my own curiosity: did you look comparability for other platforms like and Java/Android and iOS?

It's a 1:1 port of tweetnacl to javascript. So it should be compatible to tweetnacl libraries on other platforms.

@jcalfee
Copy link
Contributor

jcalfee commented Sep 13, 2019

I got some feedback (and permission to repost it): We're working on replacing eosjs-ecc with elliptic. I was directed to the eosjs develop branch. I see they did remove eosjs-ecc EOSIO/eosjs@81d2323 .. I did not find any code to encrypt or decrypt data in that library.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants