diff --git a/agent/skills/fsskills/path_scope.go b/agent/skills/fsskills/path_scope.go new file mode 100644 index 00000000..b8332edc --- /dev/null +++ b/agent/skills/fsskills/path_scope.go @@ -0,0 +1,88 @@ +// Copyright (c) Microsoft. All rights reserved. + +package fsskills + +import ( + "errors" + "fmt" + "io/fs" + "path" + "strings" +) + +type skillPathScope struct { + rootFS fs.FS + skillDirPath string + skillDirPrefix string +} + +func newSkillPathScope(rootFS fs.FS, skillDirPath string) skillPathScope { + skillDirPath = path.Clean(skillDirPath) + skillDirPrefix := "" + if skillDirPath != "." { + skillDirPrefix = skillDirPath + "/" + } + return skillPathScope{ + rootFS: rootFS, + skillDirPath: skillDirPath, + skillDirPrefix: skillDirPrefix, + } +} + +func (s skillPathScope) validateDiscoveredPathForUse(relativePath, kind string) (string, error) { + fullPath := path.Clean(relativePath) + if s.skillDirPath != "." { + fullPath = path.Clean(path.Join(s.skillDirPath, relativePath)) + } + if !s.contains(fullPath) { + return "", fmt.Errorf("%s file %q references a path outside the skill directory", kind, relativePath) + } + if hasLinkOrInspectionFailureInPath(s.rootFS, fullPath) { + return "", fmt.Errorf("%s file %q has a symbolic link or inspection failure in its path; symbolic links are not allowed", kind, relativePath) + } + if _, err := fs.Stat(s.rootFS, fullPath); err != nil { + if errors.Is(err, fs.ErrNotExist) { + return "", fmt.Errorf("%s file %q was not found in the skill directory", kind, relativePath) + } + return "", err + } + return fullPath, nil +} + +func (s skillPathScope) contains(fullPath string) bool { + if s.skillDirPath == "." { + return fullPath != ".." && !strings.HasPrefix(fullPath, "../") + } + return fullPath == s.skillDirPath || strings.HasPrefix(fullPath, s.skillDirPrefix) +} + +func hasLinkOrInspectionFailureInPath(filesystem fs.FS, pathToCheck string) bool { + currentPath := "" + for _, segment := range strings.Split(path.Clean(pathToCheck), "/") { + if segment == "" || segment == "." { + continue + } + if currentPath == "" { + currentPath = segment + } else { + currentPath = path.Join(currentPath, segment) + } + if isUnsafePath(filesystem, currentPath) { + return true + } + } + return false +} + +func isUnsafePath(filesystem fs.FS, filePath string) bool { + readLinkFS, ok := filesystem.(fs.ReadLinkFS) + if !ok { + return true + } + + info, err := readLinkFS.Lstat(filePath) + if err == nil { + return info.Mode()&fs.ModeSymlink != 0 + } + return !errors.Is(err, fs.ErrNotExist) +} diff --git a/agent/skills/fsskills/source.go b/agent/skills/fsskills/source.go index 18a428d4..a2114cc6 100644 --- a/agent/skills/fsskills/source.go +++ b/agent/skills/fsskills/source.go @@ -189,7 +189,7 @@ func (s *Source) Skills(ctx context.Context) ([]*skills.Skill, error) { if err := ctx.Err(); err != nil { return nil, err } - skill := s.parseSkillDirectory(directory.fsys, directory.path) + skill := s.parseSkillDirectory(directory) if skill == nil { continue } @@ -237,7 +237,7 @@ func searchForSkills(filesystem fs.FS, dir string, logger *slog.Logger, results sub, subErr = fs.Sub(filesystem, dir) } if subErr == nil { - *results = append(*results, discoveredSkillDir{fsys: sub, path: dir}) + *results = append(*results, discoveredSkillDir{rootFS: filesystem, skillFS: sub, path: dir}) } return } @@ -258,7 +258,10 @@ func searchForSkills(filesystem fs.FS, dir string, logger *slog.Logger, results } } -func (s *Source) parseSkillDirectory(skillFS fs.FS, logPath string) *skills.Skill { +func (s *Source) parseSkillDirectory(directory discoveredSkillDir) *skills.Skill { + scope := newSkillPathScope(directory.rootFS, directory.path) + skillFS := directory.skillFS + logPath := directory.path data, err := fs.ReadFile(skillFS, skillFileName) if err != nil { s.logger.Error("Failed to read SKILL.md", "path", logPath, "error", err) @@ -271,8 +274,8 @@ func (s *Source) parseSkillDirectory(skillFS fs.FS, logPath string) *skills.Skil return nil } - resources := s.discoverResourceFiles(skillFS, frontmatter.Name) - scripts := s.discoverScriptFiles(skillFS, frontmatter.Name) + resources := s.discoverResourceFiles(skillFS, scope, frontmatter.Name) + scripts := s.discoverScriptFiles(skillFS, scope, frontmatter.Name) var ( contentOnce sync.Once cachedContent string @@ -467,7 +470,7 @@ func leadingWhitespaceCount(line string) int { return count } -func (s *Source) discoverResourceFiles(skillFS fs.FS, skillName string) []skills.Resource { +func (s *Source) discoverResourceFiles(skillFS fs.FS, scope skillPathScope, skillName string) []skills.Resource { seen := make(map[string]bool) var resources []skills.Resource s.scanForFiles(skillFS, ".", skillName, 1, s.allowedResourceExtensions, s.resourceFilter, "resource", func(filePath string) { @@ -482,7 +485,11 @@ func (s *Source) discoverResourceFiles(skillFS fs.FS, skillName string) []skills resources = append(resources, skills.Resource{ Name: filePath, Read: func(context.Context) (any, error) { - data, err := fs.ReadFile(skillFS, filePath) + validatedPath, err := scope.validateDiscoveredPathForUse(filePath, "resource") + if err != nil { + return nil, err + } + data, err := fs.ReadFile(scope.rootFS, validatedPath) if err != nil { return nil, err } @@ -493,7 +500,7 @@ func (s *Source) discoverResourceFiles(skillFS fs.FS, skillName string) []skills return resources } -func (s *Source) discoverScriptFiles(skillFS fs.FS, skillName string) []skills.Script { +func (s *Source) discoverScriptFiles(skillFS fs.FS, scope skillPathScope, skillName string) []skills.Script { seen := make(map[string]bool) var scripts []skills.Script s.scanForFiles(skillFS, ".", skillName, 1, s.allowedScriptExtensions, s.scriptFilter, "script", func(filePath string) { @@ -504,7 +511,7 @@ func (s *Source) discoverScriptFiles(skillFS fs.FS, skillName string) []skills.S return } seen[filePath] = true - scripts = append(scripts, newScript(filePath, skillFS, s.scriptRunner)) + scripts = append(scripts, newScript(filePath, skillFS, scope, s.scriptRunner)) }) return scripts } @@ -609,19 +616,19 @@ func validateExtensions(extensions []string) { } } -func newScript(name string, fsys fs.FS, runner skills.ScriptRunner) skills.Script { +func newScript(name string, fsys fs.FS, scope skillPathScope, runner skills.ScriptRunner) skills.Script { additionalProperties := map[string]any{ "fsskills.scriptFS": fsys, } return skills.Script{ Name: name, ParametersSchema: defaultFileScriptSchema, - Run: newFileScriptRunFunc(name, runner, additionalProperties), + Run: newFileScriptRunFunc(name, scope, runner, additionalProperties), AdditionalProperties: additionalProperties, } } -func newFileScriptRunFunc(name string, runner skills.ScriptRunner, additionalProperties map[string]any) func(context.Context, *skills.Skill, []string) (any, error) { +func newFileScriptRunFunc(name string, scope skillPathScope, runner skills.ScriptRunner, additionalProperties map[string]any) func(context.Context, *skills.Skill, []string) (any, error) { return func(ctx context.Context, owner *skills.Skill, arguments []string) (any, error) { if err := requireFileSkill(name, owner); err != nil { return nil, err @@ -629,6 +636,9 @@ func newFileScriptRunFunc(name string, runner skills.ScriptRunner, additionalPro if runner == nil { return nil, fmt.Errorf("script %q cannot be executed because no file script runner was provided", name) } + if _, err := scope.validateDiscoveredPathForUse(name, "script"); err != nil { + return nil, err + } // Hand the runner a script carrying the same metadata the discovered // Script exposes (parameters schema and the backing fs.FS), so runners // that inspect them to locate/execute the file see the real values @@ -650,8 +660,9 @@ func requireFileSkill(scriptName string, skill *skills.Skill) error { } type discoveredSkillDir struct { - fsys fs.FS - path string + rootFS fs.FS + skillFS fs.FS + path string } func buildAvailableResourcesBlock(resources []skills.Resource) string { diff --git a/agent/skills/fsskills/source_script_test.go b/agent/skills/fsskills/source_script_test.go index c83099ec..9bca3584 100644 --- a/agent/skills/fsskills/source_script_test.go +++ b/agent/skills/fsskills/source_script_test.go @@ -156,6 +156,67 @@ func TestFileSource_WithRunner_ScriptsCanRun(t *testing.T) { } } +func TestFileSource_ScriptExecution_RevalidatesParentDirectoriesBeforeRun(t *testing.T) { + root := t.TempDir() + createSkillDir(t, filepath.Join(root, "trusted"), "exec-skill", "Executor test", "Body.") + createRelativeFile(t, filepath.Join(root, "trusted", "exec-skill"), "scripts/test.py", "print('trusted')") + createSkillDir(t, filepath.Join(root, "outside", "trusted"), "exec-skill", "Executor test", "Body.") + createRelativeFile(t, filepath.Join(root, "outside", "trusted", "exec-skill"), "scripts/test.py", "print('outside')") + + runnerCalled := false + source := fsskills.NewSourceOptions(fsskills.SourceOptions{ScriptRunner: func(_ context.Context, _ *skills.Skill, _ *skills.Script, _ []string) (any, error) { + runnerCalled = true + return "executed", nil + }}, os.DirFS(root)) + + loaded, err := source.Skills(t.Context()) + if err != nil { + t.Fatal(err) + } + if len(loaded) != 1 || len(loaded[0].Scripts) != 1 { + t.Fatalf("expected one skill with one script, got %d skills and %d scripts", len(loaded), len(loaded[0].Scripts)) + } + + if err := os.Rename(filepath.Join(root, "trusted"), filepath.Join(root, "trusted-real")); err != nil { + t.Fatal(err) + } + createSymlink(t, filepath.Join(root, "trusted"), filepath.Join(root, "outside", "trusted")) + + _, err = loaded[0].Scripts[0].Run(t.Context(), loaded[0], nil) + if err == nil { + t.Fatal("expected script run to fail after the discovered path was replaced with a symlink") + } + if runnerCalled { + t.Fatal("expected script runner not to be called after path revalidation failed") + } +} + +func TestFileSource_ScriptExecution_FailsWithoutLinkInspection(t *testing.T) { + runnerCalled := false + source := fsskills.NewSourceOptions(fsskills.SourceOptions{ScriptRunner: func(_ context.Context, _ *skills.Skill, _ *skills.Script, _ []string) (any, error) { + runnerCalled = true + return "executed", nil + }}, fsWithoutLinkInspection{fstest.MapFS{ + "exec-skill/SKILL.md": {Data: []byte("---\nname: exec-skill\ndescription: A skill\n---\nBody.")}, + "exec-skill/scripts/test.py": {Data: []byte("print('test')")}, + }}) + + loaded, err := source.Skills(t.Context()) + if err != nil { + t.Fatal(err) + } + if len(loaded) != 1 || len(loaded[0].Scripts) != 1 { + t.Fatalf("expected one skill with one script, got %d skills and %d scripts", len(loaded), len(loaded[0].Scripts)) + } + + if _, err := loaded[0].Scripts[0].Run(t.Context(), loaded[0], nil); err == nil { + t.Fatal("expected script run to fail when the filesystem does not support link inspection") + } + if runnerCalled { + t.Fatal("expected script runner not to be called when link inspection is unavailable") + } +} + func TestFileSource_NullRunner_DoesNotPanic(t *testing.T) { _ = fsskills.NewSource(os.DirFS(t.TempDir())) } diff --git a/agent/skills/fsskills/source_test.go b/agent/skills/fsskills/source_test.go index a2a5763c..bbe72d77 100644 --- a/agent/skills/fsskills/source_test.go +++ b/agent/skills/fsskills/source_test.go @@ -3,14 +3,20 @@ package fsskills_test import ( + "io/fs" "os" "path/filepath" "strings" "testing" + "testing/fstest" "github.com/microsoft/agent-framework-go/agent/skills/fsskills" ) +type fsWithoutLinkInspection struct { + fs.FS +} + func TestFileSource_EmptyPaths_ReturnsEmptyList(t *testing.T) { source := fsskills.NewSource() @@ -300,6 +306,50 @@ func TestFileSource_ReadResource_ValidResource_ReturnsContent(t *testing.T) { } } +func TestFileSource_ReadResource_RevalidatesParentDirectoriesBeforeUse(t *testing.T) { + root := t.TempDir() + createSkillDirWithResource(t, filepath.Join(root, "trusted"), "read-skill", "A skill", "See docs.", "references/doc.md", "trusted content") + createSkillDirWithResource(t, filepath.Join(root, "outside", "trusted"), "read-skill", "A skill", "See docs.", "references/doc.md", "outside content") + + source := fsskills.NewSource(os.DirFS(root)) + loaded, err := source.Skills(t.Context()) + if err != nil { + t.Fatal(err) + } + if len(loaded) != 1 || len(loaded[0].Resources) != 1 { + t.Fatalf("expected one skill with one resource, got %d skills and %d resources", len(loaded), len(loaded[0].Resources)) + } + + if err := os.Rename(filepath.Join(root, "trusted"), filepath.Join(root, "trusted-real")); err != nil { + t.Fatal(err) + } + createSymlink(t, filepath.Join(root, "trusted"), filepath.Join(root, "outside", "trusted")) + + _, err = loaded[0].Resources[0].Read(t.Context()) + if err == nil { + t.Fatal("expected resource read to fail after the discovered path was replaced with a symlink") + } +} + +func TestFileSource_ReadResource_FailsWithoutLinkInspection(t *testing.T) { + source := fsskills.NewSource(fsWithoutLinkInspection{fstest.MapFS{ + "read-skill/SKILL.md": {Data: []byte("---\nname: read-skill\ndescription: A skill\n---\nSee docs.")}, + "read-skill/references/doc.md": {Data: []byte("content")}, + }}) + + loaded, err := source.Skills(t.Context()) + if err != nil { + t.Fatal(err) + } + if len(loaded) != 1 || len(loaded[0].Resources) != 1 { + t.Fatalf("expected one skill with one resource, got %d skills and %d resources", len(loaded), len(loaded[0].Resources)) + } + + if _, err := loaded[0].Resources[0].Read(t.Context()); err == nil { + t.Fatal("expected resource read to fail when the filesystem does not support link inspection") + } +} + func TestFileSource_MetadataWithQuotedValues_ParsedCorrectly(t *testing.T) { root := t.TempDir() createSkillDirRaw(t, root, "quoted-meta", strings.Join([]string{