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)
2014-11-21 14:14:36 +01:00 · 2014-11-21 14:14:36 +01:00 · ca6fb4e885
commit ca6fb4e885
parent cb83af9937 ca8158719b
7 changed files with 103 additions and 25 deletions
--- a/src/script/interpreter.cpp
+++ b/src/script/interpreter.cpp
@ -207,9 +207,9 @@ bool static CheckSignatureEncoding(const valtype &vchSig, unsigned int flags, Sc
    return true;
 }

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

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

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

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

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

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

                        // If there are more signatures left than keys left,
-                        // then too many signatures have failed
+                        // then too many signatures have failed. Exit early,
+                        // without checking any further signatures.
                        if (nSigsCount > nKeysCount)
                            fSuccess = false;
                    }
--- a/src/script/interpreter.h
+++ b/src/script/interpreter.h
@ -35,8 +35,8 @@ enum
    SCRIPT_VERIFY_P2SH      = (1U << 0),

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

    // Passing a non-strict-DER signature to a checksig operation causes script failure (softfork safe, BIP62 rule 1)
--- a/src/script/script_error.cpp
+++ b/src/script/script_error.cpp
@ -61,6 +61,8 @@ const char* ScriptErrorString(const ScriptError serror)
            return "Dummy CHECKMULTISIG argument must be zero";
        case SCRIPT_ERR_DISCOURAGE_UPGRADABLE_NOPS:
            return "NOPx reserved for soft-fork upgrades";
+        case SCRIPT_ERR_PUBKEYTYPE:
+            return "Public key is neither compressed or uncompressed";
        case SCRIPT_ERR_UNKNOWN_ERROR:
        case SCRIPT_ERR_ERROR_COUNT:
        default: break;
--- a/src/script/script_error.h
+++ b/src/script/script_error.h
@ -42,6 +42,7 @@ typedef enum ScriptError_t
    SCRIPT_ERR_SIG_PUSHONLY,
    SCRIPT_ERR_SIG_HIGH_S,
    SCRIPT_ERR_SIG_NULLDUMMY,
+    SCRIPT_ERR_PUBKEYTYPE,

    /* softfork safeness */
    SCRIPT_ERR_DISCOURAGE_UPGRADABLE_NOPS,
--- a/src/test/data/script_invalid.json
+++ b/src/test/data/script_invalid.json
@ -592,12 +592,49 @@ nSequences are max.
    "",
    "P2PK NOT with hybrid pubkey but no STRICTENC"
 ],
+[
+    "0x47 0x3044022078033e4227aa05ded69d8da579966578e230d8a7fb44d5f1a0620c3853c24f78022006a2e3f4d872ac8dfdc529110aa37301d65a76255a4b6cce2992adacd4d2c4e201",
+    "0x41 0x0679be667ef9dcbbac55a06295ce870b07029bfcdb2dce28d959f2815b16f81798483ada7726a3c4655da4fbfc0e1108a8fd17b448a68554199c47d08ffb10d4b8 CHECKSIG NOT",
+    "STRICTENC",
+    "P2PK NOT with hybrid pubkey"
+],
+[
+    "0x47 0x304402207592427de20e315d644839754f2a5cca5b978b983a15e6da82109ede01722baa022032ceaf78590faa3f7743821e1b47b897ed1a57f6ee1c8a7519d23774d8de3c4401",
+    "0x41 0x0679be667ef9dcbbac55a06295ce870b07029bfcdb2dce28d959f2815b16f81798483ada7726a3c4655da4fbfc0e1108a8fd17b448a68554199c47d08ffb10d4b8 CHECKSIG NOT",
+    "STRICTENC",
+    "P2PK NOT with invalid hybrid pubkey"
+],
+[
+    "0 0x47 0x304402206797289d3dc81692edae58430276d04641ea5d86967be557163f8494da32fd78022006fc6ab77aaed4ac11ea69cd878ab26e3e24290f47a43e9adf34075d52b7142c01",
+    "1 0x21 0x038282263212c609d9ea2a6e3e172de238d8c39cabd5ac1ca10646e23fd5f51508 0x41 0x0679be667ef9dcbbac55a06295ce870b07029bfcdb2dce28d959f2815b16f81798483ada7726a3c4655da4fbfc0e1108a8fd17b448a68554199c47d08ffb10d4b8 2 CHECKMULTISIG",
+    "STRICTENC",
+    "1-of-2 with the first 1 hybrid pubkey"
+],
 [
    "0x47 0x304402201f82b99a813c9c48c8dee8d2c43b8f637b72353fe9bdcc084537bc17e2ab770402200c43b96a5f7e115f0114eabda32e068145965cb6c7b5ef64833bb4fcf9fc1b3b05",
    "0x41 0x048282263212c609d9ea2a6e3e172de238d8c39cabd5ac1ca10646e23fd5f5150811f8a8098557dfe45e8256e830b60ace62d613ac2f7b17bed31b6eaff6e26caf CHECKSIG",
    "STRICTENC",
    "P2PK with undefined hashtype"
 ],
