Add policy: null signature for failed CHECK(MULTI)SIG #8634

pull jl2012 wants to merge 1 commits into bitcoin:master from jl2012:nullfail changing 8 files +55 −2

jl2012 commented at 5:34 AM on August 31, 2016: contributor

Intended for 0.13.1

This PR adds a new policy to require that signature(s) must be empty vector for failed CHECK(MULTI)SIG operations.

This fixes malleability for some forms of scripts, for example:

"key 1" CHECKSIG IF "key 2" CHECKSIG ELSE "key 3" CHECKSIGVERIFY "locktime" CLTV ENDIF

which is spendable with both sig 1 and sig 2 anytime, or sig 3 only after locktime

Without this new policy, a relay node may inject arbitrary BIP66-compliant data to the scriptSig/witness when the ELSE branch is executed.

This may become a softfork in the future. I will prepare for a BIP later
fanquake added the label TX fees and policy on Aug 31, 2016
jl2012 cross-referenced this on Sep 1, 2016 from issue Implement LOW_S and NULLDUMMY softfork (BIP146) by jl2012
jl2012 commented at 1:41 AM on September 2, 2016: contributor

Please tag 0.13.1
fanquake added this to the milestone 0.13.1 on Sep 2, 2016
btcdrak commented at 11:00 AM on September 2, 2016: contributor

Concept ACK
jl2012 commented at 12:32 PM on September 6, 2016: contributor

This is an implementation of NULLFAIL of BIP146, except the activation logic:

https://github.com/bitcoin/bips/blob/master/bip-0146.mediawiki#NULLFAIL

in src/test/data/script_tests.json:None in bb50e7683d outdated

2150 | +["NULLFAIL should cover all signatures and signatures only"],
2151 | +["0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0", "0x01 0x14 1 1 1 1 1 1 1 1 1 1 1 1 1 1 1 1 1 1 1 1 0x01 0x14 CHECKMULTISIG NOT", "DERSIG", "OK", "BIP66 and NULLFAIL-compliant"],
2152 | +["0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0", "0x01 0x14 1 1 1 1 1 1 1 1 1 1 1 1 1 1 1 1 1 1 1 1 0x01 0x14 CHECKMULTISIG NOT", "DERSIG,NULLFAIL", "OK", "BIP66 and NULLFAIL-compliant"],
2153 | +["1 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0", "0x01 0x14 1 1 1 1 1 1 1 1 1 1 1 1 1 1 1 1 1 1 1 1 0x01 0x14 CHECKMULTISIG NOT", "DERSIG,NULLFAIL", "OK", "BIP66 and NULLFAIL-compliant, not NULLDUMMY-compliant"],
2154 | +["1 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0", "0x01 0x14 1 1 1 1 1 1 1 1 1 1 1 1 1 1 1 1 1 1 1 1 0x01 0x14 CHECKMULTISIG NOT", "DERSIG,NULLFAIL,NULLDUMMY", "SIG_NULLDUMMY", "BIP66 and NULLFAIL-compliant, not NULLDUMMY-compliant"],
2155 | +["0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0x09 0x300602010102010101", "0x01 0x14 1 1 1 1 1 1 1 1 1 1 1 1 1 1 1 1 1 1 1 1 0x01 0x14 CHECKMULTISIG NOT", "DERSIG", "OK", "BIP66 and NULLFAIL-compliant"],

jl2012 commented at 12:40 PM on September 6, 2016:

Comments for the following few lines should be read as "BIP66-compliant but not NULLFAIL-compliant". Will fix

NicolasDorier commented at 2:36 PM on September 6, 2016: contributor

CodeReview ACK bb50e7683d97ea6cc69fa5837a51fba54643f2cc
dcousens commented at 1:01 AM on September 11, 2016: contributor

concept ACK
btcdrak commented at 4:13 PM on September 12, 2016: contributor

ACK d071bfd
rubensayshi commented at 7:55 AM on September 13, 2016: contributor

utACK
MarcoFalke added the label Needs backport on Sep 15, 2016
gmaxwell commented at 7:42 PM on September 15, 2016: contributor

concept ACK, utACK, will review more.
afk11 approved
afk11 commented at 4:18 PM on September 17, 2016: contributor

