fix(security): workspace sandbox avoid time-of-check/time-of-use (TOCTOU) races (#464)

* chore: Update default host bindings from 0.0.0.0 to 127.0.0.1 for various services and examples. * config: Update default host bindings to 0.0.0.0 for improved Docker accessibility and add related documentation. * refactor: reimplement filesystem tools with `os.OpenRoot` for enhanced security and simplified path validation. * chore: revert other PR content from this branch * docs: Update Chinese README. * docs: Update Chinese README. * docs: Update Chinese README. * refactor: Reorder filesystem helper functions, extract directory entry formatting logic, and enhance `WriteFileTool`'s result message. * feat: Enhance `mkdirAllInRoot` to prevent creating directories over existing files and add tests for directory creation functionality. * Refactor filesystem tools to use a `fileReadWriter` interface for both host and sandboxed I/O, improving atomic writes and error handling. * refactor: unify filesystem read/write operations with atomic write guarantees and clearer naming. * refactor: rename `appendFileWithRW` function to `appendFile` * refactor: unify filesystem access by introducing a `fileSystem` interface and updating tools to use it directly, removing `os.Root` dependency from `sandboxFs`. * chore: run make fmt * fix: `validatePath` now returns an error when the workspace is empty.
2026-06-12 18:08:54 +00:00 · 2026-02-23 17:09:53 +08:00
parent 4cc8b90da9
commit 19c698356c
4 changed files with 630 additions and 109 deletions
@@ -3,15 +3,17 @@ package tools
 import (
 	"context"
 	"fmt"
+	"io/fs"
 	"os"
 	"path/filepath"
 	"strings"
+	"time"
 )

 // validatePath ensures the given path is within the workspace if restrict is true.
 func validatePath(path, workspace string, restrict bool) (string, error) {
 	if workspace == "" {
-		return path, nil
+		return path, fmt.Errorf("workspace is not defined")
 	}

 	absWorkspace, err := filepath.Abs(workspace)
@@ -76,16 +78,21 @@ func resolveExistingAncestor(path string) (string, error) {

 func isWithinWorkspace(candidate, workspace string) bool {
 	rel, err := filepath.Rel(filepath.Clean(workspace), filepath.Clean(candidate))
-	return err == nil && rel != ".." && !strings.HasPrefix(rel, ".."+string(os.PathSeparator))
+	return err == nil && filepath.IsLocal(rel)
 }

 type ReadFileTool struct {
-	workspace string
-	restrict  bool
+	fs fileSystem
 }

 func NewReadFileTool(workspace string, restrict bool) *ReadFileTool {
-	return &ReadFileTool{workspace: workspace, restrict: restrict}
+	var fs fileSystem
+	if restrict {
+		fs = &sandboxFs{workspace: workspace}
+	} else {
+		fs = &hostFs{}
+	}
+	return &ReadFileTool{fs: fs}
 }

 func (t *ReadFileTool) Name() string {
@@ -115,26 +122,25 @@ func (t *ReadFileTool) Execute(ctx context.Context, args map[string]any) *ToolRe
 		return ErrorResult("path is required")
 	}

-	resolvedPath, err := validatePath(path, t.workspace, t.restrict)
+	content, err := t.fs.ReadFile(path)
 	if err != nil {
 		return ErrorResult(err.Error())
 	}
-
-	content, err := os.ReadFile(resolvedPath)
-	if err != nil {
-		return ErrorResult(fmt.Sprintf("failed to read file: %v", err))
-	}
-
 	return NewToolResult(string(content))
 }

 type WriteFileTool struct {
-	workspace string
-	restrict  bool
+	fs fileSystem
 }

 func NewWriteFileTool(workspace string, restrict bool) *WriteFileTool {
-	return &WriteFileTool{workspace: workspace, restrict: restrict}
+	var fs fileSystem
+	if restrict {
+		fs = &sandboxFs{workspace: workspace}
+	} else {
+		fs = &hostFs{}
+	}
+	return &WriteFileTool{fs: fs}
 }

 func (t *WriteFileTool) Name() string {
@@ -173,30 +179,25 @@ func (t *WriteFileTool) Execute(ctx context.Context, args map[string]any) *ToolR
 		return ErrorResult("content is required")
 	}

-	resolvedPath, err := validatePath(path, t.workspace, t.restrict)
-	if err != nil {
+	if err := t.fs.WriteFile(path, []byte(content)); err != nil {
 		return ErrorResult(err.Error())
 	}

-	dir := filepath.Dir(resolvedPath)
-	if err := os.MkdirAll(dir, 0o755); err != nil {
-		return ErrorResult(fmt.Sprintf("failed to create directory: %v", err))
-	}
-
-	if err := os.WriteFile(resolvedPath, []byte(content), 0o644); err != nil {
-		return ErrorResult(fmt.Sprintf("failed to write file: %v", err))
-	}
-
 	return SilentResult(fmt.Sprintf("File written: %s", path))
 }

 type ListDirTool struct {
-	workspace string
-	restrict  bool
+	fs fileSystem
 }

 func NewListDirTool(workspace string, restrict bool) *ListDirTool {
-	return &ListDirTool{workspace: workspace, restrict: restrict}
+	var fs fileSystem
+	if restrict {
+		fs = &sandboxFs{workspace: workspace}
+	} else {
+		fs = &hostFs{}
+	}
+	return &ListDirTool{fs: fs}
 }

 func (t *ListDirTool) Name() string {
@@ -226,24 +227,179 @@ func (t *ListDirTool) Execute(ctx context.Context, args map[string]any) *ToolRes
 		path = "."
 	}

-	resolvedPath, err := validatePath(path, t.workspace, t.restrict)
-	if err != nil {
-		return ErrorResult(err.Error())
-	}
-
-	entries, err := os.ReadDir(resolvedPath)
+	entries, err := t.fs.ReadDir(path)
 	if err != nil {
 		return ErrorResult(fmt.Sprintf("failed to read directory: %v", err))
 	}
+	return formatDirEntries(entries)
+}

-	result := ""
+func formatDirEntries(entries []os.DirEntry) *ToolResult {
+	var result strings.Builder
 	for _, entry := range entries {
 		if entry.IsDir() {
-			result += "DIR:  " + entry.Name() + "\n"
+			result.WriteString("DIR:  " + entry.Name() + "\n")
 		} else {
-			result += "FILE: " + entry.Name() + "\n"
+			result.WriteString("FILE: " + entry.Name() + "\n")
+		}
+	}
+	return NewToolResult(result.String())
+}
+
+// fileSystem abstracts reading, writing, and listing files, allowing both
+// unrestricted (host filesystem) and sandbox (os.Root) implementations to share the same polymorphic interface.
+type fileSystem interface {
+	ReadFile(path string) ([]byte, error)
+	WriteFile(path string, data []byte) error
+	ReadDir(path string) ([]os.DirEntry, error)
+}
+
+// hostFs is an unrestricted fileReadWriter that operates directly on the host filesystem.
+type hostFs struct{}
+
+func (h *hostFs) ReadFile(path string) ([]byte, error) {
+	content, err := os.ReadFile(path)
+	if err != nil {
+		if os.IsNotExist(err) {
+			return nil, fmt.Errorf("failed to read file: file not found: %w", err)
+		}
+		if os.IsPermission(err) {
+			return nil, fmt.Errorf("failed to read file: access denied: %w", err)
+		}
+		return nil, fmt.Errorf("failed to read file: %w", err)
+	}
+	return content, nil
+}
+
+func (h *hostFs) ReadDir(path string) ([]os.DirEntry, error) {
+	return os.ReadDir(path)
+}
+
+func (h *hostFs) WriteFile(path string, data []byte) error {
+	dir := filepath.Dir(path)
+	if err := os.MkdirAll(dir, 0o755); err != nil {
+		return fmt.Errorf("failed to create parent directories: %w", err)
+	}
+
+	// We use a "write-then-rename" pattern here to ensure an atomic write.
+	// This prevents the target file from being left in a truncated or partial state
+	// if the operation is interrupted, as the rename operation is atomic on Linux.
+	tmpPath := fmt.Sprintf("%s.%d.tmp", path, time.Now().UnixNano())
+	if err := os.WriteFile(tmpPath, data, 0o644); err != nil {
+		os.Remove(tmpPath) // Ensure cleanup of partial/empty temp file
+		return fmt.Errorf("failed to write temp file: %w", err)
+	}
+
+	if err := os.Rename(tmpPath, path); err != nil {
+		os.Remove(tmpPath)
+		return fmt.Errorf("failed to replace original file: %w", err)
+	}
+	return nil
+}
+
+// sandboxFs is a sandboxed fileSystem that operates within a strictly defined workspace using os.Root.
+type sandboxFs struct {
+	workspace string
+}
+
+func (r *sandboxFs) execute(path string, fn func(root *os.Root, relPath string) error) error {
+	if r.workspace == "" {
+		return fmt.Errorf("workspace is not defined")
+	}
+
+	root, err := os.OpenRoot(r.workspace)
+	if err != nil {
+		return fmt.Errorf("failed to open workspace: %w", err)
+	}
+	defer root.Close()
+
+	relPath, err := getSafeRelPath(r.workspace, path)
+	if err != nil {
+		return err
+	}
+
+	return fn(root, relPath)
+}
+
+func (r *sandboxFs) ReadFile(path string) ([]byte, error) {
+	var content []byte
+	err := r.execute(path, func(root *os.Root, relPath string) error {
+		fileContent, err := root.ReadFile(relPath)
+		if err != nil {
+			if os.IsNotExist(err) {
+				return fmt.Errorf("failed to read file: file not found: %w", err)
+			}
+			// os.Root returns "escapes from parent" for paths outside the root
+			if os.IsPermission(err) || strings.Contains(err.Error(), "escapes from parent") ||
+				strings.Contains(err.Error(), "permission denied") {
+				return fmt.Errorf("failed to read file: access denied: %w", err)
+			}
+			return fmt.Errorf("failed to read file: %w", err)
+		}
+		content = fileContent
+		return nil
+	})
+	return content, err
+}
+
+func (r *sandboxFs) WriteFile(path string, data []byte) error {
+	return r.execute(path, func(root *os.Root, relPath string) error {
+		dir := filepath.Dir(relPath)
+		if dir != "." && dir != "/" {
+			if err := root.MkdirAll(dir, 0o755); err != nil {
+				return fmt.Errorf("failed to create parent directories: %w", err)
+			}
+		}
+
+		// We use a "write-then-rename" pattern here to ensure an atomic write.
+		// This prevents the target file from being left in a truncated or partial state
+		// if the operation is interrupted, as the rename operation is atomic on Linux.
+		tmpRelPath := fmt.Sprintf("%s.%d.tmp", relPath, time.Now().UnixNano())
+
+		if err := root.WriteFile(tmpRelPath, data, 0o644); err != nil {
+			root.Remove(tmpRelPath) // Ensure cleanup of partial/empty temp file
+			return fmt.Errorf("failed to write to temp file: %w", err)
+		}
+
+		if err := root.Rename(tmpRelPath, relPath); err != nil {
+			root.Remove(tmpRelPath)
+			return fmt.Errorf("failed to rename temp file over target: %w", err)
+		}
+		return nil
+	})
+}
+
+func (r *sandboxFs) ReadDir(path string) ([]os.DirEntry, error) {
+	var entries []os.DirEntry
+	err := r.execute(path, func(root *os.Root, relPath string) error {
+		dirEntries, err := fs.ReadDir(root.FS(), relPath)
+		if err != nil {
+			return err
+		}
+		entries = dirEntries
+		return nil
+	})
+	return entries, err
+}
+
+// Helper to get a safe relative path for os.Root usage
+func getSafeRelPath(workspace, path string) (string, error) {
+	if workspace == "" {
+		return "", fmt.Errorf("workspace is not defined")
+	}
+
+	rel := filepath.Clean(path)
+	if filepath.IsAbs(rel) {
+		var err error
+		rel, err = filepath.Rel(workspace, rel)
+		if err != nil {
+			return "", fmt.Errorf("failed to calculate relative path: %w", err)
 		}
 	}

-	return NewToolResult(result)
+	if !filepath.IsLocal(rel) {
+		return "", fmt.Errorf("path escapes workspace: %s", path)
+	}
+
+	return rel, nil
 }