refactor: cleanup and improve Mpi API

hko-s · wiktor-k · web-flow · commit 180340740510 · 2024-09-24T20:33:31.000+02:00
- Make `Mpi::from_slice` and `MpiRef::from_slice` safe (always strip leading zeros so that Mpis are always valid after construction) - fix: Return empty slice if zero-stripping an all-zeros slice - drop unchecked `impl From<Vec<u8>> for Mpi` (and replace usage) - drop unused `MpiRef::strip_trailing_zeroes` Fixes: #401 Co-authored-by: Wiktor Kwapisiewicz <wiktor@metacode.biz>
diff --git a/src/crypto/ecdh.rs b/src/crypto/ecdh.rs
@@ -297,7 +297,7 @@ pub fn generate_key<R: Rng + CryptoRng>(
             Ok((
                 PublicParams::ECDH(EcdhPublicParams::Known {
                     curve: ECCCurve::Curve25519,
-                    p: p.into(),
+                    p: Mpi::from_raw(p),
                     hash,
                     alg_sym,
                 }),
@@ -332,11 +332,11 @@ where
     Ok((
         PublicParams::ECDH(EcdhPublicParams::Known {
             curve: curve.clone(),
-            p: Mpi::from_raw_slice(public.to_sec1_bytes().as_ref()),
+            p: Mpi::from_slice(public.to_sec1_bytes().as_ref()),
             hash: curve.hash_algo()?,
             alg_sym: curve.sym_algo()?,
         }),
-        PlainSecretParams::ECDH(Mpi::from_raw_slice(secret.to_bytes().as_slice())),
+        PlainSecretParams::ECDH(Mpi::from_slice(secret.to_bytes().as_slice())),
     ))
 }
 
@@ -492,7 +492,7 @@ pub fn encrypt<R: CryptoRng + Rng>(
     let encrypted_session_key = aes_kw::wrap(&z, &plain_padded)?;
 
     Ok(PkeskBytes::Ecdh {
-        public_point: Mpi::from_raw_slice(&encoded_public),
+        public_point: Mpi::from_slice(&encoded_public),
         encrypted_session_key,
     })
 }
diff --git a/src/crypto/ecdsa.rs b/src/crypto/ecdsa.rs
@@ -111,12 +111,12 @@ pub fn generate_key<R: Rng + CryptoRng>(
         ECCCurve::P256 => {
             let secret = p256::SecretKey::random(&mut rng);
             let public = secret.public_key();
-            let secret = Mpi::from_raw_slice(secret.to_bytes().as_slice());
+            let secret = Mpi::from_slice(secret.to_bytes().as_slice());
 
             Ok((
                 PublicParams::ECDSA(EcdsaPublicParams::P256 {
                     key: public,
-                    p: Mpi::from_raw_slice(public.to_encoded_point(false).as_bytes()),
+                    p: Mpi::from_slice(public.to_encoded_point(false).as_bytes()),
                 }),
                 PlainSecretParams::ECDSA(secret),
             ))
@@ -125,12 +125,12 @@ pub fn generate_key<R: Rng + CryptoRng>(
         ECCCurve::P384 => {
             let secret = p384::SecretKey::random(&mut rng);
             let public = secret.public_key();
-            let secret = Mpi::from_raw_slice(secret.to_bytes().as_slice());
+            let secret = Mpi::from_slice(secret.to_bytes().as_slice());
 
             Ok((
                 PublicParams::ECDSA(EcdsaPublicParams::P384 {
                     key: public,
-                    p: Mpi::from_raw_slice(public.to_encoded_point(false).as_bytes()),
+                    p: Mpi::from_slice(public.to_encoded_point(false).as_bytes()),
                 }),
                 PlainSecretParams::ECDSA(secret),
             ))
@@ -139,12 +139,12 @@ pub fn generate_key<R: Rng + CryptoRng>(
         ECCCurve::P521 => {
             let secret = p521::SecretKey::random(&mut rng);
             let public = secret.public_key();
-            let secret = Mpi::from_raw_slice(secret.to_bytes().as_slice());
+            let secret = Mpi::from_slice(secret.to_bytes().as_slice());
 
             Ok((
                 PublicParams::ECDSA(EcdsaPublicParams::P521 {
                     key: public,
-                    p: Mpi::from_raw_slice(public.to_encoded_point(false).as_bytes()),
+                    p: Mpi::from_slice(public.to_encoded_point(false).as_bytes()),
                 }),
                 PlainSecretParams::ECDSA(secret),
             ))
@@ -153,12 +153,12 @@ pub fn generate_key<R: Rng + CryptoRng>(
         ECCCurve::Secp256k1 => {
             let secret = k256::SecretKey::random(&mut rng);
             let public = secret.public_key();
-            let secret = Mpi::from_raw_slice(secret.to_bytes().as_slice());
+            let secret = Mpi::from_slice(secret.to_bytes().as_slice());
 
             Ok((
                 PublicParams::ECDSA(EcdsaPublicParams::Secp256k1 {
                     key: public,
-                    p: Mpi::from_raw_slice(public.to_encoded_point(false).as_bytes()),
+                    p: Mpi::from_slice(public.to_encoded_point(false).as_bytes()),
                 }),
                 PlainSecretParams::ECDSA(secret),
             ))
diff --git a/src/crypto/eddsa.rs b/src/crypto/eddsa.rs
@@ -103,12 +103,12 @@ pub fn generate_key<R: Rng + CryptoRng>(
             q.extend_from_slice(&public.to_bytes());
 
             // secret key
-            let p = Mpi::from_raw_slice(&secret.to_bytes());
+            let p = Mpi::from_slice(&secret.to_bytes());
 
             (
                 PublicParams::EdDSALegacy {
                     curve: ECCCurve::Ed25519,
-                    q: q.into(),
+                    q: Mpi::from_raw(q),
                 },
                 PlainSecretParams::EdDSALegacy(p),
             )
diff --git a/src/crypto/rsa.rs b/src/crypto/rsa.rs
@@ -103,7 +103,7 @@ pub fn encrypt<R: CryptoRng + Rng>(
     let data = key.encrypt(&mut rng, Pkcs1v15Encrypt, plaintext)?;
 
     Ok(PkeskBytes::Rsa {
-        mpi: Mpi::from_raw_slice(&data[..]),
+        mpi: Mpi::from_slice(&data[..]),
     })
 }
 
diff --git a/src/packet/key/secret.rs b/src/packet/key/secret.rs
@@ -302,7 +302,7 @@ impl<D: PublicKeyTrait + PacketTrait + Clone + crate::ser::Serialize> SecretKeyT
                     // strip leading zeros, to match parse results from MPIs
                     let mpis = sig
                         .iter()
-                        .map(|v| Mpi::from_raw_slice(&v[..]))
+                        .map(|v| Mpi::from_slice(&v[..]))
                         .collect::<Vec<_>>();
 
                     signature = Some(SignatureBytes::Mpis(mpis));
diff --git a/src/types/mpi.rs b/src/types/mpi.rs
@@ -21,11 +21,12 @@ const MAX_EXTERN_MPI_BITS: u32 = 16384;
 ///
 /// ```rust
 /// use pgp::types::mpi;
+/// use pgp::types::MpiRef;
 ///
 /// // Decode the number `1`.
 /// assert_eq!(
 ///     mpi(&[0x00, 0x01, 0x01][..]).unwrap(),
-///     (&b""[..], (&[1][..]).into())
+///     (&b""[..], MpiRef::from_slice(&[1][..]))
 /// );
 /// ```
 pub fn mpi(input: &[u8]) -> IResult<&[u8], MpiRef<'_>> {
@@ -43,7 +44,7 @@ pub fn mpi(input: &[u8]) -> IResult<&[u8], MpiRef<'_>> {
             Err(needed) => Err(nom::Err::Incomplete(needed)),
             Ok(index) => {
                 let (rest, n) = number.take_split(index);
-                let n_stripped: MpiRef<'_> = strip_leading_zeros(n).into();
+                let n_stripped: MpiRef<'_> = MpiRef::from_slice(strip_leading_zeros(n));
 
                 Ok((rest, n_stripped))
             }
@@ -73,12 +74,8 @@ impl Mpi {
         Mpi(v)
     }
 
-    pub fn from_slice(slice: &[u8]) -> Self {
-        Mpi(slice.to_vec())
-    }
-
     /// Strips leading zeros.
-    pub fn from_raw_slice(raw: &[u8]) -> Self {
+    pub fn from_slice(raw: &[u8]) -> Self {
         Mpi(strip_leading_zeros(raw).to_vec())
     }
 
@@ -109,7 +106,7 @@ impl<'a> std::ops::Deref for MpiRef<'a> {
 
 impl<'a> MpiRef<'a> {
     pub fn from_slice(slice: &'a [u8]) -> Self {
-        MpiRef(slice)
+        MpiRef(strip_leading_zeros(slice))
     }
 
     pub fn to_owned(&self) -> Mpi {
@@ -123,19 +120,6 @@ impl<'a> MpiRef<'a> {
     pub fn as_bytes(&self) -> &[u8] {
         self.0
     }
-
-    /// Strip trailing zeroes.
-    pub fn strip_trailing_zeroes(&self) -> Self {
-        let mut end = self.0.len();
-        for byte in self.0.iter().rev() {
-            if *byte == 0 {
-                end -= 1;
-            } else {
-                break;
-            }
-        }
-        MpiRef::from_slice(&self.0[..end])
-    }
 }
 
 impl Serialize for Mpi {
@@ -155,24 +139,6 @@ impl<'a> Serialize for MpiRef<'a> {
     }
 }
 
-impl From<&[u8]> for Mpi {
-    fn from(other: &[u8]) -> Mpi {
-        Mpi::from_slice(other)
-    }
-}
-
-impl<'a> From<&'a [u8]> for MpiRef<'a> {
-    fn from(other: &'a [u8]) -> MpiRef<'a> {
-        MpiRef::from_slice(other)
-    }
-}
-
-impl From<Vec<u8>> for Mpi {
-    fn from(other: Vec<u8>) -> Mpi {
-        Mpi(other)
-    }
-}
-
 impl From<BigUint> for Mpi {
     fn from(other: BigUint) -> Self {
         Mpi(other.to_bytes_be())
@@ -220,7 +186,7 @@ mod tests {
         // Decode the number `511` (`0x1FF` in hex).
         assert_eq!(
             mpi(&[0x00, 0x09, 0x01, 0xFF][..]).unwrap(),
-            (&b""[..], (&[0x01, 0xFF][..]).into())
+            (&b""[..], MpiRef::from_slice(&[0x01, 0xFF][..]))
         );
 
         // Decode the number `2^255 + 7`.
@@ -232,11 +198,12 @@ mod tests {
             .unwrap(),
             (
                 &b""[..],
-                (&[
-                    0x80, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0,
-                    0, 0, 0, 0, 0, 0, 0x07
-                ][..])
-                    .into()
+                MpiRef::from_slice(
+                    &[
+                        0x80, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0,
+                        0, 0, 0, 0, 0, 0, 0, 0x07
+                    ][..]
+                )
             )
         );
     }
@@ -264,31 +231,4 @@ mod tests {
             assert_eq!(n_big, n_big2.into());
         }
     }
-
-    #[test]
-    fn test_strip_trailing_zeroes() {
-        let bytes = [1, 2, 3, 4, 0];
-        assert_eq!(
-            MpiRef::from_slice(&bytes)
-                .strip_trailing_zeroes()
-                .as_bytes(),
-            &[1, 2, 3, 4],
-        );
-
-        let bytes = [0, 1, 2, 3, 4];
-        assert_eq!(
-            MpiRef::from_slice(&bytes)
-                .strip_trailing_zeroes()
-                .as_bytes(),
-            &[0, 1, 2, 3, 4],
-        );
-
-        let bytes = [1, 2, 0, 3, 4];
-        assert_eq!(
-            MpiRef::from_slice(&bytes)
-                .strip_trailing_zeroes()
-                .as_bytes(),
-            &[1, 2, 0, 3, 4],
-        );
-    }
 }
diff --git a/src/util.rs b/src/util.rs
@@ -77,7 +77,7 @@ pub fn strip_leading_zeros(bytes: &[u8]) -> &[u8] {
     bytes
         .iter()
         .position(|b| b != &0)
-        .map_or(bytes, |offset| &bytes[offset..])
+        .map_or(&[], |offset| &bytes[offset..])
 }
 
 #[inline]
@@ -239,4 +239,11 @@ mod tests {
         write_packet_length(12870, &mut res).unwrap();
         assert_eq!(hex::encode(res), "ff00003246");
     }
+
+    #[test]
+    fn test_strip_leading_zeros_with_all_zeros() {
+        let buf = [0, 0, 0];
+        let stripped = strip_leading_zeros(&buf);
+        assert_eq!(stripped, &[]);
+    }
 }
diff --git a/tests/hsm-test.rs b/tests/hsm-test.rs
@@ -135,23 +135,17 @@ impl SecretKeyTrait for FakeHsm {
         let sig = self.sign_data.unwrap().1; // fake smartcard output
 
         let mpis = match self.public_key.algorithm() {
-            PublicKeyAlgorithm::RSA => vec![sig.into()],
+            PublicKeyAlgorithm::RSA => vec![Mpi::from_slice(sig)],
 
             PublicKeyAlgorithm::ECDSA => {
                 let mid = sig.len() / 2;
 
-                vec![
-                    Mpi::from_raw_slice(&sig[..mid]),
-                    Mpi::from_raw_slice(&sig[mid..]),
-                ]
+                vec![Mpi::from_slice(&sig[..mid]), Mpi::from_slice(&sig[mid..])]
             }
             PublicKeyAlgorithm::EdDSALegacy => {
                 assert_eq!(sig.len(), 64); // FIXME: check curve; add error handling
 
-                vec![
-                    Mpi::from_raw_slice(&sig[..32]),
-                    Mpi::from_raw_slice(&sig[32..]),
-                ]
+                vec![Mpi::from_slice(&sig[..32]), Mpi::from_slice(&sig[32..])]
             }
 
             _ => unimplemented!(),
diff --git a/tests/key_test.rs b/tests/key_test.rs
@@ -346,7 +346,7 @@ fn test_parse_details() {
         &hex::decode("4c073ae0c8445c0c").unwrap()[..]
     );
 
-    let primary_n: Mpi = hex::decode("a54cfa9142fb75265322055b11f750f49af37b64c67ad830ed7443d6c20477b0492ee9090e4cb8b0c2c5d49e87dff5ac801b1aaadb319eee9d3d29b25bd9aa634b126c0e5da4e66b414e9dbdde5dea0e38c5bfe7e5f7fdb9f4c1b1f39ed892dd4e0873a0df66ff46fd9236d291c276ce69fb972f5ef24746b6794a0f70e0694667b9de57353330c732733cc6d5f24cd772c5c7d5bdb77dc0a5b6e9d3ee0372146778cda6144976e33066fc57bfb515ef397b3aa882c0bde02d19f7a32df7b1195cb0f32e6e7455ac199fa434355f0fa43230e5237e9a6e0ff6ad5b21b4d892c6fc3842788ba48b020ee85edd135cff2808780e834b5d94cc2c2b5fa747167a20814589d7f030ee9f8a669737bdb063e6b0b88ab0fd7454c03f69678a1dd99442cfd0bf620bc5b6896cd6e2b51fdecf54c7e6368c11c70f302444ec9d5a17ceaacb4a9ac3c37db3478f8fb04a679f0957a3697e8d90152008927c751b34160c72e757efc85053dd86738931fd351cf134266e436efd64a14b35869040108082847f7f5215628e7f66513809ae0f66ea73d01f5fd965142cdb7860276d4c20faf716c40ae0632d3b180137438cb95257327607038fb3b82f76556e8dd186b77c2f51b0bfdd7552f168f2c4eb90844fdc05cf239a57690225903399783ad3736891edb87745a1180e04741526384045c2de03c463c43b27d5ab7ffd6d0ecccc249f").unwrap().into();
+    let primary_n = Mpi::from_raw(hex::decode("a54cfa9142fb75265322055b11f750f49af37b64c67ad830ed7443d6c20477b0492ee9090e4cb8b0c2c5d49e87dff5ac801b1aaadb319eee9d3d29b25bd9aa634b126c0e5da4e66b414e9dbdde5dea0e38c5bfe7e5f7fdb9f4c1b1f39ed892dd4e0873a0df66ff46fd9236d291c276ce69fb972f5ef24746b6794a0f70e0694667b9de57353330c732733cc6d5f24cd772c5c7d5bdb77dc0a5b6e9d3ee0372146778cda6144976e33066fc57bfb515ef397b3aa882c0bde02d19f7a32df7b1195cb0f32e6e7455ac199fa434355f0fa43230e5237e9a6e0ff6ad5b21b4d892c6fc3842788ba48b020ee85edd135cff2808780e834b5d94cc2c2b5fa747167a20814589d7f030ee9f8a669737bdb063e6b0b88ab0fd7454c03f69678a1dd99442cfd0bf620bc5b6896cd6e2b51fdecf54c7e6368c11c70f302444ec9d5a17ceaacb4a9ac3c37db3478f8fb04a679f0957a3697e8d90152008927c751b34160c72e757efc85053dd86738931fd351cf134266e436efd64a14b35869040108082847f7f5215628e7f66513809ae0f66ea73d01f5fd965142cdb7860276d4c20faf716c40ae0632d3b180137438cb95257327607038fb3b82f76556e8dd186b77c2f51b0bfdd7552f168f2c4eb90844fdc05cf239a57690225903399783ad3736891edb87745a1180e04741526384045c2de03c463c43b27d5ab7ffd6d0ecccc249f").unwrap());
 
     let pk = key.primary_key;
     assert_eq!(pk.version(), KeyVersion::V4);
@@ -399,7 +399,7 @@ fn test_parse_details() {
         PublicKeyAlgorithm::RSA,
         HashAlgorithm::SHA1,
         [0x7c, 0x63],
-        vec![Mpi::from(vec![
+        vec![Mpi::from_raw(vec![
             0x15, 0xb5, 0x4f, 0xca, 0x11, 0x7f, 0x1b, 0x1d, 0xc0, 0x7a, 0x05, 0x97, 0x25, 0x10,
             0x4b, 0x6a, 0x76, 0x12, 0xf8, 0x89, 0x48, 0x76, 0x74, 0xeb, 0x8b, 0x22, 0xcf, 0xeb,
             0x95, 0x80, 0x70, 0x97, 0x1b, 0x92, 0x7e, 0x35, 0x8f, 0x5d, 0xc8, 0xae, 0x22, 0x0d,
@@ -470,7 +470,7 @@ fn test_parse_details() {
         PublicKeyAlgorithm::RSA,
         HashAlgorithm::SHA1,
         [0xca, 0x6c],
-        vec![Mpi::from(vec![
+        vec![Mpi::from_raw(vec![
             0x49, 0xa0, 0xb5, 0x41, 0xbd, 0x33, 0xa8, 0xda, 0xda, 0x6e, 0xb1, 0xe5, 0x28, 0x74,
             0x18, 0xee, 0x39, 0xc8, 0x8d, 0xfd, 0x39, 0xe8, 0x3b, 0x09, 0xdc, 0x9d, 0x04, 0x91,
             0x5d, 0x66, 0xb8, 0x1d, 0x04, 0x0a, 0x90, 0xe7, 0xa6, 0x84, 0x9b, 0xb1, 0x06, 0x4f,
@@ -553,7 +553,7 @@ fn test_parse_details() {
         PublicKeyAlgorithm::RSA,
         HashAlgorithm::SHA1,
         [0x02, 0x0c],
-        vec![Mpi::from(vec![
+        vec![Mpi::from_raw(vec![
             0x5b, 0x4b, 0xeb, 0xff, 0x1a, 0x89, 0xc2, 0xe1, 0x80, 0x20, 0x26, 0x3b, 0xf4, 0x4d,
             0x2d, 0x46, 0xba, 0x96, 0x78, 0xb2, 0x88, 0xf8, 0xf9, 0xd5, 0xf1, 0x5f, 0x7d, 0x45,
             0xeb, 0xbc, 0x25, 0x2e, 0x1b, 0x2f, 0x8e, 0xd4, 0xa9, 0x6e, 0x64, 0xfa, 0x97, 0x09,

Original file line number	Diff line number	Diff line change
`@@ -103,12 +103,12 @@ pub fn generate_key<R: Rng + CryptoRng>(`
`103`	`103`	`q.extend_from_slice(&public.to_bytes());`
`104`	`104`
`105`	`105`	`// secret key`
`106`		`- let p = Mpi::from_raw_slice(&secret.to_bytes());`
	`106`	`+ let p = Mpi::from_slice(&secret.to_bytes());`
`107`	`107`
`108`	`108`	`(`
`109`	`109`	`PublicParams::EdDSALegacy {`
`110`	`110`	`curve: ECCCurve::Ed25519,`
`111`		`- q: q.into(),`
	`111`	`+ q: Mpi::from_raw(q),`
`112`	`112`	`},`
`113`	`113`	`PlainSecretParams::EdDSALegacy(p),`
`114`	`114`	`)`
Original file line number	Diff line number	Diff line change
`@@ -103,7 +103,7 @@ pub fn encrypt<R: CryptoRng + Rng>(`
`103`	`103`	`let data = key.encrypt(&mut rng, Pkcs1v15Encrypt, plaintext)?;`
`104`	`104`
`105`	`105`	`Ok(PkeskBytes::Rsa {`
`106`		`- mpi: Mpi::from_raw_slice(&data[..]),`
	`106`	`+ mpi: Mpi::from_slice(&data[..]),`
`107`	`107`	`})`
`108`	`108`	`}`
`109`	`109`
Original file line number	Diff line number	Diff line change
`@@ -77,7 +77,7 @@ pub fn strip_leading_zeros(bytes: &[u8]) -> &[u8] {`
`77`	`77`	`bytes`
`78`	`78`	`.iter()`
`79`	`79`	`.position(\|b\| b != &0)`
`80`		`- .map_or(bytes, \|offset\| &bytes[offset..])`
	`80`	`+ .map_or(&[], \|offset\| &bytes[offset..])`
`81`	`81`	`}`
`82`	`82`
`83`	`83`	`#[inline]`
`@@ -239,4 +239,11 @@ mod tests {`
`239`	`239`	`write_packet_length(12870, &mut res).unwrap();`
`240`	`240`	`assert_eq!(hex::encode(res), "ff00003246");`
`241`	`241`	`}`
	`242`	`+`
	`243`	`+ #[test]`
	`244`	`+ fn test_strip_leading_zeros_with_all_zeros() {`
	`245`	`+ let buf = [0, 0, 0];`
	`246`	`+ let stripped = strip_leading_zeros(&buf);`
	`247`	`+ assert_eq!(stripped, &[]);`
	`248`	`+ }`
`242`	`249`	`}`