diff --git a/dotnet/src/Microsoft.Agents.AI/Skills/File/AgentFileSkillsSource.cs b/dotnet/src/Microsoft.Agents.AI/Skills/File/AgentFileSkillsSource.cs index ae1714f237d..14d5d501f1e 100644 --- a/dotnet/src/Microsoft.Agents.AI/Skills/File/AgentFileSkillsSource.cs +++ b/dotnet/src/Microsoft.Agents.AI/Skills/File/AgentFileSkillsSource.cs @@ -5,6 +5,7 @@ using System.Diagnostics.CodeAnalysis; using System.IO; using System.Linq; +using System.Security; using System.Text; using System.Text.RegularExpressions; using System.Threading; @@ -21,6 +22,7 @@ namespace Microsoft.Agents.AI; /// /// /// Searches directories recursively (up to 2 levels deep) for SKILL.md files. +/// Symbolic links and reparse points below configured roots are not followed during skill discovery. /// Each file is validated for YAML frontmatter. Resource and script files are discovered by scanning the skill /// directory for files with matching extensions. Invalid resources are skipped with logged warnings. /// Resource and script paths are checked against path traversal and symlink escape attacks. @@ -114,7 +116,7 @@ public AgentFileSkillsSource( /// public override Task> GetSkillsAsync(AgentSkillsSourceContext context, CancellationToken cancellationToken = default) { - var discoveredPaths = DiscoverSkillDirectories(this._skillPaths); + var discoveredPaths = this.DiscoverSkillDirectories(this._skillPaths); LogSkillsDiscovered(this._logger, discoveredPaths.Count); @@ -138,7 +140,7 @@ public override Task> GetSkillsAsync(AgentSkillsSourceContext return Task.FromResult(skills as IList); } - private static List DiscoverSkillDirectories(IEnumerable skillPaths) + private List DiscoverSkillDirectories(IEnumerable skillPaths) { var discoveredPaths = new List(); @@ -149,17 +151,23 @@ private static List DiscoverSkillDirectories(IEnumerable skillPa continue; } - SearchDirectoriesForSkills(rootDirectory, discoveredPaths, currentDepth: 0); + this.SearchDirectoriesForSkills(rootDirectory, discoveredPaths, currentDepth: 0); } return discoveredPaths; } - private static void SearchDirectoriesForSkills(string directory, List results, int currentDepth) + private void SearchDirectoriesForSkills(string directory, List results, int currentDepth) { string skillFilePath = Path.Combine(directory, SkillFileName); if (File.Exists(skillFilePath)) { + if (IsLinkOrReparsePointOrInaccessible(skillFilePath)) + { + LogUnsafeSkillDiscoveryPath(this._logger, SanitizePathForLog(skillFilePath)); + return; + } + // Once a SKILL.md is found, this directory is the skill root. // Subdirectories are part of this skill and should not be treated as independent skill roots. results.Add(Path.GetFullPath(directory)); @@ -171,9 +179,15 @@ private static void SearchDirectoriesForSkills(string directory, List re return; } - foreach (string subdirectory in Directory.EnumerateDirectories(directory)) + foreach (string subdirectory in this.SafeEnumerateDirectories(directory, attributesToSkip: 0)) { - SearchDirectoriesForSkills(subdirectory, results, currentDepth + 1); + if (IsLinkOrReparsePointOrInaccessible(subdirectory)) + { + LogUnsafeSkillDiscoveryPath(this._logger, SanitizePathForLog(subdirectory)); + continue; + } + + this.SearchDirectoriesForSkills(subdirectory, results, currentDepth + 1); } } @@ -324,8 +338,9 @@ private void ScanDirectoryForResources(string targetDirectory, string skillDirec bool isRootDirectory = string.Equals(targetDirectory, skillDirectoryFullPath, StringComparison.OrdinalIgnoreCase); // Directory-level symlink check: skip if targetDirectory (or any intermediate - // segment) is a reparse point. The root directory is excluded — it's a caller-supplied - // trusted path, and the security boundary guards files within it, not the path itself. + // segment) is a reparse point or cannot be inspected. The root directory is excluded — + // it's a caller-supplied trusted path, and the security boundary guards files within it, + // not the path itself. if (!isRootDirectory && HasSymlinkInPath(targetDirectory, skillDirectoryFullPath)) { if (this._logger.IsEnabled(LogLevel.Warning)) @@ -386,7 +401,8 @@ private void ScanDirectoryForResources(string targetDirectory, string skillDirec } // Per-file symlink check: detects if the file (or any intermediate segment) - // is a reparse point. e.g. "references/secret.md" → symlink to "/etc/shadow" + // is a reparse point or cannot be inspected. + // e.g. "references/secret.md" → symlink to "/etc/shadow" if (HasSymlinkInPath(resolvedFilePath, skillDirectoryFullPath)) { if (this._logger.IsEnabled(LogLevel.Warning)) @@ -413,11 +429,7 @@ private void ScanDirectoryForResources(string targetDirectory, string skillDirec // Recurse into subdirectories if within depth limit if (currentDepth < this._searchDepth) { -#if NET - foreach (string subdirectory in Directory.EnumerateDirectories(targetDirectory, "*", enumerationOptions)) -#else - foreach (string subdirectory in this.SafeEnumerateDirectories(targetDirectory)) -#endif + foreach (string subdirectory in this.SafeEnumerateDirectories(targetDirectory, FileAttributes.ReparsePoint)) { this.ScanDirectoryForResources(subdirectory, skillDirectoryFullPath, skillName, resources, currentDepth + 1); } @@ -452,8 +464,9 @@ private void ScanDirectoryForScripts(string targetDirectory, string skillDirecto bool isRootDirectory = string.Equals(targetDirectory, skillDirectoryFullPath, StringComparison.OrdinalIgnoreCase); // Directory-level symlink check: skip if targetDirectory (or any intermediate - // segment) is a reparse point. The root directory is excluded — it's a caller-supplied - // trusted path, and the security boundary guards files within it, not the path itself. + // segment) is a reparse point or cannot be inspected. The root directory is excluded — + // it's a caller-supplied trusted path, and the security boundary guards files within it, + // not the path itself. if (!isRootDirectory && HasSymlinkInPath(targetDirectory, skillDirectoryFullPath)) { if (this._logger.IsEnabled(LogLevel.Warning)) @@ -501,7 +514,8 @@ private void ScanDirectoryForScripts(string targetDirectory, string skillDirecto } // Per-file symlink check: detects if the file (or any intermediate segment) - // is a reparse point. e.g. "scripts/run.py" → symlink to "/etc/shadow" + // is a reparse point or cannot be inspected. + // e.g. "scripts/run.py" → symlink to "/etc/shadow" if (HasSymlinkInPath(resolvedFilePath, skillDirectoryFullPath)) { if (this._logger.IsEnabled(LogLevel.Warning)) @@ -528,11 +542,7 @@ private void ScanDirectoryForScripts(string targetDirectory, string skillDirecto // Recurse into subdirectories if within depth limit if (currentDepth < this._searchDepth) { -#if NET - foreach (string subdirectory in Directory.EnumerateDirectories(targetDirectory, "*", enumerationOptions)) -#else - foreach (string subdirectory in this.SafeEnumerateDirectories(targetDirectory)) -#endif + foreach (string subdirectory in this.SafeEnumerateDirectories(targetDirectory, FileAttributes.ReparsePoint)) { this.ScanDirectoryForScripts(subdirectory, skillDirectoryFullPath, skillName, scripts, currentDepth + 1); } @@ -540,7 +550,8 @@ private void ScanDirectoryForScripts(string targetDirectory, string skillDirecto } /// - /// Checks whether any segment in the path (relative to the directory) is a symlink. + /// Checks whether any segment in the path (relative to the directory) is a symlink, + /// reparse point, or cannot be inspected. /// private static bool HasSymlinkInPath(string pathToCheck, string trustedBasePath) { @@ -555,7 +566,7 @@ private static bool HasSymlinkInPath(string pathToCheck, string trustedBasePath) { currentPath = Path.Combine(currentPath, segment); - if ((File.GetAttributes(currentPath) & FileAttributes.ReparsePoint) != 0) + if (IsLinkOrReparsePointOrInaccessible(currentPath)) { return true; } @@ -564,30 +575,56 @@ private static bool HasSymlinkInPath(string pathToCheck, string trustedBasePath) return false; } -#if !NET + private static bool IsLinkOrReparsePointOrInaccessible(string path) + { + try + { + return (File.GetAttributes(path) & FileAttributes.ReparsePoint) != 0; + } + catch (Exception ex) when (IsFileSystemInspectionFailure(ex)) + { + return true; + } + } + + private static bool IsFileSystemInspectionFailure(Exception exception) + { + return exception is IOException or UnauthorizedAccessException or SecurityException; + } + /// - /// Best-effort directory enumeration for target frameworks without - /// EnumerationOptions.IgnoreInaccessible support. Returns an empty - /// array when the caller lacks permission to read the directory contents, - /// so a single inaccessible child does not abort the entire skill scan. + /// Best-effort directory enumeration that returns an empty array when the + /// directory cannot be inspected, so a single inaccessible child does not + /// abort the entire skill scan. /// - private string[] SafeEnumerateDirectories(string path) + private string[] SafeEnumerateDirectories(string path, FileAttributes attributesToSkip) { try { +#if NET + var enumerationOptions = new EnumerationOptions + { + RecurseSubdirectories = false, + IgnoreInaccessible = true, + AttributesToSkip = attributesToSkip, + }; + + return Directory.EnumerateDirectories(path, "*", enumerationOptions).ToArray(); +#else + _ = attributesToSkip; return Directory.GetDirectories(path); +#endif } - catch (UnauthorizedAccessException) + catch (Exception ex) when (IsFileSystemInspectionFailure(ex)) { if (this._logger.IsEnabled(LogLevel.Warning)) { - LogDirectoryAccessDenied(this._logger, SanitizePathForLog(path)); + LogDirectoryInspectionFailed(this._logger, SanitizePathForLog(path)); } return Array.Empty(); } } -#endif private static string ParseYamlScalarValue(string yamlContent, Match kvMatch) { @@ -720,6 +757,9 @@ private static void ValidateExtensions(IEnumerable? extensions) [LoggerMessage(LogLevel.Information, "Successfully loaded {Count} skills")] private static partial void LogSkillsLoadedTotal(ILogger logger, int count); + [LoggerMessage(LogLevel.Warning, "Skipping skill discovery path '{Path}': symbolic link or reparse point detected, or path could not be inspected")] + private static partial void LogUnsafeSkillDiscoveryPath(ILogger logger, string path); + [LoggerMessage(LogLevel.Error, "SKILL.md at '{SkillFilePath}' does not contain valid YAML frontmatter delimited by '---'")] private static partial void LogInvalidFrontmatter(ILogger logger, string skillFilePath); @@ -732,10 +772,10 @@ private static void ValidateExtensions(IEnumerable? extensions) [LoggerMessage(LogLevel.Warning, "Skipping resource in skill '{SkillName}': '{ResourcePath}' references a path outside the skill directory")] private static partial void LogResourcePathTraversal(ILogger logger, string skillName, string resourcePath); - [LoggerMessage(LogLevel.Warning, "Skipping resource in skill '{SkillName}': '{ResourcePath}' is a symlink that resolves outside the skill directory")] + [LoggerMessage(LogLevel.Warning, "Skipping resource in skill '{SkillName}': '{ResourcePath}' contains a symbolic link or reparse point, or could not be inspected")] private static partial void LogResourceSymlinkEscape(ILogger logger, string skillName, string resourcePath); - [LoggerMessage(LogLevel.Warning, "Skipping resource directory '{DirectoryName}' in skill '{SkillName}': directory path contains a symlink")] + [LoggerMessage(LogLevel.Warning, "Skipping resource directory '{DirectoryName}' in skill '{SkillName}': directory path contains a symbolic link or reparse point, or could not be inspected")] private static partial void LogResourceSymlinkDirectory(ILogger logger, string skillName, string directoryName); [LoggerMessage(LogLevel.Debug, "Skipping file '{FilePath}' in skill '{SkillName}': extension '{Extension}' is not in the allowed list")] @@ -744,12 +784,12 @@ private static void ValidateExtensions(IEnumerable? extensions) [LoggerMessage(LogLevel.Warning, "Skipping script in skill '{SkillName}': '{ScriptPath}' references a path outside the skill directory")] private static partial void LogScriptPathTraversal(ILogger logger, string skillName, string scriptPath); - [LoggerMessage(LogLevel.Warning, "Skipping script in skill '{SkillName}': '{ScriptPath}' is a symlink that resolves outside the skill directory")] + [LoggerMessage(LogLevel.Warning, "Skipping script in skill '{SkillName}': '{ScriptPath}' contains a symbolic link or reparse point, or could not be inspected")] private static partial void LogScriptSymlinkEscape(ILogger logger, string skillName, string scriptPath); - [LoggerMessage(LogLevel.Warning, "Skipping script directory '{DirectoryName}' in skill '{SkillName}': directory path contains a symlink")] + [LoggerMessage(LogLevel.Warning, "Skipping script directory '{DirectoryName}' in skill '{SkillName}': directory path contains a symbolic link or reparse point, or could not be inspected")] private static partial void LogScriptSymlinkDirectory(ILogger logger, string skillName, string directoryName); - [LoggerMessage(LogLevel.Warning, "Skipping directory '{DirectoryPath}': access denied")] - private static partial void LogDirectoryAccessDenied(ILogger logger, string directoryPath); + [LoggerMessage(LogLevel.Warning, "Skipping directory '{DirectoryPath}': directory could not be inspected")] + private static partial void LogDirectoryInspectionFailed(ILogger logger, string directoryPath); } diff --git a/dotnet/tests/Microsoft.Agents.AI.UnitTests/AgentSkills/FileAgentSkillLoaderTests.cs b/dotnet/tests/Microsoft.Agents.AI.UnitTests/AgentSkills/FileAgentSkillLoaderTests.cs index b185002f67f..41a772f1b18 100644 --- a/dotnet/tests/Microsoft.Agents.AI.UnitTests/AgentSkills/FileAgentSkillLoaderTests.cs +++ b/dotnet/tests/Microsoft.Agents.AI.UnitTests/AgentSkills/FileAgentSkillLoaderTests.cs @@ -1,8 +1,16 @@ // Copyright (c) Microsoft. All rights reserved. using System; +#if NET +using System.Diagnostics; +#endif using System.IO; using System.Linq; +#if NET +using System.Runtime.Versioning; +#endif +using System.Security.AccessControl; +using System.Security.Principal; using System.Threading.Tasks; namespace Microsoft.Agents.AI.UnitTests.AgentSkills; @@ -628,6 +636,223 @@ public async Task GetSkillsAsync_DescriptionExceedsMaxLength_ExcludesSkillAsync( } #if NET + private static bool TryCreateDirectorySymbolicLink(string linkPath, string targetPath) + { + try + { + Directory.CreateSymbolicLink(linkPath, targetPath); + } + catch (IOException) + { + return false; + } + catch (UnauthorizedAccessException) + { + return false; + } + catch (PlatformNotSupportedException) + { + return false; + } + + return Directory.Exists(linkPath) + && (File.GetAttributes(linkPath) & FileAttributes.ReparsePoint) != 0; + } + + private static bool TryCreateFileSymbolicLink(string linkPath, string targetPath) + { + try + { + File.CreateSymbolicLink(linkPath, targetPath); + } + catch (IOException) + { + return false; + } + catch (UnauthorizedAccessException) + { + return false; + } + catch (PlatformNotSupportedException) + { + return false; + } + + return File.Exists(linkPath) + && (File.GetAttributes(linkPath) & FileAttributes.ReparsePoint) != 0; + } + + private static bool TryCreateDirectoryJunction(string linkPath, string targetPath) + { + if (!OperatingSystem.IsWindows()) + { + return false; + } + + string commandInterpreter = Environment.GetEnvironmentVariable("COMSPEC") ?? "cmd.exe"; + var startInfo = new ProcessStartInfo + { + FileName = commandInterpreter, + Arguments = $"/c mklink /J \"{linkPath}\" \"{targetPath}\"", + CreateNoWindow = true, + RedirectStandardError = true, + RedirectStandardOutput = true, + UseShellExecute = false, + }; + + using Process? process = Process.Start(startInfo); + if (process is null) + { + return false; + } + + process.WaitForExit(); + return process.ExitCode == 0 + && Directory.Exists(linkPath) + && (File.GetAttributes(linkPath) & FileAttributes.ReparsePoint) != 0; + } + + [Fact] + public async Task GetSkillsAsync_SymlinkedSkillDirectory_SkipsLinkedSkillAsync() + { + // Arrange + string root = Path.Combine(this._testRoot, "root"); + string outsideSkill = Path.Combine(this._testRoot, "outside", "evil-skill"); + string linkedSkill = Path.Combine(root, "evil-skill"); + Directory.CreateDirectory(root); + Directory.CreateDirectory(outsideSkill); + File.WriteAllText( + Path.Combine(outsideSkill, "SKILL.md"), + "---\nname: evil-skill\ndescription: Linked skill\n---\nBody."); + _ = CreateSkillDirectory(root, "good-skill"); + + if (!TryCreateDirectorySymbolicLink(linkedSkill, outsideSkill)) + { + return; + } + + try + { + var source = new AgentFileSkillsSource(root, s_noOpExecutor); + + // Act + var skills = await source.GetSkillsAsync(TestAgentSkillsSourceContextFactory.Create()); + + // Assert + Assert.Single(skills); + Assert.Equal("good-skill", skills[0].Frontmatter.Name); + } + finally + { + Directory.Delete(linkedSkill); + } + } + + [Fact] + public async Task GetSkillsAsync_SymlinkedSkillFile_SkipsSkillAsync() + { + // Arrange + string root = Path.Combine(this._testRoot, "root"); + string skillDirectory = Path.Combine(root, "evil-skill"); + string outsideSkillFile = Path.Combine(this._testRoot, "outside-SKILL.md"); + string linkedSkillFile = Path.Combine(skillDirectory, "SKILL.md"); + Directory.CreateDirectory(skillDirectory); + File.WriteAllText( + outsideSkillFile, + "---\nname: evil-skill\ndescription: Linked skill file\n---\nBody."); + + if (!TryCreateFileSymbolicLink(linkedSkillFile, outsideSkillFile)) + { + return; + } + + try + { + var source = new AgentFileSkillsSource(root, s_noOpExecutor); + + // Act + var skills = await source.GetSkillsAsync(TestAgentSkillsSourceContextFactory.Create()); + + // Assert + Assert.Empty(skills); + } + finally + { + File.Delete(linkedSkillFile); + } + } + + [Fact] + public async Task GetSkillsAsync_ConfiguredRootIsSymlink_DiscoversRealSkillsAsync() + { + // Arrange + string realRoot = Path.Combine(this._testRoot, "real-root"); + string linkedRoot = Path.Combine(this._testRoot, "linked-root"); + _ = CreateSkillDirectory(realRoot, "my-skill"); + + if (!TryCreateDirectorySymbolicLink(linkedRoot, realRoot)) + { + return; + } + + try + { + var source = new AgentFileSkillsSource(linkedRoot, s_noOpExecutor); + + // Act + var skills = await source.GetSkillsAsync(TestAgentSkillsSourceContextFactory.Create()); + + // Assert + Assert.Single(skills); + Assert.Equal("my-skill", skills[0].Frontmatter.Name); + } + finally + { + Directory.Delete(linkedRoot); + } + } + + [Fact] + public async Task GetSkillsAsync_JunctionedSkillDirectory_SkipsLinkedSkillAsync() + { + // Arrange + if (!OperatingSystem.IsWindows()) + { + return; + } + + string root = Path.Combine(this._testRoot, "root"); + string outsideSkill = Path.Combine(this._testRoot, "outside", "evil-skill"); + string junctionSkill = Path.Combine(root, "evil-skill"); + Directory.CreateDirectory(root); + Directory.CreateDirectory(outsideSkill); + File.WriteAllText( + Path.Combine(outsideSkill, "SKILL.md"), + "---\nname: evil-skill\ndescription: Junctioned skill\n---\nBody."); + _ = CreateSkillDirectory(root, "good-skill"); + + if (!TryCreateDirectoryJunction(junctionSkill, outsideSkill)) + { + return; + } + + try + { + var source = new AgentFileSkillsSource(root, s_noOpExecutor); + + // Act + var skills = await source.GetSkillsAsync(TestAgentSkillsSourceContextFactory.Create()); + + // Assert + Assert.Single(skills); + Assert.Equal("good-skill", skills[0].Frontmatter.Name); + } + finally + { + Directory.Delete(junctionSkill); + } + } + [Fact] public async Task GetSkillsAsync_SymlinkInPath_SkipsSymlinkedResourcesAsync() { @@ -787,6 +1012,111 @@ public async Task GetSkillsAsync_SymlinkedIntermediateSegment_SkipsSymlinkedDire } #endif + /// + /// Denies permission to list the contents of while leaving the + /// directory itself inspectable, so enumerating it fails but reading its attributes does not. + /// Returns when the environment does not honor the restriction + /// (for example, an elevated or root test host), in which case no cleanup is required. + /// + private static bool TryDenyDirectoryListing(string path, out Action restore) + { + restore = static () => { }; + + try + { +#if NET + restore = OperatingSystem.IsWindows() + ? DenyDirectoryListingOnWindows(path) + : DenyDirectoryListingOnUnix(path); +#else + restore = DenyDirectoryListingOnWindows(path); +#endif + } + catch (Exception ex) when (ex is IOException or UnauthorizedAccessException or PlatformNotSupportedException) + { + return false; + } + + // Confirm the restriction actually takes effect; elevated hosts can bypass it. + try + { + _ = Directory.GetDirectories(path); + } + catch (Exception ex) when (ex is IOException or UnauthorizedAccessException) + { + return true; + } + + restore(); + restore = static () => { }; + return false; + } + +#if NET + [SupportedOSPlatform("windows")] +#endif + private static Action DenyDirectoryListingOnWindows(string path) + { + var directoryInfo = new DirectoryInfo(path); + SecurityIdentifier user = WindowsIdentity.GetCurrent().User!; + var denyRule = new FileSystemAccessRule(user, FileSystemRights.ListDirectory, AccessControlType.Deny); + + DirectorySecurity security = directoryInfo.GetAccessControl(); + security.AddAccessRule(denyRule); + directoryInfo.SetAccessControl(security); + + return () => + { + DirectorySecurity currentSecurity = directoryInfo.GetAccessControl(); + currentSecurity.RemoveAccessRule(denyRule); + directoryInfo.SetAccessControl(currentSecurity); + }; + } + +#if NET + [UnsupportedOSPlatform("windows")] + private static Action DenyDirectoryListingOnUnix(string path) + { + UnixFileMode originalMode = File.GetUnixFileMode(path); + + // Execute-only: the directory can still be traversed and stat'ed, but not listed. + File.SetUnixFileMode(path, UnixFileMode.UserExecute); + + return () => File.SetUnixFileMode(path, originalMode); + } +#endif + + [Fact] + public async Task GetSkillsAsync_UnreadableSubdirectory_StillDiscoversSiblingSkillsAsync() + { + // Arrange — discovery must not abort when a single subdirectory cannot be enumerated. + string root = Path.Combine(this._testRoot, "root"); + string blockedDirectory = Path.Combine(root, "blocked"); + Directory.CreateDirectory(blockedDirectory); + _ = CreateSkillDirectory(root, "good-skill"); + + if (!TryDenyDirectoryListing(blockedDirectory, out Action restore)) + { + return; + } + + try + { + var source = new AgentFileSkillsSource(root, s_noOpExecutor); + + // Act + var skills = await source.GetSkillsAsync(TestAgentSkillsSourceContextFactory.Create()); + + // Assert + Assert.Single(skills); + Assert.Equal("good-skill", skills[0].Frontmatter.Name); + } + finally + { + restore(); + } + } + [Fact] public async Task GetSkillsAsync_FileWithUtf8Bom_ParsesSuccessfullyAsync() { @@ -1097,6 +1427,16 @@ private string CreateSkillDirectory(string name, string description, string body return skillDir; } + private static string CreateSkillDirectory(string root, string name) + { + string skillDirectory = Path.Combine(root, name); + Directory.CreateDirectory(skillDirectory); + File.WriteAllText( + Path.Combine(skillDirectory, "SKILL.md"), + $"---\nname: {name}\ndescription: A skill\n---\nBody."); + return skillDirectory; + } + private string CreateSkillDirectoryWithRawContent(string directoryName, string rawContent) { string skillDir = Path.Combine(this._testRoot, directoryName);