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

Create new funcionality to encrypt and decrypt data with private key. #43

Open
mmcs85 opened this issue Aug 27, 2018 · 18 comments
Open
Assignees
Labels
enhancement New feature or request

Comments

@mmcs85
Copy link

mmcs85 commented Aug 27, 2018

This is a continuation of the issue: GetScatter/ScatterWebExtension#39

There is two distinct use cases that are important.

  • Single account encryption using private key.
  • Two accounts data encryption exchange (Alice sends private data to Bob).

For the first case you can sign some TEXT with account private key and then use it on symmetrical encryption algorithms like AES-256. Or you can generate a sharedSecret between the account private key / public key.
In both cases a nonce is recommended to be used for generating the resulting symmetrical key to add entropy. Think of it as disposable keys per message.

For the second case the only option so far is to generate a sharedSecret between Alice private key / Bob public key. (indirect asymmetrical encryption). This also requires the nonce entropy.

My solution Proposal

In order to avoid using encryption inside scatter I propose to only calculate the disposable symmetrical key inside scatter and let the dapp developers use the symmetrical algorithms they choose. AES-XXX or any other.

Requirements to implementation
Note that so far the sharedSecret implemented in eosjs is based on secp256k1 curve that happens to be the default one used to create account keys in crypto. It returns 64 bytes from a sha512 hash.

The encryption key for AES-256 is 32 bytes + 16 bytes for a IV(initialization vector).

Is there a need to generate bigger encryption keys? If so a hashing algorithm that returns more bytes is needed.

check here: https://github.com/EOSIO/eosjs-ecc/blob/master/src/key_private.js#L81

In order to eosjs-ecc play nice with this implementation the crypt function would need receive as parameter the encryption key.

check here: https://github.com/EOSIO/eosjs-ecc/blob/master/src/aes.js#L56

Example:

scatter.getEncryptionKey(fromPubKey, toPubKey, nonce)
NOTE: the fromPubKey to obtain the privateKey inside scatter domain.

Returns the encryptionKey.

Advantages:

  • avoids scatter from encrypt/decrypt Data.
  • with nonce scatter can require user permission for each getEncryptionKey per message. Also avoids giving the shared secret to a rogue dapp.

Disadvantages:

  • none so far? Please comment
@ChenLi0830
Copy link

ChenLi0830 commented Aug 28, 2018

Hi @mmcs85 thanks for migrating the issue. A couple of quick questions with regards to the proposal

  1. how would you keep the record of the encryption key/keys on the client side?
  2. if there are multiple clients (web & mobile) how are you planning to sync the encryption key between those clients?

Thanks!

@mmcs85
Copy link
Author

mmcs85 commented Aug 30, 2018

1 - You can store with indexDB or localstorage.
2 - You can always reconstruct the same encryption key with the same parameters (fromPubKey, toPubKey, nonce) or with the inverse Pubkey and Privkey like asymmetric encryption. You just need to share this information between clients using a traditional server or the blockchain.

For example lets assume you want to do a scatter private chat dapp:
Lets assume you create a SmartContract with action:

sendprivmessage(account_name from, account_name to, string msg, nonce, checksum)

Use case, Alice sends msg to Bob:

  • get Alice@active public key.
  • get Bob@active public key.
  • generate nonce.
  • use scatter.getEncryptionKey(AlicePubKey, BobPubkey, nonce)
    This assumes scatter has the Alice entity loaded.
  • encript msg with AES-256 and get the checksum.
  • eos.pushTransaction(name = "sendprivmessage" , data:{from = alice, to = bob, msg = encrypt_msg, nonce, checksum})

Then Bob in another client can fetch the msg from Contract state or actions log and do:

  • use scatter.getEncryptionKey(BobPubkey, AlicePubKey, nonce)
    This assumes scatter has the Bob entity loaded.
  • decrypt using encryption_key + nonce + checksum

I hope you get the ideia.

Regards

@mlockett42
Copy link

Would also like this to be implement and have projects that could use it immediately if it was.
But the proposed solutions I feel are too complex, we only need the decryption functions. The encryption can be done with the standard eosjs functions and the target users publicly available key, it doesn't need to be in scatter.
Just something like
decrypted_data = scatter.decrypt(encrypted_data);
will do it. The only thing needed to decrypt the data is the private key which scatter has.
Also scatter would need to ask the user permission and throw an exception if the data cant be decrypted.
This link shows how to do this with eosjs if you have the private key.
EOSIO/eosjs-ecc#19 (comment)

@mmcs85
Copy link
Author

mmcs85 commented Sep 14, 2018

Encryption inside scatter already was proposed before but that may create a maintenance issue that if scatter has to update the algorithm for security reasons. It may break dapp's before they have time to update.

The solution that I proposed is to avoid doing the encryption inside scatter. Instead the symmetric encryption key is calculated inside scatter and then eosjs OR any other lib can use it to feed many symmetrical algorithms. This way the encryption algorithm is deferred to the dapp devs and they have time to update their dapps.

NOTE: It works as long the new algorithms support the returned encryption key size.

@cppfuns
Copy link

cppfuns commented Oct 12, 2018

@nsjames I also need this feature urgently. I feel that this function has a higher priority. Do you have any plans to support it?

@nsjames
Copy link
Contributor

nsjames commented Oct 13, 2018

c8b8252

See this branch. I'm not too sure if this would actually work yet as I haven't had time to test the encryption/decryption.

@mmcs85
Copy link
Author

mmcs85 commented Oct 18, 2018

Did this #141 PR to fix the public key format on the sharedSecret function

@DebugFuture
Copy link

@mmcs85 I think #141 fixed it. I did a test in my dapp.

@nsjames when will you have time to test this and release?

Thank you.

@nsjames
Copy link
Contributor

nsjames commented Nov 3, 2018

Sorry this didn't make it into the most recent update, i'll have to start revisiting it for next release. This looks good though in general. Extrapolating it into plugin specific should make it far stabler into the future ( albeit more work when adding blockchains ).

@utilsites
Copy link

Created another issue on eos-transit project to push on a standard functionality for multiple wallets on this subject: eosnewyork/eos-transit#2

@angelol
Copy link

angelol commented Mar 1, 2019

I see a huge security issue in the proposed solution. The problem is that ECIES is only secure if you can guarantee that a unique nonce is used every time. If you re-use a nonce, you leak your shared secret.

Now the problem is that the developer has to remember to request a new shared secret with a different nonce every time they want to encrypt a message. Since Scatter will ask the user for permission every time, the developer will also be tempted to minimise the number of confirmations shown to the user.

Considering the above, it doesn't take much imagination to see that this will happen rather sooner than later and that it will be used incorrectly more often than correctly. We can't expect the typical programmer to have extensive cryptography training to recognise this. I'd rather not see the FUD media articles about Scatter & EOS encryption having been hacked.

