add faq for smtp smuggling, fix bug around handling "\nX\n" for any X, reject bare carriage returns and possibly smtp-smuggling attempts

mox was already strict in its "\r\n.\r\n" handling for end-of-message in an
smtp transaction.

due to a mostly unrelated bug, sequences of "\nX\n", including "\n.\n" were
rejected with a "local processing error".

the sequence "\r\n.\n" dropped the dot, not necessarily a big problem, this is
unlikely to happen in a legimate transaction and the behaviour not
unreasonable.

we take this opportunity to reject all bare \r.  we detect all slightly
incorrect combinations of "\r\n.\r\n" with an error mentioning smtp smuggling,
in part to appease the tools checking for it.

smtp errors are 500 "bad syntax", and mention smtp smuggling.

README.md

@@ -193,6 +193,42 @@ Mox is still in early stages, and documentation is still limited. Please create
 an issue describing what is unclear or confusing, and we'll try to improve the
 documentation.
 
+## Is Mox affected by SMTP smuggling?
+
+Mox itself is not affected: it only treats "\r\n.\r\n" as SMTP end-of-message.
+But read on for caveats.
+
+SMTP smuggling exploits differences in handling by SMTP servers of: carriage
+returns (CR, or "\r"), newlines (line feeds, LF, "\n") in the context of "dot
+stuffing".  SMTP is a text-based protocol. An SMTP transaction to send a
+message is finalized with a "\r\n.\r\n" sequence. This sequence could occur in
+the message being transferred, so any verbatim "." at the start of a line in a
+message is "escaped" with another dot ("dot stuffing"), to not trigger the SMTP
+end-of-message. SMTP smuggling takes advantage of bugs in some mail servers
+that interpret other sequences than "\r\n.\r\n" as SMTP end-of-message. For
+example "\n.\n" or even "\r.\r", and perhaps even other magic character
+combinations.
+
+Before v0.0.9, mox accepted SMTP transactions with bare carriage returns
+(without newline) for compatibility with real-world email messages, considering
+them meaningless and therefore innocuous.
+
+Since v0.0.9, SMTP transactions with bare carriage returns are rejected.
+Sending messages with bare carriage returns to buggy mail servers can cause
+those mail servers to materialize non-existent messages. Now that mox rejects
+messages with bare carriage returns, sending a message through mox can no
+longer be used to trigger those bugs.
+
+Mox can still handle bare carriage returns in email messages, e.g. those
+imported from mbox files or Maildirs, or from messages added over IMAP. Mox
+still fixes up messages with bare newlines by adding the missing carriage
+returns.
+
+Before v0.0.9, an SMTP transaction for a message containing "\n.\n" would
+result in a non-specific error message, and "\r\n.\n" would result in the dot
+being dropped. Since v0.0.9, these sequences are rejected with a message
+mentioning SMTP smuggling.
+
 ## How do I import/export email?
 
 Use the import functionality on the accounts web page to import a zip/tgz with

message/writer.go

