diff --git a/src/chat/chat_tests.rs b/src/chat/chat_tests.rs index 1cb10db3e..32df45b08 100644 --- a/src/chat/chat_tests.rs +++ b/src/chat/chat_tests.rs @@ -3866,14 +3866,20 @@ async fn test_only_broadcast_owner_can_send_2() -> Result<()> { .self_fingerprint .take(); - tcm.section( - "Alice sends a message, which is not put into the broadcast chat but into a 1:1 chat", - ); + tcm.section("Alice sends a message, which is trashed"); let sent = alice.send_text(alice_broadcast_id, "Hi").await; - let rcvd = bob.recv_msg(&sent).await; - assert_eq!(rcvd.text, "Hi"); - let bob_alice_chat_id = bob.get_chat(alice).await.id; - assert_eq!(rcvd.chat_id, bob_alice_chat_id); + bob.recv_msg_trash(&sent).await; + let EventType::Warning(warning) = bob + .evtracker + .get_matching(|ev| matches!(ev, EventType::Warning(_))) + .await + else { + unreachable!() + }; + assert!( + warning.contains("This sender is not allowed to encrypt with this secret key"), + "Wrong warning: {warning}" + ); Ok(()) } @@ -3942,6 +3948,7 @@ async fn test_encrypt_decrypt_broadcast() -> Result<()> { let grpid = "grpid"; let alice_bob_contact_id = alice.add_or_lookup_contact_id(bob).await; + let bob_alice_contact_id = bob.add_or_lookup_contact_id(alice).await; tcm.section("Create a broadcast channel with Bob, and send a message"); let alice_chat_id = create_out_broadcast_ex( @@ -3965,6 +3972,7 @@ async fn test_encrypt_decrypt_broadcast() -> Result<()> { ) .await?; save_broadcast_secret(bob, bob_chat_id, secret).await?; + add_to_chat_contacts_table(bob, time(), bob_chat_id, &[bob_alice_contact_id]).await?; let sent = alice .send_text(alice_chat_id, "Symmetrically encrypted message") diff --git a/src/decrypt.rs b/src/decrypt.rs index c85a17e36..b11ee7f44 100644 --- a/src/decrypt.rs +++ b/src/decrypt.rs @@ -4,21 +4,243 @@ use std::collections::HashSet; use std::io::Cursor; -use ::pgp::composed::Message; -use anyhow::Result; +use anyhow::{Context as _, Result, bail}; use mailparse::ParsedMail; +use pgp::composed::Esk; +use pgp::composed::Message; +use pgp::composed::PlainSessionKey; +use pgp::composed::SignedSecretKey; +use pgp::composed::decrypt_session_key_with_password; +use pgp::packet::SymKeyEncryptedSessionKey; +use pgp::types::Password; +use pgp::types::StringToKey; -use crate::key::{Fingerprint, SignedPublicKey}; -use crate::pgp; +use crate::chat::ChatId; +use crate::constants::Chattype; +use crate::contact::ContactId; +use crate::context::Context; +use crate::key::{Fingerprint, SignedPublicKey, load_self_secret_keyring}; +use crate::token::Namespace; -pub fn get_encrypted_pgp_message<'a>(mail: &'a ParsedMail<'a>) -> Result>> { +/// Tries to decrypt the message, +/// returning a tuple of `(decrypted message, fingerprint)`. +/// +/// If the message wasn't encrypted, returns `Ok(None)`. +/// +/// If the message was asymmetrically encrypted, returns `Ok((decrypted message, None))`. +/// +/// If the message was symmetrically encrypted, returns `Ok((decrypted message, Some(fingerprint)))`, +/// where `fingerprint` denotes which contact is allowed to send encrypted with this symmetric secret. +/// If the message is not signed by `fingerprint`, it must be dropped. +/// +/// Otherwise, Eve could send a message to Alice +/// encrypted with the symmetric secret of someone else's broadcast channel. +/// If Alice sends an answer (or read receipt), +/// then Eve would know that Alice is in the broadcast channel. +pub(crate) async fn decrypt( + context: &Context, + mail: &mailparse::ParsedMail<'_>, +) -> Result, Option)>> { + // `pgp::composed::Message` is huge (>4kb), so, make sure that it is in a Box when held over an await point + let Some(msg) = get_encrypted_pgp_message_boxed(mail)? else { + return Ok(None); + }; + let expected_sender_fingerprint: Option; + + let plain = if let Message::Encrypted { esk, .. } = &*msg + // We only allow one ESK for symmetrically encrypted messages + // to avoid dealing with messages that are encrypted to multiple symmetric keys + // or a mix of symmetric and asymmetric keys: + && let [Esk::SymKeyEncryptedSessionKey(esk)] = &esk[..] + { + check_symmetric_encryption(esk)?; + let (psk, fingerprint) = decrypt_session_key_symmetrically(context, esk) + .await + .context("decrypt_session_key_symmetrically")?; + expected_sender_fingerprint = fingerprint; + + tokio::task::spawn_blocking(move || -> Result> { + let plain = msg + .decrypt_with_session_key(psk) + .context("decrypt_with_session_key")?; + + let plain: Message<'static> = plain.decompress()?; + Ok(plain) + }) + .await?? + } else { + // Message is asymmetrically encrypted + let secret_keys: Vec = load_self_secret_keyring(context).await?; + expected_sender_fingerprint = None; + + tokio::task::spawn_blocking(move || -> Result> { + let empty_pw = Password::empty(); + let secret_keys: Vec<&SignedSecretKey> = secret_keys.iter().collect(); + let plain = msg + .decrypt_with_keys(vec![&empty_pw], secret_keys) + .context("decrypt_with_keys")?; + + let plain: Message<'static> = plain.decompress()?; + Ok(plain) + }) + .await?? + }; + + Ok(Some((plain, expected_sender_fingerprint))) +} + +async fn decrypt_session_key_symmetrically( + context: &Context, + esk: &SymKeyEncryptedSessionKey, +) -> Result<(PlainSessionKey, Option)> { + let query_only = true; + context + .sql + .call(query_only, |conn| { + // First, try decrypting using AUTH tokens from scanned QR codes, stored in the bobstate, + // because usually there will only be 1 or 2 of it, so, it should be fast + let res: Option<(PlainSessionKey, String)> = try_decrypt_with_bobstate(esk, conn)?; + if let Some((plain_session_key, fingerprint)) = res { + return Ok((plain_session_key, Some(fingerprint))); + } + + // Then, try decrypting using broadcast secrets + let res: Option<(PlainSessionKey, Option)> = + try_decrypt_with_broadcast_secret(esk, conn)?; + if let Some((plain_session_key, fingerprint)) = res { + return Ok((plain_session_key, fingerprint)); + } + + // 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 = try_decrypt_with_auth_token(esk, conn)?; + if let Some(plain_session_key) = res { + return Ok((plain_session_key, None)); + } + + bail!("Could not find symmetric secret for session key") + }) + .await +} + +fn try_decrypt_with_bobstate( + esk: &SymKeyEncryptedSessionKey, + conn: &mut rusqlite::Connection, +) -> Result> { + let mut stmt = conn.prepare("SELECT invite FROM bobstate")?; + let mut rows = stmt.query(())?; + 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 fingerprint = invite.fingerprint().hex(); + return Ok(Some((psk, fingerprint))); + } + } + Ok(None) +} + +fn try_decrypt_with_broadcast_secret( + esk: &SymKeyEncryptedSessionKey, + conn: &mut rusqlite::Connection, +) -> Result)>> { + let Some((psk, chat_id)) = try_decrypt_with_broadcast_secret_inner(esk, conn)? else { + return Ok(None); + }; + let chat_type: Chattype = + conn.query_one("SELECT type FROM chats WHERE id=?", (chat_id,), |row| { + row.get(0) + })?; + let fp: Option = if chat_type == Chattype::OutBroadcast { + // An attacker who knows the secret will also know who owns it, + // and it's easiest code-wise to just return None here. + // But we could alternatively return the self fingerprint here + None + } else if chat_type == Chattype::InBroadcast { + let contact_id: ContactId = conn + .query_one( + "SELECT contact_id FROM chats_contacts WHERE chat_id=? AND contact_id>9", + (chat_id,), + |row| row.get(0), + ) + .context("Find InBroadcast owner")?; + let fp = conn + .query_one( + "SELECT fingerprint FROM contacts WHERE id=?", + (contact_id,), + |row| row.get(0), + ) + .context("Find owner fingerprint")?; + Some(fp) + } else { + bail!("Chat {chat_id} is not a broadcast but {chat_type}") + }; + Ok(Some((psk, fp))) +} + +fn try_decrypt_with_broadcast_secret_inner( + esk: &SymKeyEncryptedSessionKey, + conn: &mut rusqlite::Connection, +) -> Result> { + let mut stmt = conn.prepare("SELECT secret, chat_id FROM broadcast_secrets")?; + let mut rows = stmt.query(())?; + while let Some(row) = rows.next()? { + let secret: String = row.get(0)?; + if let Ok(psk) = decrypt_session_key_with_password(esk, &Password::from(secret)) { + let chat_id: ChatId = row.get(1)?; + return Ok(Some((psk, chat_id))); + } + } + Ok(None) +} + +fn try_decrypt_with_auth_token( + esk: &SymKeyEncryptedSessionKey, + conn: &mut rusqlite::Connection, +) -> Result> { + // 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. + let mut stmt = conn.prepare("SELECT token FROM tokens WHERE namespc=? ORDER BY id DESC")?; + 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)) { + return Ok(Some(psk)); + } + } + Ok(None) +} + +/// Returns Ok(()) if we want to try symmetrically decrypting the message, +/// and Err with a reason if symmetric decryption should not be tried. +/// +/// A DoS attacker could send a message with a lot of encrypted session keys, +/// all of which use a very hard-to-compute string2key algorithm. +/// We would then try to decrypt all of the encrypted session keys +/// with all of the known shared secrets. +/// In order to prevent this, we do not try to symmetrically decrypt messages +/// that use a string2key algorithm other than 'Salted'. +pub(crate) fn check_symmetric_encryption(esk: &SymKeyEncryptedSessionKey) -> Result<()> { + match esk.s2k() { + Some(StringToKey::Salted { .. }) => Ok(()), + _ => bail!("unsupported string2key algorithm"), + } +} + +/// Turns a [`ParsedMail`] into [`pgp::composed::Message`]. +/// [`pgp::composed::Message`] is huge (over 4kb), +/// so, it is put on the heap using [`Box`]. +pub fn get_encrypted_pgp_message_boxed<'a>( + mail: &'a ParsedMail<'a>, +) -> Result>>> { let Some(encrypted_data_part) = get_encrypted_mime(mail) else { return Ok(None); }; let data = encrypted_data_part.get_body_raw()?; let cursor = Cursor::new(data); let (msg, _headers) = Message::from_armor(cursor)?; - Ok(Some(msg)) + Ok(Some(Box::new(msg))) } /// Returns a reference to the encrypted payload of a message. @@ -125,8 +347,10 @@ pub(crate) fn validate_detached_signature<'a, 'b>( // First part is the content, second part is the signature. let content = first_part.raw_bytes; let ret_valid_signatures = match second_part.get_body_raw() { - Ok(signature) => pgp::pk_validate(content, &signature, public_keyring_for_validate) - .unwrap_or_default(), + Ok(signature) => { + crate::pgp::pk_validate(content, &signature, public_keyring_for_validate) + .unwrap_or_default() + } Err(_) => Default::default(), }; Some((first_part, ret_valid_signatures)) diff --git a/src/mimefactory.rs b/src/mimefactory.rs index 235f12631..4151c5074 100644 --- a/src/mimefactory.rs +++ b/src/mimefactory.rs @@ -1958,7 +1958,7 @@ impl MimeFactory { } /// Stores the unprotected headers on the outer message, and renders it. -fn render_outer_message( +pub(crate) fn render_outer_message( unprotected_headers: Vec<(&'static str, HeaderType<'static>)>, outer_message: MimePart<'static>, ) -> String { @@ -1976,7 +1976,7 @@ fn render_outer_message( /// Takes the encrypted part, wraps it in a MimePart, /// and sets the appropriate Content-Type for the outer message -fn wrap_encrypted_part(encrypted: String) -> MimePart<'static> { +pub(crate) fn wrap_encrypted_part(encrypted: String) -> MimePart<'static> { // XXX: additional newline is needed // to pass filtermail at // : diff --git a/src/mimeparser.rs b/src/mimeparser.rs index a2d07fa4d..bc30e80c8 100644 --- a/src/mimeparser.rs +++ b/src/mimeparser.rs @@ -6,7 +6,7 @@ use std::path::Path; use std::str; use std::str::FromStr; -use anyhow::{Context as _, Result, bail}; +use anyhow::{Context as _, Result, bail, ensure}; use deltachat_contact_tools::{addr_cmp, addr_normalize, sanitize_bidi_characters}; use deltachat_derive::{FromSql, ToSql}; use format_flowed::unformat_flowed; @@ -18,14 +18,15 @@ use crate::authres::handle_authres; use crate::blob::BlobObject; use crate::chat::ChatId; use crate::config::Config; +use crate::constants; use crate::contact::ContactId; use crate::context::Context; -use crate::decrypt::{get_encrypted_pgp_message, validate_detached_signature}; +use crate::decrypt::{self, validate_detached_signature}; use crate::dehtml::dehtml; use crate::download::PostMsgMetadata; use crate::events::EventType; use crate::headerdef::{HeaderDef, HeaderDefMap}; -use crate::key::{self, DcKey, Fingerprint, SignedPublicKey, load_self_secret_keyring}; +use crate::key::{self, DcKey, Fingerprint, SignedPublicKey}; use crate::log::warn; use crate::message::{self, Message, MsgId, Viewtype, get_vcard_summary, set_msg_failed}; use crate::param::{Param, Params}; @@ -35,7 +36,6 @@ use crate::tools::{ get_filemeta, parse_receive_headers, smeared_time, time, truncate_msg_text, validate_id, }; use crate::{chatlist_events, location, tools}; -use crate::{constants, token}; /// Public key extracted from `Autocrypt-Gossip` /// header with associated information. @@ -363,7 +363,6 @@ impl MimeMessage { Self::remove_secured_headers(&mut headers, &mut headers_removed, encrypted); let mut from = from.context("No from in message")?; - let private_keyring = load_self_secret_keyring(context).await?; let dkim_results = handle_authres(context, &mail, &from.addr).await?; @@ -385,24 +384,12 @@ impl MimeMessage { PreMessageMode::None }; - let encrypted_pgp_message = get_encrypted_pgp_message(&mail)?; - - let secrets: Vec; - if let Some(e) = &encrypted_pgp_message - && crate::pgp::check_symmetric_encryption(e).is_ok() - { - secrets = load_shared_secrets(context).await?; - } else { - secrets = vec![]; - } - let mail_raw; // Memory location for a possible decrypted message. let decrypted_msg; // Decrypted signed OpenPGP message. + let expected_sender_fingerprint: Option; - let (mail, is_encrypted) = match tokio::task::block_in_place(|| { - encrypted_pgp_message.map(|e| crate::pgp::decrypt(e, &private_keyring, &secrets)) - }) { - Some(Ok(mut msg)) => { + let (mail, is_encrypted) = match decrypt::decrypt(context, &mail).await { + Ok(Some((mut msg, expected_sender_fp))) => { mail_raw = msg.as_data_vec().unwrap_or_default(); let decrypted_mail = mailparse::parse_mail(&mail_raw)?; @@ -429,16 +416,19 @@ impl MimeMessage { aheader_values = protected_aheader_values; } + expected_sender_fingerprint = expected_sender_fp; (Ok(decrypted_mail), true) } - None => { + Ok(None) => { mail_raw = Vec::new(); decrypted_msg = None; + expected_sender_fingerprint = None; (Ok(mail), false) } - Some(Err(err)) => { + Err(err) => { mail_raw = Vec::new(); decrypted_msg = None; + expected_sender_fingerprint = None; warn!(context, "decryption failed: {:#}", err); (Err(err), false) } @@ -552,6 +542,22 @@ impl MimeMessage { signatures.extend(signatures_detached); content }); + + if let Some(expected_sender_fingerprint) = expected_sender_fingerprint { + ensure!( + !signatures.is_empty(), + "Unsigned message is not allowed to be encrypted with this shared secret" + ); + ensure!( + signatures.len() == 1, + "Too many signatures on symm-encrypted message" + ); + ensure!( + signatures.contains_key(&expected_sender_fingerprint.parse()?), + "This sender is not allowed to encrypt with this secret key" + ); + } + if let (Ok(mail), true) = (mail, is_encrypted) { if !signatures.is_empty() { // Unsigned "Subject" mustn't be prepended to messages shown as encrypted @@ -2110,35 +2116,6 @@ impl MimeMessage { } } -/// Loads all the shared secrets -/// that will be tried to decrypt a symmetrically-encrypted message -async fn load_shared_secrets(context: &Context) -> Result> { - // First, try decrypting using the bobstate, - // because usually there will only be 1 or 2 of it, - // so, it should be fast - let mut secrets: Vec = context - .sql - .query_map_vec("SELECT invite FROM bobstate", (), |row| { - let invite: crate::securejoin::QrInvite = row.get(0)?; - Ok(invite.authcode().to_string()) - }) - .await?; - // Then, try decrypting using broadcast secrets - secrets.extend( - context - .sql - .query_map_vec("SELECT secret FROM broadcast_secrets", (), |row| { - let secret: String = row.get(0)?; - Ok(secret) - }) - .await?, - ); - // Finally, try decrypting using AUTH tokens - // There can be a lot of AUTH tokens, because a new one is generated every time a QR code is shown - secrets.extend(token::lookup_all(context, token::Namespace::Auth).await?); - Ok(secrets) -} - fn rm_legacy_display_elements(text: &str) -> String { let mut res = None; for l in text.lines() { @@ -2656,3 +2633,5 @@ async fn handle_ndn( #[cfg(test)] mod mimeparser_tests; +#[cfg(test)] +mod shared_secret_decryption_tests; diff --git a/src/mimeparser/mimeparser_tests.rs b/src/mimeparser/mimeparser_tests.rs index 8444bee7a..b3f2e6bc9 100644 --- a/src/mimeparser/mimeparser_tests.rs +++ b/src/mimeparser/mimeparser_tests.rs @@ -2171,9 +2171,6 @@ async fn test_load_shared_secrets_with_legacy_state() -> Result<()> { () ).await?; - // This call must not fail: - load_shared_secrets(alice).await.unwrap(); - let qr: QrInvite = alice .sql .query_get_value("SELECT invite FROM bobstate", ()) diff --git a/src/mimeparser/shared_secret_decryption_tests.rs b/src/mimeparser/shared_secret_decryption_tests.rs new file mode 100644 index 000000000..c5e2ab025 --- /dev/null +++ b/src/mimeparser/shared_secret_decryption_tests.rs @@ -0,0 +1,242 @@ +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::pgp; +use crate::qr::{Qr, check_qr}; +use crate::receive_imf::receive_imf; +use crate::securejoin::{get_securejoin_qr, join_securejoin}; +use crate::test_utils::{TestContext, TestContextManager}; +use anyhow::Result; + +/// Tests that the following attack isn't possible: +/// +/// Eve is subscribed to a channel and wants to know whether Alice is also subscribed to it. +/// To achieve this, Eve sends a message to Alice +/// encrypted with the symmetric secret of this broadcast channel. +/// +/// If Alice sends an answer (or read receipt), +/// then Eve knows that Alice is in the broadcast channel. +/// +/// A similar attack would be possible with auth tokens +/// that are also used to symmetrically encrypt messages. +/// +/// To defeat this, a message that was unexpectedly +/// encrypted with a symmetric secret must be dropped. +async fn test_shared_secret_decryption_ex( + recipient_ctx: &TestContext, + from_addr: &str, + secret: &str, + signer_ctx: Option<&TestContext>, + expected_error: Option<&str>, +) -> Result<()> { + let plain_body = "Hello, this is a secure message."; + let plain_text = format!("Content-Type: text/plain; charset=utf-8\r\n\r\n{plain_body}"); + let previous_highest_msg_id = get_highest_msg_id(recipient_ctx).await; + + let signer_key = if let Some(signer_ctx) = signer_ctx { + Some(load_self_secret_key(signer_ctx).await?) + } else { + None + }; + if let Some(signer_ctx) = signer_ctx { + // The recipient needs to know the signer's pubkey + // in order to be able to validate the pubkey: + 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 boundary = "boundary123"; + let rcvd_mail = format!( + "Content-Type: multipart/encrypted; protocol=\"application/pgp-encrypted\"; boundary=\"{boundary}\"\n\ + From: {from}\n\ + To: \"hidden-recipients\": ;\n\ + Subject: [...]\n\ + MIME-Version: 1.0\n\ + Message-ID: <12345@example.org>\n\ + \n\ + --{boundary}\n\ + Content-Type: application/pgp-encrypted\n\ + \n\ + Version: 1\n\ + \n\ + --{boundary}\n\ + Content-Type: application/octet-stream; name=\"encrypted.asc\"\n\ + Content-Disposition: inline; filename=\"encrypted.asc\"\n\ + \n\ + {encrypted_msg}\n\ + --{boundary}--\n", + from = from_addr, + boundary = boundary, + encrypted_msg = encrypted_msg + ); + + let rcvd = receive_imf(recipient_ctx, rcvd_mail.as_bytes(), false) + .await + .expect("If receive_imf() adds an error here, then Bob may be notified about the error and tell the attacker, leaking that he knows the secret") + .expect("A trashed message should be created, otherwise we'll unnecessarily download it again"); + + if let Some(error_pattern) = expected_error { + assert!(rcvd.chat_id == DC_CHAT_ID_TRASH); + assert_eq!( + previous_highest_msg_id, + get_highest_msg_id(recipient_ctx).await, + "receive_imf() must not add any message. Otherwise, Bob may send something about an error to the attacker, leaking that he knows the secret" + ); + let EventType::Warning(warning) = recipient_ctx + .evtracker + .get_matching(|ev| matches!(ev, EventType::Warning(_))) + .await + else { + unreachable!() + }; + assert!(warning.contains(error_pattern), "Wrong warning: {warning}"); + } else { + let msg = recipient_ctx.get_last_msg().await; + assert_eq!(&[msg.id], rcvd.msg_ids.as_slice()); + assert_eq!(msg.text, plain_body); + assert_eq!(rcvd.chat_id.is_special(), false); + } + + Ok(()) +} + +async fn get_highest_msg_id(context: &Context) -> MsgId { + context + .sql + .query_get_value( + "SELECT MAX(id) FROM msgs WHERE chat_id!=?", + (DC_CHAT_ID_TRASH,), + ) + .await + .unwrap() + .unwrap_or_default() +} + +#[tokio::test(flavor = "multi_thread", worker_threads = 2)] +async fn test_broadcast_security_attacker_signature() -> Result<()> { + let mut tcm = TestContextManager::new(); + let alice = &tcm.alice().await; + let bob = &tcm.bob().await; + let charlie = &tcm.charlie().await; // Attacker + + let alice_chat_id = create_broadcast(alice, "Channel".to_string()).await?; + let qr = get_securejoin_qr(alice, Some(alice_chat_id)).await?; + tcm.exec_securejoin_qr(bob, alice, &qr).await; + + let secret = load_broadcast_secret(alice, alice_chat_id).await?.unwrap(); + + let charlie_addr = charlie.get_config(Config::Addr).await?.unwrap(); + + test_shared_secret_decryption_ex( + bob, + &charlie_addr, + &secret, + Some(charlie), + Some("This sender is not allowed to encrypt with this secret key"), + ) + .await +} + +#[tokio::test(flavor = "multi_thread", worker_threads = 2)] +async fn test_broadcast_security_no_signature() -> Result<()> { + let mut tcm = TestContextManager::new(); + let alice = &tcm.alice().await; + let bob = &tcm.bob().await; + + let alice_chat_id = create_broadcast(alice, "Channel".to_string()).await?; + let qr = get_securejoin_qr(alice, Some(alice_chat_id)).await?; + tcm.exec_securejoin_qr(bob, alice, &qr).await; + + let secret = load_broadcast_secret(alice, alice_chat_id).await?.unwrap(); + + test_shared_secret_decryption_ex( + bob, + "attacker@example.org", + &secret, + None, + Some("Unsigned message is not allowed to be encrypted with this shared secret"), + ) + .await +} + +#[tokio::test(flavor = "multi_thread", worker_threads = 2)] +async fn test_broadcast_security_happy_path() -> Result<()> { + let mut tcm = TestContextManager::new(); + let alice = &tcm.alice().await; + let bob = &tcm.bob().await; + + let alice_chat_id = create_broadcast(alice, "Channel".to_string()).await?; + let qr = get_securejoin_qr(alice, Some(alice_chat_id)).await?; + tcm.exec_securejoin_qr(bob, alice, &qr).await; + + let secret = load_broadcast_secret(alice, alice_chat_id).await?.unwrap(); + + let alice_addr = alice + .get_config(crate::config::Config::Addr) + .await? + .unwrap(); + + test_shared_secret_decryption_ex(bob, &alice_addr, &secret, Some(alice), None).await +} + +#[tokio::test(flavor = "multi_thread", worker_threads = 2)] +async fn test_qr_code_security() -> Result<()> { + let mut tcm = TestContextManager::new(); + let alice = &tcm.alice().await; + 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 { + unreachable!() + }; + // Start a securejoin process, but don't finish it: + join_securejoin(alice, &qr).await?; + + let charlie_addr = charlie.get_config(Config::Addr).await?.unwrap(); + + test_shared_secret_decryption_ex( + alice, + &charlie_addr, + &authcode, + Some(charlie), + Some("This sender is not allowed to encrypt with this secret key"), + ) + .await +} + +#[tokio::test(flavor = "multi_thread", worker_threads = 2)] +async fn test_qr_code_happy_path() -> Result<()> { + let mut tcm = TestContextManager::new(); + let alice = &tcm.alice().await; + let bob = &tcm.bob().await; + + 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(bob, &qr).await?; + + test_shared_secret_decryption_ex(bob, "alice@example.net", &authcode, Some(alice), None).await +} + +/// Control: Test that the behavior is the same when the shared secret is unknown +#[tokio::test(flavor = "multi_thread", worker_threads = 2)] +async fn test_unknown_secret() -> Result<()> { + let mut tcm = TestContextManager::new(); + let alice = &tcm.alice().await; + let bob = &tcm.bob().await; + + test_shared_secret_decryption_ex( + bob, + "alice@example.net", + "Some secret unknown to Bob", + Some(alice), + Some("Could not find symmetric secret for session key"), + ) + .await +} diff --git a/src/pgp.rs b/src/pgp.rs index edecac816..0818a169b 100644 --- a/src/pgp.rs +++ b/src/pgp.rs @@ -3,13 +3,13 @@ use std::collections::{BTreeMap, HashMap, HashSet}; use std::io::{BufRead, Cursor}; -use anyhow::{Context as _, Result, bail}; +use anyhow::{Context as _, Result}; use deltachat_contact_tools::EmailAddress; use pgp::armor::BlockType; use pgp::composed::{ - ArmorOptions, DecryptionOptions, Deserializable, DetachedSignature, EncryptionCaps, - KeyType as PgpKeyType, Message, MessageBuilder, SecretKeyParamsBuilder, SignedPublicKey, - SignedPublicSubKey, SignedSecretKey, SubkeyParamsBuilder, SubpacketConfig, TheRing, + ArmorOptions, Deserializable, DetachedSignature, EncryptionCaps, KeyType as PgpKeyType, + Message, MessageBuilder, SecretKeyParamsBuilder, SignedPublicKey, SignedPublicSubKey, + SignedSecretKey, SubkeyParamsBuilder, SubpacketConfig, }; use pgp::crypto::aead::{AeadAlgorithm, ChunkSize}; use pgp::crypto::ecc_curve::ECCCurve; @@ -293,94 +293,6 @@ pub fn pk_calc_signature( Ok(sig.to_armored_string(ArmorOptions::default())?) } -/// Decrypts the message: -/// - with keys from the private key keyring (passed in `private_keys_for_decryption`) -/// if the message was asymmetrically encrypted, -/// - with a shared secret/password (passed in `shared_secrets`), -/// if the message was symmetrically encrypted. -/// -/// Returns the decrypted and decompressed message. -pub fn decrypt( - msg: Message<'static>, - private_keys_for_decryption: &[SignedSecretKey], - mut shared_secrets: &[String], -) -> Result> { - let skeys: Vec<&SignedSecretKey> = private_keys_for_decryption.iter().collect(); - let empty_pw = Password::empty(); - - let decrypt_options = DecryptionOptions::new(); - let symmetric_encryption_res = check_symmetric_encryption(&msg); - if symmetric_encryption_res.is_err() { - shared_secrets = &[]; - } - - // We always try out all passwords here, - // but benchmarking (see `benches/decrypting.rs`) - // showed that the performance impact is negligible. - // We can improve this in the future if necessary. - let message_password: Vec = shared_secrets - .iter() - .map(|p| Password::from(p.as_str())) - .collect(); - let message_password: Vec<&Password> = message_password.iter().collect(); - - let ring = TheRing { - secret_keys: skeys, - key_passwords: vec![&empty_pw], - message_password, - session_keys: vec![], - decrypt_options, - }; - - let res = msg.decrypt_the_ring(ring, true); - - let (msg, _ring_result) = match res { - Ok(it) => it, - Err(err) => { - if let Err(reason) = symmetric_encryption_res { - bail!("{err:#} (Note: symmetric decryption was not tried: {reason})") - } else { - bail!("{err:#}"); - } - } - }; - - // remove one layer of compression - let msg = msg.decompress()?; - - Ok(msg) -} - -/// Returns Ok(()) if we want to try symmetrically decrypting the message, -/// and Err with a reason if symmetric decryption should not be tried. -/// -/// A DOS attacker could send a message with a lot of encrypted session keys, -/// all of which use a very hard-to-compute string2key algorithm. -/// We would then try to decrypt all of the encrypted session keys -/// with all of the known shared secrets. -/// In order to prevent this, we do not try to symmetrically decrypt messages -/// that use a string2key algorithm other than 'Salted'. -pub(crate) fn check_symmetric_encryption( - msg: &Message<'_>, -) -> std::result::Result<(), &'static str> { - let Message::Encrypted { esk, .. } = msg else { - return Err("not encrypted"); - }; - - if esk.len() > 1 { - return Err("too many esks"); - } - - let [pgp::composed::Esk::SymKeyEncryptedSessionKey(esk)] = &esk[..] else { - return Err("not symmetrically encrypted"); - }; - - match esk.s2k() { - Some(StringToKey::Salted { .. }) => Ok(()), - _ => Err("unsupported string2key algorithm"), - } -} - /// Returns fingerprints /// of all keys from the `public_keys_for_validation` keyring that /// have valid signatures in `msg` and corresponding intended recipient fingerprints @@ -515,24 +427,38 @@ mod tests { use super::*; use crate::{ - key::{load_self_public_key, load_self_secret_key}, - test_utils::{TestContextManager, alice_keypair, bob_keypair}, + decrypt, + key::{load_self_public_key, load_self_secret_key, store_self_keypair}, + mimefactory::{render_outer_message, wrap_encrypted_part}, + test_utils::{TestContext, TestContextManager, alice_keypair, bob_keypair}, + token, }; use pgp::composed::Esk; use pgp::packet::PublicKeyEncryptedSessionKey; - fn decrypt_bytes( + async fn decrypt_bytes( bytes: Vec, private_keys_for_decryption: &[SignedSecretKey], shared_secrets: &[String], ) -> Result> { - let cursor = Cursor::new(bytes); - let (msg, _headers) = Message::from_armor(cursor).unwrap(); - decrypt(msg, private_keys_for_decryption, shared_secrets) + let t = &TestContext::new().await; + + for secret in shared_secrets { + token::save(t, token::Namespace::Auth, None, secret, 0).await?; + } + let [secret_key] = private_keys_for_decryption else { + panic!("Only one private key is allowed anymore"); + }; + store_self_keypair(t, secret_key).await?; + + let mime_message = wrap_encrypted_part(bytes.try_into().unwrap()); + let rendered = render_outer_message(vec![], mime_message); + let parsed = mailparse::parse_mail(rendered.as_bytes())?; + let (decrypted, _fp) = decrypt::decrypt(t, &parsed).await?.unwrap(); + Ok(decrypted) } - #[expect(clippy::type_complexity)] - fn pk_decrypt_and_validate<'a>( + async fn pk_decrypt_and_validate<'a>( ctext: &'a [u8], private_keys_for_decryption: &'a [SignedSecretKey], public_keys_for_validation: &[SignedPublicKey], @@ -541,7 +467,7 @@ mod tests { HashMap>, Vec, )> { - let mut msg = decrypt_bytes(ctext.to_vec(), private_keys_for_decryption, &[])?; + let mut msg = decrypt_bytes(ctext.to_vec(), private_keys_for_decryption, &[]).await?; let content = msg.as_data_vec()?; let ret_signature_fingerprints = valid_signature_fingerprints(&msg, public_keys_for_validation); @@ -655,6 +581,7 @@ mod tests { &decrypt_keyring, &sig_check_keyring, ) + .await .unwrap(); assert_eq!(content, CLEARTEXT); assert_eq!(valid_signatures.len(), 1); @@ -670,6 +597,7 @@ mod tests { &decrypt_keyring, &sig_check_keyring, ) + .await .unwrap(); assert_eq!(content, CLEARTEXT); assert_eq!(valid_signatures.len(), 1); @@ -682,7 +610,9 @@ mod tests { async fn test_decrypt_no_sig_check() { let keyring = vec![KEYS.alice_secret.clone()]; let (_msg, valid_signatures, content) = - pk_decrypt_and_validate(ctext_signed().await.as_bytes(), &keyring, &[]).unwrap(); + pk_decrypt_and_validate(ctext_signed().await.as_bytes(), &keyring, &[]) + .await + .unwrap(); assert_eq!(content, CLEARTEXT); assert_eq!(valid_signatures.len(), 0); } @@ -697,6 +627,7 @@ mod tests { &decrypt_keyring, &sig_check_keyring, ) + .await .unwrap(); assert_eq!(content, CLEARTEXT); assert_eq!(valid_signatures.len(), 0); @@ -707,7 +638,9 @@ mod tests { let decrypt_keyring = vec![KEYS.bob_secret.clone()]; let ctext_unsigned = include_bytes!("../test-data/message/ctext_unsigned.asc"); let (_msg, valid_signatures, content) = - pk_decrypt_and_validate(ctext_unsigned, &decrypt_keyring, &[]).unwrap(); + pk_decrypt_and_validate(ctext_unsigned, &decrypt_keyring, &[]) + .await + .unwrap(); assert_eq!(content, CLEARTEXT); assert_eq!(valid_signatures.len(), 0); } @@ -733,31 +666,65 @@ mod tests { 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 { + hash_alg: HashAlgorithm::default(), + salt: [1; 8], + }; + + test_dont_decrypt_expensive_message_ex(s2k, false, None).await + } + + #[tokio::test(flavor = "multi_thread", worker_threads = 2)] + async fn test_dont_decrypt_expensive_message_bad_s2k() -> Result<()> { + let s2k = StringToKey::new_default(&mut thread_rng()); // Default is IteratedAndSalted + + test_dont_decrypt_expensive_message_ex(s2k, false, Some("unsupported string2key algorithm")) + .await + } + + #[tokio::test(flavor = "multi_thread", worker_threads = 2)] + async fn test_dont_decrypt_expensive_message_multiple_secrets() -> Result<()> { + let s2k = StringToKey::Salted { + hash_alg: HashAlgorithm::default(), + salt: [1; 8], + }; + + // This error message is actually not great, + // but grepping for it will lead to the correct code + test_dont_decrypt_expensive_message_ex(s2k, true, Some("decrypt_with_keys: missing key")) + .await + } + /// Test that we don't try to decrypt a message /// that is symmetrically encrypted /// with an expensive string2key algorithm - #[tokio::test(flavor = "multi_thread", worker_threads = 2)] - async fn test_dont_decrypt_expensive_message() -> Result<()> { + /// or multiple shared secrets. + /// This is to prevent possible DOS attacks on the app. + async fn test_dont_decrypt_expensive_message_ex( + s2k: StringToKey, + encrypt_twice: bool, + expected_error_msg: Option<&str>, + ) -> Result<()> { let mut tcm = TestContextManager::new(); let bob = &tcm.bob().await; let plain = Vec::from(b"this is the secret message"); let shared_secret = "shared secret"; - // Create a symmetrically encrypted message - // with an IteratedAndSalted string2key algorithm: - let shared_secret_pw = Password::from(shared_secret.to_string()); let msg = MessageBuilder::from_bytes("", plain); let mut rng = thread_rng(); - let s2k = StringToKey::new_default(&mut rng); // Default is IteratedAndSalted let mut msg = msg.seipd_v2( &mut rng, @@ -765,24 +732,28 @@ mod tests { AeadAlgorithm::Ocb, ChunkSize::C8KiB, ); - msg.encrypt_with_password(&mut rng, s2k, &shared_secret_pw)?; + msg.encrypt_with_password(&mut rng, s2k.clone(), &shared_secret_pw)?; + if encrypt_twice { + msg.encrypt_with_password(&mut rng, s2k, &shared_secret_pw)?; + } let ctext = msg.to_armored_string(&mut rng, Default::default())?; // Trying to decrypt it should fail with a helpful error message: let bob_private_keyring = crate::key::load_self_secret_keyring(bob).await?; - let error = decrypt_bytes( + let res = decrypt_bytes( ctext.into(), &bob_private_keyring, &[shared_secret.to_string()], ) - .unwrap_err(); + .await; - assert_eq!( - error.to_string(), - "missing key (Note: symmetric decryption was not tried: unsupported string2key algorithm)" - ); + if let Some(expected_error_msg) = expected_error_msg { + assert_eq!(format!("{:#}", res.unwrap_err()), expected_error_msg); + } else { + res.unwrap(); + } Ok(()) } @@ -809,12 +780,11 @@ mod tests { // Trying to decrypt it should fail with an OK error message: let bob_private_keyring = crate::key::load_self_secret_keyring(bob).await?; - let error = decrypt_bytes(ctext.into(), &bob_private_keyring, &[]).unwrap_err(); + let error = decrypt_bytes(ctext.into(), &bob_private_keyring, &[]) + .await + .unwrap_err(); - assert_eq!( - error.to_string(), - "missing key (Note: symmetric decryption was not tried: not symmetrically encrypted)" - ); + assert_eq!(format!("{error:#}"), "decrypt_with_keys: missing key"); Ok(()) } diff --git a/src/sql.rs b/src/sql.rs index 48cdb1b91..7e8b887ec 100644 --- a/src/sql.rs +++ b/src/sql.rs @@ -302,7 +302,7 @@ impl Sql { /// otherwise allocates write connection. /// /// Returns the result of the function. - async fn call<'a, F, R>(&'a self, query_only: bool, function: F) -> Result + pub async fn call<'a, F, R>(&'a self, query_only: bool, function: F) -> Result where F: 'a + FnOnce(&mut Connection) -> Result + Send, R: Send + 'static, diff --git a/src/token.rs b/src/token.rs index 9f9abe343..26db09c2e 100644 --- a/src/token.rs +++ b/src/token.rs @@ -66,22 +66,6 @@ pub async fn lookup( .await } -/// Looks up all tokens from the given namespace, -/// so that they can be used for decrypting a symmetrically-encrypted message. -/// -/// The most-recently saved tokens are returned first. -/// This improves performance when Bob scans a QR code that was just created. -pub async fn lookup_all(context: &Context, namespace: Namespace) -> Result> { - context - .sql - .query_map_vec( - "SELECT token FROM tokens WHERE namespc=? ORDER BY id DESC", - (namespace,), - |row| Ok(row.get(0)?), - ) - .await -} - pub async fn lookup_or_new( context: &Context, namespace: Namespace,