chore: rework ecdh public params to be able to represent opaque data

hko-s · hko-s · commit 276768eedd65 · 2024-09-23T16:45:53.000+02:00
diff --git a/src/composed/signed_key/key_parser.rs b/src/composed/signed_key/key_parser.rs
@@ -1,6 +1,6 @@
 use log::{debug, warn};
 
-use crate::errors::Result;
+use crate::errors::{Error, Result};
 use crate::packet::{self, Packet, Signature, SignatureType, UserAttribute, UserId};
 use crate::types::{KeyVersion, PublicKeyTrait, SignedUser, SignedUserAttribute, Tag};
 use crate::{SignedKeyDetails, SignedPublicSubKey, SignedSecretSubKey};
@@ -217,6 +217,24 @@ where
         }
     }
 
+    // Does peeking forward yield an error? If so, we consider this key to be broken.
+    // (This can happen e.g. when an unknown critical packet it encountered within a TPK/TSK)
+    if let Some(Err(e)) = packets.next_if(|peek| peek.is_err()) {
+        match e {
+            // "Unsupported" errors are by definition "soft", these packets are safe to skip silently
+            Error::Unsupported(_) => {}
+
+            _ => {
+                return Some(Err(format_err!(
+                    "error while parsing composed key: {:?}",
+                    e
+                )))
+            }
+        }
+    }
+
+    // TODO: consume any additional packets that aren't a primary key packet?
+
     // 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 {
diff --git a/src/crypto/ecdh.rs b/src/crypto/ecdh.rs
@@ -9,8 +9,7 @@ use crate::crypto::{
     Decryptor, KeyParams,
 };
 use crate::errors::{Error, Result};
