chasquid: Detect email loops

This patch implements some measures against email loops, such as keeping
a limit on the lenght of an address, and rejecting email that has too
many Received headers.

It's not perfect (a server could be actively removing Received headers),
but it should cover the normal accidents and misconfigurations.

chasquid.go

@@ -50,6 +50,7 @@ var (
 	commandCount      = expvar.NewMap("chasquid/smtpIn/commandCount")
 	responseCodeCount = expvar.NewMap("chasquid/smtpIn/responseCodeCount")
 	spfResultCount    = expvar.NewMap("chasquid/smtpIn/spfResultCount")
+	loopsDetected     = expvar.NewInt("chasquid/smtpIn/loopsDetected")
 )
 
 func main() {
@@ -648,6 +649,11 @@ func (c *Conn) MAIL(params string) (code int, msg string) {
 			return 501, "sender address must contain a domain"
 		}
 
+		// https://tools.ietf.org/html/rfc5321#section-4.5.3.1.3
+		if len(e.Address) > 256 {
+			return 501, "address too long"
+		}
+
 		// SPF check - https://tools.ietf.org/html/rfc7208#section-2.4
 		if tcp, ok := c.netconn.RemoteAddr().(*net.TCPAddr); ok {
 			c.spfResult, c.spfError = spf.CheckHost(
@@ -683,6 +689,10 @@ func (c *Conn) RCPT(params string) (code int, msg string) {
 		return 500, "unknown command"
 	}
 
+	if c.mailFrom == "" {
+		return 503, "sender not yet given"
+	}
+
 	rawAddr := ""
 	_, err := fmt.Sscanf(params[3:], "%s ", &rawAddr)
 	if err != nil {
@@ -709,8 +719,9 @@ func (c *Conn) RCPT(params string) (code int, msg string) {
 		return 501, "malformed address (IDNA conversion failed)"
 	}
 
-	if c.mailFrom == "" {
-		return 503, "sender not yet given"
+	// https://tools.ietf.org/html/rfc5321#section-4.5.3.1.3
+	if len(e.Address) > 256 {
+		return 501, "address too long"
 	}
 
 	localDst := envelope.DomainIn(e.Address, c.localDomains)
@@ -755,6 +766,10 @@ func (c *Conn) DATA(params string) (code int, msg string) {
 
 	c.tr.Debugf("-> ... %d bytes of data", len(c.data))
 
+	if err := checkData(c.data); err != nil {
+		return 554, err.Error()
+	}
+
 	c.addReceivedHeader()
 
 	// There are no partial failures here: we put it in the queue, and then if
@@ -815,6 +830,25 @@ func (c *Conn) addReceivedHeader() {
 	}
 }
 
+// checkData performs very basic checks on the body of the email, to help
+// detect very broad problems like email loops. It does not fully check the
+// sanity of the headers or the structure of the payload.
+func checkData(data []byte) error {
+	msg, err := mail.ReadMessage(bytes.NewBuffer(data))
+	if err != nil {
+		return fmt.Errorf("error parsing message: %v", err)
+	}
+
+	// This serves as a basic form of loop prevention. It's not infallible but
+	// should catch most instances of accidental looping.
+	if len(msg.Header["Received"]) > 50 {
+		loopsDetected.Add(1)
+		return fmt.Errorf("email passed through more than 50 MTAs, looping?")
+	}
+
+	return nil
+}
+
 func (c *Conn) STARTTLS(params string) (code int, msg string) {
 	if c.onTLS {
 		return 503, "You are already wearing that!"

chasquid_test.go

@@ -108,7 +108,7 @@ func sendEmailWithAuth(tb testing.TB, c *smtp.Client, auth smtp.Auth) {
 		tb.Fatalf("Data: %v", err)
 	}
 
-	msg := []byte("Hi! This is an email\n")
+	msg := []byte("Subject: Hi!\n\n This is an email\n")
 	if _, err = w.Write(msg); err != nil {
 		tb.Errorf("Data write: %v", err)
 	}

test/.gitignore

@@ -1,4 +1,5 @@
 
 # Ignore the user databases - we create them each time.
 t-*/config/**/users
+t-*/?/**/users
 

test/t-04-aliases/run.sh

@@ -13,13 +13,13 @@ chasquid -v=2 --log_dir=.logs --config_dir=config &
 wait_until_ready 1025
 
 function send_and_check() {
-		run_msmtp $1@testserver < content
-		shift
-		for i in $@; do
-				wait_for_file .mail/$i@testserver
-				mail_diff content .mail/$i@testserver
-				rm -f .mail/$i@testserver
-		done
+	run_msmtp $1@testserver < content
+	shift
+	for i in $@; do
+		wait_for_file .mail/$i@testserver
+		mail_diff content .mail/$i@testserver
+		rm -f .mail/$i@testserver
+	done
 }
 
 # Test email aliases.

test/t-09-loop/A/chasquid.conf

@@ -0,0 +1,8 @@
+smtp_address: ":1025"
+submission_address: ":1587"
+monitoring_address: ":1099"
+
+mail_delivery_agent_bin: "test-mda"
+mail_delivery_agent_args: "%to%"
+
+data_dir: "../.data-A"

test/t-09-loop/A/domains/srv-A/aliases

@@ -0,0 +1,2 @@
+
+aliasA: aliasB@srv-B

test/t-09-loop/B/chasquid.conf

@@ -0,0 +1,8 @@
+smtp_address: ":2025"
+submission_address: ":2587"
+monitoring_address: ":2099"
+
+mail_delivery_agent_bin: "test-mda"
+mail_delivery_agent_args: "%to%"
+
+data_dir: "../.data-B"

test/t-09-loop/B/domains/srv-B/aliases

@@ -0,0 +1 @@
+aliasB: aliasA@srv-A

test/t-09-loop/content

@@ -0,0 +1,9 @@
+From: userA@srv-A
+To: aliasB@srv-B
+Subject: Los espejos
+
+Yo que sentí el horror de los espejos
+no sólo ante el cristal impenetrable
+donde acaba y empieza, inhabitable,
+un imposible espacio de reflejos
+

test/t-09-loop/hosts

@@ -0,0 +1,2 @@
+srv-A localhost
+srv-B localhost

test/t-09-loop/msmtprc

@@ -0,0 +1,14 @@
+account default
+
+host srv-A
+port 1587
+
+tls on
+tls_trust_file A/certs/srv-A/fullchain.pem
+
+from userA@srv-A
+
+auth on
+user userA@srv-A
+password userA
+

test/t-09-loop/run.sh

@@ -0,0 +1,43 @@
+#!/bin/bash
+
+set -e
+. $(dirname ${0})/../util/lib.sh
+
+init
+
+rm -rf .data-A .data-B .mail
+
+# Two servers:
+# A - listens on :1025, hosts srv-A
+# B - listens on :2015, hosts srv-B
+#
+# We cause the following loop:
+#   userA -> aliasB -> aliasA -> aliasB -> ...
+
+CONFDIR=A generate_certs_for srv-A
+CONFDIR=A add_user srv-A userA userA
+
+CONFDIR=B generate_certs_for srv-B
+
+mkdir -p .logs-A .logs-B
+
+chasquid -v=2 --log_dir=.logs-A --config_dir=A \
+	--testing__outgoing_smtp_port=2025 &
+chasquid -v=2 --log_dir=.logs-B --config_dir=B \
+	--testing__outgoing_smtp_port=1025 &
+
+wait_until_ready 1025
+wait_until_ready 2025
+
+run_msmtp aliasB@srv-B < content
+
+# Wait until one of them has noticed and stopped the loop.
+while sleep 0.1; do
+	wget -q -O .data-A/vars http://localhost:1099/debug/vars
+	wget -q -O .data-B/vars http://localhost:2099/debug/vars
+	if grep -q '"chasquid/smtpIn/loopsDetected": 1,' .data-?/vars; then
+		break
+	fi
+done
+
+success