Owlibou/gnoma
2
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

refactor(security): consolidate TOCTOU-safe path canonicalization

Browse Source 
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.
This commit is contained in:
vikingowl
2026-05-20 01:50:38 +02:00
parent f6f8801040
commit 9853a522e6
4 changed files with 139 additions and 52 deletions
Download Patch File Download Diff File Expand all files Collapse all files
internal/engine/paths.go
+8 -31
View File
@@ -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.
internal/security/pathcanon.go
+50
View File
@@ -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
}
internal/security/pathcanon_test.go
+77
View File
@@ -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/<linked> 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")
}
}
internal/tool/fs/guard.go
+4 -21
View File
@@ -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)