* [Bitcoin-development] SCRIPT_VERIFY_STRICTENC and CHECKSIG NOT @ 2014-11-06 10:38 Peter Todd 2014-11-06 10:45 ` Peter Todd 2014-11-06 10:47 ` Pieter Wuille 0 siblings, 2 replies; 6+ messages in thread From: Peter Todd @ 2014-11-06 10:38 UTC (permalink / raw) To: Bitcoin Dev [-- Attachment #1: Type: text/plain, Size: 1807 bytes --] So right now git head will accept the following invalid transaction into the mempool: 0100000001140de229e08fda25cbc16ded2618cdacce49fcb18c0b6ccdace00040909adae4000000009000493046022100f7828d81c849c5448ba5ba4ef55df6b4d0ba3ae3f1a59cff3291880c2c8e524f022100d2f5bc9dc2f0674eded31023cb47e61a596e10f8f1ddd44cf92d290c9db577c70144410778d430274f8c5ec1321338151e9f27f4c676a008bdf8638d07c0b6be9ab35c71a1518063243acd4dfe96b66e3f2ec8013c8e072cd09b3834a19f81f659cc3455ac91ffffffff01102700000000000017a914e661a2229cc824329c9409f49d99cb5ac350c9288700000000 which spends the redeemScript: 0778d430274f8c5ec1321338151e9f27f4c676a008bdf8638d07c0b6be9ab35c71a1518063243acd4dfe96b66e3f2ec8013c8e072cd09b3834a19f81f659cc3455 CHECKSIG NOT That pubkey is valid and accepted by OpenSSL as it's obscure "hybrid" format. The transaction is invalid because the signature is correct, causing CHECKSIG to return 1, which is inverted to 0 by the NOT. However the implementation of the STRICTENC flag simply makes pubkey formats it doesn't recognize act as through the signature was invalid, rather than failing the transaction. Similar to the invalid due to too many sigops DoS attack I found before, this lets you fill up the mempool with garbage transactions that will never be mined. OTOH I don't see any way to exploit this in a v0.9.x IsStandard() transaction, so we haven't shipped code that actually has this vulnerability. (dunno about alt-implementations) I suggest we either change STRICTENC to simply fail unrecognized pubkeys immediately - similar to how non-standard signatures are treated - or fail the script if the pubkey is non-standard and signature verification succeeds. Thoughts? -- 'peter'[:-1]@petertodd.org 0000000000000000152dc55f27338b58325f0432d2dc6edb90c8d449d9959583 [-- Attachment #2: Digital signature --] [-- Type: application/pgp-signature, Size: 650 bytes --] ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [Bitcoin-development] SCRIPT_VERIFY_STRICTENC and CHECKSIG NOT 2014-11-06 10:38 [Bitcoin-development] SCRIPT_VERIFY_STRICTENC and CHECKSIG NOT Peter Todd @ 2014-11-06 10:45 ` Peter Todd 2014-11-06 12:39 ` Marius Hanne 2014-11-06 10:47 ` Pieter Wuille 1 sibling, 1 reply; 6+ messages in thread From: Peter Todd @ 2014-11-06 10:45 UTC (permalink / raw) To: Bitcoin Dev [-- Attachment #1: Type: text/plain, Size: 972 bytes --] On Thu, Nov 06, 2014 at 05:38:20AM -0500, Peter Todd wrote: > So right now git head will accept the following invalid transaction into > the mempool: > > 0100000001140de229e08fda25cbc16ded2618cdacce49fcb18c0b6ccdace00040909adae4000000009000493046022100f7828d81c849c5448ba5ba4ef55df6b4d0ba3ae3f1a59cff3291880c2c8e524f022100d2f5bc9dc2f0674eded31023cb47e61a596e10f8f1ddd44cf92d290c9db577c70144410778d430274f8c5ec1321338151e9f27f4c676a008bdf8638d07c0b6be9ab35c71a1518063243acd4dfe96b66e3f2ec8013c8e072cd09b3834a19f81f659cc3455ac91ffffffff01102700000000000017a914e661a2229cc824329c9409f49d99cb5ac350c9288700000000 > > which spends the redeemScript: > > 0778d430274f8c5ec1321338151e9f27f4c676a008bdf8638d07c0b6be9ab35c71a1518063243acd4dfe96b66e3f2ec8013c8e072cd09b3834a19f81f659cc3455 > CHECKSIG NOT ...and while we're at it, bitcoin-ruby's forked yet again... -- 'peter'[:-1]@petertodd.org 0000000000000000152dc55f27338b58325f0432d2dc6edb90c8d449d9959583 [-- Attachment #2: Digital signature --] [-- Type: application/pgp-signature, Size: 650 bytes --] ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [Bitcoin-development] SCRIPT_VERIFY_STRICTENC and CHECKSIG NOT 2014-11-06 10:45 ` Peter Todd @ 2014-11-06 12:39 ` Marius Hanne 0 siblings, 0 replies; 6+ messages in thread From: Marius Hanne @ 2014-11-06 12:39 UTC (permalink / raw) To: bitcoin-development [-- Attachment #1: Type: text/plain, Size: 1198 bytes --] On Thu, 6 Nov 2014 05:45:09 -0500 Peter Todd <pete@petertodd•org> wrote: > On Thu, Nov 06, 2014 at 05:38:20AM -0500, Peter Todd wrote: > > So right now git head will accept the following invalid transaction > > into the mempool: > > > > 0100000001140de229e08fda25cbc16ded2618cdacce49fcb18c0b6ccdace00040909adae4000000009000493046022100f7828d81c849c5448ba5ba4ef55df6b4d0ba3ae3f1a59cff3291880c2c8e524f022100d2f5bc9dc2f0674eded31023cb47e61a596e10f8f1ddd44cf92d290c9db577c70144410778d430274f8c5ec1321338151e9f27f4c676a008bdf8638d07c0b6be9ab35c71a1518063243acd4dfe96b66e3f2ec8013c8e072cd09b3834a19f81f659cc3455ac91ffffffff01102700000000000017a914e661a2229cc824329c9409f49d99cb5ac350c9288700000000 > > > > which spends the redeemScript: > > > > 0778d430274f8c5ec1321338151e9f27f4c676a008bdf8638d07c0b6be9ab35c71a1518063243acd4dfe96b66e3f2ec8013c8e072cd09b3834a19f81f659cc3455 > > CHECKSIG NOT > > ...and while we're at it, bitcoin-ruby's forked yet again... > It is? How do you reckon? The webbtc node just received a block: http://webbtc.com/block/0000000000000000006370724f73798f4c00c8da97f675c4dcf4605e9882913c You mean it would be forked off if this change was released? [-- Attachment #2: OpenPGP digital signature --] [-- Type: application/pgp-signature, Size: 181 bytes --] ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [Bitcoin-development] SCRIPT_VERIFY_STRICTENC and CHECKSIG NOT 2014-11-06 10:38 [Bitcoin-development] SCRIPT_VERIFY_STRICTENC and CHECKSIG NOT Peter Todd 2014-11-06 10:45 ` Peter Todd @ 2014-11-06 10:47 ` Pieter Wuille 2014-11-06 10:51 ` Pieter Wuille 2014-11-06 11:04 ` Peter Todd 1 sibling, 2 replies; 6+ messages in thread From: Pieter Wuille @ 2014-11-06 10:47 UTC (permalink / raw) To: Peter Todd; +Cc: Bitcoin Dev On Thu, Nov 6, 2014 at 2:38 AM, Peter Todd <pete@petertodd•org> wrote: > However the implementation of the STRICTENC flag simply makes pubkey > formats it doesn't recognize act as through the signature was invalid, > rather than failing the transaction. Similar to the invalid due to too > many sigops DoS attack I found before, this lets you fill up the mempool > with garbage transactions that will never be mined. OTOH I don't see any > way to exploit this in a v0.9.x IsStandard() transaction, so we haven't > shipped code that actually has this vulnerability. (dunno about > alt-implementations) Yeah, there's even a comment in script/interpreter.h currently about how STRICTENC is not softfork safe. I didn't realize that this would lead to the mempool accepting invalid transactions (I thought there was a second validity check with the actual consensus rules; if not, maybe we need to add that). > I suggest we either change STRICTENC to simply fail unrecognized pubkeys > immediately - similar to how non-standard signatures are treated - or > fail the script if the pubkey is non-standard and signature verification > succeeds. Sounds good to me, I disliked those semantics too. -- Pieter ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [Bitcoin-development] SCRIPT_VERIFY_STRICTENC and CHECKSIG NOT 2014-11-06 10:47 ` Pieter Wuille @ 2014-11-06 10:51 ` Pieter Wuille 2014-11-06 11:04 ` Peter Todd 1 sibling, 0 replies; 6+ messages in thread From: Pieter Wuille @ 2014-11-06 10:51 UTC (permalink / raw) To: Peter Todd; +Cc: Bitcoin Dev On Thu, Nov 6, 2014 at 2:47 AM, Pieter Wuille <pieter.wuille@gmail•com> wrote: >> I suggest we either change STRICTENC to simply fail unrecognized pubkeys >> immediately - similar to how non-standard signatures are treated - or >> fail the script if the pubkey is non-standard and signature verification >> succeeds. > > Sounds good to me, I disliked those semantics too. Of course: do we apply this rule to all pubkeys passed to CHECKMULTISIG (my preference...), or just the ones that are otherwise checked? This will likely make existing outputs hard to spend as well (I don't have numbers), are we okay with that? We probably can't make this a consensus rule, as it may make existing P2SH outputs/addresses unspendable. -- Pieter ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [Bitcoin-development] SCRIPT_VERIFY_STRICTENC and CHECKSIG NOT 2014-11-06 10:47 ` Pieter Wuille 2014-11-06 10:51 ` Pieter Wuille @ 2014-11-06 11:04 ` Peter Todd 1 sibling, 0 replies; 6+ messages in thread From: Peter Todd @ 2014-11-06 11:04 UTC (permalink / raw) To: Pieter Wuille; +Cc: Bitcoin Dev [-- Attachment #1: Type: text/plain, Size: 2831 bytes --] On Thu, Nov 06, 2014 at 02:47:29AM -0800, Pieter Wuille wrote: > On Thu, Nov 6, 2014 at 2:38 AM, Peter Todd <pete@petertodd•org> wrote: > > However the implementation of the STRICTENC flag simply makes pubkey > > formats it doesn't recognize act as through the signature was invalid, > > rather than failing the transaction. Similar to the invalid due to too > > many sigops DoS attack I found before, this lets you fill up the mempool > > with garbage transactions that will never be mined. OTOH I don't see any > > way to exploit this in a v0.9.x IsStandard() transaction, so we haven't > > shipped code that actually has this vulnerability. (dunno about > > alt-implementations) > > Yeah, there's even a comment in script/interpreter.h currently about > how STRICTENC is not softfork safe. Indeed. I actually was thinking about SCRIPT_VERIFY_MINIMALDATA, CScript(), and FindAndDelete() Specifically that if you were to change CScript() to convert single-character PUSHDATA's to OP_<number> you'd be making a consensus-critical change due to how FindAndDelete() is called with a a CScript() signature. You didn't make that mistake, and I couldn't find a way to exploit it anyway, but it reminded me of this STRICTENC stuff. > I didn't realize that this would > lead to the mempool accepting invalid transactions (I thought there > was a second validity check with the actual consensus rules; if not, > maybe we need to add that). It should be enough to just duplicate the CheckInputs() call in the AcceptToMemoryPool() function: if (!CheckInputs(tx, state, view, true, STANDARD_SCRIPT_VERIFY_FLAGS, true)) { return error("AcceptToMemoryPool: : ConnectInputs failed %s", hash.ToString()); } if (!CheckInputs(tx, state, view, true, MANDATORY_SCRIPT_VERIFY_FLAGS, true)) { return error("AcceptToMemoryPool: : BUG FOUND Standard verify flags passed yet mandatory flags failed. %s", hash.ToString()); } > > I suggest we either change STRICTENC to simply fail unrecognized pubkeys > > immediately - similar to how non-standard signatures are treated - or > > fail the script if the pubkey is non-standard and signature verification > > succeeds. > > Sounds good to me, I disliked those semantics too. Ok, then given we have to support hybrid encoding for awhile longer anyway - I noticed your secp256k1 library supports it - lets do the latter as a "least invasive" measure. I can't think of any case where that'd be triggered other than delibrately. Doing that should make STRICTENC a soft-fork-safe change, and we can decide at a later date if we want to get rid of hybrid-encoded pubkeys in a further tightening of the rules. -- 'peter'[:-1]@petertodd.org 000000000000000019b3c625f667bd0b93011c0a37950545a6a8fccf0b08ae73 [-- Attachment #2: Digital signature --] [-- Type: application/pgp-signature, Size: 650 bytes --] ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2014-11-06 12:58 UTC | newest] Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2014-11-06 10:38 [Bitcoin-development] SCRIPT_VERIFY_STRICTENC and CHECKSIG NOT Peter Todd 2014-11-06 10:45 ` Peter Todd 2014-11-06 12:39 ` Marius Hanne 2014-11-06 10:47 ` Pieter Wuille 2014-11-06 10:51 ` Pieter Wuille 2014-11-06 11:04 ` Peter Todd
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox