diff --git a/deltachat-rpc-client/tests/test_securejoin.py b/deltachat-rpc-client/tests/test_securejoin.py index aed5021359..d6a7161624 100644 --- a/deltachat-rpc-client/tests/test_securejoin.py +++ b/deltachat-rpc-client/tests/test_securejoin.py @@ -4,7 +4,7 @@ from deltachat_rpc_client import Chat, SpecialContactId -def test_qr_setup_contact(acfactory) -> None: +def test_qr_setup_contact(acfactory, tmp_path) -> None: alice, bob = acfactory.get_online_accounts(2) qr_code, _svg = alice.get_qr_code() @@ -24,6 +24,18 @@ def test_qr_setup_contact(acfactory) -> None: bob_contact_alice_snapshot = bob_contact_alice.get_snapshot() assert bob_contact_alice_snapshot.is_verified + # Test that if Bob changes the key, backwards verification is lost. + logging.info("Bob 2 is created") + bob2 = acfactory.new_configured_account() + bob2.export_self_keys(tmp_path) + + logging.info("Bob imports a key") + bob.import_self_keys(tmp_path / "private-key-default.asc") + + assert bob.get_config("key_id") == "2" + bob_contact_alice_snapshot = bob_contact_alice.get_snapshot() + assert not bob_contact_alice_snapshot.is_verified + @pytest.mark.parametrize("protect", [True, False]) def test_qr_securejoin(acfactory, protect): diff --git a/src/contact.rs b/src/contact.rs index f4a2702e2d..cc4a55bad9 100644 --- a/src/contact.rs +++ b/src/contact.rs @@ -1268,13 +1268,30 @@ impl Contact { return Ok(true); } - if let Some(peerstate) = Peerstate::from_addr(context, &self.addr).await? { - if peerstate.is_using_verified_key() { - return Ok(true); - } + let Some(peerstate) = Peerstate::from_addr(context, &self.addr).await? else { + return Ok(false); + }; + + let forward_verified = peerstate.is_using_verified_key(); + let backward_verified = peerstate.is_backward_verified(context).await?; + Ok(forward_verified && backward_verified) + } + + /// Returns true if we have a verified key for the contact + /// and it is the same as Autocrypt key. + /// This is enough to send messages to the contact in verified chat + /// and verify received messages, but not enough to display green checkmark + /// or add the contact to verified groups. + pub async fn is_forward_verified(&self, context: &Context) -> Result { + if self.id == ContactId::SELF { + return Ok(true); } - Ok(false) + let Some(peerstate) = Peerstate::from_addr(context, &self.addr).await? else { + return Ok(false); + }; + + Ok(peerstate.is_using_verified_key()) } /// Returns the `ContactId` that verified the contact. diff --git a/src/e2ee.rs b/src/e2ee.rs index b4dee9b7b4..e0affc5d0b 100644 --- a/src/e2ee.rs +++ b/src/e2ee.rs @@ -312,6 +312,7 @@ Sent with my Delta Chat Messenger: https://delta.chat"; secondary_verified_key: None, secondary_verified_key_fingerprint: None, secondary_verifier: None, + backward_verified_key_id: None, fingerprint_changed: false, }; vec![(Some(peerstate), addr)] diff --git a/src/mimefactory.rs b/src/mimefactory.rs index 5ed9e57266..2d9821e553 100644 --- a/src/mimefactory.rs +++ b/src/mimefactory.rs @@ -1003,17 +1003,6 @@ impl<'a> MimeFactory<'a> { "Secure-Join".to_string(), "vg-member-added".to_string(), )); - // FIXME: Old clients require Secure-Join-Fingerprint header. Remove this - // eventually. - let fingerprint = Peerstate::from_addr(context, email_to_add) - .await? - .context("No peerstate found in db")? - .public_key_fingerprint - .context("No public key fingerprint in db for the member to add")?; - headers.protected.push(Header::new( - "Secure-Join-Fingerprint".into(), - fingerprint.hex(), - )); } } SystemMessage::GroupNameChanged => { diff --git a/src/param.rs b/src/param.rs index a9f5c1a425..6465c4443b 100644 --- a/src/param.rs +++ b/src/param.rs @@ -84,7 +84,7 @@ pub enum Param { /// For Messages Arg2 = b'F', - /// For Messages + /// `Secure-Join-Fingerprint` header for `{vc,vg}-request-with-auth` messages. Arg3 = b'G', /// For Messages diff --git a/src/peerstate.rs b/src/peerstate.rs index 81ed04808a..916b6852a1 100644 --- a/src/peerstate.rs +++ b/src/peerstate.rs @@ -6,6 +6,7 @@ use num_traits::FromPrimitive; use crate::aheader::{Aheader, EncryptPreference}; use crate::chat::{self, Chat}; use crate::chatlist::Chatlist; +use crate::config::Config; use crate::constants::Chattype; use crate::contact::{addr_cmp, Contact, ContactAddress, Origin}; use crate::context::Context; @@ -83,6 +84,10 @@ pub struct Peerstate { /// The address that introduced secondary verified key. pub secondary_verifier: Option, + /// Row ID of the key in the `keypairs` table + /// that we think the peer knows as verified. + pub backward_verified_key_id: Option, + /// True if it was detected /// that the fingerprint of the key used in chats with /// opportunistic encryption was changed after Peerstate creation. @@ -108,6 +113,7 @@ impl Peerstate { secondary_verified_key: None, secondary_verified_key_fingerprint: None, secondary_verifier: None, + backward_verified_key_id: None, fingerprint_changed: false, } } @@ -137,6 +143,7 @@ impl Peerstate { secondary_verified_key: None, secondary_verified_key_fingerprint: None, secondary_verifier: None, + backward_verified_key_id: None, fingerprint_changed: false, } } @@ -148,7 +155,8 @@ impl Peerstate { verified_key, verified_key_fingerprint, \ verifier, \ secondary_verified_key, secondary_verified_key_fingerprint, \ - secondary_verifier \ + secondary_verifier, \ + backward_verified_key_id \ FROM acpeerstates \ WHERE addr=? COLLATE NOCASE LIMIT 1;"; Self::from_stmt(context, query, (addr,)).await @@ -164,7 +172,8 @@ impl Peerstate { verified_key, verified_key_fingerprint, \ verifier, \ secondary_verified_key, secondary_verified_key_fingerprint, \ - secondary_verifier \ + secondary_verifier, \ + backward_verified_key_id \ FROM acpeerstates \ WHERE public_key_fingerprint=? \ OR gossip_key_fingerprint=? \ @@ -187,7 +196,8 @@ impl Peerstate { verified_key, verified_key_fingerprint, \ verifier, \ secondary_verified_key, secondary_verified_key_fingerprint, \ - secondary_verifier \ + secondary_verifier, \ + backward_verified_key_id \ FROM acpeerstates \ WHERE verified_key_fingerprint=? \ OR addr=? COLLATE NOCASE \ @@ -255,6 +265,7 @@ impl Peerstate { let secondary_verifier: Option = row.get("secondary_verifier")?; secondary_verifier.filter(|s| !s.is_empty()) }, + backward_verified_key_id: row.get("backward_verified_key_id")?, fingerprint_changed: false, }; @@ -435,6 +446,17 @@ impl Peerstate { verified.is_some() && verified == self.peek_key_fingerprint(false) } + pub(crate) async fn is_backward_verified(&self, context: &Context) -> Result { + let Some(backward_verified_key_id) = self.backward_verified_key_id else { + return Ok(false); + }; + + let self_key_id = context.get_config_i64(Config::KeyId).await?; + + let backward_verified = backward_verified_key_id == self_key_id; + Ok(backward_verified) + } + /// Set this peerstate to verified /// Make sure to call `self.save_to_db` to save these changes /// Params: @@ -510,8 +532,9 @@ impl Peerstate { secondary_verified_key, secondary_verified_key_fingerprint, secondary_verifier, + backward_verified_key_id, addr) - VALUES (?,?,?,?,?,?,?,?,?,?,?,?,?,?,?) + VALUES (?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?) ON CONFLICT (addr) DO UPDATE SET last_seen = excluded.last_seen, @@ -527,7 +550,8 @@ impl Peerstate { verifier = excluded.verifier, secondary_verified_key = excluded.secondary_verified_key, secondary_verified_key_fingerprint = excluded.secondary_verified_key_fingerprint, - secondary_verifier = excluded.secondary_verifier", + secondary_verifier = excluded.secondary_verifier, + backward_verified_key_id = excluded.backward_verified_key_id", ( self.last_seen, self.last_seen_autocrypt, @@ -545,6 +569,7 @@ impl Peerstate { .as_ref() .map(|fp| fp.hex()), self.secondary_verifier.as_deref().unwrap_or(""), + self.backward_verified_key_id, &self.addr, ), ) @@ -806,6 +831,7 @@ mod tests { secondary_verified_key: None, secondary_verified_key_fingerprint: None, secondary_verifier: None, + backward_verified_key_id: None, fingerprint_changed: false, }; @@ -849,6 +875,7 @@ mod tests { secondary_verified_key: None, secondary_verified_key_fingerprint: None, secondary_verifier: None, + backward_verified_key_id: None, fingerprint_changed: false, }; @@ -885,6 +912,7 @@ mod tests { secondary_verified_key: None, secondary_verified_key_fingerprint: None, secondary_verifier: None, + backward_verified_key_id: None, fingerprint_changed: false, }; @@ -951,6 +979,7 @@ mod tests { secondary_verified_key: None, secondary_verified_key_fingerprint: None, secondary_verifier: None, + backward_verified_key_id: None, fingerprint_changed: false, }; diff --git a/src/qr.rs b/src/qr.rs index 02b339600e..0d0e89c826 100644 --- a/src/qr.rs +++ b/src/qr.rs @@ -1057,6 +1057,7 @@ mod tests { secondary_verified_key: None, secondary_verified_key_fingerprint: None, secondary_verifier: None, + backward_verified_key_id: None, fingerprint_changed: false, }; assert!( diff --git a/src/receive_imf.rs b/src/receive_imf.rs index 30e0ba5dd8..07b1ac4837 100644 --- a/src/receive_imf.rs +++ b/src/receive_imf.rs @@ -346,6 +346,24 @@ pub(crate) async fn receive_imf_inner( let verified_encryption = has_verified_encryption(context, &mime_parser, from_id, &to_ids).await?; + if verified_encryption == VerifiedEncryption::Verified + && mime_parser.get_header(HeaderDef::ChatVerified).is_some() + { + if let Some(peerstate) = &mut mime_parser.decryption_info.peerstate { + // NOTE: it might be better to remember ID of the key + // that we used to decrypt the message, but + // it is unlikely that default key ever changes + // as it only happens when user imports a new default key. + // + // Backward verification is not security-critical, + // it is only needed to avoid adding user who does not + // have our key as verified to protected chats. + peerstate.backward_verified_key_id = + Some(context.get_config_i64(Config::KeyId).await?).filter(|&id| id > 0); + peerstate.save_to_db(&context.sql).await?; + } + } + let received_msg = if let Some(received_msg) = received_msg { received_msg } else { @@ -2527,6 +2545,8 @@ async fn mark_recipients_as_verified( info!(context, "{verifier_addr} has verified {to_addr}."); if let Some(fp) = peerstate.gossip_key_fingerprint.clone() { peerstate.set_verified(PeerstateKeyType::GossipKey, fp, verifier_addr)?; + peerstate.backward_verified_key_id = + Some(context.get_config_i64(Config::KeyId).await?).filter(|&id| id > 0); peerstate.save_to_db(&context.sql).await?; let (to_contact_id, _) = Contact::add_or_lookup( diff --git a/src/securejoin.rs b/src/securejoin.rs index 62ad1c4635..26260a63d0 100644 --- a/src/securejoin.rs +++ b/src/securejoin.rs @@ -20,6 +20,7 @@ use crate::mimeparser::{MimeMessage, SystemMessage}; use crate::param::Param; use crate::peerstate::{Peerstate, PeerstateKeyType}; use crate::qr::check_qr; +use crate::securejoin::bob::JoinerProgress; use crate::stock_str; use crate::sync::Sync::*; use crate::token; @@ -173,7 +174,6 @@ async fn send_alice_handshake_msg( context: &Context, contact_id: ContactId, step: &str, - fingerprint: Option, ) -> Result<()> { let mut msg = Message { viewtype: Viewtype::Text, @@ -183,9 +183,6 @@ async fn send_alice_handshake_msg( }; msg.param.set_cmd(SystemMessage::SecurejoinMessage); msg.param.set(Param::Arg, step); - if let Some(fp) = fingerprint { - msg.param.set(Param::Arg3, fp.hex()); - } msg.param.set_int(Param::GuaranteeE2ee, 1); chat::send_msg( context, @@ -204,7 +201,9 @@ async fn info_chat_id(context: &Context, contact_id: ContactId) -> Result match BobState::from_db(&context.sql).await? { - Some(bobstate) => bob::handle_contact_confirm(context, bobstate, mime_message).await, - None => Ok(HandshakeMessage::Ignore), - }, + "vc-contact-confirm" => { + if let Some(mut bobstate) = BobState::from_db(&context.sql).await? { + if !bobstate.is_msg_expected(context, step.as_str()) { + warn!(context, "Unexpected vc-contact-confirm."); + return Ok(HandshakeMessage::Ignore); + } + bobstate.step_contact_confirm(context).await?; + bobstate.emit_progress(context, JoinerProgress::Succeeded); + } + Ok(HandshakeMessage::Ignore) + } "vg-member-added" => { let Some(member_added) = mime_message .get_header(HeaderDef::ChatGroupMemberAdded) @@ -496,23 +506,27 @@ pub(crate) async fn handle_securejoin_handshake( else { warn!( context, - "vg-member-added without Chat-Group-Member-Added header" + "vg-member-added without Chat-Group-Member-Added header." ); return Ok(HandshakeMessage::Propagate); }; if !context.is_self_addr(member_added).await? { info!( context, - "Member {member_added} added by unrelated SecureJoin process" + "Member {member_added} added by unrelated SecureJoin process." ); return Ok(HandshakeMessage::Propagate); } - match BobState::from_db(&context.sql).await? { - Some(bobstate) => { - bob::handle_contact_confirm(context, bobstate, mime_message).await + if let Some(mut bobstate) = BobState::from_db(&context.sql).await? { + if !bobstate.is_msg_expected(context, step.as_str()) { + warn!(context, "Unexpected vg-member-added."); + return Ok(HandshakeMessage::Propagate); } - None => Ok(HandshakeMessage::Propagate), + + bobstate.step_contact_confirm(context).await?; + bobstate.emit_progress(context, JoinerProgress::Succeeded); } + Ok(HandshakeMessage::Propagate) } "vg-member-added-received" | "vc-contact-confirm-received" => { @@ -526,23 +540,25 @@ pub(crate) async fn handle_securejoin_handshake( } } -/// observe_securejoin_on_other_device() must be called when a self-sent securejoin message is seen. +/// Observe self-sent Securejoin message. /// -/// in a multi-device-setup, there may be other devices that "see" the handshake messages. -/// if the seen messages seen are self-sent messages encrypted+signed correctly with our key, -/// we can make some conclusions of it: +/// In a multi-device-setup, there may be other devices that "see" the handshake messages. +/// If we see self-sent messages encrypted+signed correctly with our key, +/// we can make some conclusions of it. /// -/// - if we see the self-sent-message vg-member-added/vc-contact-confirm, -/// we know that we're an inviter-observer. -/// The inviting device has marked a peer as verified on vg-request-with-auth/vc-request-with-auth -/// before sending vg-member-added/vc-contact-confirm - so, if we observe vg-member-added/vc-contact-confirm, -/// we can mark the peer as verified as well. +/// If we see self-sent {vc,vg}-request-with-auth, +/// we know that we are Bob (joiner-observer) +/// that just marked peer (Alice) as forward-verified +/// either after receiving {vc,vg}-auth-required +/// or immediately after scanning the QR-code +/// if the key was already known. /// -/// - if we see the self-sent-message vg-request-with-auth/vc-request-with-auth -/// we know that we're an joiner-observer. -/// the joining device has marked the peer as verified -/// before sending vg-request-with-auth/vc-request-with-auth - so, if we observe vg-member-added-received, -/// we can mark the peer as verified as well. +/// If we see self-sent vc-contact-confirm or vg-member-added message, +/// we know that we are Alice (inviter-observer) +/// that just marked peer (Bob) as forward (and backward)-verified +/// in response to correct vc-request-with-auth message. +/// +/// In both cases we can mark the peer as forward-verified. pub(crate) async fn observe_securejoin_on_other_device( context: &Context, mime_message: &MimeMessage, @@ -556,127 +572,96 @@ pub(crate) async fn observe_securejoin_on_other_device( .context("Not a Secure-Join message")?; info!(context, "Observing secure-join message {step:?}."); - match step.as_str() { - "vg-request-with-auth" - | "vc-request-with-auth" - | "vg-member-added" - | "vc-contact-confirm" => { - if !encrypted_and_signed( - context, - mime_message, - get_self_fingerprint(context).await.as_ref(), - ) { - could_not_establish_secure_connection( - context, - contact_id, - info_chat_id(context, contact_id).await?, - "Message not encrypted correctly.", - ) - .await?; - return Ok(HandshakeMessage::Ignore); - } - let addr = Contact::get_by_id(context, contact_id) - .await? - .get_addr() - .to_lowercase(); - if mime_message.gossiped_addr.contains(&addr) { - let mut peerstate = match Peerstate::from_addr(context, &addr).await? { - Some(p) => p, - None => { - could_not_establish_secure_connection( - context, - contact_id, - info_chat_id(context, contact_id).await?, - &format!("No peerstate in db for '{}' at step {}", &addr, step), - ) - .await?; - return Ok(HandshakeMessage::Ignore); - } - }; - let fingerprint = match peerstate.gossip_key_fingerprint.clone() { - Some(fp) => fp, - None => { - could_not_establish_secure_connection( - context, - contact_id, - info_chat_id(context, contact_id).await?, - &format!( - "No gossip key fingerprint in db for '{}' at step {}", - &addr, step, - ), - ) - .await?; - return Ok(HandshakeMessage::Ignore); - } - }; - peerstate.set_verified(PeerstateKeyType::GossipKey, fingerprint, addr)?; - peerstate.prefer_encrypt = EncryptPreference::Mutual; - peerstate.save_to_db(&context.sql).await?; + if !matches!( + step.as_str(), + "vg-request-with-auth" | "vc-request-with-auth" | "vg-member-added" | "vc-contact-confirm" + ) { + return Ok(HandshakeMessage::Ignore); + }; - ChatId::set_protection_for_contact( - context, - contact_id, - mime_message.timestamp_sent, - ) - .await?; - } else if let Some(fingerprint) = - mime_message.get_header(HeaderDef::SecureJoinFingerprint) - { - // FIXME: Old versions of DC send this header instead of gossips. Remove this - // eventually. - let fingerprint = fingerprint.parse()?; - let fingerprint_found = mark_peer_as_verified( - context, - fingerprint, - Contact::get_by_id(context, contact_id) - .await? - .get_addr() - .to_owned(), - ) - .await?; - if !fingerprint_found { - could_not_establish_secure_connection( - context, - contact_id, - info_chat_id(context, contact_id).await?, - format!("Fingerprint mismatch on observing {step}.").as_ref(), - ) - .await?; - return Ok(HandshakeMessage::Ignore); - } - } else { - could_not_establish_secure_connection( - context, - contact_id, - info_chat_id(context, contact_id).await?, - &format!( - "No gossip header for '{}' at step {}, please update Delta Chat on all \ + if !encrypted_and_signed( + context, + mime_message, + get_self_fingerprint(context).await.as_ref(), + ) { + could_not_establish_secure_connection( + context, + contact_id, + info_chat_id(context, contact_id).await?, + "Message not encrypted correctly.", + ) + .await?; + return Ok(HandshakeMessage::Ignore); + } + + let addr = Contact::get_by_id(context, contact_id) + .await? + .get_addr() + .to_lowercase(); + + if !mime_message.gossiped_addr.contains(&addr) { + could_not_establish_secure_connection( + context, + contact_id, + info_chat_id(context, contact_id).await?, + &format!( + "No gossip header for '{}' at step {}, please update Delta Chat on all \ your devices.", - &addr, step, - ), - ) - .await?; - return Ok(HandshakeMessage::Ignore); - } - if step.as_str() == "vg-member-added" { - inviter_progress(context, contact_id, 800); - } - if step.as_str() == "vg-member-added" || step.as_str() == "vc-contact-confirm" { - inviter_progress(context, contact_id, 1000); - } - if step.as_str() == "vg-request-with-auth" || step.as_str() == "vc-request-with-auth" { - // This actually reflects what happens on the first device (which does the secure - // join) and causes a subsequent "vg-member-added" message to create an unblocked - // verified group. - ChatId::create_for_contact_with_blocked(context, contact_id, Blocked::Not).await?; - } - Ok(if step.as_str() == "vg-member-added" { - HandshakeMessage::Propagate - } else { - HandshakeMessage::Ignore - }) - } - _ => Ok(HandshakeMessage::Ignore), + &addr, step, + ), + ) + .await?; + return Ok(HandshakeMessage::Ignore); + } + + let Some(mut peerstate) = Peerstate::from_addr(context, &addr).await? else { + could_not_establish_secure_connection( + context, + contact_id, + info_chat_id(context, contact_id).await?, + &format!("No peerstate in db for '{}' at step {}", &addr, step), + ) + .await?; + return Ok(HandshakeMessage::Ignore); + }; + + let Some(fingerprint) = peerstate.gossip_key_fingerprint.clone() else { + could_not_establish_secure_connection( + context, + contact_id, + info_chat_id(context, contact_id).await?, + &format!( + "No gossip key fingerprint in db for '{}' at step {}", + &addr, step, + ), + ) + .await?; + return Ok(HandshakeMessage::Ignore); + }; + peerstate.set_verified(PeerstateKeyType::GossipKey, fingerprint, addr)?; + peerstate.prefer_encrypt = EncryptPreference::Mutual; + peerstate.save_to_db(&context.sql).await?; + + ChatId::set_protection_for_contact(context, contact_id, mime_message.timestamp_sent).await?; + + if step.as_str() == "vg-member-added" { + inviter_progress(context, contact_id, 800); + } + if step.as_str() == "vg-member-added" || step.as_str() == "vc-contact-confirm" { + inviter_progress(context, contact_id, 1000); + } + + if step.as_str() == "vg-request-with-auth" || step.as_str() == "vc-request-with-auth" { + // This actually reflects what happens on the first device (which does the secure + // join) and causes a subsequent "vg-member-added" message to create an unblocked + // verified group. + ChatId::create_for_contact_with_blocked(context, contact_id, Blocked::Not).await?; + } + + if step.as_str() == "vg-member-added" { + Ok(HandshakeMessage::Propagate) + } else { + Ok(HandshakeMessage::Ignore) } } @@ -724,12 +709,17 @@ async fn mark_peer_as_verified( context: &Context, fingerprint: Fingerprint, verifier: String, + backward_verified: bool, ) -> Result { let Some(ref mut peerstate) = Peerstate::from_fingerprint(context, &fingerprint).await? else { return Ok(false); }; peerstate.set_verified(PeerstateKeyType::PublicKey, fingerprint, verifier)?; peerstate.prefer_encrypt = EncryptPreference::Mutual; + if backward_verified { + peerstate.backward_verified_key_id = + Some(context.get_config_i64(Config::KeyId).await?).filter(|&id| id > 0); + } peerstate.save_to_db(&context.sql).await?; Ok(true) } @@ -980,6 +970,7 @@ mod tests { secondary_verified_key: None, secondary_verified_key_fingerprint: None, secondary_verifier: None, + backward_verified_key_id: None, fingerprint_changed: false, }; peerstate.save_to_db(&bob.ctx.sql).await?; @@ -1313,4 +1304,70 @@ First thread."#; Ok(()) } + + /// Tests that Bob gets Alice as verified + /// if `vc-contact-confirm` is lost but Alice then sends + /// a message to Bob in a verified 1:1 chat with a `Chat-Verified` header. + #[tokio::test(flavor = "multi_thread", worker_threads = 2)] + async fn test_lost_contact_confirm() { + let mut tcm = TestContextManager::new(); + let alice = tcm.alice().await; + let bob = tcm.bob().await; + alice + .set_config(Config::VerifiedOneOnOneChats, Some("1")) + .await + .unwrap(); + bob.set_config(Config::VerifiedOneOnOneChats, Some("1")) + .await + .unwrap(); + + let qr = get_securejoin_qr(&alice.ctx, None).await.unwrap(); + join_securejoin(&bob.ctx, &qr).await.unwrap(); + + // vc-request + let sent = bob.pop_sent_msg().await; + alice.recv_msg(&sent).await; + + // vc-auth-required + let sent = alice.pop_sent_msg().await; + bob.recv_msg(&sent).await; + + // vc-request-with-auth + let sent = bob.pop_sent_msg().await; + alice.recv_msg(&sent).await; + + // Alice has Bob verified now. + let contact_bob_id = + Contact::lookup_id_by_addr(&alice.ctx, "bob@example.net", Origin::Unknown) + .await + .expect("Error looking up contact") + .expect("Contact not found"); + let contact_bob = Contact::get_by_id(&alice.ctx, contact_bob_id) + .await + .unwrap(); + assert_eq!(contact_bob.is_verified(&alice.ctx).await.unwrap(), true); + + // Alice sends vc-contact-confirm, but it gets lost. + let _sent_vc_contact_confirm = alice.pop_sent_msg().await; + + // Bob should not yet have Alice verified + let contact_alice_id = + Contact::lookup_id_by_addr(&bob, "alice@example.org", Origin::Unknown) + .await + .expect("Error looking up contact") + .expect("Contact not found"); + let contact_alice = Contact::get_by_id(&bob, contact_alice_id).await.unwrap(); + assert_eq!(contact_alice.is_verified(&bob).await.unwrap(), false); + + // Alice sends a text message to Bob. + let received_hello = tcm.send_recv(&alice, &bob, "Hello!").await; + let chat_id = received_hello.chat_id; + let chat = Chat::load_from_db(&bob, chat_id).await.unwrap(); + assert_eq!(chat.is_protected(), true); + + // Received text message in a verified 1:1 chat results in backward verification + // and Bob now marks alice as verified. + let contact_alice = Contact::get_by_id(&bob, contact_alice_id).await.unwrap(); + assert_eq!(contact_alice.is_verified(&bob).await.unwrap(), true); + } } diff --git a/src/securejoin/bob.rs b/src/securejoin/bob.rs index 76b1e04777..106b49ae17 100644 --- a/src/securejoin/bob.rs +++ b/src/securejoin/bob.rs @@ -86,7 +86,7 @@ pub(super) async fn handle_auth_required( return Ok(HandshakeMessage::Ignore); }; - match bobstate.handle_message(context, message).await? { + match bobstate.handle_auth_required(context, message).await? { Some(BobHandshakeStage::Terminated(why)) => { bobstate.notify_aborted(context, why).await?; Ok(HandshakeMessage::Done) @@ -100,6 +100,9 @@ pub(super) async fn handle_auth_required( let chat_id = bobstate.joining_chat_id(context).await?; chat::add_info_msg(context, chat_id, &msg, time()).await?; } + bobstate + .set_peer_verified(context, message.timestamp_sent) + .await?; bobstate.emit_progress(context, JoinerProgress::RequestWithAuthSent); Ok(HandshakeMessage::Done) } @@ -107,46 +110,6 @@ pub(super) async fn handle_auth_required( } } -/// Handles `vc-contact-confirm` and `vg-member-added` handshake messages. -/// -/// # Bob - the joiner's side -/// ## Step 7 in the "Setup Contact protocol" -pub(super) async fn handle_contact_confirm( - context: &Context, - mut bobstate: BobState, - message: &MimeMessage, -) -> Result { - let retval = if bobstate.is_join_group() { - HandshakeMessage::Propagate - } else { - HandshakeMessage::Ignore - }; - match bobstate.handle_message(context, message).await? { - Some(BobHandshakeStage::Terminated(why)) => { - bobstate.notify_aborted(context, why).await?; - Ok(HandshakeMessage::Done) - } - Some(BobHandshakeStage::Completed) => { - // Note this goes to the 1:1 chat, as when joining a group we implicitly also - // verify both contacts (this could be a bug/security issue, see - // e.g. https://github.com/deltachat/deltachat-core-rust/issues/1177). - bobstate - .notify_peer_verified(context, message.timestamp_sent) - .await?; - bobstate.emit_progress(context, JoinerProgress::Succeeded); - Ok(retval) - } - Some(_) => { - warn!( - context, - "Impossible state returned from handling handshake message" - ); - Ok(retval) - } - None => Ok(retval), - } -} - /// Private implementations for user interactions about this [`BobState`]. impl BobState { fn is_join_group(&self) -> bool { @@ -156,7 +119,7 @@ impl BobState { } } - fn emit_progress(&self, context: &Context, progress: JoinerProgress) { + pub(crate) fn emit_progress(&self, context: &Context, progress: JoinerProgress) { let contact_id = self.invite().contact_id(); context.emit_event(EventType::SecurejoinJoinerProgress { contact_id, @@ -219,12 +182,9 @@ impl BobState { Ok(()) } - /// Notifies the user that the SecureJoin peer is verified. - /// - /// This creates an info message in the chat being joined. - async fn notify_peer_verified(&self, context: &Context, timestamp: i64) -> Result<()> { + /// Turns 1:1 chat with SecureJoin peer into protected chat. + pub(crate) async fn set_peer_verified(&self, context: &Context, timestamp: i64) -> Result<()> { let contact = Contact::get_by_id(context, self.invite().contact_id()).await?; - let chat_id = self.joining_chat_id(context).await?; self.alice_chat() .set_protection( context, @@ -233,7 +193,6 @@ impl BobState { Some(contact.id), ) .await?; - context.emit_event(EventType::ChatModified(chat_id)); Ok(()) } } @@ -242,7 +201,7 @@ impl BobState { /// /// This has an `From for usize` impl yielding numbers between 0 and a 1000 /// which can be shown as a progress bar. -enum JoinerProgress { +pub(crate) enum JoinerProgress { /// An error occurred. Error, /// vg-vc-request-with-auth sent. diff --git a/src/securejoin/bobstate.rs b/src/securejoin/bobstate.rs index 512874ad93..6f4915dc51 100644 --- a/src/securejoin/bobstate.rs +++ b/src/securejoin/bobstate.rs @@ -11,8 +11,9 @@ use anyhow::Result; use rusqlite::Connection; use super::qrinvite::QrInvite; -use super::{encrypted_and_signed, fingerprint_equals_sender, mark_peer_as_verified}; +use super::{encrypted_and_signed, verify_sender_by_fingerprint}; use crate::chat::{self, ChatId}; +use crate::config::Config; use crate::contact::{Contact, Origin}; use crate::context::Context; use crate::events::EventType; @@ -21,7 +22,9 @@ use crate::key::{load_self_public_key, DcKey}; use crate::message::{Message, Viewtype}; use crate::mimeparser::{MimeMessage, SystemMessage}; use crate::param::Param; +use crate::securejoin::Peerstate; use crate::sql::Sql; +use crate::tools::time; /// The stage of the [`BobState`] securejoin handshake protocol state machine. /// @@ -30,14 +33,9 @@ use crate::sql::Sql; #[derive(Clone, Copy, Debug, Display)] pub enum BobHandshakeStage { /// Step 2 completed: (vc|vg)-request message sent. - /// - /// Note that this is only ever returned by [`BobState::start_protocol`] and never by - /// [`BobState::handle_message`]. RequestSent, /// Step 4 completed: (vc|vg)-request-with-auth message sent. RequestWithAuthSent, - /// The protocol completed successfully. - Completed, /// The protocol prematurely terminated with given reason. Terminated(&'static str), } @@ -92,21 +90,26 @@ impl BobState { invite: QrInvite, chat_id: ChatId, ) -> Result<(Self, BobHandshakeStage, Vec)> { - let (stage, next) = - if fingerprint_equals_sender(context, invite.fingerprint(), invite.contact_id()).await? - { - // The scanned fingerprint matches Alice's key, we can proceed to step 4b. - info!(context, "Taking securejoin protocol shortcut"); - send_handshake_message(context, &invite, chat_id, BobHandshakeMsg::RequestWithAuth) - .await?; - ( - BobHandshakeStage::RequestWithAuthSent, - SecureJoinStep::ContactConfirm, - ) - } else { - send_handshake_message(context, &invite, chat_id, BobHandshakeMsg::Request).await?; - (BobHandshakeStage::RequestSent, SecureJoinStep::AuthRequired) - }; + let peer_verified = + verify_sender_by_fingerprint(context, invite.fingerprint(), invite.contact_id()) + .await?; + + let (stage, next); + if peer_verified { + // The scanned fingerprint matches Alice's key, we can proceed to step 4b. + info!(context, "Taking securejoin protocol shortcut"); + send_handshake_message(context, &invite, chat_id, BobHandshakeMsg::RequestWithAuth) + .await?; + + stage = BobHandshakeStage::RequestWithAuthSent; + next = SecureJoinStep::ContactConfirm; + } else { + send_handshake_message(context, &invite, chat_id, BobHandshakeMsg::Request).await?; + + stage = BobHandshakeStage::RequestSent; + next = SecureJoinStep::AuthRequired; + }; + let (id, aborted_states) = Self::insert_new_db_entry(context, next, invite.clone(), chat_id).await?; let state = Self { @@ -115,6 +118,12 @@ impl BobState { next, chat_id, }; + + if peer_verified { + // Mark 1:1 chat as verified already. + state.set_peer_verified(context, time()).await?; + } + Ok((state, stage, aborted_states)) } @@ -230,13 +239,13 @@ impl BobState { Ok(()) } - /// Handles the given message for the securejoin handshake for Bob. + /// Handles {vc,vg}-auth-required message of the securejoin handshake for Bob. /// /// If the message was not used for this handshake `None` is returned, otherwise the new - /// stage is returned. Once [`BobHandshakeStage::Completed`] or - /// [`BobHandshakeStage::Terminated`] are reached this [`BobState`] should be destroyed, + /// stage is returned. Once [`BobHandshakeStage::Terminated`] is reached this + /// [`BobState`] should be destroyed, /// further calling it will just result in the messages being unused by this handshake. - pub(crate) async fn handle_message( + pub(crate) async fn handle_auth_required( &mut self, context: &Context, mime_message: &MimeMessage, @@ -256,39 +265,7 @@ impl BobState { info!(context, "{} message out of sync for BobState", step); return Ok(None); } - match step.as_str() { - "vg-auth-required" | "vc-auth-required" => { - self.step_auth_required(context, mime_message).await - } - "vg-member-added" | "vc-contact-confirm" => { - self.step_contact_confirm(context, mime_message).await - } - _ => { - warn!(context, "Invalid step for BobState: {}", step); - Ok(None) - } - } - } - - /// Returns `true` if the message is expected according to the protocol. - fn is_msg_expected(&self, context: &Context, step: &str) -> bool { - let variant_matches = match self.invite { - QrInvite::Contact { .. } => step.starts_with("vc-"), - QrInvite::Group { .. } => step.starts_with("vg-"), - }; - let step_matches = self.next.matches(context, step); - variant_matches && step_matches - } - /// Handles a *vc-auth-required* or *vg-auth-required* message. - /// - /// # Bob - the joiner's side - /// ## Step 4 in the "Setup Contact protocol", section 2.1 of countermitm 0.10.0 - async fn step_auth_required( - &mut self, - context: &Context, - mime_message: &MimeMessage, - ) -> Result> { info!( context, "Bob Step 4 - handling {{vc,vg}}-auth-required message." @@ -303,14 +280,19 @@ impl BobState { .await?; return Ok(Some(BobHandshakeStage::Terminated(reason))); } - if !fingerprint_equals_sender(context, self.invite.fingerprint(), self.invite.contact_id()) - .await? + if !verify_sender_by_fingerprint( + context, + self.invite.fingerprint(), + self.invite.contact_id(), + ) + .await? { self.update_next(&context.sql, SecureJoinStep::Terminated) .await?; return Ok(Some(BobHandshakeStage::Terminated("Fingerprint mismatch"))); } info!(context, "Fingerprint verified.",); + self.update_next(&context.sql, SecureJoinStep::ContactConfirm) .await?; self.send_handshake_message(context, BobHandshakeMsg::RequestWithAuth) @@ -318,36 +300,39 @@ impl BobState { Ok(Some(BobHandshakeStage::RequestWithAuthSent)) } + /// Returns `true` if the message is expected according to the protocol. + pub(crate) fn is_msg_expected(&self, context: &Context, step: &str) -> bool { + let variant_matches = match self.invite { + QrInvite::Contact { .. } => step.starts_with("vc-"), + QrInvite::Group { .. } => step.starts_with("vg-"), + }; + let step_matches = self.next.matches(context, step); + variant_matches && step_matches + } + /// Handles a *vc-contact-confirm* or *vg-member-added* message. /// /// # Bob - the joiner's side /// ## Step 7 in the "Setup Contact protocol", section 2.1 of countermitm 0.10.0 - /// - /// This deviates from the protocol by also sending a confirmation message in response - /// to the *vc-contact-confirm* message. This has no specific value to the protocol and - /// is only done out of symmetry with *vg-member-added* handling. - async fn step_contact_confirm( - &mut self, - context: &Context, - mime_message: &MimeMessage, - ) -> Result> { - info!( - context, - "Bob Step 7 - handling vc-contact-confirm/vg-member-added message." - ); - mark_peer_as_verified( - context, - self.invite.fingerprint().clone(), - mime_message.from.addr.to_string(), - ) - .await?; + pub(crate) async fn step_contact_confirm(&mut self, context: &Context) -> Result<()> { + let fingerprint = self.invite.fingerprint(); + let Some(ref mut peerstate) = Peerstate::from_fingerprint(context, fingerprint).await? + else { + return Ok(()); + }; + + // Mark peer as backward verified. + peerstate.backward_verified_key_id = + Some(context.get_config_i64(Config::KeyId).await?).filter(|&id| id > 0); + peerstate.save_to_db(&context.sql).await?; + Contact::scaleup_origin_by_id(context, self.invite.contact_id(), Origin::SecurejoinJoined) .await?; context.emit_event(EventType::ContactsChanged(None)); self.update_next(&context.sql, SecureJoinStep::Completed) .await?; - Ok(Some(BobHandshakeStage::Completed)) + Ok(()) } /// Sends the requested handshake message to Alice. @@ -389,13 +374,13 @@ async fn send_handshake_message( // Sends the Secure-Join-Auth header in mimefactory.rs. msg.param.set(Param::Arg2, invite.authcode()); msg.param.set_int(Param::GuaranteeE2ee, 1); + + // Sends our own fingerprint in the Secure-Join-Fingerprint header. + let bob_fp = load_self_public_key(context).await?.fingerprint(); + msg.param.set(Param::Arg3, bob_fp.hex()); } }; - // Sends our own fingerprint in the Secure-Join-Fingerprint header. - let bob_fp = load_self_public_key(context).await?.fingerprint(); - msg.param.set(Param::Arg3, bob_fp.hex()); - // Sends the grpid in the Secure-Join-Group header. if let QrInvite::Group { ref grpid, .. } = invite { msg.param.set(Param::Arg4, grpid); diff --git a/src/sql/migrations.rs b/src/sql/migrations.rs index 658a243b9b..5301f7de3e 100644 --- a/src/sql/migrations.rs +++ b/src/sql/migrations.rs @@ -886,6 +886,20 @@ CREATE INDEX msgs_status_updates_index2 ON msgs_status_updates (uid); sql.set_db_version_in_cache(version).await?; } + if dbversion < 109 { + sql.execute_migration( + r#"ALTER TABLE acpeerstates + ADD COLUMN backward_verified_key_id -- What we think the contact has as our verified key + INTEGER; + UPDATE acpeerstates + SET backward_verified_key_id=(SELECT value FROM config WHERE keyname='key_id') + WHERE verified_key IS NOT NULL + "#, + 109, + ) + .await?; + } + let new_version = sql .get_raw_config_int(VERSION_CFG) .await? diff --git a/src/test_utils.rs b/src/test_utils.rs index 20de434fe3..f8ff5ba704 100644 --- a/src/test_utils.rs +++ b/src/test_utils.rs @@ -1047,7 +1047,8 @@ fn print_logevent(logevent: &LogEvent) { } } -/// Saves the other account's public key as verified. +/// Saves the other account's public key as verified +/// and peerstate as backwards verified. pub(crate) async fn mark_as_verified(this: &TestContext, other: &TestContext) { let mut peerstate = Peerstate::from_header( &EncryptHelper::new(other).await.unwrap().get_aheader(), @@ -1063,6 +1064,7 @@ pub(crate) async fn mark_as_verified(this: &TestContext, other: &TestContext) { peerstate.verified_key = peerstate.public_key.clone(); peerstate.verified_key_fingerprint = peerstate.public_key_fingerprint.clone(); + peerstate.backward_verified_key_id = Some(this.get_config_i64(Config::KeyId).await.unwrap()); peerstate.save_to_db(&this.sql).await.unwrap(); }