Skip to content

Commit

Permalink
Impl ecdsa::CheckSignatureBytes-related changes (#159)
Browse files Browse the repository at this point in the history
Corresponding `ecdsa` crate PR is RustCrypto/signatures#151

This implements the changes which eagerly validate that ECDSA signature
`r` and `s` components are in-range `NonZeroScalar` values.

This eliminates the need to do these checks as part of each ECDSA
verification implementation.
  • Loading branch information
tarcieri authored Sep 4, 2020
1 parent a6f3bd6 commit a3384dc
Show file tree
Hide file tree
Showing 6 changed files with 22 additions and 34 deletions.
2 changes: 1 addition & 1 deletion Cargo.lock

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

3 changes: 3 additions & 0 deletions k256/src/ecdsa.rs
Original file line number Diff line number Diff line change
Expand Up @@ -71,6 +71,9 @@ use core::convert::TryInto;
/// ECDSA/secp256k1 signature (fixed-size)
pub type Signature = ecdsa_core::Signature<Secp256k1>;

#[cfg(not(feature = "ecdsa"))]
impl ecdsa_core::CheckSignatureBytes for Secp256k1 {}

#[cfg(all(feature = "ecdsa", feature = "sha256"))]
impl ecdsa_core::hazmat::DigestPrimitive for Secp256k1 {
type Digest = sha2::Sha256;
Expand Down
2 changes: 1 addition & 1 deletion k256/src/ecdsa/sign.rs
Original file line number Diff line number Diff line change
Expand Up @@ -186,7 +186,7 @@ impl Scalar {
return Err(Error::new());
}

let mut signature = Signature::from_scalars(&r.into(), &s.into());
let mut signature = Signature::from_scalars(r, s)?;
let is_r_odd = bool::from(R.y.normalize().is_odd());
let is_s_high = signature.normalize_s()?;
let recovery_id = recoverable::Id((is_r_odd ^ is_s_high) as u8);
Expand Down
17 changes: 4 additions & 13 deletions k256/src/ecdsa/verify.rs
Original file line number Diff line number Diff line change
@@ -1,12 +1,10 @@
//! ECDSA verifier
use super::{recoverable, Error, Signature};
use crate::{
AffinePoint, CompressedPoint, EncodedPoint, NonZeroScalar, ProjectivePoint, Scalar, Secp256k1,
};
use crate::{AffinePoint, CompressedPoint, EncodedPoint, ProjectivePoint, Scalar, Secp256k1};
use core::convert::TryFrom;
use ecdsa_core::{hazmat::VerifyPrimitive, signature};
use elliptic_curve::{consts::U32, ops::Invert, sec1::ToEncodedPoint, FromBytes};
use elliptic_curve::{consts::U32, ops::Invert, sec1::ToEncodedPoint};
use signature::{digest::Digest, DigestVerifier, PrehashSignature};

/// ECDSA/secp256k1 verification key (i.e. public key)
Expand Down Expand Up @@ -87,15 +85,8 @@ impl TryFrom<&EncodedPoint> for VerifyKey {

impl VerifyPrimitive<Secp256k1> for AffinePoint {
fn verify_prehashed(&self, z: &Scalar, signature: &Signature) -> Result<(), Error> {
let maybe_r = NonZeroScalar::from_bytes(signature.r());
let maybe_s = NonZeroScalar::from_bytes(signature.s());

// TODO(tarcieri): replace with into conversion when available (see subtle#73)
let (r, s) = if maybe_r.is_some().into() && maybe_s.is_some().into() {
(maybe_r.unwrap(), maybe_s.unwrap())
} else {
return Err(Error::new());
};
let r = signature.r();
let s = signature.s();

// Ensure signature is "low S" normalized ala BIP 0062
if s.is_high().into() {
Expand Down
30 changes: 11 additions & 19 deletions p256/src/ecdsa.rs
Original file line number Diff line number Diff line change
Expand Up @@ -48,7 +48,7 @@ use {
crate::{AffinePoint, ProjectivePoint, Scalar},
core::borrow::Borrow,
ecdsa_core::hazmat::{SignPrimitive, VerifyPrimitive},
elliptic_curve::{ops::Invert, subtle::CtOption, FromBytes},
elliptic_curve::ops::Invert,
};

/// ECDSA/P-256 signature (fixed-size)
Expand All @@ -64,6 +64,9 @@ pub type SigningKey = ecdsa_core::SigningKey<NistP256>;
#[cfg_attr(docsrs, doc(cfg(feature = "ecdsa")))]
pub type VerifyKey = ecdsa_core::VerifyKey<NistP256>;

#[cfg(not(feature = "ecdsa"))]
impl ecdsa_core::CheckSignatureBytes for NistP256 {}

#[cfg(all(feature = "ecdsa", feature = "sha256"))]
impl ecdsa_core::hazmat::DigestPrimitive for NistP256 {
type Digest = sha2::Sha256;
Expand Down Expand Up @@ -99,36 +102,25 @@ impl SignPrimitive<NistP256> for Scalar {
return Err(Error::new());
}

Ok(Signature::from_scalars(&r.into(), &s.into()))
Signature::from_scalars(r, s)
}
}

#[cfg(feature = "ecdsa")]
impl VerifyPrimitive<NistP256> for AffinePoint {
fn verify_prehashed(&self, z: &Scalar, signature: &Signature) -> Result<(), Error> {
let maybe_r =
Scalar::from_bytes(signature.r()).and_then(|r| CtOption::new(r, !r.is_zero()));

let maybe_s =
Scalar::from_bytes(signature.s()).and_then(|s| CtOption::new(s, !s.is_zero()));

// TODO(tarcieri): replace with into conversion when available (see subtle#73)
let (r, s) = if maybe_r.is_some().into() && maybe_s.is_some().into() {
(maybe_r.unwrap(), maybe_s.unwrap())
} else {
return Err(Error::new());
};

let r = signature.r();
let s = signature.s();
let s_inv = s.invert().unwrap();
let u1 = z * &s_inv;
let u2 = r * &s_inv;
let u2 = *r * &s_inv;

let x = ((&ProjectivePoint::generator() * &u1) + &(ProjectivePoint::from(*self) * &u2))
.to_affine()
.unwrap()
.x;

if Scalar::from_bytes_reduced(&x.to_bytes()) == r {
if Scalar::from_bytes_reduced(&x.to_bytes()) == *r {
Ok(())
} else {
Err(Error::new())
Expand Down Expand Up @@ -177,8 +169,8 @@ mod tests {
let z = Scalar::from_bytes(vector.m.try_into().unwrap()).unwrap();
let sig = d.try_sign_prehashed(&k_blinded, &z).unwrap();

assert_eq!(vector.r, sig.r().as_slice());
assert_eq!(vector.s, sig.s().as_slice());
assert_eq!(vector.r, sig.r().to_bytes().as_slice());
assert_eq!(vector.s, sig.s().to_bytes().as_slice());
}
}

Expand Down
2 changes: 2 additions & 0 deletions p384/src/ecdsa.rs
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,8 @@ pub use crate::NistP384;
/// ECDSA/P-384 signature (fixed-size)
pub type Signature = ecdsa::Signature<NistP384>;

impl ecdsa::CheckSignatureBytes for NistP384 {}

#[cfg(feature = "sha384")]
#[cfg_attr(docsrs, doc(cfg(feature = "sha384")))]
impl ecdsa::hazmat::DigestPrimitive for NistP384 {
Expand Down

0 comments on commit a3384dc

Please sign in to comment.