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

Check consistency of note commitment and ciphertext in wallet #3897

Merged
merged 1 commit into from Mar 20, 2019

Conversation

@ebfull
Copy link
Contributor

commented Mar 19, 2019

Fixes #3896.

@defuse

This comment has been minimized.

Copy link
Contributor

commented Mar 19, 2019

utACK. Should the commitment be checked every time SproutNotePlaintext::decrypt is called? Would it make sense to modify that API so that you have to pass in the expected commitment to even get access to the plaintext? Here are the other calls:

@bitcartel

This comment has been minimized.

Copy link
Contributor

commented Mar 19, 2019

  1. FindMySproutNotes will throw an exception when it finds a note where the commitment does not match. Where is the exception caught and is it handled cleanly? Could application termination occur via a runtime error? Maybe AddToWalletIfInvolvingMe should catch when calling FindMySproutNotes and return false?

  2. Is it the correct behaviour to reject the entire transaction from being added to the wallet, in a situation where only one note, out of multiple joinsplits, is bad? (I think yes since the default client should only make valid joinsplits, but if we wanted to be fine-grained about notes we could just exclude the bad notes)

  3. GetFilteredNotes iterates over mapWallet. In the wallet, when AddToWalletIfInvolvingMe is invoked, are we sure that mapWallet does not contain/retain the txid of the bad transaction?

  4. Note that SetSproutNoteData which is invoked in AddToWalletIfInvolvingMe failed to detect the inconsistency as it relied on FindMySproutNotes. https://github.com/zcash/zcash/blob/master/src/wallet/wallet.cpp#L2113

@ebfull

This comment has been minimized.

Copy link
Contributor Author

commented Mar 19, 2019

@defuse said:

Should the commitment be checked every time SproutNotePlaintext::decrypt is called?

That's what we do in Sapling, and I agree, but I don't want to block this PR on that. This is the entry point for the wallet's acceptance of a note and so this is the minimal change needed to remediate.

@bitcartel said:

FindMySproutNotes will throw an exception when it finds a note where the commitment does not match. Where is the exception caught and is it handled cleanly? Could application termination occur via a runtime error? Maybe AddToWalletIfInvolvingMe should catch when calling FindMySproutNotes and return false?

The same exception is already thrown by the inner decryption routine when it fails to decrypt. edit: further, this is the documented behavior of the GetSproutNoteNullifier function.

Is it the correct behaviour to reject the entire transaction from being added to the wallet, in a situation where only one note, out of multiple joinsplits, is bad?

No, I don't think so. That would not be consistent with what we do in Sapling either.

GetFilteredNotes iterates over mapWallet. In the wallet, when AddToWalletIfInvolvingMe is invoked, are we sure that mapWallet does not contain/retain the txid of the bad transaction?

Again, this routine is failing via exception in the same manner that it would had it failed to decrypt the ciphertext in the first place.

Note that SetSproutNoteData which is invoked in AddToWalletIfInvolvingMe failed to detect the inconsistency as it relied on FindMySproutNotes. https://github.com/zcash/zcash/blob/master/src/wallet/wallet.cpp#L2113

Can you clarify what you mean or how that impacts this PR?

@zebambam zebambam requested review from bitcartel and str4d Mar 19, 2019

@zebambam

This comment has been minimized.

Copy link
Contributor

commented Mar 19, 2019

I know we reviewed this offline before committing publicly. Can we mark it reviewed publicly as well so that everyone can see please? @bitcartel @str4d

@bitcartel

This comment has been minimized.

Copy link
Contributor

commented Mar 19, 2019

Can you clarify what you mean or how that impacts this PR?

Doesn't impact the PR. Just pointing out that the comment If FindMySproutNotes() was used to obtain noteData, this should never happen was previously incorrect, but should now be correct.

@bitcartel bitcartel requested a review from daira Mar 19, 2019

Show resolved Hide resolved src/utiltest.cpp Outdated
Show resolved Hide resolved src/wallet/gtest/test_wallet.cpp Outdated

@str4d str4d added this to the v2.0.4 milestone Mar 19, 2019

