openpgp: Make the embedded signature accessor return an iterator.

  - A signature could have multiple embedded signatures, e.g. in the
    case of notarizations.  Reflect that in our API.

  - Fixes #574.

4 files changed, 157 insertions, 41 deletions

diff --git a/openpgp/src/cert/bundle.rs b/openpgp/src/cert/bundle.rs
index 42a78065..374334c1 100644
--- a/openpgp/src/cert/bundle.rs
+++ b/openpgp/src/cert/bundle.rs
@@ -290,7 +290,7 @@ impl<C> ComponentBundle<C> {
         // `t`.
         let mut error = None;
 
-        for s in self.self_signatures[i..].iter() {
+        'next_sig: for s in self.self_signatures[i..].iter() {
             if let Err(e) = s.signature_alive(t, time::Duration::new(0, 0)) {
                 // We know that t >= signature's creation time.  So,
                 // it is expired.  But an older signature might not
@@ -313,7 +313,10 @@ impl<C> ComponentBundle<C> {
             if s.typ() == crate::types::SignatureType::SubkeyBinding &&
                 s.key_flags().map(|kf| kf.for_signing()).unwrap_or(false)
             {
-                if let Some(backsig) = s.embedded_signature() {
+                let mut n = 0;
+                let mut one_good_backsig = false;
+                'next_backsig: for backsig in s.embedded_signatures() {
+                    n += 1;
                     if let Err(e) = backsig.signature_alive(
                         t, time::Duration::new(0, 0))
                     {
@@ -322,25 +325,34 @@ impl<C> ComponentBundle<C> {
                         if error.is_none() {
                             error = Some(e);
                         }
-                        continue;
+                        continue 'next_backsig;
                     }
 
                     if let Err(e) = policy.signature(backsig) {
                         if error.is_none() {
                             error = Some(e);
                         }
-                        continue;
+                        continue 'next_backsig;
                     }
-                } else {
+
+                    one_good_backsig = true;
+                }
+
+                if n == 0 {
                     // This shouldn't happen because
                     // Signature::verify_subkey_binding checks for the
-                    // primary key signature.  But, better be safe.
+                    // primary key binding signature.  But, better be
+                    // safe.
                     if error.is_none() {
                         error = Some(Error::BadSignature(
                             "Primary key binding signature missing".into())
                                      .into());
                     }
-                    continue;
+                    continue 'next_sig;
+                }
+
+                if ! one_good_backsig {
+                    continue 'next_sig;
                 }
             }
 
diff --git a/openpgp/src/cert/mod.rs b/openpgp/src/cert/mod.rs
index b3ff496f..19dae9ec 100644
--- a/openpgp/src/cert/mod.rs
+++ b/openpgp/src/cert/mod.rs
@@ -5482,4 +5482,78 @@ Pu1xwz57O4zo1VYf6TqHJzVC3OMvMUM2hhdecMUe5x6GorNaj6g=
         assert_eq!(malicious_cert.bad_signatures().len(), 1);
         Ok(())
     }
+
+    /// Checks that multiple embedded signatures are correctly
+    /// handled.
+    #[test]
+    fn multiple_embedded_signatures() -> Result<()> {
+        use crate::packet::{
+            key::Key4,
+            signature::{
+                SignatureBuilder,
+                subpacket::{Subpacket, SubpacketValue},
+            },
+        };
+
+        // We'll study this certificate, because it contains a
+        // signing-capable subkey.
+        let cert = crate::Cert::from_bytes(crate::tests::key(
+            "emmelie-dorothea-dina-samantha-awina-ed25519.pgp"))?;
+
+        // Add a bogus but plausible embedded signature subpacket with
+        // this key.
+        let key: key::SecretKey
+            = Key4::generate_ecc(true, Curve::Ed25519)?.into();
+        let mut pair = key.into_keypair()?;
+
+        // Create a malicious cert to merge in.
+        let mut pp = crate::PacketPile::from_bytes(crate::tests::key(
+            "emmelie-dorothea-dina-samantha-awina-ed25519.pgp"))?;
+        assert_eq!(pp.children().count(), 5);
+
+        if let Some(Packet::Signature(sig)) = pp.path_ref_mut(&[4]) {
+            // Prepend a bad backsig.
+            let backsig = sig.embedded_signatures().nth(0).unwrap().clone();
+            sig.unhashed_area_mut().replace(Subpacket::new(
+                SubpacketValue::EmbeddedSignature(
+                    SignatureBuilder::new(SignatureType::PrimaryKeyBinding)
+                        .sign_primary_key_binding(
+                            &mut pair,
+                            cert.primary_key().key(),
+                            cert.keys().subkeys().nth(0).unwrap().key())?),
+                false)?)?;
+            sig.unhashed_area_mut().add(Subpacket::new(
+                SubpacketValue::EmbeddedSignature(backsig), false)?)?;
+        } else {
+            panic!("expected a signature");
+        }
+
+        // Parse into cert.
+        use std::convert::TryFrom;
+        let malicious_cert = Cert::try_from(pp)?;
+        // The subkey binding signature should still be fine.
+        let p = &crate::policy::StandardPolicy::new();
+        assert_eq!(malicious_cert.with_policy(p, None)?.keys().subkeys()
+                   .for_signing().count(), 1);
+        assert_eq!(malicious_cert.bad_signatures().len(), 0);
+
+        // Now try to merge it in.
+        let merged = cert.clone().merge(malicious_cert.clone())?;
+        // The subkey binding signature should still be fine.
+        assert_eq!(merged.with_policy(p, None)?.keys().subkeys()
+                   .for_signing().count(), 1);
+        let sig = merged.with_policy(p, None)?.keys().subkeys()
+            .for_signing().nth(0).unwrap().binding_signature();
+        assert_eq!(sig.embedded_signatures().count(), 2);
+
+        // Now the other way around.
+        let merged = malicious_cert.clone().merge(cert.clone())?;
+        // The subkey binding signature should still be fine.
+        assert_eq!(merged.with_policy(p, None)?.keys().subkeys()
+                   .for_signing().count(), 1);
+        let sig = merged.with_policy(p, None)?.keys().subkeys()
+            .for_signing().nth(0).unwrap().binding_signature();
+        assert_eq!(sig.embedded_signatures().count(), 2);
+        Ok(())
+    }
 }
diff --git a/openpgp/src/packet/signature.rs b/openpgp/src/packet/signature.rs
index de13bd96..b5760d03 100644
--- a/openpgp/src/packet/signature.rs
+++ b/openpgp/src/packet/signature.rs
@@ -2538,20 +2538,29 @@ impl Signature {
         // The signature is good, but we may still need to verify the
         // back sig.
         if self.key_flags().map(|kf| kf.for_signing()).unwrap_or(false) {
-            let result = if let Some(backsig) = self.embedded_signature_mut() {
-                backsig.verify_primary_key_binding(pk, subkey)
-            } else {
-                Err(Error::BadSignature(
-                    "Primary key binding signature missing".into()).into())
-            };
-            if result.is_ok() {
-                // Mark the subpacket as authenticated by the embedded
-                // signature.
-                self.subpacket_mut(subpacket::SubpacketTag::EmbeddedSignature)
-                    .expect("must exist, we verified the signature above")
-                    .set_authenticated(true);
+            let mut last_result = Err(Error::BadSignature(
+                "Primary key binding signature missing".into()).into());
+
+            for backsig in self.subpackets_mut(SubpacketTag::EmbeddedSignature)
+            {
+                let result =
+                    if let SubpacketValue::EmbeddedSignature(sig) =
+                        backsig.value_mut()
+                {
+                    sig.verify_primary_key_binding(pk, subkey)
+                } else {
+                    unreachable!("subpackets_mut(EmbeddedSignature) returns \
+                                  EmbeddedSignatures");
+                };
+                if result.is_ok() {
+                    // Mark the subpacket as authenticated by the
+                    // embedded signature.
+                    backsig.set_authenticated(true);
+                    return result;
+                }
+                last_result = result;
             }
-            result
+            last_result
         } else {
             // No backsig required.
             Ok(())
@@ -3406,7 +3415,7 @@ mod test {
             }
         }));
         // Check the subpackets in the embedded signature.
-        let sig = sig.embedded_signature().unwrap();
+        let sig = sig.embedded_signatures().nth(0).unwrap();
         assert!(sig.hashed_area().iter().all(|p| p.authenticated()));
         assert!(sig.unhashed_area().iter().all(|p| p.authenticated()));
 
diff --git a/openpgp/src/packet/signature/subpacket.rs b/openpgp/src/packet/signature/subpacket.rs
index 5fff4d4d..58593709 100644
--- a/openpgp/src/packet/signature/subpacket.rs
+++ b/openpgp/src/packet/signature/subpacket.rs
@@ -827,6 +827,12 @@ impl SubpacketArea {
         self.iter().filter(move |sp| sp.tag() == target)
     }
 
+    pub(crate) fn subpackets_mut(&mut self, target: SubpacketTag)
+        -> impl Iterator<Item = &mut Subpacket>
+    {
+        self.iter_mut().filter(move |sp| sp.tag() == target)
+    }
+
     /// Adds the given subpacket.
     ///
     /// Adds the given subpacket to the subpacket area.  If the
@@ -1806,6 +1812,11 @@ impl Subpacket {
         &self.value
     }
 
+    /// Returns the Subpacket's value.
+    pub(crate) fn value_mut(&mut self) -> &mut SubpacketValue {
+        &mut self.value
+    }
+
     /// Returns whether the information in this subpacket has been
     /// authenticated.
     ///
@@ -2121,6 +2132,19 @@ impl SubpacketAreas {
             }))
     }
 
