Fix mempool DoS vulnerability from malleated transactions #8312

1. 
   sdaftuar commented at 1:48 am on July 7, 2016: member

   Fixes #8279

   In addition to the problem highlighted in that issue, there’s an additional, related problem in the sigops policy check. Because witness sigops are counted without checking that the witness program matches the commitment in the scriptPubKey being spent, it’s possible to change a transaction’s witness to cause the sigops policy check to fail, without changing the txid.

   Similarly, because the bytes-per-sigop check is affected by the size of the transaction including the witness, it’s possible to even remove a witness and cause that sigops check to fail, again without changing the txid.

   So this PR does the following:

   • Moves the IsStandard check to happen after checking for premature-witness. (This should prevent the bug reported in #8279 from possibly affecting 0.13.0 nodes, which should never accept witness transactions.)
   • Changes IsStandard to set a bool which will indicate if the transaction could be malleated, so that the caller can act appropriately.
   • Reorders the checks in IsStandard so that the size check is performed last, and sets the could-be-malleated bool if the test fails.
   • Changes the error for sigops failure to always set the could-be-malleated flag.
   • Adds tests to p2p-segwit.py to catch both scenarios.
2. MarcoFalke added the label P2P on Jul 7, 2016
3. jonasschnelli added this to the milestone 0.13.0 on Jul 7, 2016
4. 
   sdaftuar commented at 7:48 pm on July 7, 2016: member

   After more investigation, I’ve concluded that fixing this for segwit is more complicated than the couple of patches here (I will document the issues more fully in #8279). However, to fix this issue for 0.13.0, we can simply move the IsStandard() check to happen after the premature-witness check. I will update this PR shortly.

   We can separately consider the best way to fix these types of issues more generally after branching off for 0.13.

5. Fix DoS vulnerability in mempool acceptance

   Moves the IsStandard check to happen after the premature-witness check,
   so that adding a witness to a transaction can't prevent mempool acceptance.
   
   Note that this doesn't address the broader category of potential mempool DoS
   issues that affect transactions after segwit activation.

   bb66a11396
6. Test that unnecessary witnesses can't be used for mempool DoS

   Check that pre-segwit activation, unnecessary witnesses won't cause
   a txid to be permanently rejected.

   46c9620f11
7. sdaftuar force-pushed on Jul 8, 2016
8. 
   sdaftuar commented at 1:30 am on July 8, 2016: member

   Updated with a simple fix for 0.13.0.
9. 
   laanwj commented at 9:41 am on July 8, 2016: member

   Thanks for fixing this problem and adding a test, too utACK 46c9620
10. in qa/rpc-tests/p2p-segwit.py: in 46c9620f11

    68@@ -68,7 +69,7 @@ def on_pong(self, conn, message):
    69 
    70     def on_reject(self, conn, message):
    71         self.last_reject = message
    72-        #print message
    73+        #print (message)
    

    ---

    ---

    

    petertodd commented at 11:27 pm on July 9, 2016:

    Should be print(message)

    ---

    

    sipa commented at 6:06 pm on July 11, 2016:

    Or just remove.

    ---

    

    MarcoFalke commented at 6:18 pm on July 11, 2016:

    It is just a comment. Don’t consider it a blocker of anything.
11. 
    petertodd commented at 11:33 pm on July 9, 2016: contributor

    Lightly tested ACK https://github.com/bitcoin/bitcoin/pull/8312/commits/46c9620f11acfd2b528959d6cbab324105c3adef
12. 
    sipa commented at 6:07 pm on July 11, 2016: member

    utACK 46c9620f11acfd2b528959d6cbab324105c3adef
13. laanwj merged this on Jul 14, 2016
14. laanwj closed this on Jul 14, 2016

    ---

15. laanwj referenced this in commit ca40ef6029 on Jul 14, 2016
16. DrahtBot locked this on Sep 8, 2021

Contributors
sdaftuar laanwj petertodd sipa MarcoFalke

Labels
P2P

Milestone
0.13.0