Page MenuHomePhabricator
Differential D10692

[Cashtab] Option to send encrypted messages
ClosedPublic
Actions

Authored by emack on Dec 18 2021, 03:14.

Details

Reviewers
bytesofman
Group Reviewers
Restricted Project
Commits
rABC101ad1f4c08f: [Cashtab] Option to send encrypted messages
Summary

This diff builds on the standardized prefix approach in D10628 by introducing a Cashtab encryption prefix prior to the encrypted hex. The encryption prefix flags whether this OP_RETURN message has been encrypted or not.

The ecies-lite library is used, which is an elliptic curve cryptography library that encrypts and decrypts messages using the same eliptic curve cryptography used by Bitcoin.

End to end encryption summary:

Sender's side

  1. Send.js flags to useBch() whether the user wants to encrypt the message or not.
  2. If the encryption flag is true, useBCH.js' sendXec() function retrieves the public key corresponding to the recipient's address and then calls on ecies-lite to encrypt the message based on that public key.
  3. The encrypted string is then added to the OP_RETURN script after all the preceding prefixes.

Recipient's side

  1. As a routine tx history update, useBCH.js' parseTx() function checks if the message is an encrypted cashtab message, and if so, extract the encrypted string from the output hex, which should match the string from step 3.
  2. The ecies-lite library is then called to decrypt the encrypted message using the recipient's private key. The private key is obtained via wallet.state.slpBalancesAndUtxos.nonSlpUtxos[0].wif
  3. If the encryption is successful, the decrypted message is sent up to Tx.js for rendering. Otherwise, it is assumed the user is not authorised to view the message, which is then overwritten by a default placeholder text for Tx.js.

Sample breakdown of output hex

6a = OP_RETURN
04 = bytecount
65746162 = cashtab encryption prefix
4c = OP_PUSHDATA1
91 = bytecount
0422609b9c1ee6dd93cd332edcb809c326c14b3f663a47556ae6d7d8a6e2cd6f7ecebcff96d6de0bf697353edbf98bd792ca1e6b3842251d4c95cd3ba7856bc794e9f82a8f443681c2cb2366794b760515f58942ce880923eb084ed80c216cd256265c2e608ae20db6d53c51f954049dfdd106bfcd800936c3353f7182469326aa38d6b427d68335466b87c1abaeda275d = encrypted message

Test Plan

npm start

  1. send a normal XEC tx with no message to ensure no regression
  2. send an unencrypted message from wallet A to wallet B, ensure no regression and both wallets can view the message in tx history
  3. send an encrypted message from wallet A to wallet B, ensure only wallet B can view the decrypted message in tx history and that wallet A sees an "Only the message recipient can view this" message in red italics, and wallet B sees the decrypted message in normal black font.
  4. send a short 3 character message in encrypted mode to wallet B
  5. send a long 94 character message in encrypted mode to wallet B
  6. input a 160 character message in unencrypted mode then switch to encrypted mode and ensure the input is truncated up to the allowed 94 character limit. Send this message and ensure the underlying message sent is the 94 character message.
  7. Repeat step 5 except send it in unencrypted form and ensure the full 160 characters is sent despite switching to the truncated encrypted view.
  8. send an encrypted message to a newly created wallet with no transactions and ensure the 'Cannot send an encrypted message to a wallet with no outgoing transactions' error is displayed.
  9. send an encrypted message to a newly created wallet with incoming transactions but no outbound transactions and ensure the 'Cannot send an encrypted message to a wallet with no outgoing transactions' error is displayed.
  10. use the newly created wallet to send an outbound XEC transaction. Then ensure wallet A is able to now successfully send an encrypted message to this newly created wallet and wallet B can decrypt successfully.
  11. send an encrypted message from wallet A to wallet A, ensure wallet A can view an encrypted message to itself in tx history
  12. switch to multiple recipients mode and ensure the encryption switch and the alert is not rendered
  13. send multi recipient tx and ensure no unintended encryption
  14. send multi recipient tx with a message and ensure no unintended encryption
  15. check the TX ID for one of the above encrypted messages in blockchain, abc explorer and be.cash explorer, and ensure the message is not legible