@ebfull ebfull force-pushed the ebfull:wallet-sprout-check-commitment branch from f6300d8 to c100937 Mar 19, 2019

@ebfull

This comment has been minimized.

Copy link
Contributor Author

commented Mar 19, 2019

Addressed @str4d's review and squashed.

@zebambam zebambam requested a review from str4d Mar 19, 2019

@bitcartel bitcartel added this to Needs Prioritization in Arborist Team via automation Mar 19, 2019

@bitcartel bitcartel added the SECURITY label Mar 19, 2019

@bitcartel bitcartel moved this from Needs Prioritization to PRs That Need Review + Their Associated Issues in Arborist Team Mar 19, 2019

@bitcartel bitcartel moved this from PRs That Need Review + Their Associated Issues to In Review in Arborist Team Mar 19, 2019

@Eirik0

Eirik0 approved these changes Mar 19, 2019

Copy link
Contributor

left a comment

utACK

@str4d

str4d approved these changes Mar 19, 2019

Copy link
Contributor

left a comment

ut(ACK+cov)

// Check note plaintext against note commitment
if (note.cm() != jsdesc.commitments[n]) {
throw libzcash::note_decryption_failed();
}

This comment has been minimized.

Copy link
@str4d

str4d Mar 19, 2019

Contributor

Non-blocking: this should be moved into libzcash::SproutNotePlaintext::decrypt(), to match what we do for Sapling.

This comment has been minimized.

Copy link
@daira

daira Mar 20, 2019

Contributor

Please file another ticket for this.

@daira

daira approved these changes Mar 20, 2019

Copy link
Contributor

left a comment

ut(ACK+cov)

@ebfull

This comment has been minimized.

Copy link
Contributor Author

commented Mar 20, 2019

@zkbot r+

@zkbot

This comment has been minimized.

Copy link
Collaborator

commented Mar 20, 2019

📌 Commit c100937 has been approved by ebfull

@zkbot

This comment has been minimized.

Copy link
Collaborator

commented Mar 20, 2019

⌛️ Testing commit c100937 with merge 890c710...

zkbot added a commit that referenced this pull request Mar 20, 2019

Auto merge of #3897 - ebfull:wallet-sprout-check-commitment, r=ebfull
Check consistency of note commitment and ciphertext in wallet

Fixes #3896.
@zkbot

This comment has been minimized.

Copy link
Collaborator

commented Mar 20, 2019

💔 Test failed - pr-merge

@ebfull

This comment has been minimized.

Copy link
Contributor Author

commented Mar 20, 2019

Looks like multiple transient failures.

@zkbot retry

@zkbot

This comment has been minimized.

Copy link
Collaborator

commented Mar 20, 2019

⌛️ Testing commit c100937 with merge 6158f0a...

zkbot added a commit that referenced this pull request Mar 20, 2019

Auto merge of #3897 - ebfull:wallet-sprout-check-commitment, r=ebfull
Check consistency of note commitment and ciphertext in wallet

Fixes #3896.
@zkbot

This comment has been minimized.

Copy link
Collaborator

commented Mar 20, 2019

☀️ Test successful - pr-merge
Approved by: ebfull
Pushing 6158f0a to master...

@zkbot zkbot merged commit c100937 into zcash:master Mar 20, 2019

1 check passed

homu Test successful
Details

Arborist Team automation moved this from In Review to Released (Merged in Master) Mar 20, 2019

cronicc added a commit to ZencashOfficial/zen that referenced this pull request Apr 3, 2019

