nano-init: bound the ingress handshake read

ingressMaxHandshake sizes the bufio.Reader that reads "CONNECT <port>",
and the comment beside it says anything longer is a client that has
misunderstood. It bounds nothing. A bufio.Reader's size limits one fill,
while ReadString goes on growing a buffer of its own until it finds a
newline, so a client that opens the ingress socket and never sends one
is read until the deadline expires -- and the process accumulating it is
PID 1 in the sandbox.

Read with ReadSlice, which stops at the buffer and reports
ErrBufferFull, and answer that refusal rather than dropping it, for the
reason the malformed-handshake path already answers: a gateway that gets
nothing back cannot tell a refusal from a sandbox that never started.
The far side of this same handshake, dialSandbox in
internal/sambox/ingress.go, already bounds its read with io.LimitReader.

The added test reads a 1 MiB flood with no newline through handleIngress
and holds it to ingressMaxHandshake; against the current code it reads
all 1048576 bytes and answers nothing. The pipelined-bytes test covers
what a bounded read must not break, since bytes the gateway sends in the
same write as the handshake sit in the reader rather than the socket
when the agent is dialled and are forwarded by hand. parseIngressConnect
had no test of its own.

Author: HosniBelfeki

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)
+			}
+		})
+	}
+}