Reject invalid characters in domains

Domains in addresses had very lax validation, which allowed them to
include characters like "/".

That could be exploited to trigger a daemon panic: a MAIL FROM
"x@evil/foo" could be accepted and, on failed delivery, a DSN queued.
Attempting to deliver the DSN would cause an MTA-STS cache lookup, which
would panic because the domain includes a "/" and crash the server.
Because the DSN would be queued, the server would crash again on
restarts, and only self-recover after give_up_send_after (20h by
default).

To fix this, we now explicitly validate the domain when normalizing it,
make MTA-STS cache not panic on invalid domains, and also make the SMTP
courier fail permanently on invalid domains.

Thanks to John JEAN <john@wargan.com> for finding and reporting this
problem. The detailed report can be found in
https://johnjean.io/advisories/chasquid-unauthenticated-remote-dos/.

internal/aliases/aliases_test.go

@@ -585,6 +585,9 @@ func TestAddFile(t *testing.T) {
 		{"a", "line 1: missing ':' in line"},
 		{"a: x y z\n", "disallowed rune encountered"},
 		{"a: f via sA//sB\n", "empty server in via list"},
+		{"a: b@c/d\n", "invalid domain"},
+		{"a: b@[1.2.3.4]\n", "invalid domain"},
+		{"a: b@c/d via sA\n", "invalid domain"},
 	}
 
 	for _, c := range errcases {

internal/auth/auth_test.go

@@ -47,6 +47,7 @@ func TestDecodeResponse(t *testing.T) {
 		"", "\x00", "\x00\x00", "\x00\x00\x00", "\x00\x00\x00\x00",
 		"a\x00b", "a\x00b\x00c", "a@a\x00b@b\x00pass",
 		"\xffa@b\x00\xffa@b\x00pass",
+		"u@a/b\x00\x00pass", "u@[1.2.3.4]\x00\x00pass",
 	}
 	for _, c := range failedCases {
 		r := base64.StdEncoding.EncodeToString([]byte(c))

internal/courier/smtp.go

@@ -13,6 +13,7 @@ import (
 	"blitiri.com.ar/go/chasquid/internal/domaininfo"
 	"blitiri.com.ar/go/chasquid/internal/envelope"
 	"blitiri.com.ar/go/chasquid/internal/expvarom"
+	"blitiri.com.ar/go/chasquid/internal/normalize"
 	"blitiri.com.ar/go/chasquid/internal/smtp"
 	"blitiri.com.ar/go/chasquid/internal/sts"
 	"blitiri.com.ar/go/chasquid/internal/trace"
@@ -333,6 +334,14 @@ func (s *SMTP) fetchSTSPolicy(tr *trace.Trace, domain string) *sts.Policy {
 }
 
 func lookupMXs(tr *trace.Trace, domain string) ([]string, error, bool) {
+	// Check the domain is valid. It should have been normalized already, but
+	// it can come from sources we don't control (e.g. queue items written by
+	// older versions), and an invalid domain can cause problems later on
+	// (e.g. in the STS cache).
+	if _, err := normalize.Domain(domain); err != nil {
+		return nil, err, true
+	}
+
 	domain, err := idna.ToASCII(domain)
 	if err != nil {
 		return nil, err, true

internal/courier/smtp_test.go

@@ -262,6 +262,33 @@ func TestSTSNoMXAllowed(t *testing.T) {
 	}
 }
 
+func TestDeliverInvalidDomain(t *testing.T) {
+	// Mimic what the real resolver returns for invalid domains, so delivery
+	// would fall back to A if we didn't check the domain.
+	testMXErr["a/b"] = &net.DNSError{
+		Err:        "no such host (test)",
+		IsNotFound: true,
+	}
+	t.Cleanup(func() { delete(testMXErr, "a/b") })
+
+	// Set up an STS cache, as that is one of the things that can't handle
+	// invalid domains.
+	stsCache, err := sts.NewCache(t.ArtifactDir())
+	if err != nil {
+		t.Fatal(err)
+	}
+
+	s := newSMTP(t)
+	s.STSCache = stsCache
+	err, permanent := s.Deliver("me@me", "to@a/b", []byte("data"))
+	if err == nil {
+		t.Errorf("expected error, got nil")
+	}
+	if !permanent {
+		t.Errorf("expected permanent failure, got transient")
+	}
+}
+
 func TestNoMXServer(t *testing.T) {
 	testMX["to"] = []*net.MX{}
 

internal/normalize/normalize.go

@@ -4,6 +4,8 @@ package normalize
 
 import (
 	"bytes"
+	"errors"
+	"fmt"
 	"strings"
 
 	"blitiri.com.ar/go/chasquid/internal/envelope"
@@ -23,7 +25,8 @@ func User(user string) (string, error) {
 	return norm, nil
 }
 
-// Domain normalizes a DNS domain into a cleaned UTF-8 form.
+// Domain normalizes a DNS domain into a cleaned UTF-8 form, and checks that
+// it is valid (see checkDomain for details).
 // On error, it will also return the original domain to simplify callers.
 func Domain(domain string) (string, error) {
 	// For now, we just convert them to lower case and make sure it's in NFC
@@ -39,9 +42,44 @@ func Domain(domain string) (string, error) {
 
 	d = norm.NFC.String(d)
 	d = strings.ToLower(d)
+
+	if err := checkDomain(d); err != nil {
+		return domain, err
+	}
 	return d, nil
 }
 
+var errInvalidDomain = errors.New("invalid domain")
+
+// checkDomain checks that the domain, in its ASCII form, only contains
+// characters that can appear in DNS names: letters, digits, '-', '.', and
+// '_' (not valid in hostnames, but found in the real world).
+//
+// This is intentionally lenient: we don't check label structure or lengths,
+// as those are not a problem for us and DNS lookups will fail anyway. The
+// goal is to reject characters that could be problematic when the domain is
+// used in other contexts (e.g. '/' in file paths or URLs).
+// Note this also rejects domain literals (e.g. "[1.2.3.4]").
+func checkDomain(domain string) error {
+	// Check the ASCII form, as that is what is used for DNS lookups and
+	// other external interactions. Punycode encoding preserves ASCII
+	// characters, so they will show up here even in mixed labels.
+	a, err := idna.ToASCII(domain)
+	if err != nil {
+		return err
+	}
+
+	for _, c := range []byte(a) {
+		switch {
+		case 'a' <= c && c <= 'z', 'A' <= c && c <= 'Z', '0' <= c && c <= '9':
+		case c == '-', c == '.', c == '_':
+		default:
+			return fmt.Errorf("%w: invalid character %q", errInvalidDomain, c)
+		}
+	}
+	return nil
+}
+
 // Addr normalizes an email address, applying User and Domain to its
 // respective components.
 // On error, it will also return the original address to simplify callers.

internal/normalize/normalize_test.go

@@ -44,6 +44,9 @@ func TestDomain(t *testing.T) {
 		{"xn--aca-6ma", "ñaca"},
 		{"xn--lca", "ñ"}, // Punycode is for 'Ñ'.
 		{"e\u0301", "é"}, // Transform to NFC form.
+		{"a_b.c-d", "a_b.c-d"},
+		{"Trailing.Dot.", "trailing.dot."},
+		{"", ""},
 	}
 	for _, c := range valid {
 		nu, err := Domain(c.user)
@@ -56,7 +59,12 @@ func TestDomain(t *testing.T) {
 
 	}
 
-	invalid := []string{"xn---", "xn--xyz-ñ"}
+	invalid := []string{
+		"xn---", "xn--xyz-ñ",
+		"a/b", "a\\b", "a%2fb", "a b", "a\x00b", "a:b", "a@b",
+		"[1.2.3.4]", "[IPv6:::1]",
+		"ñ/b", // Non-ASCII label, ASCII characters are kept by Punycode.
+	}
 	for _, u := range invalid {
 		nu, err := Domain(u)
 		if err == nil {
@@ -85,7 +93,7 @@ func TestAddr(t *testing.T) {
 	}
 
 	invalid := []string{
-		"á é@i", "henry\u2163@throne", "a@xn---",
+		"á é@i", "henry\u2163@throne", "a@xn---", "a@b/c", "a@[1.2.3.4]",
 	}
 	for _, u := range invalid {
 		nu, err := Addr(u)
@@ -121,7 +129,7 @@ func TestDomainToUnicode(t *testing.T) {
 		}
 	}
 
-	invalid := []string{"a@xn---", "a@xn--xyz-ñ"}
+	invalid := []string{"a@xn---", "a@xn--xyz-ñ", "a@b/c", "a@[1.2.3.4]"}
 	for _, u := range invalid {
 		got, err := DomainToUnicode(u)
 		if err == nil {

internal/smtpsrv/conn.go

@@ -473,6 +473,16 @@ func (c *Conn) MAIL(params string) (code int, msg string) {
 			return 501, "5.1.7 Sender address too long"
 		}
 
+		// Normalize (and validate) the address before doing any lookups.
+		// Note SPF and the security level check use the address as given,
+		// and we only use the normalized version from then on.
+		normAddr, err := normalize.DomainToUnicode(addr)
+		if err != nil {
+			maillog.Rejected(c.remoteAddr, addr, nil,
+				fmt.Sprintf("malformed address: %v", err))
+			return 501, "5.1.8 Malformed sender domain"
+		}
+
 		// SPF check - https://tools.ietf.org/html/rfc7208#section-2.4
 		// We opt not to fail on errors, to avoid accidents from preventing
 		// delivery.
@@ -491,12 +501,7 @@ func (c *Conn) MAIL(params string) (code int, msg string) {
 			return 550, "5.7.3 Security level check failed"
 		}
 
-		addr, err = normalize.DomainToUnicode(addr)
+		addr = normAddr
-		if err != nil {
-			maillog.Rejected(c.remoteAddr, addr, nil,
-				fmt.Sprintf("malformed address: %v", err))
-			return 501, "5.1.8 Malformed sender domain (IDNA conversion failed)"
-		}
 	}
 
 	c.mailFrom = addr
@@ -595,7 +600,7 @@ func (c *Conn) RCPT(params string) (code int, msg string) {
 
 	addr, err := normalize.DomainToUnicode(e.Address)
 	if err != nil {
-		return 501, "5.1.2 Malformed destination domain (IDNA conversion failed)"
+		return 501, "5.1.2 Malformed destination domain"
 	}
 
 	// https://tools.ietf.org/html/rfc5321#section-4.5.3.1.3

internal/smtpsrv/server_test.go

@@ -225,7 +225,8 @@ func TestBrokenAuth(t *testing.T) {
 }
 
 func TestWrongMailParsing(t *testing.T) {
-	addrs := []string{"from", "a b c", "a @ b", "<x>", "<x y>", "><"}
+	addrs := []string{"from", "a b c", "a @ b", "<x>", "<x y>", "><",
+		"a@b/c", "a@[1.2.3.4]"}
 	for _, addr := range addrs {
 		c := mustDial(t, ModeSMTP, false)
 
@@ -247,6 +248,32 @@ func TestWrongMailParsing(t *testing.T) {
 	}
 }
 
+func TestMalformedDomain(t *testing.T) {
+	// Unauthenticated MAIL FROM.
+	c := mustDial(t, ModeSMTP, false)
+	defer c.Close()
+
+	err := c.Mail("a@b/c")
+	expectTPErr(t, err, 501, "5.1.8 Malformed sender domain")
+
+	// Authenticated RCPT TO. We need to authenticate, otherwise it may get
+	// rejected as relaying.
+	c = mustDial(t, ModeSubmission, true)
+	defer c.Close()
+
+	auth := smtp.PlainAuth("", "testuser@localhost", "testpasswd", "127.0.0.1")
+	if err := c.Auth(auth); err != nil {
+		t.Fatalf("Auth: %v", err)
+	}
+
+	if err := c.Mail("testuser@localhost"); err != nil {
+		t.Fatalf("Mail: %v", err)
+	}
+
+	err = c.Rcpt("a@b/c")
+	expectTPErr(t, err, 501, "5.1.2 Malformed destination domain")
+}
+
 func TestNullMailFrom(t *testing.T) {
 	c := mustDial(t, ModeSMTP, false)
 	defer c.Close()

internal/sts/sts.go

@@ -22,6 +22,7 @@ import (
 	"time"
 
 	"blitiri.com.ar/go/chasquid/internal/expvarom"
+	"blitiri.com.ar/go/chasquid/internal/normalize"
 	"blitiri.com.ar/go/chasquid/internal/safeio"
 	"blitiri.com.ar/go/chasquid/internal/trace"
 
@@ -174,6 +175,12 @@ func (p *Policy) MXIsAllowed(mx string) bool {
 // This can be useful for debugging and troubleshooting, but you should always
 // call Check on the policy before using it.
 func UncheckedFetch(ctx context.Context, domain string) (*Policy, error) {
+	// Check the domain is valid before doing any lookups, as we use it to
+	// construct the URL.
+	if _, err := normalize.Domain(domain); err != nil {
+		return nil, err
+	}
+
 	// Convert the domain to ascii form, as httpGet does not support IDNs in
 	// any other way.
 	domain, err := idna.ToASCII(domain)
@@ -358,19 +365,25 @@ func NewCache(dir string) (*PolicyCache, error) {
 
 const pathPrefix = "pol:"
 
-func (c *PolicyCache) domainPath(domain string) string {
+func (c *PolicyCache) domainPath(domain string) (string, error) {
-	// We assume the domain is well formed, sanity check just in case.
+	// Domains should have been normalized already, but they can come from
-	if strings.Contains(domain, "/") {
+	// sources we don't control (e.g. queue items written by older versions),
-		panic("domain contains slash")
+	// so check they're valid. Among other things, this guarantees they don't
+	// contain "/".
+	if _, err := normalize.Domain(domain); err != nil {
+		return "", err
 	}
 
-	return c.dir + "/" + pathPrefix + domain
+	return c.dir + "/" + pathPrefix + domain, nil
 }
 
 var errExpired = errors.New("cache entry expired")
 
 func (c *PolicyCache) load(domain string) (*Policy, error) {
-	fname := c.domainPath(domain)
+	fname, err := c.domainPath(domain)
+	if err != nil {
+		return nil, err
+	}
 
 	fi, err := os.Stat(fname)
 	if err != nil {
@@ -421,7 +434,10 @@ func (c *PolicyCache) store(domain string, p *Policy) error {
 		return os.Chtimes(fname, expires, expires)
 	}
 
-	fname := c.domainPath(domain)
+	fname, err := c.domainPath(domain)
+	if err != nil {
+		return err
+	}
 	err = safeio.WriteFile(fname, data, 0640, chTime)
 	if err != nil {
 		cacheIOErrors.Add(1)

internal/sts/sts_test.go

@@ -289,7 +289,11 @@ func TestCacheBasics(t *testing.T) {
 	// Simulate an expired cache entry by changing the mtime of domain.com's
 	// entry to the past.
 	expires := time.Now().Add(-1 * time.Minute)
-	os.Chtimes(c.domainPath("domain.com"), expires, expires)
+	fname, err := c.domainPath("domain.com")
+	if err != nil {
+		t.Fatalf("domainPath failed: %v", err)
+	}
+	os.Chtimes(fname, expires, expires)
 
 	// Do a third fetch, check that we don't get a cache hit.
 	p, err = c.Fetch(ctx, "domain.com")
@@ -349,7 +353,10 @@ func TestCacheBadData(t *testing.T) {
 		expvarMustEq(t, "cacheHits", cacheHits, 0)
 
 		// Edit the file, filling it with the bad content for this case.
-		fname := c.domainPath("domain.com")
+		fname, err := c.domainPath("domain.com")
+		if err != nil {
+			t.Fatalf("domainPath failed: %v", err)
+		}
 		mustRewriteAndChtime(t, fname, badContent)
 
 		// We now expect Fetch to fall back to getting the policy from the
@@ -457,15 +464,21 @@ func TestCacheSlashSafe(t *testing.T) {
 		t.Fatal(err)
 	}
 
-	defer func() {
+	if fname, err := c.domainPath("a/b"); err == nil {
-		if r := recover(); r != nil {
+		t.Errorf("domainPath did not fail as expected, returned %q", fname)
-			t.Logf("recovered: %v", r)
-		} else {
-			t.Fatalf("check did not panic as expected")
 	}
-	}()
 
-	c.domainPath("a/b")
+	if p, err := c.Fetch(t.Context(), "a/b"); err == nil {
+		t.Errorf("cache Fetch did not fail as expected, returned %v", p)
+	}
+
+	if err := c.store("a/b", &Policy{}); err == nil {
+		t.Errorf("store did not fail as expected")
+	}
+
+	if p, err := Fetch(t.Context(), "a/b"); err == nil {
+		t.Errorf("Fetch did not fail as expected, returned %v", p)
+	}
 }
 
 func TestURLForDomain(t *testing.T) {

test/t-03-queue_persistency/addtoqueue.go

@@ -23,11 +23,19 @@ var (
 	id       = flag.String("id", "mid1234", "Message ID")
 	from     = flag.String("from", "from", "Mail from")
 	rcpt     = flag.String("rcpt", "rcpt", "Rcpt to")
+	rcptType = flag.String("rcpt_type", "EMAIL",
+		"Recipient type (EMAIL, PIPE, FORWARD)")
 )
 
 func main() {
 	flag.Parse()
 
+	rType, ok := queue.Recipient_Type_value[*rcptType]
+	if !ok {
+		fmt.Printf("unknown recipient type %q\n", *rcptType)
+		os.Exit(1)
+	}
+
 	data, err := io.ReadAll(os.Stdin)
 	if err != nil {
 		fmt.Printf("error reading data: %v\n", err)
@@ -42,7 +50,7 @@ func main() {
 			Rcpt: []*queue.Recipient{
 				{
 					Address: *rcpt,
-					Type:    queue.Recipient_EMAIL,
+					Type:    queue.Recipient_Type(rType),
 					Status:  queue.Recipient_PENDING,
 				},
 			},

test/t-03-queue_persistency/run.sh

@@ -5,21 +5,58 @@ set -e
 
 init
 
+# Don't hang forever if queue items are not processed as expected.
+timeout 1m
+
+# Start with an empty queue and mail log, so leftovers from previous runs
+# don't interfere.
+rm -rf .data/queue .logs/mail_log
+
 # Add an item to the queue before starting chasquid.
 go run addtoqueue.go --queue_dir=.data/queue \
 		--from someone@testserver \
 		--rcpt someone@testserver \
 		< content
 
+# Add items with invalid domains, which older versions could have queued.
+# They should fail delivery, but not crash the daemon.
+# A DSN to an invalid domain.
+go run addtoqueue.go --queue_dir=.data/queue --id=invalid1 \
+		--from "<>" \
+		--rcpt "x@evil/foo" \
+		< content
+# A message from an invalid domain, which fails delivery and so generates a
+# DSN to the invalid domain.
+go run addtoqueue.go --queue_dir=.data/queue --id=invalid2 \
+		--from "x@evil/foo" \
+		--rcpt_type PIPE --rcpt "false" \
+		< content
+
 generate_certs_for testserver
 
 mkdir -p .logs
 chasquid -v=2 --logfile=.logs/chasquid.log --config_dir=config &
-wait_until_ready 1025
+CHASQUID_PID=$!
 
-# Check that the item in the queue was delivered.
+# Wait for all the queue items to be processed (or chasquid to exit), and
-wait_for_file .mail/someone@testserver
+# check that chasquid is still running.
+function queue_empty_or_exited() {
+	! kill -0 $CHASQUID_PID 2>/dev/null || \
+		! compgen -G '.data/queue/m:*' > /dev/null
+}
+wait_until queue_empty_or_exited
+if ! kill -0 $CHASQUID_PID 2>/dev/null; then
+	fail "chasquid exited unexpectedly"
+fi
 
+# Check that the valid item in the queue was delivered.
 mail_diff content .mail/someone@testserver
 
+# Check that the DSNs to the invalid domain failed as expected (one queued
+# directly, one generated from the failed delivery).
+if [ "$(grep -c 'from=<> to=x@evil/foo failed (permanent)' \
+		.logs/mail_log)" != 2 ]; then
+	fail "did not find the expected DSN failures in the mail log"
+fi
+
 success

test/t-12-minor_dialogs/bad_mail_from.cmy

@@ -21,7 +21,20 @@ c -> MAIL FROM:<pepe>
 c <~ 501
 
 c -> MAIL FROM:<a@xn--->
-c <- 501 5.1.8 Malformed sender domain (IDNA conversion failed)
+c <- 501 5.1.8 Malformed sender domain
+
+# Reconnect to avoid getting rejected due to too many errors.
+c close
+c tcp_connect localhost:1025
+c <~ 220
+c -> HELO localhost
+c <~ 250
+
+c -> MAIL FROM:<a@b/c>
+c <- 501 5.1.8 Malformed sender domain
+
+c -> MAIL FROM:<a@[1.2.3.4]>
+c <- 501 5.1.8 Malformed sender domain
 
 # Reconnect to avoid getting rejected due to too many errors.
 c close

test/t-12-minor_dialogs/bad_rcpt_to.cmy

@@ -26,7 +26,22 @@ c -> RCPT TO:<pepe>
 c <~ 501
 
 c -> RCPT TO:<a@xn--->
-c <- 501 5.1.2 Malformed destination domain (IDNA conversion failed)
+c <- 501 5.1.2 Malformed destination domain
+
+# Reconnect to avoid getting rejected due to too many errors.
+c close
+c tcp_connect localhost:1025
+c <~ 220
+c -> HELO localhost
+c <~ 250
+c -> MAIL FROM:<test@testy.com>
+c <~ 250
+
+c -> RCPT TO:<a@b/c>
+c <- 501 5.1.2 Malformed destination domain
+
+c -> RCPT TO:<a@[1.2.3.4]>
+c <- 501 5.1.2 Malformed destination domain
 
 # Reconnect to avoid getting rejected due to too many errors.
 c close