From d167686cf1d5eb58004b49e4ccfe5d0e4584c910 Mon Sep 17 00:00:00 2001 From: Ilker Manap Date: Wed, 30 Sep 2026 00:29:52 +0200 Subject: [PATCH] Stop a browser page, or a model definition, from running code on the nodes MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The admin listener has no login. Reaching it is meant to mean holding the control plane, so it binds to loopback and an operator reaches it over an ssh tunnel — and so does every page open in that operator's browser. The API decoded JSON whatever the Content-Type said, so a page could POST a text/plain model definition, no preflight needed. A model's profile env reached an engine the agent runs as root, unfiltered: LD_PRELOAD in a definition was code execution on every node. A page that rebound its own name to 127.0.0.1 could also read pprof's heap, which holds API keys. The admin listener now answers only when Host is an address or localhost, refuses requests a browser marks as cross-site (Origin, Sec-Fetch-Site), and takes changes only as application/json, which no page can send across origins without a preflight nobody grants. ctl sends that on every change, bodiless DELETEs included; curl and ctl are otherwise unaffected. Profile env that changes what runs rather than how — the dynamic loader's LD_*, PATH, Python's PYTHON* start-up variables, BASH_ENV and their like — is refused when a model is defined, and dropped again by the engine adapter before the process starts, whatever sent the profile. None of the live catalog's models sets any. Co-Authored-By: Claude Opus 5.5 (1M context) --- internal/agent/engines/engine.go | 5 ++ internal/agent/engines/unsafeenv.go | 30 ++++++++++ internal/cli/ctl.go | 4 +- internal/cli/server.go | 2 +- internal/control/catalog/catalog.go | 7 +++ internal/control/catalog/catalog_test.go | 19 ++++++ internal/router/adminguard.go | 75 ++++++++++++++++++++++++ internal/router/adminguard_test.go | 57 ++++++++++++++++++ 8 files changed, 197 insertions(+), 2 deletions(-) create mode 100644 internal/agent/engines/unsafeenv.go create mode 100644 internal/router/adminguard.go create mode 100644 internal/router/adminguard_test.go diff --git a/internal/agent/engines/engine.go b/internal/agent/engines/engine.go index 0e582c46..6bc5f712 100644 --- a/internal/agent/engines/engine.go +++ b/internal/agent/engines/engine.go @@ -249,6 +249,11 @@ func baseEnv(s ShardSpec) map[string]string { // Profile overrides win: they are the escape hatch for the per-cluster // tuning no adapter can guess. for k, v := range s.Profile.Env { + // Checked again here, not only where the model was defined: this is + // the process about to run as root, whatever sent the profile. + if bad, _ := UnsafeEnv(k); bad { + continue + } env[k] = v } return env diff --git a/internal/agent/engines/unsafeenv.go b/internal/agent/engines/unsafeenv.go new file mode 100644 index 00000000..f60bc53e --- /dev/null +++ b/internal/agent/engines/unsafeenv.go @@ -0,0 +1,30 @@ +package engines + +import "strings" + +// UnsafeEnv reports whether a profile may not set an environment variable, +// and why. +// +// A profile's env is the escape hatch for tuning no adapter can guess — +// CUDA_*, NCCL_*, VLLM_*, HF_* — and it reaches an engine the agent starts as +// root. Some variables are not tuning but a way to run other code in that +// process: the dynamic loader's LD_PRELOAD, Python's PYTHONSTARTUP, a shell's +// BASH_ENV. Set from a model definition, which an admin API or a config file +// supplies, they would turn "add a model" into "run this as root on every +// node". They are refused where a model is defined and again where an engine +// starts. +func UnsafeEnv(key string) (bool, string) { + k := strings.ToUpper(key) + switch { + case strings.HasPrefix(k, "LD_"), strings.HasPrefix(k, "DYLD_"): + return true, "it changes what the dynamic loader loads" + case k == "PATH": + return true, "it changes which binaries run" + case strings.HasPrefix(k, "PYTHON") && k != "PYTHONUNBUFFERED" && k != "PYTHONHASHSEED": + return true, "it changes what Python imports or runs at start" + case k == "BASH_ENV" || k == "ENV" || k == "PERL5OPT" || k == "PERL5LIB" || + k == "NODE_OPTIONS" || k == "RUBYOPT" || k == "GCONV_PATH" || k == "OPENSSL_CONF": + return true, "it makes an interpreter or library load code" + } + return false, "" +} diff --git a/internal/cli/ctl.go b/internal/cli/ctl.go index 4ebecea6..158056cf 100644 --- a/internal/cli/ctl.go +++ b/internal/cli/ctl.go @@ -67,7 +67,9 @@ func (c *ctlClient) do(ctx context.Context, method, path string, body, out any) if err != nil { return err } - if body != nil { + // On every change, body or not: the admin listener refuses anything else, + // because a browser can send a bodiless POST across origins unasked. + if method != http.MethodGet { req.Header.Set("Content-Type", "application/json") } // Who made the change, kept with a stored model so a catalog that grew diff --git a/internal/cli/server.go b/internal/cli/server.go index 82a0f3a8..927fb062 100644 --- a/internal/cli/server.go +++ b/internal/cli/server.go @@ -246,7 +246,7 @@ func newServerCmd() *cobra.Command { // enforces its own per-phase deadlines. } adminSrv := &http.Server{ - Handler: adminMux, + Handler: router.AdminGuard(adminMux), ReadHeaderTimeout: 10 * time.Second, } diff --git a/internal/control/catalog/catalog.go b/internal/control/catalog/catalog.go index 15afa763..b64277c8 100644 --- a/internal/control/catalog/catalog.go +++ b/internal/control/catalog/catalog.go @@ -302,6 +302,13 @@ func (m Model) validate() error { } seen[p.Name] = struct{}{} + for k := range p.Env { + if bad, why := engines.UnsafeEnv(k); bad { + return fmt.Errorf("model %q profile %q: env %s is not allowed: %s", + m.Name, p.Name, k, why) + } + } + if p.GPUClass == "" && !cpuCapable(p.Engine) { return fmt.Errorf("model %q profile %q: gpu_class is required for engine %q", m.Name, p.Name, p.Engine) diff --git a/internal/control/catalog/catalog_test.go b/internal/control/catalog/catalog_test.go index eafec55d..619a1c6d 100644 --- a/internal/control/catalog/catalog_test.go +++ b/internal/control/catalog/catalog_test.go @@ -302,3 +302,22 @@ func TestSimulatedNeedsEveryProfileToBeSimulated(t *testing.T) { }) } } + +func TestAProfileCannotSetWhatRunsCodeInTheEngine(t *testing.T) { + // A profile's env reaches an engine started as root. LD_PRELOAD from a + // model definition would make "add a model" into "run this everywhere". + for _, k := range []string{"LD_PRELOAD", "ld_library_path", "PYTHONSTARTUP", "PATH", "BASH_ENV"} { + m := Model{Name: "m", Source: "http://x/m.gguf", Profiles: []Profile{{ + Name: "cpu", Engine: "llamacpp", Env: map[string]string{k: "/tmp/x"}, + }}} + if _, err := New([]Model{m}); err == nil || !strings.Contains(err.Error(), "not allowed") { + t.Errorf("%s: %v", k, err) + } + } + ok := Model{Name: "m", Source: "http://x/m.gguf", Profiles: []Profile{{ + Name: "cpu", Engine: "llamacpp", Env: map[string]string{"OMP_NUM_THREADS": "4", "PYTHONUNBUFFERED": "1"}, + }}} + if _, err := New([]Model{ok}); err != nil { + t.Errorf("ordinary tuning refused: %v", err) + } +} diff --git a/internal/router/adminguard.go b/internal/router/adminguard.go new file mode 100644 index 00000000..b0749554 --- /dev/null +++ b/internal/router/adminguard.go @@ -0,0 +1,75 @@ +package router + +import ( + "net" + "net/http" + "net/netip" + "net/url" + "strings" +) + +// AdminGuard keeps a browser from being used against the admin listener. +// +// The listener has no login: reaching it is meant to mean holding the machine, +// so it binds to loopback and an operator reaches it over an ssh tunnel. But a +// browser on that operator's machine reaches it too, from any page it has +// open. A page could POST a model whose profile runs as root on every node, +// or — by pointing its own domain at 127.0.0.1 — read pprof's heap, which +// holds API keys in the clear. Three checks close that without touching what +// ctl and curl do: +// +// - Host must be an address or localhost. A page that rebinds its own name +// to 127.0.0.1 still sends that name. +// - A request carrying Origin or Sec-Fetch-Site from another site is +// refused. Browsers send them; ctl and curl do not. +// - Anything but GET and HEAD must be application/json. A page can send a +// form or text/plain anywhere without asking; JSON across origins needs a +// preflight this listener never grants. +func AdminGuard(next http.Handler) http.Handler { + return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + if !addressHost(r.Host) { + http.Error(w, "the admin listener answers to an address or localhost, not a name", http.StatusMisdirectedRequest) + return + } + if crossSite(r) { + http.Error(w, "cross-site request refused", http.StatusForbidden) + return + } + if r.Method != http.MethodGet && r.Method != http.MethodHead { + ct := r.Header.Get("Content-Type") + if mt, _, _ := strings.Cut(ct, ";"); strings.TrimSpace(strings.ToLower(mt)) != "application/json" { + http.Error(w, "a change to the admin API must be sent as application/json", http.StatusUnsupportedMediaType) + return + } + } + next.ServeHTTP(w, r) + }) +} + +func addressHost(hostport string) bool { + host := hostport + if h, _, err := net.SplitHostPort(hostport); err == nil { + host = h + } + host = strings.Trim(host, "[]") + if strings.EqualFold(host, "localhost") { + return true + } + _, err := netip.ParseAddr(host) + return err == nil +} + +func crossSite(r *http.Request) bool { + switch r.Header.Get("Sec-Fetch-Site") { + case "", "same-origin", "none": + default: + return true + } + if o := r.Header.Get("Origin"); o != "" { + u, err := url.Parse(o) + if err != nil || !strings.EqualFold(u.Host, r.Host) { + return true + } + } + return false +} diff --git a/internal/router/adminguard_test.go b/internal/router/adminguard_test.go new file mode 100644 index 00000000..8727435d --- /dev/null +++ b/internal/router/adminguard_test.go @@ -0,0 +1,57 @@ +package router + +import ( + "net/http" + "net/http/httptest" + "strings" + "testing" +) + +func TestTheAdminListenerRefusesWhatABrowserPageCouldSend(t *testing.T) { + h := AdminGuard(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) {})) + do := func(method, host, ct string, hdr map[string]string) int { + r := httptest.NewRequest(method, "/api/v1/models", strings.NewReader("{}")) + r.Host = host + if ct != "" { + r.Header.Set("Content-Type", ct) + } + for k, v := range hdr { + r.Header.Set(k, v) + } + w := httptest.NewRecorder() + h.ServeHTTP(w, r) + return w.Code + } + + // What ctl and curl send. + if got := do("POST", "127.0.0.1:9090", "application/json", nil); got != 200 { + t.Errorf("ctl's request was refused: %d", got) + } + if got := do("GET", "localhost:9090", "", nil); got != 200 { + t.Errorf("a GET through an ssh tunnel was refused: %d", got) + } + if got := do("DELETE", "[::1]:9090", "application/json", nil); got != 200 { + t.Errorf("IPv6 loopback refused: %d", got) + } + + // A page posting a form, or text/plain, needs no preflight. + if got := do("POST", "127.0.0.1:9090", "text/plain", nil); got != http.StatusUnsupportedMediaType { + t.Errorf("text/plain POST accepted: %d", got) + } + if got := do("POST", "127.0.0.1:9090", "", nil); got != http.StatusUnsupportedMediaType { + t.Errorf("a bodiless POST accepted: %d", got) + } + // DNS rebinding: the page's own name, now resolving to 127.0.0.1. + if got := do("GET", "evil.example:9090", "", nil); got != http.StatusMisdirectedRequest { + t.Errorf("a rebound name was answered: %d", got) + } + // And anything a browser marks as coming from elsewhere. + if got := do("POST", "127.0.0.1:9090", "application/json", + map[string]string{"Sec-Fetch-Site": "cross-site"}); got != http.StatusForbidden { + t.Errorf("cross-site fetch accepted: %d", got) + } + if got := do("POST", "127.0.0.1:9090", "application/json", + map[string]string{"Origin": "https://evil.example"}); got != http.StatusForbidden { + t.Errorf("foreign Origin accepted: %d", got) + } +}