From a7f317a2b8db95ac415e26e18865a871d0291757 Mon Sep 17 00:00:00 2001 From: Isis Lovecruft Date: Fri, 18 Oct 2019 19:12:57 +0000 Subject: [PATCH 1/6] Impl TryFrom<&[u8]> for all compressed point types. This reduces copy-pasta in downstream users to check the length of the slice beforehand. --- src/edwards.rs | 17 ++++++++++++++++- src/ristretto.rs | 17 ++++++++++++++++- 2 files changed, 32 insertions(+), 2 deletions(-) diff --git a/src/edwards.rs b/src/edwards.rs index 998af8d..f0ddaa4 100644 --- a/src/edwards.rs +++ b/src/edwards.rs @@ -93,6 +93,7 @@ #![allow(non_snake_case)] use core::borrow::Borrow; +use core::convert::TryFrom; use core::fmt::Debug; use core::iter::Iterator; use core::iter::Sum; @@ -335,12 +336,26 @@ impl Default for CompressedEdwardsY { } } +impl TryFrom<&[u8]> for CompressedEdwardsY { + type Error = (); + + fn try_from(bytes: &[u8]) -> Result { + if bytes.len() != 32 { + return Err(()); + } + + Ok(CompressedEdwardsY::from_slice(bytes)) + } +} + impl CompressedEdwardsY { /// Construct a `CompressedEdwardsY` from a slice of bytes. /// /// # Panics /// - /// If the input `bytes` slice does not have a length of 32. + /// If the input `bytes` slice does not have a length of 32. For + /// a panic-safe version of this API, see the implementation of + /// `TryFrom<&[u8]`. pub fn from_slice(bytes: &[u8]) -> CompressedEdwardsY { let mut tmp = [0u8; 32]; diff --git a/src/ristretto.rs b/src/ristretto.rs index 6d53e89..59ca562 100644 --- a/src/ristretto.rs +++ b/src/ristretto.rs @@ -158,6 +158,7 @@ //! https://ristretto.group/ use core::borrow::Borrow; +use core::convert::TryFrom; use core::fmt::Debug; use core::iter::Sum; use core::ops::{Add, Neg, Sub}; @@ -217,6 +218,18 @@ impl ConstantTimeEq for CompressedRistretto { } } +impl TryFrom<&[u8]> for CompressedRistretto { + type Error = (); + + fn try_from(bytes: &[u8]) -> Result { + if bytes.len() != 32 { + return Err(()); + } + + Ok(CompressedRistretto::from_slice(bytes)) + } +} + impl CompressedRistretto { /// Copy the bytes of this `CompressedRistretto`. pub fn to_bytes(&self) -> [u8; 32] { @@ -232,7 +245,9 @@ impl CompressedRistretto { /// /// # Panics /// - /// If the input `bytes` slice does not have a length of 32. + /// If the input `bytes` slice does not have a length of 32. For a + /// panic-safe version of this API, see the implementation of + /// `TryFrom<&[u8]>`. pub fn from_slice(bytes: &[u8]) -> CompressedRistretto { let mut tmp = [0u8; 32]; From db3d26f4b9df7b58a319ffc6029682b24b8f42d7 Mon Sep 17 00:00:00 2001 From: Isis Lovecruft Date: Wed, 23 Oct 2019 16:34:07 +0000 Subject: [PATCH 2/6] Fix typo in TryFrom docstring. --- src/edwards.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/edwards.rs b/src/edwards.rs index f0ddaa4..d29ca2f 100644 --- a/src/edwards.rs +++ b/src/edwards.rs @@ -355,7 +355,7 @@ impl CompressedEdwardsY { /// /// If the input `bytes` slice does not have a length of 32. For /// a panic-safe version of this API, see the implementation of - /// `TryFrom<&[u8]`. + /// `TryFrom<&[u8]>`. pub fn from_slice(bytes: &[u8]) -> CompressedEdwardsY { let mut tmp = [0u8; 32]; From 1fa0048262d416f00aaa0d255609f127592d4efb Mon Sep 17 00:00:00 2001 From: Isis Lovecruft Date: Wed, 23 Oct 2019 18:59:58 +0000 Subject: [PATCH 3/6] Implement TryFrom<&[u8]> and ValidityCheck for MontgomeryPoint. --- src/montgomery.rs | 41 +++++++++++++++++++++++++++++++++++++++++ 1 file changed, 41 insertions(+) diff --git a/src/montgomery.rs b/src/montgomery.rs index a89de22..c85397d 100644 --- a/src/montgomery.rs +++ b/src/montgomery.rs @@ -48,6 +48,7 @@ // affine and projective cakes and eat both of them too. #![allow(non_snake_case)] +use core::convert::TryFrom; use core::ops::{Mul, MulAssign}; use constants::APLUS2_OVER_FOUR; @@ -56,6 +57,7 @@ use field::FieldElement; use scalar::Scalar; use traits::Identity; +use traits::ValidityCheck; use subtle::Choice; use subtle::ConditionallySelectable; @@ -91,6 +93,45 @@ 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() + } +} + +impl TryFrom<&[u8]> for MontgomeryPoint { + type Error = (); + + fn try_from(bytes: &[u8]) -> Result { + if bytes.len() != 32 { + return Err(()); + } + + let mut array = [0u8; 32]; + array.copy_from_slice(&bytes[..32]); + + let P = MontgomeryPoint(array); + + if P.is_valid() { + return Ok(P); + } + + Err(()) + } +} + impl MontgomeryPoint { /// View this `MontgomeryPoint` as an array of bytes. pub fn as_bytes<'a>(&'a self) -> &'a [u8; 32] { From 1d8b3995c958c181e4b8254e43d211140b04940c Mon Sep 17 00:00:00 2001 From: Isis Lovecruft Date: Wed, 23 Oct 2019 19:29:02 +0000 Subject: [PATCH 4/6] Add custom error types, currently only used in TryFrom impls. --- src/edwards.rs | 9 ++++--- src/errors.rs | 66 +++++++++++++++++++++++++++++++++++++++++++++++ src/lib.rs | 5 ++++ src/montgomery.rs | 11 +++++--- src/ristretto.rs | 9 ++++--- 5 files changed, 90 insertions(+), 10 deletions(-) create mode 100644 src/errors.rs diff --git a/src/edwards.rs b/src/edwards.rs index d29ca2f..5c90195 100644 --- a/src/edwards.rs +++ b/src/edwards.rs @@ -108,6 +108,8 @@ use subtle::ConstantTimeEq; use constants; +use errors::{CurveError, InternalError}; + use field::FieldElement; use scalar::Scalar; @@ -337,11 +339,12 @@ impl Default for CompressedEdwardsY { } impl TryFrom<&[u8]> for CompressedEdwardsY { - type Error = (); + type Error = CurveError; - fn try_from(bytes: &[u8]) -> Result { + fn try_from(bytes: &[u8]) -> Result { if bytes.len() != 32 { - return Err(()); + return Err(CurveError( + InternalError::BytesLengthError{name: "CompressedEdwardsY", length: 32})); } Ok(CompressedEdwardsY::from_slice(bytes)) diff --git a/src/errors.rs b/src/errors.rs new file mode 100644 index 0000000..d6f64c6 --- /dev/null +++ b/src/errors.rs @@ -0,0 +1,66 @@ +// -*- mode: rust; -*- +// +// This file is part of curve25519-dalek. +// Copyright (c) 2019 Isis Lovecruft +// See LICENSE for licensing information. +// +// Authors: +// - Isis Agora Lovecruft + +//! Errors which may occur. +//! +//! Currently, these are only used in the implementations of `TryFrom`. +//! +//! This module optionally implements support for the types in the `failure` +//! crate. This can be enabled by building with `--features failure`. + +use core::fmt; +use core::fmt::Display; + +/// Internal errors. Most application-level developers will likely not +/// need to pay any attention to these. +#[derive(Clone, Copy, Debug, Eq, PartialEq, Hash)] +pub(crate) enum InternalError { + /// An error in the length of bytes handed to a constructor. + /// + /// To use this, pass a string specifying the `name` of the type which is + /// returning the error, and the `length` in bytes which its constructor + /// expects. + BytesLengthError { + name: &'static str, + length: usize, + }, +} + +impl Display for InternalError { + fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { + match *self { + InternalError::BytesLengthError{ name: n, length: l} + => write!(f, "{} must be {} bytes in length", n, l), + } + } +} + +#[cfg(feature = "failure")] +impl ::failure::Fail for InternalError {} + +/// Errors which may occur. +/// +/// This error may arise due to: +/// +/// * Being given bytes with a length different to what was expected. +#[derive(Clone, Copy, Eq, PartialEq, Hash, Debug)] +pub struct CurveError(pub(crate) InternalError); + +impl Display for CurveError { + fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { + write!(f, "{}", self.0) + } +} + +#[cfg(feature = "failure")] +impl ::failure::Fail for CurveError { + fn cause(&self) -> Option<&dyn (::failure::Fail)> { + Some(&self.0) + } +} diff --git a/src/lib.rs b/src/lib.rs index 0216628..5359b16 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -43,6 +43,8 @@ extern crate packed_simd; extern crate byteorder; pub extern crate digest; +#[cfg(feature = "failure")] +extern crate failure; extern crate rand_core; #[cfg(test)] extern crate rand_os; @@ -82,6 +84,9 @@ pub mod constants; // External (and internal) traits. pub mod traits; +// Errors which may occur. +pub mod errors; + //------------------------------------------------------------------------ // curve25519-dalek internal modules //------------------------------------------------------------------------ diff --git a/src/montgomery.rs b/src/montgomery.rs index c85397d..7a4f841 100644 --- a/src/montgomery.rs +++ b/src/montgomery.rs @@ -53,6 +53,7 @@ use core::ops::{Mul, MulAssign}; use constants::APLUS2_OVER_FOUR; use edwards::{CompressedEdwardsY, EdwardsPoint}; +use errors::{CurveError, InternalError}; use field::FieldElement; use scalar::Scalar; @@ -112,11 +113,12 @@ impl ValidityCheck for MontgomeryPoint { } impl TryFrom<&[u8]> for MontgomeryPoint { - type Error = (); + type Error = CurveError; - fn try_from(bytes: &[u8]) -> Result { + fn try_from(bytes: &[u8]) -> Result { if bytes.len() != 32 { - return Err(()); + return Err(CurveError( + InternalError::BytesLengthError{name: "MontgomeryPoint", length: 32})); } let mut array = [0u8; 32]; @@ -128,7 +130,8 @@ impl TryFrom<&[u8]> for MontgomeryPoint { return Ok(P); } - Err(()) + Err(CurveError( + InternalError::BytesLengthError{name: "MontgomeryPoint", length: 32})) } } diff --git a/src/ristretto.rs b/src/ristretto.rs index 59ca562..46c1c2d 100644 --- a/src/ristretto.rs +++ b/src/ristretto.rs @@ -181,6 +181,8 @@ use subtle::ConstantTimeEq; use edwards::EdwardsBasepointTable; use edwards::EdwardsPoint; +use errors::{CurveError, InternalError}; + #[allow(unused_imports)] use prelude::*; @@ -219,11 +221,12 @@ impl ConstantTimeEq for CompressedRistretto { } impl TryFrom<&[u8]> for CompressedRistretto { - type Error = (); + type Error = CurveError; - fn try_from(bytes: &[u8]) -> Result { + fn try_from(bytes: &[u8]) -> Result { if bytes.len() != 32 { - return Err(()); + return Err(CurveError( + InternalError::BytesLengthError{name: "CompressedRistretto", length: 32})); } Ok(CompressedRistretto::from_slice(bytes)) From c21224170a584a7a08b00f56e7b25f85a28a35c9 Mon Sep 17 00:00:00 2001 From: Isis Lovecruft Date: Wed, 23 Oct 2019 21:32:27 +0000 Subject: [PATCH 5/6] Remove optional failure dependency and impl std::error::Error. --- src/errors.rs | 17 ++++++++++------- src/lib.rs | 2 -- 2 files changed, 10 insertions(+), 9 deletions(-) diff --git a/src/errors.rs b/src/errors.rs index d6f64c6..13a7638 100644 --- a/src/errors.rs +++ b/src/errors.rs @@ -11,12 +11,15 @@ //! //! Currently, these are only used in the implementations of `TryFrom`. //! -//! This module optionally implements support for the types in the `failure` -//! crate. This can be enabled by building with `--features failure`. +//! If used with `std` support, this public types in this module implement the +//! `std::error::Error` trait. use core::fmt; use core::fmt::Display; +#[cfg(feature = "std")] +use std::error::Error; + /// Internal errors. Most application-level developers will likely not /// need to pay any attention to these. #[derive(Clone, Copy, Debug, Eq, PartialEq, Hash)] @@ -41,8 +44,8 @@ impl Display for InternalError { } } -#[cfg(feature = "failure")] -impl ::failure::Fail for InternalError {} +#[cfg(feature = "std")] +impl Error for InternalError { } /// Errors which may occur. /// @@ -58,9 +61,9 @@ impl Display for CurveError { } } -#[cfg(feature = "failure")] -impl ::failure::Fail for CurveError { - fn cause(&self) -> Option<&dyn (::failure::Fail)> { +#[cfg(feature = "std")] +impl Error for CurveError { + fn source(&self) -> Option<&(dyn Error + 'static)> { Some(&self.0) } } diff --git a/src/lib.rs b/src/lib.rs index 5359b16..f9bb55d 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -43,8 +43,6 @@ extern crate packed_simd; extern crate byteorder; pub extern crate digest; -#[cfg(feature = "failure")] -extern crate failure; extern crate rand_core; #[cfg(test)] extern crate rand_os; From 9ae2e3b4822caad8e958a6bd743fe647bcd0c7a3 Mon Sep 17 00:00:00 2001 From: Isis Lovecruft Date: Mon, 28 Oct 2019 17:24:02 +0000 Subject: [PATCH 6/6] Change all from_slice() constructors to return Options. We due this in lieu of implementing `TryFrom` to allow for API consumers to use the `?` operator to convert potential `None`s into their own `Result` types for better error handling with less boilerplate. Note that this is a breaking API change. --- src/edwards.rs | 31 +++++++-------------- src/errors.rs | 69 ----------------------------------------------- src/lib.rs | 3 --- src/montgomery.rs | 48 ++++++++++++++++----------------- src/ristretto.rs | 31 +++++++-------------- 5 files changed, 41 insertions(+), 141 deletions(-) delete mode 100644 src/errors.rs diff --git a/src/edwards.rs b/src/edwards.rs index 5c90195..d72ec28 100644 --- a/src/edwards.rs +++ b/src/edwards.rs @@ -93,7 +93,6 @@ #![allow(non_snake_case)] use core::borrow::Borrow; -use core::convert::TryFrom; use core::fmt::Debug; use core::iter::Iterator; use core::iter::Sum; @@ -108,8 +107,6 @@ use subtle::ConstantTimeEq; use constants; -use errors::{CurveError, InternalError}; - use field::FieldElement; use scalar::Scalar; @@ -338,33 +335,23 @@ impl Default for CompressedEdwardsY { } } -impl TryFrom<&[u8]> for CompressedEdwardsY { - type Error = CurveError; - - fn try_from(bytes: &[u8]) -> Result { - if bytes.len() != 32 { - return Err(CurveError( - InternalError::BytesLengthError{name: "CompressedEdwardsY", length: 32})); - } - - Ok(CompressedEdwardsY::from_slice(bytes)) - } -} - impl CompressedEdwardsY { /// Construct a `CompressedEdwardsY` from a slice of bytes. /// - /// # Panics + /// # Returns /// - /// If the input `bytes` slice does not have a length of 32. For - /// a panic-safe version of this API, see the implementation of - /// `TryFrom<&[u8]>`. - pub fn from_slice(bytes: &[u8]) -> CompressedEdwardsY { + /// An `Option` which is `None` if the input `bytes` + /// slice does not have a length of 32. + pub fn from_slice(bytes: &[u8]) -> Option { + if bytes.len() != 32 { + return None; + } + let mut tmp = [0u8; 32]; tmp.copy_from_slice(bytes); - CompressedEdwardsY(tmp) + Some(CompressedEdwardsY(tmp)) } } diff --git a/src/errors.rs b/src/errors.rs deleted file mode 100644 index 13a7638..0000000 --- a/src/errors.rs +++ /dev/null @@ -1,69 +0,0 @@ -// -*- mode: rust; -*- -// -// This file is part of curve25519-dalek. -// Copyright (c) 2019 Isis Lovecruft -// See LICENSE for licensing information. -// -// Authors: -// - Isis Agora Lovecruft - -//! Errors which may occur. -//! -//! Currently, these are only used in the implementations of `TryFrom`. -//! -//! If used with `std` support, this public types in this module implement the -//! `std::error::Error` trait. - -use core::fmt; -use core::fmt::Display; - -#[cfg(feature = "std")] -use std::error::Error; - -/// Internal errors. Most application-level developers will likely not -/// need to pay any attention to these. -#[derive(Clone, Copy, Debug, Eq, PartialEq, Hash)] -pub(crate) enum InternalError { - /// An error in the length of bytes handed to a constructor. - /// - /// To use this, pass a string specifying the `name` of the type which is - /// returning the error, and the `length` in bytes which its constructor - /// expects. - BytesLengthError { - name: &'static str, - length: usize, - }, -} - -impl Display for InternalError { - fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { - match *self { - InternalError::BytesLengthError{ name: n, length: l} - => write!(f, "{} must be {} bytes in length", n, l), - } - } -} - -#[cfg(feature = "std")] -impl Error for InternalError { } - -/// Errors which may occur. -/// -/// This error may arise due to: -/// -/// * Being given bytes with a length different to what was expected. -#[derive(Clone, Copy, Eq, PartialEq, Hash, Debug)] -pub struct CurveError(pub(crate) InternalError); - -impl Display for CurveError { - fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { - write!(f, "{}", self.0) - } -} - -#[cfg(feature = "std")] -impl Error for CurveError { - fn source(&self) -> Option<&(dyn Error + 'static)> { - Some(&self.0) - } -} diff --git a/src/lib.rs b/src/lib.rs index f9bb55d..0216628 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -82,9 +82,6 @@ pub mod constants; // External (and internal) traits. pub mod traits; -// Errors which may occur. -pub mod errors; - //------------------------------------------------------------------------ // curve25519-dalek internal modules //------------------------------------------------------------------------ diff --git a/src/montgomery.rs b/src/montgomery.rs index 7a4f841..66b4670 100644 --- a/src/montgomery.rs +++ b/src/montgomery.rs @@ -48,12 +48,10 @@ // affine and projective cakes and eat both of them too. #![allow(non_snake_case)] -use core::convert::TryFrom; use core::ops::{Mul, MulAssign}; use constants::APLUS2_OVER_FOUR; use edwards::{CompressedEdwardsY, EdwardsPoint}; -use errors::{CurveError, InternalError}; use field::FieldElement; use scalar::Scalar; @@ -112,29 +110,6 @@ impl ValidityCheck for MontgomeryPoint { } } -impl TryFrom<&[u8]> for MontgomeryPoint { - type Error = CurveError; - - fn try_from(bytes: &[u8]) -> Result { - if bytes.len() != 32 { - return Err(CurveError( - InternalError::BytesLengthError{name: "MontgomeryPoint", length: 32})); - } - - let mut array = [0u8; 32]; - array.copy_from_slice(&bytes[..32]); - - let P = MontgomeryPoint(array); - - if P.is_valid() { - return Ok(P); - } - - Err(CurveError( - InternalError::BytesLengthError{name: "MontgomeryPoint", length: 32})) - } -} - impl MontgomeryPoint { /// View this `MontgomeryPoint` as an array of bytes. pub fn as_bytes<'a>(&'a self) -> &'a [u8; 32] { @@ -146,6 +121,29 @@ impl MontgomeryPoint { self.0 } + /// Attempt to create a `MontgomeryPoint` from a slice of bytes. + /// + /// # Returns + /// + /// An `Option` 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 { + 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`. /// diff --git a/src/ristretto.rs b/src/ristretto.rs index 46c1c2d..b730938 100644 --- a/src/ristretto.rs +++ b/src/ristretto.rs @@ -158,7 +158,6 @@ //! https://ristretto.group/ use core::borrow::Borrow; -use core::convert::TryFrom; use core::fmt::Debug; use core::iter::Sum; use core::ops::{Add, Neg, Sub}; @@ -181,8 +180,6 @@ use subtle::ConstantTimeEq; use edwards::EdwardsBasepointTable; use edwards::EdwardsPoint; -use errors::{CurveError, InternalError}; - #[allow(unused_imports)] use prelude::*; @@ -220,19 +217,6 @@ impl ConstantTimeEq for CompressedRistretto { } } -impl TryFrom<&[u8]> for CompressedRistretto { - type Error = CurveError; - - fn try_from(bytes: &[u8]) -> Result { - if bytes.len() != 32 { - return Err(CurveError( - InternalError::BytesLengthError{name: "CompressedRistretto", length: 32})); - } - - Ok(CompressedRistretto::from_slice(bytes)) - } -} - impl CompressedRistretto { /// Copy the bytes of this `CompressedRistretto`. pub fn to_bytes(&self) -> [u8; 32] { @@ -246,17 +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. For a - /// panic-safe version of this API, see the implementation of - /// `TryFrom<&[u8]>`. - pub fn from_slice(bytes: &[u8]) -> CompressedRistretto { + /// An `Option` which is `None` if the input `bytes` + /// slice does not have a length of 32. + pub fn from_slice(bytes: &[u8]) -> Option { + 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`.