EcdsaKeypair::try_sign sometimes fail with Error::FormatEncoding

[jjteo74]

p256::ecdsa::Signature::split_bytes may return byte slices that start with 0, i.e. a negative SSH mpint, causing Error::FormatEncoding to be returned when being encoded as positive SSH mpint in the following code from signature.rs.

#[cfg(feature = "p256")]
impl TryFrom<&p256::ecdsa::Signature> for Signature {
    type Error = Error;

    fn try_from(signature: &p256::ecdsa::Signature) -> Result<Signature> {
        let (r, s) = signature.split_bytes();

        #[allow(clippy::arithmetic_side_effects)]
        let mut data = Vec::with_capacity(32 * 2 + 4 * 2 + 2);

        Mpint::from_positive_bytes(&r)?.encode(&mut data)?;
        Mpint::from_positive_bytes(&s)?.encode(&mut data)?;

        Ok(Signature {
            algorithm: Algorithm::Ecdsa {
                curve: EcdsaCurve::NistP256,
            },
            data,
        })
    }
}

The tests below demonstrate this error.

#[cfg(test)]
mod tests {
    use std::error::Error as _;

    use p256::ecdsa::{Signature, SigningKey};
    use signature::Signer as _;
    use ssh_key::{
        private::{EcdsaKeypair, KeypairData},
        PrivateKey,
    };

    // Given this key,
    const ECDSA_SHA2_NISTP256_KEY: &str = r#"-----BEGIN OPENSSH PRIVATE KEY-----
b3BlbnNzaC1rZXktdjEAAAAABG5vbmUAAAAEbm9uZQAAAAAAAAABAAAAaAAAABNlY2RzYS
1zaGEyLW5pc3RwMjU2AAAACG5pc3RwMjU2AAAAQQSzwx9FzmH5gmO5Ta/Z5ZbLI7vGd633
iSkAYVnttWFHZIukaRphqq3h2lib1Vge6S5mMpIaZQXAmcwfSb58B39SAAAAsGdtXBVnbV
wVAAAAE2VjZHNhLXNoYTItbmlzdHAyNTYAAAAIbmlzdHAyNTYAAABBBLPDH0XOYfmCY7lN
r9nllssju8Z3rfeJKQBhWe21YUdki6RpGmGqreHaWJvVWB7pLmYykhplBcCZzB9JvnwHf1
IAAAAhAIikHhEppebDtEfJ5oy0G6sD21elar603hLk3qci14AgAAAAEGJsYWRlMkB0b21h
dG9zc2gBAgMEBQYH
-----END OPENSSH PRIVATE KEY-----"#;
    // this data results in one signature component byte slice to start with `\x00`.
    const DATA: &[u8] = b"\x00\x00\x00\x30\xef\xff\xeb\xcf\xd1\x59\xb2\xdb\x54\x0a\x25\xed\xfd\x8f\xba\x2f\x5e\xb1\x43\x48\xe0\xac\x09\xfe\x11\x6a\x0d\xa1\xec\x20\x15\xf9\x67\x0a\xc9\x58\xf8\xc1\x7b\x77\x26\x34\x78\x32\x95\x1a\xb0\x98\x32\x00\x00\x00\x06\x62\x6c\x61\x64\x65\x32\x00\x00\x00\x0e\x73\x73\x68\x2d\x63\x6f\x6e\x6e\x65\x63\x74\x69\x6f\x6e\x00\x00\x00\x09\x70\x75\x62\x6c\x69\x63\x6b\x65\x79\x01\x00\x00\x00\x13\x65\x63\x64\x73\x61\x2d\x73\x68\x61\x32\x2d\x6e\x69\x73\x74\x70\x32\x35\x36\x00\x00\x00\x68\x00\x00\x00\x13\x65\x63\x64\x73\x61\x2d\x73\x68\x61\x32\x2d\x6e\x69\x73\x74\x70\x32\x35\x36\x00\x00\x00\x08\x6e\x69\x73\x74\x70\x32\x35\x36\x00\x00\x00\x41\x04\xb3\xc3\x1f\x45\xce\x61\xf9\x82\x63\xb9\x4d\xaf\xd9\xe5\x96\xcb\x23\xbb\xc6\x77\xad\xf7\x89\x29\x00\x61\x59\xed\xb5\x61\x47\x64\x8b\xa4\x69\x1a\x61\xaa\xad\xe1\xda\x58\x9b\xd5\x58\x1e\xe9\x2e\x66\x32\x92\x1a\x65\x05\xc0\x99\xcc\x1f\x49\xbe\x7c\x07\x7f\x52";

    #[test]
    fn when_try_sign_given_ecdsa_nistp256_ssh_key_should_return_format_encoding_error() {
        let key = PrivateKey::from_openssh(ECDSA_SHA2_NISTP256_KEY).unwrap();
        let error = key.try_sign(DATA).expect_err("try_sign did not fail");
        let error = error.source().unwrap();
        assert_eq!(
            error.downcast_ref(),
            Some(&ssh_key::Error::FormatEncoding),
            "try_sign error caused by encoding"
        );
    }

    #[test]
    fn when_encode_to_ssh_format_given_ecdsa_nistp256_signature_should_return_format_encoding_error() {
        let key = PrivateKey::from_openssh(ECDSA_SHA2_NISTP256_KEY).unwrap();
        let KeypairData::Ecdsa(pk) = key.key_data() else {
            panic!("not ecdsa");
        };
        let EcdsaKeypair::NistP256 { private: pk, .. } = pk else {
            panic!("not p256");
        };

        let signing_key: SigningKey =
            SigningKey::from_slice(pk.as_ref()).expect("invalid signing key");
        let sig: Signature = signing_key.try_sign(DATA).expect("signing error");
        let (r, s) = sig.split_bytes();
        // creates signature components r with \x0 prefix, which is a *negative* SSH mpint.
        assert_eq!(
            r.as_slice(),
            b"\x00\x59\xf3\xb5\xe1\x91\xb3\x47\xf2\x8e\x17\x44\xf7\xeb\x66\xbc\x14\xf8\x3f\x24\xd4\xe4\x74\x59\x25\x45\x82\x1d\xf7\x5c\xb1\x32",
            "not a positive MpInt"
        );
        assert_eq!(s.as_slice(),
            b"\x5e\xa0\xb2\x92\x30\x00\x19\x02\xe1\x79\x6e\xc8\x5b\x02\xeb\x2b\xfd\xfb\xd5\xc5\xf6\x5c\xfb\xc6\x83\x32\x25\xb0\x53\x6d\x1f\x98",
            "is positive MpInt"
        );
        // therefore causing the following to fail with `FormatEncoding` error when converting `r` to
        // positive mpint
        assert_eq!(
            ssh_key::Signature::try_from(sig),
            Err(ssh_key::Error::FormatEncoding)
        );
    }
}

[tarcieri]

Aah indeed. This seems like a bug in Mpint::from_positive_bytes which could potentially strip leading zeroes and then add one if appropriate, rather than the current behavior of returning Error::FormatEncoding if there are leading zeroes.

[tarcieri]

#171 should provide a fix