Replay protection specs #440
Conversation
|
|
||
| ## Tendermint | ||
|
|
||
| [Tendermint]((https://docs.tendermint.com/v0.33/app-dev/app-development.html#replay-protection)) provides a first layer of protection against replay attacks in its mempool. The mechanism is based on a cache of previously seen transactions. This check though, like all the checks performed in `CheckTx`, is weak, since a malicious validator could always propose a block containing invalid transactions. There's therefore the need for a more robust replay protection mechanism implemented directly in the application. |
There was a problem hiding this comment.
presumably we aren't using v0.33? (link)
There was a problem hiding this comment.
the preface should describe what replay protection is and why we need it in the first place
| /$Address/tx_counter: u64 | ||
| ``` | ||
|
|
||
| In `process_proposal` we check that `tx_counter` field in `WrapperTx` is equal to the value currently in storage; if this doesn't hold, the transaction is considered invalid. |
There was a problem hiding this comment.
and what happens in this case? (the transaction should be completely rejected, with no storage changes)
|
|
||
| In `process_proposal` we check that `tx_counter` field in `WrapperTx` is equal to the value currently in storage; if this doesn't hold, the transaction is considered invalid. | ||
|
|
||
| At last, in `finalize_block`, the protocol updates the counter key in storage, increasing its value by one. Now, if a malicious user tried to replay this transaction, the `tx_counter` in the struct would no longer be equal to the one in storage and the transaction would be deemed invalid. |
There was a problem hiding this comment.
for transactions for which a fee is paid, I presume? can we clarify this?
There was a problem hiding this comment.
Sorry I don't get it, could you explain better?
There was a problem hiding this comment.
under what conditions exactly does the protocol update the counter key?
| ## InnerTx | ||
|
|
||
| The `EncryptedTx` incapsulated inside `WrapperTx` should be protected too. That's because, if it wasn't, an attacker could extract the inner tx, rewrap it, and replay it.\ | ||
| To simplify this check we will perform it entirely in Wasm: the check of the counter will be carryed out by the validity predicates while the actual writing of the counter in storage will be done by the transactions themselves. |
There was a problem hiding this comment.
| To simplify this check we will perform it entirely in Wasm: the check of the counter will be carryed out by the validity predicates while the actual writing of the counter in storage will be done by the transactions themselves. | |
| To simplify this check we will perform it entirely in Wasm: the check of the counter will be carried out by the validity predicates while the actual writing of the counter in storage will be done by the transactions themselves. |
|
|
||
| In the specific case of a transfer, since MASP already comes with replay protection as part of the Zcash design (see the [MASP specs](https://specs.namada.net/masp.html) and [Zcash protocol specs](https://zips.z.cash/protocol/protocol.pdf)), the counter in `SignedTxData` should be optional. | ||
|
|
||
| An alternative implementation could place the protection for the inner tx in protocol, just like the wrapper one. In this case we would need to extend the `Tx` struct to hold an additional optional field (again, because of MASP) for the transaction counter and the address involved in replay protection, like so: |
There was a problem hiding this comment.
it's possible, but I think it's preferable to put the replay protection for the inner tx in WASM, it's a bit more appropriate to how the abstractions of the system are intended to work.
There was a problem hiding this comment.
Agree, would you like me to delete this part or move it to a "Discarded solutions" section of this spec to keep track of it?
There was a problem hiding this comment.
moving it to an "alternatives considered" section sounds good
|
|
||
| ## Batching | ||
|
|
||
| The implementation proposed in this document doesn't support batching of multiple transactions from a same address in a single block. More specifically, the transactions will all succeed only if they are executed in the intended order, but the order in which transactions will be included in the block by the proposer is not guaranteed. An out of order execution of multiple transactions would lead to the failure of some of them (in the worst case, the failure of all of them but one, in the best case, the failure of only one). This problem will be amplified by the introduction of Ferveo for DKG which will be able to reorder transactions. |
There was a problem hiding this comment.
well, the outer tx nonce can be incremented in the submitted transactions, right? this at least gives the proposer an incentive to order them correctly since they receive more fees, right?
There was a problem hiding this comment.
Yes, the outer counter needs to be incremented every time. If the order of transactions in the mempool of the proposer is the intended one, all the transactions will be executed correctly. The issue is when the order is off: at the moment, the protocol doesn't have a way to reorder the txs based on this counter (we could implement it). This of course works assuming that we are going to charge the fee (or at least part of it) for the inclusion in the block only, regardless of the transaction being correctly executed, otherwise, the proposer would just have an incentive to add them to the block without worrying about the order.
But if we will randomize the tx order for DKG then we are stuck with this problem.
Another scenario is if, for any reason, a transaction doesn't get to the proposer's mempool: in this case, all the transactions with a tx_counter greater than the missing one will fail, even if placed in the correct (ascending) order.
There was a problem hiding this comment.
the protocol doesn't need to reorder the transactions as long as it is incentive compatible for the proposer to order them correctly
we don't need to worry about the DKG for now (perhaps such a counter would have to be outside the encrypted inner transaction if the user wants a particular execution order to be enforced)
There was a problem hiding this comment.
for the latter case, we should probably prevent transactions with counter different by more than 1 from executing / paying any fee at all
|
|
||
| The implementation proposed in this document doesn't support batching of multiple transactions from a same address in a single block. More specifically, the transactions will all succeed only if they are executed in the intended order, but the order in which transactions will be included in the block by the proposer is not guaranteed. An out of order execution of multiple transactions would lead to the failure of some of them (in the worst case, the failure of all of them but one, in the best case, the failure of only one). This problem will be amplified by the introduction of Ferveo for DKG which will be able to reorder transactions. | ||
|
|
||
| The Wasm implementation of replay protection can't cope with this problem because every wasm run (of either a transaction or a validity predicate) is only aware of its context, meaning the wasm bytecode and the serialized transaction data. The lack of awareness of the other transactions makes it impossible to develop a replay protection mechanism supporting batching in wasm. |
There was a problem hiding this comment.
I don't follow - why can't the user just increment the counter in the transactions which they submit?
|
|
||
| To address this issue there could be two ways: | ||
|
|
||
| - Keep the proposed replay protection in Wasm and implement a batching mechanism in both the client and the ledger to embed more than one transaction in a single `Tx` struct |
There was a problem hiding this comment.
we shouldn't need to do this, I think, see above comments
|
|
||
| ## InnerTx | ||
|
|
||
| The `EncryptedTx` incapsulated inside `WrapperTx` should be protected too. That's because, if it wasn't, an attacker could extract the inner tx, rewrap it, and replay it.\ |
There was a problem hiding this comment.
it is not very clear that, even if EncryptedTx is extracted isn't the purpose of tx_counter to protect the replay thereof?What are the steps when unwrapping a transaction to check the counter? Also it would clarify a lot the document to describe the end-to-end communication between involved entities. What is assumed end to end authenticated and encrypted? how wrapperTx is being put on the network? How the integrity of the counter is protected? Are we assuming symmetric encryption?How keys are being exhanged between t he involved parties?
There was a problem hiding this comment.
Thanks for the changes - much improved. Unfortunately I don't think the proposed InnerTx method is safe, due to a race condition - a proposer (or other user) who sees a failed inner transaction can replay it if the original submitter doesn't have an opportunity to invalidate the inner counter in time. See comment.
|
|
||
| The VP of the _source_ address will then check the validity of the signature and, if it's deemed valid, will proceed to check if the pre-value of the counter in storage was equal to the one contained in the `SignedTxData` struct and if the post-value of the key in storage has been incremented by one: if any of these conditions doesn't hold the VP will discard the transactions and prevent the changes from being applied to the storage. | ||
|
|
||
| In the specific case of a transfer, since MASP already comes with replay protection as part of the Zcash design (see the [MASP specs](https://specs.namada.net/masp.html) and [Zcash protocol specs](https://zips.z.cash/protocol/protocol.pdf)), the counter in `SignedTxData` is not required and therefore should be optional. |
There was a problem hiding this comment.
| In the specific case of a transfer, since MASP already comes with replay protection as part of the Zcash design (see the [MASP specs](https://specs.namada.net/masp.html) and [Zcash protocol specs](https://zips.z.cash/protocol/protocol.pdf)), the counter in `SignedTxData` is not required and therefore should be optional. | |
| In the specific case of a shielded transfer, since MASP already comes with replay protection as part of the Zcash design (see the [MASP specs](https://specs.namada.net/masp.html) and [Zcash protocol specs](https://zips.z.cash/protocol/protocol.pdf)), the counter in `SignedTxData` is not required and therefore should be optional. |
|
|
||
| The ideal solution would be to interrupt the execution of the Wasm code after the transaction counter (if any) has been increased. This would allow performing a first run of the involved VPs and, if all of them accept the changes, let the protocol commit these changes before any possible failure. After that, the protocol would resume the execution of the transaction from the previous interrupt point until completion or failure, after which a second pass of the VPs is initiated to validate the remaining state modifications. In case of a VP rejection after the counter increase there would be no need to resume execution and the transaction could be immediately deemed invalid so that the protocol could skip to the next tx to be executed. With this solution, the counter update would be committed to storage regardless of a failure of the transaction itself. | ||
|
|
||
| Unfortunately, at the moment, Wasmer doesn't allow [yielding](https://github.com/wasmerio/wasmer/issues/1127) from the execution. For now, the responsibility will be up to the user to provide a valid inner transaction, and, in case of an invalid one, to take actions to prevent a possible replay attack: in essence, the user will be required to submit a new valid transaction to invalidate the counter of the previous one. |
There was a problem hiding this comment.
this is not safe because there is a race condition - instead, users will need to craft WASM code to wrap logic which might possibly fail in a try/catch block or something like that - is this possible? what happens if the transaction runs out of gas? if we can't make this safe we'll have to find another solution (unfortunately)...
| } | ||
| ``` | ||
|
|
||
| This new field will be signed just like the other ones and is therefore subject to the same guarantees explained in the [initial](#encryption-authentication) section. The validity of this identifier will be checked in `process_proposal` for both the outer and inner tx: if a transaction carries an unexpected chain id, it won't be applied, meaning that no modifications will be applied to storage. |
There was a problem hiding this comment.
I think we can also check the chain ID in mempool, at least on the wrapper
There was a problem hiding this comment.
ah, I see this is further down under ### Wrapper checks
|
|
||
| In general, a transaction is valid at the moment of submission, but after that, a series of external factors (ledger state, etc.) might change the mind of the submitter who's now not interested in the execution of the transaction anymore. | ||
|
|
||
| We have to introduce the concept of a lifetime (or timeout) for the transactions: basically, the `Tx` struct will hold an extra field called `expiration` stating the maximum block height up until which the submitter is willing to see the transaction executed. After the specified block height, the transaction will be considered invalid and discarded regardless of all the other checks. |
There was a problem hiding this comment.
I think it might be bit more user friendly to use expiry time (we can check it against last committed block time) as blocks duration can vary slightly
|
|
||
| The consistency of the storage subspace is of critical importance for the correct working of the replay protection mechanism. To protect it, a validity predicate will check that no changes to this subspace are applied by any wasm transaction, as those should only be available from protocol. | ||
|
|
||
| Both in `mempool_validation` and `process_proposal` we will perform a check (together with others, see the [relative](#wrapper-checks) section) on both the digests against the storage to check that neither of the transactions has already been executed: if this doesn't hold, the `WrapperTx` will not be included into the mempool/block respectively. If both checks pass then the transaction is included in the block and executed. In the `finalize_block` function we will add the transaction's hash to storage to prevent re-executions. We will first add the hash of the wrapper transaction. After that, in the following block, we deserialize the inner transaction, check the correct order of the transactions in the block and execute the tx: if it runs out of gas then we'll avoid storing its hash to allow rewrapping and executing the transaction, otherwise we'll add the hash in storage (both in case of success or failure of the tx). |
There was a problem hiding this comment.
Hmm I don't think this is right - if the transaction is executed and pays some fee, but runs out of gas, we should still prevent replay (the user can issue a new transaction)
otherwise the user may pay more fees than intended
There was a problem hiding this comment.
Actually, by the time we get to the execution of the inner transaction, the wrapper hash has already been saved to storage (in the previous block), so you can't replay the wrapper. You can only take the encrypted inner transaction and include it in a new wrapper
There was a problem hiding this comment.
I see. I think this is probably fine but we should put a TTL in inner transactions then, it's weird to have some signed data around for a long time which could potentially be executed at any moment
There was a problem hiding this comment.
We do that with this last spec revision. The struct Tx will carry an expiration field that will define the lifetime of the transaction. The user will be able to set this value and we'll perform a set of checks on it. Since it's implemented in struct Tx, this will apply to both wrappers and inners.
grarco
force-pushed
the
grarco/replay-protection-specs
branch
from
2b202f3
to
3a43f50
Compare
* feat(extension): using React context to manage state * refactor(extension): removing unused component * refactor(extension, app): refactoring route management * New pages: Rename key, View Mnemonic, and fixing bugs (anoma#441) * feat(extension, app): adding Vault context * feat(extension): creating view mnemonic page * feat(extension, setup): reflecting changes on SeedPhraseInstructions and reusing a few components * feat(extension): fixing inital loading state * feat(extension): adding rename page * feat(extension): adding navigation menu * feat(extension): handling passwords with session storage * feat(extension): storing hashed password in session storage * feat(extension,app): adding back some global styles * refactor(extension, app): making ViewAccount code clearer * fix(extension,app): fixing 404 route
Specs for replay protection (#178)