Add several policy limits and disable uncompressed keys for segwit scripts #8499

jl2012 · 2016-08-11T16:24:53Z

(See #8499 (comment) : updated on 8 Oct 2016)

btcdrak · 2016-08-11T19:56:50Z

Concept ACK

jl2012 · 2016-08-12T00:50:48Z

I think this is only a temporary solution to protect some of the most common script types. It's easy for P2WPKH. For P2WSH, it could only be done on a case-by-case basis. This approach won't work for more complicated scripts like MAST

paveljanik · 2016-08-12T11:33:24Z

*segwit.py tests fail.

jl2012 · 2016-08-19T03:51:51Z

Updated to do more sanity checks:

If the public key in P2WPKH is bigger than 33 bytes, make sure it matches the witness program
Make sure the witnessScript is <= 10000 bytes and matches the witness program

sipa · 2016-08-22T10:58:04Z

src/policy/policy.cpp

+            std::vector<unsigned char> pubKey = tx.wit.vtxinwit[i].scriptWitness.stack[1];
+            if (tx.wit.vtxinwit[i].scriptWitness.stack[0].size() > 73 || pubKey.size() > 65)
+                return true;
+            if (pubKey.size() > 33) {


Why only when the size is above 33 bytes?

The main objective of this PR is to detect extra witness data in some known forms of scripts, and DoS ban if found.

The normal size for compressed key is 33. If the size is 33, we are sure that no extra data is added. Whether the provided key matches the hash or not will be tested later in script evaluation. Testing here is just duplicated work.

If the size is bigger than 33, we don't know whether it is legit (uncompressed key) or not (malleated witness). To exclude the invalid case we must compare the hash early, and DoS ban if it does not match. If we don't compare the hash here, an attacker may increase the key size up to 65 bytes, get the transaction rejected due to insufficient fee, and not get banned

Unless I'm missing something, I think this is over-eager optimization. It took me a few reads of even this explanation to understand the reasoning behind this line, and even then I'm not quite sure if I've fully understood it. Is the only harm in being more general that we are duplicating work later?

At a minimum there should be a comment something like:
"We want to make sure malleated pubkeys don't trigger too-low-fee to avoid DoS ban"

@instagibbs >Is the only harm in being more general that we are duplicating work later?
yes, because the only purpose of this PR is to early detect some kinds of witness mutation that would trigger too-low-fee ban. We don't care if the size is same as expected or too small. Otherwise we are just doing the validation twice.

But this one is probably over optimized.

I believe this is over-optimized, yes. Let's just always check it.

sipa · 2016-08-23T11:34:19Z

I don't think this belongs in policy. It detects witness transaction inputs that are unambiguously invalid by consensus rules, so I think it belongs in script/interpreter.cpp, but for that it can't depend on CCoinsView. Would it be possible to just have a function per input, that just takes the prevout, scriptsig, and scriptwitness?

jl2012 · 2016-08-23T17:44:57Z

@sipa: can I still use Solver and txnouttype after moving to interpreter.cpp? That'd would useful when we define more common script types (e.g. HTLC)

The IsBadWitness function is only a very small subset of consensus rules and it should never be used in the consensus critical path. Actually, we don't need IsBadWitness if we fully execute the script before doing any size related policy check. So it may be reasonable to consider this as part of policy.

This is more like an "assertion" for the following policy checking, rather than for consensus purpose

jl2012 · 2016-08-25T02:49:11Z

need a 0.13.1 tag?

jl2012 · 2016-09-06T09:06:51Z

The function is updated. It returns 1 for witness-related error, and 2 for P2SH-related error. We shouldn't mark corruption possible for P2SH related error.

It also always checks the HASH160 in P2WPKH

sipa · 2016-09-06T09:09:58Z

src/policy/policy.cpp

Can you define an enum instead?

sipa · 2016-09-06T12:01:54Z

utACK 056dc99bdc538a7af04b21fe3b25586afbc03861

instagibbs · 2016-09-06T17:40:03Z

src/policy/policy.cpp

Please use the constant found here: https://github.com/bitcoin/bitcoin/blob/master/src/script/script.h#L31

instagibbs · 2016-09-06T17:40:49Z

nit aside utACK 056dc99

instagibbs · 2016-10-16T16:36:06Z

Sync is failing to complete for some reason in p2p-segwit.py

utACK fixes at 26bd36ba64cd090c862ca19d64e8e73ee79184d2 with only a couple nits

jl2012 · 2016-10-16T16:36:31Z

Comments added and edited. By the way, it seems the p2p-segwit.py would fail randomly

File "./qa/rpc-tests/p2p-segwit.py", line 2000, in run_test
self.test_non_standard_witness()
File "./qa/rpc-tests/p2p-segwit.py", line 1909, in test_non_standard_witness
self.std_node.announce_tx_and_wait_for_getdata(p2sh_txs[1])
File "./qa/rpc-tests/p2p-segwit.py", line 108, in announce_tx_and_wait_for_getdata
self.wait_for_getdata(timeout)
File "./qa/rpc-tests/p2p-segwit.py", line 98, in wait_for_getdata
self.sync(test_function, timeout)
File "./qa/rpc-tests/p2p-segwit.py", line 82, in sync
raise AssertionError("Sync failed to complete")

jl2012 · 2016-10-16T19:24:07Z

I'm removing the tests that causing random sync fail. A similar test is done already to show that policy-rejection won't blind a node to the tx, so I think it's just redundant to try it again.

TheBlueMatt · 2016-10-16T21:02:00Z

utACK 67d6ee1

…for segwit scripts 67d6ee1 remove redundant tests in p2p-segwit.py (Johnson Lau) 9260085 test segwit uncompressed key fixes (Johnson Lau) 248f3a7 Fix ismine and addwitnessaddress: no uncompressed keys in segwit (Pieter Wuille) b811124 [qa] Add tests for uncompressed pubkeys in segwit (Suhas Daftuar) 9f0397a Make test framework produce lowS signatures (Johnson Lau) 4c0c25a Require compressed keys in segwit as policy and disable signing with uncompressed keys for segwit scripts (Johnson Lau) 3ade2f6 Add standard limits for P2WSH with tests (Johnson Lau)

btcdrak · 2016-10-17T11:56:16Z

utACK 67d6ee1

Github-Pull: bitcoin#8499 Rebased-From: 3ade2f6

…uncompressed keys for segwit scripts Github-Pull: bitcoin#8499 Rebased-From: 4c0c25a

Github-Pull: bitcoin#8499 Rebased-From: 9f0397a

Github-Pull: bitcoin#8499 Rebased-From: b811124

Github-Pull: bitcoin#8499 Rebased-From: 248f3a7

Github-Pull: bitcoin#8499 Rebased-From: 9260085

Github-Pull: bitcoin#8499 Rebased-From: 67d6ee1

laanwj · 2016-10-17T12:21:33Z

Backported in #8916

instagibbs · 2016-10-17T13:49:51Z

post-merge ACK laanwj@9777fe1

Previously this was an inline test where the specificity was probably judged overly specific. As a class method it makes sense to maintain consistency. And replace some magic values with their constant equivalents. Zcash: Excludes changes to the following functions we don't have: - ExtractPubKey (bitcoin/bitcoin#6415) - IsCompressedPubKey (bitcoin/bitcoin#8499)

Previously this was an inline test where the specificity was probably judged overly specific. As a class method it makes sense to maintain consistency. And replace some magic values with their constant equivalents. Excludes changes to the following functions we don't have: - ExtractPubKey (bitcoin#6415) - IsCompressedPubKey (bitcoin#8499)

…ompressed keys for segwit scripts 67d6ee1 remove redundant tests in p2p-segwit.py (Johnson Lau) 9260085 test segwit uncompressed key fixes (Johnson Lau) 248f3a7 Fix ismine and addwitnessaddress: no uncompressed keys in segwit (Pieter Wuille) b811124 [qa] Add tests for uncompressed pubkeys in segwit (Suhas Daftuar) 9f0397a Make test framework produce lowS signatures (Johnson Lau) 4c0c25a Require compressed keys in segwit as policy and disable signing with uncompressed keys for segwit scripts (Johnson Lau) 3ade2f6 Add standard limits for P2WSH with tests (Johnson Lau)

jl2012 force-pushed the badwitnesscheck branch 2 times, most recently from 2150139 to 1c75a39 Compare August 19, 2016 03:49

jl2012 force-pushed the badwitnesscheck branch 4 times, most recently from abb2085 to 47335e4 Compare August 19, 2016 22:04

sipa reviewed Aug 22, 2016
View reviewed changes

jl2012 force-pushed the badwitnesscheck branch from 9065f89 to 27294a2 Compare August 24, 2016 09:16

jl2012 changed the title ~~[Untested] Check bad witness~~ [WIP] Check bad witness Aug 24, 2016

jl2012 changed the title ~~[WIP] Check bad witness~~ [rpc-tests needed] Check bad witness Aug 25, 2016

jl2012 mentioned this pull request Aug 25, 2016

Verify all incoming txs unless too big or too much hashing #8593

Closed

laanwj added this to the 0.13.1 milestone Sep 1, 2016

jl2012 force-pushed the badwitnesscheck branch from 27294a2 to cf802ff Compare September 6, 2016 08:44

sipa reviewed Sep 6, 2016
View reviewed changes

jl2012 force-pushed the badwitnesscheck branch 2 times, most recently from 0bc4296 to 79a2248 Compare September 6, 2016 10:27

instagibbs reviewed Sep 6, 2016
View reviewed changes

jl2012 force-pushed the badwitnesscheck branch from 79a2248 to 08f5e22 Compare September 8, 2016 08:21

remove redundant tests in p2p-segwit.py

67d6ee1

laanwj merged commit 67d6ee1 into bitcoin:master Oct 17, 2016

laanwj pushed a commit to laanwj/bitcoin that referenced this pull request Oct 17, 2016

Add standard limits for P2WSH with tests

540413d

Github-Pull: bitcoin#8499 Rebased-From: 3ade2f6

laanwj pushed a commit to laanwj/bitcoin that referenced this pull request Oct 17, 2016

Require compressed keys in segwit as policy and disable signing with …

821f3e6

…uncompressed keys for segwit scripts Github-Pull: bitcoin#8499 Rebased-From: 4c0c25a

laanwj pushed a commit to laanwj/bitcoin that referenced this pull request Oct 17, 2016

Make test framework produce lowS signatures

b4b8527

Github-Pull: bitcoin#8499 Rebased-From: 9f0397a

laanwj pushed a commit to laanwj/bitcoin that referenced this pull request Oct 17, 2016

[qa] Add tests for uncompressed pubkeys in segwit

908fced

Github-Pull: bitcoin#8499 Rebased-From: b811124

laanwj pushed a commit to laanwj/bitcoin that referenced this pull request Oct 17, 2016

Fix ismine and addwitnessaddress: no uncompressed keys in segwit

4ec21e8

Github-Pull: bitcoin#8499 Rebased-From: 248f3a7

laanwj pushed a commit to laanwj/bitcoin that referenced this pull request Oct 17, 2016

test segwit uncompressed key fixes

fef7b46

Github-Pull: bitcoin#8499 Rebased-From: 9260085

laanwj pushed a commit to laanwj/bitcoin that referenced this pull request Oct 17, 2016

remove redundant tests in p2p-segwit.py

9777fe1

Github-Pull: bitcoin#8499 Rebased-From: 67d6ee1

laanwj removed the Needs backport label Oct 17, 2016

laanwj mentioned this pull request Oct 17, 2016

0.13.1 backports #8916

Merged

meyer9 mentioned this pull request Apr 22, 2018

Merge wallet changes for Segwit phoreproject/Phore#27

Closed

dagurval mentioned this pull request Sep 26, 2019

Make test framework produce lowS signatures BitcoinUnlimited/BitcoinUnlimited#1947

Merged

bitcoin locked as resolved and limited conversation to collaborators Sep 8, 2021

Add several policy limits and disable uncompressed keys for segwit scripts #8499

Add several policy limits and disable uncompressed keys for segwit scripts #8499

Uh oh!

Conversation

jl2012 commented Aug 11, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

btcdrak commented Aug 11, 2016

Uh oh!

jl2012 commented Aug 12, 2016

Uh oh!

paveljanik commented Aug 12, 2016

Uh oh!

jl2012 commented Aug 19, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sipa commented Aug 23, 2016

Uh oh!

jl2012 commented Aug 23, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jl2012 commented Aug 25, 2016

Uh oh!

jl2012 commented Sep 6, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sipa commented Sep 6, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

instagibbs commented Sep 6, 2016

Uh oh!

instagibbs commented Oct 16, 2016

Uh oh!

jl2012 commented Oct 16, 2016

Uh oh!

jl2012 commented Oct 16, 2016

Uh oh!

TheBlueMatt commented Oct 16, 2016

Uh oh!

btcdrak commented Oct 17, 2016

Uh oh!

laanwj commented Oct 17, 2016

Uh oh!

instagibbs commented Oct 17, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

17 participants

jl2012 commented Aug 11, 2016 •

edited

Loading

jl2012 commented Aug 23, 2016 •

edited

Loading