In my opinion, the best way to solve this, is for scatter simply implement the eosjs_ecc.encrypt and eosjs_ecc.decrypt functions (https://github.com/EOSIO/eosjs-ecc/blob/2cd505ccc717913b813488bcd652981203bb7586/src/aes.js#L30). These implement the ECIES standard and are identical for Bitcoin and other cryptocurrencies that use elliptic curve cryptography. The nonce has to be generated inside scatter to make sure it cannot be reused.

Does this create a maintenance issue for scatter if the encryption gets broken? I would argue that if ECIES gets broken, scatter maintenance issues are the least of our worries. The security of Bitcoin and other chains rely on the fact that the underlying ECC is secure. So replacing a know-to-be-secure implementation of cryptography with something the every developer has to roll on its own is not a very good trade-off. In fact we are violating the golden rule of cryptography to never roll your own.

@mmcs85
Copy link
Author

mmcs85 commented Mar 1, 2019

I see a huge security issue in the proposed solution. The problem is that ECIES is only secure if you can guarantee that a unique nonce is used every time. If you re-use a nonce, you leak your shared secret.

Now the problem is that the developer has to remember to request a new shared secret with a different nonce every time they want to encrypt a message. Since Scatter will ask the user for permission every time, the developer will also be tempted to minimise the number of confirmations shown to the user.

Considering the above, it doesn't take much imagination to see that this will happen rather sooner than later and that it will be used incorrectly more often than correctly. We can't expect the typical programmer to have extensive cryptography training to recognise this. I'd rather not see the FUD media articles about Scatter & EOS encryption having been hacked.

In my opinion, the best way to solve this, is for scatter simply implement the eosjs_ecc.encrypt and eosjs_ecc.decrypt functions (https://github.com/EOSIO/eosjs-ecc/blob/2cd505ccc717913b813488bcd652981203bb7586/src/aes.js#L30). These implement the ECIES standard and are identical for Bitcoin and other cryptocurrencies that use elliptic curve cryptography. The nonce has to be generated inside scatter to make sure it cannot be reused.

Does this create a maintenance issue for scatter if the encryption gets broken? I would argue that if ECIES gets broken, scatter maintenance issues are the least of our worries. The security of Bitcoin and other chains rely on the fact that the underlying ECC is secure. So replacing a know-to-be-secure implementation of cryptography with something the every developer has to roll on its own is not a very good trade-off. In fact we are violating the golden rule of cryptography to never roll your own.

I'm glad you giving a good though about this and more eyes on this discussion the better.

1 - Can you provide more detailed information how reusing the same nonce leaks the shared secret? Its important to understand exactly how it happens.

Also symmetric key that goes to AES is based on a 512hash of the shared secret combined with nonce. Shared secret is never used directly to construct the symmetric key nor you can inverse the symmetric key and obtain the shared secret.
Also shared secret is always calculated inside the wallet.
https://github.com/EOSIO/eosjs-ecc/blob/2cd505ccc717913b813488bcd652981203bb7586/src/aes.js#L82

2 - This is a valid concern and the question is should scatter show a popup allowing per dapp encryption? Should user be allowed to whitelist? This is a big question indeed...

3 - Dapp dev can still use the same standard no issue there. But can as well change to a better one battle tested Symmetric Encryption scheme.

4 - Please note that ECIES can use multiple Symmetric Encryption schemes. So what we talking about is security vulnerabilities on symmetric encryption scheme's. Not about ECC security related to every crypto out there. If ECC is vulnerable then every transaction can be forged for example.

Finally nonce needs to be know between the 2 users of scatter. Is input to obtain the same symmetrical key on both ends. Thats the reason it is a input to the api.

@angelol
Copy link

angelol commented Mar 5, 2019

Thank you for bearing with me on this. Now to your questions:

  1. The problem is that some block cipher modes fail catastrophically if the same IV is used twice. The one used by eosjs-ecc (AES-256-cbc) thankfully isn't one of them. So while reusing an IV would not lead to a catastrophe in this case, it would still weaken the encryption scheme. But since the developer in this case has to implement their own encryption, we can't know what they will do. My thinking was: eosjs-ecc already implements a standard way of encryption native to EOS, why not use it rather than having every developer to use their own encryption? It would also make it interoperable.

  2. Yes. If the app gets hold of the shared secret, as in your proposal, it can encrypt and sign arbitrary data as many times as it wants (with the same nonce every time which weakens the security of the encryption, which makes it even worse). I'm not sure if that is consistent with scatter's security model of requiring confirmation for every action (@nsjames ?).

  3. No, they can't really use the standard encryption scheme without writing custom code, because eosjs-ecc only exports the encrypt and decrypt functions.

  4. Yeah, I was talking about the implementation in eosjs-ecc. It's somewhat of a standard to encrypt memos from back in the steem days. Using it would make it interoperable.

In order to prevent problems 2) and 3), the best way would be to perform the encryption and authentication inside scatter. I totally agree with you that it's not ideal either, but to me, it's the least evil of the two options.

By the way: While looking at the code of eosjs-ecc again, I noticed that it uses the 'aes-256-cbc' block cipher mode without authentication 🤯. While 'aes-256-cbc' is a pretty robust choice, it's only secure when used together with a message authentication code. So upon close examination, eosjs-ecc turns out not to be so great either.

🤔🤔🤔

@mmcs85
Copy link
Author

mmcs85 commented Mar 5, 2019

This is a great discussion topic thanks for taking your time to look into it!
Some notes about the topic:

1 - I agree that reusing same IV can weaken the security but that does not mean it leaks the shared secret. Developers can use existing implementations or use their own still is up to them to audit and make sure is secure enough by generating a nonce each message.

