From 6dc05d2aad9bfd77876fde7b385ffa5d83440775 Mon Sep 17 00:00:00 2001 From: Alberto Bertogli Date: Fri, 9 Oct 2026 22:57:47 +0100 Subject: [PATCH] smtpsrv: Do not exit on Accept errors Today, if Accept returns an error, we exit the daemon. Unfortunately this is not that difficult to trigger: a remote client can open a large number of connections to reach the limit of file descriptors. This patch makes the server retry instead, with an exponential backoff (up to 1s). This is a compromise to improve handling of such scenarios without introducing a lot of complexity. The problem can still be triggered but this change makes the daemon more resilient to it (and it will be improved further in follow-up patches). Users who have to be very defensive can deploy OS-level safeguards (such as firewall rules to limit number of connections, etc.) which are more effective and tunable. Thanks to John JEAN for finding and reporting this problem. The detailed report can be found in https://johnjean.io/advisories/chasquid-unauthenticated-remote-dos/. --- internal/smtpsrv/server.go | 43 ++++++++++++++++++-- internal/smtpsrv/server_test.go | 72 +++++++++++++++++++++++++++++++++ 2 files changed, 112 insertions(+), 3 deletions(-) diff --git a/internal/smtpsrv/server.go b/internal/smtpsrv/server.go index c8d28c9..04a6445 100644 --- a/internal/smtpsrv/server.go +++ b/internal/smtpsrv/server.go @@ -8,6 +8,7 @@ import ( "crypto/tls" "crypto/x509" "encoding/pem" + "errors" "flag" "fmt" "net" @@ -23,6 +24,7 @@ import ( "blitiri.com.ar/go/chasquid/internal/courier" "blitiri.com.ar/go/chasquid/internal/dkim" "blitiri.com.ar/go/chasquid/internal/domaininfo" + "blitiri.com.ar/go/chasquid/internal/expvarom" "blitiri.com.ar/go/chasquid/internal/localrpc" "blitiri.com.ar/go/chasquid/internal/maillog" "blitiri.com.ar/go/chasquid/internal/queue" @@ -40,6 +42,18 @@ var ( "how often to reload, ONLY FOR TESTING") ) +// Backoff between Accept errors. +const ( + acceptMinBackoff = 5 * time.Millisecond + acceptMaxBackoff = 1 * time.Second +) + +// Exported variables. +var ( + acceptErrors = expvarom.NewInt("chasquid/smtpIn/acceptErrors", + "count of errors accepting incoming connections") +) + // Server represents an SMTP server instance. type Server struct { // Main hostname, used for display only. @@ -338,8 +352,8 @@ func (s *Server) ListenAndServe() { } } - // Never return. If the serve goroutines have problems, they will abort - // execution. + // Never return. The serve goroutines only return if their listener is + // closed, which doesn't happen in practice. select {} } @@ -351,11 +365,34 @@ func (s *Server) serve(l net.Listener, mode SocketMode) { pdhook := path.Join(s.HookPath, "post-data") + backoff := acceptMinBackoff + var lastErrLog time.Time + for { conn, err := l.Accept() if err != nil { - log.Fatalf("Error accepting: %v", err) + if errors.Is(err, net.ErrClosed) { + // We never close the listeners, so this is unexpected. + log.Errorf("Listener %s closed, no longer accepting: %v", + l.Addr(), err) + return + } + + // Other errors are usually transient (e.g. running out of file + // descriptors), so we back off and retry instead of exiting, + // which would make it trivial to take the server down. + // Log at most once a minute to avoid flooding the logs. + acceptErrors.Add(1) + if time.Since(lastErrLog) >= time.Minute { + log.Errorf("Error accepting on %s (will retry): %v", + l.Addr(), err) + lastErrLog = time.Now() + } + time.Sleep(backoff) + backoff = min(backoff*2, acceptMaxBackoff) + continue } + backoff = acceptMinBackoff sc := &Conn{ hostname: s.Hostname, diff --git a/internal/smtpsrv/server_test.go b/internal/smtpsrv/server_test.go index cccb599..b874ab3 100644 --- a/internal/smtpsrv/server_test.go +++ b/internal/smtpsrv/server_test.go @@ -1,6 +1,7 @@ package smtpsrv import ( + "bufio" "crypto/tls" "errors" "flag" @@ -11,7 +12,9 @@ import ( "net/textproto" "os" "strings" + "syscall" "testing" + "time" "blitiri.com.ar/go/chasquid/internal/aliases" "blitiri.com.ar/go/chasquid/internal/auth" @@ -572,6 +575,75 @@ MC4CAQAwBQYDK2VwBCIEID6bjSoiW6g6NJA67RNl0SZ7zpylVOq9w/VGAXF5whnS } } +// scriptedListener is a net.Listener that returns a predefined sequence of +// results from Accept, and net.ErrClosed once they run out. +type scriptedListener struct { + results []acceptResult +} + +type acceptResult struct { + conn net.Conn + err error +} + +func (l *scriptedListener) Accept() (net.Conn, error) { + if len(l.results) == 0 { + return nil, net.ErrClosed + } + r := l.results[0] + l.results = l.results[1:] + return r.conn, r.err +} + +func (l *scriptedListener) Close() error { return nil } +func (l *scriptedListener) Addr() net.Addr { return &net.TCPAddr{} } + +func TestAcceptErrors(t *testing.T) { + srvConn, cliConn := net.Pipe() + defer cliConn.Close() + + emfile := &net.OpError{Op: "accept", Net: "tcp", + Err: os.NewSyscallError("accept4", syscall.EMFILE)} + l := &scriptedListener{results: []acceptResult{ + {err: emfile}, + {err: emfile}, + {conn: srvConn}, + {err: emfile}, + }} + + s := NewServer() + s.Hostname = "acceptsrv" + prevErrors := acceptErrors.Value() + + // serve should retry on errors, and return once the listener reports + // net.ErrClosed. + done := make(chan struct{}) + go func() { + s.serve(l, ModeSMTP) + close(done) + }() + + // The connection we got in between the errors must have been handled. + // net.Pipe is synchronous, so this also unblocks the server's write. + greeting, err := textproto.NewReader(bufio.NewReader(cliConn)).ReadLine() + if err != nil { + t.Fatalf("error reading greeting: %v", err) + } + if greeting != "220 acceptsrv ESMTP chasquid" { + t.Errorf("unexpected greeting: %q", greeting) + } + + select { + case <-done: + case <-time.After(5 * time.Second): + t.Fatalf("serve did not return after net.ErrClosed") + } + + if n := acceptErrors.Value() - prevErrors; n != 3 { + t.Errorf("acceptErrors increased by %d, expected 3", n) + } +} + // // === Benchmarks === //