Add checks to prevent unauthorized relaying and impersonation

This patch adds checks that verify:

 - The envelope from must match the authenticated user. This prevents
   impersonation at the envelope level (while still allowing bounces, of
   course).
 - If the destination is remote, then the user must have completed
   authentication. This prevents unauthorized relaying.

The patch ends up adjusting quite a few tests, as they were not written
considering these restrictions so they have to be changed accordingly.

chasquid.go

@@ -22,6 +22,7 @@ import (
 	"blitiri.com.ar/go/chasquid/internal/auth"
 	"blitiri.com.ar/go/chasquid/internal/config"
 	"blitiri.com.ar/go/chasquid/internal/courier"
+	"blitiri.com.ar/go/chasquid/internal/envelope"
 	"blitiri.com.ar/go/chasquid/internal/queue"
 	"blitiri.com.ar/go/chasquid/internal/set"
 	"blitiri.com.ar/go/chasquid/internal/systemd"
@@ -320,6 +321,7 @@ func (s *Server) serve(l net.Listener, mode SocketMode) {
 			mode:           mode,
 			tlsConfig:      s.tlsConfig,
 			userDBs:        s.userDBs,
+			localDomains:   s.localDomains,
 			deadline:       time.Now().Add(s.connTimeout),
 			commandTimeout: s.commandTimeout,
 			queue:          s.queue,
@@ -356,8 +358,9 @@ type Conn struct {
 	// Are we using TLS?
 	onTLS bool
 
-	// User databases - taken from the server at creation time.
+	// User databases and local domains, taken from the server at creation time.
 	userDBs      map[string]*userdb.DB
+	localDomains *set.String
 
 	// Have we successfully completed AUTH?
 	completedAuth bool
@@ -560,6 +563,14 @@ func (c *Conn) MAIL(params string) (code int, msg string) {
 	// but that's not according to the RFC. We reset the envelope instead.
 	c.resetEnvelope()
 
+	// If the source is local, check that it completed auth for that user.
+	if e.Address != "<>" && envelope.DomainIn(e.Address, c.localDomains) {
+		user, domain := envelope.Split(e.Address)
+		if user != c.authUser || domain != c.authDomain {
+			return 503, "user not authorized"
+		}
+	}
+
 	c.mailFrom = e.Address
 	return 250, "You feel like you are being watched"
 }
@@ -572,6 +583,11 @@ func (c *Conn) RCPT(params string) (code int, msg string) {
 		return 500, "unknown command"
 	}
 
+	// RFC says 100 is the minimum limit for this, but it seems excessive.
+	if len(c.rcptTo) > 100 {
+		return 503, "too many recipients"
+	}
+
 	// TODO: Write our own parser (we have different needs, mail.ParseAddress
 	// is useful for other things).
 	// Allow utf8, but prevent "control" characters.
@@ -585,16 +601,11 @@ func (c *Conn) RCPT(params string) (code int, msg string) {
 		return 503, "sender not yet given"
 	}
 
-	// RFC says 100 is the minimum limit for this, but it seems excessive.
-	if len(c.rcptTo) > 100 {
-		return
+	remoteDst := !envelope.DomainIn(e.Address, c.localDomains)
+	if remoteDst && !c.completedAuth {
+		return 503, "relay not allowed"
 	}
 
-	// TODO: do we allow receivers without a domain?
-	// TODO: check the case:
-	//  - local recipient, always ok
-	//  - external recipient, only ok if mailFrom is local (needs auth)
-
 	c.rcptTo = append(c.rcptTo, e.Address)
 	return 250, "You have an eerie feeling..."
 }

chasquid_test.go

@@ -24,16 +24,18 @@ import (
 
 // Flags.
 var (
-	externalServerAddr = flag.String("external_server_addr", "",
-		"address of the external server to test (defaults to use internal)")
+	externalSMTPAddr = flag.String("external_smtp_addr", "",
+		"SMTP server address to test (defaults to use internal)")
+	externalSubmissionAddr = flag.String("external_submission_addr", "",
+		"submission server address to test (defaults to use internal)")
 )
 
 var (
-	// Server address.
-	// We default to an internal one, but may get overriden via
-	// --external_server_addr.
+	// Server addresses.
+	// We default to internal ones, but may get overriden via flags.
 	// TODO: Don't hard-code the default.
-	srvAddr = "127.0.0.1:13453"
+	smtpAddr       = "127.0.0.1:13444"
+	submissionAddr = "127.0.0.1:13999"
 
 	// TLS configuration to use in the clients.
 	// Will contain the generated server certificate as root CA.
@@ -44,8 +46,14 @@ var (
 // === Tests ===
 //
 
-func mustDial(tb testing.TB, useTLS bool) *smtp.Client {
-	c, err := smtp.Dial(srvAddr)
+func mustDial(tb testing.TB, mode SocketMode, useTLS bool) *smtp.Client {
+	addr := ""
+	if mode == ModeSMTP {
+		addr = smtpAddr
+	} else {
+		addr = submissionAddr
+	}
+	c, err := smtp.Dial(addr)
 	if err != nil {
 		tb.Fatalf("smtp.Dial: %v", err)
 	}
@@ -73,18 +81,23 @@ func sendEmail(tb testing.TB, c *smtp.Client) {
 
 func sendEmailWithAuth(tb testing.TB, c *smtp.Client, auth smtp.Auth) {
 	var err error
+	from := "from@from"
 
 	if auth != nil {
 		if err = c.Auth(auth); err != nil {
 			tb.Errorf("Auth: %v", err)
 		}
+
+		// If we authenticated, we must use the user as from, as the server
+		// checks otherwise.
+		from = "testuser@localhost"
 	}
 
-	if err = c.Mail("from@from"); err != nil {
+	if err = c.Mail(from); err != nil {
 		tb.Errorf("Mail: %v", err)
 	}
 
-	if err = c.Rcpt("to@to"); err != nil {
+	if err = c.Rcpt("to@localhost"); err != nil {
 		tb.Errorf("Rcpt: %v", err)
 	}
 
@@ -104,19 +117,19 @@ func sendEmailWithAuth(tb testing.TB, c *smtp.Client, auth smtp.Auth) {
 }
 
 func TestSimple(t *testing.T) {
-	c := mustDial(t, false)
+	c := mustDial(t, ModeSMTP, false)
 	defer c.Close()
 	sendEmail(t, c)
 }
 
 func TestSimpleTLS(t *testing.T) {
-	c := mustDial(t, true)
+	c := mustDial(t, ModeSMTP, true)
 	defer c.Close()
 	sendEmail(t, c)
 }
 
 func TestManyEmails(t *testing.T) {
-	c := mustDial(t, true)
+	c := mustDial(t, ModeSMTP, true)
 	defer c.Close()
 	sendEmail(t, c)
 	sendEmail(t, c)
@@ -124,15 +137,26 @@ func TestManyEmails(t *testing.T) {
 }
 
 func TestAuth(t *testing.T) {
-	c := mustDial(t, true)
+	c := mustDial(t, ModeSubmission, true)
 	defer c.Close()
 
 	auth := smtp.PlainAuth("", "testuser@localhost", "testpasswd", "127.0.0.1")
 	sendEmailWithAuth(t, c, auth)
 }
 
+func TestAuthOnSMTP(t *testing.T) {
+	c := mustDial(t, ModeSMTP, true)
+	defer c.Close()
+
+	auth := smtp.PlainAuth("", "testuser@localhost", "testpasswd", "127.0.0.1")
+
+	// At least for now, we allow AUTH over the SMTP port to avoid unnecessary
+	// complexity, so we expect it to work.
+	sendEmailWithAuth(t, c, auth)
+}
+
 func TestWrongMailParsing(t *testing.T) {
-	c := mustDial(t, false)
+	c := mustDial(t, ModeSMTP, false)
 	defer c.Close()
 
 	addrs := []string{"from", "a b c", "a @ b", "<x>", "<x y>", "><"}
@@ -155,7 +179,7 @@ func TestWrongMailParsing(t *testing.T) {
 }
 
 func TestNullMailFrom(t *testing.T) {
-	c := mustDial(t, false)
+	c := mustDial(t, ModeSMTP, false)
 	defer c.Close()
 
 	addrs := []string{"", "<>", "  <>", " <  > "}
@@ -167,7 +191,7 @@ func TestNullMailFrom(t *testing.T) {
 }
 
 func TestRcptBeforeMail(t *testing.T) {
-	c := mustDial(t, false)
+	c := mustDial(t, ModeSMTP, false)
 	defer c.Close()
 
 	if err := c.Rcpt("to@to"); err == nil {
@@ -175,6 +199,28 @@ func TestRcptBeforeMail(t *testing.T) {
 	}
 }
 
+func TestLocalHasAuthenticated(t *testing.T) {
+	c := mustDial(t, ModeSubmission, false)
+	defer c.Close()
+
+	if err := c.Mail("from@localhost"); err == nil {
+		t.Errorf("Accepted non-authenticated local mail")
+	}
+}
+
+func TestRelayForbidden(t *testing.T) {
+	c := mustDial(t, ModeSMTP, false)
+	defer c.Close()
+
+	if err := c.Mail("from@somewhere"); err != nil {
+		t.Errorf("Mail: %v", err)
+	}
+
+	if err := c.Rcpt("to@somewhere"); err == nil {
+		t.Errorf("Accepted relay email")
+	}
+}
+
 func simpleCmd(t *testing.T, c *smtp.Client, cmd string, expected int) {
 	if err := c.Text.PrintfLine(cmd); err != nil {
 		t.Fatalf("Failed to write %s: %v", cmd, err)
@@ -186,7 +232,7 @@ func simpleCmd(t *testing.T, c *smtp.Client, cmd string, expected int) {
 }
 
 func TestSimpleCommands(t *testing.T) {
-	c := mustDial(t, false)
+	c := mustDial(t, ModeSMTP, false)
 	defer c.Close()
 	simpleCmd(t, c, "HELP", 214)
 	simpleCmd(t, c, "NOOP", 250)
@@ -195,7 +241,7 @@ func TestSimpleCommands(t *testing.T) {
 }
 
 func TestReset(t *testing.T) {
-	c := mustDial(t, false)
+	c := mustDial(t, ModeSMTP, false)
 	defer c.Close()
 
 	if err := c.Mail("from@from"); err != nil {
@@ -212,7 +258,7 @@ func TestReset(t *testing.T) {
 }
 
 func TestRepeatedStartTLS(t *testing.T) {
-	c, err := smtp.Dial(srvAddr)
+	c, err := smtp.Dial(smtpAddr)
 	if err != nil {
 		t.Fatalf("smtp.Dial: %v", err)
 	}
@@ -231,7 +277,7 @@ func TestRepeatedStartTLS(t *testing.T) {
 //
 
 func BenchmarkManyEmails(b *testing.B) {
-	c := mustDial(b, false)
+	c := mustDial(b, ModeSMTP, false)
 	defer c.Close()
 
 	b.ResetTimer()
@@ -245,7 +291,7 @@ func BenchmarkManyEmails(b *testing.B) {
 
 func BenchmarkManyEmailsParallel(b *testing.B) {
 	b.RunParallel(func(pb *testing.PB) {
-		c := mustDial(b, false)
+		c := mustDial(b, ModeSMTP, false)
 		defer c.Close()
 
 		for pb.Next() {
@@ -355,8 +401,9 @@ func realMain(m *testing.M) int {
 	flag.Parse()
 	defer glog.Flush()
 
-	if *externalServerAddr != "" {
-		srvAddr = *externalServerAddr
+	if *externalSMTPAddr != "" {
+		smtpAddr = *externalSMTPAddr
+		submissionAddr = *externalSubmissionAddr
 		tlsConfig = &tls.Config{
 			InsecureSkipVerify: true,
 		}
@@ -379,16 +426,19 @@ func realMain(m *testing.M) int {
 		s.Hostname = "localhost"
 		s.MaxDataSize = 50 * 1024 * 1025
 		s.AddCerts(tmpDir+"/cert.pem", tmpDir+"/key.pem")
-		s.AddAddr(srvAddr, ModeSMTP)
+		s.AddAddr(smtpAddr, ModeSMTP)
+		s.AddAddr(submissionAddr, ModeSubmission)
 
 		udb := userdb.New("/dev/null")
 		udb.AddUser("testuser", "testpasswd")
+		s.AddDomain("localhost")
 		s.AddUserDB("localhost", udb)
 
 		go s.ListenAndServe()
 	}
 
-	waitForServer(srvAddr)
+	waitForServer(smtpAddr)
+	waitForServer(submissionAddr)
 	return m.Run()
 }
 

internal/envelope/envelope.go

@@ -9,7 +9,7 @@ import (
 )
 
 // Split an user@domain address into user and domain.
-func split(addr string) (string, string) {
+func Split(addr string) (string, string) {
 	ps := strings.SplitN(addr, "@", 2)
 	if len(ps) != 2 {
 		return addr, ""
@@ -19,12 +19,12 @@ func split(addr string) (string, string) {
 }
 
 func UserOf(addr string) string {
-	user, _ := split(addr)
+	user, _ := Split(addr)
 	return user
 }
 
 func DomainOf(addr string) string {
-	_, domain := split(addr)
+	_, domain := Split(addr)
 	return domain
 }
 

test/t-01-simple_local/msmtprc

@@ -1,7 +1,7 @@
 account default
 
 host testserver
-port 1025
+port 1587
 
 tls on
 tls_trust_file config/domains/testserver/cert.pem
@@ -12,6 +12,9 @@ auth on
 user user@testserver
 password secretpassword
 
+account smtpport : default
+port 1025
+
 account baduser : default
 user unknownuser@testserver
 password secretpassword

test/t-01-simple_local/run.sh

@@ -7,6 +7,7 @@ init
 
 generate_certs_for testserver
 
+mkdir -p .logs
 chasquid -v=2 --log_dir=.logs --config_dir=config &
 wait_until_ready 1025
 
@@ -16,6 +17,13 @@ wait_for_file .mail/someone@testserver
 
 mail_diff content .mail/someone@testserver
 
+# At least for now, we allow AUTH over the SMTP port to avoid unnecessary
+# complexity, so we expect it to work.
+if ! run_msmtp -a smtpport someone@testserver < content 2> /dev/null; then
+	echo "ERROR: failed auth on the SMTP port"
+	exit 1
+fi
+
 if run_msmtp -a baduser someone@testserver < content 2> /dev/null; then
 	echo "ERROR: successfully sent an email with a bad password"
 	exit 1

test/t-02-exim/config/exim4.in

@@ -31,6 +31,13 @@ acl_check_data:
   accept
 
 
+# Rewrite envelope-from to server@srv-exim.
+# This is so when we redirect, we don't use user@srv-chasquid in the
+# envelope-from (we're not authorized to send mail on behalf of
+# @srv-chasquid).
+begin rewrite
+user@srv-chasquid  server@srv-exim F
+
 # Forward all incoming email to chasquid (running on :1025 in this test).
 begin routers
 

test/t-02-exim/msmtprc

@@ -1,7 +1,7 @@
 account default
 
 host srv-chasquid
-port 1025
+port 1587
 
 tls on
 tls_trust_file config/domains/srv-chasquid/cert.pem

test/t-02-exim/run.sh

@@ -37,6 +37,7 @@ generate_certs_for srv-chasquid
 # Launch chasquid at port 1025 (in config).
 # Use outgoing port 2025 which is where exim will be at.
 # Bypass MX lookup, so it can find srv-exim (via our host alias).
+mkdir -p .logs
 chasquid -v=2 --log_dir=.logs --config_dir=config \
 	--testing__outgoing_smtp_port=2025 \
 	--testing__bypass_mx_lookup &

test/util/mail_diff

@@ -25,6 +25,11 @@ for h, val in expected.items():
 
 if expected.get_payload() != msg.get_payload():
 	diff = True
+
+	if expected.is_multipart() != msg.is_multipart():
+		print("Multipart differs, expected %s, got %s" % (
+			expected.is_multipart(), msg.is_multipart()))
+	elif not msg.is_multipart():
 		exp = expected.get_payload().splitlines()
 		got = msg.get_payload().splitlines()
 		print("Payload differs:")