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 <john@wargan.com> 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

@@ -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
+	// Never return. The serve goroutines only return if their listener is
-	// execution.
+	// 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,

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 ===
 //