qa: Add explicit references to related CVE’s in p2p_invalid_block test #14696

pull lucash-dev wants to merge 2 commits into bitcoin:master from lucash-dev:mention-cve-invalid-block changing 9 files +104 −17
  1. lucash-dev commented at 5:55 am on November 9, 2018: contributor
    This functional test includes two scenarios that test for regressions of vulnerabilities, but they are only briefly described. There are freely available documents explaining in detail the issues, but without explicit mentions, the developer trying to maintain the code needs an additional step of digging in commit history and PR conversations to figure it out. Added comments to explicitly mention CVE-2018-17144 and CVE-2012-2459, for more complete documentation. This improves developer experience by making understanding the tests easier.
  2. fanquake added the label Tests on Nov 9, 2018
  3. ken2812221 commented at 9:31 am on November 9, 2018: contributor
    Concept ACK.
  4. practicalswift commented at 1:58 pm on November 9, 2018: contributor

    Concept ACK

    Good idea! @lucash-dev Would you be able to match other CVE:s against tests and include them in this PR? That would be really useful to have. It would be nice to be able to do git grep CVE-N on all issued Bitcoin Core CVE:s to be able to find the corresponding test case and/or fix

    Perhaps Bitcoin CVE guru @luke-jr can help in this matching? :-)

  5. lucash-dev commented at 3:49 pm on November 9, 2018: contributor
    I’ll do that. Sounds like a good learning opportunity too.
  6. MarcoFalke added the label Docs on Nov 9, 2018
  7. MarcoFalke renamed this:
    trivial: Add explicit references to related CVE's in p2p_invalid_block test.
    doc: Add explicit references to related CVE's in p2p_invalid_block test.
    on Nov 9, 2018
  8. in test/functional/p2p_invalid_block.py:60 in 283235e4b0 outdated 
    52@@ -53,10 +53,11 @@ def run_test(self):
53         block_time = best_block["time"] + 1
54 
55         # Use merkle-root malleability to generate an invalid block with
56-        # same blockheader.
57+        # same blockheader (CVE-2012-2459).
58         # Manufacture a block with 3 transactions (coinbase, spend of prior
59         # coinbase, spend of that spend).  Duplicate the 3rd transaction to
60         # leave merkle root and blockheader unchanged but invalidate the block.
61+        # For more information on merkle-root malleability see src/consesus/merkle.cpp.
    practicalswift commented at 5:39 pm on November 9, 2018:
    Should be “consensus” :-)
  9. in test/functional/p2p_invalid_block.py:85 in 283235e4b0 outdated 
    81@@ -81,7 +82,7 @@ def run_test(self):
82 
83         node.p2p.send_blocks_and_test([block2], node, success=False, request_block=False, reject_reason='bad-txns-duplicate')
84 
85-        # Check transactions for duplicate inputs
86+        # Check transactions for duplicate inputs (CVE-2018-17144)
    MarcoFalke commented at 9:06 pm on November 9, 2018:
    This current test case is not complete to check both aspects of the CVE. Would be great if you added the test case for the money printing part of the CVE.
    lucash-dev commented at 9:50 pm on November 9, 2018:
    I noticed that, when I was studying this source. TBH I think the test for the other CVE isn’t complete either, as it only checks that the mutated block is rejected, not that the valid block (with same merkle root) is accepted after that. I’ll add fixes for both.
    lucash-dev commented at 3:57 am on November 10, 2018:
    Added it in a new commit.
  10. MarcoFalke renamed this:
    doc: Add explicit references to related CVE's in p2p_invalid_block test.
    qa: Add explicit references to related CVE's in p2p_invalid_block test.
    on Nov 10, 2018
  11. Empact commented at 10:29 am on November 13, 2018: member
    Concept ACK
  12. DrahtBot commented at 3:22 pm on November 13, 2018: member

    The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

    Conflicts

    Reviewers, this pull request conflicts with the following ones:

    • #15045 ([test] Apply maximal flags to tx_valid tests and minimal flags to tx_invalid tests by jl2012)

    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.

  13. DrahtBot added the label Needs rebase on Nov 13, 2018
  14. DrahtBot removed the label Needs rebase on Nov 21, 2018
  15. MarcoFalke commented at 6:07 pm on November 22, 2018: member
    Needs rebase (silent conflict)
  16. laanwj commented at 12:04 pm on November 23, 2018: member
    Thanks for adding these tests! Concept ACK
  17. lucash-dev commented at 6:52 am on December 3, 2018: contributor
    Coudn’t find much info on CVE-2012-4682, CVE-2012-4683. @luke-jr could you provide any pointers?
  18. DrahtBot added the label Needs rebase on Jan 3, 2019
  19. lucash-dev commented at 6:47 am on January 7, 2019: contributor
    Rebased. Also refactored to use the new invalid tx template.
  20. fanquake commented at 6:54 am on January 7, 2019: member

    @lucash-dev Could you cleanup the trailing whitespace. That’s what’s causing Travis to fail:

    0./test/functional/data/invalid_txs.py:204:1: W293 blank line contains whitespace
