Re: [PATCH proxmox] fix #7814: send-mail: normalize LF line endings to CRLF endings

[Samuel Rufinatscha <s.rufinatscha@proxmox.com>]

Hi Shannon,

I ran some additional tests, which passed. One note/question regarding
the base64 path:

Removing \r before base64 encoding changes an valid \r\n into \n after
decoding but if I understand correctly previously it would have
preserved \r\n (as of no normalization).
 From the commit message I understand that removing CR before encoding
is intentional but is changing valid CRLF intentional here too?

Besides that I tested:

- the bytes that are passed to send() and forward() using a sendmail
mock (wrote bytes to a file). verified that \n becomes \r\n, \r is
removed and \r\n is unchanged
- created a binary attachment containing all 256 byte values and
verified that the content is unchanged after base64 decoding

On 7/30/26 4:01 PM, Shannon Sterz wrote:
> RFC5321 Section 2.3.8 requires that mails do not contain bare CR or LF
> bytes. this patch normalizes unix style LF line endings to CRLF line
> endings. it also removes bare CR bytes.
> 
> when text would be base64 encoded anyway, CR bytes will be dropped, to
> keep the behavior for encoded and non-encoded text aligned. however,
> since this code was already extensively tested against multiple MUAs,
> encoded LF line endings are kept as-is.
> 
> also adds tests to encode this behavior.
> 
> Signed-off-by: Shannon Sterz <s.sterz@proxmox.com>
> ---
> 
> Notes:
>      v1: https://lore.proxmox.com/all/20260714084719.68076-1-s.sterz@proxmox.com/
>      
>      changes since v1 (thanks @ Lukas Wagner):
>      
>      * fixed a typo (than -> then)
>      * adjusted how the capacity for forwarded messages that need to be
>        normalized is calculated.
> 
>   proxmox-sendmail/src/lib.rs | 244 ++++++++++++++++++++++++++++++++----
>   1 file changed, 218 insertions(+), 26 deletions(-)
> 
> diff --git a/proxmox-sendmail/src/lib.rs b/proxmox-sendmail/src/lib.rs
> index b3ff56e5..355ed12d 100644
> --- a/proxmox-sendmail/src/lib.rs
> +++ b/proxmox-sendmail/src/lib.rs
> @@ -50,7 +50,10 @@ fn encode_base64_formatted<T: AsRef<[u8]>>(raw: T) -> String {
>   }
>   
>   fn encode_filename_formatted(filename: &str) -> String {
> -    let encoded = format!("UTF-8''{}", utf8_percent_encode(filename, RFC5987SET));
> +    let encoded = format!(
> +        "UTF-8''{}",
> +        utf8_percent_encode(&filename.replace('\r', ""), RFC5987SET)
> +    );
>   
>       let format_prefix = |l| {
>           if let Some(l) = l {
> @@ -63,6 +66,18 @@ fn encode_filename_formatted(filename: &str) -> String {
>       format_encoded_text(&encoded, format_prefix, ";", 0, true)
>   }
>   
> +/// Strip bare CR bytes, then base64 encode. This is useful to make sure we treat encoded (header)
> +/// values the same as non-encoded one. Stripping CR bytes is necessary according to RFC 5321 Section
> +/// Section 2.3.8 (https://datatracker.ietf.org/doc/html/rfc5321#section-2.3.8).
> +fn strip_cr_base64<T: AsRef<[u8]>>(raw: T) -> String {
> +    let normalized: Vec<u8> = raw
> +        .as_ref()
> +        .iter()
> +        .filter_map(|b| (*b != b'\r').then_some(*b))
> +        .collect();
> +    proxmox_base64::encode(&normalized)
> +}
> +
>   /// Helper function used to format text to not exceed line limits imposed by standards such as RFC
>   /// 2045 Section 6.8 that specifies that Base64 content transfer encoded text "must be represented
>   /// in lines of no more than 76 characters each". It also allows taking into account any necessary
> @@ -166,11 +181,7 @@ impl Recipient {
>       fn format_recipient(&self) -> String {
>           if let Some(name) = &self.name {
>               if !name.is_ascii() {
> -                format!(
> -                    "=?utf-8?B?{}?= <{}>",
> -                    proxmox_base64::encode(name),
> -                    self.email
> -                )
> +                format!("=?utf-8?B?{}?= <{}>", strip_cr_base64(name), self.email)
>               } else {
>                   format!("{name} <{}>", self.email)
>               }
> @@ -199,7 +210,7 @@ impl Attachment<'_> {
>           if self.filename.is_ascii() {
>               let _ = write!(attachment, "{}", &self.filename);
>           } else {
> -            let encoded = proxmox_base64::encode(&self.filename);
> +            let encoded = strip_cr_base64(&self.filename);
>               let prefix_fn = |line| {
>                   if Some(0) == line {
>                       "=?utf-8?B?"
> @@ -451,7 +462,10 @@ impl<'a> Mail<'a> {
>   
>       /// Forwards an email message to a given list of recipients.
>       ///
> -    /// `message` must be compatible with ``sendmail`` (the message is piped into stdin unmodified).
> +    /// `message` must be compatible with ``sendmail``. The message is piped into `stdin`
> +    /// unmodified. One exception is stripping bare CR bytes and normalizing LF line endings to
> +    /// CRLF line endings in accordance with RFC 5321 Section 2.3.8
> +    /// (https://datatracker.ietf.org/doc/html/rfc5321#section-2.3.8).
>       #[cfg(feature = "mail-forwarder")]
>       pub fn forward(
>           mailto: &[&str],
> @@ -485,11 +499,25 @@ impl<'a> Mail<'a> {
>               .spawn()
>               .with_context(|| "could not spawn sendmail process")?;
>   
> +        // this will always have enough capacity, since the worst case is a message consisting only
> +        // of "\n" and that would exactly double in size. messages are also unlikely to be that big
> +        // that the extra allocated space here really matters. so set the capacity to the upper
> +        // bound to avoid more complex capacity calculations that would take more time to run.
> +        let mut normalized_message: Vec<u8> = Vec::with_capacity(message.len() * 2);
> +
> +        for byte in message {
> +            match byte {
> +                b'\r' => continue,
> +                b'\n' => normalized_message.extend_from_slice(&[b'\r', b'\n']),
> +                b => normalized_message.push(*b),
> +            }
> +        }
> +
>           sendmail_process
>               .stdin
>               .take()
>               .unwrap()
> -            .write_all(message)
> +            .write_all(&normalized_message)
>               .with_context(|| "couldn't write to sendmail stdin")?;
>   
>           sendmail_process
> @@ -520,7 +548,9 @@ impl<'a> Mail<'a> {
>               write!(mail, "\n--{file_boundary}--")?;
>           }
>   
> -        Ok(mail)
> +        // normalize line endings from `\n` to `\r\n` and drop bare `\r` according to RFC 5321
> +        // 2.3.8: https://datatracker.ietf.org/doc/html/rfc5321#section-2.3.8
> +        Ok(mail.replace('\r', "").replace('\n', "\r\n"))
>       }
>   
>       fn format_header(
> @@ -561,7 +591,7 @@ impl<'a> Mail<'a> {
>               writeln!(
>                   header,
>                   "Subject: =?utf-8?B?{}?=",
> -                proxmox_base64::encode(&self.subject)
> +                strip_cr_base64(&self.subject)
>               )?;
>           } else {
>               writeln!(header, "Subject: {}", self.subject)?;
> @@ -571,7 +601,7 @@ impl<'a> Mail<'a> {
>               writeln!(
>                   header,
>                   "From: =?utf-8?B?{}?= <{}>",
> -                proxmox_base64::encode(&self.mail_author),
> +                strip_cr_base64(&self.mail_author),
>                   self.mail_from
>               )?;
>           } else {
> @@ -606,17 +636,15 @@ impl<'a> Mail<'a> {
>           header.push_str("Auto-Submitted: auto-generated;\n");
>   
>           for (key, value) in &self.headers {
> -            // Strip CR/LF so an interpolated key or value cannot inject extra header lines.
> -            let key = key.replace(['\r', '\n'], "");
> -            let value = value.replace(['\r', '\n'], "");
> +            // Strip LF so an interpolated key or value cannot inject extra header lines. Bare CR
> +            // will be stripped for non-ASCII text before encoding and for ASCII text globally
> +            // before finishing formatting.
> +            let key = key.replace('\n', "");
> +            let value = value.replace('\n', "");
>               if value.is_ascii() {
>                   writeln!(header, "{key}: {value}")?;
>               } else {
> -                writeln!(
> -                    header,
> -                    "{key}: =?utf-8?B?{}?=",
> -                    proxmox_base64::encode(&value)
> -                )?;
> +                writeln!(header, "{key}: =?utf-8?B?{}?=", strip_cr_base64(&value))?;
>               }
>           }
>   
> @@ -653,7 +681,7 @@ impl<'a> Mail<'a> {
>               body.push_str(&self.body_txt);
>           } else {
>               body.push_str("Content-Transfer-Encoding: base64\n\n");
> -            body.push_str(&encode_base64_formatted(&self.body_txt));
> +            body.push_str(&encode_base64_formatted(self.body_txt.replace('\r', "")));
>           }
>   
>           if let Some(html) = &self.body_html {
> @@ -666,7 +694,7 @@ impl<'a> Mail<'a> {
>                   body.push_str(html);
>               } else {
>                   body.push_str("Content-Transfer-Encoding: base64\n\n");
> -                body.push_str(&encode_base64_formatted(html));
> +                body.push_str(&encode_base64_formatted(html.replace('\r', "")));
>               }
>   
>               write!(body, "\n--{html_boundary}--")?;
> @@ -1140,10 +1168,10 @@ Content-Transfer-Encoding: 7bit
>           ];
>   
>           let mail = Mail::new(
> -            "Sender Näme",
> -            "from@example.com",
> -            "Subject Linë",
> -            "Lorem Ipsum Dolor Sit\nAmët",
> +            "Sender Nä\rme",
> +            "from@example.\rcom",
> +            "Subject Linë\r",
> +            "Lorem Ipsum Dolor \rSit\nAmët",
>           )
>           .with_recipient_and_name("Receiver Näme", "receiver@example.com")
>           .with_attachment("deadbeef.bin", "application/octet-stream", &bin)
> @@ -1344,4 +1372,168 @@ ABCDEFGHIJKLMNOPQRSTUVWXYZabcdefghijklmnopqrstuvwxyz
>   
>           assert_eq!(suffixed, out);
>       }
> +
> +    #[test]
> +    fn line_endings_are_normalized_when_not_encoded() {
> +        let bin: [u8; 63] = [
> +            0xde, 0xad, 0xbe, 0xef, 0xde, 0xad, 0xbe, 0xef, 0xde, 0xad, 0xbe, 0xef, 0xde, 0xad,
> +            0xde, 0xad, 0xbe, 0xef, 0xde, 0xad, 0xbe, 0xef, 0xde, 0xad, 0xbe, 0xef, 0xde, 0xad,
> +            0xde, 0xad, 0xbe, 0xef, 0xde, 0xad, 0xbe, 0xef, 0xde, 0xad, 0xbe, 0xef, 0xde, 0xad,
> +            0xde, 0xad, 0xbe, 0xef, 0xde, 0xad, 0xbe, 0xef, 0xde, 0xad, 0xbe, 0xef, 0xde, 0xad,
> +            0xbe, 0xef, 0xde, 0xad, 0xbe, 0xef, 0x0d,
> +        ];
> +
> +        let mail = Mail::new(
> +            "Se\rnder Name",
> +            "fro\rm@example.com",
> +            "Sub\rject Line",
> +            "Lor\rem Ipsum Dolor Sit\nAmet",
> +        )
> +        .with_recipient_and_name("R\receiver Name", "receive\rr@example.com")
> +        .with_attachment("deadb\reef.bin", "application/octet-stream", &bin)
> +        .with_html_alt("<html lang=\"de-at\"><head></head><body>\n\t<pre>\n\t\tLorem Ipsum Dolor Si\rt Amet\n\t</pre>\n</body></html>")
> +        .with_header("X-CR-HEADE\r", "value\r");
> +
> +        let body = mail.format_mail(1732806251).expect("could not format mail");
> +
> +        assert!(body.is_ascii());
> +
> +        let expected = r#"Content-Type: multipart/mixed;
> +	boundary="----_=_NextPart_001_1732806251"
> +MIME-Version: 1.0
> +Subject: Subject Line
> +From: Sender Name <from@example.com>
> +To: Receiver Name <receiver@example.com>
> +Date: Thu, 28 Nov 2024 16:04:11 +0100
> +Auto-Submitted: auto-generated;
> +X-CR-HEADE: value
> +
> +This is a multi-part message in MIME format.
> +
> +------_=_NextPart_001_1732806251
> +Content-Type: multipart/alternative; boundary="----_=_NextPart_002_1732806251"
> +MIME-Version: 1.0
> +
> +------_=_NextPart_002_1732806251
> +Content-Type: text/plain;
> +	charset="UTF-8"
> +Content-Transfer-Encoding: 7bit
> +
> +Lorem Ipsum Dolor Sit
> +Amet
> +------_=_NextPart_002_1732806251
> +Content-Type: text/html;
> +	charset="UTF-8"
> +Content-Transfer-Encoding: 7bit
> +
> +<html lang="de-at"><head></head><body>
> +	<pre>
> +		Lorem Ipsum Dolor Sit Amet
> +	</pre>
> +</body></html>
> +------_=_NextPart_002_1732806251--
> +------_=_NextPart_001_1732806251
> +Content-Type: application/octet-stream;
> +	name="deadbeef.bin"
> +Content-Disposition: attachment;
> +	filename*0*=UTF-8''deadbeef.bin
> +Content-Transfer-Encoding: base64
> +
> +3q2+796tvu/erb7v3q3erb7v3q2+796tvu/erd6tvu/erb7v3q2+796t3q2+796tvu/erb7v
> +3q2+796tvu8N
> +------_=_NextPart_001_1732806251--"#;
> +
> +        // manually compare here to ensure we split specifically only on `\r\n` line endings
> +
> +        let lines1 = body.split("\r\n");
> +        let lines2 = expected.split("\n");
> +
> +        for ((i, line1), line2) in lines1.enumerate().zip(lines2) {
> +            if !(line1.starts_with("Date:") && line2.starts_with("Date:")) {
> +                assert_eq!(line1, line2, "lines at {i} do not match");
> +            }
> +        }
> +
> +        assert_eq!(body.lines().count(), expected.lines().count());
> +    }
> +
> +    #[test]
> +    fn line_endings_are_normalized_when_encoded() {
> +        let bin: [u8; 63] = [
> +            0xde, 0xad, 0xbe, 0xef, 0xde, 0xad, 0xbe, 0xef, 0xde, 0xad, 0xbe, 0xef, 0xde, 0xad,
> +            0xde, 0xad, 0xbe, 0xef, 0xde, 0xad, 0xbe, 0xef, 0xde, 0xad, 0xbe, 0xef, 0xde, 0xad,
> +            0xde, 0xad, 0xbe, 0xef, 0xde, 0xad, 0xbe, 0xef, 0xde, 0xad, 0xbe, 0xef, 0xde, 0xad,
> +            0xde, 0xad, 0xbe, 0xef, 0xde, 0xad, 0xbe, 0xef, 0xde, 0xad, 0xbe, 0xef, 0xde, 0xad,
> +            0xbe, 0xef, 0xde, 0xad, 0xbe, 0xef, 0x0d,
> +        ];
> +
> +        let mail = Mail::new(
> +            "Se\rnder Näme",
> +            "fro\rm@example.com",
> +            "Sub\rject Linë",
> +            "Lor\rem Ipsum Dolor Sit\nAmët",
> +        )
> +        .with_recipient_and_name("R\receiver Näme", "receive\rr@example.com")
> +        .with_attachment("🐄💀.b\rin", "ima\rge/bmp", &bin)
> +        .with_html_alt("<html lang=\"de-at\"><head></head><body>\n\t<pre>\n\t\tLorem Ipsum Dölor Si\rt Amet\n\t</pre>\n</body></html>")
> +        .with_header("X-CR-HEADE\r", "valüe\r");
> +
> +        let body = mail.format_mail(1732806251).expect("could not format mail");
> +
> +        assert!(body.is_ascii());
> +
> +        let expected = r#"Content-Type: multipart/mixed;
> +	boundary="----_=_NextPart_001_1732806251"
> +MIME-Version: 1.0
> +Subject: =?utf-8?B?U3ViamVjdCBMaW7Dqw==?=
> +From: =?utf-8?B?U2VuZGVyIE7DpG1l?= <from@example.com>
> +To: =?utf-8?B?UmVjZWl2ZXIgTsOkbWU=?= <receiver@example.com>
> +Date: Thu, 28 Nov 2024 16:04:11 +0100
> +Auto-Submitted: auto-generated;
> +X-CR-HEADE: =?utf-8?B?dmFsw7xl?=
> +
> +This is a multi-part message in MIME format.
> +
> +------_=_NextPart_001_1732806251
> +Content-Type: multipart/alternative; boundary="----_=_NextPart_002_1732806251"
> +MIME-Version: 1.0
> +
> +------_=_NextPart_002_1732806251
> +Content-Type: text/plain;
> +	charset="UTF-8"
> +Content-Transfer-Encoding: base64
> +
> +TG9yZW0gSXBzdW0gRG9sb3IgU2l0CkFtw6t0
> +------_=_NextPart_002_1732806251
> +Content-Type: text/html;
> +	charset="UTF-8"
> +Content-Transfer-Encoding: base64
> +
> +PGh0bWwgbGFuZz0iZGUtYXQiPjxoZWFkPjwvaGVhZD48Ym9keT4KCTxwcmU+CgkJTG9yZW0g
> +SXBzdW0gRMO2bG9yIFNpdCBBbWV0Cgk8L3ByZT4KPC9ib2R5PjwvaHRtbD4=
> +------_=_NextPart_002_1732806251--
> +------_=_NextPart_001_1732806251
> +Content-Type: image/bmp;
> +	name="=?utf-8?B?8J+QhPCfkoAuYmlu?="
> +Content-Disposition: attachment;
> +	filename*0*=UTF-8''%F0%9F%90%84%F0%9F%92%80.bin
> +Content-Transfer-Encoding: base64
> +
> +3q2+796tvu/erb7v3q3erb7v3q2+796tvu/erd6tvu/erb7v3q2+796t3q2+796tvu/erb7v
> +3q2+796tvu8N
> +------_=_NextPart_001_1732806251--"#;
> +
> +        // manually compare here to ensure we split specifically only on `\r\n` line endings
> +
> +        let lines1 = body.split("\r\n");
> +        let lines2 = expected.split("\n");
> +
> +        for ((i, line1), line2) in lines1.enumerate().zip(lines2) {
> +            if !(line1.starts_with("Date:") && line2.starts_with("Date:")) {
> +                assert_eq!(line1, line2, "lines at {i} do not match");
> +            }
> +        }
> +
> +        assert_eq!(body.lines().count(), expected.lines().count());
> +    }
>   }