diff --git a/agent/internal/files/files.go b/agent/internal/files/files.go index 0d7546787..e82210c8c 100644 --- a/agent/internal/files/files.go +++ b/agent/internal/files/files.go @@ -1,6 +1,9 @@ -// Package files serves the file-browser HTTP API. All paths are -// resolved relative to a fixed root (e.g. /data) and path-traversal -// attempts (.., symlinks escaping the root) are rejected. +// Package files serves the file-browser HTTP API. All paths are resolved +// relative to a fixed root (e.g. /data). The requested path is validated +// lexically, then every operation runs relative to a directory descriptor +// opened on that root, one component at a time with symlinks refused (see +// rooted_linux.go), so a filesystem change made after validation cannot +// redirect an operation outside the root. package files import ( @@ -21,10 +24,10 @@ import ( ) // errPathOutOfRoot is the only "bad path" error safe to echo back to the -// client. Anything else (EvalSymlinks filesystem errors, etc.) might leak -// absolute paths or volume internals — those get logged and 400'd with -// a generic message. Handlers route through badRequest() so the -// classification is enforced in one place. +// client. Anything else (filesystem errors, etc.) might leak absolute paths or +// volume internals — those get logged and 400'd with a generic message. +// Handlers route through fail()/badRequest() so the classification is +// enforced in one place. var errPathOutOfRoot = errors.New("path escapes root") // errDotfile rejects any path with a dot-prefixed component. Dotfiles in the @@ -38,24 +41,16 @@ func hasDotComponent(rel string) bool { return strings.HasPrefix(rel, ".") || strings.Contains(rel, "/.") } -// resolvedHasDotComponent reports whether the symlink-resolved absolute path -// resolved (already confirmed to be under h.root) has a dot-prefixed -// component below the root. hasDotComponent only sees the requested path, -// so a plain-named symlink pointing at a dotfile or into a dot-directory -// would otherwise pass. -func (h *handler) resolvedHasDotComponent(resolved string) bool { - rel, err := filepath.Rel(h.root, resolved) - if err != nil { - return true - } - if rel == "." { - return false - } - return hasDotComponent(filepath.ToSlash(rel)) -} - type handler struct { root string + + // beforeOpen is a test seam. When non-nil it is called synchronously + // immediately before a path component is opened ("open"), before the + // destination of a write is inspected ("stat"), before the final rename of + // an atomic write ("commit") and before a non-recursive delete ("remove"). + // dir is the slash-joined parent path below the root ("" for the root). + // Production code never sets it. + beforeOpen func(stage, dir, name string) } // Mount registers the file-browser HTTP handlers on the supplied router. @@ -86,6 +81,11 @@ type Entry struct { ModTime string `json:"modTime"` } +// resolve validates the client-supplied path lexically (dot components, +// escape from the root) and returns it joined below the root. It does not +// touch the filesystem: symlink and dotfile-through-symlink checks happen +// while the descriptor walk performs the operation, so there is no +// validated-then-reopened window to race. func (h *handler) resolve(rel string) (string, error) { rel = strings.TrimPrefix(rel, "/") if rel == "" { @@ -98,92 +98,24 @@ func (h *handler) resolve(rel string) (string, error) { if !strings.HasPrefix(abs, h.root+string(os.PathSeparator)) && abs != h.root { return "", errPathOutOfRoot } - // Evaluate symlinks to block the "/data/escape -> /etc/passwd" - // attack: a prefix check on the raw path passes, but the real read - // hits the linked target. If the path doesn't exist yet (a new - // file about to be written), EvalSymlinks returns an error — fall - // back to checking the deepest existing ancestor. - if resolved, err := filepath.EvalSymlinks(abs); err == nil { - if !strings.HasPrefix(resolved, h.root+string(os.PathSeparator)) && resolved != h.root { - return "", errPathOutOfRoot - } - if h.resolvedHasDotComponent(resolved) { - return "", errDotfile - } - return resolved, nil - } else if !os.IsNotExist(err) { - return "", err - } - // The target doesn't exist yet (a new file/dir about to be written). - // Walk up to the deepest ancestor that *does* exist and verify it - // resolves inside the root, so the symlink-escape check still holds - // while allowing nested creation (mkdir -p, or uploading into a new - // subtree). Checking only the immediate parent rejected "/one/two/file" - // whenever "/one" was absent, making the later MkdirAll unreachable. The - // loop terminates at worst at the filesystem root, which always exists. - for parent := filepath.Dir(abs); ; parent = filepath.Dir(parent) { - resolvedParent, err := filepath.EvalSymlinks(parent) - if err == nil { - if !strings.HasPrefix(resolvedParent, h.root+string(os.PathSeparator)) && resolvedParent != h.root { - return "", errPathOutOfRoot - } - if h.resolvedHasDotComponent(resolvedParent) { - return "", errDotfile - } - return abs, nil - } - if !os.IsNotExist(err) { - return "", err - } - } + return abs, nil } -// resolveForDelete validates that path's parent directory stays inside -// root, then returns the literal path to the named entry — the entry -// itself, not whatever it points to. Unlike resolve, it never evaluates -// symlinks on the final path component: a symlink must be deleted as the -// link it is, not dereferenced to its target (F-102/F-108 audit finding -// F-108). os.Remove/os.RemoveAll already default to that behaviour on -// their own — the bug was handing them resolve()'s fully-EvalSymlinks'd -// path instead of the raw one. -func (h *handler) resolveForDelete(rel string) (string, error) { - rel = strings.TrimPrefix(rel, "/") - if rel == "" { - return h.root, nil - } - if hasDotComponent(rel) { - return "", errDotfile - } - abs := filepath.Join(h.root, filepath.Clean("/"+rel)) - if !strings.HasPrefix(abs, h.root+string(os.PathSeparator)) && abs != h.root { - return "", errPathOutOfRoot - } - parent := filepath.Dir(abs) - resolvedParent, err := filepath.EvalSymlinks(parent) - if err != nil { - return "", err - } - if !strings.HasPrefix(resolvedParent, h.root+string(os.PathSeparator)) && resolvedParent != h.root { - return "", errPathOutOfRoot - } - if h.resolvedHasDotComponent(resolvedParent) { - return "", errDotfile - } - fi, err := os.Stat(resolvedParent) +// components validates rel and returns its path components below the root +// (nil for the root itself). +func (h *handler) components(rel string) ([]string, error) { + p, err := h.resolve(rel) if err != nil { - return "", err - } - if !fi.IsDir() { - return "", fmt.Errorf("%s: not a directory", parent) + return nil, err } - return filepath.Join(resolvedParent, filepath.Base(abs)), nil + return relComponents(h.root, p) } // badRequest writes a 400 with a client-safe message. errPathOutOfRoot and -// errDotfile are the classes we echo verbatim — everything else -// (EvalSymlinks errors, multipart parse details, etc.) is logged and -// replaced with a generic "bad request" so filesystem/implementation -// details stay inside the pod. +// errDotfile are the classes we echo verbatim — everything else (filesystem +// errors, multipart parse details, etc.) is logged and replaced with a +// generic "bad request" so filesystem/implementation details stay inside the +// pod. func (h *handler) badRequest(w http.ResponseWriter, err error) { if errors.Is(err, errPathOutOfRoot) || errors.Is(err, errDotfile) { http.Error(w, err.Error(), http.StatusBadRequest) @@ -193,47 +125,44 @@ func (h *handler) badRequest(w http.ResponseWriter, err error) { http.Error(w, "bad request", http.StatusBadRequest) } -func (h *handler) list(w http.ResponseWriter, req *http.Request) { - p, err := h.resolve(req.URL.Query().Get("path")) - if err != nil { +// fail maps an error from path validation or a descriptor walk to a response: +// bad-path classes are 400s via badRequest, everything else goes through +// httpErr (404/403/500). +func (h *handler) fail(w http.ResponseWriter, err error) { + if errors.Is(err, errPathOutOfRoot) || errors.Is(err, errDotfile) || errors.Is(err, errBadPath) { h.badRequest(w, err) return } - ents, err := os.ReadDir(p) + httpErr(w, err) +} + +func (h *handler) list(w http.ResponseWriter, req *http.Request) { + comps, err := h.components(req.URL.Query().Get("path")) if err != nil { - httpErr(w, err) + h.fail(w, err) return } - out := make([]Entry, 0, len(ents)) - for _, e := range ents { - // Skip dot-prefixed entries (dotfiles and dot-directories). - if strings.HasPrefix(e.Name(), ".") { - continue - } - fi, err := e.Info() - if err != nil { - continue - } - rel, _ := filepath.Rel(h.root, filepath.Join(p, e.Name())) - out = append(out, Entry{ - Name: e.Name(), - Path: "/" + filepath.ToSlash(rel), - Size: fi.Size(), - Mode: fi.Mode().String(), - Dir: fi.IsDir(), - ModTime: fi.ModTime().UTC().Format(time.RFC3339), - }) + out, err := h.listDir(comps) + if err != nil { + h.fail(w, err) + return } httpjson.Write(w, http.StatusOK, out) } func (h *handler) read(w http.ResponseWriter, req *http.Request) { - p, err := h.resolve(req.URL.Query().Get("path")) + comps, err := h.components(req.URL.Query().Get("path")) if err != nil { - h.badRequest(w, err) + h.fail(w, err) return } - fi, err := os.Stat(p) + f, err := h.openFile(comps) + if err != nil { + h.fail(w, err) + return + } + defer func() { _ = f.Close() }() + fi, err := f.Stat() if err != nil { httpErr(w, err) return @@ -247,16 +176,22 @@ func (h *handler) read(w http.ResponseWriter, req *http.Request) { http.Error(w, "file too large; use /files/download", http.StatusRequestEntityTooLarge) return } - http.ServeFile(w, req, p) + http.ServeContent(w, req, fi.Name(), fi.ModTime(), f) } func (h *handler) download(w http.ResponseWriter, req *http.Request) { - p, err := h.resolve(req.URL.Query().Get("path")) + comps, err := h.components(req.URL.Query().Get("path")) if err != nil { - h.badRequest(w, err) + h.fail(w, err) + return + } + f, err := h.openFile(comps) + if err != nil { + h.fail(w, err) return } - fi, err := os.Stat(p) + defer func() { _ = f.Close() }() + fi, err := f.Stat() if err != nil { httpErr(w, err) return @@ -266,8 +201,8 @@ func (h *handler) download(w http.ResponseWriter, req *http.Request) { return } w.Header().Set("Content-Disposition", - fmt.Sprintf(`attachment; filename=%q`, filepath.Base(p))) - http.ServeFile(w, req, p) + fmt.Sprintf(`attachment; filename=%q`, fi.Name())) + http.ServeContent(w, req, fi.Name(), fi.ModTime(), f) } // maxWriteBytes caps a single /files/write body. The API-side ws proxy @@ -287,53 +222,23 @@ const maxUploadFileBytes = 64 << 20 const maxUploadFiles = 64 func (h *handler) write(w http.ResponseWriter, req *http.Request) { - p, err := h.resolve(req.URL.Query().Get("path")) + comps, err := h.components(req.URL.Query().Get("path")) if err != nil { - h.badRequest(w, err) + h.fail(w, err) return } defer func() { _ = req.Body.Close() }() - dir := filepath.Dir(p) - if err := os.MkdirAll(dir, dirMode); err != nil { - httpErr(w, err) - return - } - // Write to a temp file in the same directory, then rename it over the - // target only on success. os.Create truncates the target immediately, - // so any error after that point (ENOSPC, a body that ends early, an - // agent restart mid-copy) used to leave the previous file destroyed - // (F-102). - tmp, err := os.CreateTemp(dir, ".write-*") + body := http.MaxBytesReader(w, req.Body, maxWriteBytes) + // The data goes to a temp file next to the target and is renamed over it + // only on success, so any error after the copy starts (ENOSPC, a body that + // ends early, an agent restart mid-copy) leaves the previous file intact + // (F-102). The whole sequence runs through one parent directory descriptor. + err = h.writeFile(comps, func(dst io.Writer) error { + _, copyErr := io.Copy(dst, body) + return copyErr + }) if err != nil { - httpErr(w, err) - return - } - tmpName := tmp.Name() - _, copyErr := io.Copy(tmp, http.MaxBytesReader(w, req.Body, maxWriteBytes)) - closeErr := tmp.Close() - if copyErr != nil || closeErr != nil { - _ = os.Remove(tmpName) - if copyErr != nil { - httpErr(w, copyErr) - } else { - httpErr(w, closeErr) - } - return - } - // Chmod before rename: preserve the existing file's mode when overwriting, - // else use 0o644 so the game container (a different uid) can read it. - mode := os.FileMode(0o644) - if fi, err := os.Stat(filepath.Clean(p)); err == nil { - mode = fi.Mode().Perm() - } - if err := os.Chmod(tmpName, mode); err != nil { - _ = os.Remove(tmpName) - httpErr(w, err) - return - } - if err := os.Rename(tmpName, filepath.Clean(p)); err != nil { - _ = os.Remove(tmpName) - httpErr(w, err) + h.fail(w, err) return } w.WriteHeader(http.StatusNoContent) @@ -350,13 +255,13 @@ func (h *handler) upload(w http.ResponseWriter, req *http.Request) { h.badRequest(w, err) return } - p, err := h.resolve(req.URL.Query().Get("path")) + comps, err := h.components(req.URL.Query().Get("path")) if err != nil { - h.badRequest(w, err) + h.fail(w, err) return } - if err := os.MkdirAll(p, dirMode); err != nil { - httpErr(w, err) + if err := h.makeDirs(comps); err != nil { + h.fail(w, err) return } count := 0 @@ -390,14 +295,14 @@ func (h *handler) upload(w http.ResponseWriter, req *http.Request) { h.badRequest(w, fmt.Errorf("too many files (max %d)", maxUploadFiles)) return } - saveErr := savePart(h.root, p, part.FileName(), part, maxUploadFileBytes) + saveErr := h.storePart(comps, part.FileName(), part, maxUploadFileBytes) _ = part.Close() if saveErr != nil { - if errors.Is(saveErr, io.ErrUnexpectedEOF) || errors.Is(saveErr, errDotfile) || errors.Is(saveErr, errPathOutOfRoot) { + if errors.Is(saveErr, io.ErrUnexpectedEOF) { h.badRequest(w, saveErr) return } - httpErr(w, saveErr) + h.fail(w, saveErr) return } } @@ -409,15 +314,27 @@ func (h *handler) upload(w http.ResponseWriter, req *http.Request) { w.WriteHeader(http.StatusNoContent) } -// savePart streams one multipart part into dir under a sanitized name, -// refusing anything larger than limit bytes and any destination that is a -// symlink not resolving inside root. It writes to a temp file in -// dir first and renames it over the final name only once the copy -// succeeds, so a failure partway through (a truncated/erroring source, or -// an over-limit part) removes only the temp file — a pre-existing file at -// that name is left untouched instead of being deleted (F-102). +// savePart streams one multipart part into the directory dir (an absolute +// path below root) under a sanitized name, refusing anything larger than +// limit bytes and any destination that is a symlink. It is a thin wrapper +// over handler.storePart, which does the work through directory descriptors. func savePart(root, dir, filename string, src io.Reader, limit int64) error { - // Sanitize filename — reject anything that would climb out of dir. + h := &handler{root: root} + comps, err := relComponents(h.root, dir) + if err != nil { + return err + } + return h.storePart(comps, filename, src, limit) +} + +// storePart stores one upload part in the directory comps (below the root) +// under a sanitized name. It writes to a temp file in that directory first +// and renames it over the final name only once the copy succeeds, so a +// failure partway through (a truncated/erroring source, or an over-limit +// part) removes only the temp file — a pre-existing file at that name is left +// untouched instead of being deleted (F-102). +func (h *handler) storePart(comps []string, filename string, src io.Reader, limit int64) error { + // Sanitize filename — reject anything that would climb out of the directory. name := filepath.Base(filename) if name == "." || name == ".." || name == string(os.PathSeparator) { return errors.New("invalid filename") @@ -425,127 +342,53 @@ func savePart(root, dir, filename string, src io.Reader, limit int64) error { if strings.HasPrefix(name, ".") { return errDotfile } - dstPath := filepath.Clean(filepath.Join(dir, name)) - - // Confine the final destination: an existing symlink at this name must resolve inside root. - if info, err := os.Lstat(dstPath); err == nil && info.Mode()&os.ModeSymlink != 0 { - // Compare against the root with its own symlinks resolved too, so a - // data root reached through a symlinked path still accepts in-root links. - realRoot, rootErr := filepath.EvalSymlinks(root) - if rootErr != nil { - return fmt.Errorf("resolve root: %w", rootErr) - } - resolved, evalErr := filepath.EvalSymlinks(dstPath) - if evalErr != nil || (!strings.HasPrefix(resolved, realRoot+string(os.PathSeparator)) && resolved != realRoot) { - return errPathOutOfRoot - } - } - - tmp, err := os.CreateTemp(dir, ".upload-*") - if err != nil { - return err - } - tmpName := tmp.Name() - // Read one byte past the cap: if that byte materializes the part is - // over the limit, whatever its multipart headers claimed. - n, err := io.Copy(tmp, io.LimitReader(src, limit+1)) - closeErr := tmp.Close() - if err != nil || closeErr != nil { - _ = os.Remove(tmpName) + return h.storeIn(comps, name, ".upload-", false, func(dst io.Writer) error { + // Read one byte past the cap: if that byte materializes the part is + // over the limit, whatever its multipart headers claimed. + n, err := io.Copy(dst, io.LimitReader(src, limit+1)) if err != nil { return fmt.Errorf("save %q: %w", name, err) } - return fmt.Errorf("save %q: %w", name, closeErr) - } - if n > limit { - _ = os.Remove(tmpName) - return fmt.Errorf("file %q exceeds %d-byte limit", name, limit) - } - // Chmod before rename: preserve the existing file's mode when overwriting, - // else use 0o644 so the game container (a different uid) can read it. - mode := os.FileMode(0o644) - if fi, err := os.Stat(dstPath); err == nil { - mode = fi.Mode().Perm() - } - if err := os.Chmod(tmpName, mode); err != nil { - _ = os.Remove(tmpName) - return fmt.Errorf("save %q: %w", name, err) - } - if err := os.Rename(tmpName, dstPath); err != nil { - _ = os.Remove(tmpName) - return fmt.Errorf("save %q: %w", name, err) - } - return nil + if n > limit { + return fmt.Errorf("file %q exceeds %d-byte limit", name, limit) + } + return nil + }) } func (h *handler) mkdir(w http.ResponseWriter, req *http.Request) { - p, err := h.resolve(req.URL.Query().Get("path")) + comps, err := h.components(req.URL.Query().Get("path")) if err != nil { - h.badRequest(w, err) + h.fail(w, err) return } - if err := os.MkdirAll(p, dirMode); err != nil { - httpErr(w, err) + if err := h.makeDirs(comps); err != nil { + h.fail(w, err) return } w.WriteHeader(http.StatusNoContent) } func (h *handler) del(w http.ResponseWriter, req *http.Request) { - p, err := h.resolveForDelete(req.URL.Query().Get("path")) + comps, err := h.components(req.URL.Query().Get("path")) if err != nil { - h.badRequest(w, err) + h.fail(w, err) return } - if p == h.root { + if len(comps) == 0 { http.Error(w, "refusing to delete root", http.StatusBadRequest) return } - recursive := req.URL.Query().Get("recursive") == "true" - if recursive { - // A direct request for a dot-prefixed path is denied, so a recursive - // delete must not remove protected descendants either. - if fi, lerr := os.Lstat(p); lerr == nil && fi.IsDir() { - if derr := checkNoDotDescendants(p); derr != nil { - h.badRequest(w, derr) - return - } - } - } - var rerr error - if recursive { - rerr = os.RemoveAll(p) - } else { - rerr = os.Remove(p) - } - if rerr != nil { - httpErr(w, rerr) + // A recursive delete refuses trees containing dot-prefixed entries, since a + // direct request for such a path is denied too. A symlink is deleted as the + // link it is, never dereferenced. + if err := h.removePath(comps, req.URL.Query().Get("recursive") == "true"); err != nil { + h.fail(w, err) return } w.WriteHeader(http.StatusNoContent) } -// checkNoDotDescendants walks the directory tree rooted at dir and returns -// errDotfile if any dot-prefixed entry is found. Symlinks are not followed -// (os.RemoveAll does not follow them either). -func checkNoDotDescendants(dir string) error { - entries, err := os.ReadDir(dir) - if err != nil { - return fmt.Errorf("read %s: %w", dir, err) - } - for _, e := range entries { - if strings.HasPrefix(e.Name(), ".") { - return errDotfile - } - if e.IsDir() { - if err := checkNoDotDescendants(filepath.Join(dir, e.Name())); err != nil { - return err - } - } - } - return nil -} - func httpErr(w http.ResponseWriter, err error) { switch { case errors.Is(err, os.ErrNotExist): diff --git a/agent/internal/files/files_test.go b/agent/internal/files/files_test.go index 201f79795..2057e08ba 100644 --- a/agent/internal/files/files_test.go +++ b/agent/internal/files/files_test.go @@ -550,10 +550,11 @@ func TestSavePart_RejectsDanglingSymlinkDestination(t *testing.T) { } } -// TestSavePart_AllowsInRootSymlinkDestination checks that a destination -// symlink pointing at another folder inside root is accepted: the upload -// replaces the link and the linked file is left as it was. -func TestSavePart_AllowsInRootSymlinkDestination(t *testing.T) { +// TestSavePart_RejectsInRootSymlinkDestination checks that a destination +// symlink is refused even when it points at another folder inside root: +// link traversal is rejected at every component, and both the link and the +// linked file are left as they were. +func TestSavePart_RejectsInRootSymlinkDestination(t *testing.T) { root := resolvedTempDir(t) dirA := filepath.Join(root, "a") dirB := filepath.Join(root, "b") @@ -571,14 +572,15 @@ func TestSavePart_AllowsInRootSymlinkDestination(t *testing.T) { t.Fatalf("symlink: %v", err) } - if err := savePart(root, dirA, "x", strings.NewReader("new"), 1024); err != nil { - t.Fatalf("savePart: %v", err) + err := savePart(root, dirA, "x", strings.NewReader("new"), 1024) + if !errors.Is(err, errPathOutOfRoot) { + t.Fatalf("got %v, want %v", err, errPathOutOfRoot) } - got, err := os.ReadFile(linkPath) - if err != nil || string(got) != "new" { - t.Fatalf("upload not stored: got %q err=%v", got, err) + fi, err := os.Lstat(linkPath) + if err != nil || fi.Mode()&os.ModeSymlink == 0 { + t.Fatalf("symlink was replaced: fi=%v err=%v", fi, err) } - got, err = os.ReadFile(target) + got, err := os.ReadFile(target) if err != nil || string(got) != "orig" { t.Fatalf("linked file modified: got %q err=%v", got, err) } @@ -624,10 +626,11 @@ func symlinkedRoot(t *testing.T) (string, string) { return link, realRoot } -// TestUpload_SymlinkedRootAllowsInRootSymlinkDestination checks that when -// the data root itself is reached through a symlink, uploading over an -// existing symlink whose target is inside the root is accepted. -func TestUpload_SymlinkedRootAllowsInRootSymlinkDestination(t *testing.T) { +// TestUpload_SymlinkedRootRejectsInRootSymlinkDestination checks that when +// the data root itself is reached through a symlink (the root path is +// trusted and followed), uploading over an existing symlink whose target is +// inside the root is still rejected and leaves link and target untouched. +func TestUpload_SymlinkedRootRejectsInRootSymlinkDestination(t *testing.T) { linkRoot, realRoot := symlinkedRoot(t) if err := os.Mkdir(filepath.Join(realRoot, "b"), 0o755); err != nil { t.Fatalf("mkdir: %v", err) @@ -650,14 +653,14 @@ func TestUpload_SymlinkedRootAllowsInRootSymlinkDestination(t *testing.T) { t.Fatalf("post: %v", err) } defer resp.Body.Close() - if resp.StatusCode != http.StatusNoContent { + if resp.StatusCode != http.StatusBadRequest { t.Fatalf("status=%d body=%s", resp.StatusCode, readBody(resp)) } - got, err := os.ReadFile(filepath.Join(realRoot, "x")) - if err != nil || string(got) != "new" { - t.Fatalf("upload not stored: got %q err=%v", got, err) + fi, err := os.Lstat(filepath.Join(realRoot, "x")) + if err != nil || fi.Mode()&os.ModeSymlink == 0 { + t.Fatalf("symlink was replaced: fi=%v err=%v", fi, err) } - got, err = os.ReadFile(target) + got, err := os.ReadFile(target) if err != nil || string(got) != "orig" { t.Fatalf("linked file modified: got %q err=%v", got, err) } @@ -886,8 +889,10 @@ func TestResolve_SymlinkEscape(t *testing.T) { t.Skipf("symlink unsupported: %v", err) } h := &handler{root: resolved} - if _, err := h.resolve("/escape"); !errors.Is(err, errPathOutOfRoot) { - t.Fatalf("got %v", err) + rr := httptest.NewRecorder() + h.list(rr, httptest.NewRequestWithContext(t.Context(), http.MethodGet, "/files/list?path=/escape", nil)) + if rr.Code != http.StatusBadRequest || !strings.Contains(rr.Body.String(), "escapes root") { + t.Fatalf("status=%d body=%q, want 400 escapes root", rr.Code, rr.Body.String()) } } @@ -1055,24 +1060,34 @@ func TestResolve_SymlinkToDotfileRejected(t *testing.T) { t.Fatalf("symlink: %v", err) } h := &handler{root: resolved} + call := func(fn http.HandlerFunc, method, target string) *httptest.ResponseRecorder { + rr := httptest.NewRecorder() + fn(rr, httptest.NewRequestWithContext(t.Context(), method, target, strings.NewReader("x"))) + return rr + } + wantDotfile := func(rr *httptest.ResponseRecorder, what string) { + t.Helper() + if rr.Code != http.StatusBadRequest || rr.Body.String() != "dotfile access denied\n" { + t.Fatalf("%s: status=%d body=%q, want 400 dotfile access denied", what, rr.Code, rr.Body.String()) + } + } // Existing target reached through a link to a dotfile. - if _, err := h.resolve("/manifest"); !errors.Is(err, errDotfile) { - t.Fatalf("resolve(/manifest) err=%v, want errDotfile", err) - } - // New file whose existing ancestor is a link to a dot-directory. - if _, err := h.resolve("/state/new.txt"); !errors.Is(err, errDotfile) { - t.Fatalf("resolve(/state/new.txt) err=%v, want errDotfile", err) - } + wantDotfile(call(h.read, http.MethodGet, "/files/read?path=/manifest"), "read /manifest") + // New file whose ancestor is a link to a dot-directory. + wantDotfile(call(h.write, http.MethodPost, "/files/write?path=/state/new.txt"), "write /state/new.txt") // Delete of an entry inside a dot-directory reached through a link. - if _, err := h.resolveForDelete("/state/x"); !errors.Is(err, errDotfile) { - t.Fatalf("resolveForDelete(/state/x) err=%v, want errDotfile", err) - } + wantDotfile(call(h.del, http.MethodDelete, "/files/delete?path=/state/x"), "delete /state/x") // Deleting the link itself stays allowed: it removes the link, not the // dotfile it points to. - want := filepath.Join(resolved, "manifest") - if got, err := h.resolveForDelete("/manifest"); err != nil || got != want { - t.Fatalf("resolveForDelete(/manifest) = %q, %v; want %q", got, err, want) + if rr := call(h.del, http.MethodDelete, "/files/delete?path=/manifest"); rr.Code != http.StatusNoContent { + t.Fatalf("delete /manifest: status=%d body=%q, want 204", rr.Code, rr.Body.String()) + } + if _, err := os.Lstat(filepath.Join(resolved, "manifest")); !errors.Is(err, os.ErrNotExist) { + t.Fatalf("link still present: %v", err) + } + if _, err := os.Stat(filepath.Join(resolved, ".gameplane-mods.json")); err != nil { + t.Fatalf("dotfile was removed: %v", err) } } @@ -1271,7 +1286,7 @@ func TestUpload_FilePermissions(t *testing.T) { } } -func TestUpload_ReplacesSymlinkWithTargetMode(t *testing.T) { +func TestUpload_RejectsSymlinkDestinationInRoot(t *testing.T) { root := t.TempDir() // Create target file with restricted mode @@ -1286,25 +1301,25 @@ func TestUpload_ReplacesSymlinkWithTargetMode(t *testing.T) { t.Skipf("symlink unsupported: %v", err) } - // Upload replaces the symlink + // Upload refuses the symlink destination. src := strings.NewReader("new content") - if err := savePart(root, root, "link", src, 1000); err != nil { - t.Fatalf("savePart failed: %v", err) + if err := savePart(root, root, "link", src, 1000); !errors.Is(err, errPathOutOfRoot) { + t.Fatalf("savePart err=%v, want %v", err, errPathOutOfRoot) } - // Verify the link is now a regular file (not a symlink) + // The link is still a symlink and its target is untouched. fileStat, err := os.Lstat(link) if err != nil { t.Fatal(err) } - if fileStat.Mode()&os.ModeSymlink != 0 { - t.Fatal("expected regular file, got symlink") + if fileStat.Mode()&os.ModeSymlink == 0 { + t.Fatal("symlink was replaced") } - - // Verify the new file has the target's mode (0o600), not symlink mode (0o777) - newFileMode := fileStat.Mode().Perm() - expectedMode := os.FileMode(0o600) - if newFileMode != expectedMode { - t.Errorf("expected mode %o, got %o (symlink mode is 0o777; fix uses os.Stat, not os.Lstat)", expectedMode, newFileMode) + got, err := os.ReadFile(targetFile) + if err != nil || string(got) != "original" { + t.Fatalf("target modified: got %q err=%v", got, err) + } + if fi, err := os.Stat(targetFile); err != nil || fi.Mode().Perm() != 0o600 { + t.Fatalf("target mode changed: fi=%v err=%v", fi, err) } } diff --git a/agent/internal/files/rooted_linux.go b/agent/internal/files/rooted_linux.go new file mode 100644 index 000000000..602023d45 --- /dev/null +++ b/agent/internal/files/rooted_linux.go @@ -0,0 +1,574 @@ +package files + +import ( + "crypto/rand" + "errors" + "fmt" + "io" + "os" + "path/filepath" + "slices" + "strings" + "time" + + "golang.org/x/sys/unix" +) + +// This file holds every filesystem operation of the file browser. They all +// run relative to a directory descriptor opened on the data root: each path +// component is opened with openat(O_NOFOLLOW) so a symlink at any component is +// refused instead of followed, and reads, writes, creates and deletes go +// through the descriptors they obtained rather than re-opening a path string. +// A directory or file swapped for a symlink after the request was validated +// therefore cannot redirect the operation outside the root. + +const ( + // dirFlags opens a directory component: no symlink at the last component, + // must be a directory. + dirFlags = unix.O_RDONLY | unix.O_DIRECTORY | unix.O_NOFOLLOW | unix.O_CLOEXEC + // fileFlags opens a final component for reading: no symlink. + fileFlags = unix.O_RDONLY | unix.O_NOFOLLOW | unix.O_CLOEXEC +) + +// errLinkTraversal is returned when a path component is a symlink. It wraps +// errPathOutOfRoot so it is echoed to the client as a 400 and the body still +// contains "escapes root". +var errLinkTraversal = fmt.Errorf("%w (symbolic link in path)", errPathOutOfRoot) + +// errBadPath marks an ancestor component that is not a directory. It is not +// echoed (generic 400 "bad request"), matching the old EvalSymlinks ENOTDIR +// behaviour. +var errBadPath = errors.New("path component is not a directory") + +// fire invokes the test seam, if any. +func (h *handler) fire(stage string, parents []string, name string) { + if h.beforeOpen == nil { + return + } + h.beforeOpen(stage, strings.Join(parents, "/"), name) +} + +// openat is unix.Openat retried on EINTR. +func openat(dirFD int, name string, flags int, mode uint32) (int, error) { + for { + fd, err := unix.Openat(dirFD, name, flags, mode) + if !errors.Is(err, unix.EINTR) { + return fd, err + } + } +} + +// closeFD closes a descriptor, ignoring the error (read-only directory +// descriptors; there is nothing useful to do with it). +func closeFD(fd int) { + _ = unix.Close(fd) +} + +// fileFromFD wraps fd, returned by a successful openat, in an *os.File. A +// successful openat never yields a negative descriptor; the guard keeps the +// int to uintptr conversion provably in range (gosec G115). +func fileFromFD(fd int, name string) (*os.File, error) { + if fd < 0 { + return nil, fmt.Errorf("open %q: %w", name, unix.EBADF) + } + return os.NewFile(uintptr(fd), name), nil +} + +// openRoot opens the configured data root. The root path is operator-supplied +// and trusted (it may legitimately be reached through a symlink), so it is the +// only component that is followed. +func openRoot(root string) (int, error) { + fd, err := openat(unix.AT_FDCWD, root, unix.O_RDONLY|unix.O_DIRECTORY|unix.O_CLOEXEC, 0) + if err != nil { + return -1, fmt.Errorf("open root: %w", err) + } + return fd, nil +} + +// relComponents returns the components of abs below root (nil for the root +// itself) or errPathOutOfRoot if abs is not below root. +func relComponents(root, abs string) ([]string, error) { + rel, err := filepath.Rel(root, abs) + if err != nil || rel == ".." || strings.HasPrefix(rel, ".."+string(os.PathSeparator)) { + return nil, errPathOutOfRoot + } + if rel == "." { + return nil, nil + } + return strings.Split(rel, string(os.PathSeparator)), nil +} + +// parentsOf returns all but the last of comps. +func parentsOf(comps []string) []string { + if len(comps) == 0 { + return nil + } + return comps[:len(comps)-1] +} + +// entryPath is the client-visible path of name inside the directory comps. +func entryPath(comps []string, name string) string { + return "/" + strings.Join(append(slices.Clone(comps), name), "/") +} + +// fileModeFromStat converts a stat(2) mode word to an os.FileMode, matching +// what os.Lstat reports. +func fileModeFromStat(m uint32) os.FileMode { + mode := os.FileMode(m & 0o777) + switch m & unix.S_IFMT { + case unix.S_IFBLK: + mode |= os.ModeDevice + case unix.S_IFCHR: + mode |= os.ModeDevice | os.ModeCharDevice + case unix.S_IFDIR: + mode |= os.ModeDir + case unix.S_IFIFO: + mode |= os.ModeNamedPipe + case unix.S_IFLNK: + mode |= os.ModeSymlink + case unix.S_IFSOCK: + mode |= os.ModeSocket + } + if m&unix.S_ISGID != 0 { + mode |= os.ModeSetgid + } + if m&unix.S_ISUID != 0 { + mode |= os.ModeSetuid + } + if m&unix.S_ISVTX != 0 { + mode |= os.ModeSticky + } + return mode +} + +// symlinkError builds the error for a refused symlink at name inside dirFD +// (reached through parents). The link is never followed; its target is read +// only to pick the message: a target lexically below a dot-prefixed component +// is reported as errDotfile (so the response matches a direct request for that +// dotfile), anything else as errLinkTraversal. +func (h *handler) symlinkError(dirFD int, parents []string, name string) error { + buf := make([]byte, 4096) + n, err := unix.Readlinkat(dirFD, name, buf) + if err != nil || n <= 0 { + return errLinkTraversal + } + target := string(buf[:n]) + var abs string + if filepath.IsAbs(target) { + abs = filepath.Clean(target) + } else { + abs = filepath.Join(h.root, filepath.Join(parents...), target) + } + rel, err := filepath.Rel(h.root, abs) + if err == nil && rel != "." && rel != ".." && !strings.HasPrefix(rel, ".."+string(os.PathSeparator)) && + hasDotComponent(filepath.ToSlash(rel)) { + return errDotfile + } + return errLinkTraversal +} + +// openChild opens name inside dirFD with the given flags (dirFlags or +// fileFlags, both O_NOFOLLOW). A symlink at name is refused with a +// symlinkError; Linux reports it as ELOOP or, together with O_DIRECTORY, as +// ENOTDIR, so both are disambiguated with an lstat-style fstatat. +func (h *handler) openChild(dirFD int, parents []string, name string, flags int) (int, error) { + h.fire("open", parents, name) + fd, err := openat(dirFD, name, flags, 0) + if err == nil { + return fd, nil + } + if errors.Is(err, unix.ELOOP) || errors.Is(err, unix.ENOTDIR) { + var st unix.Stat_t + if serr := unix.Fstatat(dirFD, name, &st, unix.AT_SYMLINK_NOFOLLOW); serr == nil && st.Mode&unix.S_IFMT == unix.S_IFLNK { + return -1, h.symlinkError(dirFD, parents, name) + } + } + return -1, fmt.Errorf("open %q: %w", name, err) +} + +// walkDirs opens the directory comps below rootFD, one openat per component, +// and returns a new descriptor for it (the caller closes it; rootFD stays +// open). With create, missing directories are created with mkdirat. target +// says comps ends at the operation's own target (mkdir/upload), so ENOTDIR at +// the last component is a plain error instead of a bad ancestor. +func (h *handler) walkDirs(rootFD int, comps []string, create, target bool) (int, error) { + cur, err := openat(rootFD, ".", dirFlags, 0) + if err != nil { + return -1, fmt.Errorf("open root dir: %w", err) + } + for i, name := range comps { + next, oerr := h.openChild(cur, comps[:i], name, dirFlags) + if errors.Is(oerr, unix.ENOENT) && create { + merr := unix.Mkdirat(cur, name, dirMode) + if merr != nil && !errors.Is(merr, unix.EEXIST) { + closeFD(cur) + return -1, fmt.Errorf("mkdir %q: %w", name, merr) + } + next, oerr = h.openChild(cur, comps[:i], name, dirFlags) + } + closeFD(cur) + if oerr != nil { + isTarget := target && i == len(comps)-1 + if errors.Is(oerr, unix.ENOTDIR) && !isTarget { + return -1, fmt.Errorf("%w: %w", errBadPath, oerr) + } + return -1, oerr + } + cur = next + } + return cur, nil +} + +// locate opens the directory that holds the last component of comps and +// returns it with that component's name. For the root itself (no components) +// it returns the root directory and an empty name. +func (h *handler) locate(rootFD int, comps []string, create bool) (int, string, error) { + if len(comps) == 0 { + fd, err := h.walkDirs(rootFD, nil, false, false) + return fd, "", err + } + fd, err := h.walkDirs(rootFD, parentsOf(comps), create, false) + return fd, comps[len(comps)-1], err +} + +// dirNames lists the entry names of the directory open as fd, sorted. It +// reads through a fresh descriptor for the same directory so the caller's fd +// offset is untouched. +func dirNames(fd int) ([]string, error) { + rd, err := openat(fd, ".", dirFlags, 0) + if err != nil { + return nil, fmt.Errorf("reopen directory: %w", err) + } + f, err := fileFromFD(rd, ".") + if err != nil { + return nil, err + } + defer func() { _ = f.Close() }() + names, err := f.Readdirnames(-1) + if err != nil { + return nil, fmt.Errorf("read directory: %w", err) + } + slices.Sort(names) + return names, nil +} + +// listDir returns the visible (non-dot) entries of the directory comps. +func (h *handler) listDir(comps []string) ([]Entry, error) { + rootFD, err := openRoot(h.root) + if err != nil { + return nil, err + } + defer closeFD(rootFD) + parentFD, name, err := h.locate(rootFD, comps, false) + if err != nil { + return nil, err + } + defer closeFD(parentFD) + dirFD := parentFD + if name != "" { + dirFD, err = h.openChild(parentFD, parentsOf(comps), name, dirFlags) + if err != nil { + return nil, err + } + defer closeFD(dirFD) + } + names, err := dirNames(dirFD) + if err != nil { + return nil, err + } + out := make([]Entry, 0, len(names)) + for _, n := range names { + // Skip dot-prefixed entries (dotfiles and dot-directories). + if strings.HasPrefix(n, ".") { + continue + } + var st unix.Stat_t + if unix.Fstatat(dirFD, n, &st, unix.AT_SYMLINK_NOFOLLOW) != nil { + continue + } + mode := fileModeFromStat(st.Mode) + out = append(out, Entry{ + Name: n, + Path: entryPath(comps, n), + Size: st.Size, + Mode: mode.String(), + Dir: mode.IsDir(), + ModTime: time.Unix(st.Mtim.Unix()).UTC().Format(time.RFC3339), + }) + } + return out, nil +} + +// openFile opens the file (or directory) comps for reading without following +// a symlink at any component. The caller closes the returned file. +func (h *handler) openFile(comps []string) (*os.File, error) { + rootFD, err := openRoot(h.root) + if err != nil { + return nil, err + } + defer closeFD(rootFD) + parentFD, name, err := h.locate(rootFD, comps, false) + if err != nil { + return nil, err + } + defer closeFD(parentFD) + if name == "" { + name = "." // the root directory itself + } + fd, err := h.openChild(parentFD, parentsOf(comps), name, fileFlags) + if err != nil { + return nil, err + } + return fileFromFD(fd, name) +} + +// makeDirs creates the directory comps (and any missing ancestors), the +// descriptor-relative equivalent of os.MkdirAll. +func (h *handler) makeDirs(comps []string) error { + rootFD, err := openRoot(h.root) + if err != nil { + return err + } + defer closeFD(rootFD) + fd, err := h.walkDirs(rootFD, comps, true, true) + if err != nil { + return err + } + closeFD(fd) + return nil +} + +// createTemp creates a fresh 0o600 temp file named prefix+random in dirFD, +// never following a symlink. +func createTemp(dirFD int, prefix string) (*os.File, string, error) { + for range 16 { + name := prefix + rand.Text() + fd, err := openat(dirFD, name, unix.O_WRONLY|unix.O_CREAT|unix.O_EXCL|unix.O_NOFOLLOW|unix.O_CLOEXEC, 0o600) + if err == nil { + f, ferr := fileFromFD(fd, name) + return f, name, ferr + } + if !errors.Is(err, unix.EEXIST) { + return nil, "", fmt.Errorf("create temp file: %w", err) + } + } + return nil, "", fmt.Errorf("create temp file: %w", unix.EEXIST) +} + +// destMode returns the permission bits a new file at name should get: those of +// the existing regular file, else 0o644 so the game container (a different +// uid) can read it. A symlink at name is refused. +func (h *handler) destMode(dirFD int, parents []string, name string) (os.FileMode, error) { + h.fire("stat", parents, name) + var st unix.Stat_t + err := unix.Fstatat(dirFD, name, &st, unix.AT_SYMLINK_NOFOLLOW) + if errors.Is(err, unix.ENOENT) { + return 0o644, nil + } + if err != nil { + return 0, fmt.Errorf("stat %q: %w", name, err) + } + if st.Mode&unix.S_IFMT == unix.S_IFLNK { + return 0, h.symlinkError(dirFD, parents, name) + } + return fileModeFromStat(st.Mode).Perm(), nil +} + +// storeFile writes name inside dirFD atomically: fill streams the content +// into a temp file created in dirFD, which is chmodded, fsynced and renamed +// over name with renameat in the same directory. On any failure only the temp +// file is removed; an existing name is left untouched. renameat replaces a +// symlink that appears at name in the meantime instead of following it. +func (h *handler) storeFile(dirFD int, parents []string, name, prefix string, fill func(io.Writer) error) error { + mode, err := h.destMode(dirFD, parents, name) + if err != nil { + return err + } + tmp, tmpName, err := createTemp(dirFD, prefix) + if err != nil { + return err + } + abort := func(cause error) error { + _ = tmp.Close() + _ = unix.Unlinkat(dirFD, tmpName, 0) + return cause + } + if err = fill(tmp); err != nil { + return abort(err) + } + // Chmod before rename: preserve the existing file's mode when overwriting, + // else use 0o644 so the game container (a different uid) can read it. + if err = tmp.Chmod(mode); err != nil { + return abort(fmt.Errorf("chmod %q: %w", name, err)) + } + if err = tmp.Sync(); err != nil { + return abort(fmt.Errorf("sync %q: %w", name, err)) + } + if err = tmp.Close(); err != nil { + return abort(fmt.Errorf("close %q: %w", name, err)) + } + h.fire("commit", parents, name) + if err = unix.Renameat(dirFD, tmpName, dirFD, name); err != nil { + return abort(fmt.Errorf("rename %q: %w", name, err)) + } + return nil +} + +// storeIn opens the directory comps (creating missing ancestors when create is +// set) and stores name in it atomically via storeFile. +func (h *handler) storeIn(comps []string, name, prefix string, create bool, fill func(io.Writer) error) error { + rootFD, err := openRoot(h.root) + if err != nil { + return err + } + defer closeFD(rootFD) + dirFD, err := h.walkDirs(rootFD, comps, create, false) + if err != nil { + return err + } + defer closeFD(dirFD) + return h.storeFile(dirFD, comps, name, prefix, fill) +} + +// writeFile atomically writes the file comps, creating missing ancestors. +func (h *handler) writeFile(comps []string, fill func(io.Writer) error) error { + if len(comps) == 0 { + return fmt.Errorf("write root: %w", unix.EISDIR) + } + return h.storeIn(parentsOf(comps), comps[len(comps)-1], ".write-", true, fill) +} + +// removeEntry removes the single entry name in dirFD like os.Remove: unlink, +// then rmdir. A symlink is removed as the link it is. +func removeEntry(dirFD int, name string) error { + uerr := unix.Unlinkat(dirFD, name, 0) + if uerr == nil { + return nil + } + rerr := unix.Unlinkat(dirFD, name, unix.AT_REMOVEDIR) + if rerr == nil { + return nil + } + if errors.Is(rerr, unix.ENOTDIR) { + return uerr + } + return rerr +} + +// removePath deletes the entry comps. Non-recursive: unlink/rmdir of the one +// entry. Recursive: refuses (errDotfile) a tree containing dot-prefixed +// entries, then removes the tree through descriptors without following +// symlinks (they are unlinked as links). +func (h *handler) removePath(comps []string, recursive bool) error { + rootFD, err := openRoot(h.root) + if err != nil { + return err + } + defer closeFD(rootFD) + parentFD, name, err := h.locate(rootFD, comps, false) + if err != nil { + if errors.Is(err, os.ErrNotExist) { + // A missing parent was a generic 400 before descriptors; keep it. + return fmt.Errorf("%w: %w", errBadPath, err) + } + return err + } + defer closeFD(parentFD) + parents := parentsOf(comps) + if !recursive { + h.fire("remove", parents, name) + return removeEntry(parentFD, name) + } + if err = h.checkNoDotAt(parentFD, parents, name); err != nil { + return err + } + return h.removeTree(parentFD, parents, name) +} + +// checkNoDotAt returns errDotfile if the directory tree at name (inside dirFD) +// contains a dot-prefixed entry. Symlinks are not followed. +func (h *handler) checkNoDotAt(dirFD int, parents []string, name string) error { + var st unix.Stat_t + err := unix.Fstatat(dirFD, name, &st, unix.AT_SYMLINK_NOFOLLOW) + if errors.Is(err, unix.ENOENT) { + return nil + } + if err != nil { + return fmt.Errorf("stat %q: %w", name, err) + } + if st.Mode&unix.S_IFMT != unix.S_IFDIR { + return nil + } + fd, err := h.openChild(dirFD, parents, name, dirFlags) + if err != nil { + return err + } + defer closeFD(fd) + names, err := dirNames(fd) + if err != nil { + return err + } + here := append(slices.Clone(parents), name) + for _, n := range names { + if strings.HasPrefix(n, ".") { + return errDotfile + } + if err = h.checkNoDotAt(fd, here, n); err != nil { + return err + } + } + return nil +} + +// removeTree removes name inside dirFD, recursing into real directories only. +// A missing entry is not an error (os.RemoveAll semantics). +func (h *handler) removeTree(dirFD int, parents []string, name string) error { + var st unix.Stat_t + err := unix.Fstatat(dirFD, name, &st, unix.AT_SYMLINK_NOFOLLOW) + if errors.Is(err, unix.ENOENT) { + return nil + } + if err != nil { + return fmt.Errorf("stat %q: %w", name, err) + } + if st.Mode&unix.S_IFMT != unix.S_IFDIR { + err = unix.Unlinkat(dirFD, name, 0) + if err != nil && !errors.Is(err, unix.ENOENT) { + return fmt.Errorf("remove %q: %w", name, err) + } + return nil + } + fd, err := h.openChild(dirFD, parents, name, dirFlags) + if err != nil { + return err + } + err = h.removeChildren(fd, append(slices.Clone(parents), name)) + closeFD(fd) + if err != nil { + return err + } + err = unix.Unlinkat(dirFD, name, unix.AT_REMOVEDIR) + if err != nil && !errors.Is(err, unix.ENOENT) { + return fmt.Errorf("remove %q: %w", name, err) + } + return nil +} + +// removeChildren removes every entry of the directory open as fd. A +// dot-prefixed entry (created after the pre-check) stops the removal with +// errDotfile instead of being deleted. +func (h *handler) removeChildren(fd int, path []string) error { + names, err := dirNames(fd) + if err != nil { + return err + } + for _, n := range names { + if strings.HasPrefix(n, ".") { + return errDotfile + } + if err = h.removeTree(fd, path, n); err != nil { + return err + } + } + return nil +} diff --git a/agent/internal/files/rooted_test.go b/agent/internal/files/rooted_test.go new file mode 100644 index 000000000..22779dc63 --- /dev/null +++ b/agent/internal/files/rooted_test.go @@ -0,0 +1,495 @@ +package files + +import ( + "encoding/json" + "errors" + "io" + "net/http" + "net/http/httptest" + "os" + "path/filepath" + "slices" + "strings" + "testing" + + "golang.org/x/sys/unix" +) + +const outsideSecret = "outside-secret" + +// put writes content to path, creating parent directories. +func put(t *testing.T, path, content string) { + t.Helper() + if err := os.MkdirAll(filepath.Dir(path), 0o755); err != nil { + t.Fatalf("mkdir %s: %v", filepath.Dir(path), err) + } + if err := os.WriteFile(path, []byte(content), 0o600); err != nil { + t.Fatalf("write %s: %v", path, err) + } +} + +// raceHandler returns a handler on a fresh root and a separate "outside" +// directory holding secret.txt, which must never be read, written or deleted. +func raceHandler(t *testing.T) (*handler, string, string) { + t.Helper() + root := resolvedTempDir(t) + outside := resolvedTempDir(t) + put(t, filepath.Join(outside, "secret.txt"), outsideSecret) + return &handler{root: root}, root, outside +} + +// serve calls a handler method directly (same goroutine, no server). +func serve(t *testing.T, fn http.HandlerFunc, method, target string, body io.Reader) *httptest.ResponseRecorder { + t.Helper() + rr := httptest.NewRecorder() + fn(rr, httptest.NewRequestWithContext(t.Context(), method, target, body)) + return rr +} + +// swapForSymlink moves path aside (path+".moved") and puts a symlink to +// target in its place, simulating another process racing the request. +func swapForSymlink(t *testing.T, path, target string) { + t.Helper() + if err := os.Rename(path, path+".moved"); err != nil { + t.Fatalf("move aside %s: %v", path, err) + } + if err := os.Symlink(target, path); err != nil { + t.Fatalf("symlink %s: %v", path, err) + } +} + +// raceAt installs a hook that runs swap on the nth call matching +// (stage, dir, name) and fails the test if that call is never reached. +func raceAt(t *testing.T, h *handler, stage, dir, name string, nth int, swap func()) { + t.Helper() + seen := 0 + h.beforeOpen = func(s, d, n string) { + if s != stage || d != dir || n != name { + return + } + seen++ + if seen == nth { + swap() + } + } + t.Cleanup(func() { + if seen < nth { + t.Errorf("race hook %s %q/%q never reached call %d (saw %d)", stage, dir, name, nth, seen) + } + }) +} + +func assertRejected(t *testing.T, rr *httptest.ResponseRecorder) { + t.Helper() + if rr.Code != http.StatusBadRequest { + t.Fatalf("status=%d body=%q, want 400", rr.Code, rr.Body.String()) + } + if strings.Contains(rr.Body.String(), outsideSecret) { + t.Fatalf("response leaked outside content: %q", rr.Body.String()) + } + if !strings.Contains(rr.Body.String(), "escapes root") { + t.Fatalf("body=%q, want it to mention 'escapes root'", rr.Body.String()) + } +} + +func assertOutsideUntouched(t *testing.T, outside string) { + t.Helper() + entries, err := os.ReadDir(outside) + if err != nil { + t.Fatalf("readdir outside: %v", err) + } + if len(entries) != 1 || entries[0].Name() != "secret.txt" { + t.Fatalf("outside directory modified: %v", entries) + } + got, err := os.ReadFile(filepath.Join(outside, "secret.txt")) + if err != nil || string(got) != outsideSecret { + t.Fatalf("outside secret modified: got %q err=%v", got, err) + } +} + +func TestRace_ReadDirectorySwappedForSymlink(t *testing.T) { + h, root, outside := raceHandler(t) + put(t, filepath.Join(root, "sub", "secret.txt"), "inside") + raceAt(t, h, "open", "", "sub", 1, func() { swapForSymlink(t, filepath.Join(root, "sub"), outside) }) + rr := serve(t, h.read, http.MethodGet, "/files/read?path=/sub/secret.txt", nil) + assertRejected(t, rr) + assertOutsideUntouched(t, outside) +} + +func TestRace_ReadFileSwappedForSymlink(t *testing.T) { + h, root, outside := raceHandler(t) + put(t, filepath.Join(root, "data.txt"), "inside") + raceAt(t, h, "open", "", "data.txt", 1, func() { + swapForSymlink(t, filepath.Join(root, "data.txt"), filepath.Join(outside, "secret.txt")) + }) + rr := serve(t, h.read, http.MethodGet, "/files/read?path=/data.txt", nil) + assertRejected(t, rr) + assertOutsideUntouched(t, outside) +} + +func TestRace_DownloadFileSwappedForSymlink(t *testing.T) { + h, root, outside := raceHandler(t) + put(t, filepath.Join(root, "data.txt"), "inside") + raceAt(t, h, "open", "", "data.txt", 1, func() { + swapForSymlink(t, filepath.Join(root, "data.txt"), filepath.Join(outside, "secret.txt")) + }) + rr := serve(t, h.download, http.MethodGet, "/files/download?path=/data.txt", nil) + assertRejected(t, rr) + assertOutsideUntouched(t, outside) +} + +func TestRace_ListDirectorySwappedForSymlink(t *testing.T) { + h, root, outside := raceHandler(t) + put(t, filepath.Join(root, "sub", "a.txt"), "inside") + raceAt(t, h, "open", "", "sub", 1, func() { swapForSymlink(t, filepath.Join(root, "sub"), outside) }) + rr := serve(t, h.list, http.MethodGet, "/files/list?path=/sub", nil) + assertRejected(t, rr) + if strings.Contains(rr.Body.String(), "secret.txt") { + t.Fatalf("listing leaked outside names: %q", rr.Body.String()) + } + assertOutsideUntouched(t, outside) +} + +func TestRace_WriteDirectorySwappedForSymlink(t *testing.T) { + h, root, outside := raceHandler(t) + if err := os.Mkdir(filepath.Join(root, "sub"), 0o755); err != nil { + t.Fatalf("mkdir: %v", err) + } + raceAt(t, h, "open", "", "sub", 1, func() { swapForSymlink(t, filepath.Join(root, "sub"), outside) }) + rr := serve(t, h.write, http.MethodPost, "/files/write?path=/sub/new.txt", strings.NewReader("payload")) + assertRejected(t, rr) + assertOutsideUntouched(t, outside) +} + +// The destination name is swapped for a symlink to an outside file right +// before the final rename: renameat replaces the link itself, the outside +// file is not written through it. +func TestRace_WriteDestinationSwappedForSymlink(t *testing.T) { + h, root, outside := raceHandler(t) + put(t, filepath.Join(root, "target.txt"), "old") + raceAt(t, h, "commit", "", "target.txt", 1, func() { + swapForSymlink(t, filepath.Join(root, "target.txt"), filepath.Join(outside, "secret.txt")) + }) + rr := serve(t, h.write, http.MethodPost, "/files/write?path=/target.txt", strings.NewReader("new")) + if rr.Code != http.StatusNoContent { + t.Fatalf("status=%d body=%q, want 204", rr.Code, rr.Body.String()) + } + assertOutsideUntouched(t, outside) + fi, err := os.Lstat(filepath.Join(root, "target.txt")) + if err != nil || !fi.Mode().IsRegular() { + t.Fatalf("target.txt not a regular file after write: fi=%v err=%v", fi, err) + } + got, err := os.ReadFile(filepath.Join(root, "target.txt")) + if err != nil || string(got) != "new" { + t.Fatalf("target.txt content=%q err=%v", got, err) + } +} + +// The directory is swapped for a symlink after its descriptor was opened: +// the write lands in the original (moved) directory, never outside the root. +func TestRace_HeldDirectoryDescriptorStaysInRoot(t *testing.T) { + h, root, outside := raceHandler(t) + if err := os.Mkdir(filepath.Join(root, "sub"), 0o755); err != nil { + t.Fatalf("mkdir: %v", err) + } + raceAt(t, h, "commit", "sub", "f.txt", 1, func() { swapForSymlink(t, filepath.Join(root, "sub"), outside) }) + rr := serve(t, h.write, http.MethodPost, "/files/write?path=/sub/f.txt", strings.NewReader("payload")) + if rr.Code != http.StatusNoContent { + t.Fatalf("status=%d body=%q, want 204", rr.Code, rr.Body.String()) + } + got, err := os.ReadFile(filepath.Join(root, "sub.moved", "f.txt")) + if err != nil || string(got) != "payload" { + t.Fatalf("write did not land in the original directory: got %q err=%v", got, err) + } + assertOutsideUntouched(t, outside) +} + +func TestRace_MkdirAncestorSwappedForSymlink(t *testing.T) { + h, root, outside := raceHandler(t) + if err := os.Mkdir(filepath.Join(root, "a"), 0o755); err != nil { + t.Fatalf("mkdir: %v", err) + } + raceAt(t, h, "open", "", "a", 1, func() { swapForSymlink(t, filepath.Join(root, "a"), outside) }) + rr := serve(t, h.mkdir, http.MethodPost, "/files/mkdir?path=/a/b/c", nil) + assertRejected(t, rr) + assertOutsideUntouched(t, outside) +} + +// The upload directory is swapped between the directory creation step (open #1) +// and the part being stored (open #2). +func TestRace_UploadDirectorySwappedForSymlink(t *testing.T) { + h, root, outside := raceHandler(t) + if err := os.Mkdir(filepath.Join(root, "up"), 0o755); err != nil { + t.Fatalf("mkdir: %v", err) + } + raceAt(t, h, "open", "", "up", 2, func() { swapForSymlink(t, filepath.Join(root, "up"), outside) }) + buf, ct := multipartBody(t, map[string]string{"x.txt": "payload"}) + req := httptest.NewRequestWithContext(t.Context(), http.MethodPost, "/files/upload?path=/up", buf) + req.Header.Set("Content-Type", ct) + rr := httptest.NewRecorder() + h.upload(rr, req) + assertRejected(t, rr) + assertOutsideUntouched(t, outside) +} + +func TestRace_DeleteAncestorSwappedForSymlink(t *testing.T) { + h, root, outside := raceHandler(t) + put(t, filepath.Join(root, "a", "b", "file"), "inside") + put(t, filepath.Join(outside, "b", "file"), "outside-keep") + raceAt(t, h, "open", "", "a", 1, func() { swapForSymlink(t, filepath.Join(root, "a"), outside) }) + rr := serve(t, h.del, http.MethodDelete, "/files/delete?path=/a/b&recursive=true", nil) + if rr.Code != http.StatusBadRequest { + t.Fatalf("status=%d body=%q, want 400", rr.Code, rr.Body.String()) + } + got, err := os.ReadFile(filepath.Join(outside, "b", "file")) + if err != nil || string(got) != "outside-keep" { + t.Fatalf("outside file deleted or modified: got %q err=%v", got, err) + } +} + +// A child directory of the tree being deleted is swapped for a symlink after +// the removal pass saw it as a directory (open #2: the first open of +// tree/inner is the dotfile pre-check, the second is the removal pass). +func TestRace_RecursiveDeleteChildSwappedForSymlink(t *testing.T) { + h, root, outside := raceHandler(t) + put(t, filepath.Join(root, "tree", "inner", "file"), "inside") + raceAt(t, h, "open", "tree", "inner", 2, func() { swapForSymlink(t, filepath.Join(root, "tree", "inner"), outside) }) + rr := serve(t, h.del, http.MethodDelete, "/files/delete?path=/tree&recursive=true", nil) + assertRejected(t, rr) + assertOutsideUntouched(t, outside) +} + +func TestRooted_InRootSymlinkRejectedAtEveryComponent(t *testing.T) { + h, root, _ := raceHandler(t) + put(t, filepath.Join(root, "real", "f.txt"), "inside") + if err := os.Symlink(filepath.Join(root, "real"), filepath.Join(root, "lnk")); err != nil { + t.Fatalf("symlink: %v", err) + } + cases := []struct { + name string + fn http.HandlerFunc + method string + target string + }{ + {"read", h.read, http.MethodGet, "/files/read?path=/lnk/f.txt"}, + {"download", h.download, http.MethodGet, "/files/download?path=/lnk/f.txt"}, + {"list", h.list, http.MethodGet, "/files/list?path=/lnk"}, + {"write", h.write, http.MethodPost, "/files/write?path=/lnk/n.txt"}, + {"mkdir", h.mkdir, http.MethodPost, "/files/mkdir?path=/lnk/sub"}, + {"delete", h.del, http.MethodDelete, "/files/delete?path=/lnk/f.txt"}, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + rr := serve(t, tc.fn, tc.method, tc.target, strings.NewReader("x")) + if rr.Code != http.StatusBadRequest || !strings.Contains(rr.Body.String(), "escapes root") { + t.Fatalf("status=%d body=%q, want 400 escapes root", rr.Code, rr.Body.String()) + } + }) + } + entries, err := os.ReadDir(filepath.Join(root, "real")) + if err != nil || len(entries) != 1 || entries[0].Name() != "f.txt" { + t.Fatalf("real directory modified: %v err=%v", entries, err) + } +} + +func TestRooted_WriteRejectsSymlinkDestination(t *testing.T) { + h, root, _ := raceHandler(t) + put(t, filepath.Join(root, "real.txt"), "orig") + if err := os.Symlink(filepath.Join(root, "real.txt"), filepath.Join(root, "link")); err != nil { + t.Fatalf("symlink: %v", err) + } + rr := serve(t, h.write, http.MethodPost, "/files/write?path=/link", strings.NewReader("new")) + if rr.Code != http.StatusBadRequest { + t.Fatalf("status=%d body=%q, want 400", rr.Code, rr.Body.String()) + } + got, err := os.ReadFile(filepath.Join(root, "real.txt")) + if err != nil || string(got) != "orig" { + t.Fatalf("target modified: got %q err=%v", got, err) + } + entries, err := os.ReadDir(root) + if err != nil || len(entries) != 2 { + t.Fatalf("unexpected root contents (stray temp file?): %v err=%v", entries, err) + } +} + +func TestRooted_RecursiveDeleteDoesNotFollowSymlinks(t *testing.T) { + h, root, outside := raceHandler(t) + put(t, filepath.Join(root, "tree", "a.txt"), "inside") + if err := os.Symlink(outside, filepath.Join(root, "tree", "out")); err != nil { + t.Fatalf("symlink: %v", err) + } + rr := serve(t, h.del, http.MethodDelete, "/files/delete?path=/tree&recursive=true", nil) + if rr.Code != http.StatusNoContent { + t.Fatalf("status=%d body=%q, want 204", rr.Code, rr.Body.String()) + } + if _, err := os.Lstat(filepath.Join(root, "tree")); !errors.Is(err, os.ErrNotExist) { + t.Fatalf("tree still present: %v", err) + } + assertOutsideUntouched(t, outside) +} + +func TestRooted_SymlinkedRootStillServes(t *testing.T) { + linkRoot, realRoot := symlinkedRoot(t) + put(t, filepath.Join(realRoot, "f.txt"), "hello") + h := &handler{root: linkRoot} + rr := serve(t, h.read, http.MethodGet, "/files/read?path=/f.txt", nil) + if rr.Code != http.StatusOK || rr.Body.String() != "hello" { + t.Fatalf("status=%d body=%q, want 200 hello", rr.Code, rr.Body.String()) + } + rr = serve(t, h.write, http.MethodPost, "/files/write?path=/g/n.txt", strings.NewReader("new")) + if rr.Code != http.StatusNoContent { + t.Fatalf("write status=%d body=%q, want 204", rr.Code, rr.Body.String()) + } + got, err := os.ReadFile(filepath.Join(realRoot, "g", "n.txt")) + if err != nil || string(got) != "new" { + t.Fatalf("write not stored: got %q err=%v", got, err) + } +} + +func TestRooted_RelativeSymlinkToDotTargetIsDotfile(t *testing.T) { + h, root, _ := raceHandler(t) + if err := os.Symlink(".hidden/x", filepath.Join(root, "rel")); err != nil { + t.Fatalf("symlink: %v", err) + } + rr := serve(t, h.read, http.MethodGet, "/files/read?path=/rel", nil) + if rr.Code != http.StatusBadRequest || rr.Body.String() != "dotfile access denied\n" { + t.Fatalf("status=%d body=%q, want 400 dotfile access denied", rr.Code, rr.Body.String()) + } +} + +func TestRooted_DeleteMissingEntries(t *testing.T) { + h, root, _ := raceHandler(t) + put(t, filepath.Join(root, "keep"), "x") + // A missing parent stays a generic 400, a missing final entry a 404, and a + // recursive delete of a missing entry succeeds (os.RemoveAll semantics). + cases := []struct { + target string + want int + }{ + {"/files/delete?path=/missing/x", http.StatusBadRequest}, + {"/files/delete?path=/nope", http.StatusNotFound}, + {"/files/delete?path=/nope&recursive=true", http.StatusNoContent}, + } + for _, tc := range cases { + rr := serve(t, h.del, http.MethodDelete, tc.target, nil) + if rr.Code != tc.want { + t.Fatalf("%s: status=%d body=%q, want %d", tc.target, rr.Code, rr.Body.String(), tc.want) + } + } +} + +func TestRooted_WriteToRootFails(t *testing.T) { + h, _, _ := raceHandler(t) + rr := serve(t, h.write, http.MethodPost, "/files/write?path=/", strings.NewReader("x")) + if rr.Code != http.StatusInternalServerError { + t.Fatalf("status=%d body=%q, want 500", rr.Code, rr.Body.String()) + } +} + +func TestRooted_ListReportsLinksAndPaths(t *testing.T) { + h, root, outside := raceHandler(t) + put(t, filepath.Join(root, "d", "f.txt"), "12345") + if err := os.Symlink(outside, filepath.Join(root, "d", "lnk")); err != nil { + t.Fatalf("symlink: %v", err) + } + rr := serve(t, h.list, http.MethodGet, "/files/list?path=/d", nil) + if rr.Code != http.StatusOK { + t.Fatalf("status=%d body=%q", rr.Code, rr.Body.String()) + } + var ents []Entry + if err := json.Unmarshal(rr.Body.Bytes(), &ents); err != nil { + t.Fatalf("decode: %v", err) + } + if len(ents) != 2 { + t.Fatalf("got %d entries: %+v", len(ents), ents) + } + // Sorted by name: f.txt then lnk. + if ents[0].Name != "f.txt" || ents[0].Path != "/d/f.txt" || ents[0].Size != 5 || ents[0].Dir || ents[0].ModTime == "" { + t.Fatalf("file entry=%+v", ents[0]) + } + if ents[1].Name != "lnk" || ents[1].Path != "/d/lnk" || ents[1].Dir || !strings.HasPrefix(ents[1].Mode, "L") { + t.Fatalf("link entry=%+v, want a non-dir entry with mode starting L", ents[1]) + } +} + +func TestRelComponents(t *testing.T) { + cases := []struct { + name string + root string + abs string + want []string + wantErr error + }{ + {"root", "/data", "/data", nil, nil}, + {"nested", "/data", "/data/a/b", []string{"a", "b"}, nil}, + {"outside", "/data", "/etc", nil, errPathOutOfRoot}, + {"sibling prefix", "/data", "/data2/x", nil, errPathOutOfRoot}, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + got, err := relComponents(tc.root, tc.abs) + if !errors.Is(err, tc.wantErr) { + t.Fatalf("err=%v, want %v", err, tc.wantErr) + } + if !slices.Equal(got, tc.want) { + t.Fatalf("got %v, want %v", got, tc.want) + } + }) + } +} + +func TestFileModeFromStat(t *testing.T) { + cases := []struct { + name string + in uint32 + want os.FileMode + }{ + {"regular", unix.S_IFREG | 0o644, 0o644}, + {"dir", unix.S_IFDIR | 0o755, os.ModeDir | 0o755}, + {"symlink", unix.S_IFLNK | 0o777, os.ModeSymlink | 0o777}, + {"fifo", unix.S_IFIFO | 0o600, os.ModeNamedPipe | 0o600}, + {"socket", unix.S_IFSOCK | 0o600, os.ModeSocket | 0o600}, + {"char device", unix.S_IFCHR | 0o600, os.ModeDevice | os.ModeCharDevice | 0o600}, + {"block device", unix.S_IFBLK | 0o600, os.ModeDevice | 0o600}, + {"special bits", unix.S_IFREG | unix.S_ISUID | unix.S_ISGID | unix.S_ISVTX | 0o755, os.ModeSetuid | os.ModeSetgid | os.ModeSticky | 0o755}, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + if got := fileModeFromStat(tc.in); got != tc.want { + t.Fatalf("got %v, want %v", got, tc.want) + } + }) + } +} + +func TestRemoveEntry(t *testing.T) { + dir := resolvedTempDir(t) + put(t, filepath.Join(dir, "f"), "x") + put(t, filepath.Join(dir, "full", "x"), "x") + if err := os.Mkdir(filepath.Join(dir, "empty"), 0o755); err != nil { + t.Fatalf("mkdir: %v", err) + } + fd, err := openat(unix.AT_FDCWD, dir, dirFlags, 0) + if err != nil { + t.Fatalf("open dir: %v", err) + } + defer closeFD(fd) + if err := removeEntry(fd, "f"); err != nil { + t.Fatalf("remove file: %v", err) + } + if err := removeEntry(fd, "empty"); err != nil { + t.Fatalf("remove empty dir: %v", err) + } + if err := removeEntry(fd, "full"); !errors.Is(err, unix.ENOTEMPTY) { + t.Fatalf("remove non-empty dir: got %v, want ENOTEMPTY", err) + } + if err := removeEntry(fd, "missing"); !errors.Is(err, os.ErrNotExist) { + t.Fatalf("remove missing: got %v, want ErrNotExist", err) + } +} + +func TestFileFromFDRejectsNegative(t *testing.T) { + if f, err := fileFromFD(-1, "x"); f != nil || !errors.Is(err, unix.EBADF) { + t.Fatalf("got f=%v err=%v, want nil and EBADF", f, err) + } +} diff --git a/agent/specs.md b/agent/specs.md index 49b211db6..5c806cd24 100644 --- a/agent/specs.md +++ b/agent/specs.md @@ -11,7 +11,7 @@ The agent is a per-pod HTTP/HTTPS sidecar that runs inside every game pod to exp - **Request authentication**: Verify incoming requests via mTLS (preferred) or shared-secret bearer token. - **Console & RCON**: Duplex WebSocket forwarding user commands to the game via RCON and echoing responses; supports Valve/Source, Telnet, WebSocket, BattlEye, Satisfactory, and Palworld protocols. -- **File I/O**: List, read, download, upload, write, mkdir, delete within the `/data` volume; reject path traversal and symlink escape. +- **File I/O**: List, read, download, upload, write, mkdir, delete within the `/data` volume; reject path traversal and any symlink traversal; every operation runs through directory descriptors rooted at the data directory, so it cannot be redirected by a concurrent filesystem change. File uploads and downloads have no router-level total-duration timeout. The remaining file operations retain a 30-second timeout. Authentication, body @@ -64,7 +64,7 @@ Per-package roles: - **`auth`**: Two modes: mTLS (agent listens TLS, requires client cert signed by `--tls-client-ca`), or shared-secret bearer token (fallback for dev). - **`caps`**: Unmarshals JSON capabilities blob from `GAMEPLANE_CAPABILITIES` env; exposes `Spec` with `Players`, `Quiesce`, `Lifecycle`, `Actions`, `Status`, `Mods`. - **`console`**: Accepts `{ kind: "cmd", body: "" }` JSON over WebSocket, runs it via RCON, replies with `{ kind: "out"|"err", body: "" }`. On an RCON failure the `err` body is the generic `upstream unavailable`; the detailed error is logged with every occurrence of the submitted command replaced by ``, since RCON clients embed the command in their errors and it may carry a secret. -- **`files`**: Walks the filesystem under `--data-root`, enforces path-traversal protection (no `..`, no symlinks escaping the root), handles multipart uploads. +- **`files`**: Walks the filesystem under `--data-root`, validates paths lexically, then performs every operation relative to a directory descriptor on the root with `openat(O_NOFOLLOW)` per component (no `..`, no symlink at any component), handles multipart uploads. - **`heartbeat`**: Runs a background goroutine that every 20 seconds patches `gameservers//status` with `status.agent.lastHeartbeat`, `status.agent.playersOnline`, `status.agent.playersMax`, `status.agent.gameVersion`, and resource usage via the pod's ServiceAccount. `gameVersion` is currently always patched `null` ("unknown") — the agent has no source for the game's actual running version; it must never be filled in with the game/template identifier (e.g. `minecraft-java`), which is a different value. - **`lifecycle`**: HTTP handler for the operator's `/lifecycle/stop` call; runs module-declared stop commands over RCON before the game process terminates. - **`logs`**: Tails a game log file (path from `--game-log-path`) over WebSocket; supports streaming from end (default, "live") or start ("backlog"). @@ -149,7 +149,7 @@ Resource usage env vars (set by the operator): | `/files/read` | GET | Read file inline (small files only); query param `path` | | `/files/download` | GET | Download file as attachment; query param `path` | | `/files/write` | POST | Overwrite or create file; query param `path`; body is raw file content | -| `/files/upload` | POST | Upload one or more files to a directory; query param `path`; body is `multipart/form-data` with `files[]`; returns HTTP 400 if destination path is a symlink that resolves outside the data root or cannot be resolved | +| `/files/upload` | POST | Upload one or more files to a directory; query param `path`; body is `multipart/form-data` with `files[]`; returns HTTP 400 if the destination directory or a destination file name is, or is reached through, a symlink | | `/files/mkdir` | POST | Create directory (recursive); query param `path` | | `/files/delete` | DELETE | Delete file or directory; query param `path`, optional `recursive` (boolean) | | `/logs/tail` | GET | Tail game log over WebSocket; query params `from` (enum: `start`, `end`, default `end`) and `tail` (optional positive integer, capped at 20000 lines and 4 MiB bytes; replays last N lines before following; no replay after rotation) | @@ -183,12 +183,12 @@ All endpoints on the `--addr` control mux, except `/healthz`, return `401 Unauth - **RCON is a lower-trust boundary**: The agent uses `netguard.IsAllowed()` for WebRcon dial operations (permissive, allows loopback and private addresses on the assumption game servers run inside the cluster), and `netguard.IsPublic()` (strict, permissive only for well-known registries) for mod-install downloads, assuming modules are less trusted than the operator. - **Gameaction validation is independent**: Both the API (stdin pod-attach) and the agent (RCON) call `gameaction.Resolve()` independently to validate action inputs (no control characters, 512-char cap, required-ness checks, etc.). Neither trusts the other. The players moderation endpoints (kick/ban reason) also call gameaction.CheckText, so moderation text follows the same character policy as actions. - **No persistent storage**: The agent has no database. GameServer status patches flow through the operator; all transient state (WebSocket streams, RCON sessions) is in-memory. -- **Path confinement via per-component validation**: The `files` package enforces path confinement through `resolve()` and `resolveForDelete()`, which validate against direct traversal (`..`, `/`, absolute paths), symlink escape (`resolve()` resolves both the target and the deepest existing ancestor; `resolveForDelete()` resolves only the parent so a symlink is deleted as a link), and dotfile access (any dot-prefixed path component is rejected, in the requested path and in its symlink-resolved target or deepest existing ancestor, so a plain-named symlink cannot reach a dotfile or dot-directory; `savePart` likewise rejects dot-prefixed upload filenames; `/files/list` excludes dot-prefixed entries from its output). The `mods` package uses `ConfinePath(rootDir, untrustedName)` as the authoritative guard for single-component mod paths (upload, download, removal, archive swap), and `ConfineRelPath(root, relPath)` for multi-component archive entry paths during extraction. ConfinePath returns a cleaned, absolute path guaranteed to be confined within rootDir (or raises an error if escape is attempted), validating against both direct traversal and symlink escape. The prior `safeName()` function remains in use as caller-side defense-in-depth (pre-filtering before ConfinePath), but ConfinePath is the authoritative guard within the function boundary where the archive operation occurs. +- **Path confinement via per-component validation**: The `files` package validates the requested path lexically in `resolve()` (direct traversal: `..`, `/`, absolute paths; any dot-prefixed component is rejected as a dotfile) and then performs every operation relative to a directory descriptor opened on the data root (`rooted_linux.go`). Each component is opened with `openat(O_NOFOLLOW)` (directories also `O_DIRECTORY`), so a symlink at any component of the path, ancestor or final, is refused with HTTP 400 (`dotfile access denied` when the link's target lies below a dot-prefixed component, otherwise `path escapes root (symbolic link in path)`). Reads, writes, creates, deletes and listings use the descriptors they obtained (`fstatat`, `mkdirat`, `unlinkat`, `renameat`, `ServeContent` on the opened fd) and never re-open a path string, so replacing a checked directory or file with a symlink between validation and use cannot move the operation outside the root; a delete removes a symlink as the link it is and a recursive delete never follows links. The data-root path itself is the one trusted, followed component (it may be a symlink). `savePart` likewise rejects dot-prefixed upload filenames; `/files/list` excludes dot-prefixed entries from its output and a delete refuses trees containing them. The `mods` package uses `ConfinePath(rootDir, untrustedName)` as the authoritative guard for single-component mod paths (upload, download, removal, archive swap), and `ConfineRelPath(root, relPath)` for multi-component archive entry paths during extraction. ConfinePath returns a cleaned, absolute path guaranteed to be confined within rootDir (or raises an error if escape is attempted), validating against both direct traversal and symlink escape. The prior `safeName()` function remains in use as caller-side defense-in-depth (pre-filtering before ConfinePath), but ConfinePath is the authoritative guard within the function boundary where the archive operation occurs. - **WebRcon dials through netguard**: `rcon.websocket.go`'s `ensureLocked()` method dials the Rust WebSocket using `netguard.IsAllowed()` dial policy for defense-in-depth, allowing loopback and private addresses (since game servers legitimately run inside the cluster) while blocking the ranges `netguard.IsAllowed` rejects (link-local, multicast, NAT64/6to4). - **WebRcon dial errors are redacted**: On dial failure, `ensureLocked()` strips the URL path (which carries the escaped password) from any `*url.Error` via `redactURLErr`. If the redacted message still contains the password in raw, path-escaped, or query-escaped form (`errorLeaksSecret`; passwords shorter than three characters are not checked), it returns a generic `connection failed` error without the original cause; otherwise it wraps the redacted error with `%w` so `errors.Is`/`errors.As` still work. - **Agent-created directories are 0755**: `files` (mkdir, upload and write ancestors) and `mods` (mods dir, archive extraction dirs) create directories 0o755, matching the 0o644 files, because the game container runs as a different uid and on templates without an fsGroup reaches the shared volume only through the "other" bits. The gosec G301 finding for these two files is scoped in `.golangci.yml`. - **Partial uploads never linger, and never destroy an existing file**: `files.savePart` writes to a temp file in the destination directory and renames it over the final name only once the copy succeeds. A save that fails after the temp file is opened — a source read error, an `io.ErrUnexpectedEOF` from a truncated multipart body, or an over-the-limit part — removes only the temp file, so a client abort mid-upload can't leave a half-written file where a later `/files/read` or `/files/download` would serve it as complete, and can't delete a file that already existed at that name. -- **Symlink confinement on upload**: `files.savePart` confines the upload destination the same way as other file operations: an existing symlink at the destination name must resolve inside the data root (after resolving both the symlink target and the root's own symlinks). If the symlink resolves outside root or is dangling, the upload is rejected with HTTP 400 (`errPathOutOfRoot`), leaving the symlink untouched. Regular (non-symlink) files are always overwritten by a successful upload. +- **Symlink confinement on upload**: `files.savePart` confines the upload destination the same way as other file operations: an existing symlink at the destination name (pointing inside the root, outside it, or dangling) is rejected with HTTP 400 (`errLinkTraversal`, which wraps `errPathOutOfRoot`), leaving the symlink and its target untouched. Regular (non-symlink) files are overwritten by a successful upload through an atomic `renameat` in the same directory descriptor. - **Extracted mod files stay world-readable**: `mods.moduleFileMode` is `0o644`, not a tighter `0o600`, because the mods volume is shared with the game container, which runs as whatever uid its image needs — a different uid than the agent's own. At `0o600` the game process couldn't read its own mods. This is the rationale behind the `.golangci.yml` gosec G302 exclusion scoped to `agent/internal/mods/mods.go`. - **Resource usage is in-pod**: The `usage` package reads from `/proc` or cgroups; no external metrics pipeline required. Cgroup mode is a fallback for older clusters; proc mode (default in production) requires the operator to set `ShareProcessNamespace: true`. - **Module capabilities drive behavior**: Every game-specific handler (players, quiesce, lifecycle, status, actions) reads its config from `--capabilities` (JSON unmarshaled into `caps.Spec`). New games require no agent code change. @@ -236,8 +236,8 @@ The agent and operator modules use `k8s.io/apimachinery`/`k8s.io/client-go` v0.3 - **mTLS + token auth**: All protected endpoints require either a valid mTLS client cert (signed by `--tls-client-ca`) or a bearer token (from `--api-token-file`). - **SSRF guard on mod downloads**: `netguard.IsPublic()` gates every mod download dial; private or loopback registries are rejected regardless of `allowedHosts`, which only narrows the host allowlist. - **Console-injection guard on RCON actions**: `gameaction.Resolve()` validates action inputs independently on the agent side; control characters, oversized inputs, and required-parameter validation prevent blind command injection. Kick/ban reasons go through the same guard (gameaction.CheckText) before being rendered into the module's .Reason template variable. -- **Path-traversal protection**: The `files` package rejects `..`, symlinks escaping `--data-root`, and dotfile access; all I/O is confined. -- **No half-written uploads, and no lost files on a failed write**: `files.write` and `files.savePart` write to a temp file in the destination directory first and rename it over the target only once the copy succeeds. A failure partway through (truncated body, read error, over-limit part, out of space) removes only the temp file — a pre-existing file at that path is never truncated or deleted by a failed write or upload. +- **Path-traversal protection**: The `files` package rejects `..`, any symlink component below `--data-root`, and dotfile access; all I/O goes through descriptors rooted at the data directory, so it is also safe against a path being swapped for a symlink after validation (race-oriented tests cover this). +- **No half-written uploads, and no lost files on a failed write**: `files.write` and `files.savePart` write to a temp file in the destination directory first and rename it over the target only once the copy succeeds. A failure partway through (truncated body, read error, over-limit part, out of space) removes only the temp file — a pre-existing file at that path is never truncated or deleted by a failed write or upload. The temp file is created, chmodded, fsynced and renamed through the same parent directory descriptor. - **Extracted mod files are 0o644, not 0o600**: `mods.moduleFileMode` grants group+world read so the game container — which runs under whatever uid its own image needs (not the agent's uid) — can read its shared mods volume; `FSGroup` alone doesn't help here since it only changes group ownership, not mode bits. The tradeoff is scoped: the `.golangci.yml` gosec G302 exclusion applies to `agent/internal/mods/mods.go` only. - **Low privilege within pod**: The agent is a sidecar container (not privileged, not root unless the game container is). It reads `/proc` only for the game process and the pause process; it cannot access other pods' data. - **No unauthenticated game-data exposure**: `/metrics` (its own separate, unauthenticated listener) is public, and `/healthz` is outside the auth middleware on the control mux (in mTLS mode the TLS handshake still requires a verified client certificate for it; in bearer-token fallback mode it answers without a token); all game data (`/files`, `/logs`, `/console`, `/players`) requires authentication. `/metrics` is deliberately never reachable through the mTLS control mux, so a Prometheus scraper is never handed the client cert that would also unlock those authenticated routes. diff --git a/docs/security.md b/docs/security.md index 659a68fc7..ffb1481a9 100644 --- a/docs/security.md +++ b/docs/security.md @@ -447,6 +447,35 @@ serve a `GameTemplate` whose image/command runs in your cluster — only point Gameplane at module sources you trust, and prefer signed, pinned installs for third-party games. Authoring details: [`module-authoring.md`](module-authoring.md). +## Agent file browser (path confinement) + +The agent's `/files/*` routes (list, read, download, write, upload, mkdir, +delete) operate on the game data volume and are reachable by anyone who can +use the dashboard file browser, while the game container (a different uid) +can create symlinks on the same volume. The agent therefore treats the data +directory contents as untrusted and does not trust a path it validated +earlier: + +- The requested path is validated lexically (no `..` escape, no dot-prefixed + component) and every operation then runs relative to a directory descriptor + opened on the data root, opening one component at a time with + `openat(O_NOFOLLOW)`. A symlink at any component is refused (HTTP 400); there + is no "follow it if it stays inside the root" case. +- Reads, writes, creates and deletes use the descriptors they obtained + (`fstatat`, `mkdirat`, `unlinkat`, `renameat`, serving from the opened file) + and never re-open an absolute path, so swapping a checked directory or file + for a symlink between validation and use cannot redirect the operation out of + the root. Writes are atomic: temp file in the parent descriptor, `fsync`, + `renameat` in the same directory. +- Recursive delete walks descriptors and unlinks links as links; it refuses + trees that contain dot-prefixed entries (agent state such as the mods + manifest). +- The data-root path itself is operator-configured and trusted (it may be a + symlink); only components below it are checked. + +The mods package confines its own paths separately (`ConfinePath`/`ConfineRelPath`) +and does not use this descriptor walk. + ## Runtime mod installs (agent) Separately from the module supply chain above, a running server can install