[Policy] Reject SIGHASH_SINGLE with output out of bound

jl2012 commented at 3:13 pm on May 31, 2018: contributor

This makes using SIGHASH_SINGLE without a matching output non-standard. Signature of this form is insecure, as it commits to no output while users might think it commits to one. It is even worse in non-segwit scripts, which is effectively NOINPUT|NONE, so any UTXO of the same key could be stolen.

This is one of the earliest unintended consensus behavior which could be fixed with a softfork. The first step is to make it non-standard.

jl2012 force-pushed on May 31, 2018

MarcoFalke added the label TX fees and policy on May 31, 2018

in src/policy/policy.h:70 in 95c7e71314 outdated

65@@ -66,7 +66,8 @@ static constexpr unsigned int STANDARD_SCRIPT_VERIFY_FLAGS = MANDATORY_SCRIPT_VE
66                                                              SCRIPT_VERIFY_WITNESS |
67                                                              SCRIPT_VERIFY_DISCOURAGE_UPGRADABLE_WITNESS_PROGRAM |
68                                                              SCRIPT_VERIFY_WITNESS_PUBKEYTYPE |
69-                                                             SCRIPT_VERIFY_CONST_SCRIPTCODE;
70+                                                             SCRIPT_VERIFY_CONST_SCRIPTCODE |
71+                                                             SCRIPT_VERIFY_INSECURE_SINGLE;

instagibbs commented at 3:17 pm on May 31, 2018:

SCRIPT_VERIFY_SECURE_SIGHASH_SINGLE since it’s making sure it’s secure, not that it’s insecure, and to make it self-evident what it is?

jl2012 commented at 3:47 pm on May 31, 2018:

fixed

jl2012 force-pushed on May 31, 2018

in src/script/interpreter.h:151 in 146cf1296e outdated

