diff --git a/workspace-server/internal/handlers/org_helpers.go b/workspace-server/internal/handlers/org_helpers.go index 824fd2d7..b491e90f 100644 --- a/workspace-server/internal/handlers/org_helpers.go +++ b/workspace-server/internal/handlers/org_helpers.go @@ -317,6 +317,12 @@ func mergePlugins(defaultPlugins, wsPlugins []string) []string { // Follows Go's standard pattern for SSRF-class path sanitization; using // strings.HasPrefix on an absolute-path pair plus the separator guard rejects // sibling directories that share a prefix (e.g. "/foo" vs "/foobar"). +// +// CWE-59 mitigation: filepath.Abs does NOT resolve symlinks, so a path like +// "workspaces/dev/inner" where "inner" is a symlink to "/etc" would lexically +// pass the prefix check. We call filepath.EvalSymlinks to canonicalize the +// path and re-check that it is still inside root. This closes the symlink- +// based traversal vector (CWE-59, follow-up to #369). func resolveInsideRoot(root, userPath string) (string, error) { if userPath == "" { return "", fmt.Errorf("path is empty") @@ -333,9 +339,18 @@ func resolveInsideRoot(root, userPath string) (string, error) { if err != nil { return "", fmt.Errorf("joined abs: %w", err) } + // CWE-59: resolve symlinks before final prefix check. + // If the path contains a symlink pointing outside root, EvalSymlinks + // will canonicalize to the external path and fail the guard below. + resolved, err := filepath.EvalSymlinks(absJoined) + if err != nil { + // If EvalSymlinks fails (e.g. broken symlink), fail closed — + // broken symlinks should not be used as org files. + return "", fmt.Errorf("resolve symlink: %w", err) + } // Allow exact-root match (rare but valid) and any descendant. - if absJoined != absRoot && !strings.HasPrefix(absJoined, absRoot+string(filepath.Separator)) { + if resolved != absRoot && !strings.HasPrefix(resolved, absRoot+string(filepath.Separator)) { return "", fmt.Errorf("path escapes root") } - return absJoined, nil + return absJoined, nil // return the lexical path, not the resolved one } diff --git a/workspace-server/internal/handlers/org_path_test.go b/workspace-server/internal/handlers/org_path_test.go index 2ec707ff..e1561850 100644 --- a/workspace-server/internal/handlers/org_path_test.go +++ b/workspace-server/internal/handlers/org_path_test.go @@ -78,6 +78,48 @@ func TestResolveInsideRoot_RejectsPrefixSibling(t *testing.T) { } } +// TestResolveInsideRoot_RejectsSymlinkTraversal is a regression test for +// CWE-59 (symlink-based path traversal). An attacker plants a symlink inside +// the allowed directory that points outside; the function must reject it. +func TestResolveInsideRoot_RejectsSymlinkTraversal(t *testing.T) { + tmp := t.TempDir() + // Create a subdirectory inside root. + inner := filepath.Join(tmp, "workspaces", "dev") + if err := os.MkdirAll(inner, 0o755); err != nil { + t.Fatal(err) + } + // Plant a symlink that resolves outside root. + sym := filepath.Join(inner, "leaked") + if err := os.Symlink("/etc", sym); err != nil { + t.Fatal(err) + } + + // Lexically, "workspaces/dev/leaked" is inside tmp — but after symlink + // resolution it points to /etc and must be rejected. + if _, err := resolveInsideRoot(tmp, filepath.Join("workspaces", "dev", "leaked")); err == nil { + t.Error("symlink pointing outside root must be rejected (CWE-59)") + } + + // Symlink that stays inside root is fine. + safe := filepath.Join(inner, "safe") + if err := os.Symlink(filepath.Join(tmp, "other"), safe); err != nil { + t.Fatal(err) + } + if _, err := resolveInsideRoot(tmp, filepath.Join("workspaces", "dev", "safe")); err != nil { + t.Errorf("symlink staying inside root must be allowed: %v", err) + } + + // Broken symlink (target does not exist) must also be rejected — broken + // symlinks cannot be valid org files. + broken := filepath.Join(inner, "broken") + if err := os.Symlink("/nonexistent/broken", broken); err != nil { + t.Fatal(err) + } + if _, err := resolveInsideRoot(tmp, filepath.Join("workspaces", "dev", "broken")); err == nil { + t.Error("broken symlink must be rejected") + } +} + func TestResolveInsideRoot_DeepSubpath(t *testing.T) { tmp := t.TempDir() deep := filepath.Join(tmp, "a", "b", "c")