tendermint: Constant time hash equality

[thanethomson]

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]

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]

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 a6670f0 - do you perhaps remember?

[tony-iqlusion]

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]

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

[thanethomson]

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