Fix the merge operation.

- Previously, the merge operation unconditionally unlinked emails
    from UIDs that are revoked.  This means that anyone could unlink
    any email address by just generating a TPK with that address,
    revoking that UID, then uploading the TPK.

  - Add a test that demonstrates the problem.

  - Fix two tests that assumed the merge operation would eagerly
    unlink addresses before the verification token was validated.

  - Fixes #84.

database/src/fs.rs

@@ -813,6 +813,13 @@ mod tests {
         test::test_steal_uid(&mut db);
     }
 
+    #[test]
+    fn uid_unlinking() {
+        let tmpdir = TempDir::new().unwrap();
+        let mut db = Filesystem::new(tmpdir.path()).unwrap();
+        test::test_unlink_uid(&mut db);
+    }
+
     #[test]
     fn same_email_1() {
         let tmpdir = TempDir::new().unwrap();

database/src/lib.rs

@@ -328,7 +328,7 @@ pub trait Database: Sync + Send {
     }
 
     fn merge_or_publish(&self, tpk: &TPK) -> Result<Vec<(Email, String)>> {
-        use std::collections::HashMap;
+        use std::collections::{HashMap, HashSet};
         use openpgp::RevocationStatus;
 
         if let RevocationStatus::Revoked(_) = tpk.revoked(None) {
@@ -338,9 +338,9 @@ pub trait Database: Sync + Send {
         }
 
         let fpr = Fingerprint::try_from(tpk.primary().fingerprint())?;
-        let mut all_uids = Vec::default();
+        let mut revoked_uids = HashSet::new();
         let mut unverified_uids: HashMap<Email, Verify> = HashMap::new();
-        let mut verified_uids = Vec::default();
+        let mut verified_uids = HashSet::new();
 
         let _ = self.lock();
 
@@ -354,32 +354,25 @@ pub trait Database: Sync + Send {
             };
 
             match uid.revoked(None) {
-                // XXX: This looks wrong.  It means that anyone can
-                // unlink email addresses by uploading TPK with a
-                // revoked userid matching the target address.
-                RevocationStatus::CouldBe(_) | RevocationStatus::Revoked(_) => {
-                    match self.lookup(&Query::ByEmail(email.clone())) {
-                        Ok(None) => (),
-                        Ok(Some(other_tpk)) =>
-                            all_uids.push((email, other_tpk.fingerprint())),
-                        Err(_) => (),
-                    };
-                }
+                RevocationStatus::CouldBe(_) => {
+                    // XXX: Check the revocation, if it checks out,
+                    // "fall through".
+                },
+                RevocationStatus::Revoked(_) => {
+                    revoked_uids.insert(email);
+                },
                 RevocationStatus::NotAsFarAsWeKnow => {
                     let add_to_verified =
                         match self.lookup(&Query::ByEmail(email.clone()))
                     {
                         Ok(None) => false,
-                        Ok(Some(other_tpk)) => {
-                            all_uids.push((email.clone(),
-                                           other_tpk.fingerprint()));
-                            other_tpk.fingerprint() == tpk.fingerprint()
-                        },
+                        Ok(Some(other_tpk)) =>
+                            other_tpk.fingerprint() == tpk.fingerprint(),
                         Err(_) => false,
                     };
 
                     if add_to_verified {
-                        verified_uids.push(email.clone());
+                        verified_uids.insert(email);
                     } else {
                         // Hackaround mutable borrow in else.
                         let updated =
@@ -405,8 +398,8 @@ pub trait Database: Sync + Send {
 
         let tpk = filter_userids(&tpk, |_| false)?;
 
-        for (email, fpr) in all_uids {
-            self.unlink_email(&email, &Fingerprint::try_from(fpr).unwrap())?;
+        for email in revoked_uids.difference(&verified_uids) {
+            self.unlink_email(&email, &fpr)?;
         }
 
         // merge or update key db
@@ -417,9 +410,6 @@ pub trait Database: Sync + Send {
         self.update_tpk(&fpr, Some(tpk))?;
 
         self.link_subkeys(&fpr, subkeys)?;
-        for email in verified_uids {
-            self.link_email(&email, &fpr)?;
-        }
 
         let mut tokens = Vec::new();
         for (fp, verify) in unverified_uids.into_iter() {

database/src/test.rs

@@ -283,7 +283,8 @@ pub fn test_uid_replacement<D: Database>(db: &mut D) {
     // replace
     let tokens = db.merge_or_publish(&tpk2).unwrap();
 
-    assert!(db.by_email(&email).is_none());
+    // Before tokens are verified, the first binding is still valid.
+    assert!(db.by_email(&email).is_some());
     assert!(db.verify_token(&tokens[0].1).unwrap().is_some());
     assert_eq!(
         TPK::from_bytes(&db.by_email(&email).unwrap().as_bytes()).unwrap().fingerprint(),
@@ -555,7 +556,8 @@ pub fn test_steal_uid<D: Database>(db: &mut D) {
     let tokens = db.merge_or_publish(&tpk2).unwrap();
 
     assert_eq!(tokens.len(), 1);
-    assert!(db.by_email(&email1).is_none());
+    // Before tokens are verified, the first binding is still valid.
+    assert!(db.by_email(&email1).is_some());
     assert!(db.verify_token(&tokens[0].1).unwrap().is_some());
 
     assert_eq!(
@@ -564,6 +566,43 @@ pub fn test_steal_uid<D: Database>(db: &mut D) {
     );
 }
 
+pub fn test_unlink_uid<D: Database>(db: &mut D) {
+    let uid = "Test A <test_a@example.com>";
+    let email = Email::from_str(uid).unwrap();
+
+    // Upload key and verify it.
+    let tpk = TPKBuilder::default().add_userid(uid).generate().unwrap().0;
+    let tokens = db.merge_or_publish(&tpk).unwrap();
+    assert!(db.verify_token(&tokens[0].1).unwrap().is_some());
+    assert!(db.by_email(&email).is_some());
+
+    // Create a 2nd key with same uid, and revoke the uid.
+    let tpk_evil = TPKBuilder::default().add_userid(uid).generate().unwrap().0;
+    let sig = {
+        let uid = tpk_evil.userids()
+            .find(|b| b.userid().userid() == uid.as_bytes()).unwrap();
+        assert_eq!(RevocationStatus::NotAsFarAsWeKnow, uid.revoked(None));
+
+        let mut keypair = tpk_evil.primary().clone().into_keypair().unwrap();
+        uid.revoke(
+            &mut keypair,
+            ReasonForRevocation::UIDRetired,
+            b"I just had to quit, I couldn't bear it any longer",
+        )
+        .unwrap()
+    };
+    assert_eq!(sig.sigtype(), SignatureType::CertificateRevocation);
+    let tpk_evil = tpk_evil.merge_packets(vec![sig.into()]).unwrap();
+    let tokens = db.merge_or_publish(&tpk_evil).unwrap();
+    assert_eq!(tokens.len(), 0);
+
+    // Check that when looking up by email, we still get the former
+    // TPK.
+    assert_eq!(
+        db.lookup(&Query::ByEmail(email)).unwrap().unwrap().fingerprint(),
+        tpk.fingerprint());
+}
+
 pub fn get_userids(armored: &str) -> Vec<UserID> {
     let tpk = TPK::from_bytes(armored.as_bytes()).unwrap();
     tpk.userids().map(|binding| binding.userid().clone()).collect()