From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from gate001.proxmox.com (gate001.proxmox.com [45.144.208.40]) by lore.proxmox.com (Postfix) with ESMTPS id 8280B1FF0E5 for ; Wed, 29 Jul 2026 10:18:30 +0200 (CEST) Received: from gate001.proxmox.com (localhost.localdomain [127.0.0.1]) by gate001.proxmox.com (Proxmox) with ESMTP id 0C3B820968; Wed, 29 Jul 2026 10:18:30 +0200 (CEST) Mime-Version: 1.0 Content-Transfer-Encoding: quoted-printable Content-Type: text/plain; charset=UTF-8 Date: Wed, 29 Jul 2026 10:18:06 +0200 Message-Id: From: "Lukas Wagner" To: "Shannon Sterz" , Subject: Re: [PATCH proxmox] fix #7814: send-mail: normalize LF line endings to CRLF endings X-Mailer: aerc 0.21.0-0-g5549850facc2-dirty References: <20260714084719.68076-1-s.sterz@proxmox.com> In-Reply-To: <20260714084719.68076-1-s.sterz@proxmox.com> X-Bm-Milter-Handled: 55990f41-d878-4baa-be0a-ee34c49e34d2 X-Bm-Transport-Timestamp: 1785313047892 X-SPAM-LEVEL: Spam detection results: 0 AWL 0.254 Adjusted score from AWL reputation of From: address DMARC_MISSING 0.1 Missing DMARC policy KAM_DMARC_STATUS 0.01 Test Rule for DKIM or SPF Failure with Strict Alignment (newer systems) RCVD_IN_DNSWL_LOW -0.7 Sender listed at https://www.dnswl.org/, low trust SPF_HELO_NONE 0.001 SPF: HELO does not publish an SPF Record SPF_PASS -0.001 SPF: sender matches SPF record Message-ID-Hash: TSJACTZOZ6RJK2PUOMJLPZCNIONJTEZW X-Message-ID-Hash: TSJACTZOZ6RJK2PUOMJLPZCNIONJTEZW X-MailFrom: l.wagner@proxmox.com X-Mailman-Rule-Misses: dmarc-mitigation; no-senders; approved; loop; banned-address; emergency; member-moderation; nonmember-moderation; administrivia; implicit-dest; max-recipients; max-size; news-moderation; no-subject; digests; suspicious-header X-Mailman-Version: 3.3.10 Precedence: list List-Id: Proxmox VE development discussion List-Help: List-Owner: List-Post: List-Subscribe: List-Unsubscribe: Hi Shannon, sorry for the delay in getting to review this. Some notes inline. For what it's worth, this seems to work as advertised. Code also looks okay to me in its current form. If you decide to act upon my feedback, I'll quickly go over it again and send another R-b for that as well. Tested-by: Lukas Wagner Reviewed-by: Lukas Wagner On Tue Jul 14, 2026 at 10:47 AM CEST, 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 > --- > proxmox-sendmail/src/lib.rs | 246 ++++++++++++++++++++++++++++++++---- > 1 file changed, 220 insertions(+), 26 deletions(-) > > diff --git a/proxmox-sendmail/src/lib.rs b/proxmox-sendmail/src/lib.rs > index b3ff56e5..a73e8eb7 100644 > --- a/proxmox-sendmail/src/lib.rs > +++ b/proxmox-sendmail/src/lib.rs > @@ -50,7 +50,10 @@ fn encode_base64_formatted>(raw: T) -> = String { > } > =20 > fn encode_filename_formatted(filename: &str) -> String { > - let encoded =3D format!("UTF-8''{}", utf8_percent_encode(filename, R= FC5987SET)); > + let encoded =3D format!( > + "UTF-8''{}", > + utf8_percent_encode(&filename.replace('\r', ""), RFC5987SET) > + ); > =20 > let format_prefix =3D |l| { > if let Some(l) =3D l { > @@ -63,6 +66,18 @@ fn encode_filename_formatted(filename: &str) -> String= { > format_encoded_text(&encoded, format_prefix, ";", 0, true) > } > =20 > +/// Strip bare CR bytes, than base64 encode. This is useful to make sure= we treat encoded (header) nit: should be 'then' instead of 'than' > +/// 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>(raw: T) -> String { > + let normalized: Vec =3D raw > + .as_ref() > + .iter() > + .filter_map(|b| (*b !=3D b'\r').then_some(*b)) > + .collect(); > + proxmox_base64::encode(&normalized) > +} > + > /// Helper function used to format text to not exceed line limits impose= d 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) =3D &self.name { > if !name.is_ascii() { > - format!( > - "=3D?utf-8?B?{}?=3D <{}>", > - proxmox_base64::encode(name), > - self.email > - ) > + format!("=3D?utf-8?B?{}?=3D <{}>", strip_cr_base64(name)= , self.email) > } else { > format!("{name} <{}>", self.email) > } > @@ -199,7 +210,7 @@ impl Attachment<'_> { > if self.filename.is_ascii() { > let _ =3D write!(attachment, "{}", &self.filename); > } else { > - let encoded =3D proxmox_base64::encode(&self.filename); > + let encoded =3D strip_cr_base64(&self.filename); > let prefix_fn =3D |line| { > if Some(0) =3D=3D line { > "=3D?utf-8?B?" > @@ -451,7 +462,10 @@ impl<'a> Mail<'a> { > =20 > /// Forwards an email message to a given list of recipients. > /// > - /// `message` must be compatible with ``sendmail`` (the message is p= iped into stdin unmodified). > + /// `message` must be compatible with ``sendmail``. The message is p= iped into `stdin` > + /// unmodified. One exception is stripping bare CR bytes and normali= zing 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 =3D "mail-forwarder")] > pub fn forward( > mailto: &[&str], > @@ -485,11 +499,27 @@ impl<'a> Mail<'a> { > .spawn() > .with_context(|| "could not spawn sendmail process")?; > =20 > + let capacity =3D message.iter().fold(0, |acc, e| match *e { > + b'\r' =3D> acc, > + b'\n' =3D> acc + 2, > + _ =3D> acc + 1, > + }); No hard feelings, this is not a hot code path after all, but I guess you could just multiply message.len() with some constant factor (e.g. 2) to get the initial capacity and avoid the double iteration. The message is likely not going to be larger than a couple hundred bytes or a few kilobytes, so the extra memory does not matter IMO. > + > + let mut normalized_message: Vec =3D Vec::with_capacity(capac= ity); > + > + for byte in message { > + match byte { > + b'\r' =3D> continue, > + b'\n' =3D> normalized_message.extend_from_slice(&[b'\r',= b'\n']), > + b =3D> normalized_message.push(*b), > + } > + } > + > sendmail_process > .stdin > .take() > .unwrap() > - .write_all(message) > + .write_all(&normalized_message) > .with_context(|| "couldn't write to sendmail stdin")?; > =20 > sendmail_process > @@ -520,7 +550,9 @@ impl<'a> Mail<'a> { > write!(mail, "\n--{file_boundary}--")?; > } > =20 > - 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")) > } > =20 > fn format_header( > @@ -561,7 +593,7 @@ impl<'a> Mail<'a> { > writeln!( > header, > "Subject: =3D?utf-8?B?{}?=3D", > - proxmox_base64::encode(&self.subject) > + strip_cr_base64(&self.subject) > )?; > } else { > writeln!(header, "Subject: {}", self.subject)?; > @@ -571,7 +603,7 @@ impl<'a> Mail<'a> { > writeln!( > header, > "From: =3D?utf-8?B?{}?=3D <{}>", > - proxmox_base64::encode(&self.mail_author), > + strip_cr_base64(&self.mail_author), > self.mail_from > )?; > } else { > @@ -606,17 +638,15 @@ impl<'a> Mail<'a> { > header.push_str("Auto-Submitted: auto-generated;\n"); > =20 > for (key, value) in &self.headers { > - // Strip CR/LF so an interpolated key or value cannot inject= extra header lines. > - let key =3D key.replace(['\r', '\n'], ""); > - let value =3D value.replace(['\r', '\n'], ""); > + // Strip LF so an interpolated key or value cannot inject ex= tra header lines. Bare CR > + // will be stripped for non-ASCII text before encoding and f= or ASCII text globally > + // before finishing formatting. > + let key =3D key.replace('\n', ""); > + let value =3D value.replace('\n', ""); I wonder whether it would be better to just return errors if there are control characters in Subject, headers, recipient... what do you think?=20