diff --git a/api/src/lib.rs b/api/src/lib.rs index bd14216..566abe8 100644 --- a/api/src/lib.rs +++ b/api/src/lib.rs @@ -2259,6 +2259,24 @@ pub trait Service: Send { fn on_command(&mut self, from: &Sender, args: &[&str], ctx: &mut ServiceCtx, net: &dyn NetView, store: &mut dyn Store); } +/// True if `addr` is safe to store and to embed in an SMTP header / hand to +/// `sendmail -t`: no control characters (notably CR/LF, which would inject mail +/// headers), no spaces, a single `@` with non-empty local and domain parts, and +/// a sane length. Deliberately permissive on the charset otherwise. +pub fn valid_email(addr: &str) -> bool { + if addr.is_empty() || addr.len() > 254 { + return false; + } + if addr.chars().any(|c| c.is_control() || c == ' ') { + return false; + } + let mut parts = addr.split('@'); + matches!( + (parts.next(), parts.next(), parts.next()), + (Some(local), Some(domain), None) if !local.is_empty() && !domain.is_empty() + ) +} + // Reject a look-alike / mixed-script registration name (nick or channel): the // impersonation trick that a message-content filter never sees at the registration // seam. Returns a rejection reason, or None if the name is fine. Genuine @@ -2760,4 +2778,21 @@ mod help_tests { help("Svc", &sender(), &mut ctx, "b", T, Some("nope")); assert!(texts(&ctx)[0].contains("No help")); } + + #[test] + fn valid_email_rejects_header_injection() { + assert!(valid_email("alice@example.com")); + assert!(valid_email("a.b+c@sub.domain.co.uk")); + // CR/LF (SMTP header injection), spaces and other controls are rejected. + assert!(!valid_email("victim@example.com\r\nBcc: attacker@evil.com")); + assert!(!valid_email("victim@example.com\nBcc: x@evil.com")); + assert!(!valid_email("a b@example.com")); + assert!(!valid_email("bad\temail@example.com")); + // Structural nonsense is rejected too. + assert!(!valid_email("")); + assert!(!valid_email("no-at-sign")); + assert!(!valid_email("two@at@signs.com")); + assert!(!valid_email("@nolocal.com")); + assert!(!valid_email("nodomain@")); + } } diff --git a/modules/nickserv/src/register.rs b/modules/nickserv/src/register.rs index eef4fb7..431d81d 100644 --- a/modules/nickserv/src/register.rs +++ b/modules/nickserv/src/register.rs @@ -22,6 +22,12 @@ pub fn handle(me: &str, from: &Sender, args: &[&str], ctx: &mut ServiceCtx, db: } } let email = args.get(2).map(|s| s.to_string()); + if let Some(addr) = &email { + if !echo_api::valid_email(addr) { + ctx.notice(me, from.uid, "That doesn't look like a valid email address."); + return; + } + } ctx.defer_register(from.nick, *password, email, RegReply::NickServ { agent: me.to_string(), uid: from.uid.to_string(), diff --git a/modules/nickserv/src/set.rs b/modules/nickserv/src/set.rs index 9af49d8..e06b6d4 100644 --- a/modules/nickserv/src/set.rs +++ b/modules/nickserv/src/set.rs @@ -30,6 +30,10 @@ pub fn handle(me: &str, from: &Sender, args: &[&str], ctx: &mut ServiceCtx, db: let email = args.get(2).map(|s| s.to_string()); let cleared = email.is_none(); if let Some(addr) = &email { + if !echo_api::valid_email(addr) { + ctx.notice(me, from.uid, "That doesn't look like a valid email address."); + return; + } if db.is_forbidden(ForbidKind::Email, addr).is_some() { ctx.notice(me, from.uid, "That email address is forbidden by network policy. Use a different one."); return; diff --git a/src/engine/register.rs b/src/engine/register.rs index 1ab918f..a8cb6c7 100644 --- a/src/engine/register.rs +++ b/src/engine/register.rs @@ -30,6 +30,12 @@ impl Engine { // confusable check), applied to every authority write so it can't create a name // IRC would have rejected. fn authority_name_ok(&self, name: &str) -> Result<(), AuthorityStatus> { + // A control char in the name would inject into the confirmation email's + // Subject/body (the name is interpolated there); IRC nicks can't carry one, + // but a website-provided name must be checked. + if name.is_empty() || name.chars().any(char::is_control) { + return Err(AuthorityStatus::Invalid); + } if self.db.is_forbidden(echo_api::ForbidKind::Nick, name).is_some() { return Err(AuthorityStatus::Invalid); } @@ -49,6 +55,11 @@ impl Engine { return status; } } + if let Some(addr) = &email { + if !echo_api::valid_email(addr) { + return AuthorityStatus::Invalid; + } + } match self.db.provision_account(name, scram256, scram512, email) { Ok(()) => AuthorityStatus::Ok, Err(RegError::Exists) => AuthorityStatus::AlreadyExists, @@ -63,9 +74,10 @@ impl Engine { if self.db.registrations_frozen() || self.db.readonly() { return AuthorityStatus::Invalid; } - // The IRC REGISTER rejects a FORBIDden email; a website write must too. + // The IRC REGISTER rejects a FORBIDden email; a website write must too — + // and must reject a malformed/CRLF-bearing one before it reaches a mail header. if let Some(addr) = &email { - if self.db.is_forbidden(echo_api::ForbidKind::Email, addr).is_some() { + if !echo_api::valid_email(addr) || self.db.is_forbidden(echo_api::ForbidKind::Email, addr).is_some() { return AuthorityStatus::Invalid; } } @@ -127,6 +139,9 @@ impl Engine { pub fn authority_set_email(&mut self, account: &str, email: Option) -> AuthorityStatus { if let Some(addr) = &email { + if !echo_api::valid_email(addr) { + return AuthorityStatus::Invalid; // no CRLF/garbage into a mail header + } if self.db.is_forbidden(echo_api::ForbidKind::Email, addr).is_some() { return AuthorityStatus::Invalid; // same email-forbid policy as IRC SET } diff --git a/src/grpc.rs b/src/grpc.rs index b12f09a..0d7a2e0 100644 --- a/src/grpc.rs +++ b/src/grpc.rs @@ -730,6 +730,26 @@ mod tests { assert_eq!(engine.lock().await.directory_snapshot().0[0].email, None, "empty string clears the email"); } + // The website authority path can't smuggle a CRLF into a mail header via the + // stored email or the account name (SMTP header injection through sendmail -t). + #[tokio::test] + async fn authority_rejects_crlf_in_email_and_name() { + let (engine, _tx) = engine_with("acct-inject"); + let svc = accounts_svc(engine.clone()); + svc.register(authed(RegisterRequest { name: "frank".into(), password: "x".into(), email: String::new() }, "t")).await.unwrap(); + + let bad = "frank@example.com\r\nBcc: attacker@evil.com".to_string(); + let se = svc.set_email(authed(SetEmailRequest { account: "frank".into(), email: bad.clone() }, "t")).await.unwrap().into_inner(); + assert_eq!(se.status, PbStatus::Invalid as i32, "CRLF email refused on SET"); + assert_eq!(engine.lock().await.directory_snapshot().0[0].email, None, "poisoned email not stored"); + + let reg = svc.register(authed(RegisterRequest { name: "mallory".into(), password: "x".into(), email: bad }, "t")).await.unwrap().into_inner(); + assert_eq!(reg.status, PbStatus::Invalid as i32, "CRLF email refused on register"); + + let name = svc.register(authed(RegisterRequest { name: "eve\r\nBcc: x@evil.com".into(), password: "x".into(), email: String::new() }, "t")).await.unwrap().into_inner(); + assert_eq!(name.status, PbStatus::Invalid as i32, "control char in the account name refused"); + } + // Full round trip through the real confirmation-email pipeline: register with // email confirmation on, capture the code echo actually emails out (never // returned by the RPC — proving the caller can't skip owning the inbox), then diff --git a/src/link.rs b/src/link.rs index 7d3175d..b190e10 100644 --- a/src/link.rs +++ b/src/link.rs @@ -271,10 +271,20 @@ fn dispatch_email(email: &Option, to: String, subject: Str return tracing::warn!(%to, "email requested but not configured"); }; tokio::spawn(async move { + // Hard backstop against header injection: never let a control char reach + // the raw SMTP header block, whatever the caller did. Bodies (text/html) + // may legitimately contain newlines, so only the header values are checked. + if [to.as_str(), subject.as_str(), email.from.as_str()].iter().any(|s| s.chars().any(char::is_control)) { + return tracing::warn!(%to, "refusing to send an email with control characters in a header"); + } let headers = format!("From: {}\r\nTo: {to}\r\nSubject: {subject}\r\nMIME-Version: 1.0\r\n", email.from); let msg = match html { Some(html) => { - let b = "echo-alt-boundary-x9"; + // Random per-message boundary so body content can never be mistaken + // for the delimiter. + let mut rb = [0u8; 8]; + rand_core::RngCore::fill_bytes(&mut rand_core::OsRng, &mut rb); + let b = format!("echo-bnd-{:016x}", u64::from_le_bytes(rb)); format!( "{headers}Content-Type: multipart/alternative; boundary=\"{b}\"\r\n\r\n\ --{b}\r\nContent-Type: text/plain; charset=utf-8\r\n\r\n{text}\r\n\