From 61792e3bb46d9b4692e7c5061346b3c4d1676b34 Mon Sep 17 00:00:00 2001 From: Alberto Bertogli Date: Thu, 8 Oct 2026 23:54:15 +0100 Subject: [PATCH] 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 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 | 3 ++ internal/auth/auth_test.go | 1 + internal/courier/smtp.go | 9 +++++ internal/courier/smtp_test.go | 27 ++++++++++++++ internal/normalize/normalize.go | 40 ++++++++++++++++++++- internal/normalize/normalize_test.go | 14 ++++++-- internal/smtpsrv/conn.go | 19 ++++++---- internal/smtpsrv/server_test.go | 29 ++++++++++++++- internal/sts/sts.go | 30 ++++++++++++---- internal/sts/sts_test.go | 33 +++++++++++------ test/t-03-queue_persistency/addtoqueue.go | 10 +++++- test/t-03-queue_persistency/run.sh | 43 +++++++++++++++++++++-- test/t-12-minor_dialogs/bad_mail_from.cmy | 15 +++++++- test/t-12-minor_dialogs/bad_rcpt_to.cmy | 17 ++++++++- 14 files changed, 255 insertions(+), 35 deletions(-) diff --git a/internal/aliases/aliases_test.go b/internal/aliases/aliases_test.go index 314a0cd..ca093dc 100644 --- a/internal/aliases/aliases_test.go +++ b/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 { diff --git a/internal/auth/auth_test.go b/internal/auth/auth_test.go index 181b50b..3a630b0 100644 --- a/internal/auth/auth_test.go +++ b/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)) diff --git a/internal/courier/smtp.go b/internal/courier/smtp.go index 9886ba1..8b761b6 100644 --- a/internal/courier/smtp.go +++ b/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 diff --git a/internal/courier/smtp_test.go b/internal/courier/smtp_test.go index 80048b6..ab95c92 100644 --- a/internal/courier/smtp_test.go +++ b/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{} diff --git a/internal/normalize/normalize.go b/internal/normalize/normalize.go index 6fd6f6a..c70bda0 100644 --- a/internal/normalize/normalize.go +++ b/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. diff --git a/internal/normalize/normalize_test.go b/internal/normalize/normalize_test.go index 623bd98..27edd2e 100644 --- a/internal/normalize/normalize_test.go +++ b/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 { diff --git a/internal/smtpsrv/conn.go b/internal/smtpsrv/conn.go index 3afefe5..c4b1934 100644 --- a/internal/smtpsrv/conn.go +++ b/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) - 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)" - } + addr = normAddr } 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 diff --git a/internal/smtpsrv/server_test.go b/internal/smtpsrv/server_test.go index 13241f0..cccb599 100644 --- a/internal/smtpsrv/server_test.go +++ b/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", "", "", "><"} + addrs := []string{"from", "a b c", "a @ b", "", "", "><", + "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() diff --git a/internal/sts/sts.go b/internal/sts/sts.go index 4b6f369..c20182c 100644 --- a/internal/sts/sts.go +++ b/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 { - // We assume the domain is well formed, sanity check just in case. - if strings.Contains(domain, "/") { - panic("domain contains slash") +func (c *PolicyCache) domainPath(domain string) (string, error) { + // Domains should have been normalized already, but they can come from + // sources we don't control (e.g. queue items written by older versions), + // 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) diff --git a/internal/sts/sts_test.go b/internal/sts/sts_test.go index f3617b7..75879a7 100644 --- a/internal/sts/sts_test.go +++ b/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 r := recover(); r != nil { - t.Logf("recovered: %v", r) - } else { - t.Fatalf("check did not panic as expected") - } - }() + if fname, err := c.domainPath("a/b"); err == nil { + t.Errorf("domainPath did not fail as expected, returned %q", fname) + } - 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) { diff --git a/test/t-03-queue_persistency/addtoqueue.go b/test/t-03-queue_persistency/addtoqueue.go index b5228be..2ddfdea 100644 --- a/test/t-03-queue_persistency/addtoqueue.go +++ b/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, }, }, diff --git a/test/t-03-queue_persistency/run.sh b/test/t-03-queue_persistency/run.sh index c8d52e0..f38282c 100755 --- a/test/t-03-queue_persistency/run.sh +++ b/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_file .mail/someone@testserver +# Wait for all the queue items to be processed (or chasquid to exit), and +# 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 diff --git a/test/t-12-minor_dialogs/bad_mail_from.cmy b/test/t-12-minor_dialogs/bad_mail_from.cmy index fe7632e..9cea602 100644 --- a/test/t-12-minor_dialogs/bad_mail_from.cmy +++ b/test/t-12-minor_dialogs/bad_mail_from.cmy @@ -21,7 +21,20 @@ c -> MAIL FROM: c <~ 501 c -> MAIL FROM: -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: +c <- 501 5.1.8 Malformed sender domain + +c -> MAIL FROM: +c <- 501 5.1.8 Malformed sender domain # Reconnect to avoid getting rejected due to too many errors. c close diff --git a/test/t-12-minor_dialogs/bad_rcpt_to.cmy b/test/t-12-minor_dialogs/bad_rcpt_to.cmy index dbb4b42..e11d67a 100644 --- a/test/t-12-minor_dialogs/bad_rcpt_to.cmy +++ b/test/t-12-minor_dialogs/bad_rcpt_to.cmy @@ -26,7 +26,22 @@ c -> RCPT TO: c <~ 501 c -> RCPT TO: -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: +c <~ 250 + +c -> RCPT TO: +c <- 501 5.1.2 Malformed destination domain + +c -> RCPT TO: +c <- 501 5.1.2 Malformed destination domain # Reconnect to avoid getting rejected due to too many errors. c close