fix: Check that peer SecureJoin messages (except vc/vg-request) gossip our addr+pubkey

This fixes the following identity-misbinding attack:

It appears that Bob’s messages in the SecureJoin protocol do not properly “bind” to Alice’s public
key or fingerprint. Even though Bob’s messages carry Alice’s public key and address as a gossip in
the protected payload, Alice does not reject the message if the gossiped key is different from her
own key. As a result, Mallory could perform an identity-misbinding attack. If Mallory obtained
Alice’s QR invite code, she could change her own QR code to contain the same tokens as in Alice’s QR
code, and convince Bob to scan the modified QR code, possibly as an insider attacker. Mallory would
forward messages from Bob to Alice and craft appropriate responses for Bob on his own. In the end,
Bob would believe he is talking to Mallory, but Alice would believe she is talking to Bob.

Author: iequidoo

src/securejoin.rs

@@ -295,6 +295,23 @@ pub(crate) async fn handle_securejoin_handshake(
 
     let join_vg = step.starts_with("vg-");
 
+    if !matches!(step.as_str(), "vg-request" | "vc-request") {
+        let mut self_found = false;
+        let self_fingerprint = load_self_public_key(context).await?.fingerprint();
+        for (addr, key) in &mime_message.gossiped_keys {
+            if key.fingerprint() == self_fingerprint && context.is_self_addr(addr).await? {
+                self_found = true;
+                break;
+            }
+        }
+        if !self_found {
+            // This message isn't intended for us. Possibly the peer doesn't own the key which the
+            // message is signed with but forwarded someone's message to us.
+            warn!(context, "Step {step}: No self addr+pubkey gossip found.");
+            return Ok(HandshakeMessage::Ignore);
+        }
+    }
+
     match step.as_str() {
         "vg-request" | "vc-request" => {
             /*=======================================================
@@ -753,19 +770,32 @@ mod tests {
     use crate::tools::{EmailAddress, SystemTime};
     use std::time::Duration;
 
+    #[derive(PartialEq)]
+    enum SetupContactCase {
+        Normal,
+        CheckProtectionTimestamp,
+        WrongAliceGossip,
+    }
+
     #[tokio::test(flavor = "multi_thread", worker_threads = 2)]
     async fn test_setup_contact() {
-        test_setup_contact_ex(false).await
+        test_setup_contact_ex(SetupContactCase::Normal).await
     }
 
     #[tokio::test(flavor = "multi_thread", worker_threads = 2)]
     async fn test_setup_contact_protection_timestamp() {
-        test_setup_contact_ex(true).await
+        test_setup_contact_ex(SetupContactCase::CheckProtectionTimestamp).await
     }
 
-    async fn test_setup_contact_ex(check_protection_timestamp: bool) {
+    #[tokio::test(flavor = "multi_thread", worker_threads = 2)]
+    async fn test_setup_contact_wrong_alice_gossip() {
+        test_setup_contact_ex(SetupContactCase::WrongAliceGossip).await
+    }
+
+    async fn test_setup_contact_ex(case: SetupContactCase) {
         let mut tcm = TestContextManager::new();
         let alice = tcm.alice().await;
+        let alice_addr = &alice.get_config(Config::Addr).await.unwrap().unwrap();
         let bob = tcm.bob().await;
         alice
             .set_config(Config::VerifiedOneOnOneChats, Some("1"))
@@ -798,10 +828,7 @@ mod tests {
         );
 
         let sent = bob.pop_sent_msg().await;
-        assert_eq!(
-            sent.recipient(),
-            EmailAddress::new("alice@example.org").unwrap()
-        );
+        assert_eq!(sent.recipient(), EmailAddress::new(alice_addr).unwrap());
         let msg = alice.parse_msg(&sent).await;
         assert!(!msg.was_encrypted());
         assert_eq!(msg.get_header(HeaderDef::SecureJoin).unwrap(), "vc-request");
@@ -839,7 +866,7 @@ mod tests {
                 progress,
             } => {
                 let alice_contact_id =
-                    Contact::lookup_id_by_addr(&bob.ctx, "alice@example.org", Origin::Unknown)
+                    Contact::lookup_id_by_addr(&bob.ctx, alice_addr, Origin::Unknown)
                         .await
                         .expect("Error looking up contact")
                         .expect("Contact not found");
@@ -851,7 +878,7 @@ mod tests {
 
         // Check Bob sent the right message.
         let sent = bob.pop_sent_msg().await;
-        let msg = alice.parse_msg(&sent).await;
+        let mut msg = alice.parse_msg(&sent).await;
         let vc_request_with_auth_ts_sent = msg
             .get_header(HeaderDef::Date)
             .and_then(|value| mailparse::dateparse(value).ok())
@@ -868,6 +895,30 @@ mod tests {
             bob_fp.hex()
         );
 
+        if case == SetupContactCase::WrongAliceGossip {
+            let wrong_pubkey = load_self_public_key(&bob).await.unwrap();
+            let alice_pubkey = msg
+                .gossiped_keys
+                .insert(alice_addr.to_string(), wrong_pubkey)
+                .unwrap();
+            let contact_bob = alice.add_or_lookup_contact(&bob).await;
+            let handshake_msg = handle_securejoin_handshake(&alice, &msg, contact_bob.id)
+                .await
+                .unwrap();
+            assert_eq!(handshake_msg, HandshakeMessage::Ignore);
+            assert_eq!(contact_bob.is_verified(&alice.ctx).await.unwrap(), false);
+
+            msg.gossiped_keys
+                .insert(alice_addr.to_string(), alice_pubkey)
+                .unwrap();
+            let handshake_msg = handle_securejoin_handshake(&alice, &msg, contact_bob.id)
+                .await
+                .unwrap();
+            assert_eq!(handshake_msg, HandshakeMessage::Ignore);
+            assert!(contact_bob.is_verified(&alice.ctx).await.unwrap());
+            return;
+        }
+
         // Alice should not yet have Bob verified
         let contact_bob_id =
             Contact::lookup_id_by_addr(&alice.ctx, "bob@example.net", Origin::Unknown)
@@ -879,7 +930,7 @@ mod tests {
             .unwrap();
         assert_eq!(contact_bob.is_verified(&alice.ctx).await.unwrap(), false);
 
-        if check_protection_timestamp {
+        if case == SetupContactCase::CheckProtectionTimestamp {
             SystemTime::shift(Duration::from_secs(3600));
         }
 
@@ -908,7 +959,7 @@ mod tests {
             assert!(msg.is_info());
             let expected_text = chat_protection_enabled(&alice).await;
             assert_eq!(msg.get_text(), expected_text);
-            if check_protection_timestamp {
+            if case == SetupContactCase::CheckProtectionTimestamp {
                 assert_eq!(msg.timestamp_sort, vc_request_with_auth_ts_sent);
             }
         }
@@ -923,11 +974,10 @@ mod tests {
         );
 
         // Bob should not yet have Alice verified
-        let contact_alice_id =
-            Contact::lookup_id_by_addr(&bob.ctx, "alice@example.org", Origin::Unknown)
-                .await
-                .expect("Error looking up contact")
-                .expect("Contact not found");
+        let contact_alice_id = Contact::lookup_id_by_addr(&bob.ctx, alice_addr, Origin::Unknown)
+            .await
+            .expect("Error looking up contact")
+            .expect("Contact not found");
         let contact_alice = Contact::get_by_id(&bob.ctx, contact_alice_id)
             .await
             .unwrap();