cosmos-sdk
cosmos-sdk copied to clipboard
cosmos
[Bug]: remove amino support in unordered txs
Is there an existing issue for this?
- [X] I have searched the existing issues
What happened?
Due to the limitations of amino signing we need to remove the ability to sign messages that use the new unordered field in the tx from being signed with amino.
Cosmos SDK Version
main
How to reproduce?
No response
It's important that we understand also if there's a possibility for which given:
tx.TxRaw{auth_info_bytes, body_info_bytes, signatures} it is not possible to have more than one valid encodings for this structure, otherwise it would yield two tx hashes for the same Tx, which breaks security assumptions of unordered Txs (opens replayability exploits).
This is related to ADR-027, and should be investigated.
@alexanderbez in theory it should, but @aaronc was mentioning about some possible edge cases.
We should check the history
We can either block in x/tx in the decode tx function if unrodered is set to true and in the authinfo the user is signing with amino we should return an error or check this in the ante handler what is the signing format
https://github.com/cosmos/cosmos-sdk/blob/main/x/tx/decode/decode.go#L79
Keep in mind that amino isn't the only malleability. Signatures themselves could be malleable
Right, I'm still confused as to whether or not this is safe. From what I've been told, ADR-027 indicates decoding should be safe.
Yes, but ADR-027 can't control whether a third party intercepts a tx, mutates a signature - say via some malleability in ASN1 - and rebroadcasts it. We would need to study each supported signature format carefully to make sure they are deterministic and all bets are off is an app supports custom signature algorithms.
well in that case, it seems like if signatures are part of the decoded tx, then idk how we will/can support unordered txs.
I mean the first step is to study the signatures a bit more to see if what I'm saying is true, at least some of the time. But, yes assuming it is true, I'm not sure there's any way this particular approach could be made safe.
Which signature is using ASN1 encoding?
Pre-launch of the Cosmos Hub I at least tried to ensure our secp signature has no malleability. (no negative s field, or zero points). Tony Arcieri was very careful in ensuring we did not use ASN-1 anywhere in our codebases.
But we used a direct 64 or 65 byte encoding in both Secp and Ed25519 IIRC
I am generally confused why the tx hash includes the signature generally anyway, since aggregate signatures would remove them
Pre-launch of the Cosmos Hub I at least tried to ensure our secp signature has no malleability. (no negative s field, or zero points). Tony Arcieri was very careful in ensuring we did not use ASN-1 anywhere in our codebases.
That's good. I also did check our secp and from what I can tell it uses a deterministic compact encoding. Just noting that as a general principle this ADR-027 proto encoding check can't do anything about signature malleability, and we need to check carefully there too.
Why don't we just use x := SHA256(encode(tx)), where tx is the transaction without AuthInfo/signatures?
Why don't we just use
x := SHA256(encode(tx)), wheretxis the transaction without AuthInfo/signatures?
this is cleaner
how come? i thought the above fixes the issue with amino as well?
No not at all. Amino tx body and auth info are malleable
I'm going to overtake https://github.com/cosmos/cosmos-sdk/pull/19700 and update that PR.
talking with people at the Icf retreat we discussed malleability in relation to signing. It came up that we should not use the encoded data as input in the txcache instead we should hash the decoded data since this will produce the same hash every time.
cc @aaronc