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.

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

curve!: use constant-time compressed Ristretto equality testing #669

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

AaronFeickert
Copy link
Contributor

@AaronFeickert AaronFeickert commented Jul 5, 2024

In line with the safety goals, this PR ensures that CompressedRistretto equality testing is always done in constant time.

Previous work in #229 implemented ConstantTimeEq for CompressedRistretto, but this is not used for Eq equality testing. It's already the case that RistrettoPoint and Scalar perform all equality testing in constant time; this PR unifies this behavior for compressed points as well.

BREAKING CHANGE: As noted by @tarcieri, this can break certain uses of match.

@AaronFeickert
Copy link
Contributor Author

Note that dalek-cryptography/subtle#131 would also supply a marker trait that could be useful here to signal this "all equality is constant time" behavior.

Comment on lines 218 to 219
#[derive(Copy, Clone, Eq, PartialEq, Hash)]
#[derive(Copy, Clone, Hash)]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

While it's unlikely to cause real-world breakages, I believe this is technically a breaking change.

Here's an example of what is possible with a derived PartialEq which won't be possible with this change: https://play.rust-lang.org/?version=stable&mode=debug&edition=2021&gist=33e85cff0772c968767a0417a8a7a541

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Interesting! I was not aware of this behavior. Thoughts on the tradeoff between the functionality and the implications of the breaking change?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would strongly agree with making this change if it weren't for the potential breakages

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ought this PR stay open until the next breaking release? I remain of the opinion that the benefit (consistency with safety goals) outweighs the cost.

@AaronFeickert AaronFeickert force-pushed the compressed-eq-is-ct branch 2 times, most recently from e03cc24 to 326eeda Compare July 26, 2024 15:37
@rozbb
Copy link
Contributor

rozbb commented Jul 30, 2024

Yeah that's unfortunate. I really doubt anyone has ever used a CompressedRistrettoPoint in a match statement, especially since this is a relatively new feature. But semver is semver. Tbh I'm not opposed to bumping the major version number soon.

@AaronFeickert
Copy link
Contributor Author

Given that this seems to be a desired change, I'll keep it open for the next breaking release. Feel free to close if this isn't the case!

@AaronFeickert AaronFeickert changed the title curve: use constant-time compressed equality testing curve!: use constant-time compressed equality testing Aug 2, 2024
@AaronFeickert AaronFeickert changed the title curve!: use constant-time compressed equality testing curve!: use constant-time compressed Ristretto equality testing Aug 2, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants