Correctly sanitize input everywhere

deltachat-contact-tools/src/lib.rs

@@ -22,7 +22,8 @@
     clippy::bool_assert_comparison,
     clippy::manual_split_once,
     clippy::format_push_string,
-    clippy::bool_to_int_with_if
+    clippy::bool_to_int_with_if,
+    clippy::manual_range_contains
 )]
 
 use std::fmt;
@@ -36,7 +37,6 @@ use once_cell::sync::Lazy;
 use regex::Regex;
 
 // TODOs to clean up:
-// - Check if sanitizing is done correctly everywhere
 // - Apply lints everywhere (https://doc.rust-lang.org/cargo/reference/workspaces.html#the-lints-table)
 
 #[derive(Debug)]
@@ -263,27 +263,27 @@ impl rusqlite::types::ToSql for ContactAddress {
     }
 }
 
-/// Make the name and address
+/// Takes a name and an address and sanitizes them:
+/// - Extracts a name from the addr if the addr is in form "Alice <alice@example.org>"
+/// - Removes special characters from the name, see [`sanitize_name()`]
+/// - Removes the name if it is equal to the address by setting it to ""
 pub fn sanitize_name_and_addr(name: &str, addr: &str) -> (String, String) {
     static ADDR_WITH_NAME_REGEX: Lazy<Regex> = Lazy::new(|| Regex::new("(.*)<(.*)>").unwrap());
     let (name, addr) = if let Some(captures) = ADDR_WITH_NAME_REGEX.captures(addr.as_ref()) {
         (
             if name.is_empty() {
-                strip_rtlo_characters(captures.get(1).map_or("", |m| m.as_str()))
+                captures.get(1).map_or("", |m| m.as_str())
             } else {
-                strip_rtlo_characters(name)
+                name
             },
             captures
                 .get(2)
                 .map_or("".to_string(), |m| m.as_str().to_string()),
         )
     } else {
-        (
-            strip_rtlo_characters(&normalize_name(name)),
-            addr.to_string(),
-        )
+        (name, addr.to_string())
     };
-    let mut name = normalize_name(&name);
+    let mut name = sanitize_name(name);
 
     // If the 'display name' is just the address, remove it:
     // Otherwise, the contact would sometimes be shown as "alice@example.com (alice@example.com)" (see `get_name_n_addr()`).
@@ -295,31 +295,77 @@ pub fn sanitize_name_and_addr(name: &str, addr: &str) -> (String, String) {
     (name, addr)
 }
 
-/// Normalize a name.
+/// Sanitizes a name.
 ///
-/// - Remove quotes (come from some bad MUA implementations)
-/// - Trims the resulting string
-///
-/// Typically, this function is not needed as it is called implicitly by `Contact::add_address_book`.
-pub fn normalize_name(full_name: &str) -> String {
-    let full_name = full_name.trim();
-    if full_name.is_empty() {
-        return full_name.into();
-    }
-
-    match full_name.as_bytes() {
-        [b'\'', .., b'\''] | [b'\"', .., b'\"'] | [b'<', .., b'>'] => full_name
-            .get(1..full_name.len() - 1)
+/// - Removes newlines and trims the string
+/// - Removes quotes (come from some bad MUA implementations)
+/// - Removes potentially-malicious bidi characters
+pub fn sanitize_name(name: &str) -> String {
+    let name = sanitize_single_line(name);
+
+    match name.as_bytes() {
+        [b'\'', .., b'\''] | [b'\"', .., b'\"'] | [b'<', .., b'>'] => name
+            .get(1..name.len() - 1)
             .map_or("".to_string(), |s| s.trim().to_string()),
-        _ => full_name.to_string(),
+        _ => name.to_string(),
     }
 }
 
+/// Sanitizes user input
+///
+/// - Removes newlines and trims the string
+/// - Removes potentially-malicious bidi characters
+pub fn sanitize_single_line(input: &str) -> String {
+    sanitize_bidi_characters(input.replace(['\n', '\r'], " ").trim())
+}
+
 const RTLO_CHARACTERS: [char; 5] = ['\u{202A}', '\u{202B}', '\u{202C}', '\u{202D}', '\u{202E}'];
-/// This method strips all occurrences of the RTLO Unicode character.
-/// [Why is this needed](https://github.com/deltachat/deltachat-core-rust/issues/3479)?
-pub fn strip_rtlo_characters(input_str: &str) -> String {
-    input_str.replace(|char| RTLO_CHARACTERS.contains(&char), "")
+const ISOLATE_CHARACTERS: [char; 3] = ['\u{2066}', '\u{2067}', '\u{2068}'];
+const POP_ISOLATE_CHARACTER: char = '\u{2069}';
+/// Some control unicode characters can influence whether adjacent text is shown from
+/// left to right or from right to left.
+///
+/// Since user input is not supposed to influence how adjacent text looks,
+/// this function removes some of these characters.
+///
+/// Also see https://github.com/deltachat/deltachat-core-rust/issues/3479.
+pub fn sanitize_bidi_characters(input_str: &str) -> String {
+    // RTLO_CHARACTERS are apparently rarely used in practice.
+    // They can impact all following text, so, better remove them all:
+    let input_str = input_str.replace(|char| RTLO_CHARACTERS.contains(&char), "");
+
+    // If the ISOLATE characters are not ended with a POP DIRECTIONAL ISOLATE character,
+    // we regard the input as potentially malicious and simply remove all ISOLATE characters.
+    // See https://en.wikipedia.org/wiki/Bidirectional_text#Unicode_bidi_support
+    // and https://www.w3.org/International/questions/qa-bidi-unicode-controls.en
+    // for an explanation about ISOLATE characters.
+    fn isolate_characters_are_valid(input_str: &str) -> bool {
+        let mut isolate_character_nesting: i32 = 0;
+        for char in input_str.chars() {
+            if ISOLATE_CHARACTERS.contains(&char) {
+                isolate_character_nesting += 1;
+            } else if char == POP_ISOLATE_CHARACTER {
+                isolate_character_nesting -= 1;
+            }
+
+            // According to Wikipedia, 125 levels are allowed:
+            // https://en.wikipedia.org/wiki/Unicode_control_characters
+            // (although, in practice, we could also significantly lower this number)
+            if isolate_character_nesting < 0 || isolate_character_nesting > 125 {
+                return false;
+            }
+        }
+        isolate_character_nesting == 0
+    }
+
+    if isolate_characters_are_valid(&input_str) {
+        input_str
+    } else {
+        input_str.replace(
+            |char| ISOLATE_CHARACTERS.contains(&char) || POP_ISOLATE_CHARACTER == char,
+            "",
+        )
+    }
 }
 
 /// Returns false if addr is an invalid address, otherwise true.
@@ -668,4 +714,64 @@ END:VCARD
             assert_eq!(contacts[0].profile_image.as_deref().unwrap(), "/9j/4AAQSkZJRgABAQAAAQABAAD/4gIoSUNDX1BST0ZJTEUAAQEAAAIYAAAAAAQwAABtbnRyUkdCIFhZWiAAAAAAAAAAAAAAAABhY3NwAAAAAAAAAAAAAAAAL8bRuAJYoZUYrI4ZY3VWwxw4Ay28AAGBISScmf/2Q==");
         }
     }
+
+    #[test]
+    fn test_sanitize_name() {
+        assert_eq!(&sanitize_name(" hello world   "), "hello world");
+        assert_eq!(&sanitize_name("<"), "<");
+        assert_eq!(&sanitize_name(">"), ">");
+        assert_eq!(&sanitize_name("'"), "'");
+        assert_eq!(&sanitize_name("\""), "\"");
+    }
+
+    #[test]
+    fn test_sanitize_single_line() {
+        assert_eq!(sanitize_single_line("Hi\naiae "), "Hi aiae");
+        assert_eq!(sanitize_single_line("\r\nahte\n\r"), "ahte");
+    }
+
+    #[test]
+    fn test_sanitize_bidi_characters() {
+        // Legit inputs:
+        assert_eq!(
+            &sanitize_bidi_characters("Tes\u{2067}ting Delta Chat\u{2069}"),
+            "Tes\u{2067}ting Delta Chat\u{2069}"
+        );
+
+        assert_eq!(
+            &sanitize_bidi_characters("Tes\u{2067}ting \u{2068} Delta Chat\u{2069}\u{2069}"),
+            "Tes\u{2067}ting \u{2068} Delta Chat\u{2069}\u{2069}"
+        );
+
+        assert_eq!(
+            &sanitize_bidi_characters("Tes\u{2067}ting\u{2069} Delta Chat\u{2067}\u{2069}"),
+            "Tes\u{2067}ting\u{2069} Delta Chat\u{2067}\u{2069}"
+        );
+
+        // Potentially-malicious inputs:
+        assert_eq!(
+            &sanitize_bidi_characters("Tes\u{202C}ting Delta Chat"),
+            "Testing Delta Chat"
+        );
+
+        assert_eq!(
+            &sanitize_bidi_characters("Testing Delta Chat\u{2069}"),
+            "Testing Delta Chat"
+        );
+
+        assert_eq!(
+            &sanitize_bidi_characters("Tes\u{2067}ting Delta Chat"),
+            "Testing Delta Chat"
+        );
+
+        assert_eq!(
+            &sanitize_bidi_characters("Tes\u{2069}ting Delta Chat\u{2067}"),
+            "Testing Delta Chat"
+        );
+
+        assert_eq!(
+            &sanitize_bidi_characters("Tes\u{2068}ting Delta Chat"),
+            "Testing Delta Chat"
+        );
+    }
 }

src/chat.rs

@@ -8,7 +8,7 @@ use std::str::FromStr;
 use std::time::Duration;
 
 use anyhow::{anyhow, bail, ensure, Context as _, Result};
-use deltachat_contact_tools::{strip_rtlo_characters, ContactAddress};
+use deltachat_contact_tools::{sanitize_bidi_characters, sanitize_single_line, ContactAddress};
 use deltachat_derive::{FromSql, ToSql};
 use serde::{Deserialize, Serialize};
 use strum_macros::EnumIter;
@@ -46,8 +46,8 @@ use crate::stock_str;
 use crate::sync::{self, Sync::*, SyncData};
 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,
-    smeared_time, time, IsNoneOrEmpty, SystemTime,
+    create_smeared_timestamps, get_abs_path, gm2local_offset, smeared_time, time, IsNoneOrEmpty,
+    SystemTime,
 };
 
 /// An chat item, such as a message or a marker.
