SCRIPT_VERIFY_STRICTENC and CHECKSIG NOT
6 messages, 6 Nov 2014. Started by Peter Todd.
Peter Todd (3), Pieter Wuille (2), Marius Hanne
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?
On Thu, Nov 06, 2014 at 05:38:20AM -0500, Peter Todd wrote:
Quoted text (9 lines)
> 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...
On Thu, Nov 6, 2014 at 2:38 AM, Peter Todd <pete@petertodd•org> wrote:
Quoted text (8 lines)
> 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).
Quoted text (4 lines)
> 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.
On Thu, Nov 6, 2014 at 2:47 AM, Pieter Wuille <pieter.wuille@gmail•com> wrote:
Quoted text (6 lines)
>> 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.
On Thu, Nov 06, 2014 at 02:47:29AM -0800, Pieter Wuille wrote:
Quoted text (12 lines)
> 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.
Quoted text (4 lines)
> 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());
}
Quoted text (6 lines)
> > 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.
On Thu, 6 Nov 2014 05:45:09 -0500 Peter Todd <pete@petertodd•org> wrote:
Quoted text (13 lines)
> 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?