From 9853a522e6f85703570d34df81d2de80eda38054 Mon Sep 17 00:00:00 2001 From: vikingowl <26+vikingowl@noreply.somegit.dev> Date: Wed, 20 May 2026 01:50:38 +0200 Subject: [PATCH] refactor(security): consolidate TOCTOU-safe path canonicalization MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 3c87527 added engine/paths.go:resolveCanonical, duplicating the ancestor-walk + EvalSymlinks algorithm that already lived in fs/guard.go:ResolveWrite. Two implementations of the same TOCTOU defense is exactly the wrong shape for security code — a bug fix in one would silently miss the other. Extracts the shared algorithm to security.CanonicalizePath. Both call sites become thin wrappers that pre-anchor relative paths against the appropriate root (cwd for engine, workspace root for guard). The "hit-root" defensive branch in engine's version (commented "highly unlikely") is tightened to match guard's error behavior. Adds focused unit tests for the helper covering existing path, non-existent leaf, non-existent mid-component, symlinked ancestor, and relative-path rejection. --- internal/engine/paths.go | 39 +++------------ internal/security/pathcanon.go | 50 +++++++++++++++++++ internal/security/pathcanon_test.go | 77 +++++++++++++++++++++++++++++ internal/tool/fs/guard.go | 25 ++-------- 4 files changed, 139 insertions(+), 52 deletions(-) create mode 100644 internal/security/pathcanon.go create mode 100644 internal/security/pathcanon_test.go diff --git a/internal/engine/paths.go b/internal/engine/paths.go index 8992186..37a6e70 100644 --- a/internal/engine/paths.go +++ b/internal/engine/paths.go @@ -8,12 +8,14 @@ import ( "strings" "somegit.dev/Owlibou/gnoma/internal/message" + "somegit.dev/Owlibou/gnoma/internal/security" "somegit.dev/Owlibou/gnoma/internal/tool" ) // isUnderAllowedPaths reports whether target is equal to or a descendant of any -// path in allowed. Both sides are cleaned before comparison. Returns false when -// allowed is empty. +// path in allowed. Both sides are canonicalized (symlink-evaluated, with the +// non-existent tail preserved) before comparison. Returns false when allowed +// is empty. // // The trailing-separator check prevents "/tmp" from matching "/tmpx/foo". func isUnderAllowedPaths(target string, allowed []string) bool { @@ -39,40 +41,15 @@ func isUnderAllowedPaths(target string, allowed []string) bool { return false } -// resolveCanonical returns the absolute, symlink-evaluated path. -// If the path doesn't exist, it resolves the deepest existing ancestor and -// appends the remaining tail. +// resolveCanonical absolutises against the process cwd and delegates the +// symlink-aware ancestor walk to security.CanonicalizePath. Kept as a thin +// wrapper so callers in this package can pass relative paths. func resolveCanonical(path string) (string, error) { abs, err := filepath.Abs(path) if err != nil { return "", err } - - ancestor := abs - var tail []string - for { - if _, err := os.Lstat(ancestor); err == nil { - break - } - parent := filepath.Dir(ancestor) - if parent == ancestor { - // Hit root, nothing exists? highly unlikely for Abs() but handle it. - break - } - tail = append([]string{filepath.Base(ancestor)}, tail...) - ancestor = parent - } - - canonicalAncestor, err := filepath.EvalSymlinks(ancestor) - if err != nil { - return "", err - } - - resolved := canonicalAncestor - if len(tail) > 0 { - resolved = filepath.Join(append([]string{canonicalAncestor}, tail...)...) - } - return filepath.Clean(resolved), nil + return security.CanonicalizePath(abs) } // checkPathRestriction enforces AllowedPaths on a single tool call. diff --git a/internal/security/pathcanon.go b/internal/security/pathcanon.go new file mode 100644 index 0000000..35b8ca9 --- /dev/null +++ b/internal/security/pathcanon.go @@ -0,0 +1,50 @@ +package security + +import ( + "fmt" + "os" + "path/filepath" +) + +// CanonicalizePath resolves absPath to its absolute, symlink-evaluated form, +// tolerating non-existent leaf and intermediate components. +// +// The algorithm walks back from absPath toward "/" until it finds a path that +// exists (via Lstat), runs filepath.EvalSymlinks on that ancestor, then +// rejoins the non-existent tail. This defends against the TOCTOU sandbox +// escape "leaf does not exist yet, so EvalSymlinks errors → caller skips the +// symlink check → write proceeds outside the workspace through a symlinked +// parent." +// +// absPath must be absolute; callers pre-anchor relative paths against the +// appropriate root (process cwd for general use, workspace root for +// sandboxed tools). Returns an error if no existing ancestor is reachable +// (would imply a broken filesystem; "/" always Lstats fine on a sane host). +func CanonicalizePath(absPath string) (string, error) { + if !filepath.IsAbs(absPath) { + return "", fmt.Errorf("canonicalize: %q is not absolute", absPath) + } + + ancestor := filepath.Clean(absPath) + tail := "" + for { + if _, err := os.Lstat(ancestor); err == nil { + break + } + parent := filepath.Dir(ancestor) + if parent == ancestor { + return "", fmt.Errorf("canonicalize %q: no existing ancestor", absPath) + } + tail = filepath.Join(filepath.Base(ancestor), tail) + ancestor = parent + } + + canonicalAncestor, err := filepath.EvalSymlinks(ancestor) + if err != nil { + return "", fmt.Errorf("canonicalize ancestor of %q: %w", absPath, err) + } + if tail == "" { + return filepath.Clean(canonicalAncestor), nil + } + return filepath.Clean(filepath.Join(canonicalAncestor, tail)), nil +} diff --git a/internal/security/pathcanon_test.go b/internal/security/pathcanon_test.go new file mode 100644 index 0000000..e837601 --- /dev/null +++ b/internal/security/pathcanon_test.go @@ -0,0 +1,77 @@ +package security + +import ( + "os" + "path/filepath" + "testing" +) + +func TestCanonicalizePath_ExistingPath(t *testing.T) { + dir := t.TempDir() + got, err := CanonicalizePath(dir) + if err != nil { + t.Fatalf("CanonicalizePath: %v", err) + } + want, _ := filepath.EvalSymlinks(dir) + if got != filepath.Clean(want) { + t.Errorf("got %q, want %q", got, want) + } +} + +func TestCanonicalizePath_NonExistentLeaf(t *testing.T) { + dir := t.TempDir() + leaf := filepath.Join(dir, "does-not-exist.txt") + got, err := CanonicalizePath(leaf) + if err != nil { + t.Fatalf("CanonicalizePath: %v", err) + } + canonicalDir, _ := filepath.EvalSymlinks(dir) + want := filepath.Join(canonicalDir, "does-not-exist.txt") + if got != want { + t.Errorf("got %q, want %q", got, want) + } +} + +func TestCanonicalizePath_NonExistentMidComponent(t *testing.T) { + dir := t.TempDir() + deep := filepath.Join(dir, "missing-mid", "and-leaf.txt") + got, err := CanonicalizePath(deep) + if err != nil { + t.Fatalf("CanonicalizePath: %v", err) + } + canonicalDir, _ := filepath.EvalSymlinks(dir) + want := filepath.Join(canonicalDir, "missing-mid", "and-leaf.txt") + if got != want { + t.Errorf("got %q, want %q", got, want) + } +} + +func TestCanonicalizePath_ResolvesSymlinkAncestor(t *testing.T) { + // real/ where /real is a symlink to a sibling tempdir; the + // canonical form must point through the resolved target. + parent := t.TempDir() + target := filepath.Join(parent, "target") + link := filepath.Join(parent, "link") + if err := os.Mkdir(target, 0o700); err != nil { + t.Fatal(err) + } + if err := os.Symlink(target, link); err != nil { + t.Fatal(err) + } + + got, err := CanonicalizePath(filepath.Join(link, "new-file.txt")) + if err != nil { + t.Fatalf("CanonicalizePath: %v", err) + } + canonicalTarget, _ := filepath.EvalSymlinks(target) + want := filepath.Join(canonicalTarget, "new-file.txt") + if got != want { + t.Errorf("got %q, want %q (symlinked parent should be resolved)", got, want) + } +} + +func TestCanonicalizePath_RejectsRelativePath(t *testing.T) { + if _, err := CanonicalizePath("relative/path"); err == nil { + t.Error("expected error for relative path, got nil") + } +} diff --git a/internal/tool/fs/guard.go b/internal/tool/fs/guard.go index 6afdbc7..bac14c4 100644 --- a/internal/tool/fs/guard.go +++ b/internal/tool/fs/guard.go @@ -6,6 +6,8 @@ import ( "os" "path/filepath" "strings" + + "somegit.dev/Owlibou/gnoma/internal/security" ) var ErrOutsideWorkspace = errors.New("path outside workspace") @@ -67,28 +69,9 @@ func (g *Guard) ResolveWrite(path string) (string, error) { if err != nil { return "", err } - - ancestor := abs - tail := "" - for { - if _, err := os.Lstat(ancestor); err == nil { - break - } - parent := filepath.Dir(ancestor) - if parent == ancestor { - return "", fmt.Errorf("resolve %q: no existing ancestor", path) - } - tail = filepath.Join(filepath.Base(ancestor), tail) - ancestor = parent - } - - canonicalAncestor, err := filepath.EvalSymlinks(ancestor) + resolved, err := security.CanonicalizePath(abs) if err != nil { - return "", fmt.Errorf("resolve ancestor of %q: %w", path, err) - } - resolved := canonicalAncestor - if tail != "" { - resolved = filepath.Join(canonicalAncestor, tail) + return "", fmt.Errorf("resolve %q: %w", path, err) } if !g.contains(resolved) { return "", fmt.Errorf("%w: %s", ErrOutsideWorkspace, path)