+
+["
+Order of CHECKMULTISIG evaluation tests, inverted by swapping the order of
+pubkeys/signatures so they fail due to the STRICTENC rules on validly encoded
+signatures and pubkeys.
+"],
+[
+    "0 0x47 0x3044022044dc17b0887c161bb67ba9635bf758735bdde503e4b0a0987f587f14a4e1143d022009a215772d49a85dae40d8ca03955af26ad3978a0ff965faa12915e9586249a501 0x47 0x3044022044dc17b0887c161bb67ba9635bf758735bdde503e4b0a0987f587f14a4e1143d022009a215772d49a85dae40d8ca03955af26ad3978a0ff965faa12915e9586249a501",
+    "2 0x21 0x02865c40293a680cb9c020e7b1e106d8c1916d3cef99aa431a56d253e69256dac0 0 2 CHECKMULTISIG NOT",
+    "STRICTENC",
+    "2-of-2 CHECKMULTISIG NOT with the first pubkey invalid, and both signatures validly encoded."
+],
+[
+    "0 0x47 0x3044022044dc17b0887c161bb67ba9635bf758735bdde503e4b0a0987f587f14a4e1143d022009a215772d49a85dae40d8ca03955af26ad3978a0ff965faa12915e9586249a501 0",
+    "2 0x21 0x02865c40293a680cb9c020e7b1e106d8c1916d3cef99aa431a56d253e69256dac0 0x21 0x02865c40293a680cb9c020e7b1e106d8c1916d3cef99aa431a56d253e69256dac0 2 CHECKMULTISIG NOT",
+    "STRICTENC",
+    "2-of-2 CHECKMULTISIG NOT with both pubkeys valid, but first signature invalid."
+],
+
 [
    "0x47 0x30440220166848cd5b82a32b5944d90de3c35249354b43773c2ece1844ee8d1103e2f6c602203b6b046da4243c77adef80ada9201b27bbfdf7f9d5428f40434b060432afd62005",
    "0x41 0x048282263212c609d9ea2a6e3e172de238d8c39cabd5ac1ca10646e23fd5f5150811f8a8098557dfe45e8256e830b60ace62d613ac2f7b17bed31b6eaff6e26caf CHECKSIG NOT",
--- a/src/test/data/script_valid.json
+++ b/src/test/data/script_valid.json
@ -743,12 +743,6 @@ nSequences are max.
    "",
    "P2PK with hybrid pubkey but no STRICTENC"
 ],
-[
-    "0x47 0x3044022078033e4227aa05ded69d8da579966578e230d8a7fb44d5f1a0620c3853c24f78022006a2e3f4d872ac8dfdc529110aa37301d65a76255a4b6cce2992adacd4d2c4e201",
-    "0x41 0x0679be667ef9dcbbac55a06295ce870b07029bfcdb2dce28d959f2815b16f81798483ada7726a3c4655da4fbfc0e1108a8fd17b448a68554199c47d08ffb10d4b8 CHECKSIG NOT",
-    "STRICTENC",
-    "P2PK NOT with hybrid pubkey"
-],
 [
    "0x47 0x3044022078d6c447887e88dcbe1bc5b613645280df6f4e5935648bc226e9d91da71b3216022047d6b7ef0949b228fc1b359afb8d50500268711354298217b983c26970790c7601",
    "0x41 0x0679be667ef9dcbbac55a06295ce870b07029bfcdb2dce28d959f2815b16f81798483ada7726a3c4655da4fbfc0e1108a8fd17b448a68554199c47d08ffb10d4b8 CHECKSIG NOT",
@ -756,11 +750,42 @@ nSequences are max.
    "P2PK NOT with invalid hybrid pubkey but no STRICTENC"
 ],
 [
-    "0x47 0x304402207592427de20e315d644839754f2a5cca5b978b983a15e6da82109ede01722baa022032ceaf78590faa3f7743821e1b47b897ed1a57f6ee1c8a7519d23774d8de3c4401",
-    "0x41 0x0679be667ef9dcbbac55a06295ce870b07029bfcdb2dce28d959f2815b16f81798483ada7726a3c4655da4fbfc0e1108a8fd17b448a68554199c47d08ffb10d4b8 CHECKSIG NOT",
-    "STRICTENC",
-    "P2PK NOT with invalid hybrid pubkey"
+    "0 0x47 0x304402203b269b9fbc0936877bf855b5fb41757218d9548b246370d991442a5f5bd1c3440220235268a4eaa8c67e543c6e37da81dd36d3b1be2de6b4fef04113389ca6ddc04501",
+    "1 0x41 0x0679be667ef9dcbbac55a06295ce870b07029bfcdb2dce28d959f2815b16f81798483ada7726a3c4655da4fbfc0e1108a8fd17b448a68554199c47d08ffb10d4b8 0x21 0x038282263212c609d9ea2a6e3e172de238d8c39cabd5ac1ca10646e23fd5f51508 2 CHECKMULTISIG",
+    "",
+    "1-of-2 with the second 1 hybrid pubkey and no STRICTENC"
 ],
+[
+    "0 0x47 0x3044022044dc17b0887c161bb67ba9635bf758735bdde503e4b0a0987f587f14a4e1143d022009a215772d49a85dae40d8ca03955af26ad3978a0ff965faa12915e9586249a501",
+    "1 0x41 0x0679be667ef9dcbbac55a06295ce870b07029bfcdb2dce28d959f2815b16f81798483ada7726a3c4655da4fbfc0e1108a8fd17b448a68554199c47d08ffb10d4b8 0x21 0x038282263212c609d9ea2a6e3e172de238d8c39cabd5ac1ca10646e23fd5f51508 2 CHECKMULTISIG",
+    "STRICTENC",
+    "1-of-2 with the second 1 hybrid pubkey"
+],
+
+["
+CHECKMULTISIG evaluation order tests. CHECKMULTISIG evaluates signatures and
+pubkeys in a specific order, and will exit early if the number of signatures
+left to check is greater than the number of keys left. As STRICTENC fails the
+script when it reaches an invalidly encoded signature or pubkey, we can use it
+to test the exact order in which signatures and pubkeys are evaluated by
+distinguishing CHECKMULTISIG returning false on the stack and the script as a
+whole failing.
+
+See also the corresponding inverted versions of these tests in script_invalid.json
+"],
+[
+    "0 0x47 0x3044022044dc17b0887c161bb67ba9635bf758735bdde503e4b0a0987f587f14a4e1143d022009a215772d49a85dae40d8ca03955af26ad3978a0ff965faa12915e9586249a501 0x47 0x3044022044dc17b0887c161bb67ba9635bf758735bdde503e4b0a0987f587f14a4e1143d022009a215772d49a85dae40d8ca03955af26ad3978a0ff965faa12915e9586249a501",
+    "2 0 0x21 0x02865c40293a680cb9c020e7b1e106d8c1916d3cef99aa431a56d253e69256dac0 2 CHECKMULTISIG NOT",
+    "STRICTENC",
+    "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."
+],
+[
+    "0 0 0x47 0x3044022044dc17b0887c161bb67ba9635bf758735bdde503e4b0a0987f587f14a4e1143d022009a215772d49a85dae40d8ca03955af26ad3978a0ff965faa12915e9586249a501",
+    "2 0x21 0x02865c40293a680cb9c020e7b1e106d8c1916d3cef99aa431a56d253e69256dac0 0x21 0x02865c40293a680cb9c020e7b1e106d8c1916d3cef99aa431a56d253e69256dac0 2 CHECKMULTISIG NOT",
+    "STRICTENC",
+    "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."
+],
+
 [
    "0x47 0x304402204649e9517ef0377a8f8270bd423053fd98ddff62d74ea553e9579558abbb75e4022044a2b2344469c12e35ed898987711272b634733dd0f5e051288eceb04bd4669e05",
    "0x41 0x048282263212c609d9ea2a6e3e172de238d8c39cabd5ac1ca10646e23fd5f5150811f8a8098557dfe45e8256e830b60ace62d613ac2f7b17bed31b6eaff6e26caf CHECKSIG",
--- a/src/test/script_tests.cpp
+++ b/src/test/script_tests.cpp
@ -428,15 +428,24 @@ BOOST_AUTO_TEST_CASE(script_build)
    bad.push_back(TestBuilder(CScript() << ToByteVector(keys.pubkey0H) << OP_CHECKSIG << OP_NOT,
                              "P2PK NOT with hybrid pubkey but no STRICTENC", 0
                             ).PushSig(keys.key0, SIGHASH_ALL));
-    good.push_back(TestBuilder(CScript() << ToByteVector(keys.pubkey0H) << OP_CHECKSIG << OP_NOT,
-                               "P2PK NOT with hybrid pubkey", SCRIPT_VERIFY_STRICTENC
-                              ).PushSig(keys.key0, SIGHASH_ALL));
+    bad.push_back(TestBuilder(CScript() << ToByteVector(keys.pubkey0H) << OP_CHECKSIG << OP_NOT,
+                              "P2PK NOT with hybrid pubkey", SCRIPT_VERIFY_STRICTENC
+                             ).PushSig(keys.key0, SIGHASH_ALL));
    good.push_back(TestBuilder(CScript() << ToByteVector(keys.pubkey0H) << OP_CHECKSIG << OP_NOT,
                               "P2PK NOT with invalid hybrid pubkey but no STRICTENC", 0
                              ).PushSig(keys.key0, SIGHASH_ALL).DamagePush(10));
-    good.push_back(TestBuilder(CScript() << ToByteVector(keys.pubkey0H) << OP_CHECKSIG << OP_NOT,
-                               "P2PK NOT with invalid hybrid pubkey", SCRIPT_VERIFY_STRICTENC
-                              ).PushSig(keys.key0, SIGHASH_ALL).DamagePush(10));
+    bad.push_back(TestBuilder(CScript() << ToByteVector(keys.pubkey0H) << OP_CHECKSIG << OP_NOT,
+                              "P2PK NOT with invalid hybrid pubkey", SCRIPT_VERIFY_STRICTENC
+                             ).PushSig(keys.key0, SIGHASH_ALL).DamagePush(10));
+    good.push_back(TestBuilder(CScript() << OP_1 << ToByteVector(keys.pubkey0H) << ToByteVector(keys.pubkey1C) << OP_2 << OP_CHECKMULTISIG,
+                               "1-of-2 with the second 1 hybrid pubkey and no STRICTENC", 0
+                              ).Num(0).PushSig(keys.key1, SIGHASH_ALL));
+    good.push_back(TestBuilder(CScript() << OP_1 << ToByteVector(keys.pubkey0H) << ToByteVector(keys.pubkey1C) << OP_2 << OP_CHECKMULTISIG,
+                               "1-of-2 with the second 1 hybrid pubkey", SCRIPT_VERIFY_STRICTENC
+                              ).Num(0).PushSig(keys.key1, SIGHASH_ALL));
+    bad.push_back(TestBuilder(CScript() << OP_1 << ToByteVector(keys.pubkey1C) << ToByteVector(keys.pubkey0H) << OP_2 << OP_CHECKMULTISIG,
+                              "1-of-2 with the first 1 hybrid pubkey", SCRIPT_VERIFY_STRICTENC
+                             ).Num(0).PushSig(keys.key1, SIGHASH_ALL));

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