smtpsrv: Strict CRLF enforcement in DATA contents

The RFCs are very clear that in DATA contents:

> CR and LF MUST only occur together as CRLF; they MUST NOT appear
> independently in the body.

https://www.rfc-editor.org/rfc/rfc5322#section-2.3
https://www.rfc-editor.org/rfc/rfc5321#section-2.3.8

Allowing "independent" CR and LF can cause a number of problems.

In particular, there is a new "SMTP smuggling attack" published recently
that involves the server incorrectly parsing the end of DATA marker
`\r\n.\r\n`, which an attacker can exploit to impersonate a server when
email is transmitted server-to-server.

https://www.postfix.org/smtp-smuggling.html
https://sec-consult.com/blog/detail/smtp-smuggling-spoofing-e-mails-worldwide/

Currently, chasquid is vulnerable to this attack, because Go's standard
libraries net/textproto and net/mail do not enforce CRLF strictly.

This patch fixes the problem by introducing a new "dot reader" function
that strictly enforces CRLF when reading dot-terminated data, used in
the DATA input processing.

When an invalid newline terminator is found, the connection is aborted
immediately because we cannot safely recover from that state.

Note this also means the internal message representation will be using
\r\n, and thus the MDA and hook invocations will now see different (but
more compliant) line endings.

See #47 for more details and
discussion.

internal/smtpsrv/conn.go

@@ -11,7 +11,6 @@ import (
 	"math/rand"
 	"net"
 	"net/mail"
-	"net/textproto"
 	"os"
 	"os/exec"
 	"strconv"
@@ -312,6 +311,9 @@ loop:
 			if err != nil {
 				break
 			}
+		} else if code < 0 {
+			// Negative code means that we have to break the connection.
+			break
 		}
 	}
 
