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 dangerous Default impl for Point #161

Merged
merged 1 commit into from
May 3, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 1 addition & 4 deletions schnorr_fun/src/frost.rs
Original file line number Diff line number Diff line change
Expand Up @@ -832,10 +832,7 @@ impl<H: Digest<OutputSize = U32> + Clone, NG> Frost<H, NG> {
g!({ agg_nonce[0] } + binding_coeff * { agg_nonce[1] })
.normalize()
.non_zero()
.unwrap_or_else(|| {
// Use the same trick as the MuSig spec
G.normalize()
})
.unwrap_or(Point::generator())
.into_point_with_even_y();

let challenge = self
Expand Down
6 changes: 1 addition & 5 deletions schnorr_fun/src/musig.rs
Original file line number Diff line number Diff line change
Expand Up @@ -540,11 +540,7 @@ impl<H: Digest<OutputSize = U32> + Clone, NG> MuSig<H, NG> {
let (R, r_needs_negation) = g!({ agg_Rs.0[0] } + b * { agg_Rs.0[1] })
.normalize()
.non_zero()
.unwrap_or_else(|| {
// if final nonce is zero we set it to generator as in MuSig spec
debug_assert!(G.is_y_even());
G.normalize()
})
.unwrap_or(Point::generator())
.into_point_with_even_y();

for R_i in &mut Rs {
Expand Down
47 changes: 42 additions & 5 deletions secp256kfun/src/point.rs
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,7 @@ use rand_core::RngCore;
/// - `S`: A [`Secrecy`] to determine whether operations on this point should be done in constant-time or not. By default points are [`Public`] so operations run in variable time.
/// - `Z`: A [`ZeroChoice`] to keep track of whether the point might be zero (the point at infinity) or is guaranteed to be non-zero.
///
/// # Serialization
/// ## Serialization
///
/// Only points that are normalized (i.e. `T` ≠ `NonNormal`) can be serialized. A Point that is
/// `EvenY` points serialize to and from their 32-byte x-only representation.
Expand All @@ -46,13 +46,26 @@ use rand_core::RngCore;
/// [`Public`]: crate::marker::Public
/// [`is_zero`]: crate::Point::is_zero
/// [_identity element_]: https://en.wikipedia.org/wiki/Identity_element
#[derive(Default)]
pub struct Point<T = Normal, S = Public, Z = NonZero>(
pub(crate) backend::Point,
pub(crate) T,
PhantomData<(Z, S)>,
);

/// The default for `Point`<_,_,Zero>` is [`Point::zero`].
impl<T: Default, S> Default for Point<T, S, Zero> {
fn default() -> Self {
Point::zero()
}
}

/// The default for `Point`<_,_,Zero>` is [`Point::generator`].
impl<T: Default + PointType, S> Default for Point<T, S, NonZero> {
fn default() -> Self {
Point::generator()
}
}

impl<Z, S, T: Clone> Clone for Point<T, S, Z> {
fn clone(&self) -> Self {
Point::from_inner(self.0, self.1.clone())
Expand Down Expand Up @@ -147,7 +160,7 @@ impl<Z: ZeroChoice> Point<Normal, Public, Z> {
}
}

impl<T: PointType, S> Point<T, S, NonZero> {
impl<T, S> Point<T, S, NonZero> {
/// Converts this point into the point with the same x-coordinate but with
/// an even y-coordinate. Returns a Point marked `EvenY` with a `bool`
/// indicating whether the point had to be negated to make its y-coordinate
Expand All @@ -159,12 +172,36 @@ impl<T: PointType, S> Point<T, S, NonZero> {
/// let point = Point::random(&mut rand::thread_rng());
/// let (point_with_even_y, was_odd) = point.clone().into_point_with_even_y();
/// ```
pub fn into_point_with_even_y(self) -> (Point<EvenY, S, NonZero>, bool) {
pub fn into_point_with_even_y(self) -> (Point<EvenY, S, NonZero>, bool)
where
T: PointType,
{
let normalized = self.normalize();
let needs_negation = !normalized.is_y_even();
let negated = normalized.conditional_negate(needs_negation);
(Point::from_inner(negated.0, EvenY), needs_negation)
}

/// Returns the generator point [`G`] defined in [_Standards for Efficient Cryptography_].
///
/// This is sometimes more useful than just using `secp256kfun::G` since it allows the compiler
/// to infer types.
///
/// ## Examples
///
/// ```
/// use secp256kfun::{marker::*, Point, G};
/// assert_eq!(Point::<Normal, Public, _>::generator(), *G);
/// ```
///
/// [_Standards for Efficient Cryptography_]: https://www.secg.org/sec1-v2.pdf
/// [`G`]: crate::G
pub fn generator() -> Self
where
T: Default,
{
Self::from_inner(backend::G_POINT, T::default())
}
}

impl Point<EvenY, Public, NonZero> {
Expand Down Expand Up @@ -401,7 +438,7 @@ impl<T, S> Point<T, S, Zero> {
/// [`identity_element`]: https://en.wikipedia.org/wiki/Identity_element
pub fn zero() -> Self
where
T: PointType + Default,
T: Default,
{
Self::from_inner(backend::Point::zero(), T::default())
}
Expand Down