Skip to content

fix: Mark the gossip keys from the message as verified, not the ones from the db #5247

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
Hocuri merged 1 commit into from
Feb 6, 2024
Merged

fix: Mark the gossip keys from the message as verified, not the ones from the db #5247

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
21 changes: 11 additions & 10 deletions src/mimeparser.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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<Fingerprint>,
/// The set of mail recipient addresses for which gossip headers were applied, regardless of
/// whether they modified any peerstates.
pub gossiped_addr: HashSet<String>,
/// 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<String, SignedPublicKey>,

/// True if the message is a forwarded message.
pub is_forwarded: bool,
Expand Down Expand Up @@ -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();
Expand Down Expand Up @@ -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,
Expand Down Expand Up @@ -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,
Expand Down Expand Up @@ -1728,9 +1729,9 @@ async fn update_gossip_peerstates(
from: &str,
recipients: &[SingleInfo],
gossip_headers: Vec<String>,
) -> Result<HashSet<String>> {
) -> Result<HashMap<String, SignedPublicKey>> {
// XXX split the parsing from the modification part
let mut gossiped_addr: HashSet<String> = Default::default();
let mut gossiped_keys: HashMap<String, SignedPublicKey> = Default::default();

for value in &gossip_headers {
let header = match value.parse::<Aheader>() {
Expand Down Expand Up @@ -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)
Expand Down
61 changes: 23 additions & 38 deletions src/peerstate.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Copy link
Collaborator Author

@Hocuri Hocuri Feb 3, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not completely sure whether we still need the fingerprint parameter. We could also remove it and unconditionally set the verified_key - I can't come up with a scenario where this would be a problem, but also I'm not sure I thought of every possible scenario.

Copy link
Collaborator

@iequidoo iequidoo Feb 3, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's still needed -- see the verify_sender_by_fingerprint() call for Alice in the Steps 5+6 in "Setup verified contact" protocol section, in that case the fingerprint must be taken from the Secure-Join-Fingerprint header

Copy link
Collaborator Author

@Hocuri Hocuri Feb 4, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since verify_sender_by_fingerprint() first loads the key with this fingerprint and only then calls set_verified(), I don't think that this particular usage of set_verified() requires the fingerprint parameter?

Copy link
Collaborator

@iequidoo iequidoo Feb 4, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, i didn't noticed -- verify_sender_by_fingerprint() already checks the fingerprint on its own:

.filter(|&fp| fp == fingerprint)

Looks like this parameter indeed can be removed, but mark_peer_as_verified() must check then that the fingerprint is a public key fingerprint of the peer.

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);
}
}
Expand Down
10 changes: 5 additions & 5 deletions src/receive_imf.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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,
Expand Down Expand Up @@ -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.
//
Expand All @@ -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?;
Expand All @@ -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?;
}
}
Expand Down
25 changes: 15 additions & 10 deletions src/securejoin.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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);
}
}
}

Expand Down Expand Up @@ -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,
Expand All @@ -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(
Expand All @@ -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?;

Expand Down Expand Up @@ -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 =
Expand Down