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

Impl TryFrom<&[u8]> for all compressed point types. #296

13 changes: 9 additions & 4 deletions src/edwards.rs
Original file line number Diff line number Diff line change
Expand Up @@ -338,15 +338,20 @@ impl Default for CompressedEdwardsY {
impl CompressedEdwardsY {
/// Construct a `CompressedEdwardsY` from a slice of bytes.
///
/// # Panics
/// # Returns
///
/// If the input `bytes` slice does not have a length of 32.
pub fn from_slice(bytes: &[u8]) -> CompressedEdwardsY {
/// An `Option<CompressedEdwardsY>` which is `None` if the input `bytes`
/// slice does not have a length of 32.
pub fn from_slice(bytes: &[u8]) -> Option<CompressedEdwardsY> {
if bytes.len() != 32 {
return None;
}

let mut tmp = [0u8; 32];

tmp.copy_from_slice(bytes);

CompressedEdwardsY(tmp)
Some(CompressedEdwardsY(tmp))
}
}

Expand Down
42 changes: 42 additions & 0 deletions src/montgomery.rs
Original file line number Diff line number Diff line change
Expand Up @@ -56,6 +56,7 @@ use field::FieldElement;
use scalar::Scalar;

use traits::Identity;
use traits::ValidityCheck;

use subtle::Choice;
use subtle::ConditionallySelectable;
Expand Down Expand Up @@ -93,6 +94,24 @@ impl PartialEq for MontgomeryPoint {

impl Eq for MontgomeryPoint {}

impl ValidityCheck for MontgomeryPoint {
/// Decode the \\(u\\)-coordinate field element and re-encode it
/// to its canonical form to check whether the original was valid.
///
/// There are no other required checks for the Mongomery form of the curve,
/// as every element in \\( \mathbb{F}\_{q} \\) lies either on the curve or
/// its quadratic twist. (cf. §5.2 of "Montgomery Curves and Their
/// Arithmetic" by [Costello and Smith][costello-smith].)
///
/// [costello-smith]: https://eprint.iacr.org/2017/212.pdf
fn is_valid(&self) -> bool {
let maybe_u: FieldElement = FieldElement::from_bytes(&self.0);
let u: [u8; 32] = maybe_u.to_bytes();

u.ct_eq(&self.0).into()
}
}
Copy link
Contributor

Choose a reason for hiding this comment

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

This is not the right definition of validity; all 32-byte strings are valid representations of elements of the curve+twist pair, and this check should not exist in the source code.

Copy link
Member Author

Choose a reason for hiding this comment

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

§5.2 of "Montgomery Curves and Their Arithmetic" reads:

both E_{(A,B)}(𝔽_q) and E_{(A,B')}(𝔽_q) have cryptographically strong (i.e. almost prime) group orders. This means that every element of 𝔽_q corresponds to a point on a cryptographically strong Montgomery curve, and implementers need not perform any point validation checks in this protocol.

My reading of that is that we do not need to check that a 32-byte string is a canonical representation of a field element, however we probably should check (which the decoding function explicitly does not do) in order to avoid inputs in [2^255 - 18, 2^255).

Copy link
Contributor

Choose a reason for hiding this comment

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

That is the check that the encoding is canonical, which is why we don't need it.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, I realise that is the the check that the encoding is canonical. I'm arguing that we should do the check to ensure the 32-bytes are a canonical element in 𝔽_q, otherwise the callee must check this if they care about potential "non-contributory" inputs. (My understanding of TryFrom is essentially "here's some random crap, make it absolutely safe if you can".)

Copy link
Member Author

@isislovecruft isislovecruft Nov 13, 2019

Choose a reason for hiding this comment

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

I mean, consumers of our library are doing similar checks on their own (cf. tendermint/tmkms#279). I'm arguing that we should give them proper tools to Just Do The Thing And Not Have To Worry, since the whole point of this PR is to reduce copy-pasta.


impl Zeroize for MontgomeryPoint {
fn zeroize(&mut self) {
self.0.zeroize();
Expand All @@ -110,6 +129,29 @@ impl MontgomeryPoint {
self.0
}

/// Attempt to create a `MontgomeryPoint` from a slice of bytes.
///
/// # Returns
///
/// An `Option<MontgomeryPoint>` which is `None` if the length of the slice
/// of bytes is not 32, or if the bytes did not represent a canonical
/// `FieldElement`.
pub fn from_slice(bytes: &[u8]) -> Option<MontgomeryPoint> {
if bytes.len() != 32 {
return None;
}

let mut array = [0u8; 32];
array.copy_from_slice(&bytes[..32]);

let P = MontgomeryPoint(array);

if P.is_valid() {
return Some(P);
}
None
}

/// Attempt to convert to an `EdwardsPoint`, using the supplied
/// choice of sign for the `EdwardsPoint`.
///
Expand Down
13 changes: 9 additions & 4 deletions src/ristretto.rs
Original file line number Diff line number Diff line change
Expand Up @@ -230,15 +230,20 @@ impl CompressedRistretto {

/// Construct a `CompressedRistretto` from a slice of bytes.
///
/// # Panics
/// # Returns
///
/// If the input `bytes` slice does not have a length of 32.
pub fn from_slice(bytes: &[u8]) -> CompressedRistretto {
/// An `Option<CompressedRistretto>` which is `None` if the input `bytes`
/// slice does not have a length of 32.
pub fn from_slice(bytes: &[u8]) -> Option<CompressedRistretto> {
if bytes.len() != 32 {
return None;
}

let mut tmp = [0u8; 32];

tmp.copy_from_slice(bytes);

CompressedRistretto(tmp)
Some(CompressedRistretto(tmp))
}

/// Attempt to decompress to an `RistrettoPoint`.
Expand Down