Merge pull request #158 from ZencashOfficial/AddressIndexing_development
* update zeromq dependency fixing [CVE-2019-6250](https://cve.mitre.org/cgi-bin/cvename.cgi?name=CVE-2019-6250)
* fix libsodium fall-back dl-path
* `-blockmaxcomplexity` switch, limit transactions to be included into blocks based on block complexity. Block complexity is the sum of transaction complexity per block. Transaction complexity is the number of inputs of a transaction squared. Like -mempooltxinputlimit this switch is intended as a last resort when unable to build blocks fast enough because of poor GBT performance. 0  or negative values means no limit is applied. (default: 0)
* `-deprecatedgetblocktemplate` switch, disable block complexity calculation and use the previous GetBlockTemplate implementation.
* `getblockfinalityindex` RPC method, display contextual confidence value of needed confirmations for block to be considered final, taking into account possible parallel penalized chains
* `getglobaltips` utility RPC method, display known chaintips including possible parallel penalized chains
* additions to the 51% attack prevention implementation adjusting block propagation of penalized chains
* multiple test cases added for all new features
* Backport of zcash/zcash#3897, fixes security issue https://z.cash/support/security/announcements/security-announcement-2019-03-19/ reported to Zcash by Alexis Enston, thanks to the original reporter and the Zcash team for notifying us about the issue!
* Updates to documentation addressing DNS rebinding attacks with ZMQ/AMQP, Zcash PR zcash/zcash#3890, credit to @zebambam.

cronicc added a commit to ZencashOfficial/zen that referenced this pull request Apr 3, 2019

Merge pull request #154 from ZencashOfficial/development
* update zeromq dependency fixing [CVE-2019-6250](https://cve.mitre.org/cgi-bin/cvename.cgi?name=CVE-2019-6250)
* fix libsodium fall-back dl-path
* `-blockmaxcomplexity` switch, limit transactions to be included into blocks based on block complexity. Block complexity is the sum of transaction complexity per block. Transaction complexity is the number of inputs of a transaction squared. Like -mempooltxinputlimit this switch is intended as a last resort when unable to build blocks fast enough because of poor GBT performance. 0  or negative values means no limit is applied. (default: 0)
* `-deprecatedgetblocktemplate` switch, disable block complexity calculation and use the previous GetBlockTemplate implementation.
* `getblockfinalityindex` RPC method, display contextual confidence value of needed confirmations for block to be considered final, taking into account possible parallel penalized chains
* `getglobaltips` utility RPC method, display known chaintips including possible parallel penalized chains
* additions to the 51% attack prevention implementation adjusting block propagation of penalized chains
* multiple test cases added for all new features
* Backport of zcash/zcash#3897, fixes security issue https://z.cash/support/security/announcements/security-announcement-2019-03-19/ reported to Zcash by Alexis Enston, thanks to the original reporter and the Zcash team for notifying us about the issue!
* Updates to documentation addressing DNS rebinding attacks with ZMQP/AMQP, Zcash PR zcash/zcash#3890, credit to @zebambam.

cronicc added a commit to ZencashOfficial/zen that referenced this pull request Apr 8, 2019

Merge pull request #164 from ZencashOfficial/development
* Update zeromq dependency fixing [CVE-2019-6250](https://cve.mitre.org/cgi-bin/cvename.cgi?name=CVE-2019-6250)
* Fix libsodium fall-back dl-path
* `-blockmaxcomplexity` switch, limit transactions to be included into blocks based on block complexity. Block complexity is the sum of transaction complexity per block. Transaction complexity is the number of inputs of a transaction squared. Like -mempooltxinputlimit this switch is intended as a last resort when unable to build blocks fast enough because of poor GBT performance. 0  or negative values means no limit is applied. (default: 0)
* `-deprecatedgetblocktemplate` switch, disable block complexity calculation and use the previous GetBlockTemplate implementation.
* `getblockfinalityindex` RPC method, display contextual confidence value of needed confirmations for block to be considered final, taking into account possible parallel penalized chains
* `getglobaltips` utility RPC method, display known chaintips including possible parallel penalized chains
* Additions to the 51% attack prevention implementation adjusting block propagation of penalized chains
* Multiple test cases added for all new features
* Backport of zcash/zcash#3897, fixes security issue https://z.cash/support/security/announcements/security-announcement-2019-03-19/ reported to Zcash by Alexis Enston, thanks to the original reporter and the Zcash team for notifying us about the issue!
* Updates to documentation addressing DNS rebinding attacks with ZMQ/AMQP, Zcash PR zcash/zcash#3890, credit to @zebambam.
* New mainnet and testnet checkpoint blocks
* Aria2 added to fetch-params.sh DL methods, wget resume support more robust
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.