WIP: smtpsrv: Strict CRLF enforcement in DATA contents

**WIP: THIS IS A WORK IN PROGRESS PATCH, AND IT MAY BE EDITED.**

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.

See #47 for more details and
discussion.

**WIP: THIS IS A WORK IN PROGRESS PATCH, AND IT MAY BE EDITED.**

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 <somewhere@horns.com>
+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 <somewhere@horns.com>
+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
+