1./test/functional/data/invalid_txs.py:205:1: W293 blank line contains whitespace
2./test/functional/data/invalid_txs.py:210:10: E131 continuation line unaligned for hanging indent
  21. fanquake removed the label Needs rebase on Jan 7, 2019
  22. laanwj commented at 1:42 pm on January 31, 2019: member
    utACK a342ecd20afc422ece488dca43dd9086dd37798b Could squash some commits and fix spelling in commit messages
  23. lucash-dev commented at 1:54 am on February 1, 2019: contributor

    utACK a342ecd Could squash some commits and fix spelling in commit messages

    Sorry, should’ve prefixed the PR with “WIP” while I was working adding more comments for more CVEs. It’s been a while since I’ve had time to work on this PR, though. It has also already grown far beyond the original intention (just a couple comments). So probably better if I just fix the commit history, and open a new PR later on when I manage to add comments/tests for the remaining CVEs. Thanks for reviewing!

  24. lucash-dev commented at 5:43 am on February 1, 2019: contributor
    Fixed the commit history.
  25. in test/functional/data/invalid_txs.py:205 in 203c3284ee outdated 
    200+        tx.vin.append(vin)
201+        tx.vout.append(CTxOut(1, basic_p2sh))
202+        tx.calc_sha256()
203+        return tx
204+
205+
    practicalswift commented at 7:57 pm on February 1, 2019:
    Drop one of the newlines here :-)
    lucash-dev commented at 6:44 pm on February 2, 2019:
    :man_facepalming:
  26. practicalswift commented at 7:57 pm on February 1, 2019: contributor
    utACK 203c3284eec263ac94339779e36f20a32c77f877 modulo nit and squash
  27. lucash-dev commented at 6:58 pm on February 2, 2019: contributor

    utACK 203c328 modulo nit and squash

    Fixed nit. Further squashed to 2 commits. one for adding comments, other for new tests. Let me know if you’d like just one commit. just thought it would make sense to separate these two, but it’s all the same to me. And thanks for reviewing!

  28. DrahtBot added the label Needs rebase on Apr 10, 2019
  29. Added comments referencing multiple CVEs in tests and production code. 
    This commit adds comments referencing multiple CVEs both in production and test code.
CVEs covered in this commit:

CVE-2010-5137
CVE-2010-5139
CVE-2010-5141
CVE-2012-1909
CVE-2012-2459
CVE-2012-3789
CVE-2018-17144
    38bfca6bb2
  30. New regression testing for CVE-2018-17144, CVE-2012-2459, and CVE-2010-5137. 
    CVE-2018-17144 and CVE-2012-2459 are only partially tested for regression.
- CVE-2018-17144 is not tested for the inflation bug.
- CVE-2012-2459 is only tested for the mutated block being rejected, not
for the original block being accepted afterwards.

This commit fixes that limitation.

Also added functional test for CVE-2010-5137.
    0c62e3aa73
  31. DrahtBot removed the label Needs rebase on Jun 2, 2019
  32. lucash-dev commented at 0:08 am on June 4, 2019: contributor
    Rebased.
  34. laanwj commented at 1:53 pm on September 18, 2019: member
    ACK 0c62e3aa73839e97e65a3155e06a98d84b700a1e, checked the CVE numbers, thanks for adding documentation
  35. laanwj renamed this:
    qa: Add explicit references to related CVE's in p2p_invalid_block test.
    qa: Add explicit references to related CVE's in p2p_invalid_block test
    on Sep 18, 2019
  36. laanwj referenced this in commit feb162d500 on Sep 18, 2019
  37. laanwj merged this on Sep 18, 2019
  38. laanwj closed this on Sep 18, 2019
  39. sidhujag referenced this in commit adb509b68a on Sep 23, 2019
  40. jasonbcox referenced this in commit 6d3edf189e on Oct 24, 2020
  41. jasonbcox referenced this in commit 41e566cae1 on Oct 24, 2020
  42. random-zebra referenced this in commit 2ab948e1ad on May 4, 2021
  43. vijaydasmp referenced this in commit 4c6baadf32 on Dec 6, 2021
  44. Munkybooty referenced this in commit 271957d60d on Dec 7, 2021
  45. vijaydasmp referenced this in commit 91ec735a0f on Dec 10, 2021
  46. vijaydasmp referenced this in commit 6fca1741db on Dec 13, 2021
  47. vijaydasmp referenced this in commit c86af5d217 on Dec 13, 2021
  48. vijaydasmp referenced this in commit 452d182739 on Dec 15, 2021
  49. Munkybooty referenced this in commit d503c9d1e7 on Dec 15, 2021
  50. Munkybooty referenced this in commit d590365a64 on Dec 15, 2021
  51. DrahtBot locked this on Dec 16, 2021


lucash-dev ken2812221 practicalswift MarcoFalke Empact DrahtBot laanwj fanquake

Labels
Docs Tests

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-12-26 09:12 UTC
This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me