[Policy] Reject SIGHASH_SINGLE with output out of bound by jl2012 · Pull Request #13360 · bitcoin/bitcoin

jl2012 · 2018-05-31T15:13:31Z

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.

instagibbs · 2018-05-31T15:17:06Z

src/policy/policy.h

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?

promag · 2018-05-31T23:43:18Z

src/script/interpreter.h

Remove reference to const bool?

Christewart · 2018-06-06T00:14:16Z

src/script/sign.cpp

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?

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

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

Christewart · 2018-06-06T00:15:09Z

strong concept ACK.

Empact · 2018-06-07T00:06:53Z

src/script/interpreter.cpp

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

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

jl2012 · 2018-07-13T07:20:54Z

rebased

DrahtBot · 2018-09-07T18:51:16Z

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.

practicalswift · 2018-09-26T05:35:45Z

src/script/interpreter.h

 {
 public:
-    virtual bool CheckSig(const std::vector<unsigned char>& scriptSig, const std::vector<unsigned char>& vchPubKey, const CScript& scriptCode, SigVersion sigversion) const
+    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


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

Just for consistency?

NicolasDorier · 2018-09-26T10:10:48Z

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 · 2019-02-05T15:28:25Z

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 · 2019-05-07T17:07:07Z

The last travis run for this pull request was 241 days ago and is thus outdated. To trigger a fresh travis build, this pull request should be closed and re-opened.

DrahtBot · 2019-10-02T12:52:50Z

Needs rebase

adamjonas · 2020-05-02T14:14:00Z

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

maflcko · 2020-08-13T07:15:10Z

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

jl2012 force-pushed the insecure_single branch from fee7697 to 95c7e71 Compare May 31, 2018 15:15

maflcko added the TX fees and policy label May 31, 2018

instagibbs reviewed May 31, 2018

View reviewed changes

jl2012 force-pushed the insecure_single branch 2 times, most recently from 2ce4d80 to 146cf12 Compare May 31, 2018 16:13

promag reviewed May 31, 2018

View reviewed changes

jl2012 force-pushed the insecure_single branch from 146cf12 to afe08aa Compare June 5, 2018 20:32

Christewart reviewed Jun 6, 2018

View reviewed changes

Empact reviewed Jun 7, 2018

View reviewed changes

DrahtBot mentioned this pull request Jun 14, 2018

Moving final scriptSig construction from CombineSignatures to ProduceSignature (PSBT signer logic) #13425

Merged

This was referenced Jun 26, 2018

[tests] Reduced number of validations in tx_validationcache_tests #13533

Merged

BIP 174 PSBT Serializations and RPCs #13557

Merged

DrahtBot added the Needs rebase label Jul 7, 2018

jl2012 force-pushed the insecure_single branch from afe08aa to 62fe63e Compare July 12, 2018 20:16

[Policy] Reject SIGHASH_SINGLE with output out of bound

04848bb

jl2012 force-pushed the insecure_single branch from 62fe63e to 04848bb Compare July 12, 2018 20:57

DrahtBot removed the Needs rebase label Jul 13, 2018

DrahtBot mentioned this pull request Aug 27, 2018

Implement sighash cache in CHECKMULTISIG #14079

Closed

DrahtBot mentioned this pull request Sep 7, 2018

refactor: privatize SignatureExtractorChecker [moveonly] #13266

Merged

DrahtBot closed this Sep 7, 2018

DrahtBot reopened this Sep 7, 2018

practicalswift reviewed Sep 26, 2018

View reviewed changes

DrahtBot mentioned this pull request Sep 27, 2018

[refactoring] Create common interface for CMutableTransaction and CTransaction with static polymorphism #14337

Closed

DrahtBot closed this May 7, 2019

DrahtBot reopened this May 7, 2019

DrahtBot added the Needs rebase label Oct 2, 2019

corollari mentioned this pull request Apr 2, 2020

Add fix to SIGHASH_SINGLE bug TheBlueMatt/bips#8

Closed

maflcko added the Up for grabs label Aug 13, 2020

maflcko closed this Aug 13, 2020

bitcoin locked as resolved and limited conversation to collaborators Feb 15, 2022

Conversation

jl2012 commented May 31, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Christewart commented Jun 6, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jl2012 commented Jul 13, 2018

Uh oh!

DrahtBot commented Sep 7, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Conflicts

Uh oh!

practicalswift Sep 26, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

NicolasDorier commented Sep 26, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jl2012 commented Feb 5, 2019

Uh oh!

DrahtBot commented May 7, 2019

Uh oh!

DrahtBot commented Oct 2, 2019

Uh oh!

adamjonas commented May 2, 2020

Uh oh!

maflcko commented Aug 13, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

10 participants

DrahtBot commented Sep 7, 2018 •

edited

Loading

practicalswift Sep 26, 2018 •

edited

Loading

NicolasDorier commented Sep 26, 2018 •

edited

Loading