From 35e8df450cfb3edb0d6dc84957f9db54463a8fac Mon Sep 17 00:00:00 2001 From: HosniBelfeki Date: Sat, 19 Sep 2026 13:19:53 +0100 Subject: [PATCH] nano-init: bound the ingress handshake read ingressMaxHandshake sizes the bufio.Reader that reads "CONNECT ", 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. --- cmd/nano-init/ingress.go | 28 ++++-- cmd/nano-init/ingress_test.go | 166 ++++++++++++++++++++++++++++++++++ 2 files changed, 188 insertions(+), 6 deletions(-) diff --git a/cmd/nano-init/ingress.go b/cmd/nano-init/ingress.go index d7595544..6de57352 100644 --- a/cmd/nano-init/ingress.go +++ b/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)) diff --git a/cmd/nano-init/ingress_test.go b/cmd/nano-init/ingress_test.go index b3bcef64..ccb73476 100644 --- a/cmd/nano-init/ingress_test.go +++ b/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) + } + }) + } +}