search

informalsystems / tendermint-rs

Client libraries for Tendermint/CometBFT in Rust!
Apache License 2.0
591 stars 216 forks source link

tendermint: Constant time hash equality #1094

Open thanethomson opened 2 years ago

thanethomson commented 2 years ago

Version(s) of tendermint-rs: master, v0.23.x

Description

At present we have two Hash types in tendermint-rs: one in the tendermint-rpc crate in abci::transaction::Hash and one in the tendermint crate in the hash module.

I'm currently (in #1092) looking at removing the one in the tendermint-rpc crate and replacing it with the one from the tendermint crate to deduplicate this domain type. The one in the tendermint-rpc crate, however, implements subtle::ConstantTimeEq and the one in tendermint doesn't.

Perhaps @tony-iqlusion or @hdevalence would like to weigh in here, but I'd think that it'd be important to implement subtle::ConstantTimeEq for the tendermint::hash::Hash struct. The fact that we don't implement it for tendermint::hash::Hash may actually be a problem (part of the reason I raised this issue simply being for awareness).

Definition of "done"

When tendermint::hash::Hash implements subtle::ConstantTimeEq.

tony-iqlusion commented 2 years ago

When does the Hash type represent a secret value? Aren't they always computed over public chain data?

Or in other words: what attack/sidechannel is possible here?

thanethomson commented 2 years ago

When does the Hash type represent a secret value? Aren't they always computed over public chain data?

Good question. What was the initial reasoning in having ConstantTimeEq implemented for tendermint_rpc::abci::transaction::Hash? See https://github.com/informalsystems/tendermint-rs/commit/a6670f043e717006e08e38e125b7f697e319655f - do you perhaps remember?

tony-iqlusion commented 2 years ago

Haha wow, I did that, did I? Retroactively I have no justification for it. I'd probably suggest it's unneeded and just moving to derived Eq/PartialEq.

thanethomson commented 2 years ago

No problem :slightly_smiling_face: I'll remove it. We can always add it back in future if it becomes apparent we're hashing sensitive data.

thanethomson commented 2 years ago

Actually, I'll keep this issue open until I've removed it.