Replay protection fix

[grarco]

Describe your changes

Closes #1530.
Closes #1683.

This PR includes the signature on the raw transaction header made by the inner tx signer(s) and the corresponding validation in the VPs.

It also fixes the replay protection issue but in a different way than the one discussed in #1530. There we stated that the problem was that using the raw transaction header to avoid replaying the inner transaction was flawed because the hash of this header can be trivially modified by changing some of its field. This problem is actually solved if we force a signature on the raw header which cannot be modified anymore. So, to summarize:

• The signer(s) of the inner transaction sign the Raw transaction header
• We keep the current implementation of replay protection based on two hashes:
  • Hash of the Wrapper header
  • Hash of the Raw header
• Given that the header contains the commitments to Data and Code these sections are not signed anymore (we sign the header with the commitments and than check the validity of these commitments)
• Masp section doesn't need to be replay protected nor signed and a commitment to it is present in the Data section
• ExtraData section doesn't need to be replay protected. A commitment to this optional section is present in Data
• Since neither the Masp nor the ExtraData sections are mentioned in the header, adding, modifying or removing these sections wouldn't change the hash used for replay protection (but would still invalidate the commitments). So no malleability attack of these section can deceive the replay protection mechanism (of course the real signers of the tx can always produce valid transactions with different Masp or ExtraData sections simply by changing the hash of the header)
• The only problem could be in case a transaction carried more than one Data or Code section. In this case, since we don't sign these sections directly anymore, malleability attacks could be mounted on the transaction. Still, this is a use case which we are not considering at the moment, and, in case, I suspect that we should add commitments for each of these sections in the header. The signatures alone, indeed, would not prevent a malicious user from modifying these sections. By adding these commitments, the safety of the transaction should be guaranteed.

Also, given that the header of both the inner and wrapper tx must match (expect for the TxType), this PR removes some checks on the raw header hash that don't make sense anymore, together with the corresponding error codes and some unit tests.

Indicate on which release or other PRs this topic is based on

v0.23.0

Checklist before merging to draft

• I have added a changelog
• Git history is in acceptable state

[murisi]

Since neither the Masp nor the ExtraData sections are mentioned in the header, adding, modifying or removing these sections wouldn't change the hash used for replay protection (but would still invalidate the commitments). So no malleability attack of these section can deceive the replay protection mechanism.

Hi @grarco , thanks for looking into the replay protection issues. If the Masp, ExtraData, or inner Signature section is tampered with, when would it be detected that the commitments have been invalidated? Before the gas fee is deducted from the wrapper signer or after? I am thinking back to the issue you brought up here: #1567 .

[murisi]

Also, given that the hardware wallet code is being frozen for auditing, is there a way to solve some of these issues without modifying the transaction format or the signing algorithm? Like, could we not somehow store the hash of the signature section over the wrapper, and the hash of the signature section over (data, code, MASP, extras) (as returned by verify_signatures) in replay storage? cc @adrianbrink

[grarco]

Since neither the Masp nor the ExtraData sections are mentioned in the header, adding, modifying or removing these sections wouldn't change the hash used for replay protection (but would still invalidate the commitments). So no malleability attack of these section can deceive the replay protection mechanism.

Hi @grarco , thanks for looking into the replay protection issues. If the Masp, ExtraData, or inner Signature section is tampered with, when would it be detected that the commitments have been invalidated? Before the gas fee is deducted from the wrapper signer or after? I am thinking back to the issue you brought up here: #1567 .

So if the tampering happens directly on the encrypted sections, it's discovered while validating the wrapper transaction, so the tx is discarded and no fees are paid. This is because the wrapper signer still signs over all of the sections. If instead the tampering happens on the sections before they are encrypted, it gets discovered when we run the decrypted transaction, so after the fees have been paid. But this scenario can only happen if:

• The tampering has been performed by the wrapper signer themselves
• The tampering happened on the communication channel between the wrapper signer and the entity that produced the inner tx, still the wrapper signer is responsible for checking the validity of the received message

So fee payment looks correct to me in this case

[grarco]

Also, given that the hardware wallet code is being frozen for auditing, is there a way to solve some of these issues without modifying the transaction format or the signing algorithm? Like, could we not somehow store the hash of the signature section over the wrapper, and the hash of the signature section over (data, code, MASP, extras) (as returned by verify_signatures) in replay storage? cc @adrianbrink

I see, so this PR, if correct, I think proposes the least effort (in terms signatures effort) but it requires changing the Signature section format (and therefore the tx format) and also the signing algorithm. This is because the PR aims at closing #1683 too, it actually comes down to collapsing this and #1530 to the same problem, the lack of a signature on the Header coming from the inner tx signer.

The Signature struct has been changed in a single field, the targets field which has been turned from a Vec to a single Hash: in theory we can bring back the Vec without any problem (it will simply be a vector of one element).

For the signing algorithm we could do more or less the same, just revert it to the previous version's implementation (expecting an array of hashes) even though we'll provide only one hash.

If instead we are somehow bounded to the specific section's hashes that we pass to a signature, than of course non of this can be applied and we should probably stick to the solution proposed in #1530 (lexicographically sorted concatenation of sections' hashes), though this would not solve in any way the missing signature on the header

[Fraccaman]

is this mergeable in 0.24.0?

[murisi]

Looks good to me, thanks!

Oops, just realized that there will be issue with CompressedSignature section that will break web client.

[murisi]

Looks good to me. There's just a minor issue with CompressedSignature expansion.

[murisi]

Is this file supposed to be here?

[grarco]

Ah definitely not 👍🏻

[murisi]

CompressedSignature is the compressed form of a Signature returned by the hardware wallet. The software client expands it before sending it off to the protocol. Here, line 630 always expands the 0th index to the header_hash(). So how can we allow the hardware wallet to specify that a signature is over the raw_header_hash()? I'm thinking that we can use the sentinel index 255 to specify tx.raw_header_hash(). Like:

} else if idx == 255 {
    /// The 255th section is the raw header
    targets.push(tx.raw_header_hash());
} else {

[grarco]

Nice catch! Yes I think this solution works fine