-use crate::types::PkeskBytes;
-use crate::types::{Mpi, PlainSecretParams, PublicParams};
+use crate::types::{EcdhPublicParams, Mpi, PkeskBytes, PlainSecretParams, PublicParams};
 
 /// 20 octets representing "Anonymous Sender    ".
 const ANON_SENDER: [u8; 20] = [
@@ -296,12 +295,12 @@ pub fn generate_key<R: Rng + CryptoRng>(
             let alg_sym = curve.sym_algo()?;
 
             Ok((
-                PublicParams::ECDH {
-                    curve,
+                PublicParams::ECDH(EcdhPublicParams::Known {
+                    curve: ECCCurve::Curve25519,
                     p: p.into(),
                     hash,
                     alg_sym,
-                },
+                }),
                 PlainSecretParams::ECDH(Mpi::from_raw(q)),
             ))
         }
@@ -331,12 +330,12 @@ where
     let public = secret.public_key();
 
     Ok((
-        PublicParams::ECDH {
+        PublicParams::ECDH(EcdhPublicParams::Known {
             curve: curve.clone(),
             p: Mpi::from_raw_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())),
     ))
 }
@@ -560,12 +559,12 @@ mod tests {
                     rng.fill_bytes(&mut plain);
 
                     let values = match pkey {
-                        PublicParams::ECDH {
+                        PublicParams::ECDH(EcdhPublicParams::Known {
                             ref curve,
                             ref p,
                             hash,
                             alg_sym,
-                        } => encrypt(
+                        }) => encrypt(
                             &mut rng,
                             curve,
                             alg_sym,
diff --git a/src/packet/key/public.rs b/src/packet/key/public.rs
@@ -4,6 +4,7 @@ use md5::Md5;
 use rand::Rng;
 use sha1_checked::{Digest, Sha1};
 
+use crate::types::EcdhPublicParams;
 use crate::{
     crypto::{self, ecc_curve::ECCCurve, hash::HashAlgorithm, public_key::PublicKeyAlgorithm},
     errors::Result,
@@ -141,10 +142,10 @@ impl PubKeyInner {
         if version != KeyVersion::V4 {
             if matches!(
                 public_params,
-                PublicParams::ECDH {
+                PublicParams::ECDH(EcdhPublicParams::Known {
                     curve: ECCCurve::Curve25519,
                     ..
-                }
+                })
             ) {
                 bail!(
                     "ECDH over Curve25519 is illegal for key version {}",
@@ -465,19 +466,25 @@ impl PublicKeyTrait for PubKeyInner {
 
                 crypto::ecdsa::verify(params, hash, hashed, sig)
             }
-            PublicParams::ECDH {
+            PublicParams::ECDH(EcdhPublicParams::Known {
                 ref curve,
                 ref hash,
                 ref alg_sym,
                 ..
-            } => {
+            }) => {
                 bail!(
                     "ECDH ({:?} {:?} {:?}) can not be used for verify operations",
                     curve,
                     hash,
                     alg_sym
                 );
             }
+            PublicParams::ECDH(EcdhPublicParams::Unsupported { ref curve, .. }) => {
+                bail!(
+                    "ECDH (unsupported: {:?}) can not be used for verify operations",
+                    curve,
+                );
+            }
             PublicParams::Elgamal { .. } => {
                 unimplemented_err!("verify Elgamal");
             }
@@ -520,12 +527,12 @@ impl PublicKeyTrait for PubKeyInner {
             PublicParams::EdDSALegacy { .. } => bail!("EdDSALegacy is only used for signing"),
             PublicParams::Ed25519 { .. } => bail!("Ed25519 is only used for signing"),
             PublicParams::ECDSA { .. } => bail!("ECDSA is only used for signing"),
-            PublicParams::ECDH {
+            PublicParams::ECDH(EcdhPublicParams::Known {
                 ref curve,
                 hash,
                 alg_sym,
                 ref p,
-            } => {
+            }) => {
                 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.
@@ -548,6 +555,9 @@ impl PublicKeyTrait for PubKeyInner {
                     plain,
                 )
             }
+            PublicParams::ECDH(EcdhPublicParams::Unsupported { ref curve, .. }) => {
+                unsupported_err!("ECDH over curve {:?} is unsupported", curve)
+            }
             PublicParams::X25519 { ref public } => {
                 let (sym_alg, plain) = match typ {
                     EskType::V6 => (None, plain),
diff --git a/src/packet/public_key_parser.rs b/src/packet/public_key_parser.rs
@@ -5,12 +5,14 @@ use nom::multi::length_data;
 use nom::number::streaming::{be_u16, be_u32, be_u8};
 use nom::sequence::{pair, tuple};
 
-use crate::crypto::ecc_curve::ecc_curve_from_oid;
+use crate::crypto::ecc_curve::{ecc_curve_from_oid, ECCCurve};
 use crate::crypto::hash::HashAlgorithm;
 use crate::crypto::public_key::PublicKeyAlgorithm;
 use crate::crypto::sym::SymmetricKeyAlgorithm;
 use crate::errors::IResult;
-use crate::types::{mpi, EcdsaPublicParams, KeyVersion, Mpi, MpiRef, PublicParams};
+use crate::types::{
+    mpi, EcdhPublicParams, EcdsaPublicParams, KeyVersion, Mpi, MpiRef, PublicParams,
+};
 
 #[inline]
 fn to_owned(mref: MpiRef<'_>) -> Mpi {
@@ -91,30 +93,46 @@ fn x448(i: &[u8]) -> IResult<&[u8], PublicParams> {
 
 /// Ref: <https://www.rfc-editor.org/rfc/rfc9580.html#name-algorithm-specific-part-for-ecd>
 fn ecdh(i: &[u8]) -> IResult<&[u8], PublicParams> {
-    map(
-        tuple((
-            // a one-octet size of the following field
-            // octets representing a curve OID
-            map_opt(length_data(be_u8), ecc_curve_from_oid),
+    // a one-octet size of the following field
+    // octets representing a curve OID
+    let (i, curve) = map_opt(length_data(be_u8), ecc_curve_from_oid)(i)?;
+
+    match curve {
+        ECCCurve::Curve25519 | ECCCurve::P256 | ECCCurve::P384 | ECCCurve::P521 => {
             // MPI of an EC point representing a public key
-            mpi,
+            let (i, mpi) = mpi(i)?;
+
             // a one-octet size of the following fields
-            be_u8,
+            let (i, _len2) = be_u8(i)?;
+
             // a one-octet value 01, reserved for future extensions
-            tag(&[1][..]),
+            let (i, _tag) = tag(&[1][..])(i)?;
+
             // a one-octet hash function ID used with a KDF
-            map(be_u8, HashAlgorithm::from),
+            let (i, hash) = map(be_u8, HashAlgorithm::from)(i)?;
+
             // a one-octet algorithm ID for the symmetric algorithm used to wrap
             // the symmetric key used for the message encryption
-            map_res(be_u8, SymmetricKeyAlgorithm::try_from),
+            let (i, alg_sym) = map_res(be_u8, SymmetricKeyAlgorithm::try_from)(i)?;
+
+            Ok((
+                i,
+                PublicParams::ECDH(EcdhPublicParams::Known {
+                    curve,
+                    p: mpi.to_owned(),
+                    hash,
+                    alg_sym,
+                }),
+            ))
+        }
+        _ => Ok((
+            i,
+            PublicParams::ECDH(EcdhPublicParams::Unsupported {
+                curve,
+                opaque: vec![],
+            }),
         )),
-        |(curve, p, _len2, _tag, hash, alg_sym)| PublicParams::ECDH {
-            curve,
-            p: p.to_owned(),
-            hash,
-            alg_sym,
-        },
-    )(i)
+    }
 }
 
 fn elgamal(i: &[u8]) -> IResult<&[u8], PublicParams> {
diff --git a/src/types/params/plain_secret.rs b/src/types/params/plain_secret.rs
@@ -178,12 +178,12 @@ impl<'a> PlainSecretParamsRef<'a> {
                 _ => unreachable!("inconsistent key state"),
             },
             PlainSecretParamsRef::ECDH(d) => match public_params {
-                PublicParams::ECDH {
+                PublicParams::ECDH(EcdhPublicParams::Known {
                     ref curve,
                     ref hash,
                     ref alg_sym,
                     ..
-                } => match curve {
+                }) => match curve {
                     ECCCurve::Curve25519 => {
                         const SIZE: usize = ECCCurve::Curve25519.secret_key_length();
 
@@ -224,6 +224,9 @@ impl<'a> PlainSecretParamsRef<'a> {
                     }
                     _ => unsupported_err!("curve {:?} for ECDH", curve.to_string()),
                 },
+                PublicParams::ECDH(EcdhPublicParams::Unsupported { ref curve, .. }) => {
+                    unsupported_err!("curve {:?} for ECDH", curve)
+                }
                 _ => unreachable!("inconsistent key state"),
             },
             PlainSecretParamsRef::EdDSALegacy(d) => match public_params {
diff --git a/src/types/params/public.rs b/src/types/params/public.rs
@@ -12,44 +12,30 @@ use crate::types::{Mpi, MpiRef};
 /// Represent the public parameters for the different algorithms.
 #[derive(PartialEq, Eq, Clone, Debug)]
 pub enum PublicParams {
-    RSA {
-        n: Mpi,
-        e: Mpi,
-    },
-    DSA {
-        p: Mpi,
-        q: Mpi,
-        g: Mpi,
-        y: Mpi,
-    },
+    RSA { n: Mpi, e: Mpi },
+    DSA { p: Mpi, q: Mpi, g: Mpi, y: Mpi },
     ECDSA(EcdsaPublicParams),
-    ECDH {
+    ECDH(EcdhPublicParams),
+    Elgamal { p: Mpi, g: Mpi, y: Mpi },
+    EdDSALegacy { curve: ECCCurve, q: Mpi },
+    Ed25519 { public: [u8; 32] },
+    X25519 { public: [u8; 32] },
+    X448 { public: [u8; 56] },
+    Unknown { data: Vec<u8> },
+}
+
+#[derive(Debug, PartialEq, Eq, Clone)]
+pub enum EcdhPublicParams {
+    /// ECDH public parameters for a curve that we know uses Mpi representation
+    Known {
         curve: ECCCurve,
         p: Mpi,
         hash: HashAlgorithm,
         alg_sym: SymmetricKeyAlgorithm,
     },
-    Elgamal {
-        p: Mpi,
-        g: Mpi,
-        y: Mpi,
-    },
-    EdDSALegacy {
-        curve: ECCCurve,
-        q: Mpi,
-    },
-    Ed25519 {
-        public: [u8; 32],
-    },
-    X25519 {
-        public: [u8; 32],
-    },
-    X448 {
-        public: [u8; 56],
-    },
-    Unknown {
-        data: Vec<u8>,
-    },
+
+    /// Public parameters for a curve that we don't know about (which might not use Mpi representation).
+    Unsupported { curve: ECCCurve, opaque: Vec<u8> },
 }
 
 #[derive(Debug, PartialEq, Eq, Clone)]
@@ -201,12 +187,12 @@ impl Serialize for PublicParams {
             PublicParams::ECDSA(params) => {
                 params.to_writer(writer)?;
             }
-            PublicParams::ECDH {
+            PublicParams::ECDH(EcdhPublicParams::Known {
                 ref curve,
                 ref p,
                 ref hash,
                 ref alg_sym,
-            } => {
+            }) => {
                 let oid = curve.oid();
                 writer.write_u8(oid.len().try_into()?)?;
                 writer.write_all(&oid)?;
@@ -218,6 +204,16 @@ impl Serialize for PublicParams {
                 writer.write_u8((*hash).into())?;
                 writer.write_u8((*alg_sym).into())?;
             }
+            PublicParams::ECDH(EcdhPublicParams::Unsupported {
+                ref curve,
+                ref opaque,
+            }) => {
+                let oid = curve.oid();
+                writer.write_u8(oid.len().try_into()?)?;
+                writer.write_all(&oid)?;
+
+                writer.write_all(opaque)?;
+            }
             PublicParams::Elgamal {
                 ref p,
                 ref g,
diff --git a/tests/hsm-test.rs b/tests/hsm-test.rs
@@ -8,7 +8,7 @@ use pgp::crypto::hash::HashAlgorithm;
 use pgp::crypto::public_key::PublicKeyAlgorithm;
 use pgp::crypto::sym::SymmetricKeyAlgorithm;
 use pgp::packet::{PacketTrait, PublicKey, SignatureConfig};
-use pgp::types::{EskType, Fingerprint, PkeskBytes, SignatureBytes};
+use pgp::types::{EcdhPublicParams, EskType, Fingerprint, PkeskBytes, SignatureBytes};
 use pgp::types::{KeyId, Mpi, PublicKeyTrait, PublicParams, SecretKeyTrait};
 use pgp::{packet, Deserializable, Esk};
 use pgp::{Message, SignedPublicKey};
@@ -187,12 +187,12 @@ impl FakeHsm {
             }
 
             (
-                PublicParams::ECDH {
+                PublicParams::ECDH(EcdhPublicParams::Known {
                     curve,
                     alg_sym,
                     hash,
                     ..
-                },
+                }),
                 PkeskBytes::Ecdh {
                     public_point,
                     encrypted_session_key,
diff --git a/tests/key_test.rs b/tests/key_test.rs
