diff --git a/main.go b/main.go index ae68795..7cbfda9 100644 --- a/main.go +++ b/main.go @@ -174,11 +174,19 @@ func startDebugServer(cfg config.DebugConfig) (*http.Server, error) { return nil, fmt.Errorf("mutex_profile_fraction must not be negative") } + if !cfg.AllowPublic { + if err := checkLoopbackBind(cfg.Bind); err != nil { + return nil, err + } + } + listener, err := net.Listen("tcp", cfg.Bind) if err != nil { return nil, fmt.Errorf("listen on %q: %w", cfg.Bind, err) } if !cfg.AllowPublic { + // Backstop: a hostname can resolve differently between + // checkLoopbackBind and the bind itself. addr, ok := listener.Addr().(*net.TCPAddr) if !ok || !addr.IP.IsLoopback() { listener.Close() @@ -198,6 +206,36 @@ func startDebugServer(cfg config.DebugConfig) (*http.Server, error) { return srv, nil } +// checkLoopbackBind rejects a non-loopback debug bind before any socket is +// opened. +// +// Checking only after net.Listen meant "0.0.0.0:6060" was really bound, and +// accepting into the backlog, for the moment before the check closed it. Not +// an exposure, since Serve never started, but a public port should not be +// claimed at all, and ":6060" deserves a clearer error than "resolves to a +// non-loopback address". A hostname must resolve to loopback addresses only. +func checkLoopbackBind(bind string) error { + host, _, err := net.SplitHostPort(bind) + if err != nil { + return fmt.Errorf("debug.bind %q: %w", bind, err) + } + if host == "" { + return fmt.Errorf("bind %q listens on every interface; use 127.0.0.1 or set debug.allow_public = true to acknowledge public pprof exposure", bind) + } + var ips []net.IP + if ip := net.ParseIP(host); ip != nil { + ips = []net.IP{ip} + } else if ips, err = net.LookupIP(host); err != nil { + return fmt.Errorf("debug.bind %q: %w", bind, err) + } + for _, ip := range ips { + if !ip.IsLoopback() { + return fmt.Errorf("bind %q resolves to a non-loopback address; set debug.allow_public = true to acknowledge public pprof exposure", bind) + } + } + return nil +} + // serveDebug runs the pprof listener until it stops. // // A failure here must not end the process. pprof is opt-in and optional; the diff --git a/main_test.go b/main_test.go index 716c354..891016d 100644 --- a/main_test.go +++ b/main_test.go @@ -248,6 +248,63 @@ func TestDebugServerRejectsPublicBindWithoutAcknowledgement(t *testing.T) { } } +// The loopback check runs before the port is claimed (#64 item 5). +// +// The public port is taken first by the test itself. A server that binds +// before checking fails with "address already in use"; one that checks first +// never touches the socket and reports the allow_public guidance instead. +func TestDebugServerRejectsPublicBindBeforeListening(t *testing.T) { + held, err := net.Listen("tcp", "0.0.0.0:0") + if err != nil { + t.Fatalf("hold a public port: %v", err) + } + defer held.Close() + + _, err = startDebugServer(config.DebugConfig{Bind: held.Addr().String()}) + if err == nil { + t.Fatal("public debug bind was accepted without allow_public") + } + if !strings.Contains(err.Error(), "debug.allow_public = true") { + t.Fatalf("error = %q; want the allow_public guidance, not a listen error", err) + } +} + +func TestDebugServerNamesAnAllInterfacesBind(t *testing.T) { + _, err := startDebugServer(config.DebugConfig{Bind: ":0"}) + if err == nil { + t.Fatal(`":0" was accepted without allow_public`) + } + if !strings.Contains(err.Error(), "every interface") { + t.Fatalf("error = %q, want it to say the bind covers every interface", err) + } +} + +func TestDebugServerRejectsAMalformedBind(t *testing.T) { + _, err := startDebugServer(config.DebugConfig{Bind: "6060"}) + if err == nil || !strings.Contains(err.Error(), "debug.bind") { + t.Fatalf("error = %v, want a debug.bind parse error", err) + } +} + +func TestDebugServerAcceptsLoopbackBinds(t *testing.T) { + for _, bind := range []string{"127.0.0.1:0", "[::1]:0", "localhost:0"} { + t.Run(bind, func(t *testing.T) { + if bind == "[::1]:0" { + if l, err := net.Listen("tcp", bind); err != nil { + t.Skipf("no IPv6 loopback here: %v", err) + } else { + l.Close() + } + } + srv, err := startDebugServer(config.DebugConfig{Bind: bind}) + if err != nil { + t.Fatalf("startDebugServer(%q): %v", bind, err) + } + srv.Close() + }) + } +} + // failingListener returns a permanent Accept error, the shape of a listener // that has died under a running server. net/http retries temporary errors, so // a plain error is what actually terminates Serve.