diff --git a/CHANGELOG.md b/CHANGELOG.md index 2ba5fd561..b5d78e1c3 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,8 +7,10 @@ - Remove upper limit on the attachment size. #4253 - Update rPGP to 0.10.1. #4236 - Compress `mime_headers` column with HTML emails stored in database +- Strip BIDI characters in system messages, files, group names and contact names #3479 - maybe_add_time_based_warnings(): Use release date instead of the provider DB update one + ### Fixes - Fix python bindings README documentation on installing the bindings from source. - Show a warning if quota list is empty #4261 @@ -219,7 +221,6 @@ - Do not treat invalid email addresses as an exception #3942 - Add timeouts to HTTP requests #3948 - ## 1.105.0 ### Changes @@ -305,7 +306,6 @@ - Disable read timeout during IMAP IDLE #3826 - Bots automatically accept mailing lists #3831 - ## 1.102.0 ### Changes diff --git a/src/chat.rs b/src/chat.rs index 8e505d32d..ef7c3b024 100644 --- a/src/chat.rs +++ b/src/chat.rs @@ -36,8 +36,8 @@ use crate::smtp::send_msg_to_smtp; use crate::stock_str; use crate::tools::{ buf_compress, create_id, create_outgoing_rfc724_mid, create_smeared_timestamp, - create_smeared_timestamps, get_abs_path, gm2local_offset, improve_single_line_input, time, - IsNoneOrEmpty, + create_smeared_timestamps, get_abs_path, gm2local_offset, improve_single_line_input, + strip_rtlo_characters, time, IsNoneOrEmpty, }; use crate::webxdc::WEBXDC_SUFFIX; use crate::{location, sql}; @@ -269,6 +269,7 @@ impl ChatId { create_protected: ProtectionStatus, param: Option, ) -> Result { + let grpname = strip_rtlo_characters(grpname); let row_id = context.sql.insert( "INSERT INTO chats (type, name, grpid, blocked, created_timestamp, protected, param) VALUES(?, ?, ?, ?, ?, ?, ?);", @@ -2208,6 +2209,13 @@ pub async fn send_msg_sync(context: &Context, chat_id: ChatId, msg: &mut Message } async fn send_msg_inner(context: &Context, chat_id: ChatId, msg: &mut Message) -> Result { + // protect all system messages againts RTLO attacks + if msg.is_system_message() { + if let Some(text) = &msg.text { + msg.text = Some(strip_rtlo_characters(text.as_ref())); + } + } + if prepare_send_msg(context, chat_id, msg).await?.is_some() { context.emit_msgs_changed(msg.chat_id, msg.id); @@ -3808,6 +3816,7 @@ mod tests { use crate::message::delete_msgs; use crate::receive_imf::receive_imf; use crate::test_utils::TestContext; + use tokio::fs; #[tokio::test(flavor = "multi_thread", worker_threads = 2)] async fn test_chat_info() { @@ -6107,4 +6116,30 @@ mod tests { Ok(()) } + + #[tokio::test(flavor = "multi_thread", worker_threads = 2)] + async fn test_blob_renaming() -> Result<()> { + let alice = TestContext::new_alice().await; + let bob = TestContext::new_bob().await; + let chat_id = create_group_chat(&alice, ProtectionStatus::Unprotected, "Group").await?; + add_contact_to_chat( + &alice, + chat_id, + Contact::create(&alice, "bob", "bob@example.net").await?, + ) + .await?; + let dir = tempfile::tempdir()?; + let file = dir.path().join("harmless_file.\u{202e}txt.exe"); + fs::write(&file, "aaa").await?; + let mut msg = Message::new(Viewtype::File); + msg.set_file(file.to_str().unwrap(), None); + let msg = bob.recv_msg(&alice.send_msg(chat_id, &mut msg).await).await; + + // the file bob receives should not contain BIDI-control characters + assert_eq!( + Some("$BLOBDIR/harmless_file.txt.exe"), + msg.param.get(Param::File), + ); + Ok(()) + } } diff --git a/src/contact.rs b/src/contact.rs index 07430d308..c3ef90283 100644 --- a/src/contact.rs +++ b/src/contact.rs @@ -32,7 +32,10 @@ use crate::mimeparser::AvatarAction; use crate::param::{Param, Params}; use crate::peerstate::{Peerstate, PeerstateVerifiedStatus}; use crate::sql::{self, params_iter}; -use crate::tools::{duration_to_str, get_abs_path, improve_single_line_input, time, EmailAddress}; +use crate::tools::{ + duration_to_str, get_abs_path, improve_single_line_input, strip_rtlo_characters, time, + EmailAddress, +}; use crate::{chat, stock_str}; /// Time during which a contact is considered as seen recently. @@ -536,7 +539,7 @@ impl Contact { return Ok((ContactId::SELF, sth_modified)); } - let mut name = name; + let mut name = strip_rtlo_characters(name); #[allow(clippy::collapsible_if)] if origin <= Origin::OutgoingTo { // The user may accidentally have written to a "noreply" address with another MUA: @@ -551,7 +554,7 @@ impl Contact { // For these kind of email addresses, sender and address often don't belong together // (like hocuri ). In this example, hocuri shouldn't // be saved as the displayname for notifications@github.com. - name = ""; + name = "".to_string(); } } @@ -1291,18 +1294,20 @@ fn sanitize_name_and_addr(name: &str, addr: &str) -> (String, String) { if let Some(captures) = ADDR_WITH_NAME_REGEX.captures(addr.as_ref()) { ( if name.is_empty() { - captures - .get(1) - .map_or("".to_string(), |m| normalize_name(m.as_str())) + strip_rtlo_characters( + &captures + .get(1) + .map_or("".to_string(), |m| normalize_name(m.as_str())), + ) } else { - name.to_string() + strip_rtlo_characters(name) }, captures .get(2) .map_or("".to_string(), |m| m.as_str().to_string()), ) } else { - (name.to_string(), addr.to_string()) + (strip_rtlo_characters(name), addr.to_string()) } } @@ -1489,7 +1494,7 @@ pub fn normalize_name(full_name: &str) -> String { match full_name.as_bytes() { [b'\'', .., b'\''] | [b'\"', .., b'\"'] | [b'<', .., b'>'] => full_name .get(1..full_name.len() - 1) - .map_or("".to_string(), |s| s.trim().into()), + .map_or("".to_string(), |s| s.trim().to_string()), _ => full_name.to_string(), } } diff --git a/src/mimeparser.rs b/src/mimeparser.rs index de5009ce4..f19e3af57 100644 --- a/src/mimeparser.rs +++ b/src/mimeparser.rs @@ -33,7 +33,7 @@ use crate::peerstate::Peerstate; use crate::simplify::{simplify, SimplifiedText}; use crate::stock_str; use crate::sync::SyncItems; -use crate::tools::{get_filemeta, parse_receive_headers, truncate_by_lines}; +use crate::tools::{get_filemeta, parse_receive_headers, strip_rtlo_characters, truncate_by_lines}; use crate::{location, tools}; /// A parsed MIME message. @@ -1952,6 +1952,8 @@ fn get_attachment_filename( }; } + let desired_filename = desired_filename.map(|filename| strip_rtlo_characters(&filename)); + Ok(desired_filename) } diff --git a/src/receive_imf.rs b/src/receive_imf.rs index 01aaa4faa..8ee1b324c 100644 --- a/src/receive_imf.rs +++ b/src/receive_imf.rs @@ -37,7 +37,9 @@ use crate::reaction::{set_msg_reaction, Reaction}; use crate::securejoin::{self, handle_securejoin_handshake, observe_securejoin_on_other_device}; use crate::sql; use crate::stock_str; -use crate::tools::{buf_compress, extract_grpid_from_rfc724_mid, smeared_time}; +use crate::tools::{ + buf_compress, extract_grpid_from_rfc724_mid, smeared_time, strip_rtlo_characters, +}; use crate::{contact, imap}; /// This is the struct that is returned after receiving one email (aka MIME message). @@ -1077,7 +1079,7 @@ async fn add_parts( let mut created_db_entries = Vec::with_capacity(mime_parser.parts.len()); - for part in &mime_parser.parts { + for part in &mut mime_parser.parts { if part.is_reaction { set_msg_reaction( context, @@ -1093,6 +1095,7 @@ async fn add_parts( if is_system_message != SystemMessage::Unknown { param.set_int(Param::Cmd, is_system_message as i32); } + if let Some(replace_msg_id) = replace_msg_id { let placeholder = Message::load_from_db(context, replace_msg_id).await?; for key in [ @@ -1681,7 +1684,7 @@ async fn apply_group_changes( .sql .execute( "UPDATE chats SET name=? WHERE id=?;", - paramsv![grpname.to_string(), chat_id], + paramsv![strip_rtlo_characters(grpname), chat_id], ) .await?; send_event_chat_modified = true; diff --git a/src/tools.rs b/src/tools.rs index 71874cf83..23b66c934 100644 --- a/src/tools.rs +++ b/src/tools.rs @@ -560,9 +560,11 @@ impl rusqlite::types::ToSql for EmailAddress { } } -/// Makes sure that a user input that is not supposed to contain newlines does not contain newlines. +/// Sanitizes user input +/// - strip newlines +/// - strip malicious bidi characters pub(crate) fn improve_single_line_input(input: &str) -> String { - input.replace(['\n', '\r'], " ").trim().to_string() + strip_rtlo_characters(input.replace(['\n', '\r'], " ").trim()) } pub(crate) trait IsNoneOrEmpty { @@ -701,6 +703,13 @@ pub(crate) fn buf_decompress(buf: &[u8]) -> Result> { Ok(mem::take(decompressor.get_mut())) } +const RTLO_CHARACTERS: [char; 5] = ['\u{202A}', '\u{202B}', '\u{202C}', '\u{202D}', '\u{202E}']; +/// This method strips all occurances of the RTLO Unicode character. +/// [Why is this needed](https://github.com/deltachat/deltachat-core-rust/issues/3479)? +pub(crate) fn strip_rtlo_characters(input_str: &str) -> String { + input_str.replace(|char| RTLO_CHARACTERS.contains(&char), "") +} + #[cfg(test)] mod tests { #![allow(clippy::indexing_slicing)] diff --git a/src/webxdc.rs b/src/webxdc.rs index 07d7da54a..d1c92b350 100644 --- a/src/webxdc.rs +++ b/src/webxdc.rs @@ -20,6 +20,7 @@ use crate::mimeparser::SystemMessage; use crate::param::Param; use crate::param::Params; use crate::scheduler::InterruptInfo; +use crate::tools::strip_rtlo_characters; use crate::tools::{create_smeared_timestamp, get_abs_path}; use crate::{chat, EventType}; @@ -293,13 +294,13 @@ impl Context { can_info_msg: bool, from_id: ContactId, ) -> Result { - let update_str = update_str.trim(); + let update_str = strip_rtlo_characters(update_str.trim()); if update_str.is_empty() { bail!("create_status_update_record: empty update."); } let status_update_item: StatusUpdateItem = - if let Ok(item) = serde_json::from_str::(update_str) { + if let Ok(item) = serde_json::from_str::(&update_str) { item } else { bail!("create_status_update_record: no valid update item."); @@ -351,7 +352,9 @@ impl Context { .param .update_timestamp(Param::WebxdcSummaryTimestamp, timestamp)? { - instance.param.set(Param::WebxdcSummary, summary); + instance + .param + .set(Param::WebxdcSummary, strip_rtlo_characters(summary)); param_changed = true; } }