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