tACK & code review bb50e76
in src/script/interpreter.cpp:None in d071bfd39b outdated
```
 905 | @@ -902,6 +906,7 @@ bool EvalScript(vector<vector<unsigned char> >& stack, const CScript& script, un
 906 |                          return set_error(serror, SCRIPT_ERR_INVALID_STACK_OPERATION);
 907 |  
 908 |                      int nKeysCount = CScriptNum(stacktop(-i), fRequireMinimal).getint();
 909 | +                    int ikey2 = nKeysCount + 2;
```
CodeShark commented at 4:29 AM on September 22, 2016:

Small nit: moving this line below the next will avoid performing the addition if the range check on nKeysCount fails. Also, since this is only used for cleanup, I would suggest giving the variable a more descriptive name like nNonSigCleanupItems or adding a comment describing what it's for.

jl2012 commented at 7:07 AM on September 22, 2016:

Addressed with 5d23cfe
CodeShark commented at 4:46 AM on September 22, 2016: contributor

utACK bb50e7683d97ea6cc69fa5837a51fba54643f2cc
jl2012 force-pushed on Sep 22, 2016
jl2012 force-pushed on Sep 22, 2016
jtimon commented at 7:21 PM on September 22, 2016: contributor

ut ACK 1912bf1

in src/script/interpreter.cpp:None in 1912bf12d0 outdated

 879 | @@ -880,6 +880,10 @@ bool EvalScript(vector<vector<unsigned char> >& stack, const CScript& script, un
 880 |                      bool fSuccess = checker.CheckSig(vchSig, vchPubKey, scriptCode, sigversion);
 881 |  
 882 |                      popstack(stack);
 883 | +
 884 | +                    if (!fSuccess && (flags & SCRIPT_VERIFY_NULLFAIL) && stacktop(-1).size())

instagibbs commented at 8:26 PM on September 22, 2016:

nit of nits: just check vchSig.size() instead, move before popstack

jl2012 commented at 3:12 AM on September 23, 2016:

Addressed with cd102b5

in src/script/interpreter.cpp:None in 1912bf12d0 outdated
```
 911 | @@ -908,6 +912,9 @@ bool EvalScript(vector<vector<unsigned char> >& stack, const CScript& script, un
 912 |                      if (nOpCount > MAX_OPS_PER_SCRIPT)
 913 |                          return set_error(serror, SCRIPT_ERR_OP_COUNT);
 914 |                      int ikey = ++i;
 915 | +                    // ikey2 is the position of last non-signature item in the stack. Top stack item = 1.
 916 | +                    // With SCRIPT_VERIFY_NULLFAIL, this is used for cleanup if operation fails.
 917 | +                    int ikey2 = nKeysCount + 2;
```
instagibbs commented at 8:47 PM on September 22, 2016:

This code section is a bit confusing already with all these different counters, why not set

int ikey2 = i;

right after the following line:

i += nKeysCount;

below? From there the two values diverge.

jl2012 commented at 3:16 AM on September 23, 2016:

I prefer the current way as it's clearly correct. The way that i is calculated looks a bit obscure to me

instagibbs commented at 3:28 AM on September 23, 2016:

Oh, I misread. Yes, the i is calculated quite oddly, but it's the value we're calculating yet again here.
instagibbs changes_requested
instagibbs commented at 8:57 PM on September 22, 2016: member

utACK 1912bf12d06011f989c0db4ae7cbf433cfff18da

I'd like to see the commits squashed though.
jl2012 force-pushed on Sep 23, 2016
CodeShark commented at 5:32 AM on September 23, 2016: contributor

utACK cd102b57a4c148fb93428e656e7282732c65033c
sipa approved
sipa commented at 3:04 PM on September 27, 2016: member

utACK
btcdrak commented at 3:15 PM on September 27, 2016: contributor

needs rebase
Add policy: null signature for failed CHECK(MULTI)SIG e41bd449ab
jl2012 force-pushed on Sep 27, 2016
jl2012 commented at 3:42 PM on September 27, 2016: contributor

rebased
laanwj merged this on Sep 27, 2016
laanwj closed this on Sep 27, 2016
laanwj referenced this in commit fc4f4547b7 on Sep 27, 2016
laanwj referenced this in commit 3e80ab7f2a on Oct 13, 2016
laanwj removed the label Needs backport on Oct 13, 2016
codablock referenced this in commit 92cd46cefd on Sep 19, 2017
codablock referenced this in commit a75d6110ee on Jan 12, 2018
andvgal referenced this in commit 8b60b63e06 on Jan 6, 2019
bitcoin locked this on Sep 8, 2021

Contributors

Labels

Milestone
0.13.1

Linked (view graph)

#8533 Implement LOW_S and NULLDUMMY softfork (BIP146)