smtpsrv: Use SNI as hostname only if it matches one of our certs

Today, the client can give us any SNI, and still complete a valid
negotiation, which means it can be attacker-controlled (including having
problematic characters).

We use that SNI as our hostname in a variety of places, including
Received and Authentication-Results headers. This creates an
opportunity for an attacker to manipulate those headers.

To fix that problem, this patch only takes the SNI if it matches one of
our certs' hostnames. Otherwise we use the configured hostname (same as
if there was no SNI today).

Once Go 1.27 is our minimum supported version, we can simplify this
further by checking the SNI against ConnectionState.LocalCertificate.

internal/smtpsrv/conn.go

@@ -214,7 +214,7 @@ func (c *Conn) Handle() {
 
 		cstate := tc.ConnectionState()
 		c.tlsConnState = &cstate
-		if name := c.tlsConnState.ServerName; name != "" {
+		if name := c.validServerName(); name != "" {
 			c.hostname = name
 		}
 	}
@@ -837,6 +837,35 @@ func (c *Conn) addReceivedHeader() {
 	}
 }
 
+// validServerName returns the server name the client requested via TLS SNI,
+// if one of our certificates is valid for it; otherwise it returns "".
+//
+// The SNI is controlled by the client, and Go accepts almost any value (e.g.
+// with spaces, ";", or CR/LF), so we must not use it blindly: it ends up in
+// our greeting, and the Received and Authentication-Results headers.
+// VerifyHostname only accepts valid hostnames, and then checks them against
+// the certificate.
+//
+// TODO: Once we can depend on Go 1.27, use ConnectionState.LocalCertificate
+// to check against the certificate we actually presented. Today they are
+// almost always the same, and when not, the name is still valid for one of
+// our certificates.
+func (c *Conn) validServerName() string {
+	name := c.tlsConnState.ServerName
+	if name == "" {
+		return ""
+	}
+
+	for _, cert := range c.tlsConfig.Certificates {
+		if cert.Leaf != nil && cert.Leaf.VerifyHostname(name) == nil {
+			return name
+		}
+	}
+
+	c.tr.Debugf("ignoring SNI %q, no certificate is valid for it", name)
+	return ""
+}
+
 // Escape "(" and ")" with quoted-pairs, for use inside header comments.
 // https://datatracker.ietf.org/doc/html/rfc5322#section-3.2.2
 var commentEscaper = strings.NewReplacer("(", `\(`, ")", `\)`)
@@ -1173,7 +1202,7 @@ func (c *Conn) STARTTLS(params string) (code int, msg string) {
 
 	// If the client requested a specific server and we complied, that's our
 	// identity from now on.
-	if name := c.tlsConnState.ServerName; name != "" {
+	if name := c.validServerName(); name != "" {
 		c.hostname = name
 	}
 

internal/smtpsrv/conn_test.go

@@ -1,9 +1,14 @@
 package smtpsrv
 
 import (
+	"crypto/ed25519"
+	"crypto/tls"
+	"crypto/x509"
+	"math/big"
 	"net"
 	"strings"
 	"testing"
+	"time"
 
 	"blitiri.com.ar/go/chasquid/internal/domaininfo"
 	"blitiri.com.ar/go/chasquid/internal/trace"
@@ -153,6 +158,71 @@ func TestReceivedHeaderEnvelopeFrom(t *testing.T) {
 	}
 }
 
+func TestValidServerName(t *testing.T) {
+	// Self-signed certificate valid for mx.example.com and *.example.net.
+	pub, priv, err := ed25519.GenerateKey(nil)
+	if err != nil {
+		t.Fatal(err)
+	}
+	tmpl := &x509.Certificate{
+		SerialNumber: big.NewInt(1),
+		DNSNames:     []string{"mx.example.com", "*.example.net"},
+		NotBefore:    time.Now(),
+		NotAfter:     time.Now().Add(time.Hour),
+	}
+	der, err := x509.CreateCertificate(nil, tmpl, tmpl, pub, priv)
+	if err != nil {
+		t.Fatal(err)
+	}
+	leaf, err := x509.ParseCertificate(der)
+	if err != nil {
+		t.Fatal(err)
+	}
+	cert := tls.Certificate{
+		Certificate: [][]byte{der},
+		PrivateKey:  priv,
+		Leaf:        leaf,
+	}
+
+	cases := []struct {
+		sni  string
+		want string
+	}{
+		{"", ""},
+		{"mx.example.com", "mx.example.com"},
+		{"MX.Example.COM", "MX.Example.COM"},
+		{"a.example.net", "a.example.net"},
+		{"a.b.example.net", ""},
+		{"other.example.com", ""},
+		{"mx.example.com; dkim=pass header.d=example.org", ""},
+		{"mx.example.com\r\nX-Injected: yes", ""},
+		{"a;b.example.net", ""},
+	}
+	for _, c := range cases {
+		conn := &Conn{
+			tlsConfig:    &tls.Config{Certificates: []tls.Certificate{cert}},
+			tlsConnState: &tls.ConnectionState{ServerName: c.sni},
+			tr:           trace.New("test", "TestValidServerName"),
+		}
+		if got := conn.validServerName(); got != c.want {
+			t.Errorf("SNI %q: got %q, want %q", c.sni, got, c.want)
+		}
+		conn.tr.Finish()
+	}
+
+	// Without the parsed leaf, we can't check, so the SNI is not used.
+	cert.Leaf = nil
+	conn := &Conn{
+		tlsConfig:    &tls.Config{Certificates: []tls.Certificate{cert}},
+		tlsConnState: &tls.ConnectionState{ServerName: "mx.example.com"},
+		tr:           trace.New("test", "TestValidServerName"),
+	}
+	if got := conn.validServerName(); got != "" {
+		t.Errorf("without leaf: got %q, want \"\"", got)
+	}
+	conn.tr.Finish()
+}
+
 func TestSanitizeEHLODomain(t *testing.T) {
 	equal := []string{
 		"domain", "do.main", "do-main",

internal/smtpsrv/server_test.go

@@ -550,6 +550,55 @@ func TestStartTLSOnTLS(t *testing.T) {
 	}
 }
 
+// The server name requested via SNI must only be used as our hostname if one
+// of our certificates is valid for it.
+func TestServerNameFromSNI(t *testing.T) {
+	cases := []struct {
+		sni  string
+		want string
+	}{
+		{"localhost", "localhost"},
+		{"other.example.com", "localhost"},
+		{"localhost; dkim=pass header.d=example.com", "localhost"},
+	}
+
+	for _, c := range cases {
+		cfg := tlsConfig.Clone()
+		cfg.ServerName = c.sni
+		cfg.InsecureSkipVerify = true
+
+		// STARTTLS, and then EHLO, whose reply starts with our hostname.
+		sc, err := smtp.Dial(smtpAddr)
+		if err != nil {
+			t.Fatalf("smtp.Dial: %v", err)
+		}
+		if err := sc.StartTLS(cfg); err != nil {
+			t.Fatalf("%q: StartTLS: %v", c.sni, err)
+		}
+		msg := simpleCmd(t, sc, "EHLO test", 250)
+		if !strings.HasPrefix(msg, c.want+" ") {
+			t.Errorf("%q: STARTTLS: EHLO reply %q, expected hostname %q",
+				c.sni, msg, c.want)
+		}
+		sc.Close()
+
+		// TLS-wrapped, where the greeting contains our hostname.
+		conn, err := tls.Dial("tcp", submissionTLSAddr, cfg)
+		if err != nil {
+			t.Fatalf("%q: tls.Dial: %v", c.sni, err)
+		}
+		_, msg, err = textproto.NewConn(conn).ReadResponse(220)
+		if err != nil {
+			t.Fatalf("%q: reading greeting: %v", c.sni, err)
+		}
+		if !strings.HasPrefix(msg, c.want+" ") {
+			t.Errorf("%q: TLS: greeting %q, expected hostname %q",
+				c.sni, msg, c.want)
+		}
+		conn.Close()
+	}
+}
+
 func TestAddDKIMSigner(t *testing.T) {
 	s := NewServer()
 	err := s.AddDKIMSigner("example.com", "selector", "keyfile-does-not-exist")