Diff Detail

Repository
rABC Bitcoin ABC
Branch
privateMsg
Lint
Lint Passed
Unit
No Test Coverage
Build Status
Buildable 17672
Build 35170: Build Diffcashtab-tests
Build 35169: arc lint + arc unit

Event Timeline

emack created this revision.Dec 18 2021, 03:14
Herald added a reviewer: Restricted Project. · View Herald TranscriptDec 18 2021, 03:14
emack requested review of this revision.Dec 18 2021, 03:14
emack mentioned this in D10630: [Cashtab] Add encrypted messages option (for discussion only).Dec 18 2021, 03:15
Harbormaster completed remote builds in B17672: Diff 31456.Dec 18 2021, 03:18
emack edited the summary of this revision. (Show Details)Dec 18 2021, 03:19
emack edited the summary of this revision. (Show Details)Dec 18 2021, 03:32
hungsam added a subscriber: hungsam.Dec 18 2021, 04:46
hungsam added inline comments.
web/cashtab/src/hooks/useBCH.js
17

should we use "import" instead of "require"?

78

publicKeys are included in the wallet. if we are passing wallet here, we should extract the publicKeys from the wallet instead of passing them in as argument.

240

the publicKey used to encrypt the message is of type Buffer.
"fundingWif" here is a hex string. Should it be of type Buffer as well? maybe that's the reason you are getting "bad private key" message?

emack marked 3 inline comments as done.Dec 20 2021, 13:21
emack added inline comments.
web/cashtab/src/hooks/useBCH.js
17

Done

78

we're also extracting the WIF from the WIF from the wallet object in addition to the public keys, hence passing the object itself.

240

Done

emack updated this revision to Diff 31468.Dec 20 2021, 13:22
emack marked 3 inline comments as done.
emack edited the summary of this revision. (Show Details)

WIF library used to decode the private key from the wif object

emack edited the summary of this revision. (Show Details)Dec 20 2021, 13:23
emack edited the test plan for this revision. (Show Details)
Harbormaster completed remote builds in B17684: Diff 31468.Dec 20 2021, 13:27
bytesofman requested changes to this revision.Dec 20 2021, 18:49
  1. I get an error when attempting to send a longer encrypted msg

image.png (503×483 px, 33 KB)

image.png (154×279 px, 14 KB)

  1. Add some padding in between the Public/private switch and the TextArea

image.png (260×468 px, 9 KB)

