Only create signatures with even S, and verification mode to check.

sipa · Pieter Wuille · commit a81cd96805ce · 2013-08-16T00:17:50.000+02:00
To fix a minor malleability found by Sergio Lerner (reported here: https://bitcointalk.org/index.php?topic=8392.msg1245898#msg1245898) The problem is that if (R,S) is a valid ECDSA signature for a given message and public key, (R,-S) is also valid. Modulo N (the order of the secp256k1 curve), this means that both (R,S) and (R,N-S) are valid. Given that N is odd, S and N-S have a different lowest bit. We solve the problem by forcing signatures to have an even S value, excluding one of the alternatives. This commit just changes the signing code to always produce even S values, and adds a verification mode to check it. This code is not enabled anywhere yet. Existing tests in key_tests.cpp verify that the produced signatures are still valid.
diff --git a/src/key.cpp b/src/key.cpp
@@ -194,9 +194,26 @@ class CECKey {
     }
 
     bool Sign(const uint256 &hash, std::vector<unsigned char>& vchSig) {
+        vchSig.clear();
+        ECDSA_SIG *sig = ECDSA_do_sign((unsigned char*)&hash, sizeof(hash), pkey);
+        if (sig == NULL)
+            return false;
+        if (BN_is_odd(sig->s)) {
+            // enforce even S values, by negating the value (modulo the order) if odd
+            BN_CTX *ctx = BN_CTX_new();
+            BN_CTX_start(ctx);
+            const EC_GROUP *group = EC_KEY_get0_group(pkey);
+            BIGNUM *order = BN_CTX_get(ctx);
+            EC_GROUP_get_order(group, order, ctx);
+            BN_sub(sig->s, order, sig->s);
+            BN_CTX_end(ctx);
+            BN_CTX_free(ctx);
+        }
         unsigned int nSize = ECDSA_size(pkey);
         vchSig.resize(nSize); // Make sure it is big enough
-        assert(ECDSA_sign(0, (unsigned char*)&hash, sizeof(hash), &vchSig[0], &nSize, pkey));
+        unsigned char *pos = &vchSig[0];
+        nSize = i2d_ECDSA_SIG(sig, &pos);
+        ECDSA_SIG_free(sig);
         vchSig.resize(nSize); // Shrink to fit actual size
         return true;
     }
diff --git a/src/script.cpp b/src/script.cpp
@@ -227,7 +227,10 @@ const char* GetOpName(opcodetype opcode)
     }
 }
 
-bool IsCanonicalPubKey(const valtype &vchPubKey) {
+bool IsCanonicalPubKey(const valtype &vchPubKey, unsigned int flags) {
+    if (!(flags & SCRIPT_VERIFY_STRICTENC))
+        return true;
+
     if (vchPubKey.size() < 33)
         return error("Non-canonical public key: too short");
     if (vchPubKey[0] == 0x04) {
@@ -242,7 +245,10 @@ bool IsCanonicalPubKey(const valtype &vchPubKey) {
     return true;
 }
 
-bool IsCanonicalSignature(const valtype &vchSig) {
+bool IsCanonicalSignature(const valtype &vchSig, unsigned int flags) {
+    if (!(flags & SCRIPT_VERIFY_STRICTENC))
+        return true;
+
     // See https://bitcointalk.org/index.php?topic=8392.msg127623#msg127623
     // A canonical signature exists of: <30> <total len> <02> <len R> <R> <02> <len S> <S> <hashtype>
     // Where R and S are not negative (their first byte has its highest bit not set), and not
@@ -286,6 +292,11 @@ bool IsCanonicalSignature(const valtype &vchSig) {
     if (nLenS > 1 && (S[0] == 0x00) && !(S[1] & 0x80))
         return error("Non-canonical signature: S value excessively padded");
 
+    if (flags & SCRIPT_VERIFY_EVEN_S) {
+        if (S[nLenS-1] & 1)
+            return error("Non-canonical signature: S value odd");
+    }
+
     return true;
 }
 
@@ -302,7 +313,6 @@ bool EvalScript(vector<vector<unsigned char> >& stack, const CScript& script, co
     if (script.size() > 10000)
         return false;
     int nOpCount = 0;
-    bool fStrictEncodings = flags & SCRIPT_VERIFY_STRICTENC;
 
     try
     {
@@ -841,9 +851,8 @@ bool EvalScript(vector<vector<unsigned char> >& stack, const CScript& script, co
                     // Drop the signature, since there's no way for a signature to sign itself
                     scriptCode.FindAndDelete(CScript(vchSig));
 
-                    bool fSuccess = (!fStrictEncodings || (IsCanonicalSignature(vchSig) && IsCanonicalPubKey(vchPubKey)));
-                    if (fSuccess)
-                        fSuccess = CheckSig(vchSig, vchPubKey, scriptCode, txTo, nIn, nHashType, flags);
+                    bool fSuccess = IsCanonicalSignature(vchSig, flags) && IsCanonicalPubKey(vchPubKey, flags) &&
+                        CheckSig(vchSig, vchPubKey, scriptCode, txTo, nIn, nHashType, flags);
 
                     popstack(stack);
                     popstack(stack);
@@ -903,9 +912,8 @@ bool EvalScript(vector<vector<unsigned char> >& stack, const CScript& script, co
                         valtype& vchPubKey = stacktop(-ikey);
 
                         // Check signature
-                        bool fOk = (!fStrictEncodings || (IsCanonicalSignature(vchSig) && IsCanonicalPubKey(vchPubKey)));
-                        if (fOk)
-                            fOk = CheckSig(vchSig, vchPubKey, scriptCode, txTo, nIn, nHashType, flags);
+                        bool fOk = IsCanonicalSignature(vchSig, flags) && IsCanonicalPubKey(vchPubKey, flags) &&
+                            CheckSig(vchSig, vchPubKey, scriptCode, txTo, nIn, nHashType, flags);
 
                         if (fOk) {
                             isig++;
diff --git a/src/script.h b/src/script.h
@@ -32,9 +32,10 @@ enum
 enum
 {
     SCRIPT_VERIFY_NONE      = 0,
-    SCRIPT_VERIFY_P2SH      = (1U << 0),
-    SCRIPT_VERIFY_STRICTENC = (1U << 1),
-    SCRIPT_VERIFY_NOCACHE   = (1U << 2),
+    SCRIPT_VERIFY_P2SH      = (1U << 0), // evaluate P2SH (BIP16) subscripts
+    SCRIPT_VERIFY_STRICTENC = (1U << 1), // enforce strict conformance to DER and SEC2 for signatures and pubkeys
+    SCRIPT_VERIFY_EVEN_S    = (1U << 2), // enforce even S values in signatures (depends on STRICTENC)
+    SCRIPT_VERIFY_NOCACHE   = (1U << 3), // do not store results in signature cache (but do query it)
 };
 
 enum txnouttype
@@ -665,8 +666,8 @@ class CScriptCompressor
     }
 };
 
-bool IsCanonicalPubKey(const std::vector<unsigned char> &vchPubKey);
-bool IsCanonicalSignature(const std::vector<unsigned char> &vchSig);
+bool IsCanonicalPubKey(const std::vector<unsigned char> &vchPubKey, unsigned int flags);
+bool IsCanonicalSignature(const std::vector<unsigned char> &vchSig, unsigned int flags);
 
 bool EvalScript(std::vector<std::vector<unsigned char> >& stack, const CScript& script, const CTransaction& txTo, unsigned int nIn, unsigned int flags, int nHashType);
 bool Solver(const CScript& scriptPubKey, txnouttype& typeRet, std::vector<std::vector<unsigned char> >& vSolutionsRet);
diff --git a/src/test/canonical_tests.cpp b/src/test/canonical_tests.cpp
@@ -64,7 +64,7 @@ BOOST_AUTO_TEST_CASE(script_canon)
         string test = tv.get_str();
         if (IsHex(test)) {
             std::vector<unsigned char> sig = ParseHex(test);
-            BOOST_CHECK_MESSAGE(IsCanonicalSignature(sig), test);
+            BOOST_CHECK_MESSAGE(IsCanonicalSignature(sig, SCRIPT_VERIFY_STRICTENC), test);
             BOOST_CHECK_MESSAGE(IsCanonicalSignature_OpenSSL(sig), test);
         }
     }
@@ -78,7 +78,7 @@ BOOST_AUTO_TEST_CASE(script_noncanon)
         string test = tv.get_str();
         if (IsHex(test)) {
             std::vector<unsigned char> sig = ParseHex(test);
-            BOOST_CHECK_MESSAGE(!IsCanonicalSignature(sig), test);
+            BOOST_CHECK_MESSAGE(!IsCanonicalSignature(sig, SCRIPT_VERIFY_STRICTENC), test);
             BOOST_CHECK_MESSAGE(!IsCanonicalSignature_OpenSSL(sig), test);
         }
     }

Original file line number	Diff line number	Diff line change
`@@ -64,7 +64,7 @@ BOOST_AUTO_TEST_CASE(script_canon)`
`64`	`64`	`string test = tv.get_str();`
`65`	`65`	`if (IsHex(test)) {`
`66`	`66`	`std::vector<unsigned char> sig = ParseHex(test);`
`67`		`- BOOST_CHECK_MESSAGE(IsCanonicalSignature(sig), test);`
	`67`	`+ BOOST_CHECK_MESSAGE(IsCanonicalSignature(sig, SCRIPT_VERIFY_STRICTENC), test);`
`68`	`68`	`BOOST_CHECK_MESSAGE(IsCanonicalSignature_OpenSSL(sig), test);`
`69`	`69`	`}`
`70`	`70`	`}`
`@@ -78,7 +78,7 @@ BOOST_AUTO_TEST_CASE(script_noncanon)`
`78`	`78`	`string test = tv.get_str();`
`79`	`79`	`if (IsHex(test)) {`
`80`	`80`	`std::vector<unsigned char> sig = ParseHex(test);`
`81`		`- BOOST_CHECK_MESSAGE(!IsCanonicalSignature(sig), test);`
	`81`	`+ BOOST_CHECK_MESSAGE(!IsCanonicalSignature(sig, SCRIPT_VERIFY_STRICTENC), test);`
`82`	`82`	`BOOST_CHECK_MESSAGE(!IsCanonicalSignature_OpenSSL(sig), test);`
`83`	`83`	`}`
`84`	`84`	`}`