Skip to content

Commit ca6fb4e

Browse files
laanwjlaanwj
committed
Merge pull request #5247
ca81587 Test the exact order of CHECKMULTISIG sig/pubkey evaluation (Peter Todd) 98b135f Make STRICTENC invalid pubkeys fail the script rather than the opcode. (Pieter Wuille)
2 parents cb83af9 + ca81587 commit ca6fb4e

File tree

7 files changed

+102
-24
lines changed

7 files changed

+102
-24
lines changed

‎src/script/interpreter.cpp‎

Lines changed: 11 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -207,9 +207,9 @@ bool static CheckSignatureEncoding(const valtype &vchSig, unsigned int flags, Sc
207207
return true;
208208
}
209209

210-
bool static CheckPubKeyEncoding(const valtype &vchSig, unsigned int flags) {
210+
bool static CheckPubKeyEncoding(const valtype &vchSig, unsigned int flags, ScriptError* serror) {
211211
if ((flags & SCRIPT_VERIFY_STRICTENC) != 0 && !IsCompressedOrUncompressedPubKey(vchSig)) {
212-
return false;
212+
return set_error(serror, SCRIPT_ERR_PUBKEYTYPE);
213213
}
214214
return true;
215215
}
@@ -792,11 +792,11 @@ bool EvalScript(vector<vector<unsigned char> >& stack, const CScript& script, un
792792
// Drop the signature, since there's no way for a signature to sign itself
793793
scriptCode.FindAndDelete(CScript(vchSig));
794794

795-
if (!CheckSignatureEncoding(vchSig, flags, serror)) {
795+
if (!CheckSignatureEncoding(vchSig, flags, serror) || !CheckPubKeyEncoding(vchPubKey, flags, serror)) {
796796
//serror is set
797797
return false;
798798
}
799-
bool fSuccess = CheckPubKeyEncoding(vchPubKey, flags) && checker.CheckSig(vchSig, vchPubKey, scriptCode);
799+
bool fSuccess = checker.CheckSig(vchSig, vchPubKey, scriptCode);
800800

801801
popstack(stack);
802802
popstack(stack);
@@ -855,13 +855,16 @@ bool EvalScript(vector<vector<unsigned char> >& stack, const CScript& script, un
855855
valtype& vchSig = stacktop(-isig);
856856
valtype& vchPubKey = stacktop(-ikey);
857857

858-
if (!CheckSignatureEncoding(vchSig, flags, serror)) {
858+
// Note how this makes the exact order of pubkey/signature evaluation
859+
// distinguishable by CHECKMULTISIG NOT if the STRICTENC flag is set.
860+
// See the script_(in)valid tests for details.
861+
if (!CheckSignatureEncoding(vchSig, flags, serror) || !CheckPubKeyEncoding(vchPubKey, flags, serror)) {
859862
// serror is set
860863
return false;
861864
}
862865

863866
// Check signature
864-
bool fOk = CheckPubKeyEncoding(vchPubKey, flags) && checker.CheckSig(vchSig, vchPubKey, scriptCode);
867+
bool fOk = checker.CheckSig(vchSig, vchPubKey, scriptCode);
865868

866869
if (fOk) {
867870
isig++;
@@ -871,7 +874,8 @@ bool EvalScript(vector<vector<unsigned char> >& stack, const CScript& script, un
871874
nKeysCount--;
872875

873876
// If there are more signatures left than keys left,
874-
// then too many signatures have failed
877+
// then too many signatures have failed. Exit early,
878+
// without checking any further signatures.
875879
if (nSigsCount > nKeysCount)
876880
fSuccess = false;
877881
}

‎src/script/interpreter.h‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -35,8 +35,8 @@ enum
3535
SCRIPT_VERIFY_P2SH = (1U << 0),
3636

3737
// Passing a non-strict-DER signature or one with undefined hashtype to a checksig operation causes script failure.
38-
// Passing a pubkey that is not (0x04 + 64 bytes) or (0x02 or 0x03 + 32 bytes) to checksig causes that pubkey to be
39-
// skipped (not softfork safe: this flag can widen the validity of OP_CHECKSIG OP_NOT).
38+
// Evaluating a pubkey that is not (0x04 + 64 bytes) or (0x02 or 0x03 + 32 bytes) by checksig causes script failure.
39+
// (softfork safe, but not used or intended as a consensus rule).
4040
SCRIPT_VERIFY_STRICTENC = (1U << 1),
4141

4242
// Passing a non-strict-DER signature to a checksig operation causes script failure (softfork safe, BIP62 rule 1)

‎src/script/script_error.cpp‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -61,6 +61,8 @@ const char* ScriptErrorString(const ScriptError serror)
6161
return "Dummy CHECKMULTISIG argument must be zero";
6262
case SCRIPT_ERR_DISCOURAGE_UPGRADABLE_NOPS:
6363
return "NOPx reserved for soft-fork upgrades";
64+
case SCRIPT_ERR_PUBKEYTYPE:
65+
return "Public key is neither compressed or uncompressed";
6466
case SCRIPT_ERR_UNKNOWN_ERROR:
6567
case SCRIPT_ERR_ERROR_COUNT:
6668
default: break;

‎src/script/script_error.h‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -42,6 +42,7 @@ typedef enum ScriptError_t
4242
SCRIPT_ERR_SIG_PUSHONLY,
4343
SCRIPT_ERR_SIG_HIGH_S,
4444
SCRIPT_ERR_SIG_NULLDUMMY,
45+
SCRIPT_ERR_PUBKEYTYPE,
4546

4647
/* softfork safeness */
4748
SCRIPT_ERR_DISCOURAGE_UPGRADABLE_NOPS,

‎src/test/data/script_invalid.json‎

Lines changed: 37 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -592,12 +592,49 @@ nSequences are max.
592592
"",
593593
"P2PK NOT with hybrid pubkey but no STRICTENC"
594594
],
595+
[
596+
"0x47 0x3044022078033e4227aa05ded69d8da579966578e230d8a7fb44d5f1a0620c3853c24f78022006a2e3f4d872ac8dfdc529110aa37301d65a76255a4b6cce2992adacd4d2c4e201",
597+
"0x41 0x0679be667ef9dcbbac55a06295ce870b07029bfcdb2dce28d959f2815b16f81798483ada7726a3c4655da4fbfc0e1108a8fd17b448a68554199c47d08ffb10d4b8 CHECKSIG NOT",
598+
"STRICTENC",
599+
"P2PK NOT with hybrid pubkey"
600+
],
601+
[
602+
"0x47 0x304402207592427de20e315d644839754f2a5cca5b978b983a15e6da82109ede01722baa022032ceaf78590faa3f7743821e1b47b897ed1a57f6ee1c8a7519d23774d8de3c4401",
603+
"0x41 0x0679be667ef9dcbbac55a06295ce870b07029bfcdb2dce28d959f2815b16f81798483ada7726a3c4655da4fbfc0e1108a8fd17b448a68554199c47d08ffb10d4b8 CHECKSIG NOT",
604+
"STRICTENC",
605+
"P2PK NOT with invalid hybrid pubkey"
606+
],
607+
[
608+
"0 0x47 0x304402206797289d3dc81692edae58430276d04641ea5d86967be557163f8494da32fd78022006fc6ab77aaed4ac11ea69cd878ab26e3e24290f47a43e9adf34075d52b7142c01",
609+
"1 0x21 0x038282263212c609d9ea2a6e3e172de238d8c39cabd5ac1ca10646e23fd5f51508 0x41 0x0679be667ef9dcbbac55a06295ce870b07029bfcdb2dce28d959f2815b16f81798483ada7726a3c4655da4fbfc0e1108a8fd17b448a68554199c47d08ffb10d4b8 2 CHECKMULTISIG",
610+
"STRICTENC",
611+
"1-of-2 with the first 1 hybrid pubkey"
612+
],
595613
[
596614
"0x47 0x304402201f82b99a813c9c48c8dee8d2c43b8f637b72353fe9bdcc084537bc17e2ab770402200c43b96a5f7e115f0114eabda32e068145965cb6c7b5ef64833bb4fcf9fc1b3b05",
597615
"0x41 0x048282263212c609d9ea2a6e3e172de238d8c39cabd5ac1ca10646e23fd5f5150811f8a8098557dfe45e8256e830b60ace62d613ac2f7b17bed31b6eaff6e26caf CHECKSIG",
598616
"STRICTENC",
599617
"P2PK with undefined hashtype"
600618
],
619+
620+
["
621+
Order of CHECKMULTISIG evaluation tests, inverted by swapping the order of
622+
pubkeys/signatures so they fail due to the STRICTENC rules on validly encoded
623+
signatures and pubkeys.
624+
"],
625+
[
626+
"0 0x47 0x3044022044dc17b0887c161bb67ba9635bf758735bdde503e4b0a0987f587f14a4e1143d022009a215772d49a85dae40d8ca03955af26ad3978a0ff965faa12915e9586249a501 0x47 0x3044022044dc17b0887c161bb67ba9635bf758735bdde503e4b0a0987f587f14a4e1143d022009a215772d49a85dae40d8ca03955af26ad3978a0ff965faa12915e9586249a501",
627+
"2 0x21 0x02865c40293a680cb9c020e7b1e106d8c1916d3cef99aa431a56d253e69256dac0 0 2 CHECKMULTISIG NOT",
628+
"STRICTENC",
629+
"2-of-2 CHECKMULTISIG NOT with the first pubkey invalid, and both signatures validly encoded."
630+
],
631+
[
632+
"0 0x47 0x3044022044dc17b0887c161bb67ba9635bf758735bdde503e4b0a0987f587f14a4e1143d022009a215772d49a85dae40d8ca03955af26ad3978a0ff965faa12915e9586249a501 0",
633+
"2 0x21 0x02865c40293a680cb9c020e7b1e106d8c1916d3cef99aa431a56d253e69256dac0 0x21 0x02865c40293a680cb9c020e7b1e106d8c1916d3cef99aa431a56d253e69256dac0 2 CHECKMULTISIG NOT",
634+
"STRICTENC",
635+
"2-of-2 CHECKMULTISIG NOT with both pubkeys valid, but first signature invalid."
636+
],
637+
601638
[
602639
"0x47 0x30440220166848cd5b82a32b5944d90de3c35249354b43773c2ece1844ee8d1103e2f6c602203b6b046da4243c77adef80ada9201b27bbfdf7f9d5428f40434b060432afd62005",
603640
"0x41 0x048282263212c609d9ea2a6e3e172de238d8c39cabd5ac1ca10646e23fd5f5150811f8a8098557dfe45e8256e830b60ace62d613ac2f7b17bed31b6eaff6e26caf CHECKSIG NOT",

‎src/test/data/script_valid.json‎

Lines changed: 34 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -743,24 +743,49 @@ nSequences are max.
743743
"",
744744
"P2PK with hybrid pubkey but no STRICTENC"
745745
],
746-
[
747-
"0x47 0x3044022078033e4227aa05ded69d8da579966578e230d8a7fb44d5f1a0620c3853c24f78022006a2e3f4d872ac8dfdc529110aa37301d65a76255a4b6cce2992adacd4d2c4e201",
748-
"0x41 0x0679be667ef9dcbbac55a06295ce870b07029bfcdb2dce28d959f2815b16f81798483ada7726a3c4655da4fbfc0e1108a8fd17b448a68554199c47d08ffb10d4b8 CHECKSIG NOT",
749-
"STRICTENC",
750-
"P2PK NOT with hybrid pubkey"
751-
],
752746
[
753747
"0x47 0x3044022078d6c447887e88dcbe1bc5b613645280df6f4e5935648bc226e9d91da71b3216022047d6b7ef0949b228fc1b359afb8d50500268711354298217b983c26970790c7601",
754748
"0x41 0x0679be667ef9dcbbac55a06295ce870b07029bfcdb2dce28d959f2815b16f81798483ada7726a3c4655da4fbfc0e1108a8fd17b448a68554199c47d08ffb10d4b8 CHECKSIG NOT",
755749
"",
756750
"P2PK NOT with invalid hybrid pubkey but no STRICTENC"
757751
],
758752
[
759-
"0x47 0x304402207592427de20e315d644839754f2a5cca5b978b983a15e6da82109ede01722baa022032ceaf78590faa3f7743821e1b47b897ed1a57f6ee1c8a7519d23774d8de3c4401",
760-
"0x41 0x0679be667ef9dcbbac55a06295ce870b07029bfcdb2dce28d959f2815b16f81798483ada7726a3c4655da4fbfc0e1108a8fd17b448a68554199c47d08ffb10d4b8 CHECKSIG NOT",
753+
"0 0x47 0x304402203b269b9fbc0936877bf855b5fb41757218d9548b246370d991442a5f5bd1c3440220235268a4eaa8c67e543c6e37da81dd36d3b1be2de6b4fef04113389ca6ddc04501",
754+
"1 0x41 0x0679be667ef9dcbbac55a06295ce870b07029bfcdb2dce28d959f2815b16f81798483ada7726a3c4655da4fbfc0e1108a8fd17b448a68554199c47d08ffb10d4b8 0x21 0x038282263212c609d9ea2a6e3e172de238d8c39cabd5ac1ca10646e23fd5f51508 2 CHECKMULTISIG",
755+
"",
756+
"1-of-2 with the second 1 hybrid pubkey and no STRICTENC"
757+
],
758+
[
759+
"0 0x47 0x3044022044dc17b0887c161bb67ba9635bf758735bdde503e4b0a0987f587f14a4e1143d022009a215772d49a85dae40d8ca03955af26ad3978a0ff965faa12915e9586249a501",
760+
"1 0x41 0x0679be667ef9dcbbac55a06295ce870b07029bfcdb2dce28d959f2815b16f81798483ada7726a3c4655da4fbfc0e1108a8fd17b448a68554199c47d08ffb10d4b8 0x21 0x038282263212c609d9ea2a6e3e172de238d8c39cabd5ac1ca10646e23fd5f51508 2 CHECKMULTISIG",
761+
"STRICTENC",
762+
"1-of-2 with the second 1 hybrid pubkey"
763+
],
764+
765+
["
766+
CHECKMULTISIG evaluation order tests. CHECKMULTISIG evaluates signatures and
767+
pubkeys in a specific order, and will exit early if the number of signatures
768+
left to check is greater than the number of keys left. As STRICTENC fails the
769+
script when it reaches an invalidly encoded signature or pubkey, we can use it
770+
to test the exact order in which signatures and pubkeys are evaluated by
771+
distinguishing CHECKMULTISIG returning false on the stack and the script as a
772+
whole failing.
773+
774+
See also the corresponding inverted versions of these tests in script_invalid.json
775+
"],
776+
[
777+
"0 0x47 0x3044022044dc17b0887c161bb67ba9635bf758735bdde503e4b0a0987f587f14a4e1143d022009a215772d49a85dae40d8ca03955af26ad3978a0ff965faa12915e9586249a501 0x47 0x3044022044dc17b0887c161bb67ba9635bf758735bdde503e4b0a0987f587f14a4e1143d022009a215772d49a85dae40d8ca03955af26ad3978a0ff965faa12915e9586249a501",
778+
"2 0 0x21 0x02865c40293a680cb9c020e7b1e106d8c1916d3cef99aa431a56d253e69256dac0 2 CHECKMULTISIG NOT",
761779
"STRICTENC",
762-
"P2PK NOT with invalid hybrid pubkey"
780+
"2-of-2 CHECKMULTISIG NOT with the second pubkey invalid, and both signatures validly encoded. Valid pubkey fails, and CHECKMULTISIG exits early, prior to evaluation of second invalid pubkey."
763781
],
782+
[
783+
"0 0 0x47 0x3044022044dc17b0887c161bb67ba9635bf758735bdde503e4b0a0987f587f14a4e1143d022009a215772d49a85dae40d8ca03955af26ad3978a0ff965faa12915e9586249a501",
784+
"2 0x21 0x02865c40293a680cb9c020e7b1e106d8c1916d3cef99aa431a56d253e69256dac0 0x21 0x02865c40293a680cb9c020e7b1e106d8c1916d3cef99aa431a56d253e69256dac0 2 CHECKMULTISIG NOT",
785+
"STRICTENC",
786+
"2-of-2 CHECKMULTISIG NOT with both pubkeys valid, but second signature invalid. Valid pubkey fails, and CHECKMULTISIG exits early, prior to evaluation of second invalid signature."
787+
],
788+
764789
[
765790
"0x47 0x304402204649e9517ef0377a8f8270bd423053fd98ddff62d74ea553e9579558abbb75e4022044a2b2344469c12e35ed898987711272b634733dd0f5e051288eceb04bd4669e05",
766791
"0x41 0x048282263212c609d9ea2a6e3e172de238d8c39cabd5ac1ca10646e23fd5f5150811f8a8098557dfe45e8256e830b60ace62d613ac2f7b17bed31b6eaff6e26caf CHECKSIG",

‎src/test/script_tests.cpp‎

Lines changed: 15 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -428,15 +428,24 @@ BOOST_AUTO_TEST_CASE(script_build)
428428
bad.push_back(TestBuilder(CScript() << ToByteVector(keys.pubkey0H) << OP_CHECKSIG << OP_NOT,
429429
"P2PK NOT with hybrid pubkey but no STRICTENC", 0
430430
).PushSig(keys.key0, SIGHASH_ALL));
431-
good.push_back(TestBuilder(CScript() << ToByteVector(keys.pubkey0H) << OP_CHECKSIG << OP_NOT,
432-
"P2PK NOT with hybrid pubkey", SCRIPT_VERIFY_STRICTENC
433-
).PushSig(keys.key0, SIGHASH_ALL));
431+
bad.push_back(TestBuilder(CScript() << ToByteVector(keys.pubkey0H) << OP_CHECKSIG << OP_NOT,
432+
"P2PK NOT with hybrid pubkey", SCRIPT_VERIFY_STRICTENC
433+
).PushSig(keys.key0, SIGHASH_ALL));
434434
good.push_back(TestBuilder(CScript() << ToByteVector(keys.pubkey0H) << OP_CHECKSIG << OP_NOT,
435435
"P2PK NOT with invalid hybrid pubkey but no STRICTENC", 0
436436
).PushSig(keys.key0, SIGHASH_ALL).DamagePush(10));
437-
good.push_back(TestBuilder(CScript() << ToByteVector(keys.pubkey0H) << OP_CHECKSIG << OP_NOT,
438-
"P2PK NOT with invalid hybrid pubkey", SCRIPT_VERIFY_STRICTENC
439-
).PushSig(keys.key0, SIGHASH_ALL).DamagePush(10));
437+
bad.push_back(TestBuilder(CScript() << ToByteVector(keys.pubkey0H) << OP_CHECKSIG << OP_NOT,
438+
"P2PK NOT with invalid hybrid pubkey", SCRIPT_VERIFY_STRICTENC
439+
).PushSig(keys.key0, SIGHASH_ALL).DamagePush(10));
440+
good.push_back(TestBuilder(CScript() << OP_1 << ToByteVector(keys.pubkey0H) << ToByteVector(keys.pubkey1C) << OP_2 << OP_CHECKMULTISIG,
441+
"1-of-2 with the second 1 hybrid pubkey and no STRICTENC", 0
442+
).Num(0).PushSig(keys.key1, SIGHASH_ALL));
443+
good.push_back(TestBuilder(CScript() << OP_1 << ToByteVector(keys.pubkey0H) << ToByteVector(keys.pubkey1C) << OP_2 << OP_CHECKMULTISIG,
444+
"1-of-2 with the second 1 hybrid pubkey", SCRIPT_VERIFY_STRICTENC
445+
).Num(0).PushSig(keys.key1, SIGHASH_ALL));
446+
bad.push_back(TestBuilder(CScript() << OP_1 << ToByteVector(keys.pubkey1C) << ToByteVector(keys.pubkey0H) << OP_2 << OP_CHECKMULTISIG,
447+
"1-of-2 with the first 1 hybrid pubkey", SCRIPT_VERIFY_STRICTENC
448+
).Num(0).PushSig(keys.key1, SIGHASH_ALL));
440449

441450
good.push_back(TestBuilder(CScript() << ToByteVector(keys.pubkey1) << OP_CHECKSIG,
442451
"P2PK with undefined hashtype but no STRICTENC", 0

0 commit comments

Comments
 (0)