From 4da9e392ca2924f8bb7076d7e0426b8e03bfb571 Mon Sep 17 00:00:00 2001 From: iequidoo Date: Fri, 28 Apr 2023 13:12:22 -0400 Subject: [PATCH 1/2] BREAKING: Remove Secure-Join-Fingerprint header from "vc-contact-confirm", "vg-member-added" messages (#3836) It's a compatibility crutch for old clients (< 1.107.0), they require it in the subject messages. Currently DC sends Autocrypt key gossips instead, they're better because knowing a key allows not to only trust the peer, but also encrypt to it. Before it was a problem for other devices on a joining side going online after a successful Securejoin setup -- they didn't have a joiner's key to encrypt to it. So, we decided not to complicate the Securejoin with sending keys instead, but rely on the Autocrypt. Also there's a PR to the Countermitm doc documenting when gossips in 'vc-request-with-auth' are needed: Bob's own key fingerprint ``Bob_FP``, the second challenge ``AUTH`` from step 1 and optionally an Autocrypt-Gossip header for Alice's Autocrypt key in order for a second device of Bob to learn Alice's verified key. --- CHANGELOG.md | 1 + src/mimefactory.rs | 11 ----------- src/securejoin.rs | 37 +++---------------------------------- 3 files changed, 4 insertions(+), 45 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 359ab0d20f..95e8a6b13e 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -6,6 +6,7 @@ - BREAKING: jsonrpc: - `get_chatlist_items_by_entries` now takes only chatids instead of `ChatListEntries` - `get_chatlist_entries` now returns `Vec` of chatids instead of `ChatListEntries` +- BREAKING: Remove Secure-Join-Fingerprint header from "vc-contact-confirm", "vg-member-added" messages. ## [1.114.0] - 2023-04-24 diff --git a/src/mimefactory.rs b/src/mimefactory.rs index 24a22de0e6..d8a48e6cad 100644 --- a/src/mimefactory.rs +++ b/src/mimefactory.rs @@ -945,17 +945,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/securejoin.rs b/src/securejoin.rs index f854b7d980..af46968af2 100644 --- a/src/securejoin.rs +++ b/src/securejoin.rs @@ -465,14 +465,9 @@ pub(crate) async fn handle_securejoin_handshake( info_chat_id(context, contact_id).await?, ) .await?; - send_alice_handshake_msg( - context, - contact_id, - "vc-contact-confirm", - Some(fingerprint), - ) - .await - .context("failed sending vc-contact-confirm message")?; + send_alice_handshake_msg(context, contact_id, "vc-contact-confirm", None) + .await + .context("failed sending vc-contact-confirm message")?; inviter_progress!(context, contact_id, 1000); } @@ -631,32 +626,6 @@ pub(crate) async fn observe_securejoin_on_other_device( } peerstate.prefer_encrypt = EncryptPreference::Mutual; peerstate.save_to_db(&context.sql).await.unwrap_or_default(); - } 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()?; - if mark_peer_as_verified( - context, - fingerprint, - Contact::load_from_db(context, contact_id) - .await? - .get_addr() - .to_owned(), - ) - .await - .is_err() - { - 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, From 3178308c14aa6c0de4bb9d3e0fc0096ce42b4b5e Mon Sep 17 00:00:00 2001 From: iequidoo Date: Fri, 28 Apr 2023 14:01:21 -0400 Subject: [PATCH 2/2] BREAKING: Remove "vc-contact-confirm-received", "vg-member-added-received" messages from Securejoin protocol (#3836) They are an overkill and do not solve any problem. Also they aren't documented anywhere. And all necessary tests were added before and they work w/o the removed code. AFAIU, they were added for Alice (joiner) to be sure that Bob (joinee) has Alice two-way verified, i.e. Bob can add Alice to other verified groups because Bob knows that Alice trusts them. But that means Alice should retry sending vc-contact-confirm/vg-member-added messages until getting *-received message from Bob. But then better let Bob retry sending *-auth-required until getting vc-contact-confirm/vg-member-added from Alice. And if Alice sees no more retries from Bob, either Bob has got vc-contact-confirm/vg-member-added from Alice or there are communication problems. But the latter can't be solved anyway by adding extra messages to the protocol. Note that there are no retries currently, instead we rely on that Bob will eventually receive a message from Alice and thus know that Alice verified them. But i could miss some details why they were added, so just in case, citing from #3836: @iequidoo: Btw, can somebody explain the purpose of vg-member-added-received/vc-contact-confirm-received messages in the protocol? They look excessive and for me it's like a try to solve that problem with two friends that want to meet and drink some beer but only if each of them is sure that their beermate is also going to the meeting :) Even if Bob didn't receive vg-member-added message (e.g. because of mail delivery problems), we can consider Bob joined -- Bob can try sending messages to the group and that must work afaiu. @flub: Yes, this is a weird state. It is currently the difference between an internal state "un-idirectional verified" and the exposed state "bi-directional verified". The latter (bi-) means both know that both have each other verified, in the former (uni-) only one of them knows this and the other hasn't figured this out yet. IIRC the last time this was discussed the revelation (at least to me) was that the main practical difference between these two is that bi-directional allows you to add the other person to a verified group, while if you only have them uni-directional verified you can not do they since they don't trust you enough (IIRC, there should be a cryptpad somewhere with this written down). @link2xt: When Bob receives vc-auth-required, he already has Alice one-way verified. When Alice receives vc-request-with-auth, she has Bob two-way verified (she has verified Bob's key and she know Bob has verified her), but Bob still does not know about this. When Bob receives vc-contact-confirm (or vg-member-added) he sets Alice state to "two-way verified". The question is what happens if vc-contact-confirm/vg-member-added is lost. In this case Alice has Bob two-way verified, while Bob has Alice only one-way verified. Because of this, Bob cannot safely add Alice to any verified group, because Bob thinks maybe Alice has not received vc-request-with-auth and has Bob completely unverified. However, Alice still can add Bob to verified groups and if Bob later receives any message from Alice in a verified group, he sets Alice to two-way verified, so eventually both Alice and Bob converge to a two-way verified state. This is not how it is currently implemented, but this was the idea during the discussion with @flub and @missytake. But vg-member-added-received/vc-contact-confirm-received is an overkill and can be removed IMO, it does not solve any problem. --- CHANGELOG.md | 1 + src/securejoin.rs | 72 ++++---------------------------------- src/securejoin/bobstate.rs | 20 ----------- 3 files changed, 8 insertions(+), 85 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 95e8a6b13e..616b11545b 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,6 +7,7 @@ - `get_chatlist_items_by_entries` now takes only chatids instead of `ChatListEntries` - `get_chatlist_entries` now returns `Vec` of chatids instead of `ChatListEntries` - BREAKING: Remove Secure-Join-Fingerprint header from "vc-contact-confirm", "vg-member-added" messages. +- BREAKING: Remove "vc-contact-confirm-received", "vg-member-added-received" messages from Securejoin protocol. ## [1.114.0] - 2023-04-24 diff --git a/src/securejoin.rs b/src/securejoin.rs index af46968af2..642d5c822e 100644 --- a/src/securejoin.rs +++ b/src/securejoin.rs @@ -9,7 +9,7 @@ use crate::aheader::EncryptPreference; use crate::chat::{self, Chat, ChatId, ChatIdBlocked}; use crate::config::Config; use crate::constants::Blocked; -use crate::contact::{Contact, ContactId, Origin, VerifiedStatus}; +use crate::contact::{Contact, ContactId, Origin}; use crate::context::Context; use crate::e2ee::ensure_secret_key_exists; use crate::events::EventType; @@ -489,33 +489,8 @@ pub(crate) async fn handle_securejoin_handshake( } } "vg-member-added-received" | "vc-contact-confirm-received" => { - /*========================================================== - ==== Alice - the inviter side ==== - ==== Step 8 in "Out-of-band verified groups" protocol ==== - ==========================================================*/ - - if let Ok(contact) = Contact::get_by_id(context, contact_id).await { - if contact.is_verified(context).await? == VerifiedStatus::Unverified { - warn!(context, "{} invalid.", step); - return Ok(HandshakeMessage::Ignore); - } - if join_vg { - let field_grpid = mime_message - .get_header(HeaderDef::SecureJoinGroup) - .map(|s| s.as_str()) - .unwrap_or_else(|| ""); - if let Err(err) = chat::get_chat_id_by_grpid(context, field_grpid).await { - warn!(context, "Failed to lookup chat_id from grpid: {}", err); - return Err( - err.context(format!("Chat for group {} not found", &field_grpid)) - ); - } - } - Ok(HandshakeMessage::Ignore) // "Done" deletes the message and breaks multi-device - } else { - warn!(context, "{} invalid.", step); - Ok(HandshakeMessage::Ignore) - } + // The peer isn't updated yet and still sends these messages. + Ok(HandshakeMessage::Ignore) } _ => { warn!(context, "invalid step: {}", step); @@ -535,12 +510,6 @@ pub(crate) async fn handle_securejoin_handshake( /// 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 the self-sent-message vg-member-added-received -/// we know that we're an joiner-observer. -/// the joining device has marked the peer as verified on vg-member-added/vc-contact-confirm -/// before sending vg-member-added-received - so, if we observe vg-member-added-received, -/// we can mark the peer as verified as well. pub(crate) async fn observe_securejoin_on_other_device( context: &Context, mime_message: &MimeMessage, @@ -558,9 +527,7 @@ pub(crate) async fn observe_securejoin_on_other_device( "vg-request-with-auth" | "vc-request-with-auth" | "vg-member-added" - | "vc-contact-confirm" - | "vg-member-added-received" - | "vc-contact-confirm-received" => { + | "vc-contact-confirm" => { if !encrypted_and_signed( context, mime_message, @@ -929,7 +896,7 @@ mod tests { VerifiedStatus::Unverified ); - // Step 7: Bob receives vc-contact-confirm, sends vc-contact-confirm-received + // Step 7: Bob receives vc-contact-confirm bob.recv_msg(&sent).await; assert_eq!( contact_alice.is_verified(&bob.ctx).await.unwrap(), @@ -954,15 +921,6 @@ mod tests { let text = msg.get_text().unwrap(); assert!(text.contains("alice@example.org verified")); } - - // Check Bob sent the final message - let sent = bob.pop_sent_msg().await; - let msg = alice.parse_msg(&sent).await; - assert!(msg.was_encrypted()); - assert_eq!( - msg.get_header(HeaderDef::SecureJoin).unwrap(), - "vc-contact-confirm-received" - ); } #[tokio::test(flavor = "multi_thread", worker_threads = 2)] @@ -1080,20 +1038,12 @@ mod tests { VerifiedStatus::Unverified ); - // Step 7: Bob receives vc-contact-confirm, sends vc-contact-confirm-received + // Step 7: Bob receives vc-contact-confirm bob.recv_msg(&sent).await; assert_eq!( contact_alice.is_verified(&bob.ctx).await?, VerifiedStatus::BidirectVerified ); - - let sent = bob.pop_sent_msg().await; - let msg = alice.parse_msg(&sent).await; - assert!(msg.was_encrypted()); - assert_eq!( - msg.get_header(HeaderDef::SecureJoin).unwrap(), - "vc-contact-confirm-received" - ); Ok(()) } @@ -1277,7 +1227,7 @@ mod tests { VerifiedStatus::Unverified ); - // Step 7: Bob receives vg-member-added, sends vg-member-added-received + // Step 7: Bob receives vg-member-added bob.recv_msg(&sent).await; { // Bob has Alice verified, message shows up in the group chat. @@ -1324,14 +1274,6 @@ mod tests { } } - let sent = bob.pop_sent_msg().await; - let msg = alice.parse_msg(&sent).await; - assert!(msg.was_encrypted()); - assert_eq!( - msg.get_header(HeaderDef::SecureJoin).unwrap(), - "vg-member-added-received" - ); - let bob_chat = Chat::load_from_db(&bob.ctx, bob_chatid).await?; assert!(bob_chat.is_protected()); assert!(bob_chat.typ == Chattype::Group); diff --git a/src/securejoin/bobstate.rs b/src/securejoin/bobstate.rs index 04b1edd333..dc9406d758 100644 --- a/src/securejoin/bobstate.rs +++ b/src/securejoin/bobstate.rs @@ -386,17 +386,6 @@ impl BobState { } } - self.send_handshake_message(context, BobHandshakeMsg::ContactConfirmReceived) - .await - .map_err(|_| { - warn!( - context, - "Failed to send vc-contact-confirm-received/vg-member-added-received" - ); - }) - // This is not an error affecting the protocol outcome. - .ok(); - self.update_next(&context.sql, SecureJoinStep::Completed) .await?; Ok(Some(BobHandshakeStage::Completed)) @@ -442,9 +431,6 @@ async fn send_handshake_message( msg.param.set(Param::Arg2, invite.authcode()); msg.param.set_int(Param::GuaranteeE2ee, 1); } - BobHandshakeMsg::ContactConfirmReceived => { - msg.param.set_int(Param::GuaranteeE2ee, 1); - } }; // Sends our own fingerprint in the Secure-Join-Fingerprint header. @@ -466,8 +452,6 @@ enum BobHandshakeMsg { Request, /// vc-request-with-auth or vg-request-with-auth RequestWithAuth, - /// vc-contact-confirm-received or vg-member-added-received - ContactConfirmReceived, } impl BobHandshakeMsg { @@ -495,10 +479,6 @@ impl BobHandshakeMsg { QrInvite::Contact { .. } => "vc-request-with-auth", QrInvite::Group { .. } => "vg-request-with-auth", }, - Self::ContactConfirmReceived => match invite { - QrInvite::Contact { .. } => "vc-contact-confirm-received", - QrInvite::Group { .. } => "vg-member-added-received", - }, } } }