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

Fix curv25519 coordinates #162

Merged
merged 3 commits into from
Aug 22, 2022
Merged

Fix curv25519 coordinates #162

merged 3 commits into from
Aug 22, 2022

Conversation

elichai
Copy link
Contributor

@elichai elichai commented Dec 28, 2021

The current code had the following bugs:

  • In the y coordinate it took the bytes serialization of the point and interpreted it as the y coordinate, ignoring the fact that the serialization encodes the parity bit of the x coordinate in the most significant bit (which is the reason djb made the field elements are 255 bits)
  • In the x coordinate there was a bug in xrecover using (q-3)/4 instead of (q-1)/4.
  • when recovering the x coordinate it did not flip it to the right parity using the parity bit.

I generated test vectors using https://github.com/dalek-cryptography/curve25519-dalek and fixed the issues making the test pass.

references:
https://ed25519.cr.yp.to/python/ed25519.py
https://datatracker.ietf.org/doc/html/rfc8032#section-5.1.3
https://github.com/dalek-cryptography/curve25519-dalek/blob/076cf34/src/edwards.rs#L193
https://github.com/dalek-cryptography/curve25519-dalek/blob/076cf34/src/edwards.rs#L518
https://github.com/dalek-cryptography/curve25519-dalek/blob/076cf34/src/field.rs#L229

@elichai elichai merged commit bc626bb into master Aug 22, 2022
@elichai elichai deleted the fix-25519 branch August 22, 2022 09:03
qalisander pushed a commit to GridlockNetwork/curv that referenced this pull request Jul 1, 2023
* Add test vectors for curve25519 coordinates

* Make (q-1)/4 actually (q-1)/4 and not (q-3)/4

* Fix curve25519 coordinates by clearing the parity bit

(cherry picked from commit bc626bb)
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

2 participants