fix: Correctly sanitize input everywhere (#5697)

Best reviewed commit-by-commit; the commit messages explain what is
done.

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");

src/imap.rs

@@ -16,7 +16,7 @@ use std::{
 use anyhow::{bail, format_err, Context as _, Result};
 use async_channel::Receiver;
 use async_imap::types::{Fetch, Flag, Name, NameAttribute, UnsolicitedResponse};
-use deltachat_contact_tools::{normalize_name, ContactAddress};
+use deltachat_contact_tools::ContactAddress;
 use futures::{FutureExt as _, StreamExt, TryStreamExt};
 use futures_lite::FutureExt;
 use num_traits::FromPrimitive;
@@ -2424,12 +2424,6 @@ async fn add_all_recipients_as_contacts(
 
     let mut any_modified = false;
     for recipient in recipients {
-        let display_name_normalized = recipient
-            .display_name
-            .as_ref()
-            .map(|s| normalize_name(s))
-            .unwrap_or_default();
-
         let recipient_addr = match ContactAddress::new(&recipient.addr) {
             Err(err) => {
                 warn!(
@@ -2445,7 +2439,7 @@ async fn add_all_recipients_as_contacts(
 
         let (_, modified) = Contact::add_or_lookup(
             context,
-            &display_name_normalized,
+            &recipient.display_name.unwrap_or_default(),
             &recipient_addr,
             Origin::OutgoingTo,
         )

src/mimeparser.rs

@@ -6,7 +6,7 @@ use std::path::Path;
 use std::str;
 
 use anyhow::{bail, Context as _, Result};
-use deltachat_contact_tools::{addr_cmp, addr_normalize, strip_rtlo_characters};
+use deltachat_contact_tools::{addr_cmp, addr_normalize, sanitize_bidi_characters};
 use deltachat_derive::{FromSql, ToSql};
 use format_flowed::unformat_flowed;
 use lettre_email::mime::Mime;
@@ -2048,7 +2048,7 @@ fn get_attachment_filename(
         };
     }
 
-    let desired_filename = desired_filename.map(|filename| strip_rtlo_characters(&filename));
+    let desired_filename = desired_filename.map(|filename| sanitize_bidi_characters(&filename));
 
     Ok(desired_filename)
 }

src/receive_imf.rs

@@ -4,9 +4,7 @@ use std::collections::HashSet;
 use std::str::FromStr;
 
 use anyhow::{Context as _, Result};
-use deltachat_contact_tools::{
-    addr_cmp, may_be_valid_addr, normalize_name, strip_rtlo_characters, ContactAddress,
-};
+use deltachat_contact_tools::{addr_cmp, may_be_valid_addr, sanitize_single_line, ContactAddress};
 use iroh_gossip::proto::TopicId;
 use mailparse::{parse_mail, SingleInfo};
 use num_traits::FromPrimitive;
@@ -660,10 +658,10 @@ pub async fn from_field_to_contact_id(
         }
     };
 
-    let from_id = add_or_lookup_contact_by_addr(
+    let (from_id, _) = Contact::add_or_lookup(
         context,
-        display_name,
-        from_addr,
+        display_name.unwrap_or_default(),
+        &from_addr,
         Origin::IncomingUnknownFrom,
     )
     .await?;
@@ -2141,6 +2139,8 @@ async fn apply_group_changes(
             .map(|grpname| grpname.trim())
             .filter(|grpname| grpname.len() < 200)
         {
+            let grpname = &sanitize_single_line(grpname);
+            let old_name = &sanitize_single_line(old_name);
             if chat_id
                 .update_timestamp(
                     context,
@@ -2152,10 +2152,7 @@ async fn apply_group_changes(
                 info!(context, "Updating grpname for chat {chat_id}.");
                 context
                     .sql
-                    .execute(
-                        "UPDATE chats SET name=? WHERE id=?;",
-                        (strip_rtlo_characters(grpname), chat_id),
-                    )
+                    .execute("UPDATE chats SET name=? WHERE id=?;", (grpname, chat_id))
                     .await?;
                 send_event_chat_modified = true;
             }
@@ -2428,7 +2425,7 @@ fn compute_mailinglist_name(
         }
     }
 
-    strip_rtlo_characters(&name)
+    sanitize_single_line(&name)
 }
 
 /// Set ListId param on the contact and ListPost param the chat.
@@ -2850,8 +2847,9 @@ async fn add_or_lookup_contacts_by_address_list(
         }
         let display_name = info.display_name.as_deref();
         if let Ok(addr) = ContactAddress::new(addr) {
-            let contact_id =
-                add_or_lookup_contact_by_addr(context, display_name, addr, origin).await?;
+            let (contact_id, _) =
+                Contact::add_or_lookup(context, display_name.unwrap_or_default(), &addr, origin)
+                    .await?;
             contact_ids.insert(contact_id);
         } else {
             warn!(context, "Contact with address {:?} cannot exist.", addr);
@@ -2861,22 +2859,5 @@ async fn add_or_lookup_contacts_by_address_list(
     Ok(contact_ids.into_iter().collect::<Vec<ContactId>>())
 }
 
-/// Add contacts to database on receiving messages.
-async fn add_or_lookup_contact_by_addr(
-    context: &Context,
-    display_name: Option<&str>,
-    addr: ContactAddress,
-    origin: Origin,
-) -> Result<ContactId> {
-    if context.is_self_addr(&addr).await? {
-        return Ok(ContactId::SELF);
-    }
-    let display_name_normalized = display_name.map(normalize_name).unwrap_or_default();
-
-    let (contact_id, _modified) =
-        Contact::add_or_lookup(context, &display_name_normalized, &addr, origin).await?;
-    Ok(contact_id)
-}
-
 #[cfg(test)]
 mod tests;

src/receive_imf/tests.rs

@@ -4886,19 +4886,63 @@ async fn test_make_n_send_vcard() -> Result<()> {
 #[tokio::test(flavor = "multi_thread", worker_threads = 2)]
 async fn test_group_no_recipients() -> Result<()> {
     let t = &TestContext::new_alice().await;
-    let raw = b"From: alice@example.org\n\
-                    Subject: Group\n\
-                    Chat-Version: 1.0\n\
-                    Chat-Group-Name: Group\n\
-                    Chat-Group-ID: GePFDkwEj2K\n\
-                    Message-ID: <foobar@localhost>\n\
-                    \n\
-                    Hello!";
+    let raw = "From: alice@example.org
+Subject: Group
+Chat-Version: 1.0
+Chat-Group-Name: Group
+ name\u{202B}
+Chat-Group-ID: GePFDkwEj2K
+Message-ID: <foobar@localhost>
+
+Hello!"
+        .as_bytes();
     let received = receive_imf(t, raw, false).await?.unwrap();
     let msg = Message::load_from_db(t, *received.msg_ids.last().unwrap()).await?;
     let chat = Chat::load_from_db(t, msg.chat_id).await?;
     assert_eq!(chat.typ, Chattype::Group);
 
+    // Check that the weird group name is sanitzied correctly:
+    let mail = mailparse::parse_mail(raw).unwrap();
+    assert_eq!(
+        mail.headers
+            .get_header(HeaderDef::ChatGroupName)
+            .unwrap()
+            .get_value_raw(),
+        "Group\n name\u{202B}".as_bytes()
+    );
+    assert_eq!(chat.name, "Group name");
+
+    Ok(())
+}
+
+#[tokio::test(flavor = "multi_thread", worker_threads = 2)]
+async fn test_group_name_with_newline() -> Result<()> {
+    let t = &TestContext::new_alice().await;
+    let raw = "From: alice@example.org
+Subject: Group
+Chat-Version: 1.0
+Chat-Group-Name: =?utf-8?q?Delta=0D=0AChat?=
+Chat-Group-ID: GePFDkwEj2K
+Message-ID: <foobar@localhost>
+
+Hello!"
+        .as_bytes();
+    let received = receive_imf(t, raw, false).await?.unwrap();
+    let msg = Message::load_from_db(t, *received.msg_ids.last().unwrap()).await?;
+    let chat = Chat::load_from_db(t, msg.chat_id).await?;
+    assert_eq!(chat.typ, Chattype::Group);
+
+    // Check that the weird group name is sanitzied correctly:
+    let mail = mailparse::parse_mail(raw).unwrap();
+    assert_eq!(
+        mail.headers
+            .get_header(HeaderDef::ChatGroupName)
+            .unwrap()
+            .get_value(),
+        "Delta\r\nChat"
+    );
+    assert_eq!(chat.name, "Delta  Chat");
+
     Ok(())
 }
 

src/tools.rs

@@ -22,7 +22,7 @@ pub use std::time::SystemTime;
 use anyhow::{bail, Context as _, Result};
 use base64::Engine as _;
 use chrono::{Local, NaiveDateTime, NaiveTime, TimeZone};
-use deltachat_contact_tools::{strip_rtlo_characters, EmailAddress};
+use deltachat_contact_tools::EmailAddress;
 #[cfg(test)]
 pub use deltachat_time::SystemTimeTools as SystemTime;
 use futures::{StreamExt, TryStreamExt};
@@ -511,13 +511,6 @@ pub fn parse_mailto(mailto_url: &str) -> Option<MailTo> {
     }
 }
 
-/// Sanitizes user input
-/// - strip newlines
-/// - strip malicious bidi characters
-pub(crate) fn improve_single_line_input(input: &str) -> String {
-    strip_rtlo_characters(input.replace(['\n', '\r'], " ").trim())
-}
-
 pub(crate) trait IsNoneOrEmpty<T> {
     /// Returns true if an Option does not contain a string
     /// or contains an empty string.
@@ -1025,12 +1018,6 @@ DKIM Results: Passed=true";
         assert_eq!(h, 50);
     }
 
-    #[test]
-    fn test_improve_single_line_input() {
-        assert_eq!(improve_single_line_input("Hi\naiae "), "Hi aiae");
-        assert_eq!(improve_single_line_input("\r\nahte\n\r"), "ahte");
-    }
-
     #[tokio::test(flavor = "multi_thread", worker_threads = 2)]
     async fn test_maybe_warn_on_bad_time() {
         let t = TestContext::new().await;

src/update_helper.rs

@@ -183,8 +183,8 @@ mod tests {
                  Message-ID: <msg3@example.org>\n\
                  Chat-Version: 1.0\n\
                  Chat-Group-ID: abcde123456\n\
-                 Chat-Group-Name: another name update\n\
-                 Chat-Group-Name-Changed: a name update\n\
+                 Chat-Group-Name: =?utf-8?q?another=0Aname update?=\n\
+                 Chat-Group-Name-Changed: =?utf-8?q?a=0Aname update?=\n\
                  Date: Sun, 22 Mar 2021 03:00:00 +0000\n\
                  \n\
                  third message\n",
@@ -198,7 +198,7 @@ mod tests {
                  Message-ID: <msg2@example.org>\n\
                  Chat-Version: 1.0\n\
                  Chat-Group-ID: abcde123456\n\
-                 Chat-Group-Name: a name update\n\
+                 Chat-Group-Name: =?utf-8?q?a=0Aname update?=\n\
                  Chat-Group-Name-Changed: initial name\n\
                  Date: Sun, 22 Mar 2021 02:00:00 +0000\n\
                  \n\
@@ -210,6 +210,9 @@ mod tests {
         let chat = Chat::load_from_db(&t, msg.chat_id).await?;
         assert_eq!(chat.name, "another name update");
 
+        // Assert that the \n was correctly removed from the group name also in the system message
+        assert_eq!(msg.text.contains('\n'), false);
+
         Ok(())
     }
 }

src/webxdc.rs

@@ -22,7 +22,7 @@ use std::path::Path;
 
 use anyhow::{anyhow, bail, ensure, format_err, Context as _, Result};
 
-use deltachat_contact_tools::strip_rtlo_characters;
+use deltachat_contact_tools::sanitize_bidi_characters;
 use deltachat_derive::FromSql;
 use lettre_email::PartBuilder;
 use rusqlite::OptionalExtension;
@@ -349,7 +349,7 @@ impl Context {
             {
                 instance
                     .param
-                    .set(Param::WebxdcSummary, strip_rtlo_characters(summary));
+                    .set(Param::WebxdcSummary, sanitize_bidi_characters(summary));
                 param_changed = true;
             }
         }