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

Implement optional contributory behaviour check #78

Conversation

isislovecruft
Copy link
Member

No description provided.

@rozbb
Copy link
Collaborator

rozbb commented Sep 14, 2021

I agree that this function needs to be available. The HPKE spec requires this check, and the way it's performed now could be better.

I think, given this, the docs don't have to do as much justification as they currently do. A simple paragraph explaining what the check does and a link to one of the included blog posts would be sufficient I think.

On naming, I think the function should be called is_all_zeros or something to that effect, since that is the wording that the specs use for validation ("contributory" doesn't appear anywhere in the HPKE spec, for example). The downside of is_all_zeros though is that it's the negation of the current function. Something like that though.

@AnomalRoil
Copy link

Totally supportive of adding this, given how people are using X25519 to do all kind of things now.
How about also rejecting here the other points mentioned in https://cr.yp.to/ecdh.html#validate ?

And how about adding a ct check to see if the resulting shared secret is all zeroes or not?

@dalek-cryptography dalek-cryptography deleted a comment from rozbb Sep 14, 2021
/// [relevant]: https://tools.ietf.org/html/rfc7748#page-15
/// [public]: https://vnhacker.blogspot.com/2015/09/why-not-validating-curve25519-public.html
/// [discussions]: https://vnhacker.blogspot.com/2016/08/the-internet-of-broken-protocols.html
pub fn was_contributory(&self) -> bool {
Copy link
Member Author

Choose a reason for hiding this comment

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

#[must_use] seems appropriate here

Good call, thanks! (Oops, I accidentally deleted @rozbb's original comment somehow.)

@isislovecruft
Copy link
Member Author

Totally supportive of adding this, given how people are using X25519 to do all kind of things now.
How about also rejecting here the other points mentioned in https://cr.yp.to/ecdh.html#validate ?

That's not necessary, as those are checking that the other party's public keys are not points of small order. Any such public keys would result in the shared secret being the identity element. Thus, we only check afterwards that the shared secret is not the identity.

And how about adding a ct check to see if the resulting shared secret is all zeroes or not?

This is a constant time check for if the resulting shared secret is the identity (all zeroes).

@isislovecruft isislovecruft merged commit 254e43d into dalek-cryptography:develop Sep 14, 2021
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.

None yet

3 participants