From a852657fb6bb34e014af6c64ed56aec4f692f7d2 Mon Sep 17 00:00:00 2001 From: Alberto Bertogli Date: Sat, 23 Dec 2023 02:38:07 +0000 Subject: [PATCH] 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. See https://github.com/albertito/chasquid/issues/47 for more details and discussion. **WIP: THIS IS A WORK IN PROGRESS PATCH, AND IT MAY BE EDITED.** --- internal/smtpsrv/conn.go | 42 +++------- internal/smtpsrv/conn_test.go | 53 ------------- internal/smtpsrv/dotreader.go | 79 +++++++++++++++++++ internal/smtpsrv/dotreader_test.go | 78 ++++++++++++++++++ test/t-12-minor_dialogs/bad_data_dot.cmy | 30 +++++++ test/t-12-minor_dialogs/bad_data_dot_2.cmy | 30 +++++++ .../bad_data_dot_on_message_too_big.cmy | 37 +++++++++ test/t-12-minor_dialogs/config/chasquid.conf | 3 + test/t-12-minor_dialogs/message_too_big.cmy | 28 +++++++ test/util/chamuyero | 9 ++- 10 files changed, 304 insertions(+), 85 deletions(-) create mode 100644 internal/smtpsrv/dotreader.go create mode 100644 internal/smtpsrv/dotreader_test.go create mode 100644 test/t-12-minor_dialogs/bad_data_dot.cmy create mode 100644 test/t-12-minor_dialogs/bad_data_dot_2.cmy create mode 100644 test/t-12-minor_dialogs/bad_data_dot_on_message_too_big.cmy create mode 100644 test/t-12-minor_dialogs/message_too_big.cmy diff --git a/internal/smtpsrv/conn.go b/internal/smtpsrv/conn.go index d0a0c31..f5187b9 100644 --- a/internal/smtpsrv/conn.go +++ b/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 { diff --git a/internal/smtpsrv/conn_test.go b/internal/smtpsrv/conn_test.go index 17412a2..0453fe2 100644 --- a/internal/smtpsrv/conn_test.go +++ b/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 { diff --git a/internal/smtpsrv/dotreader.go b/internal/smtpsrv/dotreader.go new file mode 100644 index 0000000..947b320 --- /dev/null +++ b/internal/smtpsrv/dotreader.go @@ -0,0 +1,79 @@ +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 + ) + prev := prevOther + last4 := make([]byte, 4) + +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 != prevOther && prev != prevCRLF { + return buf, errInvalidLineEnding + } + prev = prevCR + case '\n': + if prev != prevCR { + return buf, errInvalidLineEnding + } + // If we come from a '\r\n.\r', then we're done. + if bytes.Equal(last4, []byte("\r\n.\r")) { + break loop + } + prev = prevCRLF + default: + if prev == prevCR { + return buf, errInvalidLineEnding + } + 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 { + buf = append(buf, b) + } + } + + if n > max { + return buf, errMessageTooLarge + } + + // If we made it this far, buf ends in "\r\n.\r"; do not include the final + // ".\r" in the returned buffer. + return buf[:len(buf)-2], nil +} diff --git a/internal/smtpsrv/dotreader_test.go b/internal/smtpsrv/dotreader_test.go new file mode 100644 index 0000000..890f559 --- /dev/null +++ b/internal/smtpsrv/dotreader_test.go @@ -0,0 +1,78 @@ +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. + {"\r\n.\r\n", 10, "\r\n", nil}, + {"abc\r\n.\r\n", 10, "abc\r\n", 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}, + } + + 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) + } +} diff --git a/test/t-12-minor_dialogs/bad_data_dot.cmy b/test/t-12-minor_dialogs/bad_data_dot.cmy new file mode 100644 index 0000000..be0dc85 --- /dev/null +++ b/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 +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 + diff --git a/test/t-12-minor_dialogs/bad_data_dot_2.cmy b/test/t-12-minor_dialogs/bad_data_dot_2.cmy new file mode 100644 index 0000000..02ee353 --- /dev/null +++ b/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 +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 + diff --git a/test/t-12-minor_dialogs/bad_data_dot_on_message_too_big.cmy b/test/t-12-minor_dialogs/bad_data_dot_on_message_too_big.cmy new file mode 100644 index 0000000..4b8a6f6 --- /dev/null +++ b/test/t-12-minor_dialogs/bad_data_dot_on_message_too_big.cmy @@ -0,0 +1,37 @@ + +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 -> Subject: Message too big +c -> + +# Max message size is 1 MiB. Note this includes line endings but converted to +# \n (as per textproto.DotReader), and excluding the final ".". +# We already sent (in the header) 26. +# Send lines of len 900 to stay under the limit. +# (1024 * 1024 - 26) - (900 * 1165) = 50 +c ~> ('a' * 899 + '\r\n') * 1165 + +# We have 50 characters left before the message is too big. +c ~> 'b' * 55 + '\r\n' + +# At this point the message is too big. The remainder data should be +# discarded. +# We use a "bad ." to try to do an SMTP smuggling attack. +c ~> '.\n' +c -> HELP +c -> HELP + +# And now the "good .". +c -> . + +c <- 521 5.5.2 Error reading DATA: invalid line ending + diff --git a/test/t-12-minor_dialogs/config/chasquid.conf b/test/t-12-minor_dialogs/config/chasquid.conf index 9a35641..3680c99 100644 --- a/test/t-12-minor_dialogs/config/chasquid.conf +++ b/test/t-12-minor_dialogs/config/chasquid.conf @@ -13,3 +13,6 @@ mail_log_path: "../.logs/mail_log" suffix_separators: "+-" drop_characters: "._" + +# Small max data size so we can reach it more easily in the tests. +max_data_size_mb: 1 diff --git a/test/t-12-minor_dialogs/message_too_big.cmy b/test/t-12-minor_dialogs/message_too_big.cmy new file mode 100644 index 0000000..ae15d41 --- /dev/null +++ b/test/t-12-minor_dialogs/message_too_big.cmy @@ -0,0 +1,28 @@ + +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 -> Subject: Message too big +c -> + +# Max message size is 1 MiB. Note this includes line endings but converted to +# \n (as per textproto.DotReader), and excluding the final ".". +# We already sent (in the header) 26. +# Send lines of len 900 to stay under the limit. +# (1024 * 1024 - 26) - (900 * 1166) = -850 +c ~> ('a' * 899 + '\r\n') * 1166 + +c -> . + +c <~ 552 5.3.4 Message too big +c -> QUIT +c <~ 221 + diff --git a/test/util/chamuyero b/test/util/chamuyero index 0c5bc56..3f41d4f 100755 --- a/test/util/chamuyero +++ b/test/util/chamuyero @@ -204,12 +204,15 @@ class Interpreter (object): sock.connect() self.procs[proc] = sock - # -> Send to a process stdin, with a \n at the end. - # .> Send to a process stdin, no \n at the end. + # -> Send to a process stdin, with a \r\n at the end. + # .> Send to a process stdin, no \r\n at the end. + # ~> Send to a process stdin, string is python-evaluated. elif op == "->": - self.procs[proc].write(params + "\n") + self.procs[proc].write(params + "\r\n") elif op == ".>": self.procs[proc].write(params) + elif op == "~>": + self.procs[proc].write(eval(params)) # <- Read from the process, expect matching input. # <~ Read from the process, match input using regexp.