+    pub(crate) fn subpackets_mut(&mut self, tag: SubpacketTag)
+        -> impl Iterator<Item = &mut Subpacket>
+    {
+        self.hashed_area.subpackets_mut(tag).chain(
+            self.unhashed_area
+                .iter_mut()
+                .filter(move |sp| {
+                    (tag == SubpacketTag::Issuer
+                     || tag == SubpacketTag::IssuerFingerprint
+                     || tag == SubpacketTag::EmbeddedSignature)
+                        && sp.tag() == tag
+            }))
+    }
 
     /// Returns the value of the Signature Creation Time subpacket.
     ///
@@ -3481,18 +3505,16 @@ impl SubpacketAreas {
     /// subpacket in the hashed subpacket area, the last one is
     /// returned.  Otherwise, the last one is returned from the
     /// unhashed subpacket area.
-    pub fn embedded_signature(&self) -> Option<&Signature> {
-        // 1 signature packet body
-        if let Some(sb)
-                = self.subpacket(SubpacketTag::EmbeddedSignature) {
+    pub fn embedded_signatures(&self) -> impl Iterator<Item = &Signature> {
+        self.subpackets(SubpacketTag::EmbeddedSignature).map(|sb| {
             if let SubpacketValue::EmbeddedSignature(v) = &sb.value {
-                Some(v)
+                v
             } else {
-                None
+                unreachable!(
+                    "subpackets(EmbeddedSignature) returns EmbeddedSignatures"
+                );
             }
-        } else {
-            None
-        }
+        })
     }
 
     /// Returns a mutable reference to the Embedded Signature
@@ -3515,18 +3537,17 @@ impl SubpacketAreas {
     /// subpacket in the hashed subpacket area, the last one is
     /// returned.  Otherwise, the last one is returned from the
     /// unhashed subpacket area.
-    pub fn embedded_signature_mut(&mut self) -> Option<&mut Signature> {
-        // 1 signature packet body
-        if let Some(sb)
-                = self.subpacket_mut(SubpacketTag::EmbeddedSignature) {
+    pub fn embedded_signatures_mut(&mut self)
+                                   -> impl Iterator<Item = &mut Signature> {
+        self.subpackets_mut(SubpacketTag::EmbeddedSignature).map(|sb| {
             if let SubpacketValue::EmbeddedSignature(v) = &mut sb.value {
-                Some(v)
+                v
             } else {
-                None
+                unreachable!(
+                    "subpackets_mut(EmbeddedSignature) returns EmbeddedSignatures"
+                );
             }
-        } else {
-            None
-        }
+        })
     }
 
     /// Returns the intended recipients.
@@ -7010,7 +7031,7 @@ fn accessors() {
     sig = sig.set_embedded_signature(embedded_sig.clone()).unwrap();
     let sig_ =
         sig.clone().sign_hash(&mut keypair, hash.clone()).unwrap();
-    assert_eq!(sig_.embedded_signature(), Some(&embedded_sig));
+    assert_eq!(sig_.embedded_signatures().nth(0), Some(&embedded_sig));
 
     sig = sig.set_issuer_fingerprint(fp.clone()).unwrap();
     let sig_ =
@@ -7663,7 +7684,7 @@ fn subpacket_test_2() {
                        authenticated: false,
                    }));
 
-        assert!(sig.embedded_signature().is_some());
+        assert_eq!(sig.embedded_signatures().count(), 1);
         assert!(sig.subpacket(SubpacketTag::EmbeddedSignature)
                 .is_some());
     }