nano-init: bound the ingress handshake read

cmd/nano-init/ingress.go

@@ -17,6 +17,7 @@ package main
 import (
 	"bufio"
 	"context"
+	"errors"
 	"fmt"
 	"io"
 	"log"
@@ -95,18 +96,24 @@ func handleIngress(ctx context.Context, conn net.Conn) error {
 
 	_ = conn.SetReadDeadline(time.Now().Add(ingressConnectTimeout))
 	reader := bufio.NewReaderSize(conn, ingressMaxHandshake)
-	line, err := reader.ReadString('\n')
+	// ReadSlice rather than ReadString: ReadString grows a buffer of its own
+	// until it finds a newline, so the size above would bound nothing, and a
+	// client that never sends one could make this process -- PID 1 in the
+	// sandbox -- accumulate for as long as the deadline allows. ReadSlice
+	// stops at the buffer and says so. The far side of this handshake bounds
+	// its read the same way; see dialSandbox in internal/sambox/ingress.go.
+	line, err := reader.ReadSlice('\n')
 	if err != nil {
+		if errors.Is(err, bufio.ErrBufferFull) {
+			return refuseIngress(conn, fmt.Errorf("the handshake is longer than %d bytes", ingressMaxHandshake))
+		}
 		return fmt.Errorf("read the ingress handshake: %w", err)
 	}
 	_ = conn.SetReadDeadline(time.Time{})
 
-	port, err := parseIngressConnect(line)
+	port, err := parseIngressConnect(string(line))
 	if err != nil {
-		// Answered rather than dropped: a gateway that gets nothing back
-		// cannot tell a refusal from a sandbox that never started.
-		_, _ = io.WriteString(conn, "ERR "+err.Error()+"\n")
-		return err
+		return refuseIngress(conn, err)
 	}
 
 	// The agent is in this namespace, which is the whole reason this hop
@@ -138,6 +145,15 @@ func handleIngress(ctx context.Context, conn net.Conn) error {
 	return nil
 }
 
+// refuseIngress tells the gateway why its handshake was not honoured.
+//
+// Answered rather than dropped: a gateway that gets nothing back cannot tell
+// a refusal from a sandbox that never started.
+func refuseIngress(conn net.Conn, err error) error {
+	_, _ = io.WriteString(conn, "ERR "+err.Error()+"\n")
+	return err
+}
+
 // parseIngressConnect reads the one line the gateway sends first.
 func parseIngressConnect(line string) (int, error) {
 	fields := strings.Fields(strings.TrimSpace(line))

cmd/nano-init/ingress_test.go

@@ -15,9 +15,14 @@
 package main
 
 import (
+	"bufio"
+	"bytes"
 	"context"
+	"io"
+	"net"
 	"os"
 	"path/filepath"
+	"strings"
 	"syscall"
 	"testing"
 	"time"
@@ -69,3 +74,164 @@ func TestIngressSocketIsOwnerOnly(t *testing.T) {
 		t.Error("serveIngress did not return after cancellation")
 	}
 }
+
+// floodConn is a client that opens the ingress socket and then never sends the
+// newline the handshake ends with. It counts what the handler reads, so a test
+// can hold the handler to the bound the package documents.
+type floodConn struct {
+	read  int
+	limit int
+	reply bytes.Buffer
+}
+
+func (c *floodConn) Read(p []byte) (int, error) {
+	if c.read >= c.limit {
+		return 0, io.EOF
+	}
+	n := len(p)
+	if remaining := c.limit - c.read; n > remaining {
+		n = remaining
+	}
+	for i := range p[:n] {
+		p[i] = 'A'
+	}
+	c.read += n
+	return n, nil
+}
+
+func (c *floodConn) Write(p []byte) (int, error)      { return c.reply.Write(p) }
+func (c *floodConn) Close() error                     { return nil }
+func (c *floodConn) LocalAddr() net.Addr              { return floodAddr{} }
+func (c *floodConn) RemoteAddr() net.Addr             { return floodAddr{} }
+func (c *floodConn) SetDeadline(time.Time) error      { return nil }
+func (c *floodConn) SetReadDeadline(time.Time) error  { return nil }
+func (c *floodConn) SetWriteDeadline(time.Time) error { return nil }
+
+type floodAddr struct{}
+
+func (floodAddr) Network() string { return "flood" }
+func (floodAddr) String() string  { return "flood" }
+
+// TestHandleIngressBoundsTheHandshake holds the handshake read to the size the
+// package names. ingressMaxHandshake sizes a bufio.Reader, and that bounds only
+// what one fill holds: ReadString goes on growing a buffer of its own until it
+// finds a newline, so a client that sends none could make this process -- PID 1
+// in the sandbox -- accumulate for as long as the read deadline allows.
+func TestHandleIngressBoundsTheHandshake(t *testing.T) {
+	const flood = 1 << 20
+	conn := &floodConn{limit: flood}
+
+	err := handleIngress(context.Background(), conn)
+	if err == nil {
+		t.Fatal("handleIngress accepted a handshake with no newline, want an error")
+	}
+	if got := conn.read; got > ingressMaxHandshake {
+		t.Errorf("handleIngress read %d bytes of a %d byte flood, want at most %d", got, flood, ingressMaxHandshake)
+	}
+	if answer := conn.reply.String(); !strings.HasPrefix(answer, "ERR ") {
+		t.Errorf("handleIngress answered %q, want an ERR line: a gateway that gets nothing back cannot tell a refusal from a sandbox that never started", answer)
+	}
+}
+
+// TestHandleIngressRelaysPipelinedBytes covers what a bounded read must not
+// break. The gateway may send its first request bytes in the same write as the
+// handshake, and those are in the reader rather than the socket by the time the
+// agent is dialled, so they are forwarded by hand.
+func TestHandleIngressRelaysPipelinedBytes(t *testing.T) {
+	const pipelined = "HELLO"
+
+	agent, err := net.Listen("tcp", "127.0.0.1:0")
+	if err != nil {
+		t.Fatalf("listen as the agent: %v", err)
+	}
+	defer func() { _ = agent.Close() }()
+	_, port, err := net.SplitHostPort(agent.Addr().String())
+	if err != nil {
+		t.Fatalf("split the agent address: %v", err)
+	}
+
+	delivered := make(chan string, 1)
+	go func() {
+		c, err := agent.Accept()
+		if err != nil {
+			delivered <- "accept: " + err.Error()
+			return
+		}
+		defer func() { _ = c.Close() }()
+		buf := make([]byte, len(pipelined))
+		if _, err := io.ReadFull(c, buf); err != nil {
+			delivered <- "read: " + err.Error()
+			return
+		}
+		delivered <- string(buf)
+	}()
+
+	client, server := net.Pipe()
+	defer func() { _ = client.Close() }()
+	done := make(chan error, 1)
+	go func() { done <- handleIngress(context.Background(), server) }()
+	go func() { _, _ = io.WriteString(client, "CONNECT "+port+"\n"+pipelined) }()
+
+	reply, err := bufio.NewReader(io.LimitReader(client, 128)).ReadString('\n')
+	if err != nil {
+		t.Fatalf("read the handshake answer: %v", err)
+	}
+	if got := strings.TrimSpace(reply); got != "OK" {
+		t.Fatalf("the handshake answer is %q, want OK", got)
+	}
+
+	select {
+	case got := <-delivered:
+		if got != pipelined {
+			t.Errorf("the agent received %q, want %q", got, pipelined)
+		}
+	case <-time.After(5 * time.Second):
+		t.Fatal("the agent never received the bytes pipelined behind the handshake")
+	}
+
+	select {
+	case <-done:
+	case <-time.After(5 * time.Second):
+		t.Error("handleIngress did not return once both ends were done")
+	}
+}
+
+// TestParseIngressConnect pins the one line the gateway sends first. Everything
+// past it is relayed verbatim, so this is where a malformed request has to stop.
+func TestParseIngressConnect(t *testing.T) {
+	for _, tc := range []struct {
+		name string
+		line string
+		want int
+	}{
+		{name: "port", line: "CONNECT 8080\n", want: 8080},
+		{name: "lowercase verb", line: "connect 8080\n", want: 8080},
+		{name: "extra spaces", line: "  CONNECT   8080  \n", want: 8080},
+		{name: "lowest port", line: "CONNECT 1\n", want: 1},
+		{name: "highest port", line: "CONNECT 65535\n", want: 65535},
+		{name: "port zero", line: "CONNECT 0\n"},
+		{name: "above the port range", line: "CONNECT 65536\n"},
+		{name: "negative", line: "CONNECT -1\n"},
+		{name: "not a number", line: "CONNECT http\n"},
+		{name: "wrong verb", line: "GET 8080\n"},
+		{name: "no port", line: "CONNECT\n"},
+		{name: "trailing junk", line: "CONNECT 8080 now\n"},
+		{name: "empty", line: "\n"},
+	} {
+		t.Run(tc.name, func(t *testing.T) {
+			got, err := parseIngressConnect(tc.line)
+			if tc.want == 0 {
+				if err == nil {
+					t.Fatalf("parseIngressConnect(%q) = %d, want an error", tc.line, got)
+				}
+				return
+			}
+			if err != nil {
+				t.Fatalf("parseIngressConnect(%q) returned %v, want %d", tc.line, err, tc.want)
+			}
+			if got != tc.want {
+				t.Errorf("parseIngressConnect(%q) = %d, want %d", tc.line, got, tc.want)
+			}
+		})
+	}
+}