fix: implement various constraints that rfc 9580 mandates

hko-s · hko-s · commit 5682b08f1522 · 2024-09-23T15:46:20.000+02:00
diff --git a/src/composed/signed_key/key_parser.rs b/src/composed/signed_key/key_parser.rs
@@ -217,6 +217,27 @@ where
         }
     }
 
+    // Every subkey for a version 6 primary key MUST be a version 6 subkey.
+    // (See https://www.rfc-editor.org/rfc/rfc9580.html#section-10.1.1-5)
+    if primary_key.version() == KeyVersion::V6 {
+        for sub in &public_subkey_container {
+            if sub.version() != KeyVersion::V6 {
+                return Some(Err(crate::errors::Error::Message(format!(
+                    "Illegal public subkey {:?} in v6 key",
+                    sub.version()
+                ))));
+            }
+        }
+        for sub in &secret_subkey_container {
+            if sub.version() != KeyVersion::V6 {
+                return Some(Err(crate::errors::Error::Message(format!(
+                    "Illegal secret subkey {:?} in v6 key",
+                    sub.version()
+                ))));
+            }
+        }
+    }
+
     Some(Ok((
         primary_key,
         SignedKeyDetails::new(
diff --git a/src/crypto/ecdh.rs b/src/crypto/ecdh.rs
@@ -424,6 +424,16 @@ pub fn encrypt<R: CryptoRng + Rng>(
 ) -> Result<PkeskBytes> {
     debug!("ECDH encrypt");
 
+    // Implementations MUST NOT use MD5, SHA-1, or RIPEMD-160 as a hash function in an ECDH KDF.
+    // (See https://www.rfc-editor.org/rfc/rfc9580.html#section-9.5-3)
+    ensure!(
+        hash != HashAlgorithm::MD5
+            && hash != HashAlgorithm::SHA1
+            && hash != HashAlgorithm::RIPEMD160,
+        "{:?} is not a legal hash function for ECDH KDF",
+        hash
+    );
+
     // Maximum length for `plain`:
     // - padding increases the length (at least) to a length of the next multiple of 8.
     // - aes keywrap adds another 8 bytes
diff --git a/src/crypto/ecdsa.rs b/src/crypto/ecdsa.rs
@@ -175,6 +175,7 @@ pub fn verify(
     hashed: &[u8],
     sig: &[Mpi],
 ) -> Result<()> {
+    // NOTE: the `None` case will run into an `unsupported_err`, below, so it's ok not to consider it here
     if let Some(field_size) = p.secret_key_length() {
         // Error out for size mismatches that would get rejected in ecdsa::hazmat::bits2field
         ensure!(
@@ -183,6 +184,22 @@ pub fn verify(
             hash,
             p
         );
+
+        // RFC 9580:
+        // An ECDSA signature MUST use a hash algorithm with a digest size of at least the curve's "fsize" value [..]"
+        // (See https://www.rfc-editor.org/rfc/rfc9580.html#section-5.2.3.2-5)
+        let min_hash_len = match field_size {
+            // "[..] except in the case of NIST P-521, for which at least a 512-bit hash algorithm MUST be used"
+            521 => 512,
+
+            f => f,
+        };
+        ensure!(
+            hash.digest_size() * 8 >= min_hash_len,
+            "ECDSA signature: hash algorithm {:?} is too weak for key {:?}",
+            hash,
+            p
+        );
     }
 
     match p {
diff --git a/src/crypto/eddsa.rs b/src/crypto/eddsa.rs
@@ -126,12 +126,18 @@ pub fn generate_key<R: Rng + CryptoRng>(
 pub fn verify(
     curve: &ECCCurve,
     public: &[u8],
-    _hash: HashAlgorithm,
+    hash: HashAlgorithm,
     hashed: &[u8],
     sig_bytes: &[u8],
 ) -> Result<()> {
     match *curve {
         ECCCurve::Ed25519 => {
+            ensure!(
+                hash.digest_size() * 8 >= 256,
+                "EdDSA signature: hash algorithm {:?} is too weak for Ed25519",
+                hash,
+            );
+
             let pk: ed25519_dalek::VerifyingKey = public.try_into()?;
             let sig = sig_bytes.try_into()?;
 
diff --git a/src/crypto/sym.rs b/src/crypto/sym.rs
@@ -229,7 +229,9 @@ impl SymmetricKeyAlgorithm {
 
         {
             match self {
-                SymmetricKeyAlgorithm::Plaintext => {}
+                SymmetricKeyAlgorithm::Plaintext => {
+                    bail!("'Plaintext' is not a legal cipher for encrypted data")
+                }
                 SymmetricKeyAlgorithm::IDEA => decrypt!(
                     Idea,
                     key,
@@ -350,7 +352,9 @@ impl SymmetricKeyAlgorithm {
         ciphertext: &mut [u8],
     ) -> Result<()> {
         match self {
-            SymmetricKeyAlgorithm::Plaintext => {}
+            SymmetricKeyAlgorithm::Plaintext => {
+                bail!("'Plaintext' is not a legal cipher for encrypted data")
+            }
             SymmetricKeyAlgorithm::IDEA => {
                 decrypt_regular!(Idea, key, iv_vec, ciphertext)
             }
@@ -488,7 +492,9 @@ impl SymmetricKeyAlgorithm {
 
         {
             match self {
-                SymmetricKeyAlgorithm::Plaintext => {}
+                SymmetricKeyAlgorithm::Plaintext => {
+                    bail!("'Plaintext' is not a legal cipher for encrypted data")
+                }
                 SymmetricKeyAlgorithm::IDEA => {
                     encrypt!(Idea, key, iv_vec, prefix, data, bs, resync)
                 }
@@ -540,7 +546,9 @@ impl SymmetricKeyAlgorithm {
     ) -> Result<()> {
         // TODO: actual cfb mode used in pgp
         match self {
-            SymmetricKeyAlgorithm::Plaintext => {}
+            SymmetricKeyAlgorithm::Plaintext => {
+                bail!("'Plaintext' is not a legal cipher for encrypted data")
+            }
             SymmetricKeyAlgorithm::IDEA => encrypt_regular!(Idea, key, iv_vec, plaintext),
             SymmetricKeyAlgorithm::TripleDES => {
                 encrypt_regular!(TdesEde3, key, iv_vec, plaintext);
diff --git a/src/packet/key/public.rs b/src/packet/key/public.rs
@@ -525,15 +525,29 @@ impl PublicKeyTrait for PubKeyInner {
                 hash,
                 alg_sym,
                 ref p,
-            } => crypto::ecdh::encrypt(
-                rng,
-                curve,
-                alg_sym,
-                hash,
-                self.fingerprint().as_bytes(),
-                p.as_bytes(),
-                plain,
-            ),
+            } => {
+                if self.version() == KeyVersion::V6 {
+                    // An implementation MUST NOT encrypt any message to a version 6 ECDH key over a
+                    // listed curve that announces a different KDF or KEK parameter.
+                    //
+                    // (See https://www.rfc-editor.org/rfc/rfc9580.html#section-11.5.1-2)
+                    if curve.hash_algo()? != hash || curve.sym_algo()? != alg_sym {
+                        bail!("Unsupported KDF/KEK parameters for {:?} and KeyVersion::V6: {:?}, {:?}",curve,
+                           hash,
+                            alg_sym);
+                    }
+                }
+
+                crypto::ecdh::encrypt(
+                    rng,
+                    curve,
+                    alg_sym,
+                    hash,
+                    self.fingerprint().as_bytes(),
+                    p.as_bytes(),
+                    plain,
+                )
+            }
             PublicParams::X25519 { ref public } => {
                 let (sym_alg, plain) = match typ {
                     EskType::V6 => (None, plain),
diff --git a/src/packet/public_key_encrypted_session_key.rs b/src/packet/public_key_encrypted_session_key.rs
@@ -113,6 +113,12 @@ impl PublicKeyEncryptedSessionKey {
         session_key: &[u8],
         pkey: &impl PublicKeyTrait,
     ) -> Result<Self> {
+        // "An implementation MUST NOT generate ElGamal v6 PKESK packets."
+        // (See https://www.rfc-editor.org/rfc/rfc9580.html#section-5.1.4-6)
+        if pkey.algorithm() == PublicKeyAlgorithm::Elgamal {
+            bail!("ElGamal is not a legal encryption mechanism for v6 PKESK");
+        }
+
         // the session key, and a checksum (for some algorithms)
         let data =
             Self::prepare_session_key_for_encryption(None, session_key, pkey.public_params());
diff --git a/src/packet/signature/config.rs b/src/packet/signature/config.rs
@@ -430,6 +430,18 @@ impl SignatureConfig {
                         bail!("Unknown critical subpacket {:?}", packet);
                     }
 
+                    // If the version octet does not match the signature version, the receiving
+                    // implementation MUST treat it as a malformed signature
+                    //
+                    // (See https://www.rfc-editor.org/rfc/rfc9580.html#section-5.2.3.35-3)
+                    if let SubpacketData::IssuerFingerprint(fp) = &packet.data {
+                        match (self.version(), fp.version()) {
+                            (SignatureVersion::V6, Some(KeyVersion::V6)) => {},
+                            (SignatureVersion::V4, Some(KeyVersion::V4)) => {},
+                            _ => bail!("IntendedRecipientFingerprint {:?} doesn't match signature version {:?}", fp, self.version())
+                        }
+                    }
+
                     packet.to_writer(&mut hashed_subpackets)?;
                 }
 
diff --git a/src/packet/signature/types.rs b/src/packet/signature/types.rs
@@ -209,6 +209,17 @@ impl Signature {
         let mut hasher = self.config.hash_alg.new_hasher()?;
 
         if let SignatureVersionSpecific::V6 { salt } = &self.config.version_specific {
+            // Salt size must match the expected length for the hash algorithm that is used
+            //
+            // See: https://www.rfc-editor.org/rfc/rfc9580.html#section-5.2.3-2.10.2.1.1
+            ensure_eq!(
+                self.config.hash_alg.salt_len(),
+                Some(salt.len()),
+                "Illegal salt length {} for a V6 Signature using {:?}",
+                salt.len(),
+                self.config.hash_alg
+            );
+
             hasher.update(salt.as_ref())
         }
 
diff --git a/src/types/params/secret.rs b/src/types/params/secret.rs
@@ -43,6 +43,17 @@ impl SecretParams {
         params: &PublicParams,
     ) -> Result<Self> {
         let (_, params) = parse_secret_fields(key_ver, alg, params)(data)?;
+
+        // Version 6 secret keys may only use "S2K Usage" 0, 253 or 254
+        //
+        // See: https://www.rfc-editor.org/rfc/rfc9580.html#section-3.7.2.1-8
+        if key_ver == KeyVersion::V6 && ![0, 253, 254].contains(&params.string_to_key_id()) {
+            bail!(
+                "Illegal S2K Usage setting {} for a V6 key",
+                params.string_to_key_id()
+            )
+        }
+
         Ok(params)
     }
 
