git » chasquid » commit 85305f4

smtpsrv: Close the connection after 3 errors (lowering from 10)

author Alberto Bertogli
2021-06-10 17:41:28 UTC
committer Alberto Bertogli
2021-06-11 09:34:20 UTC
parent 44eb0b903a82d1ef3cc4ec23135432c0af44c133
smtpsrv: Close the connection after 3 errors (lowering from 10)

Today, we close the connection after 10 errors. While this is fine for
normal use, it is unnecessarily large.

Lowering it to 3 helps with defense-in-depth for cross-protocol attacks
(e.g. https://alpaca-attack.com/), while still being large enough for
useful troubleshooting and normal operation.

As part of this change, we also remove the AUTH-specific failures limit,
because they're covered by the connection limit.
internal/smtpsrv/conn.go +3 -9
internal/smtpsrv/server_test.go +11 -11
test/t-12-minor_dialogs/auth_multi_dialog.cmy +7 -0
test/t-12-minor_dialogs/auth_too_many_failures.cmy +1 -5
test/t-12-minor_dialogs/bad_data.cmy +7 -0
test/t-12-minor_dialogs/bad_mail_from.cmy +14 -0
test/t-12-minor_dialogs/bad_rcpt_to.cmy +18 -0 
diff --git a/internal/smtpsrv/conn.go b/internal/smtpsrv/conn.go
index 3397331..e6d3d54 100644
--- a/internal/smtpsrv/conn.go
+++ b/internal/smtpsrv/conn.go
@@ -145,9 +145,6 @@ type Conn struct {
 	// Have we successfully completed AUTH?
 	completedAuth bool
 
-	// How many times have we attempted AUTH?
-	authAttempts int
-
 	// Authenticated user and domain, empty if !completedAuth.
 	authUser   string
 	authDomain string
@@ -291,8 +288,10 @@ loop:
 				// Be verbose about errors, to help troubleshooting.
 				c.tr.Errorf("%s failed: %d  %s", cmd, code, msg)
 
+				// Close the connection after 3 errors.
+				// This helps prevent cross-protocol attacks.
 				errCount++
-				if errCount > 10 {
+				if errCount >= 3 {
 					// https://tools.ietf.org/html/rfc5321#section-4.3.2
 					c.tr.Errorf("too many errors, breaking connection")
 					_ = c.writeResponse(421, "4.5.0 Too many errors, bye")
@@ -1016,11 +1015,6 @@ func (c *Conn) AUTH(params string) (code int, msg string) {
 		return 503, "5.5.1 You are already wearing that!"
 	}
 
-	if c.authAttempts > 3 {
-		return 503, "5.7.8 Too many attempts, go away"
-	}
-	c.authAttempts++
-
 	// We only support PLAIN for now, so no need to make this too complicated.
 	// Params should be either "PLAIN" or "PLAIN <response>".
 	// If the response is not there, we reply with 334, and expect the
diff --git a/internal/smtpsrv/server_test.go b/internal/smtpsrv/server_test.go
index 675a658..891ee2e 100644
--- a/internal/smtpsrv/server_test.go
+++ b/internal/smtpsrv/server_test.go
@@ -214,25 +214,25 @@ func TestBrokenAuth(t *testing.T) {
 }
 
 func TestWrongMailParsing(t *testing.T) {
-	c := mustDial(t, ModeSMTP, false)
-	defer c.Close()
-
 	addrs := []string{"from", "a b c", "a @ b", "<x>", "<x y>", "><"}
-
 	for _, addr := range addrs {
+		c := mustDial(t, ModeSMTP, false)
+
 		if err := c.Mail(addr); err == nil {
 			t.Errorf("Mail not failed as expected with %q", addr)
 		}
-	}
 
-	if err := c.Mail("from@plain"); err != nil {
-		t.Errorf("Mail: %v", err)
-	}
+		if err := c.Mail("from@plain"); err != nil {
+			t.Errorf("Mail: %v", err)
+		}
 
-	for _, addr := range addrs {
-		if err := c.Rcpt(addr); err == nil {
-			t.Errorf("Rcpt not failed as expected with %q", addr)
+		for _, addr := range addrs {
+			if err := c.Rcpt(addr); err == nil {
+				t.Errorf("Rcpt not failed as expected with %q", addr)
+			}
 		}
+
+		c.Close()
 	}
 }
 
diff --git a/test/t-12-minor_dialogs/auth_multi_dialog.cmy b/test/t-12-minor_dialogs/auth_multi_dialog.cmy
index 53dbee0..ffd53f2 100644
--- a/test/t-12-minor_dialogs/auth_multi_dialog.cmy
+++ b/test/t-12-minor_dialogs/auth_multi_dialog.cmy
@@ -13,6 +13,13 @@ c <~ 334
 c -> dXNlckB0ZXN0c2VydmVyAHlalala==
 c <~ 501 5.5.2 Error decoding AUTH response
 
+# Reconnect to avoid getting rejected due to too many errors.
+c close
+c tls_connect localhost:1465
+c <~ 220
+c -> EHLO localhost
+c <... 250 HELP
+
 c -> AUTH PLAIN
 c <~ 334
 c -> dXNlckB0ZXN0c2VydmVyAHVzZXJAdGVzdHNlcnZlcgB3cm9uZ3Bhc3N3b3Jk
diff --git a/test/t-12-minor_dialogs/auth_too_many_failures.cmy b/test/t-12-minor_dialogs/auth_too_many_failures.cmy
index 76d5cbe..3b6ddfd 100644
--- a/test/t-12-minor_dialogs/auth_too_many_failures.cmy
+++ b/test/t-12-minor_dialogs/auth_too_many_failures.cmy
@@ -10,9 +10,5 @@ c <~ 501
 c -> AUTH PLAIN something
 c <~ 501
 c -> AUTH PLAIN something
-c <~ 501
-c -> AUTH PLAIN something
-c <~ 501
-c -> AUTH PLAIN something
-c <~ 503 5.7.8 Too many attempts, go away
+c <~ 421 4.5.0 Too many errors, bye
 
diff --git a/test/t-12-minor_dialogs/bad_data.cmy b/test/t-12-minor_dialogs/bad_data.cmy
index 6fdcac3..5ca434e 100644
--- a/test/t-12-minor_dialogs/bad_data.cmy
+++ b/test/t-12-minor_dialogs/bad_data.cmy
@@ -11,6 +11,13 @@ c <~ 250
 c -> DATA
 c <- 503 5.5.1 Sender not yet given
 
+# Reconnect to avoid getting rejected due to too many errors.
+c close
+c tcp_connect localhost:1025
+c <~ 220
+c -> HELO localhost
+c <~ 250
+
 c -> MAIL FROM:<a@b>
 c <~ 250
 c -> RCPT TO: user@testserver
diff --git a/test/t-12-minor_dialogs/bad_mail_from.cmy b/test/t-12-minor_dialogs/bad_mail_from.cmy
index fbb430e..fe7632e 100644
--- a/test/t-12-minor_dialogs/bad_mail_from.cmy
+++ b/test/t-12-minor_dialogs/bad_mail_from.cmy
@@ -10,11 +10,25 @@ c <- 500 5.5.2 Unknown command
 c -> MAIL FROM:
 c <~ 500
 
+# Reconnect to avoid getting rejected due to too many errors.
+c close
+c tcp_connect localhost:1025
+c <~ 220
+c -> HELO localhost
+c <~ 250
+
 c -> MAIL FROM:<pepe>
 c <~ 501
 
 c -> MAIL FROM:<a@xn--->
 c <- 501 5.1.8 Malformed sender domain (IDNA conversion failed)
 
+# Reconnect to avoid getting rejected due to too many errors.
+c close
+c tcp_connect localhost:1025
+c <~ 220
+c -> HELO localhost
+c <~ 250
+
 c -> MAIL FROM:<aaaa5aaaaXaaaa5aaaaXaaaa5aaaaXaaaa5aaaaXaaaa5aaaaXaaaa5aaaaXaaaa5aaaaXaaaa5aaaaXaaaa5aaaaXaaaa5aaaaXaaaa5aaaaXaaaa5aaaaXaaaa5aaaaXaaaa5aaaaXaaaa5aaaaX@bbbb5bbbbXbbbb5bbbbXbbbb5bbbbXbbbb5bbbbXbbbb5bbbbXbbbb5bbbbXbbbb5bbbbXbbbb5bbbbXbbbb5bbbbXbbbb5bbbbXbbbb5bbbbXbbbb5bbbbXbbbb5bbbbXbbbb5bbbbXbbbb5bbbbX>
 c <- 501 5.1.7 Sender address too long
diff --git a/test/t-12-minor_dialogs/bad_rcpt_to.cmy b/test/t-12-minor_dialogs/bad_rcpt_to.cmy
index e31ee89..dbb4b42 100644
--- a/test/t-12-minor_dialogs/bad_rcpt_to.cmy
+++ b/test/t-12-minor_dialogs/bad_rcpt_to.cmy
@@ -13,12 +13,30 @@ c <- 500 5.5.2 Unknown command
 c -> RCPT TO:
 c <~ 500
 
+# Reconnect to avoid getting rejected due to too many errors.
+c close
+c tcp_connect localhost:1025
+c <~ 220
+c -> HELO localhost
+c <~ 250
+c -> MAIL FROM:<test@testy.com>
+c <~ 250
+
 c -> RCPT TO:<pepe>
 c <~ 501
 
 c -> RCPT TO:<a@xn--->
 c <- 501 5.1.2 Malformed destination domain (IDNA conversion failed)
 
+# Reconnect to avoid getting rejected due to too many errors.
+c close
+c tcp_connect localhost:1025
+c <~ 220
+c -> HELO localhost
+c <~ 250
+c -> MAIL FROM:<test@testy.com>
+c <~ 250
+
 c -> RCPT TO:<henryⅣ@testserver>
 c <- 550 5.1.3 Destination address is invalid