Protect against RTLO attacks (#3609)

Protect against RTLO attackts

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

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,
+    create_smeared_timestamps, get_abs_path, gm2local_offset, improve_single_line_input,
-    IsNoneOrEmpty,
+    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<String>,
     ) -> Result<Self> {
+        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<MsgId> {
+    // 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(())
+    }
 }

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 <notifications@github.com>). 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
+                strip_rtlo_characters(
-                    .get(1)
+                    &captures
-                    .map_or("".to_string(), |m| normalize_name(m.as_str()))
+                        .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(),
     }
 }

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)
 }
 

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;

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<T> {
@@ -701,6 +703,13 @@ pub(crate) fn buf_decompress(buf: &[u8]) -> Result<Vec<u8>> {
     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)]

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<StatusUpdateSerial> {
-        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::<StatusUpdateItem>(update_str) {
+            if let Ok(item) = serde_json::from_str::<StatusUpdateItem>(&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;
             }
         }