2 - Again there is a difference between shared secret and encryption key.
encryption_key = sha512(nonce+shared_secret) <-- not reversible

A encryption_key leak means you can read only 1 message per nonce.
If app dev reuses the nonce then you can think of a open channel leak between 2 peers.
A shared secret leak means a leak for every possible open channel you can create between 2 peers. I don't believe my proposal leaks the shared secret.

There is also the possibility to not provide a nonce and make scatter generate it by default and return it to the dapp.

3 - how so? you can use any standard symmetrical encryption algorithm implementation in Javascript. eosjs-cc is not mandatory to use. Whats the added value for making it inter operable?

I agree on the point that using encrypt and decrypt on scatter indeed has the advantage of enforcing generating a ephemeral key each time by generating a nonce and hiding the encryption key.

Did not know that aes-256-cbc was not robust enough. But that kinda proves the point of scatter maintenance issue. In the future any other mode or AES can just get broken. and a new symmetrical algorithm can be developed and you can still use the same encryption_key above as input to this new algorithm.

@nsjames
Copy link
Contributor

nsjames commented Mar 6, 2019

Right, we don't even use CBC inside of Scatter since it has a few "hmmmm" things in it, we default to GCM.

I think the larger benefit here is that if we can create a forward secrecy pattern which is more dynamic then we could even do things like EOS <> ETH encrypted chats, or even EOS <> BTC. Which should be the real goal there.

@angelol
Copy link

angelol commented Mar 6, 2019

Alright guys, I've changed my mind. Thank you for letting me challenge the proposal.

Since eosjs-ecc is not the as foolproof as I hoped it would be, it's probably better to let the developer choose their own encryption. There are good libraries out there like tweetnacl-js that are suited for this.

If it's compatible with Scatter's security model, then requiring only one confirmation to set up a "secure channel" as you describe it per public/private key pair should be fine. The resulting shared secret can then be used together with a random nonce and tweetnacl's secretbox to implement a secure system.

The key derivation is the same as on ETH and BTC, so it could be used to implement EOS <> BTC communication schemes, which is awesome. I love the idea.

@friedger
Copy link

friedger commented May 21, 2019

Another use case for this function is user storage for preferences, personal data, etc.

dApps could use storage that is address-based access controlled (like https://github.com/blockstack/gaia). The encryption key returned by 'getEncryptionKey' can be used as the private key for the gaia bucket/address.

(All the blockstack apps on app.co/blockstack could then authenticate with eos accounts)

@medatlas
Copy link

medatlas commented Jun 30, 2019

I see a huge security issue in the proposed solution. The problem is that ECIES is only secure if you can guarantee that a unique nonce is used every time. If you re-use a nonce, you leak your shared secret.

Now the problem is that the developer has to remember to request a new shared secret with a different nonce every time they want to encrypt a message. Since Scatter will ask the user for permission every time, the developer will also be tempted to minimise the number of confirmations shown to the user.

Considering the above, it doesn't take much imagination to see that this will happen rather sooner than later and that it will be used incorrectly more often than correctly. We can't expect the typical programmer to have extensive cryptography training to recognise this. I'd rather not see the FUD media articles about Scatter & EOS encryption having been hacked.

In my opinion, the best way to solve this, is for scatter simply implement the eosjs_ecc.encrypt and eosjs_ecc.decrypt functions (https://github.com/EOSIO/eosjs-ecc/blob/2cd505ccc717913b813488bcd652981203bb7586/src/aes.js#L30). These implement the ECIES standard and are identical for Bitcoin and other cryptocurrencies that use elliptic curve cryptography. The nonce has to be generated inside scatter to make sure it cannot be reused.

Does this create a maintenance issue for scatter if the encryption gets broken? I would argue that if ECIES gets broken, scatter maintenance issues are the least of our worries. The security of Bitcoin and other chains rely on the fact that the underlying ECC is secure. So replacing a know-to-be-secure implementation of cryptography with something the every developer has to roll on its own is not a very good trade-off. In fact we are violating the golden rule of cryptography to never roll your own.

The solution would be to make Scatter generate a random nonce and return a mp with the shared key and nonce. But still, developers may not use the returned nonce. So, the ideal is to do the encryption inside Scatter.

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

No branches or pull requests

11 participants