fix: Domain separation between securejoin auth tokens and broadcast channel secrets (#7981)

Can be reviewed commit-by-commit.

This fixes another silly thing you can do with securejoinv3: show Bob a
QR code with auth token that is a broadcast channel secret of a known
channel, then never respond. Bob will decrypt messages from the channel
and drop them because they are sent by the "wrong" sender.

This can be avoided with domain separation, instead of
encrypting/decrypting securejoinv3 messages directly with auth token,
encrypt/decrypt them with `securejoin/<auth token>` as the secret or
even `securejoinv3/<alice's fingerprint>/<auth token>`. For existing
broadcast channels we cannot do this, but for securejoinv3 that is not
released yet this looks like an improvement that avoids at least this
problem.

Credits to link2xt for noticing the problem.

This also adds Alice's fingerprint to the auth tokens, which
was pretty easy to do. I find it hard to develop an intuition for
whether this is important, or whether we will be annoyed by it in the
future.

**Note:** This means that QR code scans will not work if one of the chat
partners uses a self-compiled core between c724e2981 and merging this PR
here. This is fine; we will just have to tell the other developers to
update their self-compiled cores.

src/decrypt.rs

@@ -19,6 +19,7 @@ use crate::chat::ChatId;
 use crate::constants::Chattype;
 use crate::contact::ContactId;
 use crate::context::Context;
+use crate::key::self_fingerprint;
 use crate::key::{Fingerprint, SignedPublicKey, load_self_secret_keyring};
 use crate::token::Namespace;
 
@@ -93,6 +94,7 @@ async fn decrypt_session_key_symmetrically(
     context: &Context,
     esk: &SymKeyEncryptedSessionKey,
 ) -> Result<(PlainSessionKey, Option<String>)> {
+    let self_fp = self_fingerprint(context).await?;
     let query_only = true;
     context
         .sql
@@ -114,7 +116,7 @@ async fn decrypt_session_key_symmetrically(
             // Finally, try decrypting using own AUTH tokens
             // There can be a lot of AUTH tokens,
             // because a new one is generated every time a QR code is shown
-            let res: Option<PlainSessionKey> = try_decrypt_with_auth_token(esk, conn)?;
+            let res: Option<PlainSessionKey> = try_decrypt_with_auth_token(esk, conn, self_fp)?;
             if let Some(plain_session_key) = res {
                 return Ok((plain_session_key, None));
             }
@@ -133,7 +135,9 @@ fn try_decrypt_with_bobstate(
     while let Some(row) = rows.next()? {
         let invite: crate::securejoin::QrInvite = row.get(0)?;
         let authcode = invite.authcode().to_string();
-        if let Ok(psk) = decrypt_session_key_with_password(esk, &Password::from(authcode)) {
+        let alice_fp = invite.fingerprint().hex();
+        let shared_secret = format!("securejoin/{alice_fp}/{authcode}");
+        if let Ok(psk) = decrypt_session_key_with_password(esk, &Password::from(shared_secret)) {
             let fingerprint = invite.fingerprint().hex();
             return Ok(Some((psk, fingerprint)));
         }
@@ -198,6 +202,7 @@ fn try_decrypt_with_broadcast_secret_inner(
 fn try_decrypt_with_auth_token(
     esk: &SymKeyEncryptedSessionKey,
     conn: &mut rusqlite::Connection,
+    self_fingerprint: &str,
 ) -> Result<Option<PlainSessionKey>> {
     // ORDER BY id DESC to query the most-recently saved tokens are returned first.
     // This improves performance when Bob scans a QR code that was just created.
@@ -205,7 +210,8 @@ fn try_decrypt_with_auth_token(
     let mut rows = stmt.query((Namespace::Auth,))?;
     while let Some(row) = rows.next()? {
         let token: String = row.get(0)?;
-        if let Ok(psk) = decrypt_session_key_with_password(esk, &Password::from(token)) {
+        let shared_secret = format!("securejoin/{self_fingerprint}/{token}");
+        if let Ok(psk) = decrypt_session_key_with_password(esk, &Password::from(shared_secret)) {
             return Ok(Some(psk));
         }
     }

src/mimefactory.rs

@@ -2335,6 +2335,7 @@ pub(crate) async fn render_symm_encrypted_securejoin_message(
     rfc724_mid: &str,
     attach_self_pubkey: bool,
     auth: &str,
+    shared_secret: &str,
 ) -> Result<String> {
     info!(context, "Sending secure-join message {step:?}.");
 
@@ -2427,7 +2428,7 @@ pub(crate) async fn render_symm_encrypted_securejoin_message(
         // Only sign the message if we attach the pubkey.
         let sign = attach_self_pubkey;
         let encrypted = encrypt_helper
-            .encrypt_symmetrically(context, auth, message, compress, sign)
+            .encrypt_symmetrically(context, shared_secret, message, compress, sign)
             .await?;
 
         wrap_encrypted_part(encrypted)

src/mimeparser/shared_secret_decryption_tests.rs

@@ -1,7 +1,7 @@
 use super::*;
 use crate::chat::{create_broadcast, load_broadcast_secret};
 use crate::constants::DC_CHAT_ID_TRASH;
-use crate::key::load_self_secret_key;
+use crate::key::{load_self_secret_key, self_fingerprint};
 use crate::pgp;
 use crate::qr::{Qr, check_qr};
 use crate::receive_imf::receive_imf;
@@ -26,7 +26,7 @@ use anyhow::Result;
 async fn test_shared_secret_decryption_ex(
     recipient_ctx: &TestContext,
     from_addr: &str,
-    secret: &str,
+    secret_for_encryption: &str,
     signer_ctx: Option<&TestContext>,
     expected_error: Option<&str>,
 ) -> Result<()> {
@@ -45,8 +45,13 @@ async fn test_shared_secret_decryption_ex(
         recipient_ctx.add_or_lookup_contact(signer_ctx).await;
     }
 
-    let encrypted_msg =
-        pgp::symm_encrypt_message(plain_text.as_bytes().to_vec(), signer_key, secret, true).await?;
+    let encrypted_msg = pgp::symm_encrypt_message(
+        plain_text.as_bytes().to_vec(),
+        signer_key,
+        secret_for_encryption,
+        true,
+    )
+    .await?;
 
     let boundary = "boundary123";
     let rcvd_mail = format!(
@@ -189,19 +194,21 @@ async fn test_qr_code_security() -> Result<()> {
     let bob = &tcm.bob().await;
     let charlie = &tcm.charlie().await; // Attacker
 
-    let qr = get_securejoin_qr(bob, None).await?;
-    let Qr::AskVerifyContact { authcode, .. } = check_qr(alice, &qr).await? else {
+    let qr = get_securejoin_qr(alice, None).await?;
+    let Qr::AskVerifyContact { authcode, .. } = check_qr(bob, &qr).await? else {
         unreachable!()
     };
     // Start a securejoin process, but don't finish it:
-    join_securejoin(alice, &qr).await?;
+    join_securejoin(bob, &qr).await?;
 
     let charlie_addr = charlie.get_config(Config::Addr).await?.unwrap();
 
+    let alice_fp = self_fingerprint(alice).await?;
+    let secret_for_encryption = dbg!(format!("securejoin/{alice_fp}/{authcode}"));
     test_shared_secret_decryption_ex(
-        alice,
+        bob,
         &charlie_addr,
-        &authcode,
+        &secret_for_encryption,
         Some(charlie),
         Some("This sender is not allowed to encrypt with this secret key"),
     )
@@ -221,7 +228,16 @@ async fn test_qr_code_happy_path() -> Result<()> {
     // Start a securejoin process, but don't finish it:
     join_securejoin(bob, &qr).await?;
 
-    test_shared_secret_decryption_ex(bob, "alice@example.net", &authcode, Some(alice), None).await
+    let alice_fp = self_fingerprint(alice).await?;
+    let secret_for_encryption = format!("securejoin/{alice_fp}/{authcode}");
+    test_shared_secret_decryption_ex(
+        bob,
+        "alice@example.net",
+        &secret_for_encryption,
+        Some(alice),
+        None,
+    )
+    .await
 }
 
 /// Control: Test that the behavior is the same when the shared secret is unknown

src/pgp.rs

@@ -590,7 +590,7 @@ mod tests {
     use super::*;
     use crate::{
         decrypt,
-        key::{load_self_public_key, load_self_secret_key, store_self_keypair},
+        key::{load_self_public_key, self_fingerprint, store_self_keypair},
         mimefactory::{render_outer_message, wrap_encrypted_part},
         test_utils::{TestContext, TestContextManager, alice_keypair, bob_keypair},
         token,
@@ -601,11 +601,11 @@ mod tests {
     async fn decrypt_bytes(
         bytes: Vec<u8>,
         private_keys_for_decryption: &[SignedSecretKey],
-        shared_secrets: &[String],
+        auth_tokens_for_decryption: &[String],
     ) -> Result<pgp::composed::Message<'static>> {
         let t = &TestContext::new().await;
 
-        for secret in shared_secrets {
+        for secret in auth_tokens_for_decryption {
             token::save(t, token::Namespace::Auth, None, secret, 0).await?;
         }
         let [secret_key] = private_keys_for_decryption else {
@@ -807,36 +807,6 @@ mod tests {
         assert_eq!(valid_signatures.len(), 0);
     }
 
-    #[tokio::test(flavor = "multi_thread", worker_threads = 2)]
-    async fn test_encrypt_decrypt_broadcast() -> Result<()> {
-        let mut tcm = TestContextManager::new();
-        let alice = &tcm.alice().await;
-        let bob = &tcm.bob().await;
-
-        let plain = Vec::from(b"this is the secret message");
-        let shared_secret = "shared secret";
-        let ctext = symm_encrypt_message(
-            plain.clone(),
-            Some(load_self_secret_key(alice).await?),
-            shared_secret,
-            true,
-        )
-        .await?;
-
-        let bob_private_keyring = crate::key::load_self_secret_keyring(bob).await?;
-        let mut decrypted = decrypt_bytes(
-            ctext.into(),
-            &bob_private_keyring,
-            &[shared_secret.to_string()],
-        )
-        .await
-        .unwrap();
-
-        assert_eq!(decrypted.as_data_vec()?, plain);
-
-        Ok(())
-    }
-
     #[tokio::test(flavor = "multi_thread", worker_threads = 2)]
     async fn test_dont_decrypt_expensive_message_happy_path() -> Result<()> {
         let s2k = StringToKey::Salted {
@@ -883,8 +853,9 @@ mod tests {
 
         let plain = Vec::from(b"this is the secret message");
         let shared_secret = "shared secret";
+        let bob_fp = self_fingerprint(bob).await?;
 
-        let shared_secret_pw = Password::from(shared_secret.to_string());
+        let shared_secret_pw = Password::from(format!("securejoin/{bob_fp}/{shared_secret}"));
         let msg = MessageBuilder::from_bytes("", plain);
         let mut rng = thread_rng();
 

src/securejoin.rs

@@ -17,7 +17,7 @@ use crate::context::Context;
 use crate::e2ee::ensure_secret_key_exists;
 use crate::events::EventType;
 use crate::headerdef::HeaderDef;
-use crate::key::{DcKey, Fingerprint, load_self_public_key};
+use crate::key::{DcKey, Fingerprint, load_self_public_key, self_fingerprint};
 use crate::log::LogExt as _;
 use crate::log::warn;
 use crate::message::{self, Message, MsgId, Viewtype};
@@ -540,12 +540,15 @@ pub(crate) async fn handle_securejoin_handshake(
             let rfc724_mid = create_outgoing_rfc724_mid();
             let addr = ContactAddress::new(&mime_message.from.addr)?;
             let attach_self_pubkey = true;
+            let self_fp = self_fingerprint(context).await?;
+            let shared_secret = format!("securejoin/{self_fp}/{auth}");
             let rendered_message = mimefactory::render_symm_encrypted_securejoin_message(
                 context,
                 "vc-pubkey",
                 &rfc724_mid,
                 attach_self_pubkey,
                 auth,
+                &shared_secret,
             )
             .await?;
 

src/securejoin/bob.rs

@@ -312,13 +312,17 @@ pub(crate) async fn send_handshake_message(
         let rfc724_mid = create_outgoing_rfc724_mid();
         let contact = Contact::get_by_id(context, invite.contact_id()).await?;
         let recipient = contact.get_addr();
+        let alice_fp = invite.fingerprint().hex();
+        let auth = invite.authcode();
+        let shared_secret = format!("securejoin/{alice_fp}/{auth}");
         let attach_self_pubkey = false;
         let rendered_message = mimefactory::render_symm_encrypted_securejoin_message(
             context,
             "vc-request-pubkey",
             &rfc724_mid,
             attach_self_pubkey,
-            invite.authcode(),
+            auth,
+            &shared_secret,
         )
         .await?;