@@ -66,7 +66,7 @@ Top:
 		for i := o; i < len(buf); i++ {
 			if buf[i] == '\n' && (i > 0 && buf[i-1] != '\r' || i == 0 && origtail[2] != '\r') {
 				// Write buffer leading up to missing \r.
-				if i > o+1 {
+				if i > o {
 					n, err := w.writer.Write(buf[o:i])
 					if n > 0 {
 						wrote += n

message/writer_test.go

@@ -44,11 +44,11 @@ func TestMsgWriter(t *testing.T) {
 	// Check \n is replaced with \r\n.
 	var b strings.Builder
 	mw := NewWriter(&b)
-	msg := "key: value\n\nline1\r\nline2\n"
+	msg := "key: value\n\nline1\r\nline2\nx\n.\n"
 	_, err := mw.Write([]byte(msg))
 	tcheck(t, err, "write")
 	got := b.String()
-	exp := "key: value\r\n\r\nline1\r\nline2\r\n"
+	exp := "key: value\r\n\r\nline1\r\nline2\r\nx\r\n.\r\n"
 	if got != exp {
 		t.Fatalf("got %q, expected %q", got, exp)
 	}

smtp/data.go

@@ -7,16 +7,19 @@ import (
 	"io"
 )
 
+var ErrCRLF = errors.New("invalid bare carriage return or newline")
+
 var errMissingCRLF = errors.New("missing crlf at end of message")
 
 // DataWrite reads data (a mail message) from r, and writes it to smtp
 // connection w with dot stuffing, as required by the SMTP data command.
+//
+// Messages with bare carriage returns or bare newlines result in an error.
 func DataWrite(w io.Writer, r io.Reader) error {
 	// ../rfc/5321:2003
 
 	var prevlast, last byte = '\r', '\n' // Start on a new line, so we insert a dot if the first byte is a dot.
 	// todo: at least for smtp submission we should probably set a max line length, eg 1000 octects including crlf. ../rfc/5321:3512
-	// todo: at least for smtp submission or a pedantic mode, we should refuse messages with bare \r or bare \n.
 	buf := make([]byte, 8*1024)
 	for {
 		nr, err := r.Read(buf)
@@ -32,13 +35,25 @@ func DataWrite(w io.Writer, r io.Reader) error {
 				}
 				// Look for the next newline, or end of buffer.
 				n := 0
+				firstcr := -1
 				for n < len(p) {
 					c := p[n]
-					n++
 					if c == '\n' {
+						if firstcr < 0 {
+							// Bare newline.
+							return ErrCRLF
+						} else if firstcr != n-1 {
+							// Bare carriage return.
+							return ErrCRLF
+						}
+						n++
 						break
+					} else if c == '\r' && firstcr < 0 {
+						firstcr = n
 					}
+					n++
 				}
+
 				if _, err := w.Write(p[:n]); err != nil {
 					return err
 				}
@@ -70,12 +85,20 @@ var dotcrlf = []byte(".\r\n")
 
 // DataReader is an io.Reader that reads data from an SMTP DATA command, doing dot
 // unstuffing and returning io.EOF when a bare dot is received. Use NewDataReader.
+//
+// Bare carriage returns, and the sequences "[^\r]\n." and "\n.\n" result in an
+// error.
 type DataReader struct {
 	// ../rfc/5321:2003
 	r           *bufio.Reader
 	plast, last byte
 	buf         []byte // From previous read.
 	err         error  // Read error, for after r.buf is exhausted.
+
+	// When we see invalid combinations of CR and LF, we keep reading, and report an
+	// error at the final "\r\n.\r\n". We cannot just stop reading and return an error,
+	// the SMTP protocol would become out of sync.
+	badcrlf bool
 }
 
 // NewDataReader returns an initialized DataReader.
@@ -108,16 +131,36 @@ func (r *DataReader) Read(p []byte) (int, error) {
 			}
 		}
 		if len(r.buf) > 0 {
-			// We require crlf. A bare LF is not a line ending. ../rfc/5321:2032
-			// todo: we could return an error for a bare \n.
+			// Reject bare \r.
+			for i, c := range r.buf {
+				if c == '\r' && (i == len(r.buf) || r.buf[i+1] != '\n') {
+					r.badcrlf = true
+				}
+			}
+
+			// We require crlf. A bare LF is not a line ending for the end of the SMTP
+			// transaction. ../rfc/5321:2032
+			// Bare newlines are accepted as message data, unless around a bare dot. The SMTP
+			// server adds missing carriage returns. We don't reject bare newlines outright,
+			// real-world messages like that occur.
 			if r.plast == '\r' && r.last == '\n' {
 				if bytes.Equal(r.buf, dotcrlf) {
 					r.buf = nil
 					r.err = io.EOF
+					if r.badcrlf {
+						r.err = ErrCRLF
+					}
 					break
 				} else if r.buf[0] == '.' {
+					// Reject "\r\n.\n".
+					if len(r.buf) >= 2 && r.buf[1] == '\n' {
+						r.badcrlf = true
+					}
 					r.buf = r.buf[1:]
 				}
+			} else if r.last == '\n' && (bytes.HasPrefix(r.buf, []byte(".\n")) || bytes.HasPrefix(r.buf, []byte(".\r\n"))) {
+				// Reject "[^\r]\n.\n" and "[^\r]\n.\r\n"
+				r.badcrlf = true
 			}
 			n := len(r.buf)
 			if n > len(p) {

smtp/data_test.go

@@ -9,13 +9,24 @@ import (
 )
 
 func TestDataWrite(t *testing.T) {
-	if err := DataWrite(io.Discard, strings.NewReader("bad")); err == nil || !errors.Is(err, errMissingCRLF) {
-		t.Fatalf("got err %v, expected errMissingCRLF", err)
+	checkBad := func(s string, expErr error) {
+		t.Helper()
+		if err := DataWrite(io.Discard, strings.NewReader(s)); err == nil || !errors.Is(err, expErr) {
+			t.Fatalf("got err %v, expected %v", err, expErr)
 		}
-	if err := DataWrite(io.Discard, strings.NewReader(".")); err == nil || !errors.Is(err, errMissingCRLF) {
-		t.Fatalf("got err %v, expected errMissingCRLF", err)
 	}
 
+	checkBad("bad", errMissingCRLF)
+	checkBad(".", errMissingCRLF)
+	checkBad("bare \r is bad\r\n", ErrCRLF)
+	checkBad("bare \n is bad\r\n", ErrCRLF)
+	checkBad("\n.\nis bad\r\n", ErrCRLF)
+	checkBad("\r.\ris bad\r\n", ErrCRLF)
+	checkBad("\r\n.\ris bad\r\n", ErrCRLF)
+	checkBad("\r\n.\nis bad\r\n", ErrCRLF)
+	checkBad("\n.\ris bad\r\n", ErrCRLF)
+	checkBad("\n.\r\nis bad\r\n", ErrCRLF)
+
 	check := func(msg, want string) {
 		t.Helper()
 		w := &strings.Builder{}
@@ -58,13 +69,15 @@ func TestDataReader(t *testing.T) {
 		return wrote, nil
 	}
 
-	check := func(data, want string) {
+	check := func(data, want string, expErr error) {
 		t.Helper()
 
 		s := &strings.Builder{}
 		dr := NewDataReader(bufio.NewReader(strings.NewReader(data)))
 		if _, err := io.Copy(s, dr); err != nil {
-			t.Fatalf("got err %v", err)
+			if expErr == nil || !errors.Is(err, expErr) {
+				t.Fatalf("got err %v, expected %v", err, expErr)
+			}
 		} else if got := s.String(); got != want {
 			t.Fatalf("got %q, expected %q, for %q", got, want, data)
 		}
@@ -72,16 +85,43 @@ func TestDataReader(t *testing.T) {
 		s = &strings.Builder{}
 		dr = NewDataReader(bufio.NewReader(strings.NewReader(data)))
 		if _, err := smallCopy(s, dr); err != nil {
-			t.Fatalf("got err %v", err)
+			if expErr == nil || !errors.Is(err, expErr) {
+				t.Fatalf("got err %v, expected %v", err, expErr)
+			}
 		} else if got := s.String(); got != want {
 			t.Fatalf("got %q, expected %q, for %q", got, want, data)
 		}
 	}
 
-	check("test\r\n.\r\n", "test\r\n")
-	check(".\r\n", "")
-	check(".test\r\n.\r\n", "test\r\n") // Unnecessary dot, but valid in SMTP.
-	check("..test\r\n.\r\n", ".test\r\n")
+	check("test\r\n.\r\n", "test\r\n", nil)
+	check(".\r\n", "", nil)
+	check(".test\r\n.\r\n", "test\r\n", nil) // Unnecessary dot, but valid in SMTP.
+	check("..test\r\n.\r\n", ".test\r\n", nil)
+
+	check("..test\ntest.\n\r\n.\r\n", ".test\ntest.\n\r\n", nil) // Bare newlines are allowed.
+	check("..test\ntest\n", "", io.ErrUnexpectedEOF)             // Missing end-of-message.
+
+	// Bare \r is rejected.
+	check("bare \r is rejected\r\n.\r\n", "", ErrCRLF)
+	check("bad:\r.\ris rejected\r\n.\r\n", "", ErrCRLF)
+	check("bad:\r.\nis rejected\r\n.\r\n", "", ErrCRLF)
+
+	// Suspicious bare newlines around a dot are rejected.
+	check("bad:\n.\nis rejected\r\n.\r\n", "", ErrCRLF)
+	check("bad:\n.\r\nis rejected\r\n.\r\n", "", ErrCRLF)
+	check("bad:\r\n.\nis rejected\r\n.\r\n", "", ErrCRLF)
+
+	// Suspicious near-smtp-endings at start of message.
+	check(".\ris rejected\r\n.\r\n", "", ErrCRLF)
+	check(".\nis rejected\r\n.\r\n", "", ErrCRLF)
+	check("\n.\ris rejected\r\n.\r\n", "", ErrCRLF)
+	check("\r.\ris rejected\r\n.\r\n", "", ErrCRLF)
+	check("\n.\nis rejected\r\n.\r\n", "", ErrCRLF)
+	check("\r.\nis rejected\r\n.\r\n", "", ErrCRLF)
+	check("\r.\r\nis rejected\r\n.\r\n", "", ErrCRLF)
+	check("\n.\r\nis rejected\r\n.\r\n", "", ErrCRLF)
+	check("\r\n.\ris rejected\r\n.\r\n", "", ErrCRLF)
+	check("\r\n.\nis rejected\r\n.\r\n", "", ErrCRLF)
 
 	s := &strings.Builder{}
 	dr := NewDataReader(bufio.NewReader(strings.NewReader("no end")))

smtpserver/server.go

@@ -1650,6 +1650,11 @@ func (c *conn) cmdData(p *parser) {
 			panic(fmt.Errorf("remote sent too much DATA: %w", errIO))
 		}
 
+		if errors.Is(err, smtp.ErrCRLF) {
+			c.writecodeline(smtp.C500BadSyntax, smtp.SeProto5Syntax2, fmt.Sprintf("invalid bare \\r or \\n, may be smtp smuggling (%s)", mox.ReceivedID(c.cid)), err)
+			return
+		}
+
 		// Something is failing on our side. We want to let remote know. So write an error response,
 		// then discard the remaining data so the remote client is more likely to see our
 		// response. Our write is synchronous, there is a risk no window/buffer space is

smtpserver/server_test.go

@@ -141,6 +141,34 @@ func (ts *testserver) close() {
 }
 
 func (ts *testserver) run(fn func(helloErr error, client *smtpclient.Client)) {
+	ts.runRaw(func(conn net.Conn) {
+		ts.t.Helper()
+
+		auth := ts.auth
+		if auth == nil && ts.user != "" {
+			auth = func(mechanisms []string, cs *tls.ConnectionState) (sasl.Client, error) {
+				return sasl.NewClientPlain(ts.user, ts.pass), nil
+			}
+		}
+
+		ourHostname := mox.Conf.Static.HostnameDomain
+		remoteHostname := dns.Domain{ASCII: "mox.example"}
+		opts := smtpclient.Opts{
+			Auth:    auth,
+			RootCAs: mox.Conf.Static.TLS.CertPool,
+		}
+		log := pkglog.WithCid(ts.cid - 1)
+		client, err := smtpclient.New(ctxbg, log.Logger, conn, ts.tlsmode, ts.tlspkix, ourHostname, remoteHostname, opts)
+		if err != nil {
+			conn.Close()
+		} else {
+			defer client.Close()
+		}
+		fn(err, client)
+	})
+}
+
+func (ts *testserver) runRaw(fn func(clientConn net.Conn)) {
 	ts.t.Helper()
 
 	ts.cid += 2
@@ -159,27 +187,7 @@ func (ts *testserver) run(fn func(helloErr error, client *smtpclient.Client)) {
 		close(serverdone)
 	}()
 
-	auth := ts.auth
-	if auth == nil && ts.user != "" {
-		auth = func(mechanisms []string, cs *tls.ConnectionState) (sasl.Client, error) {
-			return sasl.NewClientPlain(ts.user, ts.pass), nil
-		}
-	}
-
-	ourHostname := mox.Conf.Static.HostnameDomain
-	remoteHostname := dns.Domain{ASCII: "mox.example"}
-	opts := smtpclient.Opts{
-		Auth:    auth,
-		RootCAs: mox.Conf.Static.TLS.CertPool,
-	}
-	log := pkglog.WithCid(ts.cid - 1)
-	client, err := smtpclient.New(ctxbg, log.Logger, clientConn, ts.tlsmode, ts.tlspkix, ourHostname, remoteHostname, opts)
-	if err != nil {
-		clientConn.Close()
-	} else {
-		defer client.Close()
-	}
-	fn(err, client)
+	fn(clientConn)
 }
 
 // Just a cert that appears valid. SMTP client will not verify anything about it
@@ -1582,3 +1590,72 @@ test email
 		}
 	})
 }
+
+func TestSmuggle(t *testing.T) {
+	resolver := dns.MockResolver{
+		A: map[string][]string{
+			"example.org.": {"127.0.0.10"}, // For mx check.
+		},
+		PTR: map[string][]string{
+			"127.0.0.10": {"example.org."}, // For iprev check.
+		},
+	}
+	ts := newTestServer(t, filepath.FromSlash("../testdata/smtpsmuggle/mox.conf"), resolver)
+	ts.tlsmode = smtpclient.TLSSkip
+	defer ts.close()
+
+	test := func(data string) {
+		t.Helper()
+
+		ts.runRaw(func(conn net.Conn) {
+			t.Helper()
+
+			ourHostname := mox.Conf.Static.HostnameDomain
+			remoteHostname := dns.Domain{ASCII: "mox.example"}
+			opts := smtpclient.Opts{
+				RootCAs: mox.Conf.Static.TLS.CertPool,
+			}
+			log := pkglog.WithCid(ts.cid - 1)
+			_, err := smtpclient.New(ctxbg, log.Logger, conn, ts.tlsmode, ts.tlspkix, ourHostname, remoteHostname, opts)
+			tcheck(t, err, "smtpclient")
+			defer conn.Close()
+
+			write := func(s string) {
+				_, err := conn.Write([]byte(s))
+				tcheck(t, err, "write")
+			}
+
+			readPrefixLine := func(prefix string) string {
+				t.Helper()
+				buf := make([]byte, 512)
+				n, err := conn.Read(buf)
+				tcheck(t, err, "read")
+				s := strings.TrimRight(string(buf[:n]), "\r\n")
+				if !strings.HasPrefix(s, prefix) {
+					t.Fatalf("got smtp response %q, expected line with prefix %q", s, prefix)
+				}
+				return s
+			}
+
+			write("MAIL FROM:<remote@example.org>\r\n")
+			readPrefixLine("2")
+			write("RCPT TO:<mjl@mox.example>\r\n")
+			readPrefixLine("2")
+
+			write("DATA\r\n")
+			readPrefixLine("3")
+			write("\r\n") // Empty header.
+			write(data)
+			write("\r\n.\r\n") // End of message.
+			line := readPrefixLine("5")
+			if !strings.Contains(line, "smug") {
+				t.Errorf("got 5xx error with message %q, expected error text containing smug", line)
+			}
+		})
+	}
+
+	test("\r\n.\n")
+	test("\n.\n")
+	test("\r.\r")
+	test("\n.\r\n")
+}

testdata/smtpsmuggle/domains.conf

@@ -0,0 +1,7 @@
+Domains:
+	mox.example: nil
+Accounts:
+	mjl:
+		Domain: mox.example
+		Destinations:
+			mjl@mox.example: nil

testdata/smtpsmuggle/mox.conf

@@ -0,0 +1,9 @@
+DataDir: data
+User: 1000
+LogLevel: trace
+Hostname: mox.example
+Postmaster:
+	Account: mjl
+	Mailbox: postmaster
+Listeners:
+	local: nil

webmail/api.go

@@ -233,6 +233,18 @@ func (w Webmail) MessageSubmit(ctx context.Context, m SubmitMessage) {
 
 	// todo: consider making this an HTTP POST, so we can upload as regular form, which is probably more efficient for encoding for the client and we can stream the data in.
 
+	// Prevent any accidental control characters, or attempts at getting bare \r or \n
+	// into messages.
+	for _, l := range [][]string{m.To, m.Cc, m.Bcc, {m.From, m.Subject, m.ReplyTo, m.UserAgent}} {
+		for _, s := range l {
+			for _, c := range s {
+				if c < 0x20 {
+					xcheckuserf(ctx, errors.New("control characters not allowed"), "checking header values")
+				}
+			}
+		}
+	}
+
 	reqInfo := ctx.Value(requestInfoCtxKey).(requestInfo)
 	log := pkglog.WithContext(ctx).With(slog.String("account", reqInfo.AccountName))
 	acc, err := store.OpenAccount(log, reqInfo.AccountName)