From 4bf08b6051c4e59a5e3f5996f771da08b82ea3ad Mon Sep 17 00:00:00 2001 From: Hocuri Date: Sat, 3 Feb 2024 16:49:16 +0100 Subject: [PATCH] fix: Mark the gossip keys from the message as verified, not the ones from the db Previously, `set_verified()` loaded the gossip key from the database and then set it as verified. Now, if, for some reason, the database doesn't contain the key that was just gossiped with the incoming message, then we'll mark the wrong key as verified ("gossip key injection"). Instead, remember the `SignedPublicKey`s in the `MimeMessage` and directly mark those as verified. --- src/mimeparser.rs | 21 ++++++++-------- src/peerstate.rs | 61 +++++++++++++++++----------------------------- src/receive_imf.rs | 10 ++++---- src/securejoin.rs | 25 +++++++++++-------- 4 files changed, 54 insertions(+), 63 deletions(-) diff --git a/src/mimeparser.rs b/src/mimeparser.rs index 36d38277ff..967500f15d 100644 --- a/src/mimeparser.rs +++ b/src/mimeparser.rs @@ -82,9 +82,10 @@ pub(crate) struct MimeMessage { /// If a message is not encrypted or the signature is not valid, /// this set is empty. pub signatures: HashSet, - /// The set of mail recipient addresses for which gossip headers were applied, regardless of - /// whether they modified any peerstates. - pub gossiped_addr: HashSet, + /// The mail recipient addresses for which gossip headers were applied + /// and their respective gossiped keys, + /// regardless of whether they modified any peerstates. + pub gossiped_keys: HashMap, /// True if the message is a forwarded message. pub is_forwarded: bool, @@ -282,7 +283,7 @@ impl MimeMessage { // Memory location for a possible decrypted message. let mut mail_raw = Vec::new(); - let mut gossiped_addr = Default::default(); + let mut gossiped_keys = Default::default(); let mut from_is_signed = false; hop_info += "\n\n"; hop_info += &decryption_info.dkim_results.to_string(); @@ -322,7 +323,7 @@ impl MimeMessage { // but only if the mail was correctly signed. Probably it's ok to not require // encryption here, but let's follow the standard. let gossip_headers = mail.headers.get_all_values("Autocrypt-Gossip"); - gossiped_addr = update_gossip_peerstates( + gossiped_keys = update_gossip_peerstates( context, message_time, &from.addr, @@ -413,7 +414,7 @@ impl MimeMessage { // only non-empty if it was a valid autocrypt message signatures, - gossiped_addr, + gossiped_keys, is_forwarded: false, mdn_reports: Vec::new(), is_system_message: SystemMessage::Unknown, @@ -1728,9 +1729,9 @@ async fn update_gossip_peerstates( from: &str, recipients: &[SingleInfo], gossip_headers: Vec, -) -> Result> { +) -> Result> { // XXX split the parsing from the modification part - let mut gossiped_addr: HashSet = Default::default(); + let mut gossiped_keys: HashMap = Default::default(); for value in &gossip_headers { let header = match value.parse::() { @@ -1774,10 +1775,10 @@ async fn update_gossip_peerstates( .handle_fingerprint_change(context, message_time) .await?; - gossiped_addr.insert(header.addr.to_lowercase()); + gossiped_keys.insert(header.addr.to_lowercase(), header.public_key); } - Ok(gossiped_addr) + Ok(gossiped_keys) } /// Message Disposition Notification (RFC 8098) diff --git a/src/peerstate.rs b/src/peerstate.rs index 916b6852a1..a8b9909474 100644 --- a/src/peerstate.rs +++ b/src/peerstate.rs @@ -457,59 +457,44 @@ impl Peerstate { Ok(backward_verified) } - /// Set this peerstate to verified - /// Make sure to call `self.save_to_db` to save these changes + /// Set this peerstate to verified; + /// make sure to call `self.save_to_db` to save these changes. + /// /// Params: - /// verifier: + /// + /// * key: The new verified key. + /// * fingerprint: Only set to verified if the key's fingerprint matches this. + /// * verifier: /// The address which introduces the given contact. /// If we are verifying the contact, use that contacts address. pub fn set_verified( &mut self, - which_key: PeerstateKeyType, + key: SignedPublicKey, fingerprint: Fingerprint, verifier: String, ) -> Result<()> { - match which_key { - PeerstateKeyType::PublicKey => { - if self.public_key_fingerprint.is_some() - && self.public_key_fingerprint.as_ref().unwrap() == &fingerprint - { - self.verified_key = self.public_key.clone(); - self.verified_key_fingerprint = Some(fingerprint); - self.verifier = Some(verifier); - Ok(()) - } else { - Err(Error::msg(format!( - "{fingerprint} is not peer's public key fingerprint", - ))) - } - } - PeerstateKeyType::GossipKey => { - if self.gossip_key_fingerprint.is_some() - && self.gossip_key_fingerprint.as_ref().unwrap() == &fingerprint - { - self.verified_key = self.gossip_key.clone(); - self.verified_key_fingerprint = Some(fingerprint); - self.verifier = Some(verifier); - Ok(()) - } else { - Err(Error::msg(format!( - "{fingerprint} is not peer's gossip key fingerprint", - ))) - } - } + if key.fingerprint() == fingerprint { + self.verified_key = Some(key); + self.verified_key_fingerprint = Some(fingerprint); + self.verifier = Some(verifier); + Ok(()) + } else { + Err(Error::msg(format!( + "{fingerprint} is not peer's key fingerprint", + ))) } } - /// Sets current gossiped key as the secondary verified key. + /// Sets the gossiped key as the secondary verified key. /// /// If gossiped key is the same as the current verified key, /// do nothing to avoid overwriting secondary verified key /// which may be different. - pub fn set_secondary_verified_key_from_gossip(&mut self, verifier: String) { - if self.gossip_key_fingerprint != self.verified_key_fingerprint { - self.secondary_verified_key = self.gossip_key.clone(); - self.secondary_verified_key_fingerprint = self.gossip_key_fingerprint.clone(); + pub fn set_secondary_verified_key(&mut self, gossip_key: SignedPublicKey, verifier: String) { + let fingerprint = gossip_key.fingerprint(); + if self.verified_key_fingerprint.as_ref() != Some(&fingerprint) { + self.secondary_verified_key = Some(gossip_key); + self.secondary_verified_key_fingerprint = Some(fingerprint); self.secondary_verifier = Some(verifier); } } diff --git a/src/receive_imf.rs b/src/receive_imf.rs index c0b3c12890..e2cc72dde1 100644 --- a/src/receive_imf.rs +++ b/src/receive_imf.rs @@ -31,7 +31,7 @@ use crate::message::{ }; use crate::mimeparser::{parse_message_ids, AvatarAction, MimeMessage, SystemMessage}; use crate::param::{Param, Params}; -use crate::peerstate::{Peerstate, PeerstateKeyType}; +use crate::peerstate::Peerstate; use crate::reaction::{set_msg_reaction, Reaction}; use crate::securejoin::{self, handle_securejoin_handshake, observe_securejoin_on_other_device}; use crate::simplify; @@ -440,7 +440,7 @@ pub(crate) async fn receive_imf_inner( && mime_parser .recipients .iter() - .all(|recipient| mime_parser.gossiped_addr.contains(&recipient.addr)) + .all(|recipient| mime_parser.gossiped_keys.contains_key(&recipient.addr)) { info!( context, @@ -2572,7 +2572,7 @@ async fn mark_recipients_as_verified( for (to_addr, is_verified) in rows { // mark gossiped keys (if any) as verified - if mimeparser.gossiped_addr.contains(&to_addr.to_lowercase()) { + if let Some(gossiped_key) = mimeparser.gossiped_keys.get(&to_addr.to_lowercase()) { if let Some(mut peerstate) = Peerstate::from_addr(context, &to_addr).await? { // If we're here, we know the gossip key is verified. // @@ -2587,7 +2587,7 @@ async fn mark_recipients_as_verified( if !is_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.set_verified(gossiped_key.clone(), 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?; @@ -2609,7 +2609,7 @@ async fn mark_recipients_as_verified( } else { // The contact already has a verified key. // Store gossiped key as the secondary verified key. - peerstate.set_secondary_verified_key_from_gossip(verifier_addr); + peerstate.set_secondary_verified_key(gossiped_key.clone(), verifier_addr); peerstate.save_to_db(&context.sql).await?; } } diff --git a/src/securejoin.rs b/src/securejoin.rs index 26260a63d0..06480a437e 100644 --- a/src/securejoin.rs +++ b/src/securejoin.rs @@ -18,7 +18,7 @@ use crate::key::{load_self_public_key, DcKey, Fingerprint}; use crate::message::{Message, Viewtype}; use crate::mimeparser::{MimeMessage, SystemMessage}; use crate::param::Param; -use crate::peerstate::{Peerstate, PeerstateKeyType}; +use crate::peerstate::Peerstate; use crate::qr::check_qr; use crate::securejoin::bob::JoinerProgress; use crate::stock_str; @@ -229,11 +229,13 @@ async fn verify_sender_by_fingerprint( .filter(|&fp| fp == fingerprint) .is_some() { - let verifier = contact.get_addr().to_owned(); - peerstate.set_verified(PeerstateKeyType::PublicKey, fingerprint.clone(), verifier)?; - peerstate.prefer_encrypt = EncryptPreference::Mutual; - peerstate.save_to_db(&context.sql).await?; - return Ok(true); + if let Some(public_key) = &peerstate.public_key { + let verifier = contact.get_addr().to_owned(); + peerstate.set_verified(public_key.clone(), fingerprint.clone(), verifier)?; + peerstate.prefer_encrypt = EncryptPreference::Mutual; + peerstate.save_to_db(&context.sql).await?; + return Ok(true); + } } } @@ -599,7 +601,7 @@ pub(crate) async fn observe_securejoin_on_other_device( .get_addr() .to_lowercase(); - if !mime_message.gossiped_addr.contains(&addr) { + let Some(key) = mime_message.gossiped_keys.get(&addr) else { could_not_establish_secure_connection( context, contact_id, @@ -612,7 +614,7 @@ pub(crate) async fn observe_securejoin_on_other_device( ) .await?; return Ok(HandshakeMessage::Ignore); - } + }; let Some(mut peerstate) = Peerstate::from_addr(context, &addr).await? else { could_not_establish_secure_connection( @@ -638,7 +640,7 @@ pub(crate) async fn observe_securejoin_on_other_device( .await?; return Ok(HandshakeMessage::Ignore); }; - peerstate.set_verified(PeerstateKeyType::GossipKey, fingerprint, addr)?; + peerstate.set_verified(key.clone(), fingerprint, addr)?; peerstate.prefer_encrypt = EncryptPreference::Mutual; peerstate.save_to_db(&context.sql).await?; @@ -714,7 +716,10 @@ async fn mark_peer_as_verified( let Some(ref mut peerstate) = Peerstate::from_fingerprint(context, &fingerprint).await? else { return Ok(false); }; - peerstate.set_verified(PeerstateKeyType::PublicKey, fingerprint, verifier)?; + let Some(ref public_key) = peerstate.public_key else { + return Ok(false); + }; + peerstate.set_verified(public_key.clone(), fingerprint, verifier)?; peerstate.prefer_encrypt = EncryptPreference::Mutual; if backward_verified { peerstate.backward_verified_key_id =