@@ -321,7 +321,7 @@ impl ChatId {
         param: Option<String>,
         timestamp: i64,
     ) -> Result<Self> {
-        let grpname = strip_rtlo_characters(grpname);
+        let grpname = sanitize_single_line(grpname);
         let timestamp = cmp::min(timestamp, smeared_time(context));
         let row_id =
             context.sql.insert(
@@ -2858,7 +2858,7 @@ 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 against RTLO attacks
     if msg.is_system_message() {
-        msg.text = strip_rtlo_characters(&msg.text);
+        msg.text = sanitize_bidi_characters(&msg.text);
     }
 
     if !prepare_send_msg(context, chat_id, msg).await?.is_empty() {
@@ -3503,7 +3503,7 @@ pub async fn create_group_chat(
     protect: ProtectionStatus,
     chat_name: &str,
 ) -> Result<ChatId> {
-    let chat_name = improve_single_line_input(chat_name);
+    let chat_name = sanitize_single_line(chat_name);
     ensure!(!chat_name.is_empty(), "Invalid chat name");
 
     let grpid = create_id();
@@ -4017,7 +4017,7 @@ async fn rename_ex(
     chat_id: ChatId,
     new_name: &str,
 ) -> Result<()> {
-    let new_name = improve_single_line_input(new_name);
+    let new_name = sanitize_single_line(new_name);
     /* the function only sets the names of group chats; normal chats get their names from the contacts */
     let mut success = false;
 
@@ -4048,7 +4048,7 @@ async fn rename_ex(
             if chat.is_promoted()
                 && !chat.is_mailing_list()
                 && chat.typ != Chattype::Broadcast
-                && improve_single_line_input(&chat.name) != new_name
+                && sanitize_single_line(&chat.name) != new_name
             {
                 msg.viewtype = Viewtype::Text;
                 msg.text =

src/config.rs

@@ -6,7 +6,7 @@ use std::str::FromStr;
 
 use anyhow::{ensure, Context as _, Result};
 use base64::Engine as _;
-use deltachat_contact_tools::addr_cmp;
+use deltachat_contact_tools::{addr_cmp, sanitize_single_line};
 use serde::{Deserialize, Serialize};
 use strum::{EnumProperty, IntoEnumIterator};
 use strum_macros::{AsRefStr, Display, EnumIter, EnumString};
@@ -20,7 +20,7 @@ use crate::log::LogExt;
 use crate::mimefactory::RECOMMENDED_FILE_SIZE;
 use crate::provider::{get_provider_by_id, Provider};
 use crate::sync::{self, Sync::*, SyncData};
-use crate::tools::{get_abs_path, improve_single_line_input};
+use crate::tools::get_abs_path;
 
 /// The available configuration keys.
 #[derive(
@@ -647,7 +647,7 @@ impl Context {
             }
             Config::Displayname => {
                 if let Some(v) = value {
-                    better_value = improve_single_line_input(v);
+                    better_value = sanitize_single_line(v);
                     value = Some(&better_value);
                 }
                 self.sql.set_raw_config(key.as_ref(), value).await?;

src/contact.rs

@@ -11,7 +11,7 @@ use async_channel::{self as channel, Receiver, Sender};
 use base64::Engine as _;
 pub use deltachat_contact_tools::may_be_valid_addr;
 use deltachat_contact_tools::{
-    self as contact_tools, addr_cmp, addr_normalize, sanitize_name_and_addr, strip_rtlo_characters,
+    self as contact_tools, addr_cmp, addr_normalize, sanitize_name, sanitize_name_and_addr,
     ContactAddress, VcardContact,
 };
 use deltachat_derive::{FromSql, ToSql};
@@ -37,9 +37,7 @@ use crate::param::{Param, Params};
 use crate::peerstate::Peerstate;
 use crate::sql::{self, params_iter};
 use crate::sync::{self, Sync::*};
-use crate::tools::{
-    duration_to_str, get_abs_path, improve_single_line_input, smeared_time, time, SystemTime,
-};
+use crate::tools::{duration_to_str, get_abs_path, smeared_time, time, SystemTime};
 use crate::{chat, chatlist_events, stock_str};
 
 /// Time during which a contact is considered as seen recently.
@@ -626,9 +624,7 @@ impl Contact {
         name: &str,
         addr: &str,
     ) -> Result<ContactId> {
-        let name = improve_single_line_input(name);
-
-        let (name, addr) = sanitize_name_and_addr(&name, addr);
+        let (name, addr) = sanitize_name_and_addr(name, addr);
         let addr = ContactAddress::new(&addr)?;
 
         let (contact_id, sth_modified) =
@@ -769,7 +765,7 @@ impl Contact {
             return Ok((ContactId::SELF, sth_modified));
         }
 
-        let mut name = strip_rtlo_characters(name);
+        let mut name = sanitize_name(name);
         #[allow(clippy::collapsible_if)]
         if origin <= Origin::OutgoingTo {
             // The user may accidentally have written to a "noreply" address with another MUA:
@@ -1924,7 +1920,7 @@ impl RecentlySeenLoop {
 
 #[cfg(test)]
 mod tests {
-    use deltachat_contact_tools::{may_be_valid_addr, normalize_name};
+    use deltachat_contact_tools::may_be_valid_addr;
 
     use super::*;
     use crate::chat::{get_chat_contacts, send_text_msg, Chat};
@@ -1963,15 +1959,6 @@ mod tests {
         assert_eq!(may_be_valid_addr("user@domain.tld."), false);
     }
 
-    #[test]
-    fn test_normalize_name() {
-        assert_eq!(&normalize_name(" hello world   "), "hello world");
-        assert_eq!(&normalize_name("<"), "<");
-        assert_eq!(&normalize_name(">"), ">");
-        assert_eq!(&normalize_name("'"), "'");
-        assert_eq!(&normalize_name("\""), "\"");
-    }
-
     #[test]
     fn test_normalize_addr() {
         assert_eq!(addr_normalize("mailto:john@doe.com"), "john@doe.com");