ssh-key: private key checkint improvements (#577)

This commit adds support for storing the "checkint" for a private key
in the `PrivateKey` type. This makes it possible to reserialize a
decoded private key byte-for-byte.

Additionally, encryption and random key generation now generate a random
"checkint".

The fallback deterministic method is moved from the public to private
key, ensuring that a "checkint" is always secret. This should help
prevent hypothetical bitflipping attacks which would be possible if the
deterministic checkint were computed from public data.

ssh-key/src/private.rs

@@ -173,6 +173,9 @@ pub struct PrivateKey {
     /// KDF options.
     kdf: Kdf,
 
+    /// "Checkint" value used to verify successful decryption.
+    checkint: Option<u32>,
+
     /// Public key.
     public_key: PublicKey,
 
@@ -250,6 +253,7 @@ impl PrivateKey {
             return Ok(Self {
                 cipher,
                 kdf,
+                checkint: None,
                 public_key: public_key.into(),
                 key_data: KeypairData::Encrypted(ciphertext),
             });
@@ -289,8 +293,9 @@ impl PrivateKey {
             self.key_data.encode(&mut pem_encoder)?;
         } else {
             let len = self.encoded_privatekey_comment_pair_len(Cipher::None)?;
+            let checkint = self.checkint.unwrap_or_else(|| self.key_data.checkint());
             pem_encoder.encode_usize(len)?;
-            self.encode_privatekey_comment_pair(&mut pem_encoder, Cipher::None)?;
+            self.encode_privatekey_comment_pair(&mut pem_encoder, Cipher::None, checkint)?;
         }
 
         let encoded_len = pem_encoder.finish()?;
@@ -370,12 +375,15 @@ impl PrivateKey {
     #[cfg_attr(docsrs, doc(cfg(feature = "encryption")))]
     pub fn encrypt(
         &self,
-        rng: impl CryptoRng + RngCore,
+        mut rng: impl CryptoRng + RngCore,
         password: impl AsRef<[u8]>,
     ) -> Result<Self> {
+        let checkint = rng.next_u32();
+
         self.encrypt_with(
             Cipher::default(),
             Kdf::new(Default::default(), rng)?,
+            checkint,
             password,
         )
     }
@@ -390,6 +398,7 @@ impl PrivateKey {
         &self,
         cipher: Cipher,
         kdf: Kdf,
+        checkint: u32,
         password: impl AsRef<[u8]>,
     ) -> Result<Self> {
         if self.is_encrypted() {
@@ -401,12 +410,13 @@ impl PrivateKey {
         let mut out = Vec::with_capacity(msg_len);
 
         // Encode and encrypt private key
-        self.encode_privatekey_comment_pair(&mut out, cipher)?;
+        self.encode_privatekey_comment_pair(&mut out, cipher, checkint)?;
         cipher.encrypt(&key_bytes, &iv_bytes, out.as_mut_slice())?;
 
         Ok(Self {
             cipher,
             kdf,
+            checkint: None,
             public_key: self.public_key.key_data.clone().into(),
             key_data: KeypairData::Encrypted(out),
         })
@@ -463,13 +473,15 @@ impl PrivateKey {
     /// Generate a random Ed25519 private key.
     #[cfg(feature = "ed25519")]
     #[cfg_attr(docsrs, doc(cfg(feature = "ed25519")))]
-    pub fn random_ed25519(rng: impl CryptoRng + RngCore) -> Self {
+    pub fn random_ed25519(mut rng: impl CryptoRng + RngCore) -> Self {
+        let checkint = rng.next_u32();
         let key_data = KeypairData::from(Ed25519Keypair::random(rng));
         let public_key = public::KeyData::try_from(&key_data).expect("invalid key");
 
         Self {
             cipher: Cipher::None,
             kdf: Kdf::None,
+            checkint: Some(checkint),
             public_key: public_key.into(),
             key_data,
         }
@@ -559,6 +571,7 @@ impl PrivateKey {
         Ok(Self {
             cipher: Cipher::None,
             kdf: Kdf::None,
+            checkint: Some(checkint1),
             public_key,
             key_data,
         })
@@ -570,12 +583,11 @@ impl PrivateKey {
         &self,
         encoder: &mut impl Encoder,
         cipher: Cipher,
+        checkint: u32,
     ) -> Result<()> {
         let unpadded_len = self.unpadded_privatekey_comment_pair_len()?;
         let padding_len = cipher.padding_len(unpadded_len);
 
-        // Compute checkint (uses deterministic method)
-        let checkint = public::KeyData::try_from(&self.key_data)?.checkint();
         encoder.encode_u32(checkint)?;
         encoder.encode_u32(checkint)?;
         self.key_data.encode(encoder)?;
@@ -660,6 +672,7 @@ impl TryFrom<KeypairData> for PrivateKey {
         Ok(Self {
             cipher: Cipher::None,
             kdf: Kdf::None,
+            checkint: None,
             public_key: public_key.into(),
             key_data,
         })

ssh-key/src/private/keypair.rs

@@ -153,6 +153,32 @@ impl KeypairData {
     pub fn is_rsa(&self) -> bool {
         matches!(self, Self::Rsa(_))
     }
+
+    /// Compute a deterministic "checkint" for this private key.
+    ///
+    /// This is a sort of primitive pseudo-MAC used by the OpenSSH key format.
+    // TODO(tarcieri): true randomness or a better algorithm?
+    pub(super) fn checkint(&self) -> u32 {
+        let bytes = match self {
+            #[cfg(feature = "alloc")]
+            Self::Dsa(dsa) => dsa.private.as_bytes(),
+            #[cfg(feature = "ecdsa")]
+            Self::Ecdsa(ecdsa) => ecdsa.private_key_bytes(),
+            Self::Ed25519(ed25519) => ed25519.private.as_ref(),
+            #[cfg(feature = "alloc")]
+            Self::Encrypted(ciphertext) => ciphertext.as_ref(),
+            #[cfg(feature = "alloc")]
+            Self::Rsa(rsa) => rsa.private.d.as_bytes(),
+        };
+
+        let mut n = 0u32;
+
+        for chunk in bytes.chunks_exact(4) {
+            n ^= u32::from_be_bytes(chunk.try_into().expect("not 4 bytes"));
+        }
+
+        n
+    }
 }
 
 impl Decode for KeypairData {

ssh-key/src/public.rs

@@ -322,30 +322,6 @@ impl KeyData {
         matches!(self, Self::Rsa(_))
     }
 
-    /// Compute a "checkint" from a public key.
-    ///
-    /// This is a sort of primitive pseudo-MAC used by the OpenSSH key format.
-    // TODO(tarcieri): true randomness or a better algorithm?
-    pub(crate) fn checkint(&self) -> u32 {
-        let bytes = match self {
-            #[cfg(feature = "alloc")]
-            Self::Dsa(dsa) => dsa.checkint_bytes(),
-            #[cfg(feature = "ecdsa")]
-            Self::Ecdsa(ecdsa) => ecdsa.as_sec1_bytes(),
-            Self::Ed25519(ed25519) => ed25519.as_ref(),
-            #[cfg(feature = "alloc")]
-            Self::Rsa(rsa) => rsa.checkint_bytes(),
-        };
-
-        let mut n = 0u32;
-
-        for chunk in bytes.chunks_exact(4) {
-            n ^= u32::from_be_bytes(chunk.try_into().expect("not 4 bytes"));
-        }
-
-        n
-    }
-
     /// Decode [`KeyData`] for the specified algorithm.
     pub(crate) fn decode_algorithm(
         decoder: &mut impl Decoder,

ssh-key/src/public/dsa.rs

@@ -27,15 +27,6 @@ pub struct DsaPublicKey {
     pub y: MPInt,
 }
 
-impl DsaPublicKey {
-    /// Borrow the bytes used to compute a "checkint" for this key.
-    ///
-    /// This is a sort of primitive pseudo-MAC used by the OpenSSH key format.
-    pub(super) fn checkint_bytes(&self) -> &[u8] {
-        self.y.as_bytes()
-    }
-}
-
 impl Decode for DsaPublicKey {
     fn decode(decoder: &mut impl Decoder) -> Result<Self> {
         let p = MPInt::decode(decoder)?;

ssh-key/src/public/rsa.rs

@@ -20,15 +20,6 @@ pub struct RsaPublicKey {
     pub n: MPInt,
 }
 
-impl RsaPublicKey {
-    /// Borrow the bytes used to compute a "checkint" for this key.
-    ///
-    /// This is a sort of primitive pseudo-MAC used by the OpenSSH key format.
-    pub(super) fn checkint_bytes(&self) -> &[u8] {
-        self.n.as_bytes()
-    }
-}
-
 impl Decode for RsaPublicKey {
     fn decode(decoder: &mut impl Decoder) -> Result<Self> {
         let e = MPInt::decode(decoder)?;