From 8ca198882e994b8ef17473ee3069bbfaa3bada05 Mon Sep 17 00:00:00 2001
From: Mechiel Lukkien <mechiel@ueber.net>
Date: Sat, 14 Oct 2023 22:30:43 +0200
Subject: [PATCH] security fix: use correct domain for mta-sts, that of the
 email address

the original next-hop domain. not anything after resolving cname's, because
then it takes just a single injected dns cname record to lead us to an
unrelated server (that we would verify, but it's the wrong server).

also don't fallback to just strict tls when something is wrong. we must use the
policy to check if an mx host is allowed. the whole idea is that unsigned dns
records cannot be trusted.

i noticed this while implementing dane.
---
 queue/direct.go | 21 ++++++++++-----------
 1 file changed, 10 insertions(+), 11 deletions(-)

diff --git a/queue/direct.go b/queue/direct.go
index f5082f6..ebcdf0a 100644
--- a/queue/direct.go
+++ b/queue/direct.go
@@ -116,19 +116,18 @@ func deliverDirect(cid int64, qlog *mlog.Log, resolver dns.Resolver, dialer smtp
 		return
 	}
 
-	// Check for MTA-STS policy and enforce it if needed. We have to check the
-	// effective domain (found after following CNAME record(s)): there will certainly
-	// not be an MTA-STS record for the original recipient domain, because that is not
-	// allowed when a CNAME record is present.
+	// Check for MTA-STS policy and enforce it if needed.
+	// We must check at the original next-hop, i.e. recipient domain, not following any
+	// CNAMEs. If we were to follow CNAMEs and ask for MTA-STS at that domain, it
+	// would only take a single CNAME DNS response to direct us to an unrelated domain.
 	var policy *mtasts.Policy
-	tlsModeDefault := smtpclient.TLSOpportunistic
-	if !expandedNextHop.IsZero() {
+	if !origNextHop.IsZero() {
 		cidctx := context.WithValue(mox.Shutdown, mlog.CidKey, cid)
-		policy, _, err = mtastsdb.Get(cidctx, resolver, expandedNextHop)
+		policy, _, err = mtastsdb.Get(cidctx, resolver, origNextHop)
 		if err != nil {
-			// No need to refuse to deliver if we have some mtasts error.
-			qlog.Infox("mtasts failed, continuing with strict tls requirement", err, mlog.Field("domain", expandedNextHop))
-			tlsModeDefault = smtpclient.TLSStrictStartTLS
+			qlog.Infox("mtasts lookup temporary error, aborting delivery attempt", err, mlog.Field("domain", origNextHop))
+			fail(qlog, m, backoff, false, dsn.NameIP{}, "", err.Error())
+			return
 		}
 		// note: policy can be nil, if a domain does not implement MTA-STS or it's the
 		// first time we fetch the policy and if we encountered an error.
@@ -168,7 +167,7 @@ func deliverDirect(cid int64, qlog *mlog.Log, resolver dns.Resolver, dialer smtp
 		nqlog := qlog.WithCid(cid)
 		var remoteIP net.IP
 
-		tlsMode := tlsModeDefault
+		tlsMode := smtpclient.TLSOpportunistic
 		if policy != nil && policy.Mode == mtasts.ModeEnforce {
 			tlsMode = smtpclient.TLSStrictStartTLS
 		}