* [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: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
* 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
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