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)