From e12d655245ab5f00656cdbb907f71fbf11fc3ea5 Mon Sep 17 00:00:00 2001
From: ryan <ryan@distrust.co>
Date: Wed, 26 Feb 2025 01:46:45 -0500
Subject: [PATCH] miniquorum: dedup signatures, invalidate duped signatures

---
 crates/miniquorum/src/lib.rs | 81 ++++++++++++++++++++++++++++++++++--
 1 file changed, 77 insertions(+), 4 deletions(-)

diff --git a/crates/miniquorum/src/lib.rs b/crates/miniquorum/src/lib.rs
index f57f351..af889d8 100644
--- a/crates/miniquorum/src/lib.rs
+++ b/crates/miniquorum/src/lib.rs
@@ -61,6 +61,26 @@ pub enum BaseError {
     /// The JSON object is not a valid value.
     #[error("the JSON object is not a valid value")]
     InvalidJSONValue,
+
+    /// No signing key was found on smartcard.
+    #[error("no signing key was found on smartcard")]
+    NoSigningKey,
+
+    /// A signature exists for the current smartcard.
+    #[error("a signature exists for the key on the current smartcard: {0}")]
+    ConflictingSignature(openpgp::Fingerprint),
+
+    /// A bad packet type was encountered.
+    #[error("a bad OpenPGP packet was encountered: {0}")]
+    BadOpenPGPPacket(openpgp::packet::Tag),
+
+    /// A signature could not have been added; a smartcard might not have been pluggedi n.
+    #[error("a signature could not be added")]
+    NoSignatureAdded,
+
+    /// The signature matched a key that was already used to verify another signature.
+    #[error("signature {1} matched key {0} previously used to sign signature {2}")]
+    DuplicateSignature(openpgp::Fingerprint, usize, usize),
 }
 
 impl BaseError {
@@ -151,7 +171,11 @@ impl PayloadVerification {
 
     /// Set a threshold for required signatures.
     pub fn with_threshold(self, threshold: u8) -> Self {
-        Self { one_each: false, threshold, ..self }
+        Self {
+            one_each: false,
+            threshold,
+            ..self
+        }
     }
 
     /// Require a single valid signature; other signatures may be invalid.
@@ -259,6 +283,12 @@ impl Payload {
     ///
     /// The method may error if a signature could not be created.
     pub fn add_signature(&mut self) -> Result<(), Box<dyn std::error::Error>> {
+        let signatures = self
+            .signatures
+            .iter()
+            .map(|signature_text| Packet::from_bytes(signature_text.as_bytes()).map_err(Into::into))
+            .collect::<Result<Vec<_>, Box<dyn std::error::Error>>>()?;
+
         let unhashed = unhashed(serde_json::to_value(&self)?)?;
         let builder =
             SignatureBuilder::new(SignatureType::Binary).set_hash_algo(HashAlgorithm::SHA512);
@@ -268,10 +298,26 @@ impl Payload {
             ..Default::default()
         };
 
+        let mut has_signed_any = false;
         for backend in card_backend_pcsc::PcscBackend::cards(None)? {
             let mut card = Card::<Open>::new(backend?)?;
             let mut transaction = card.transaction()?;
 
+            let key_fps = transaction.fingerprints()?;
+            let signing_key_fp = key_fps.signature().ok_or(BaseError::NoSigningKey)?;
+
+            for packet in &signatures {
+                let Packet::Signature(signature) = packet else {
+                    return Err(BaseError::BadOpenPGPPacket(packet.tag()).into());
+                };
+
+                for issuer_fp in signature.issuer_fingerprints() {
+                    if issuer_fp.as_bytes() == signing_key_fp.as_bytes() {
+                        return Err(BaseError::ConflictingSignature(issuer_fp.clone()).into());
+                    }
+                }
+            }
+
             let cardholder_name = format_name(transaction.cardholder_name()?);
             let card_id = transaction.application_identifier()?.ident();
             let mut pin = None;
@@ -329,9 +375,14 @@ impl Payload {
             writer.finalize()?;
 
             self.signatures.push(String::from_utf8(armored_signature)?);
+            has_signed_any = true;
         }
 
-        Ok(())
+        if has_signed_any {
+            Ok(())
+        } else {
+            Err(BaseError::NoSignatureAdded.into())
+        }
     }
 
     /// Verify the keychain and certificates using either a Key ID or an OpenPGP card.
@@ -370,14 +421,32 @@ impl Payload {
             threshold = certs.len() as u8;
         }
 
-        for signature in &self.signatures {
+        let mut seen = std::collections::HashMap::new();
+
+        for (index, signature) in self.signatures.iter().enumerate() {
+            dbg!(&index);
             let packet = Packet::from_bytes(signature.as_bytes())?;
             let Packet::Signature(signature) = packet else {
                 panic!("bad packet found: {}", packet.tag());
             };
             let mut signature_matched = false;
-            for issuer in signature.get_issuers() {
+            // NOTE: It is allowable, by the specification, to have a packet that doesn't include
+            // an issuer fingerprint, but instead just a key ID. However, filtering by both key ID
+            // and by fingerprint triggers the "duplicate signature" mechanism. For that reason, we
+            // are only going to filter over fingerprints.
+            //
+            // Any program that makes these signatures should be using fingerprints.
+            for issuer in signature.issuer_fingerprints() {
+                let mut currently_seen = std::collections::HashMap::new();
                 for cert in &certs {
+                    if let Some(seen_index) = seen.get(&cert.fingerprint()) {
+                        return Err(BaseError::DuplicateSignature(
+                            cert.fingerprint(),
+                            index,
+                            *seen_index,
+                        )
+                        .into());
+                    }
                     match cert
                         .with_policy(&policy, None)?
                         .keys()
@@ -390,6 +459,9 @@ impl Payload {
                         Some(Ok(())) => {
                             // key found, signature matched
                             signature_matched = true;
+
+                            // mark the cert as seen, so it isn't reusable
+                            currently_seen.insert(cert.fingerprint(), index);
                         }
                         Some(Err(e)) => {
                             if error_on_invalid {
@@ -401,6 +473,7 @@ impl Payload {
                         }
                     }
                 }
+                seen.extend(currently_seen);
             }
 
             if signature_matched {