web/cashtab/package.json
15 ↗(On Diff #31468)

This library is very large -- about 650kb. So this feature would about double the size of the Cashtab bundle. Is a lightweight alternative available?

Evaluate whether or not this library is unworkable:

https://www.npmjs.com/package/ecies-lite

web/cashtab/src/components/Common/Ticker.js
39 ↗(On Diff #31468)

The dev standard is for app prefixes to be 4 bytes long. While Cashtab is configured to handle arbitrary prefix length, we can't expect other apps will do this as it's not standard.

Please use 65746162 to signify "Cashtab Encrypted" (this is the usual cashtab prefix, _tab, with the _ replaced by an 'e')

This revision now requires changes to proceed.Dec 20 2021, 18:49
emack edited the summary of this revision. (Show Details)Dec 22 2021, 06:56
emack edited the test plan for this revision. (Show Details)
emack updated this revision to Diff 31525.Dec 22 2021, 06:57
emack marked 2 inline comments as done.
  • Fixed excessive API calls in Send.js by moving the BCH instantiation into useEffect(), then setting it to state for other functions to reference.
  • Replaced eccryptoJS with ecies-lite in package.json and package-lock.json to optimise bundle size. Encryption/decryption tested end to end all ok.
  • The scriptpubkey error encountered on long messages is due to the resulting output hex size. An encrypted 160 character message is a significantly longer OP_RETURN message than an unencrypted one, which pushes it over the limit. Based on threshold testing, a safe frontend limit for encrypted messages is 94 characters, as 95 and above consistently causes the error. Interim fix used here is to dynamically set the max input length to 94 chars if the encryption switch is on. Will devise a proper fix once we fork bch-api and have more control in addressing this limitation.
  • Updated encryption prefix to 65746162.
  • Added UI padding under the encryption switch
  • Expanded test plan above
Harbormaster completed remote builds in B17720: Diff 31525.Dec 22 2021, 07:01
emack updated this revision to Diff 31532.Dec 22 2021, 11:41

Fixed regression in non-OP_RETURN message sends

Harbormaster completed remote builds in B17727: Diff 31532.Dec 22 2021, 11:45
emack edited the test plan for this revision. (Show Details)Dec 22 2021, 12:03
bytesofman requested changes to this revision.Dec 22 2021, 19:32
bytesofman added inline comments.
web/cashtab/src/components/Send/Send.js
107 ↗(On Diff #31532)

If opReturnMsg is going to be its own state field now, remove this key from formData

834 ↗(On Diff #31532)

If 94 is in Ticker.js, should call it here instead of hardcoding, i.e. (max ${currency.opReturn.encrytptedMsgLimit} characters

web/cashtab/src/hooks/useBCH.js
1254 ↗(On Diff #31532)

en-encrypted ?

web/cashtab/src/utils/cashMethods.js
283 ↗(On Diff #31532)

Same structure in new library?

Is there a reference URL for more about what is going on here -- this is a weird function to appear in Cashtab.

This revision now requires changes to proceed.Dec 22 2021, 19:32
emack marked 4 inline comments as done.Dec 23 2021, 01:50
emack added inline comments.
web/cashtab/src/utils/cashMethods.js
283 ↗(On Diff #31532)

ecies-lite has the same structure as the previous eccrypto-js library because the same ECIES encryption scheme is used in both implementations. Under the Usage section of ecies-lite, note the comments on the structure of the ECIES body, it has the same structures as eccrypto-js around:

  • epk: ephemeral public key;
  • iv: initialization vector for the cipher algorithm;
  • ct: cipher text with the derived encrypt key;
  • mac: MAC value of the above fields using the derived MAC key;

In terms of what is happening within this function, the ECIES scheme needs those parameters to decrypt the data, hence this function is extracting those specific params from the encrypted message buffer.

  • "iv" is the initialization vector which could be a random buffer
  • "ephemPublicKey" is the sender's ephemeral public key buffer
  • "ciphertext" is the encrypted message buffer.
  • "mac" buffer helps to check integrity of the message (checksum)

in both eccrypto-js and ecies-lite implementations, as can be seen in this code block in useBCH.js below, they are concatenated into a single buffer as part of the encryption process.

image.png (176×681 px, 21 KB)

When it comes to decrypting in parseTxData(), this convertToEncryptStruct function then splits them out of the concatenated buffer before returning them for input into ecies-lite's decrypt function alongside the private key buffer.

emack updated this revision to Diff 31552.Dec 23 2021, 01:51
emack marked an inline comment as done.
  • removed opReturnMsg from formData
  • used app params for both encrypted (94) and unencrypted (160) char limits
  • see above for clarification on the convertToEncryptStruct function
  • rebased to latest master and merged with the existing public message alert on public messages
Harbormaster completed remote builds in B17739: Diff 31552.Dec 23 2021, 01:54
bytesofman requested changes to this revision.Dec 23 2021, 03:29
bytesofman added inline comments.
web/cashtab/src/components/Send/Send.js
307 ↗(On Diff #31552)

this param should still be called optionalOpReturnMsg

in the case of an unencrypted msg, it's weird that encryptedOpReturnMsg is the variable being used.

328 ↗(On Diff #31552)

call this isEncryptedOptionalOpReturnMsg so that it is clearly related to the optionalOpReturnMsg param

web/cashtab/src/hooks/useBCH.js
1215 ↗(On Diff #31552)

The logic in this if (encryptionFlag) block is not related to the sendXec function and should be compartmentalized in its own function, something like handleEncryptedOpReturn

Then this can be wrapped in try...catch to make error handling predictable

Will need unit tests for this function and new unit tests for the new cases of sendXec introduced by this diff

1222 ↗(On Diff #31552)

Is this the only possible error condition here? If we don't know, then need to wrap await getPublicKey in try...catch

web/cashtab/src/utils/cashMethods.js
336 ↗(On Diff #31552)

We need some more information about what this function is doing.

Your explanation in the code review comments is great for me, but this is still a very weird function a developer new to the Cashtab repo to come across. What are all the hardcoded values? If these are standards or the function is somehow boilerplate, a URL reference is okay. Otherwise the function needs more line by line commenting.

This revision now requires changes to proceed.Dec 23 2021, 03:29
emack edited the test plan for this revision. (Show Details)Dec 27 2021, 03:21
emack updated this revision to Diff 31585.Dec 27 2021, 03:56
emack marked 3 inline comments as done.
  • moved encryption handling logic into a separate handleEncryptedOpReturn function along with unit tests
  • moved the public key extraction api call into a separate getRecipientPublicKey function for future re-use along with unit tests
  • added new unit tests for sendXec for message encryption scenarios
  • Added both reference URLs and inline comments for the parsing logic in the convertToEncryptStruct function
  • adjusted various encryption variables
Harbormaster completed remote builds in B17759: Diff 31585.Dec 27 2021, 04:12
bytesofman requested changes to this revision.Dec 29 2021, 15:42

Looking good. A few more adjustments before we get this into prod:

  1. The "Only the msg recipient can view this" in history is indistinguishable from an encrypted msg of that text; it's not immediately clear to the user that this is message cannot and should not be rendered to them. Make the text red and italicized.

image.png (170×465 px, 18 KB)

  1. Because the OP_RETURN msg has been moved out of the formData object and into its own state field, the earlier diff that clears all forms on tx send is broken -- it no longer clears the OP_RETURN field.
  1. The 'no transaction history' and 'No outgoing transactions found in the recipient address" error messages that appear in notifications are not useful for non-technical users. If these errors are received from bch-api, the app should throw a better error msg that the user can parse. Say, "Cannot send an encrypted message to a wallet with no outgoing transactions" (suitable for both cases).
This revision now requires changes to proceed.Dec 29 2021, 15:42
emack edited the summary of this revision. (Show Details)Dec 30 2021, 01:48
emack edited the test plan for this revision. (Show Details)
emack updated this revision to Diff 31587.Dec 30 2021, 02:02
  • added decryptionSuccess flag in useBCH's parseTxData() so Tx.js can conditionally distinguish between a successfully decrypted message vs one that is not authorized to be rendered by the wallet. The conditional rendering logic is expanded in Tx.js for code readability.
  • fixed the post-send input clearance logic for the OP_RETURN field
  • enhanced error messages on incompatible encryption recipients
  • updated manual test plan
Harbormaster completed remote builds in B17761: Diff 31587.Dec 30 2021, 02:07
bytesofman accepted this revision.Dec 30 2021, 10:53
This revision is now accepted and ready to land.Dec 30 2021, 10:53
Closed by commit rABC101ad1f4c08f: [Cashtab] Option to send encrypted messages (authored by emack). · Explain WhyDec 30 2021, 11:06
This revision was automatically updated to reflect the committed changes.
emack added a commit: rABC101ad1f4c08f: [Cashtab] Option to send encrypted messages.

Revision Contents
Changeset List

PathSize
web/
cashtab/
81 lines
1 line
src/
components/
Common/
7 lines
Send/
23 lines
Wallet/
22 lines
__tests__/
__snapshots__/
hooks/
__mocks__/
6 lines
126 lines
7 lines
utils/
53 lines

Diff 31456

View Options

web/cashtab/package-lock.json

Loading...
View Options

web/cashtab/package.json

Loading...
View Options

web/cashtab/src/components/Common/Ticker.js

Loading...
View Options

web/cashtab/src/components/Send/Send.js

Loading...
View Options

web/cashtab/src/components/Wallet/Tx.js

Loading...
View Options

web/cashtab/src/components/Wallet/__tests__/__snapshots__/Wallet.test.js.snap

Loading...
View Options

web/cashtab/src/hooks/__mocks__/mockParsedTxs.js

Loading...
View Options

web/cashtab/src/hooks/useBCH.js

Loading...
View Options

web/cashtab/src/hooks/useWallet.js

Loading...
View Options

web/cashtab/src/utils/cashMethods.js

Loading...