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

Fix mempool DoS vulnerability from malleated transactions #8312

Merged
merged 2 commits into from Jul 14, 2016

Conversation

Projects
None yet
6 participants
@sdaftuar
Member

sdaftuar commented Jul 7, 2016

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.
@sdaftuar

This comment has been minimized.

Show comment
Hide comment
@sdaftuar

sdaftuar Jul 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.

Member

sdaftuar commented Jul 7, 2016

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.

sdaftuar added some commits Jul 7, 2016

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.
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.
@sdaftuar

This comment has been minimized.

Show comment
Hide comment
@sdaftuar

sdaftuar Jul 8, 2016

Member

Updated with a simple fix for 0.13.0.

Member

sdaftuar commented Jul 8, 2016

Updated with a simple fix for 0.13.0.

@laanwj

This comment has been minimized.

Show comment
Hide comment
@laanwj

laanwj Jul 8, 2016

Member

Thanks for fixing this problem and adding a test, too
utACK 46c9620

Member

laanwj commented Jul 8, 2016

Thanks for fixing this problem and adding a test, too
utACK 46c9620

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

This comment has been minimized.

@petertodd

petertodd Jul 9, 2016

Contributor

Should be print(message)

@petertodd

petertodd Jul 9, 2016

Contributor

Should be print(message)

This comment has been minimized.

@sipa

sipa Jul 11, 2016

Member

Or just remove.

@sipa

sipa Jul 11, 2016

Member

Or just remove.

This comment has been minimized.

@MarcoFalke

MarcoFalke Jul 11, 2016

Member

It is just a comment. Don't consider it a blocker of anything.

@MarcoFalke

MarcoFalke Jul 11, 2016

Member

It is just a comment. Don't consider it a blocker of anything.

@petertodd

This comment has been minimized.

Show comment
Hide comment
@petertodd

petertodd Jul 9, 2016

Contributor

Lightly tested ACK 46c9620

Contributor

petertodd commented Jul 9, 2016

Lightly tested ACK 46c9620

@sipa

This comment has been minimized.

Show comment
Hide comment
@sipa

sipa Jul 11, 2016

Member

utACK 46c9620

Member

sipa commented Jul 11, 2016

utACK 46c9620

@laanwj laanwj merged commit 46c9620 into bitcoin:master Jul 14, 2016

1 check passed

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

laanwj added a commit that referenced this pull request Jul 14, 2016

Merge #8312: Fix mempool DoS vulnerability from malleated transactions
46c9620 Test that unnecessary witnesses can't be used for mempool DoS (Suhas Daftuar)
bb66a11 Fix DoS vulnerability in mempool acceptance (Suhas Daftuar)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment