git » chasquid » commit 9de93ed

diff --git a/internal/smtpsrv/conn.go b/internal/smtpsrv/conn.go
index ba01e0e..a35bae2 100644
--- a/internal/smtpsrv/conn.go
+++ b/internal/smtpsrv/conn.go
@@ -19,6 +19,7 @@ import (
 	"strings"
 	"syscall"
 	"time"
+	"unicode"
 
 	"blitiri.com.ar/go/chasquid/internal/aliases"
 	"blitiri.com.ar/go/chasquid/internal/auth"
@@ -468,6 +469,10 @@ func (c *Conn) MAIL(params string) (code int, msg string) {
 			return 501, "5.1.8 Sender address must contain a domain"
 		}
 
+		if !isSafeAddress(addr) {
+			return 501, "5.1.7 Sender address malformed"
+		}
+
 		// https://tools.ietf.org/html/rfc5321#section-4.5.3.1.3
 		if len(addr) > 256 {
 			return 501, "5.1.7 Sender address too long"
@@ -594,7 +599,7 @@ func (c *Conn) RCPT(params string) (code int, msg string) {
 	}
 
 	e, err := mail.ParseAddress(rawAddr)
-	if err != nil || e.Address == "" {
+	if err != nil || e.Address == "" || !isSafeAddress(e.Address) {
 		return 501, "5.1.3 Malformed destination address"
 	}
 
@@ -788,7 +793,10 @@ func (c *Conn) addReceivedHeader() {
 	}
 
 	// Note we must NOT include c.rcptTo, that would leak BCCs.
-	received += fmt.Sprintf("envelope from %q)\n", c.mailFrom)
+	// We are inside a comment, where quotes have no special meaning, so we
+	// also escape "(" and ")" to prevent them from altering its structure.
+	received += fmt.Sprintf("envelope from %s)\n",
+		commentEscaper.Replace(fmt.Sprintf("%q", c.mailFrom)))
 
 	// This should be the last part in the Received header, by RFC.
 	// The ";" is a mandatory separator. The date format is not standard but
@@ -829,6 +837,10 @@ func (c *Conn) addReceivedHeader() {
 	}
 }
 
+// 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("(", `\(`, ")", `\)`)
+
 // addrLiteral converts a net.Addr (must be TCP) into a string for use as
 // address literal, compliant with
 // https://tools.ietf.org/html/rfc5321#section-4.1.3.
@@ -871,6 +883,18 @@ func checkData(data []byte) error {
 	return nil
 }
 
+// isSafeAddress checks that the address (as returned by mail.ParseAddress,
+// which removes the quoting from the local part) doesn't contain characters
+// that could be problematic where we use it:
+//   - Control characters (e.g. in logs or hooks). CR, LF and other
+//     whitespace are already excluded by how we parse the commands.
+//   - More than one "@" (possible with quoted local parts like "a@b"@c), as
+//     we split addresses on the first one.
+func isSafeAddress(addr string) bool {
+	return strings.Count(addr, "@") == 1 &&
+		!strings.ContainsFunc(addr, unicode.IsControl)
+}
+
 // Placeholder EHLO domain, used when the one given by the client has no valid
 // characters. The .invalid TLD is reserved for names that are guaranteed to
 // be invalid (RFC 6761).
diff --git a/internal/smtpsrv/conn_test.go b/internal/smtpsrv/conn_test.go
index 75e538a..552c452 100644
--- a/internal/smtpsrv/conn_test.go
+++ b/internal/smtpsrv/conn_test.go
@@ -2,6 +2,7 @@ package smtpsrv
 
 import (
 	"net"
+	"strings"
 	"testing"
 
 	"blitiri.com.ar/go/chasquid/internal/domaininfo"
@@ -113,6 +114,45 @@ func TestAddrLiteral(t *testing.T) {
 	}
 }
 
+func TestReceivedHeaderEnvelopeFrom(t *testing.T) {
+	c := &Conn{
+		hostname:   "mx.example.com",
+		ehloDomain: "client.example.com",
+		remoteAddr: &net.TCPAddr{IP: net.ParseIP("192.0.2.1"), Port: 1234},
+		tr:         trace.New("test", "TestReceivedHeaderEnvelopeFrom"),
+		// The local part could have been quoted, see isSafeAddress.
+		mailFrom: `x)(y; z\@example.org`,
+		data:     []byte("Subject: test\n\nbody\n"),
+	}
+	defer c.tr.Finish()
+	c.addReceivedHeader()
+
+	hdr, _, _ := strings.Cut(string(c.data), "Subject:")
+	want := `envelope from "x\)\(y; z\\@example.org")`
+	if !strings.Contains(hdr, want) {
+		t.Errorf("Received header does not contain %q:\n%s", want, hdr)
+	}
+
+	// Parentheses must be balanced, considering the escaped ones.
+	depth := 0
+	for i := 0; i < len(hdr); i++ {
+		switch hdr[i] {
+		case '\\':
+			i++ // Skip the escaped character.
+		case '(':
+			depth++
+		case ')':
+			depth--
+			if depth < 0 {
+				t.Fatalf("unbalanced ')' at %d:\n%s", i, hdr)
+			}
+		}
+	}
+	if depth != 0 {
+		t.Errorf("unbalanced comments (depth %d):\n%s", depth, hdr)
+	}
+}
+
 func TestSanitizeEHLODomain(t *testing.T) {
 	equal := []string{
 		"domain", "do.main", "do-main",
@@ -168,3 +208,29 @@ func TestEHLODomainIsSanitized(t *testing.T) {
 		}
 	}
 }
+
+func TestIsSafeAddress(t *testing.T) {
+	safe := []string{
+		"john@example.com", "ñandú@example.com", "用户@例子.广告",
+		// Characters that need quoting are fine, we're lenient with them.
+		"a;b@example.com", "a(b)@example.com", `a"b@example.com`,
+	}
+	for _, addr := range safe {
+		if !isSafeAddress(addr) {
+			t.Errorf("%q: expected safe, got unsafe", addr)
+		}
+	}
+
+	unsafe := []string{
+		// More than one "@" (or none).
+		"a@b@example.com", "@a@example.com", "example.com",
+		// Control characters (C1, U+009B is CSI).
+		"a\u0081b@example.com", "a\u009b31mb@example.com",
+		"a@exam\u0085ple.com",
+	}
+	for _, addr := range unsafe {
+		if isSafeAddress(addr) {
+			t.Errorf("%q: expected unsafe, got safe", addr)
+		}
+	}
+}
diff --git a/internal/smtpsrv/server_test.go b/internal/smtpsrv/server_test.go
index 7fd3d84..3540237 100644
--- a/internal/smtpsrv/server_test.go
+++ b/internal/smtpsrv/server_test.go
@@ -277,6 +277,43 @@ func TestMalformedDomain(t *testing.T) {
 	expectTPErr(t, err, 501, "5.1.2 Malformed destination domain")
 }
 
+func TestUnsafeAddress(t *testing.T) {
+	// Unauthenticated MAIL FROM.
+	c := mustDial(t, ModeSMTP, false)
+	defer c.Close()
+
+	err := c.Mail(`"a@b"@from`)
+	expectTPErr(t, err, 501, "5.1.7 Sender address malformed")
+
+	err = c.Mail("a\u009bb@from")
+	expectTPErr(t, err, 501, "5.1.7 Sender address malformed")
+
+	// Characters that need quoting are fine.
+	if err := c.Mail(`"a;b"@from`); err != nil {
+		t.Errorf("Mail: %v", err)
+	}
+
+	// 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"@remote`)
+	expectTPErr(t, err, 501, "5.1.3 Malformed destination address")
+
+	err = c.Rcpt("a\u009bb@remote")
+	expectTPErr(t, err, 501, "5.1.3 Malformed destination address")
+}
+
 func TestNullMailFrom(t *testing.T) {
 	c := mustDial(t, ModeSMTP, false)
 	defer c.Close()