[Bitcoin-development] SCRIPT_VERIFY_STRICTENC and CHECKSIG NOT

[Bitcoin-development] SCRIPT_VERIFY_STRICTENC and CHECKSIG NOT

[Peter Todd]

[-- 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 --]

Re: [Bitcoin-development] SCRIPT_VERIFY_STRICTENC and CHECKSIG NOT

[Peter Todd]

[-- 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 --]

Re: [Bitcoin-development] SCRIPT_VERIFY_STRICTENC and CHECKSIG NOT

[Marius Hanne]

[-- 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 --]

Re: [Bitcoin-development] SCRIPT_VERIFY_STRICTENC and CHECKSIG NOT

[Pieter Wuille]

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

Re: [Bitcoin-development] SCRIPT_VERIFY_STRICTENC and CHECKSIG NOT

[Pieter Wuille]

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

Re: [Bitcoin-development] SCRIPT_VERIFY_STRICTENC and CHECKSIG NOT

[Peter Todd]

[-- 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 --]