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

rpc: have verifytxoutproof check the number of txns in proof structure #13452

Merged
merged 2 commits into from Jul 9, 2018

Conversation

Projects
None yet
9 participants
@instagibbs
Copy link
Member

instagibbs commented Jun 12, 2018

Recent publication of a weakness in Bitcoin's merkle tree construction demonstrates many SPV applications vulnerable to an expensive to pull off yet still plausible attack: https://bitslog.wordpress.com/2018/06/09/leaf-node-weakness-in-bitcoin-merkle-tree-design/

This change would at least allow verifytxoutproof to properly validate that the proof matches a known block, with known number of transactions any time after the full block is processed. This should neuter the attack entirely.

The negative is that a header-only processed block/future syncing mode would cause this to fail until the node has imported the data required.

related: #13451

importprunedfunds needs this check as well. Can expand it to cover this if people like the idea.

@MarcoFalke

This comment has been minimized.

Copy link
Member

MarcoFalke commented Jun 12, 2018

utACK 614fd8d. Looks like a nice belt-and-suspenders with no strings attached: I think it is reasonable for a node to fail here, in case it hasn't downloaded the transactions (and thus doesn't know the number of transactions).

@MarcoFalke MarcoFalke changed the title have verifytxoutproof check the number of txns in proof structure rpc: have verifytxoutproof check the number of txns in proof structure Jun 12, 2018

@luke-jr

This comment has been minimized.

Copy link
Member

luke-jr commented Jun 12, 2018

utACK

@achow101

This comment has been minimized.

Copy link
Member

achow101 commented Jun 12, 2018

utACK 614fd8d

@MarcoFalke MarcoFalke added this to the 0.16.2 milestone Jun 13, 2018

@@ -328,6 +328,10 @@ static UniValue verifytxoutproof(const JSONRPCRequest& request)
throw JSONRPCError(RPC_INVALID_ADDRESS_OR_KEY, "Block not found in chain");
}

