diff --git a/allowedpaths/sandbox.go b/allowedpaths/sandbox.go index 9a3fd9c1c..30029d8f0 100644 --- a/allowedpaths/sandbox.go +++ b/allowedpaths/sandbox.go @@ -381,7 +381,10 @@ func (s *Sandbox) openWithSymlinkFallback(root *os.Root, relPath, absPath string // All operations are fd-relative through os.Root — no filesystem path is // re-resolved through the mutable namespace after initial validation. func (s *Sandbox) Access(path string, cwd string, mode uint32) error { - absPath := toAbs(path, cwd) + absPath, ok := s.resolveAbsPath(path, cwd, false) + if !ok { + return &os.PathError{Op: "access", Path: path, Err: os.ErrPermission} + } if s == nil { return &os.PathError{Op: "access", Path: path, Err: os.ErrPermission} @@ -425,6 +428,184 @@ func toAbs(path, cwd string) string { return filepath.Join(cwd, path) } +// containsDotDot reports whether any component is "..". +func containsDotDot(components []string) bool { + for _, c := range components { + if c == ".." { + return true + } + } + return false +} + +// splitComponents splits path into non-empty components, without collapsing +// "." or ".." — callers walk those explicitly so that ".." is applied +// against a resolved location rather than the raw string. path is +// normalized with filepath.FromSlash first so that "/"-separated input +// (which shell scripts may use even on Windows) splits the same way as +// native-separator input. +func splitComponents(path string) []string { + if path == "" { + return nil + } + path = filepath.FromSlash(path) + parts := strings.Split(path, string(filepath.Separator)) + out := make([]string, 0, len(parts)) + for _, p := range parts { + if p != "" { + out = append(out, p) + } + } + return out +} + +// resolveAbsPath resolves path (joined against cwd when relative) to an +// absolute path, walking one component at a time: ".." pops a level off +// the location resolved *so far*, and any component found to be a symlink +// through a configured sandbox root is followed before continuing, but only +// when a later ".." in the path could actually pop through it — this +// matches POSIX/kernel path-walk semantics for the case that matters while +// leaving every other symlink component untouched for the existing +// downstream resolution (os.Root's native escape detection, +// resolveFollowingSymlinks's cross-root fallback) to handle exactly as +// before, including their error messages. +// +// This is deliberately not a lexical filepath.Join/Clean. Join collapses +// ".." against the raw string before any symlink is examined, so for a +// path like "link/../file" where link resolves outside the sandbox, it +// discards "link" entirely and produces "/file" — a different target +// than the kernel would resolve (which follows link first, then applies +// ".." to *its* target). Walking component by component avoids that +// divergence by only ever applying ".." to what has actually been resolved. +// +// Components that lie outside every configured root are joined lexically: +// Lstat can't be performed on them without bypassing the sandbox, and +// resolve() rejects the resulting path downstream exactly as it does today. +// +// When preserveLast is true, the final path component is never resolved +// even if it is a symlink — this matches lstat/readlink semantics, where +// the operation targets the link itself rather than what it points to. +// +// It returns ok=false if the symlink chain exceeds maxSymlinkHops, mirroring +// resolveRootFollowingSymlinks's hop limit — callers must treat that as an +// outright resolution failure (not fall through to a partially-resolved +// path), since the underlying open/stat syscall would otherwise silently +// follow the one remaining unresolved symlink itself. +// +// This resolver is for read-only operations only. Write operations +// (Open with write flags, Truncate, TruncateToZeroIfAtLeast) use the plain +// lexical toAbs instead: writeopen's O_NOFOLLOW walk needs literal symlink +// component names intact to detect and reject symlink write targets, which +// eagerly resolving components here would erase. +func (s *Sandbox) resolveAbsPath(path, cwd string, preserveLast bool) (string, bool) { + if s == nil { + return toAbs(path, cwd), true + } + + var resolved string + var pending []string + if filepath.IsAbs(path) { + // Preserve the volume/drive (e.g. "C:" on Windows; empty on + // Unix) as the root instead of discarding it — otherwise an + // absolute Windows path loses its drive letter and no longer + // matches any configured root. + volume := filepath.VolumeName(path) + resolved = volume + string(filepath.Separator) + pending = splitComponents(path[len(volume):]) + } else { + resolved = cwd + pending = splitComponents(path) + } + + hops := 0 + for len(pending) > 0 { + c := pending[0] + pending = pending[1:] + + switch c { + case ".": + continue + case "..": + resolved = filepath.Dir(resolved) + continue + } + + isLast := len(pending) == 0 + if preserveLast && isLast { + resolved = filepath.Join(resolved, c) + continue + } + + candidate := filepath.Join(resolved, c) + if !containsDotDot(pending) { + // No later ".." can ever pop through whatever this component + // resolves to, so there's nothing for the kernel-semantics walk + // to get wrong here. Leave the component unresolved and let the + // existing downstream resolution (os.Root's own symlink + // following, or the cross-root fallback) handle it exactly as + // before this fix, including its error messages. + resolved = candidate + continue + } + ar, rel, ok := s.resolve(candidate) + if !ok { + resolved = candidate + continue + } + info, err := ar.root.Lstat(rel) + if err != nil { + // A later ".." depends on this component actually existing + // and being poppable. Silently continuing here — as the + // lexical fallback above does for components outside every + // root — would let the pending ".." land on the wrong + // target, unlike a real open/stat syscall which fails at + // this component instead of collapsing past it. Fail + // resolution outright rather than guess. + return "", false + } + if !isLast && !info.IsDir() && info.Mode()&fs.ModeSymlink == 0 { + // Same reasoning: the kernel requires every non-final + // component to be a directory (ENOTDIR otherwise); a later + // ".." must not be allowed to pop through one that isn't. + return "", false + } + if info.Mode()&fs.ModeSymlink == 0 { + resolved = candidate + continue + } + if hops >= maxSymlinkHops { + // Too many symlink follows. Returning a partially-resolved + // path here would leave one unfollowed symlink as the final + // joined component, which the eventual open/stat syscall + // would then follow on its own — silently defeating this + // limit. Fail outright instead, matching + // resolveRootFollowingSymlinks's hop-overflow behavior. + return "", false + } + target, err := ar.root.Readlink(rel) + if err != nil { + resolved = candidate + continue + } + hops++ + + targetAbs := target + if !filepath.IsAbs(targetAbs) { + targetAbs = filepath.Join(resolved, targetAbs) + } + // In containers, host symlinks use host-absolute paths (e.g. + // /var/log/pods/...) that don't include the mount prefix. Prepend + // it so the path matches our roots, unless it's already there. + if s.hostPrefix != "" && !strings.HasPrefix(targetAbs, s.hostPrefix+string(filepath.Separator)) { + targetAbs = filepath.Join(s.hostPrefix, targetAbs) + } + targetVolume := filepath.VolumeName(targetAbs) + resolved = targetVolume + string(filepath.Separator) + pending = append(splitComponents(targetAbs[len(targetVolume):]), pending...) + } + return filepath.Clean(resolved), true +} + // IsDevNull reports whether path refers to the platform's null device. func IsDevNull(path string) bool { if path == "/dev/null" { @@ -480,11 +661,24 @@ func (s *Sandbox) Open(path string, cwd string, flag int, perm os.FileMode) (io. return nil, &os.PathError{Op: "open", Path: path, Err: os.ErrPermission} } - absPath := toAbs(path, cwd) + var absPath string + var ok bool + if flag&writeOpenFlags != 0 { + // Write opens must not have any symlink component — intermediate or + // final — resolved away: writeopen's O_NOFOLLOW walk (see + // resolveWriteTarget below) needs the literal component names to + // correctly detect and reject symlink write targets. So writes stay + // on the plain lexical join rather than the symlink-aware resolver. + absPath = toAbs(path, cwd) + } else { + absPath, ok = s.resolveAbsPath(path, cwd, false) + if !ok { + return nil, &os.PathError{Op: "open", Path: path, Err: os.ErrPermission} + } + } var ar *root var relPath string - var ok bool if flag&writeOpenFlags == 0 { ar, relPath, ok = s.resolve(absPath) } else { @@ -566,6 +760,8 @@ func (s *Sandbox) Truncate(path string, cwd string, size int64, create bool) err return &os.PathError{Op: "truncate", Path: path, Err: syscall.EINVAL} } + // Writes must not have any symlink component resolved away; see the + // comment in Open above. absPath := toAbs(path, cwd) ar, relPath, ok := s.resolveWriteTarget(absPath) @@ -637,6 +833,8 @@ func (s *Sandbox) TruncateToZeroIfAtLeast(path string, cwd string, minSize int64 return 0, false, &os.PathError{Op: "truncate", Path: path, Err: syscall.EINVAL} } + // Writes must not have any symlink component resolved away; see the + // comment in Open above. absPath := toAbs(path, cwd) ar, relPath, ok := s.resolveWriteTarget(absPath) @@ -701,7 +899,10 @@ func (s *Sandbox) ReadDirForGlob(path string, cwd string) ([]fs.DirEntry, error) // maxEntries+1 to cap the read at the OS level; if the directory has more // entries than the limit an error is returned. func (s *Sandbox) readDirN(path string, cwd string, maxEntries int) ([]fs.DirEntry, error) { - absPath := toAbs(path, cwd) + absPath, ok := s.resolveAbsPath(path, cwd, false) + if !ok { + return nil, &os.PathError{Op: "readdir", Path: path, Err: os.ErrPermission} + } ar, relPath, ok := s.resolve(absPath) if !ok { @@ -749,7 +950,10 @@ func (s *Sandbox) readDirN(path string, cwd string, maxEntries int) ([]fs.DirEnt // via ReadDir(n). The caller must close the returned handle when done. // Returns fs.ReadDirFile to expose only read-only directory methods. func (s *Sandbox) OpenDir(path string, cwd string) (fs.ReadDirFile, error) { - absPath := toAbs(path, cwd) + absPath, ok := s.resolveAbsPath(path, cwd, false) + if !ok { + return nil, &os.PathError{Op: "opendir", Path: path, Err: os.ErrPermission} + } ar, relPath, ok := s.resolve(absPath) if !ok { @@ -767,7 +971,10 @@ func (s *Sandbox) OpenDir(path string, cwd string) (fs.ReadDirFile, error) { // entry. More efficient than reading all entries when only emptiness // needs to be determined. func (s *Sandbox) IsDirEmpty(path string, cwd string) (bool, error) { - absPath := toAbs(path, cwd) + absPath, ok := s.resolveAbsPath(path, cwd, false) + if !ok { + return false, &os.PathError{Op: "readdir", Path: path, Err: os.ErrPermission} + } ar, relPath, ok := s.resolve(absPath) if !ok { @@ -796,7 +1003,10 @@ func (s *Sandbox) IsDirEmpty(path string, cwd string) (bool, error) { // pages may overlap or miss entries. This is an acceptable tradeoff to achieve // O(n) memory regardless of offset value, where n = min(maxRead, entries). func (s *Sandbox) ReadDirLimited(path string, cwd string, offset, maxRead int) ([]fs.DirEntry, bool, error) { - absPath := toAbs(path, cwd) + absPath, ok := s.resolveAbsPath(path, cwd, false) + if !ok { + return nil, false, &os.PathError{Op: "readdir", Path: path, Err: os.ErrPermission} + } ar, relPath, ok := s.resolve(absPath) if !ok { return nil, false, &os.PathError{Op: "readdir", Path: path, Err: os.ErrPermission} @@ -889,7 +1099,10 @@ func (s *Sandbox) Stat(path string, cwd string) (fs.FileInfo, error) { return os.Stat(os.DevNull) } - absPath := toAbs(path, cwd) + absPath, ok := s.resolveAbsPath(path, cwd, false) + if !ok { + return nil, &os.PathError{Op: "stat", Path: path, Err: os.ErrPermission} + } ar, relPath, ok := s.resolve(absPath) if !ok { @@ -923,7 +1136,10 @@ func (s *Sandbox) Lstat(path string, cwd string) (fs.FileInfo, error) { return os.Stat(os.DevNull) } - absPath := toAbs(path, cwd) + absPath, ok := s.resolveAbsPath(path, cwd, true) + if !ok { + return nil, &os.PathError{Op: "lstat", Path: path, Err: os.ErrPermission} + } ar, relPath, ok := s.resolve(absPath) if !ok { @@ -950,7 +1166,10 @@ func (s *Sandbox) Lstat(path string, cwd string) (fs.FileInfo, error) { // Readlink returns the destination of a symbolic link within the sandbox. func (s *Sandbox) Readlink(path string, cwd string) (string, error) { - absPath := toAbs(path, cwd) + absPath, ok := s.resolveAbsPath(path, cwd, true) + if !ok { + return "", &os.PathError{Op: "readlink", Path: path, Err: os.ErrPermission} + } ar, relPath, ok := s.resolve(absPath) if !ok { diff --git a/allowedpaths/sandbox_unix_test.go b/allowedpaths/sandbox_unix_test.go index bf934f277..c5ee84af7 100644 --- a/allowedpaths/sandbox_unix_test.go +++ b/allowedpaths/sandbox_unix_test.go @@ -659,6 +659,124 @@ func TestSandboxSymlinkWriteDirectoryComponentReportsActionableError(t *testing. assert.Equal(t, "target", string(got)) } +// TestDotDotAfterSymlinkFollowsKernelSemantics reproduces the scenario from +// issue #561: "link/../sibling.txt", where link is a directory symlink whose +// target's parent differs from link's own literal parent. A lexical +// filepath.Join/Clean collapses ".." against the raw string before the +// symlink is examined, landing back in link's literal parent directory +// ("sub") and reading the wrong file. The kernel (and this resolver) instead +// follows the symlink first, then applies ".." to the resolved target's +// parent ("other"). +func TestDotDotAfterSymlinkFollowsKernelSemantics(t *testing.T) { + dir := t.TempDir() + sub := filepath.Join(dir, "sub") + other := filepath.Join(dir, "other") + real := filepath.Join(other, "real") + require.NoError(t, os.MkdirAll(sub, 0o755)) + require.NoError(t, os.MkdirAll(real, 0o755)) + require.NoError(t, os.WriteFile(filepath.Join(sub, "sibling.txt"), []byte("wrong"), 0o644)) + require.NoError(t, os.WriteFile(filepath.Join(other, "sibling.txt"), []byte("correct"), 0o644)) + require.NoError(t, os.Symlink(real, filepath.Join(sub, "link"))) + + sb, _, err := New([]string{dir}) + require.NoError(t, err) + defer sb.Close() + + // filepath.Join would call Clean and collapse ".." before it ever + // reaches the resolver, defeating the point of this test — build the + // raw path string instead. + rawPath := "sub" + string(filepath.Separator) + "link" + string(filepath.Separator) + ".." + string(filepath.Separator) + "sibling.txt" + f, err := sb.Open(rawPath, dir, os.O_RDONLY, 0) + require.NoError(t, err) + defer f.Close() + + buf := make([]byte, 64) + n, _ := f.Read(buf) + assert.Equal(t, "correct", string(buf[:n])) +} + +// TestDotDotAfterSymlinkEscapeDenied verifies that when a symlink resolves +// outside every configured root, a trailing ".." that would otherwise land +// back inside the sandbox under lexical collapsing is still denied: the +// resolver follows the symlink to its actual (outside) location first, so +// the ".." is applied there and the final path is rejected as +// out-of-sandbox, rather than being silently redirected to an unrelated +// in-sandbox file. +func TestDotDotAfterSymlinkEscapeDenied(t *testing.T) { + dir := t.TempDir() + outside := t.TempDir() + require.NoError(t, os.Symlink(outside, filepath.Join(dir, "link"))) + // A file that a buggy lexical resolver would wrongly read via + // filepath.Join(dir, "link", "..", "file.txt") == dir/file.txt. + require.NoError(t, os.WriteFile(filepath.Join(dir, "file.txt"), []byte("wrong"), 0o644)) + + sb, _, err := New([]string{dir}) + require.NoError(t, err) + defer sb.Close() + + rawPath := "link" + string(filepath.Separator) + ".." + string(filepath.Separator) + "file.txt" + _, err = sb.Open(rawPath, dir, os.O_RDONLY, 0) + require.Error(t, err) + assert.ErrorIs(t, err, os.ErrPermission) +} + +// TestDotDotAfterMissingComponentDenied verifies that a nonexistent +// intermediate component preceding a symlink component followed by ".." is +// not silently skipped over: a real open/stat syscall fails at the missing +// component (ENOENT) rather than letting ".." collapse past it and land on +// an unrelated existing file. +func TestDotDotAfterMissingComponentDenied(t *testing.T) { + dir := t.TempDir() + sub := filepath.Join(dir, "sub") + other := filepath.Join(dir, "other") + real := filepath.Join(other, "real") + require.NoError(t, os.MkdirAll(sub, 0o755)) + require.NoError(t, os.MkdirAll(real, 0o755)) + require.NoError(t, os.Symlink(real, filepath.Join(sub, "link"))) + // A buggy resolver treats "missing" as if it existed and lets the two + // trailing ".." pop past it and "real", landing on other/file.txt. + require.NoError(t, os.WriteFile(filepath.Join(other, "file.txt"), []byte("wrong"), 0o644)) + + sb, _, err := New([]string{dir}) + require.NoError(t, err) + defer sb.Close() + + // "missing" does not exist under link's target. + rawPath := "sub" + string(filepath.Separator) + "link" + string(filepath.Separator) + "missing" + + string(filepath.Separator) + ".." + string(filepath.Separator) + ".." + string(filepath.Separator) + "file.txt" + _, err = sb.Open(rawPath, dir, os.O_RDONLY, 0) + require.Error(t, err) +} + +// TestDotDotAfterNonDirectoryComponentDenied verifies that a regular file +// used as an intermediate ("non-final") path component before a trailing +// ".." is rejected rather than silently treated as a traversable directory: +// a real kernel walk fails with ENOTDIR here instead of letting ".." pop +// past it. +func TestDotDotAfterNonDirectoryComponentDenied(t *testing.T) { + dir := t.TempDir() + sub := filepath.Join(dir, "sub") + other := filepath.Join(dir, "other") + real := filepath.Join(other, "real") + require.NoError(t, os.MkdirAll(sub, 0o755)) + require.NoError(t, os.MkdirAll(real, 0o755)) + require.NoError(t, os.Symlink(real, filepath.Join(sub, "link"))) + require.NoError(t, os.WriteFile(filepath.Join(real, "notadir"), []byte("file"), 0o644)) + // A buggy resolver treats "notadir" as a traversable directory and + // lets the two trailing ".." pop past it and "real", landing on + // other/file.txt. + require.NoError(t, os.WriteFile(filepath.Join(other, "file.txt"), []byte("wrong"), 0o644)) + + sb, _, err := New([]string{dir}) + require.NoError(t, err) + defer sb.Close() + + rawPath := "sub" + string(filepath.Separator) + "link" + string(filepath.Separator) + "notadir" + + string(filepath.Separator) + ".." + string(filepath.Separator) + ".." + string(filepath.Separator) + "file.txt" + _, err = sb.Open(rawPath, dir, os.O_RDONLY, 0) + require.Error(t, err) +} + // --- Cross-root symlink tests --- // TestCrossRootSymlinkOpen verifies that a symlink in one allowed root diff --git a/analysis/symbols_allowedpaths.go b/analysis/symbols_allowedpaths.go index 74ebe0a8f..399d00644 100644 --- a/analysis/symbols_allowedpaths.go +++ b/analysis/symbols_allowedpaths.go @@ -56,10 +56,12 @@ var allowedpathsAllowedSymbols = []string{ "path/filepath.Clean", // 🟢 normalizes a path; pure function, no I/O. "path/filepath.Dir", // 🟢 returns directory portion of a path; pure function, no I/O. "path/filepath.EvalSymlinks", // 🟠 resolves symlinks via os.Lstat; the sandbox uses this at setup time to record canonical root paths so builtins like `pwd -P` can reflect the symlink resolution that os.Root has implicitly followed. + "path/filepath.FromSlash", // 🟢 converts '/' to OS separator without otherwise normalising; pure function, no I/O. "path/filepath.IsAbs", // 🟢 checks if path is absolute; pure function, no I/O. "path/filepath.Join", // 🟢 joins path elements; pure function, no I/O. "path/filepath.Rel", // 🟢 returns relative path; pure path computation. "path/filepath.Separator", // 🟢 OS path separator constant; pure constant. + "path/filepath.VolumeName", // 🟢 returns the leading volume/drive name (e.g. "C:"); pure string computation, no I/O. "slices.SortFunc", // 🟢 sorts a slice with a comparison function; pure function, no I/O. "sync.Once", // 🟢 ensures one-time execution; used to close file descriptors at most once. "strings.Compare", // 🟢 compares two strings lexicographically; pure function, no I/O.