docknetwork / dock-substrate

Substrate node for Dock blockchain
57 stars 18 forks source link

[Attack] Cross-chain signature replay #3

Open bddap opened 4 years ago

bddap commented 4 years ago

Many planed signable types, like Revoke and DIDRemoval include a field: last_updated_in_block: BlockNumber,

The field is intended to mitigate replay attacks. AFAIK the mitigation works, but only in the context of a single chain.

In a multi chain environment, e.g. testnet+mainnet, the user may (unwisely) use the same public DID key on both chains. In that case, a signature posted to testnet may be replayed on mainnet and counted valid.

bddap commented 4 years ago

On simple mitigation, if necessary, would be to use BlockHash in place of BlockNumber.

lovesh commented 4 years ago

Good point. But how would BlockHash be available during block execution. It should only be available after extrinsics of the block have been processed (and state changes applied). Or would blockhash be just the hash of an included exstrinsics? Btw, the attack is less likely as you should have created/updated the DID (or other objects) in the same block number in both chains (its an == check and not >)

bddap commented 4 years ago

My understanding is that last_updated_in_block refers to the last block during which an update occurred, not the current block. Similarly, BlockHash would refer to the last block during which an update occurred, not the current block.

bddap commented 4 years ago

Oh, scratch that. I see the issue. During block execution, we only have the hash for the previous block, not the current one.

bddap commented 4 years ago

Ok, new idea. This one is stolen from substrate.

The CheckGenesis SignedExtension prevents cross-chain signature replay (but not for hard-forks). We maybe implement something similar.

lovesh commented 4 years ago

Can you elaborate how? I think you are saying to include the genesis hash in the payload by DID. I can see how that we can implement Encode for the StateChange enum and the while encoding (serializing) before signing, you (the node as well as the SDK) include the genesis hash. I see how the SDK can using api.genesisHash but don't see how to do it for the node

bddap commented 4 years ago

I'm assuming the runtime somehow has read access to the genesis hash. The CheckGenesis extension must be getting it from somewhere.

lovesh commented 4 years ago

I see it would somehow know but how exactly, i don't know. Got some pointers in Riot chat, https://github.com/paritytech/substrate/blob/877d79bcebe7728d502c33227c2e4dea0766e22d/bin/node/runtime/src/lib.rs#L553 and https://github.com/paritytech/substrate/blob/877d79bcebe7728d502c33227c2e4dea0766e22d/bin/node/runtime/src/lib.rs#L674 is where CheckGenesis is used within the runtime But i have to try them

lovesh commented 4 years ago

You can get the genesis hash with <system::Module<T>>::block_hash(T::BlockNumber::from(0)). See here https://github.com/docknetwork/dock-substrate/blob/c7f18d2d7ade33ab6f7c18fb1845ee259f3f7ae8/runtime/src/did.rs#L396. I am printing (storing since io_print didn't work) the blockhash here https://github.com/docknetwork/dock-substrate/blob/c7f18d2d7ade33ab6f7c18fb1845ee259f3f7ae8/runtime/src/did.rs#L784. Run with SKIP_WASM_BUILD=1 flag to print