From 1f9b640d9ae8dbd92d90fa3a5e26caaa67411eb8 Mon Sep 17 00:00:00 2001 From: Mechiel Lukkien Date: Mon, 1 Jan 2024 18:30:31 +0100 Subject: [PATCH] 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 | 36 +++++++++ message/writer.go | 2 +- message/writer_test.go | 4 +- smtp/data.go | 51 ++++++++++++- smtp/data_test.go | 64 +++++++++++++--- smtpserver/server.go | 5 ++ smtpserver/server_test.go | 119 ++++++++++++++++++++++++------ testdata/smtpsmuggle/domains.conf | 7 ++ testdata/smtpsmuggle/mox.conf | 9 +++ webmail/api.go | 12 +++ 10 files changed, 269 insertions(+), 40 deletions(-) create mode 100644 testdata/smtpsmuggle/domains.conf create mode 100644 testdata/smtpsmuggle/mox.conf diff --git a/README.md b/README.md index 15012a2..1ba362e 100644 --- a/README.md +++ b/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 diff --git a/message/writer.go b/message/writer.go index d0606df..021150f 100644 --- a/message/writer.go +++ b/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 diff --git a/message/writer_test.go b/message/writer_test.go index 65f1cae..ff18b49 100644 --- a/message/writer_test.go +++ b/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) } diff --git a/smtp/data.go b/smtp/data.go index bc008f0..c3f293b 100644 --- a/smtp/data.go +++ b/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) { diff --git a/smtp/data_test.go b/smtp/data_test.go index 085a1d2..ac0496c 100644 --- a/smtp/data_test.go +++ b/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) - } - if err := DataWrite(io.Discard, strings.NewReader(".")); 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) + } } + 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"))) diff --git a/smtpserver/server.go b/smtpserver/server.go index 890b38b..f05a9f0 100644 --- a/smtpserver/server.go +++ b/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 diff --git a/smtpserver/server_test.go b/smtpserver/server_test.go index e1e79b2..1608fe2 100644 --- a/smtpserver/server_test.go +++ b/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:\r\n") + readPrefixLine("2") + write("RCPT TO:\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") +} diff --git a/testdata/smtpsmuggle/domains.conf b/testdata/smtpsmuggle/domains.conf new file mode 100644 index 0000000..449f11e --- /dev/null +++ b/testdata/smtpsmuggle/domains.conf @@ -0,0 +1,7 @@ +Domains: + mox.example: nil +Accounts: + mjl: + Domain: mox.example + Destinations: + mjl@mox.example: nil diff --git a/testdata/smtpsmuggle/mox.conf b/testdata/smtpsmuggle/mox.conf new file mode 100644 index 0000000..e1286db --- /dev/null +++ b/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 diff --git a/webmail/api.go b/webmail/api.go index a97e720..2490010 100644 --- a/webmail/api.go +++ b/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)