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

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 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

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

MarcoFalke locked this on Sep 8, 2021