fix: do not include CRLF before MIME boundary in the part body

This change adds a test and updates mailparse from 0.15.0 to 0.16.0. mailparse 0.16.0 includes a fix for the bug that resulted in CRLF being included at the end of the body. Workaround for the bug in the `pk_validate` function is also removed.
deltachat · Feb 10, 2025 · a0ff0d7 · a0ff0d7
1 parent 0687264
commit a0ff0d7
Show file tree

Hide file tree

Showing 5 changed files with 78 additions and 13 deletions.
diff --git a/Cargo.lock b/Cargo.lock
diff --git a/Cargo.toml b/Cargo.toml
@@ -69,7 +69,7 @@ iroh = { version = "0.32", default-features = false }
 kamadak-exif = "0.6.1"
 lettre_email = { git = "https://github.com/deltachat/lettre", branch = "master" }
 libc = { workspace = true }
-mailparse = "0.15"
+mailparse = "0.16"
 mime = "0.3.17"
 num_cpus = "1.16"
 num-derive = "0.4"

diff --git a/src/html.rs b/src/html.rs
@@ -371,7 +371,6 @@ and will be wrapped as usual.<br/>
 mime-modified should not be set set as there is no html and no special stuff;<br/>
 although not being a delta-message.<br/>
 test some special html-characters as &lt; &gt; and &amp; but also &quot; and &#x27; :)<br/>
-<br/>
 </body></html>
 "#
         );
@@ -405,7 +404,6 @@ test some special html-characters as &lt; &gt; and &amp; but also &quot; and &#x
             r##"<html>
   <p>mime-modified <b>set</b>; simplify is always regarded as lossy.</p>
 </html>
-
 "##
         );
     }
@@ -422,7 +420,6 @@ test some special html-characters as &lt; &gt; and &amp; but also &quot; and &#x
     this is <b>html</b>
   </p>
 </html>
-
 "##
         );
     }

diff --git a/src/mimeparser/mimeparser_tests.rs b/src/mimeparser/mimeparser_tests.rs
@@ -1817,3 +1817,78 @@ async fn test_protect_autocrypt() -> Result<()> {
 
     Ok(())
 }
+
+/// Tests that CRLF before MIME boundary
+/// is not treated as the part body.
+///
+/// RFC 2046 explicitly says that
+/// "The CRLF preceding the boundary delimiter line is conceptually attached
+/// to the boundary so that it is possible to have a part that does not end
+/// with a CRLF (line break). Body parts that must be considered to end with
+/// line breaks, therefore, must have two CRLFs preceding the boundary delimiter
+/// line, the first of which is part of the preceding body part,
+/// and the second of which is part of the encapsulation boundary."
+#[tokio::test(flavor = "multi_thread", worker_threads = 2)]
+async fn test_mimeparser_trailing_newlines() {
+    let context = TestContext::new_alice().await;
+
+    // Example from <https://www.rfc-editor.org/rfc/rfc2046#section-5.1.1>
+    // with a `Content-Disposition` headers added to turn files
+    // into attachments.
+    let raw = b"From: Nathaniel Borenstein <[email protected]>\r
+To: Ned Freed <[email protected]>\r
+Date: Sun, 21 Mar 1993 23:56:48 -0800 (PST)\r
+Subject: Sample message\r
+MIME-Version: 1.0\r
+Content-type: multipart/mixed; boundary=\"simple boundary\"\r
+\r
+This is the preamble.  It is to be ignored, though it\r
+is a handy place for composition agents to include an\r
+explanatory note to non-MIME conformant readers.\r
+\r
+--simple boundary\r
+Content-Disposition: attachment; filename=\"file1.txt\"\r
+\r
+This is implicitly typed plain US-ASCII text.\r
+It does NOT end with a linebreak.\r
+--simple boundary\r
+Content-type: text/plain; charset=us-ascii\r
+Content-Disposition: attachment; filename=\"file2.txt\"\r
+\r
+This is explicitly typed plain US-ASCII text.\r
+It DOES end with a linebreak.\r
+\r
+--simple boundary--\r
+\r
+This is the epilogue.  It is also to be ignored.";
+
+    let mimeparser = MimeMessage::from_bytes(&context, &raw[..], None)
+        .await
+        .unwrap();
+
+    assert_eq!(mimeparser.parts.len(), 2);
+
+    assert_eq!(mimeparser.parts[0].typ, Viewtype::File);
+    let blob: BlobObject = mimeparser.parts[0]
+        .param
+        .get_blob(Param::File, &context)
+        .await
+        .unwrap()
+        .unwrap();
+    assert_eq!(
+        tokio::fs::read_to_string(blob.to_abs_path()).await.unwrap(),
+        "This is implicitly typed plain US-ASCII text.\r\nIt does NOT end with a linebreak."
+    );
+
+    assert_eq!(mimeparser.parts[1].typ, Viewtype::File);
+    let blob: BlobObject = mimeparser.parts[1]
+        .param
+        .get_blob(Param::File, &context)
+        .await
+        .unwrap()
+        .unwrap();
+    assert_eq!(
+        tokio::fs::read_to_string(blob.to_abs_path()).await.unwrap(),
+        "This is explicitly typed plain US-ASCII text.\r\nIt DOES end with a linebreak.\r\n"
+    );
+}
diff --git a/src/pgp.rs b/src/pgp.rs
@@ -379,13 +379,6 @@ pub fn pk_validate(
 
     let standalone_signature = StandaloneSignature::from_armor_single(Cursor::new(signature))?.0;
 
-    // Remove trailing CRLF before the delimiter.
-    // According to RFC 3156 it is considered to be part of the MIME delimiter for the purpose of
-    // OpenPGP signature calculation.
-    let content = content
-        .get(..content.len().saturating_sub(2))
-        .context("index is out of range")?;
-
     for pkey in public_keys_for_validation {
         if standalone_signature.verify(pkey, content).is_ok() {
             let fp = pkey.dc_fingerprint();