Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Check that secret-type material uses constant-time comparisons #139

Closed
AaronFeickert opened this issue Oct 26, 2022 · 1 comment · Fixed by #219
Closed

Check that secret-type material uses constant-time comparisons #139

AaronFeickert opened this issue Oct 26, 2022 · 1 comment · Fixed by #219

Comments

@AaronFeickert
Copy link
Contributor

It's often important that secret types use constant-time comparisons to avoid leaking data. Much of this is abstracted by the use of upstream key types, but this should be checked for custom types.

For example, MACs and other derived keys almost certainly need this property. See the subtle crate for good information and tooling on this.
@AaronFeickert
Copy link
Contributor Author

Note that SafeArray from tari_utilities implements constant-time equality testing when instantiated using a type that implements ConstantTimeEq. The use of a Hidden type with SafeArray under the hood should provide the desired behavior for such secret types.

@AaronFeickert AaronFeickert mentioned this issue Jan 15, 2024
Merged
SWvheerden pushed a commit that referenced this issue Feb 7, 2024
@AaronFeickert
feat: add constant-time trait bounds (#219)
a6cef07 
Currently, the only implementation of the `SecretKey` and `PublicKey`
traits is for Ristretto, where both
[scalars](https://github.com/dalek-cryptography/curve25519-dalek/blob/ba737a379071191158bacfa6d138f6249b12fc09/curve25519-dalek/src/scalar.rs#L296-L300)
and [group
elements](https://github.com/dalek-cryptography/curve25519-dalek/blob/ba737a379071191158bacfa6d138f6249b12fc09/curve25519-dalek/src/ristretto.rs#L822-L826)
use constant-time equality in their underlying `PartialEq`
implementations, and which support the `ConstantTimeEq` trait.

This PR does what it can to encourage the use of constant-time equality
for keys by doing a few things.

First, it requires that any types implementing `SecretKey` or
`PublicKey` also implement `ConstantTimeEq`. Unfortunately, this doesn't
guarantee that their `PartialEq` implementation defaults to this, and it
doesn't appear possible to enforce this at the trait level.

It also sets a good example by manually implementing `PartialEq` on the
Ristretto key types to use their `ConstantTimeEq` implementations. This
isn't strictly necessary, but hopefully helps to indicate best practice.
It also implements `ConstantTimeEq` directly as required by the new
trait bounds.

Finally, it implements `ConstantTimeEq` for `DiffieHellmanSharedSecret`
using the new trait bound, and removes a redundant `Zeroize` trait
bound.

Note that this doesn't actually change the current implementations'
behavior, and therefore incurs no performance hit.

Closes #139.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

feat: add constant-time trait bounds AaronFeickert/tari-crypto
1 participant
@AaronFeickert