Harden Gopher parsing and recover server goroutinesHEADv1.6.4main

Reject oversized or malformed Gopher selectors as parse errors instead of panicking. Keep search requests bounded, and wrap shared server goroutines with panic logging so one bad connection cannot take down the process.

4 files changed, 143 insertions, 15 deletions

diff --git a/gopher/request.go b/gopher/request.go
index fdd7480..3aeaf74 100644
--- a/gopher/request.go
+++ b/gopher/request.go
@@ -11,6 +11,11 @@ import (
 	"tildegit.org/tjp/sliderule/internal/types"
 )
 
+const (
+	maxSelectorLen = 255
+	maxRequestLen  = 4096
+)
+
 // ParseRequest parses a gopher protocol request into a sliderule.Request object.
 func ParseRequest(rdr io.Reader) (*types.Request, error) {
 	selector, search, err := readFullRequest(rdr)
@@ -34,9 +39,7 @@ func ParseRequest(rdr io.Reader) (*types.Request, error) {
 }
 
 func readFullRequest(rdr io.Reader) (string, string, error) {
-	// The vast majority of requests will fit in this size:
-	// the specified 255 byte max for selector, then CRLF.
-	buf := make([]byte, 257)
+	buf := make([]byte, maxSelectorLen+len("\r\n"))
 
 	n, err := rdr.Read(buf)
 	if err != nil && !errors.Is(err, io.EOF) {
@@ -44,29 +47,46 @@ func readFullRequest(rdr io.Reader) (string, string, error) {
 	}
 	buf = buf[:n]
 
-	// Full-text search transactions are the exception, they
-	// may be longer because there is an additional search string
-	if n == 257 && buf[256] != '\n' {
-		intake := buf[n:cap(buf)]
-		total := n
+	if lineEnd := bytes.IndexByte(buf, '\n'); lineEnd >= 0 {
+		buf = buf[:lineEnd+1]
+	} else if tab := bytes.IndexByte(buf, '\t'); tab >= 0 {
+		// Full-text search transactions append TAB and a search string.
+		// RFC 1436 still caps the selector at 255 bytes, but gives no
+		// search-string limit, so keep a small implementation cap.
 		for {
-			intake = append(intake, 0)
-			intake = intake[:cap(intake)]
+			if len(buf) >= maxRequestLen {
+				return "", "", errors.New("gopher request too long")
+			}
 
+			intake := make([]byte, maxRequestLen-len(buf))
 			n, err = rdr.Read(intake)
 			if err != nil && err != io.EOF {
 				return "", "", err
 			}
-			total += n
 
-			if n < cap(intake) || intake[cap(intake)-1] == '\n' {
+			buf = append(buf, intake[:n]...)
+			if lineEnd := bytes.IndexByte(intake[:n], '\n'); lineEnd >= 0 {
+				buf = buf[:len(buf)-n+lineEnd+1]
+				break
+			}
+			if err == io.EOF {
 				break
 			}
-			intake = intake[n:]
 		}
-		buf = buf[:total]
+	} else if len(buf) > maxSelectorLen {
+		return "", "", errors.New("gopher selector too long")
 	}
 
 	selector, search, _ := bytes.Cut(buf, []byte{'\t'})
-	return strings.TrimRight(string(selector), "\r\n"), strings.TrimRight(string(search), "\r\n"), nil
+	selector = bytes.TrimRight(selector, "\r\n")
+	search = bytes.TrimRight(search, "\r\n")
+
+	if len(selector) > maxSelectorLen {
+		return "", "", errors.New("gopher selector too long")
+	}
+	if bytes.ContainsAny(selector, "\x00\r\n") || bytes.ContainsAny(search, "\x00\t\r\n") {
+		return "", "", errors.New("malformed gopher request")
+	}
+
+	return string(selector), string(search), nil
 }
diff --git a/gopher/request_test.go b/gopher/request_test.go
index 74030c6..3488f39 100644
--- a/gopher/request_test.go
+++ b/gopher/request_test.go
@@ -2,6 +2,7 @@ package gopher_test
 
 import (
 	"bytes"
+	"strings"
 	"testing"
 
 	"github.com/stretchr/testify/assert"
@@ -41,3 +42,28 @@ func TestParseRequest(t *testing.T) {
 		})
 	}
 }
+
+func TestParseRequestLimits(t *testing.T) {
+	t.Run("max selector", func(t *testing.T) {
+		request, err := gopher.ParseRequest(bytes.NewBufferString(strings.Repeat("a", 255) + "\r\n"))
+		require.Nil(t, err)
+		assert.Equal(t, "/"+strings.Repeat("a", 255), request.Path)
+	})
+
+	t.Run("selector too long", func(t *testing.T) {
+		_, err := gopher.ParseRequest(bytes.NewBufferString(strings.Repeat("a", 256) + "\r\n"))
+		require.NotNil(t, err)
+	})
+
+	t.Run("search may extend request line", func(t *testing.T) {
+		request, err := gopher.ParseRequest(bytes.NewBufferString(strings.Repeat("a", 255) + "\t" + strings.Repeat("b", 255) + "\r\n"))
+		require.Nil(t, err)
+		assert.Equal(t, "/"+strings.Repeat("a", 255), request.Path)
+		assert.Equal(t, strings.Repeat("b", 255), request.RawQuery)
+	})
+
+	t.Run("long malformed selector does not panic", func(t *testing.T) {
+		_, err := gopher.ParseRequest(bytes.NewBufferString(strings.Repeat("a", 1487) + "\r\n"))
+		require.NotNil(t, err)
+	})
+}
diff --git a/internal/server.go b/internal/server.go
index 46b95ef..3ae9be2 100644
--- a/internal/server.go
+++ b/internal/server.go
@@ -2,7 +2,9 @@ package internal
 
 import (
 	"context"
+	"fmt"
 	"net"
+	"runtime/debug"
 	"strings"
 	"sync"
 
@@ -84,6 +86,7 @@ func (s *Server) Serve() error {
 		go func() {
 			defer s.Wg.Done()
 			defer func() { _ = conn.Close() }()
+			defer s.recoverPanic("connection", "remote", conn.RemoteAddr().String())
 			s.HandleConn(conn)
 		}()
 	}
@@ -129,12 +132,25 @@ func (s *Server) propagateClose() {
 	s.Wg.Add(1)
 	go func() {
 		defer s.Wg.Done()
+		defer s.recoverPanic("close propagation")
 
 		<-s.Ctx.Done()
 		_ = s.Listener.Close()
 	}()
 }
 
+func (s *Server) recoverPanic(goroutine string, keyvals ...any) {
+	if r := recover(); r != nil {
+		keyvals = append([]any{
+			"msg", "panic in server goroutine",
+			"goroutine", goroutine,
+			"panic", fmt.Sprintf("%v", r),
+			"stack", string(debug.Stack()),
+		}, keyvals...)
+		_ = s.ErrorLog.Log(keyvals...)
+	}
+}
+
 // JoinDefaultPort appends ":<port>" iff the address does not already contain a port.
 func JoinDefaultPort(address string, port string) string {
 	if len(address) > 0 && address[0] == '[' {
diff --git a/internal/server_test.go b/internal/server_test.go
new file mode 100644
index 0000000..62d8681
--- /dev/null
+++ b/internal/server_test.go
@@ -0,0 +1,66 @@
+package internal_test
+
+import (
+	"context"
+	"fmt"
+	"net"
+	"strings"
+	"sync"
+	"testing"
+	"time"
+
+	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
+
+	"tildegit.org/tjp/sliderule/internal"
+)
+
+type captureLogger struct {
+	mu   sync.Mutex
+	logs []string
+}
+
+func (cl *captureLogger) Log(keyvals ...any) error {
+	cl.mu.Lock()
+	defer cl.mu.Unlock()
+
+	for _, val := range keyvals {
+		cl.logs = append(cl.logs, fmt.Sprint(val))
+	}
+	return nil
+}
+
+func (cl *captureLogger) String() string {
+	cl.mu.Lock()
+	defer cl.mu.Unlock()
+
+	return strings.Join(cl.logs, " ")
+}
+
+func TestServerRecoversConnectionPanic(t *testing.T) {
+	var logs captureLogger
+	server, err := internal.NewServer(
+		context.Background(),
+		"localhost",
+		"tcp",
+		"127.0.0.1:0",
+		&logs,
+		func(net.Conn) { panic("boom") },
+	)
+	require.Nil(t, err)
+
+	done := make(chan error, 1)
+	go func() { done <- server.Serve() }()
+
+	conn, err := net.Dial(server.Network(), server.Address())
+	require.Nil(t, err)
+	_ = conn.Close()
+
+	require.Eventually(t, func() bool {
+		return strings.Contains(logs.String(), "panic in server goroutine")
+	}, time.Second, 10*time.Millisecond)
+
+	server.Close()
+	assert.Nil(t, <-done)
+	assert.Contains(t, logs.String(), "boom")
+}