@@ -638,19 +640,19 @@ func (c *Conn) DATA(params string) (code int, msg string) {
 	// one, we don't want the command timeout to interfere.
 	c.conn.SetDeadline(c.deadline)
 
-	// Create a dot reader, limited to the maximum size.
-	dotr := textproto.NewReader(bufio.NewReader(
-		io.LimitReader(c.reader, c.maxDataSize))).DotReader()
-	c.data, err = io.ReadAll(dotr)
+	// Read the data. Enforce CRLF correctness, and maximum size.
+	c.data, err = readUntilDot(c.reader, int(c.maxDataSize))
 	if err != nil {
-		if err == io.ErrUnexpectedEOF {
-			// Message is too big already. But we need to keep reading until we see
-			// the "\r\n.\r\n", otherwise we will treat the remanent data that
-			// the user keeps sending as commands, and that's a security
-			// issue.
-			readUntilDot(c.reader)
+		if err == errMessageTooLarge {
+			// Message is too big; excess data has already been discarded.
 			return 552, "5.3.4 Message too big"
 		}
+		if err == errInvalidLineEnding {
+			// We can't properly recover from this, so we have to drop the
+			// connection.
+			c.writeResponse(521, "5.5.2 Error reading DATA: invalid line ending")
+			return -1, "Invalid line ending, closing connection"
+		}
 		return 554, fmt.Sprintf("5.4.0 Error reading DATA: %v", err)
 	}
 
@@ -952,24 +954,6 @@ func boolToStr(b bool) string {
 	return "0"
 }
 
-func readUntilDot(r *bufio.Reader) {
-	prevMore := false
-	for {
-		// The reader will not read more than the size of the buffer,
-		// so this doesn't cause increased memory consumption.
-		// The reader's data deadline will prevent this from continuing
-		// forever.
-		l, more, err := r.ReadLine()
-		if err != nil {
-			break
-		}
-		if !more && !prevMore && string(l) == "." {
-			break
-		}
-		prevMore = more
-	}
-}
-
 // STARTTLS SMTP command handler.
 func (c *Conn) STARTTLS(params string) (code int, msg string) {
 	if c.onTLS {

internal/smtpsrv/conn_test.go

@@ -1,10 +1,8 @@
 package smtpsrv
 
 import (
-	"bufio"
 	"net"
 	"os"
-	"strings"
 	"testing"
 
 	"blitiri.com.ar/go/chasquid/internal/domaininfo"
@@ -87,57 +85,6 @@ func TestIsHeader(t *testing.T) {
 	}
 }
 
-func TestReadUntilDot(t *testing.T) {
-	// This must be > than the minimum buffer size for bufio.Reader, which
-	// unfortunately is not available to us. The current value is 16, these
-	// tests will break if it gets increased, and the nonfinal cases will need
-	// to be adjusted.
-	size := 20
-	xs := "12345678901234567890"
-
-	final := []string{
-		"", ".", "..",
-		".\r\n", "\r\n.", "\r\n.\r\n",
-		".\n", "\n.", "\n.\n",
-		".\r", "\r.", "\r.\r",
-		xs + "\r\n.\r\n",
-		xs + "1234\r\n.\r\n",
-		xs + xs + "\r\n.\r\n",
-		xs + xs + xs + "\r\n.\r\n",
-		xs + "." + xs + "\n.",
-		xs + ".\n" + xs + "\n.",
-	}
-	for _, s := range final {
-		t.Logf("testing %q", s)
-		buf := bufio.NewReaderSize(strings.NewReader(s), size)
-		readUntilDot(buf)
-		if r := buf.Buffered(); r != 0 {
-			t.Errorf("%q: there are %d remaining bytes", s, r)
-		}
-	}
-
-	nonfinal := []struct {
-		s string
-		r int
-	}{
-		{".\na", 1},
-		{"\n.\na", 1},
-		{"\n.\nabc", 3},
-		{"\n.\n12345678", 8},
-		{"\n.\n" + xs, size - 3},
-		{"\n.\n" + xs + xs, size - 3},
-		{"\n.\n.\n", 2},
-	}
-	for _, c := range nonfinal {
-		t.Logf("testing %q", c.s)
-		buf := bufio.NewReaderSize(strings.NewReader(c.s), size)
-		readUntilDot(buf)
-		if r := buf.Buffered(); r != c.r {
-			t.Errorf("%q: expected %d remaining bytes, got %d", c.s, c.r, r)
-		}
-	}
-}
-
 func TestAddrLiteral(t *testing.T) {
 	// TCP addresses.
 	casesTCP := []struct {

internal/smtpsrv/dotreader.go

@@ -0,0 +1,96 @@
+package smtpsrv
+
+import (
+	"bufio"
+	"bytes"
+	"errors"
+	"io"
+)
+
+var (
+	errMessageTooLarge   = errors.New("message too large")
+	errInvalidLineEnding = errors.New("invalid line ending")
+)
+
+// readUntilDot reads from r until it encounters a dot-terminated line, or we
+// read max bytes.
+func readUntilDot(r *bufio.Reader, max int) ([]byte, error) {
+	buf := make([]byte, 0, 1024)
+	n := 0
+
+	// Little state machine.
+	const (
+		prevOther = iota
+		prevCR
+		prevCRLF
+	)
+	// Start as if we just came from a '\r\n'; that way we avoid the need
+	// for special-casing the dot-stuffing at the very beginning.
+	prev := prevCRLF
+	last4 := make([]byte, 4)
+	skip := false
+
+loop:
+	for {
+		b, err := r.ReadByte()
+		if err == io.EOF {
+			return buf, io.ErrUnexpectedEOF
+		} else if err != nil {
+			return buf, err
+		}
+		n++
+
+		switch b {
+		case '\r':
+			if prev == prevCR {
+				return buf, errInvalidLineEnding
+			}
+			prev = prevCR
+		case '\n':
+			if prev != prevCR {
+				return buf, errInvalidLineEnding
+			}
+			// If we come from a '\r\n.\r', we're done.
+			if bytes.Equal(last4, []byte("\r\n.\r")) {
+				break loop
+			}
+
+			// If we are only starting and see ".\r\n", we're also done; in
+			// that case the message is empty.
+			if n == 3 && bytes.Equal(last4, []byte("\x00\x00.\r")) {
+				return []byte{}, nil
+			}
+			prev = prevCRLF
+		default:
+			if prev == prevCR {
+				return buf, errInvalidLineEnding
+			}
+			if b == '.' && prev == prevCRLF {
+				// We come from "\r\n" and got a "."; as per dot-stuffing
+				// rules, we should skip that '.' in the output.
+				// https://www.rfc-editor.org/rfc/rfc5321#section-4.5.2
+				skip = true
+			}
+			prev = prevOther
+		}
+
+		// Keep the last 4 bytes separately, because they may not be in buf on
+		// messages that are too large.
+		copy(last4, last4[1:])
+		last4[3] = b
+
+		if len(buf) < max && !skip {
+			buf = append(buf, b)
+		}
+		skip = false
+	}
+
+	if n > max {
+		return buf, errMessageTooLarge
+	}
+
+	// If we made it this far, buf ends in "\r\n\r" (because we skipped the
+	// '.' due to dot-stuffing); do not include the final "\r" in the returned
+	// buffer.
+	return buf[:len(buf)-1], nil
+}

internal/smtpsrv/dotreader_test.go

@@ -0,0 +1,89 @@
+package smtpsrv
+
+import (
+	"bufio"
+	"bytes"
+	"io"
+	"strings"
+	"testing"
+)
+
+func TestReadUntilDot(t *testing.T) {
+	cases := []struct {
+		input   string
+		max     int
+		want    string
+		wantErr error
+	}{
+		// EOF before any input -> unexpected EOF.
+		{"", 0, "", io.ErrUnexpectedEOF},
+		{"", 1, "", io.ErrUnexpectedEOF},
+
+		// EOF after exceeding max -> unexpected EOF.
+		{"abcdef", 2, "ab", io.ErrUnexpectedEOF},
+
+		// \n at the beginning of the buffer are just as invalid, and the
+		// error takes precedence over the unexpected EOF.
+		{"\n", 0, "", errInvalidLineEnding},
+		{"\n", 1, "", errInvalidLineEnding},
+		{"\n", 2, "", errInvalidLineEnding},
+		{"\n\r\n.\r\n", 10, "", errInvalidLineEnding},
+
+		// \r and then EOF -> unexpected EOF, because we never had a chance to
+		// assess if the line ending is valid or not.
+		{"\r", 2, "\r", io.ErrUnexpectedEOF},
+
+		// Lonely \r -> invalid line ending.
+		{"abc\rdef", 10, "abc\r", errInvalidLineEnding},
+		{"abc\r\rdef", 10, "abc\r", errInvalidLineEnding},
+
+		// Lonely \n -> invalid line ending.
+		{"abc\ndef", 10, "abc", errInvalidLineEnding},
+
+		// Various valid cases.
+		{"abc\r\n.\r\n", 10, "abc\r\n", nil},
+		{"\r\n.\r\n", 10, "\r\n", nil},
+
+		// Start with the final dot - the smallest "message" (empty).
+		{".\r\n", 10, "", nil},
+
+		// Max bytes reached -> message too large.
+		{"abc\r\n.\r\n", 5, "abc\r\n", errMessageTooLarge},
+		{"abcdefg\r\n.\r\n", 5, "abcde", errMessageTooLarge},
+		{"ab\r\ncdefg\r\n.\r\n", 5, "ab\r\nc", errMessageTooLarge},
+
+		// Dot-stuffing.
+		// https://www.rfc-editor.org/rfc/rfc5321#section-4.5.2
+		{"abc\r\n.def\r\n.\r\n", 20, "abc\r\ndef\r\n", nil},
+		{"abc\r\n..def\r\n.\r\n", 20, "abc\r\n.def\r\n", nil},
+		{"abc\r\n..\r\n.\r\n", 20, "abc\r\n.\r\n", nil},
+		{".x\r\n.\r\n", 20, "x\r\n", nil},
+		{"..\r\n.\r\n", 20, ".\r\n", nil},
+	}
+
+	for i, c := range cases {
+		r := bufio.NewReader(strings.NewReader(c.input))
+		got, err := readUntilDot(r, c.max)
+		if err != c.wantErr {
+			t.Errorf("case %d %q: got error %v, want %v", i, c.input, err, c.wantErr)
+		}
+		if !bytes.Equal(got, []byte(c.want)) {
+			t.Errorf("case %d %q: got %q, want %q", i, c.input, got, c.want)
+		}
+	}
+}
+
+type badBuffer bytes.Buffer
+
+func (b *badBuffer) Read(p []byte) (int, error) {
+	// Return an arbitrary non-EOF error for testing.
+	return 0, io.ErrNoProgress
+}
+
+func TestReadUntilDotReadError(t *testing.T) {
+	r := bufio.NewReader(&badBuffer{})
+	_, err := readUntilDot(r, 10)
+	if err != io.ErrNoProgress {
+		t.Errorf("got error %v, want %v", err, io.ErrNoProgress)
+	}
+}

test/t-12-minor_dialogs/bad_data_dot.cmy

@@ -0,0 +1,30 @@
+
+c tcp_connect localhost:1025
+
+c <~ 220
+c -> EHLO localhost
+c <... 250 HELP
+c -> MAIL FROM: <>
+c <~ 250
+c -> RCPT TO: user@testserver
+c <~ 250
+c -> DATA
+c <~ 354
+c -> From: Mailer daemon <[email protected]>
+c -> Subject: I've come to haunt you
+c -> 
+c -> Muahahahaha
+c -> 
+
+# This is NOT a proper terminator, because it doesn't end with \r\n.
+# Processing should continue. If the parser incorrectly accepts this as a
+# valid DATA terminator (which would expose us to an SMTP smuggling attack),
+# then the subsequent lines will result in the server returning an error
+# instead of a successful response to the QUIT.
+c ~> '.\n'
+
+c -> That was a bad line ending, this is a good one.
+c ~> 'xxx\r\n.\r\n'
+
+c <- 521 5.5.2 Error reading DATA: invalid line ending
+

test/t-12-minor_dialogs/bad_data_dot_2.cmy

@@ -0,0 +1,30 @@
+
+c tcp_connect localhost:1025
+
+c <~ 220
+c -> EHLO localhost
+c <... 250 HELP
+c -> MAIL FROM: <>
+c <~ 250
+c -> RCPT TO: user@testserver
+c <~ 250
+c -> DATA
+c <~ 354
+c -> From: Mailer daemon <[email protected]>
+c -> Subject: I've come to haunt you
+c -> 
+c -> Muahahahaha
+c -> 
+
+# This is NOT a proper terminator, because it doesn't end with \r\n.
+# Processing should continue. If the parser incorrectly accepts this as a
+# valid DATA terminator (which would expose us to an SMTP smuggling attack),
+# then the subsequent lines will result in the server returning an error
+# instead of a successful response to the QUIT.
+c ~> 'xxx\n.\n'
+
+c -> That was a bad line ending, this is a good one.
+c ~> '\r\n.\r\n'
+
+c <- 521 5.5.2 Error reading DATA: invalid line ending
+