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

pull instagibbs wants to merge 2 commits into bitcoin:master from instagibbs:actuallyverifytxoutproof changing 4 files +84 −4
  1. instagibbs commented at 9:16 pm on June 12, 2018: member

    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.

  2. MarcoFalke added the label RPC/REST/ZMQ on Jun 12, 2018
  3. MarcoFalke commented at 10:20 pm on June 12, 2018: member
    utACK 614fd8db3259b01e47d7b551ac389bd3137c8c5a. 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).
  4. MarcoFalke renamed this:
    have verifytxoutproof check the number of txns in proof structure
    rpc: have verifytxoutproof check the number of txns in proof structure
    on Jun 12, 2018
  5. luke-jr commented at 10:46 pm on June 12, 2018: member
    utACK
  6. achow101 commented at 11:15 pm on June 12, 2018: member
    utACK 614fd8db3259b01e47d7b551ac389bd3137c8c5a
  7. MarcoFalke added this to the milestone 0.16.2 on Jun 13, 2018
  8. MarcoFalke added the label Needs backport on Jun 13, 2018
  9. in src/rpc/rawtransaction.cpp:331 in 614fd8db32 outdated 
    327@@ -328,6 +328,10 @@ static UniValue verifytxoutproof(const JSONRPCRequest& request)
328         throw JSONRPCError(RPC_INVALID_ADDRESS_OR_KEY, "Block not found in chain");
329     }
330 
331+    if (pindex->nTx != merkleBlock.txn.GetNumTransactions()) {
    sipa commented at 6:34 pm on June 13, 2018:
    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.
    instagibbs commented at 6:43 pm on June 13, 2018:

    deja vu: #11120 (review)

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

    sipa commented at 6:49 pm on June 13, 2018:

    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.

    luke-jr commented at 4:16 am on June 16, 2018:
    @sipa Is it well-defined that a headers-only blockindex will have nTx==0? The comments say nTx simply is unreliable in that case…
  10. have verifytxoutproof check the number of txns in proof structure ed82f17000
  11. instagibbs force-pushed on Jun 14, 2018
  12. instagibbs commented at 1:55 pm on June 14, 2018: member
    took @sipa suggestions
  13. MarcoFalke deleted a comment on Jun 15, 2018
  14. luke-jr commented at 4:22 am on June 16, 2018: member

    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. :| )

  15. sipa commented at 4:26 am on June 16, 2018: member
    @luke-jr Really? That would be unfortunate. I assumed that nTX == 0 was only the case if we never accepted the full block.
  16. luke-jr commented at 4:32 am on June 16, 2018: member
    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).
  17. sipa commented at 4:40 am on June 16, 2018: member
    Can you point to the asserts?
  18. luke-jr commented at 4:15 pm on June 16, 2018: member
    Nevermind, looks like there’s only one left in master, and it’s wrapped under a !fHavePruned check.
  19. instagibbs commented at 1:51 pm on June 18, 2018: member

    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 https://github.com/bitcoin/bitcoin/pull/13451

  20. in src/rpc/rawtransaction.cpp:327 in ed82f17000 outdated 
    323@@ -324,12 +324,17 @@ static UniValue verifytxoutproof(const JSONRPCRequest& request)
324     LOCK(cs_main);
325 
326     const CBlockIndex* pindex = LookupBlockIndex(merkleBlock.header.GetHash());
327-    if (!pindex || !chainActive.Contains(pindex)) {
328+    if (!pindex || !chainActive.Contains(pindex) || pindex->nTx == 0) {
    sdaftuar commented at 8:28 pm on June 21, 2018:
    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.
  21. in src/rpc/rawtransaction.cpp:332 in ed82f17000 outdated 
    330     }
331 
332-    for (const uint256& hash : vMatch)
333-        res.push_back(hash.GetHex());
334+    // Check if proof is valid, only add results if so
335+    if (pindex->nTx == merkleBlock.txn.GetNumTransactions()) {
    sdaftuar commented at 8:30 pm on June 21, 2018:
    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.
  22. in src/rpc/rawtransaction.cpp:310 in ed82f17000 outdated 
    306@@ -307,7 +307,7 @@ static UniValue verifytxoutproof(const JSONRPCRequest& request)
307             "\nArguments:\n"
308             "1. \"proof\"    (string, required) The hex-encoded proof generated by gettxoutproof\n"
309             "\nResult:\n"
310-            "[\"txid\"]      (array, strings) The txid(s) which the proof commits to, or empty array if the proof is invalid\n"
311+            "[\"txid\"]      (array, strings) The txid(s) which the proof commits to, or empty array if the proof can not be validated.\n"
    sdaftuar commented at 8:32 pm on June 21, 2018:
    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!
  23. sipa commented at 2:19 am on June 22, 2018: member
    utACK ed82f1700006830b6fe34572b66245c1487ccd29
  24. sdaftuar approved
  25. sdaftuar commented at 9:55 am on June 22, 2018: member

    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: https://github.com/sdaftuar/bitcoin/commit/e6db4054e09abaca49f86e39bddce298f44faf79

  26. [qa] Add a test for merkle proof malleation d280617bf5
  27. instagibbs commented at 2:42 pm on June 22, 2018: member
    cherry-picked @sdaftuar test, thanks!
  28. DrahtBot commented at 11:24 am on June 26, 2018: member
    • #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.

  29. laanwj commented at 6:25 pm on July 9, 2018: member
    utACK d280617bf569f84457eaea546541dc74c67cd1e4
  30. laanwj merged this on Jul 9, 2018
  31. laanwj closed this on Jul 9, 2018
  32. laanwj referenced this in commit 7e74c54fed on Jul 9, 2018
  33. MarcoFalke referenced this in commit 56aad652c2 on Jul 13, 2018
  34. MarcoFalke referenced this in commit 21966b47af on Jul 13, 2018
  35. MarcoFalke referenced this in commit 6b9dc8ceae on Jul 13, 2018
  36. MarcoFalke referenced this in commit b72c0bd4c9 on Jul 13, 2018
  37. fanquake removed the label Needs backport on Jul 13, 2018
  38. fanquake commented at 11:57 pm on July 13, 2018: member
    Should be backported in #13644.
  39. luke-jr referenced this in commit c3f8bdceec on Jul 18, 2018
  40. luke-jr referenced this in commit 2e2fd7e75a on Jul 18, 2018
  41. sipa added the label Needs release notes on Aug 14, 2018
  42. HashUnlimited referenced this in commit f8924bacbf on Jan 11, 2019
  43. fanquake removed the label Needs release note on Mar 20, 2019
  44. PastaPastaPasta referenced this in commit 300572e4f1 on Dec 16, 2020
  45. PastaPastaPasta referenced this in commit 46b08dada0 on Dec 18, 2020
  46. PastaPastaPasta referenced this in commit 0da4f596bb on Dec 18, 2020
  47. DrahtBot locked this on Dec 16, 2021


instagibbs MarcoFalke luke-jr achow101 sipa sdaftuar DrahtBot laanwj fanquake

Labels
RPC/REST/ZMQ

Milestone
0.16.2

github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin/bitcoin. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2024-11-17 12:12 UTC
This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me