-
Notifications
You must be signed in to change notification settings - Fork 557
Implement Lizard encoding/decoding #826
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
base: main
Are you sure you want to change the base?
Conversation
/// This function does not produce cryptographically random-looking Ristretto points. Use | ||
/// [`Self::hash_from_bytes`] for that. DO NOT USE THIS FUNCTION unless you really know what | ||
/// you're doing. | ||
pub fn map_to_curve(mut bytes: [u8; 32]) -> RistrettoPoint { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jrose-signal I've modified this function a little bit from from_uniform_bytes_single_elligator
. It does the masking here rather than requiring the caller to do it. I think that should simplify your code but let me know if it doesn't
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Alas, we have one place where we are not masking, where we are not using this as a bidirectional Elligator map but merely as a performance hack for a faster from_uniform_bytes
(and changing that would be incompatible with existing clients). I wish we didn't, but we do.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
While here, I also suggest the doc comment be very specific about what "bottom bit" and "top two bits" means; since the argument is just "bytes", a client wouldn't immediately think it's going to be used as a little-endian Curve25519 scalar value, and a bytestring doesn't usually have a "top". Though maybe that's at odds with "we (Signal) still need a non-masking version".
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Alas, we have one place where we are not masking, where we are not using this as a bidirectional Elligator map but merely as a performance hack for a faster
from_uniform_bytes
(and changing that would be incompatible with existing clients). I wish we didn't, but we do.
Oof ok I think I can make something work. Can you share that code by any chance?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
While here, I also suggest the doc comment be very specific about what "bottom bit" and "top two bits" means; since the argument is just "bytes", a client wouldn't immediately think it's going to be used as a little-endian Curve25519 scalar value, and a bytestring doesn't usually have a "top". Though maybe that's at odds with "we (Signal) still need a non-masking version".
Great catch. Will fix
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
with exactly one caller here https://github.com/signalapp/libsignal/blob/main/rust/zkgroup/src/crypto/profile_key_struct.rs#L47
which needs to be fast because it's in the "try all 64 possibilities for recovering a full 32 bytes loop" here https://github.com/signalapp/libsignal/blob/main/rust/zkgroup/src/crypto/profile_key_encryption.rs#L100
(My non-cryptographer self suspects once we already gave up on hitting the entire curve we could have used a simpler function to produce points from bytes, but it's too late now.)
|
||
/// Computes the possible bytestrings that could have produced this point via | ||
/// [`Self::map_to_curve`]. | ||
pub fn map_to_curve_inverse(&self) -> [CtOption<[u8; 32]>; 8] { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jrose-signal another API change: I've renamed decode_253_bits
to this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Very reasonable, and definitely a nicer signature. Rolfe and I talked about going even further and returning [[u8; 32]; 8]
by duplicating present values over absent ones, since there can already be duplicates; however, it's definitely extra work over the basic API.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see. I think the explicitness here is nice fwiw
The Lizard encoding/decoding algorithms specify a map from 16-byte strings to the Ristretto group, and back. This is useful for ElGamal encryption, where plaintexts are group elements.
Signal relies on Lizard, and has been vendoring
curve25519-dalek
for years now (bc we don't exportFieldElement
). I figured upstreaming would be a valuable addition to dalek, so long as it is extensively documented so as to be maintainable. It also doesn't add any new dependencies.Some highlights of this PR:
map_to_curve
algorithms necessary for downstream. Most of this is from the Signal repoThis was prepared jointly with @rolfe-signal and with input from @jrose-signal. Thank you all so much for your work on this!