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

Add inverse Elligator2 mapping to Curve25519. #357

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

Conversation

lealanko
Copy link

This PR adds montgomery_elligator_decode, an inverse to montgomery_elligator_encode. This is my first contribution, so I'm very interested in advice on how to best accommodate Dalek's conventions.

Some points of note:

  • I'm not fond of the naming these two functions. The encode function is clearly named from the perspective of hash-to-curve operations, where pre-existing uniform data is "encoded" into a curve point. However, the inverse mapping can be used to map an existing curve point invertibly into uniform-looking data. Calling this operation "decode" is confusing. I would suggest more neutral naming like map_to_field and map_from_field.
  • The implementation is not constant-time, since the expected usage is to call the function repeatedly with randomly generated points until one is found that is in the mapping's domain. Still, in that case the timing leaks some information about the random generator. Should a constant-time version be provided, even if that would make rejection sampling more expensive?
  • As part of its input checking, the function has to ensure that the point is on the curve. This is expensive, and seems to be in conflict with Dalek's stated goal of "making illegal states unrepresentable". There seems to be need for a type that contains a u-coordinate that is guaranteed to belong to a point on the curve.
  • I test some randomly generated points to check that functions are inverses of each other, but I have no idea what's a reasonable number of iterations. Also, I'm not fond of using OsRng since that means that test runs are unpredictable..
  • montgomery_elligator_encode does compute the sign of the v-coordinate (as eps_is_sq) but it does not return it. This is an unfortunate asymmetry with the inverse function (whose output depends on the sign), and it may be critical for applications that require a full curve point, not just a u-coordinate.

I can try to address some of these issues already within this PR if that's a reasonable thing to do.

@lealanko lealanko force-pushed the feature/elligator-decode-montgomery branch from 7e95845 to 43dc857 Compare June 20, 2021 19:29
@lealanko lealanko force-pushed the feature/elligator-decode-montgomery branch from 43dc857 to ab0cc6c Compare June 20, 2021 19:44
The result of the sqrt(2u(u+A)) test can be used directly to compute
the final field element. This optimization was adapted from Loup
Vaillant's Monocypher library.
SnowyCoder added a commit to SnowyCoder/curve25519-dalek that referenced this pull request Jun 21, 2023
Code stolen from @lealanko dalek-cryptography#357
This commit only modifies the API to be more "standard" conforming (as
described in https://www.elligator.org/), having a tweak and "dirty" key
generation. Also various tests were added to ensure that it conforms to
already-developed implementations.

The original pull-request differed from standard implementations when
checking for element positivity since FieldElement::is_positive performs
an entirely different check than what the standard describes.

Since this commit includes standard-conformity, the original maps
downloaded from https://www.elligator.org/ are included in the vendor
folder.
@@ -193,6 +193,81 @@ pub(crate) fn elligator_encode(r_0: &FieldElement) -> MontgomeryPoint {
MontgomeryPoint(u.to_bytes())
}

/// Perform the inverse Elligator2 mapping from a Montgomery point to
/// field element. This algorithm is based on [Elligator:
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: the indefinite article

Suggested change
/// field element. This algorithm is based on [Elligator:
/// a field element. This algorithm is based on [Elligator:

// Both r and -r are valid results. Pick the nonnegative one.
let result = FieldElement::conditional_select(&r, &-&r, r.is_negative());

return Some(result);
Copy link
Contributor

Choose a reason for hiding this comment

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

You may omit return here in the last line of a function, as in other functions in this file.

Suggested change
return Some(result);
Some(result)

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