nano-init: bound the ingress handshake read - #445
HosniBelfeki wants to merge 1 commit into
Conversation
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.
There was a problem hiding this comment.
Code Review
This pull request improves the security of the ingress handshake in cmd/nano-init/ingress.go by replacing ReadString with ReadSlice to bound the maximum handshake size and prevent memory accumulation. It also refactors handshake refusal logic into a helper function and adds comprehensive unit tests in cmd/nano-init/ingress_test.go to verify handshake bounds, pipelined bytes relaying, and connection string parsing. There are no review comments, and I have no feedback to provide.
|
see https://github.com/aojea/agents.net/tree/main , I've been juggling between here and there, but I'm leaning towards try to get some wider consensus first and expose it as a library |
|
Makes sense — where it lives is your call, and I'm not trying to get ahead of the library question. One thing that might make it easier either way: every other place that reads an ingress head from the gateway bounds it, and nano-init is the one that doesn't.
Different wire formats — an HTTP CONNECT head in the first, the bare So it's fairly orthogonal to where the code ends up — if nano-init folds into the library, a bounded read should travel with it rather than be carried over as is. Happy to hold this until the library question settles, or to send the same to agents.net if that's the better home. |
What
cmd/nano-init/ingress.gonames a bound on the ingress handshake that it does not apply:bufio.NewReaderSizesizes one fill.ReadStringcallsReadBytes, which keeps appending full buffers and growing a slice of its own until it finds the delimiter, so the constant limits nothing. A client that opens the socket and never sends a newline is read untilingressConnectTimeoutexpires, and the process accumulating it is PID 1 in the sandbox.Standalone demonstration of the primitive:
Severity
Robustness, not a security fix, and I would rather say so plainly than oversell it. The ingress socket is
0600and reached by the gateway or by the agent already running inside the sandbox — and that agent can exhaust the guest's memory directly without this path. What is wrong here is that a stated limit is not a limit.Fix
Read with
ReadSlice, which stops at the buffer and reportsbufio.ErrBufferFull, and answer that refusal instead of dropping it — for the reason the malformed-handshake path already answers, now factored intorefuseIngressso both paths keep that property.This is not a new convention. The far side of the same
CONNECT <port>/OKhandshake already bounds its read:internal/sambox/ingress.go:216bufio.NewReader(io.LimitReader(conn, 128)).ReadString('\n')cmd/nano-init/ingress.go:98bufio.NewReaderSize(conn, ingressMaxHandshake)Tests
ingress_test.gopreviously held one test, on socket permissions; the handshake had none.TestHandleIngressBoundsTheHandshake— reads a 1 MiB flood with no newline throughhandleIngressand holds it toingressMaxHandshake. Against the current code it fails as the defect predicts:TestHandleIngressRelaysPipelinedBytes— covers what a bounded read must not break: bytes the gateway sends in the same write as the handshake are in the reader rather than the socket when the agent is dialled, so they are forwarded by hand. It passes both before and after, which is the point of adding it.TestParseIngressConnect— table test for the parser, which had no coverage.Verified on linux/amd64, Go 1.26,
go test -race ./...green for the wholecmd/nano-initmodule.