if (pindex->nTx != merkleBlock.txn.GetNumTransactions()) {

This comment has been minimized.

@sipa

sipa Jun 13, 2018

Member

Is it right to throw an RPC error here? This is more similar to being given an invalid proof (as opposed to an incorrectly formatted/unusable one), in which case Null is returned currently.

This comment has been minimized.

@instagibbs

instagibbs Jun 13, 2018

Member

deja vu: #11120 (comment)

(I can also just have it clear the return list)

This comment has been minimized.

@sipa

sipa Jun 13, 2018

Member

Yes, I feel there is a big difference between not having the block (which means being unable to validate) and being given an intentionally fraudulent proof.

Requiring software to deal and match RPC errors instead of being covered by dealing with the existing "invalid proof" case sounds much nicer.

I also think you need to check for nTx = 0 by the way (as it may mean a block for which we only have headers etc), and treat that as not having the block.

This comment has been minimized.

@luke-jr

luke-jr Jun 16, 2018

Member

@sipa Is it well-defined that a headers-only blockindex will have nTx==0? The comments say nTx simply is unreliable in that case...

@instagibbs instagibbs force-pushed the instagibbs:actuallyverifytxoutproof branch from 614fd8d to ed82f17 Jun 14, 2018

@instagibbs

This comment has been minimized.

Copy link
Member

instagibbs commented Jun 14, 2018

took @sipa suggestions

@bitcoin bitcoin deleted a comment from DrahtBot Jun 15, 2018

@luke-jr

This comment has been minimized.

Copy link
Member

luke-jr commented Jun 16, 2018

I think this actually breaks in pruned mode, since nTx gets set to 0. :/

( Actually, I can't find where pruning resets nTx... just asserts and comments implying it must. :| )

@sipa

This comment has been minimized.

Copy link
Member

sipa commented Jun 16, 2018

@luke-jr Really? That would be unfortunate. I assumed that nTX == 0 was only the case if we never accepted the full block.

@luke-jr

This comment has been minimized.

Copy link
Member

luke-jr commented Jun 16, 2018

I'm not sure on the actual behaviour right now, since I can't find where it's zeroed. It may be that the asserts are an unrelated bug (but I'd expect we'd have noticed if that were the case).

@sipa

This comment has been minimized.

Copy link
Member

sipa commented Jun 16, 2018

Can you point to the asserts?

@luke-jr

This comment has been minimized.

Copy link
Member

luke-jr commented Jun 16, 2018

Nevermind, looks like there's only one left in master, and it's wrapped under a !fHavePruned check.

@instagibbs

This comment has been minimized.

Copy link
Member

instagibbs commented Jun 18, 2018

I think this actually breaks in pruned mode, since nTx gets set to 0. :/

This is hopefully not the case, because I have a merged PR with tests that pass #13451

@sipa

This comment has been minimized.

Copy link
Member

sipa commented Jun 22, 2018

utACK ed82f17

@sdaftuar
Copy link
Member

sdaftuar left a comment

ACK apart from the nits I left.

I also wrote up a test that exercises the logic, if you feel like including that with this patch: sdaftuar@e6db405

@@ -324,12 +324,17 @@ static UniValue verifytxoutproof(const JSONRPCRequest& request)
LOCK(cs_main);

const CBlockIndex* pindex = LookupBlockIndex(merkleBlock.header.GetHash());
if (!pindex || !chainActive.Contains(pindex)) {
if (!pindex || !chainActive.Contains(pindex) || pindex->nTx == 0) {

This comment has been minimized.

@sdaftuar

sdaftuar Jun 22, 2018

Member

pindex->nTx == 0 implies !chainActive.Contains(pindex), so I think we can eliminate that condition. You can't be in chainactive if you never received the transactions for a block. Alternatively, we could add a comment for future readers that explains that this is redundant.

for (const uint256& hash : vMatch)
res.push_back(hash.GetHex());
// Check if proof is valid, only add results if so
if (pindex->nTx == merkleBlock.txn.GetNumTransactions()) {

This comment has been minimized.

@sdaftuar

sdaftuar Jun 22, 2018

Member

nit: I think if the proof lies about the number of transactions, we should throw an error, rather than just return an empty list, but I don't feel strongly about the API if you like it this way.

@@ -307,7 +307,7 @@ static UniValue verifytxoutproof(const JSONRPCRequest& request)
"\nArguments:\n"
"1. \"proof\" (string, required) The hex-encoded proof generated by gettxoutproof\n"
"\nResult:\n"
"[\"txid\"] (array, strings) The txid(s) which the proof commits to, or empty array if the proof is invalid\n"
"[\"txid\"] (array, strings) The txid(s) which the proof commits to, or empty array if the proof can not be validated.\n"

This comment has been minimized.

@sdaftuar

sdaftuar Jun 22, 2018

Member

I think we should leave the language here unchanged? If we can't validate the proof because the number of transactions differs from what we stored in our blockindex, then the proof is invalid!

@instagibbs

This comment has been minimized.

Copy link
Member

instagibbs commented Jun 22, 2018

cherry-picked @sdaftuar test, thanks!

@DrahtBot

This comment has been minimized.

Copy link
Contributor

DrahtBot commented Jun 26, 2018

Note to reviewers: This pull request conflicts with the following ones:
  • #13054 (tests: Enable automatic detection of undefined names in Python tests scripts. Remove wildcard imports. by practicalswift)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

@laanwj

This comment has been minimized.

Copy link
Member

laanwj commented Jul 9, 2018

utACK d280617

@laanwj laanwj merged commit d280617 into bitcoin:master Jul 9, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

laanwj added a commit that referenced this pull request Jul 9, 2018

Merge #13452: rpc: have verifytxoutproof check the number of txns in …
…proof structure

d280617 [qa] Add a test for merkle proof malleation (Suhas Daftuar)
ed82f17 have verifytxoutproof check the number of txns in proof structure (Gregory Sanders)

Pull request description:

  Recent publication of a weakness in Bitcoin's merkle tree construction demonstrates many SPV applications vulnerable to an expensive to pull off yet still plausible attack: https://bitslog.wordpress.com/2018/06/09/leaf-node-weakness-in-bitcoin-merkle-tree-design/

  This change would at least allow `verifytxoutproof` to properly validate that the proof matches a known block, with known number of transactions any time after the full block is processed. This should neuter the attack entirely.

  The negative is that a header-only processed block/future syncing mode would cause this to fail until the node has imported the data required.

  related: #13451

  `importprunedfunds` needs this check as well. Can expand it to cover this if people like the idea.

Tree-SHA512: 0682ec2b622a38b29f3f635323e0a8b6fc071e8a6fd134c954579926ee7b516e642966bafa667016744ce49c16e19b24dbc8801f982a36ad0a6a4aff6d93f82b

MarcoFalke added a commit to MarcoFalke/bitcoin that referenced this pull request Jul 13, 2018

MarcoFalke added a commit to MarcoFalke/bitcoin that referenced this pull request Jul 13, 2018

MarcoFalke added a commit to MarcoFalke/bitcoin that referenced this pull request Jul 13, 2018

MarcoFalke added a commit to MarcoFalke/bitcoin that referenced this pull request Jul 13, 2018

@fanquake

This comment has been minimized.

Copy link
Member

fanquake commented Jul 13, 2018

Should be backported in #13644.

luke-jr added a commit to bitcoinknots/bitcoin that referenced this pull request Jul 18, 2018

luke-jr added a commit to bitcoinknots/bitcoin that referenced this pull request Jul 18, 2018

HashUnlimited pushed a commit to chaincoin/chaincoin that referenced this pull request Jan 11, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment