From a81cd96805ce6b65cca3a40ebbd3b2eb428abb7b Mon Sep 17 00:00:00 2001 From: Pieter Wuille Date: Wed, 26 Dec 2012 21:10:49 +0100 Subject: [PATCH] Only create signatures with even S, and verification mode to check. 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. --- src/key.cpp | 19 ++++++++++++++++++- src/script.cpp | 26 +++++++++++++++++--------- src/script.h | 11 ++++++----- src/test/canonical_tests.cpp | 4 ++-- 4 files changed, 43 insertions(+), 17 deletions(-) diff --git a/src/key.cpp b/src/key.cpp index 1ab4c62eb..2247c9947 100644 --- a/src/key.cpp +++ b/src/key.cpp @@ -194,9 +194,26 @@ public: } bool Sign(const uint256 &hash, std::vector& 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 index 5699fbfb6..2df2e9f0d 100644 --- 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> <02> <02> // 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 >& 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 >& 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 >& 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 index 03afe8b65..fd5b12921 100644 --- 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 @@ public: } }; -bool IsCanonicalPubKey(const std::vector &vchPubKey); -bool IsCanonicalSignature(const std::vector &vchSig); +bool IsCanonicalPubKey(const std::vector &vchPubKey, unsigned int flags); +bool IsCanonicalSignature(const std::vector &vchSig, unsigned int flags); bool EvalScript(std::vector >& stack, const CScript& script, const CTransaction& txTo, unsigned int nIn, unsigned int flags, int nHashType); bool Solver(const CScript& scriptPubKey, txnouttype& typeRet, std::vector >& vSolutionsRet); diff --git a/src/test/canonical_tests.cpp b/src/test/canonical_tests.cpp index 42d21f8ac..09988da25 100644 --- 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 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 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); } }