Skip to content

Commit

Permalink
ssh-key: accept undersized mpints in ECDSA signatures (#291)
Browse files Browse the repository at this point in the history 
Leading zeros must be stripped from mpint values (per RFC 4251), meaning it's
possible to see "short" `mpint` values when using them to construct signatures.

These values must be padded with leading zeros before being used in
`pXYZ::ecdsa::Signature`, which expect exact-size arrays.

See #290 for details.
  • Loading branch information
@mkeeter
mkeeter authored Sep 5, 2024
1 parent bac2931 commit f28658e
Showing 1 changed file with 62 additions and 17 deletions.
79 changes: 62 additions & 17 deletions ssh-key/src/signature.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,6 @@ use crate::{private::Ed25519Keypair, public::Ed25519PublicKey};
use {
crate::{private::DsaKeypair, public::DsaPublicKey},
bigint::BigUint,
core::iter,
sha1::Sha1,
signature::{DigestSigner, DigestVerifier},
};
Expand All @@ -24,6 +23,9 @@ use crate::{
public::EcdsaPublicKey,
};

#[cfg(any(feature = "dsa", feature = "p256", feature = "p384", feature = "p521"))]
use core::iter;

#[cfg(feature = "rsa")]
use {
crate::{private::RsaKeypair, public::RsaPublicKey, HashAlg},
Expand Down Expand Up @@ -151,7 +153,7 @@ fn ecdsa_sig_size(mut data: &[u8], curve: EcdsaCurve, sk_trailer: bool) -> Resul
for _ in 0..2 {
let component = Mpint::decode(reader)?;
let bytes = component.as_positive_bytes().ok_or(Error::FormatEncoding)?;
if bytes.len() != curve.field_size() {
if bytes.len() > curve.field_size() {
return Err(encoding::Error::Length.into());
}
}
Expand Down Expand Up @@ -517,6 +519,17 @@ impl_signature_for_curve!(p256, "p256", NistP256, 32);
impl_signature_for_curve!(p384, "p384", NistP384, 48);
impl_signature_for_curve!(p521, "p521", NistP521, 66);

/// Build a generic sized object from a `u8` iterator, with leading zero padding
#[cfg(any(feature = "p256", feature = "p384", feature = "p521"))]
fn zero_pad_field_bytes<B: FromIterator<u8> + Copy>(m: Mpint) -> Option<B> {
use core::mem::size_of;

let bytes = m.as_positive_bytes()?;
size_of::<B>()
.checked_sub(bytes.len())
.map(|i| B::from_iter(iter::repeat(0u8).take(i).chain(bytes.iter().cloned())))
}

#[cfg(feature = "p256")]
impl TryFrom<&Signature> for p256::ecdsa::Signature {
type Error = Error;
Expand All @@ -536,11 +549,11 @@ fn p256_signature_from_openssh_bytes(mut signature_bytes: &[u8]) -> Result<p256:
let r = Mpint::decode(reader)?;
let s = Mpint::decode(reader)?;

match (r.as_positive_bytes(), s.as_positive_bytes()) {
(Some(r), Some(s)) => Ok(p256::ecdsa::Signature::from_scalars(
p256::FieldBytes::try_from(r).map_err(|_| Error::Crypto)?,
p256::FieldBytes::try_from(s).map_err(|_| Error::Crypto)?,
)?),
match (
zero_pad_field_bytes::<p256::FieldBytes>(r),
zero_pad_field_bytes::<p256::FieldBytes>(s),
) {
(Some(r), Some(s)) => Ok(p256::ecdsa::Signature::from_scalars(r, s)?),
_ => Err(Error::Crypto),
}
}
Expand All @@ -558,11 +571,11 @@ impl TryFrom<&Signature> for p384::ecdsa::Signature {
let r = Mpint::decode(reader)?;
let s = Mpint::decode(reader)?;

match (r.as_positive_bytes(), s.as_positive_bytes()) {
(Some(r), Some(s)) => Ok(p384::ecdsa::Signature::from_scalars(
p384::FieldBytes::try_from(r).map_err(|_| Error::Crypto)?,
p384::FieldBytes::try_from(s).map_err(|_| Error::Crypto)?,
)?),
match (
zero_pad_field_bytes::<p384::FieldBytes>(r),
zero_pad_field_bytes::<p384::FieldBytes>(s),
) {
(Some(r), Some(s)) => Ok(p384::ecdsa::Signature::from_scalars(r, s)?),
_ => Err(Error::Crypto),
}
}
Expand All @@ -584,11 +597,11 @@ impl TryFrom<&Signature> for p521::ecdsa::Signature {
let r = Mpint::decode(reader)?;
let s = Mpint::decode(reader)?;

match (r.as_positive_bytes(), s.as_positive_bytes()) {
(Some(r), Some(s)) => Ok(p521::ecdsa::Signature::from_scalars(
p521::FieldBytes::try_from(r).map_err(|_| Error::Crypto)?,
p521::FieldBytes::try_from(s).map_err(|_| Error::Crypto)?,
)?),
match (
zero_pad_field_bytes::<p521::FieldBytes>(r),
zero_pad_field_bytes::<p521::FieldBytes>(s),
) {
(Some(r), Some(s)) => Ok(p521::ecdsa::Signature::from_scalars(r, s)?),
_ => Err(Error::Crypto),
}
}
Expand Down Expand Up @@ -699,6 +712,9 @@ mod tests {
signature::{Signer, Verifier},
};

#[cfg(feature = "p256")]
use super::{zero_pad_field_bytes, Mpint};

const DSA_SIGNATURE: &[u8] = &hex!("000000077373682d6473730000002866725bf3c56100e975e21fff28a60f73717534d285ea3e1beefc2891f7189d00bd4d94627e84c55c");
const ECDSA_SHA2_P256_SIGNATURE: &[u8] = &hex!("0000001365636473612d736861322d6e6973747032353600000048000000201298ab320720a32139cda8a40c97a13dc54ce032ea3c6f09ea9e87501e48fa1d0000002046e4ac697a6424a9870b9ef04ca1182cd741965f989bd1f1f4a26fd83cf70348");
const ED25519_SIGNATURE: &[u8] = &hex!("0000000b7373682d65643235353139000000403d6b9906b76875aef1e7b2f1e02078a94f439aebb9a4734da1a851a81e22ce0199bbf820387a8de9c834c9c3cc778d9972dcbe70f68d53cc6bc9e26b02b46d04");
Expand All @@ -716,6 +732,35 @@ mod tests {
let _ssh_signature = Signature::try_from(p256_signature).unwrap();
}

#[cfg(feature = "p256")]
#[test]
fn zero_pad_field_bytes_p256() {
let i = Mpint::from_bytes(&hex!(
"1122334455667788112233445566778811223344556677881122334455667788"
))
.unwrap();
let fb = zero_pad_field_bytes::<p256::FieldBytes>(i);
assert!(fb.is_some());

// too long
let i = Mpint::from_bytes(&hex!(
"991122334455667788112233445566778811223344556677881122334455667788"
))
.unwrap();
let fb = zero_pad_field_bytes::<p256::FieldBytes>(i);
assert!(fb.is_none());

// short is okay
let i = Mpint::from_bytes(&hex!(
"22334455667788112233445566778811223344556677881122334455667788"
))
.unwrap();
let fb = zero_pad_field_bytes::<p256::FieldBytes>(i)
.expect("failed to build FieldBytes from short hex string");
assert_eq!(fb[0], 0x00);
assert_eq!(fb[1], 0x22);
}

#[test]
fn decode_dsa() {
let signature = Signature::try_from(DSA_SIGNATURE).unwrap();
Expand Down

0 comments on commit f28658e

Please sign in to comment.