From 85305f4bd9320d68cb96be100fd60d6a43a25c87 Mon Sep 17 00:00:00 2001 From: Alberto Bertogli Date: Thu, 10 Jun 2021 18:41:28 +0100 Subject: [PATCH] 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 | 12 +++------- internal/smtpsrv/server_test.go | 24 +++++++++---------- test/t-12-minor_dialogs/auth_multi_dialog.cmy | 7 ++++++ .../auth_too_many_failures.cmy | 6 +---- test/t-12-minor_dialogs/bad_data.cmy | 7 ++++++ test/t-12-minor_dialogs/bad_mail_from.cmy | 14 +++++++++++ test/t-12-minor_dialogs/bad_rcpt_to.cmy | 18 ++++++++++++++ 7 files changed, 62 insertions(+), 26 deletions(-) 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 ". // 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", "", "", "><"} - 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) - } - - for _, addr := range addrs { - if err := c.Rcpt(addr); err == nil { - t.Errorf("Rcpt not failed as expected with %q", addr) + 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) + } + } + + 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: 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: c <~ 501 c -> MAIL FROM: 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: 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: +c <~ 250 + c -> RCPT TO: c <~ 501 c -> RCPT TO: 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: +c <~ 250 + c -> RCPT TO: c <- 550 5.1.3 Destination address is invalid