147@@ -144,7 +148,7 @@ uint256 SignatureHash(const CScript& scriptCode, const T& txTo, unsigned int nIn
148 class BaseSignatureChecker
149 {
150 public:
151-    virtual bool CheckSig(const std::vector<unsigned char>& scriptSig, const std::vector<unsigned char>& vchPubKey, const CScript& scriptCode, SigVersion sigversion) const
152+    virtual bool CheckSig(const std::vector<unsigned char>& scriptSig, const std::vector<unsigned char>& vchPubKey, const CScript& scriptCode, SigVersion sigversion, const bool& no_insecure_single) const

promag commented at 11:43 pm on May 31, 2018:

Remove reference to const bool?

jl2012 commented at 8:32 pm on June 5, 2018:

fixed

jl2012 force-pushed on Jun 5, 2018

in src/script/sign.cpp:386 in afe08aa5c4 outdated

394@@ -395,7 +395,7 @@ class DummySignatureChecker final : public BaseSignatureChecker
395 {
396 public:
397     DummySignatureChecker() {}
398-    bool CheckSig(const std::vector<unsigned char>& scriptSig, const std::vector<unsigned char>& vchPubKey, const CScript& scriptCode, SigVersion sigversion) const override { return true; }
399+    bool CheckSig(const std::vector<unsigned char>& scriptSig, const std::vector<unsigned char>& vchPubKey, const CScript& scriptCode, SigVersion sigversion, const bool require_secure_single) const override { return true; }

Christewart commented at 0:14 am on June 6, 2018:

Why not just pass unsigned int flags into CheckSig? It seems like there might be flags in the future we want to check for as well, so why not bite the bullet and just pass in the flags?

Empact commented at 0:10 am on June 7, 2018:

I could go either way, but as is this avoids an internal dependency in CheckSig on flags’ implementation.

jl2012 commented at 7:20 am on July 13, 2018:

It is also used in sign.cpp so I’d like to avoid using flags

Christewart commented at 0:15 am on June 6, 2018: member

strong concept ACK.

in src/script/interpreter.cpp:1321 in afe08aa5c4 outdated

1317@@ -1318,6 +1318,8 @@ bool GenericTransactionSignatureChecker<T>::CheckSig(const std::vector<unsigned
1318     if (vchSig.empty())
1319         return false;
1320     int nHashType = vchSig.back();
1321+    if (((nHashType & 0x1f) == SIGHASH_SINGLE) && (nIn >= txTo->vout.size()) && require_secure_single)

Empact commented at 0:06 am on June 7, 2018:

nit: I would lead with require_secure_single since it’s a feature flipper

jl2012 commented at 2:05 am on June 7, 2018:

It is ranked by rarity. The use if SIGHASH_SINGLE is rare. If it is not used, the remaining conditions will not be inspected.

DrahtBot added the label Needs rebase on Jul 7, 2018

jl2012 force-pushed on Jul 12, 2018

[Policy] Reject SIGHASH_SINGLE with output out of bound 04848bbf95

jl2012 force-pushed on Jul 12, 2018

DrahtBot removed the label Needs rebase on Jul 13, 2018

jl2012 commented at 7:20 am on July 13, 2018: contributor

rebased

DrahtBot commented at 6:51 pm on September 7, 2018: member

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#13533 ([tests] Reduced number of validations in tx_validationcache_tests by lucash-dev)
#13266 ([moveonly] privatize SignatureExtractorChecker by Empact)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

DrahtBot closed this on Sep 7, 2018

DrahtBot reopened this on Sep 7, 2018

in src/script/interpreter.h:151 in 04848bbf95

147@@ -144,7 +148,7 @@ uint256 SignatureHash(const CScript& scriptCode, const T& txTo, unsigned int nIn
148 class BaseSignatureChecker
149 {
150 public:
151-    virtual bool CheckSig(const std::vector<unsigned char>& scriptSig, const std::vector<unsigned char>& vchPubKey, const CScript& scriptCode, SigVersion sigversion) const
152+    virtual bool CheckSig(const std::vector<unsigned char>& scriptSig, const std::vector<unsigned char>& vchPubKey, const CScript& scriptCode, SigVersion sigversion, const bool require_secure_single) const

practicalswift commented at 5:35 am on September 26, 2018:

~~Should require_secure_single be an unnamed parameter since it is not used?~~

jl2012 commented at 5:56 am on September 26, 2018:

Just for consistency?

NicolasDorier commented at 10:10 am on September 26, 2018: contributor

which is effectively NOINPUT|NONE, so any UTXO of the same key could be stolen.

~Just being pedantic (and maybe out of topic), but NOINPUT|NONE is not equivalent to giving a key. NOINPUT|NONE works well with OP_CODESEPARATOR to select which path of the script the signer wants to execute.~

EDIT:

I’m wrong, NOINPUT says scriptCode of the input is set to an empty script.

jl2012 commented at 3:28 pm on February 5, 2019: contributor

This patch must be used with the NULLFAIL rule. Otherwise, it would be a hardfork as one may use a script CHECKSIG NOT with an out-of-bound SIGHASH_SINGLE signature. The script will fail before this patch, and pass after this patch.

NULLFAIL will fix the problem as the script will fail after the patch, due to the use of invalid signature

Another option is to throw an exception, instead of return false in GenericTransactionSignatureChecker(). It will be reported as SCRIPT_ERR_UNKNOWN_ERROR, unless #15074 is adopted.

DrahtBot closed this on May 7, 2019

DrahtBot commented at 5:07 pm on May 7, 2019: member

DrahtBot reopened this on May 7, 2019

DrahtBot commented at 12:52 pm on October 2, 2019: member

DrahtBot added the label Needs rebase on Oct 2, 2019

adamjonas commented at 2:14 pm on May 2, 2020: member

@jl2012 Rebased after #13266 and #18422 at https://github.com/bitcoin/bitcoin/compare/master...adamjonas:pr/13360 if there is interest in continuing to work on this.

MarcoFalke added the label Up for grabs on Aug 13, 2020

MarcoFalke commented at 7:15 am on August 13, 2020: member

Closing for now. Let me know when you want to work on this again

MarcoFalke closed this on Aug 13, 2020

DrahtBot locked this on Feb 15, 2022

[Policy] Reject SIGHASH_SINGLE with output out of bound #13360

Conflicts