more strict junk checks for some first-time senders: when TLS isn't used and when recipient address isn't in To/Cc header

both cases are quite typical for spammers, and not for legitimate senders.
this doesn't apply to known senders. and it only requires that the content look
more like ham instead of spam. so legitimate mail can still get through with
these properties.

docker-compose-integration.yml

@@ -115,7 +115,7 @@ services:
     volumes:
       # todo: figure out how to mount files with a uid that the process in the container can read...
       - ./testdata/integration/resolv.conf:/etc/resolv.conf
-    command: ["sh", "-c", "set -e; chmod o+r /etc/resolv.conf; (echo 'maillog_file = /dev/stdout'; echo 'mydestination = $$myhostname, localhost.$$mydomain, localhost, $$mydomain') >>/etc/postfix/main.cf; echo 'root: moxtest1@mox1.example' >>/etc/postfix/aliases; newaliases; postfix start-fg"]
+    command: ["sh", "-c", "set -e; chmod o+r /etc/resolv.conf; (echo 'maillog_file = /dev/stdout'; echo 'mydestination = $$myhostname, localhost.$$mydomain, localhost, $$mydomain'; echo 'smtp_tls_security_level = may') >>/etc/postfix/main.cf; echo 'root: postfix@mox1.example' >>/etc/postfix/aliases; newaliases; postfix start-fg"]
     healthcheck:
       test: netstat -nlt | grep ':25 '
       interval: 1s

integration_test.go

@@ -158,7 +158,7 @@ This is the message.
 
 	xlog.Print("submitting email to postfix, waiting for imap notification at moxacmepebble")
 	t0 = time.Now()
-	deliver(true, true, "moxacmepebble.mox1.example:993", "moxtest1@mox1.example", "accountpass1234", func() {
+	deliver(false, true, "moxacmepebble.mox1.example:993", "moxtest1@mox1.example", "accountpass1234", func() {
 		submit(true, "moxtest1@mox1.example", "accountpass1234", "moxacmepebble.mox1.example:465", "root@postfix.example")
 	})
 	xlog.Print("success", mlog.Field("duration", time.Since(t0)))

main.go

@@ -2377,7 +2377,7 @@ can be found in message headers.
 
 	data, err := io.ReadAll(os.Stdin)
 	xcheckf(err, "read message")
-	dmarcFrom, _, err := message.From(mlog.New("dmarcverify"), false, bytes.NewReader(data))
+	dmarcFrom, _, _, err := message.From(mlog.New("dmarcverify"), false, bytes.NewReader(data))
 	xcheckf(err, "extract dmarc from message")
 
 	const ignoreTestMode = false

message/from.go

@@ -17,7 +17,7 @@ import (
 // From headers may be present. From returns an error if there is not exactly
 // one address. This address can be used for evaluating a DMARC policy against
 // SPF and DKIM results.
-func From(log *mlog.Log, strict bool, r io.ReaderAt) (raddr smtp.Address, header textproto.MIMEHeader, rerr error) {
+func From(log *mlog.Log, strict bool, r io.ReaderAt) (raddr smtp.Address, envelope *Envelope, header textproto.MIMEHeader, rerr error) {
 	// ../rfc/7489:1243
 
 	// todo: only allow utf8 if enabled in session/message?
@@ -25,20 +25,20 @@ func From(log *mlog.Log, strict bool, r io.ReaderAt) (raddr smtp.Address, header
 	p, err := Parse(log, strict, r)
 	if err != nil {
 		// todo: should we continue with p, perhaps headers can be parsed?
-		return raddr, nil, fmt.Errorf("parsing message: %v", err)
+		return raddr, nil, nil, fmt.Errorf("parsing message: %v", err)
 	}
 	header, err = p.Header()
 	if err != nil {
-		return raddr, nil, fmt.Errorf("parsing message header: %v", err)
+		return raddr, nil, nil, fmt.Errorf("parsing message header: %v", err)
 	}
 	from := p.Envelope.From
 	if len(from) != 1 {
-		return raddr, nil, fmt.Errorf("from header has %d addresses, need exactly 1 address", len(from))
+		return raddr, nil, nil, fmt.Errorf("from header has %d addresses, need exactly 1 address", len(from))
 	}
 	d, err := dns.ParseDomain(from[0].Host)
 	if err != nil {
-		return raddr, nil, fmt.Errorf("bad domain in from address: %v", err)
+		return raddr, nil, nil, fmt.Errorf("bad domain in from address: %v", err)
 	}
 	addr := smtp.Address{Localpart: smtp.Localpart(from[0].User), Domain: d}
-	return addr, textproto.MIMEHeader(header), nil
+	return addr, p.Envelope, textproto.MIMEHeader(header), nil
 }

smtpserver/analyze.go

@@ -16,6 +16,7 @@ import (
 	"github.com/mjl-/mox/dns"
 	"github.com/mjl-/mox/dnsbl"
 	"github.com/mjl-/mox/iprev"
+	"github.com/mjl-/mox/message"
 	"github.com/mjl-/mox/mlog"
 	"github.com/mjl-/mox/mox-"
 	"github.com/mjl-/mox/publicsuffix"
@@ -26,10 +27,13 @@ import (
 )
 
 type delivery struct {
+	tls         bool
 	m           *store.Message
 	dataFile    *os.File
 	rcptAcc     rcptAccount
 	acc         *store.Account
+	msgTo       []message.Address
+	msgCc       []message.Address
 	msgFrom     smtp.Address
 	dnsBLs      []dns.Domain
 	dmarcUse    bool
@@ -369,11 +373,41 @@ func analyze(ctx context.Context, log *mlog.Log, resolver dns.Resolver, d delive
 		jitter := (jitterRand.Float64() - 0.5) / 10
 		threshold := jf.Threshold + jitter
 
-		// With an iprev fail, we set a higher bar for content.
+		rcptToMatch := func(l []message.Address) bool {
+			// todo: we use Go's net/mail to parse message header addresses. it does not allow empty quoted strings (contrary to spec), leaving To empty. so we don't verify To address for that unusual case for now. ../rfc/5322:961 ../rfc/5322:743
+			if d.rcptAcc.rcptTo.Localpart == "" {
+				return true
+			}
+			for _, a := range l {
+				dom, err := dns.ParseDomain(a.Host)
+				if err != nil {
+					continue
+				}
+				if dom == d.rcptAcc.rcptTo.IPDomain.Domain && smtp.Localpart(a.User) == d.rcptAcc.rcptTo.Localpart {
+					return true
+				}
+			}
+			return false
+		}
+
+		// todo: some of these checks should also apply for reputation-based analysis with a weak signal, e.g. verified dkim/spf signal from new domain.
+		// With an iprev fail, non-TLS connection or our address not in To/Cc header, we set a higher bar for content.
 		reason = reasonJunkContent
 		if suspiciousIPrevFail && threshold > 0.25 {
 			threshold = 0.25
-			log.Info("setting junk threshold due to iprev fail", mlog.Field("threshold", 0.25))
+			log.Info("setting junk threshold due to iprev fail", mlog.Field("threshold", threshold))
+			reason = reasonJunkContentStrict
+		} else if !d.tls && threshold > 0.25 {
+			threshold = 0.25
+			log.Info("setting junk threshold due to plaintext smtp", mlog.Field("threshold", threshold))
+			reason = reasonJunkContentStrict
+		} else if (rs == nil || !rs.IsForward) && threshold > 0.25 && !rcptToMatch(d.msgTo) && !rcptToMatch(d.msgCc) {
+			// A common theme in junk messages is your recipient address not being in the To/Cc
+			// headers. We may be in Bcc, but that's unusual for first-time senders. Some
+			// providers (e.g. gmail) does not DKIM-sign Bcc headers, so junk messages can be
+			// sent with matching Bcc headers. We don't get here for known senders.
+			threshold = 0.25
+			log.Info("setting junk threshold due to smtp rcpt to and message to/cc address mismatch", mlog.Field("threshold", threshold))
 			reason = reasonJunkContentStrict
 		}
 		accept = contentProb <= threshold

smtpserver/server.go

@@ -1765,7 +1765,7 @@ func (c *conn) submit(ctx context.Context, recvHdrFor func(string) string, msgWr
 	// for other users.
 	// We don't check the Sender field, there is no expectation of verification, ../rfc/7489:2948
 	// and with Resent headers it seems valid to have someone else as Sender. ../rfc/5322:1578
-	msgFrom, header, err := message.From(c.log, true, dataFile)
+	msgFrom, _, header, err := message.From(c.log, true, dataFile)
 	if err != nil {
 		metricSubmission.WithLabelValues("badmessage").Inc()
 		c.log.Infox("parsing message From address", err, mlog.Field("user", c.username))
@@ -1961,7 +1961,7 @@ func (c *conn) xlocalserveError(lp smtp.Localpart) {
 func (c *conn) deliver(ctx context.Context, recvHdrFor func(string) string, msgWriter *message.Writer, iprevStatus iprev.Status, iprevAuthentic bool, dataFile *os.File) {
 	// todo: in decision making process, if we run into (some) temporary errors, attempt to continue. if we decide to accept, all good. if we decide to reject, we'll make it a temporary reject.
 
-	msgFrom, headers, err := message.From(c.log, false, dataFile)
+	msgFrom, envelope, headers, err := message.From(c.log, false, dataFile)
 	if err != nil {
 		c.log.Infox("parsing message for From address", err)
 	}
@@ -2461,7 +2461,12 @@ func (c *conn) deliver(ctx context.Context, recvHdrFor func(string) string, msgW
 			m.ReceivedTLSVersion = 1 // Signals plain text delivery.
 		}
 
-		d := delivery{&m, dataFile, rcptAcc, acc, msgFrom, c.dnsBLs, dmarcUse, dmarcResult, dkimResults, iprevStatus}
+		var msgTo, msgCc []message.Address
+		if envelope != nil {
+			msgTo = envelope.To
+			msgCc = envelope.CC
+		}
+		d := delivery{c.tls, &m, dataFile, rcptAcc, acc, msgTo, msgCc, msgFrom, c.dnsBLs, dmarcUse, dmarcResult, dkimResults, iprevStatus}
 		a := analyze(ctx, log, c.resolver, d)
 
 		// Any DMARC result override is stored in the evaluation for outgoing DMARC

smtpserver/server_test.go

@@ -574,21 +574,21 @@ func TestForward(t *testing.T) {
 		totalEvaluations := 0
 
 		var msgBad = strings.ReplaceAll(`From: <remote@bad.example>
-To: <mjl3@mox.example>
+To: <mjl@mox.example>
 Subject: test
 Message-Id: <bad@example.org>
 
 test email
 `, "\n", "\r\n")
 		var msgOK = strings.ReplaceAll(`From: <remote@good.example>
-To: <mjl3@mox.example>
+To: <mjl@mox.example>
 Subject: other
 Message-Id: <good@example.org>
 
 unrelated message.
 `, "\n", "\r\n")
 		var msgOK2 = strings.ReplaceAll(`From: <other@forward.example>
-To: <mjl3@mox.example>
+To: <mjl@mox.example>
 Subject: non-forward
 Message-Id: <regular@example.org>
 
@@ -655,7 +655,13 @@ happens to come from forwarding mail server.
 
 			mailFrom := "other@forward.example"
 
-			err = client.Deliver(ctxbg, mailFrom, rcptTo, int64(len(msgOK2)), strings.NewReader(msgOK2), false, false, false)
+			// Ensure To header matches.
+			msg := msgOK2
+			if forward {
+				msg = strings.ReplaceAll(msg, "<mjl@mox.example>", "<mjl3@mox.example>")
+			}
+
+			err = client.Deliver(ctxbg, mailFrom, rcptTo, int64(len(msg)), strings.NewReader(msg), false, false, false)
 			if forward {
 				tcheck(t, err, "deliver")
 				totalEvaluations += 1
@@ -1418,9 +1424,11 @@ func TestEmptylocalpart(t *testing.T) {
 		t.Helper()
 		ts.run(func(err error, client *smtpclient.Client) {
 			t.Helper()
+
 			mailFrom := `""@other.example`
+			msg := strings.ReplaceAll(deliverMessage, "To: <mjl@mox.example>", `To: <""@mox.example>`)
 			if err == nil {
-				err = client.Deliver(ctxbg, mailFrom, rcptTo, int64(len(deliverMessage)), strings.NewReader(deliverMessage), false, false, false)
+				err = client.Deliver(ctxbg, mailFrom, rcptTo, int64(len(msg)), strings.NewReader(msg), false, false, false)
 			}
 			var cerr smtpclient.Error
 			if expErr == nil && err != nil || expErr != nil && (err == nil || !errors.As(err, &cerr) || cerr.Code != expErr.Code || cerr.Secode != expErr.Secode) {

testdata/integration/moxacmepebble.sh

@@ -19,6 +19,9 @@ TLS:
                         # So certificates from moxmail2 are trusted, and pebble's certificate is trusted.
 			- /integration/tls/ca.pem
 EOF
+# Recognize postfix@mox1.example as destination, and that it is a forwarding destination.
+# Postfix seems to keep the mailfrom when forwarding, so we match on that verifieddomain (but using DKIM).
+sed -i -e 's/moxtest1@mox1.example: nil/moxtest1@mox1.example: nil\n\t\t\tpostfix@mox1.example:\n\t\t\t\tRulesets:\n\t\t\t\t\t-\n\t\t\t\t\t\tSMTPMailFromRegexp: .*\n\t\t\t\t\t\tVerifiedDomain: mox1.example\n\t\t\t\t\t\tIsForward: true\n\t\t\t\t\t\tMailbox: Inbox/' config/domains.conf
 
 (
 	cat /integration/example.zone;