Attack by modifying non-segwit transactions after segwit is accepted ?

In a previous thread ("New BIP: Dealing with OP_IF and OP_NOTIF malleability in P2WSH") it was briefly discussed what happens if someone modifies segwit data during transmission. I think the discussion should continue.

What worries me is what happens with non-segwit transactions after segwit is activated. I've followed the code from transaction arrival to transaction relay and it seems that a malicious node could receive a non-segwit tx, and re-format it into a segwit tx having as high as 400 Kbytes of segwit witness program data, and then relay it. Both transaction would have the same hash.

The MAX_SCRIPT_ELEMENT_SIZE limit is only enforced on segwit execution, not in old non-segwit execution, so witness program stack elements could be as large as 400 Kbytes (MAX_STANDARD_TX_WEIGHT prevents increasing more). Such large modified transaction will probably not be properly relayed by the network due too low fee/byte, so the honest miner will probably win and forward the original transaction through the network. But if the attacker has better connectivity with the network and he modifies the original transaction adding segwit witness program data only up to the point where the transaction is relayed but miners are discouraged to include it in blocks due to low fees/byte, then the attacker has successfully prevented a transaction from being mined (or at least it will take much more).

Also an attacker can encode arbitrary data (such as virus signatures or illegal content) into passing non-segwit transactions.

One solution would be to increase the transaction version to 3 for segwit transactions, so a non-segwit transaction cannot be converted into a segwit transaction without changing the transaction hash. But this seems not to be a good solution, because it does not solve all the problems. Transactions having a mixture of segwit and non-segwit inputs could suffer the same attack (even if they are version 3).

I proposed that a rule is added to IsStandardTX() that prevents witness programs of having a stack elements of length greater than MAX_SCRIPT_ELEMENT_SIZE. (currently this is not a rule)

That's a simple check that prevents most of the problems.

A long term solution would be to add the maximum size of the witness stack in bytes (maxWitnessSize) as a field for each input, or as a field of the whole transaction.

Regards

Adding witness data to a non-segwit script is invalid by consensus:

https://github.com/bitcoin/bitcoin/blob/d612837814020ae832499d18e6ee5eb919a87907/src/script/interpreter.cpp#L1467

This PR will detect such violation early and ban the peer:

https://github.com/bitcoin/bitcoin/pull/8499

Another approach is to run the scripts of all incoming transactions. That's not too bad as you have already fetched the utxos which is a major part of validation.

Because there was a discussion on reddit about this topic, I want to clarify that Johnson Lau explained how a check in the code prevents this attack. So there is no real attack.

Also note that the subject of this thread has a question mark, which means that I'm asking the community for clarification, not asserting the existence of a vulnerability.

The segwit code is complex, and some key parts of the consensus code are spread over the source files (such as state.CorruptionPossible() relation to DoS banning, IsNull() check in witness program serialization, etc.).

Thanks again Johnson for your clarifications.

On Wed, Aug 24, 2016 at 10:49 PM, Johnson Lau <jl2012@xbt•hk> wrote:

> Adding witness data to a non-segwit script is invalid by consensus:
>
> https://github.com/bitcoin/bitcoin/blob/d612837814020ae832499d18e6ee5e
> b919a87907/src/script/interpreter.cpp#L1467
>
>
> This PR will detect such violation early and ban the peer:
>
> https://github.com/bitcoin/bitcoin/pull/8499
>
>
> Another approach is to run the scripts of all incoming transactions.
> That's not too bad as you have already fetched the utxos which is a major
> part of validation.
>

Thank you so much for taking time to actually review the codes. I hope you will keep raising questions when you feel something might be wrong. This is how things supposed to work and we should not be affected by some forum discussions.

> On August 26, 2016 at 9:16 AM Sergio Demian Lerner <sergio.d.lerner@gmail•com> wrote:
>
>     Because there was a discussion on reddit about this topic, I want to clarify that Johnson Lau explained how a check in the code prevents this attack.
>     So there is no real attack.
>
>     Also note that the subject of this thread has a question mark, which means that I'm asking the community for clarification, not asserting the existence of a vulnerability.
>
>     The segwit code is complex, and some key parts of the consensus code are spread over the source files (such as state.CorruptionPossible() relation to DoS banning, IsNull() check in witness program serialization, etc.).
>
>     Thanks again Johnson for your clarifications.
>
>
>     On Wed, Aug 24, 2016 at 10:49 PM, Johnson Lau <jl2012@xbt•hk mailto:jl2012@xbt.hk > wrote:
>
>         > >
> >         Adding witness data to a non-segwit script is invalid by consensus:
> >
> >         https://github.com/bitcoin/bitcoin/blob/d612837814020ae832499d18e6ee5eb919a87907/src/script/interpreter.cpp#L1467 https://github.com/bitcoin/bitcoin/blob/d612837814020ae832499d18e6ee5eb919a87907/src/script/interpreter.cpp#L1467
> >
> >
> >         This PR will detect such violation early and ban the peer:
> >
> >         https://github.com/bitcoin/bitcoin/pull/8499 https://github.com/bitcoin/bitcoin/pull/8499
> >
> >
> >
> >
> >         Another approach is to run the scripts of all incoming transactions. That's not too bad as you have already fetched the utxos which is a major part of validation.
> >
> >     >
>