From aa047159fa06e6a8c92a1d6a64806fea837ab5c9 Mon Sep 17 00:00:00 2001 From: Roger Barreto <19890735+rogerbarreto@users.noreply.github.com> Date: Tue, 10 Mar 2026 12:36:36 +0000 Subject: [PATCH 1/2] .Net: Harden plugin security defaults for WebFileDownloadPlugin, HttpPlugin, and FileIOPlugin - Change AllowedDomains and AllowedFolders defaults from null (allow-all) to empty (deny-all) - Change DisableFileOverwrite default to true - Set MaximumDownloadSize default to 10 MB - Add path canonicalization via Path.GetFullPath() to prevent directory traversal - Switch folder matching from exact to prefix-based with separator handling - Fix fileMode variable not being used in FileStream constructor - Add XML doc security remarks to all three plugins - Update tests for new defaults Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- .../src/Plugins/Plugins.Core/FileIOPlugin.cs | 66 +++++++++++--- dotnet/src/Plugins/Plugins.Core/HttpPlugin.cs | 22 ++++- .../Core/FileIOPluginTests.cs | 31 +++++-- .../Plugins.UnitTests/Core/HttpPluginTests.cs | 20 ++++- .../Web/WebFileDownloadPluginTests.cs | 45 ++++++++-- .../Plugins.Web/WebFileDownloadPlugin.cs | 86 +++++++++++++++---- 6 files changed, 225 insertions(+), 45 deletions(-) diff --git a/dotnet/src/Plugins/Plugins.Core/FileIOPlugin.cs b/dotnet/src/Plugins/Plugins.Core/FileIOPlugin.cs index 6863ca886c41..9a94a08538a0 100644 --- a/dotnet/src/Plugins/Plugins.Core/FileIOPlugin.cs +++ b/dotnet/src/Plugins/Plugins.Core/FileIOPlugin.cs @@ -12,11 +12,26 @@ namespace Microsoft.SemanticKernel.Plugins.Core; /// /// Read and write from a file. /// +/// +/// +/// This plugin is secure by default. must be explicitly configured +/// before any file operations are permitted. By default, all file paths are denied. +/// +/// +/// When exposing this plugin to an LLM via auto function calling, ensure that +/// is restricted to trusted values only. +/// +/// public sealed class FileIOPlugin { /// - /// List of allowed folders to read from or write to. + /// List of allowed folders to read from or write to. Subdirectories of allowed folders are also permitted. /// + /// + /// Defaults to an empty collection (no folders allowed). Must be explicitly populated + /// with trusted directory paths before any file operations will succeed. + /// Paths are canonicalized before validation to prevent directory traversal. + /// public IEnumerable? AllowedFolders { get => this._allowedFolders; @@ -24,9 +39,12 @@ public IEnumerable? AllowedFolders } /// - /// Set to true to disable overwriting existing files. + /// Set to false to allow overwriting existing files. /// - public bool DisableFileOverwrite { get; set; } = false; + /// + /// Defaults to true (overwriting is disabled). + /// + public bool DisableFileOverwrite { get; set; } = true; /// /// Read a file @@ -67,6 +85,11 @@ public async Task WriteAsync( throw new InvalidOperationException("Writing to the provided location is not allowed."); } + if (this.DisableFileOverwrite && File.Exists(path)) + { + throw new InvalidOperationException("Overwriting existing files is disabled."); + } + byte[] text = Encoding.UTF8.GetBytes(content); var fileMode = this.DisableFileOverwrite ? FileMode.CreateNew : FileMode.Create; using var writer = new FileStream(path, fileMode, FileAccess.Write, FileShare.None); @@ -78,11 +101,12 @@ await writer.WriteAsync(text } #region private - private HashSet? _allowedFolders; + private HashSet? _allowedFolders = []; /// - /// If a list of allowed folder has been provided, the folder of the provided path is checked - /// to verify it is in the allowed folder list. + /// If a list of allowed folder has been provided, the folder of the provided filePath is checked + /// to verify it is in the allowed folder list. Paths are canonicalized before comparison. + /// Subdirectories of allowed folders are also permitted. /// private bool IsFilePathAllowed(string path) { @@ -93,11 +117,6 @@ private bool IsFilePathAllowed(string path) throw new ArgumentException("Invalid file path, UNC paths are not supported.", nameof(path)); } - if (this.DisableFileOverwrite && File.Exists(path)) - { - throw new ArgumentException("Invalid file path, overwriting existing files is disabled.", nameof(path)); - } - string? directoryPath = Path.GetDirectoryName(path); if (string.IsNullOrEmpty(directoryPath)) @@ -111,7 +130,30 @@ private bool IsFilePathAllowed(string path) throw new UnauthorizedAccessException($"File is read-only: {path}"); } - return this._allowedFolders is null || this._allowedFolders.Contains(directoryPath); + if (this._allowedFolders is null || this._allowedFolders.Count == 0) + { + return false; + } + + var canonicalDir = Path.GetFullPath(directoryPath); + + foreach (var allowedFolder in this._allowedFolders) + { + var canonicalAllowed = Path.GetFullPath(allowedFolder); + var separator = Path.DirectorySeparatorChar.ToString(); + if (!canonicalAllowed.EndsWith(separator, StringComparison.OrdinalIgnoreCase)) + { + canonicalAllowed += separator; + } + + if (canonicalDir.StartsWith(canonicalAllowed, StringComparison.OrdinalIgnoreCase) + || (canonicalDir + separator).Equals(canonicalAllowed, StringComparison.OrdinalIgnoreCase)) + { + return true; + } + } + + return false; } #endregion } diff --git a/dotnet/src/Plugins/Plugins.Core/HttpPlugin.cs b/dotnet/src/Plugins/Plugins.Core/HttpPlugin.cs index ed3d9635f8a1..89f49d8d4705 100644 --- a/dotnet/src/Plugins/Plugins.Core/HttpPlugin.cs +++ b/dotnet/src/Plugins/Plugins.Core/HttpPlugin.cs @@ -14,6 +14,16 @@ namespace Microsoft.SemanticKernel.Plugins.Core; /// /// A plugin that provides HTTP functionality. /// +/// +/// +/// This plugin is secure by default. must be explicitly configured +/// before any HTTP requests are permitted. By default, all domains are denied. +/// +/// +/// When exposing this plugin to an LLM via auto function calling, ensure that +/// is restricted to trusted values only. +/// +/// [System.Diagnostics.CodeAnalysis.SuppressMessage("Design", "CA1054:URI-like parameters should not be strings", Justification = "Semantic Kernel operates on strings")] public sealed class HttpPlugin @@ -39,8 +49,12 @@ public HttpPlugin(HttpClient? client = null) => this._client = client ?? HttpClientProvider.GetHttpClient(); /// - /// List of allowed domains to download from. + /// List of allowed domains to send requests to. /// + /// + /// Defaults to an empty collection (no domains allowed). Must be explicitly populated + /// with trusted domains before any requests will succeed. + /// public IEnumerable? AllowedDomains { get => this._allowedDomains; @@ -100,7 +114,7 @@ public Task DeleteAsync( this.SendRequestAsync(uri, HttpMethod.Delete, requestContent: null, cancellationToken); #region private - private HashSet? _allowedDomains; + private HashSet? _allowedDomains = []; /// /// If a list of allowed domains has been provided, the host of the provided uri is checked @@ -110,7 +124,9 @@ private bool IsUriAllowed(Uri uri) { Verify.NotNull(uri); - return this._allowedDomains is null || this._allowedDomains.Contains(uri.Host); + return this._allowedDomains is not null + && this._allowedDomains.Count > 0 + && this._allowedDomains.Contains(uri.Host); } /// Sends an HTTP request and returns the response content as a string. diff --git a/dotnet/src/Plugins/Plugins.UnitTests/Core/FileIOPluginTests.cs b/dotnet/src/Plugins/Plugins.UnitTests/Core/FileIOPluginTests.cs index 0d43603f54d3..50b44004233d 100644 --- a/dotnet/src/Plugins/Plugins.UnitTests/Core/FileIOPluginTests.cs +++ b/dotnet/src/Plugins/Plugins.UnitTests/Core/FileIOPluginTests.cs @@ -29,7 +29,7 @@ public void ItCanBeImported() public async Task ItCanReadAsync() { // Arrange - var plugin = new FileIOPlugin(); + var plugin = new FileIOPlugin() { AllowedFolders = [Path.GetTempPath()] }; var path = Path.GetTempFileName(); await File.WriteAllTextAsync(path, "hello world"); @@ -44,7 +44,7 @@ public async Task ItCanReadAsync() public async Task ItCannotReadAsync() { // Arrange - var plugin = new FileIOPlugin(); + var plugin = new FileIOPlugin() { AllowedFolders = [Path.GetTempPath()] }; var path = Path.GetTempFileName(); File.Delete(path); @@ -62,7 +62,11 @@ Task Fn() public async Task ItCanWriteAsync() { // Arrange - var plugin = new FileIOPlugin(); + var plugin = new FileIOPlugin() + { + AllowedFolders = [Path.GetTempPath()], + DisableFileOverwrite = false + }; var path = Path.GetTempFileName(); // Act @@ -76,7 +80,11 @@ public async Task ItCanWriteAsync() public async Task ItCannotWriteAsync() { // Arrange - var plugin = new FileIOPlugin(); + var plugin = new FileIOPlugin() + { + AllowedFolders = [Path.GetTempPath()], + DisableFileOverwrite = false + }; var path = Path.GetTempFileName(); File.SetAttributes(path, FileAttributes.ReadOnly); @@ -90,13 +98,26 @@ Task Fn() _ = await Assert.ThrowsAsync(Fn); } + [Fact] + public async Task ItDeniesAllPathsWithDefaultConfigAsync() + { + // Arrange + var plugin = new FileIOPlugin(); + var path = Path.GetTempFileName(); + + // Act & Assert - default config denies all paths + await Assert.ThrowsAsync(async () => await plugin.ReadAsync(path)); + await Assert.ThrowsAsync(async () => await plugin.WriteAsync(path, "hello world")); + } + [Fact] public async Task ItCannotWriteToDisallowedFoldersAsync() { // Arrange var plugin = new FileIOPlugin() { - AllowedFolders = [Path.GetTempPath()] + AllowedFolders = [Path.GetTempPath()], + DisableFileOverwrite = false }; // Act & Assert diff --git a/dotnet/src/Plugins/Plugins.UnitTests/Core/HttpPluginTests.cs b/dotnet/src/Plugins/Plugins.UnitTests/Core/HttpPluginTests.cs index b64d87671bb0..d867e56c0630 100644 --- a/dotnet/src/Plugins/Plugins.UnitTests/Core/HttpPluginTests.cs +++ b/dotnet/src/Plugins/Plugins.UnitTests/Core/HttpPluginTests.cs @@ -44,7 +44,7 @@ public async Task ItCanGetAsync() // Arrange var mockHandler = this.CreateMock(); using var client = new HttpClient(mockHandler.Object); - var plugin = new HttpPlugin(client); + var plugin = new HttpPlugin(client) { AllowedDomains = ["www.example.com"] }; // Act var result = await plugin.GetAsync(this._uriString); @@ -60,7 +60,7 @@ public async Task ItCanPostAsync() // Arrange var mockHandler = this.CreateMock(); using var client = new HttpClient(mockHandler.Object); - var plugin = new HttpPlugin(client); + var plugin = new HttpPlugin(client) { AllowedDomains = ["www.example.com"] }; // Act var result = await plugin.PostAsync(this._uriString, this._content); @@ -76,7 +76,7 @@ public async Task ItCanPutAsync() // Arrange var mockHandler = this.CreateMock(); using var client = new HttpClient(mockHandler.Object); - var plugin = new HttpPlugin(client); + var plugin = new HttpPlugin(client) { AllowedDomains = ["www.example.com"] }; // Act var result = await plugin.PutAsync(this._uriString, this._content); @@ -92,7 +92,7 @@ public async Task ItCanDeleteAsync() // Arrange var mockHandler = this.CreateMock(); using var client = new HttpClient(mockHandler.Object); - var plugin = new HttpPlugin(client); + var plugin = new HttpPlugin(client) { AllowedDomains = ["www.example.com"] }; // Act var result = await plugin.DeleteAsync(this._uriString); @@ -102,6 +102,18 @@ public async Task ItCanDeleteAsync() this.VerifyMock(mockHandler, HttpMethod.Delete); } + [Fact] + public async Task ItDeniesAllDomainsWithDefaultConfigAsync() + { + // Arrange + var mockHandler = this.CreateMock(); + using var client = new HttpClient(mockHandler.Object); + var plugin = new HttpPlugin(client); + + // Act & Assert - default config denies all domains + await Assert.ThrowsAsync(async () => await plugin.GetAsync(this._uriString)); + } + [Fact] public async Task ItThrowsInvalidOperationExceptionForInvalidDomainAsync() { diff --git a/dotnet/src/Plugins/Plugins.UnitTests/Web/WebFileDownloadPluginTests.cs b/dotnet/src/Plugins/Plugins.UnitTests/Web/WebFileDownloadPluginTests.cs index 98ae88dcee64..df8c3b5d76c5 100644 --- a/dotnet/src/Plugins/Plugins.UnitTests/Web/WebFileDownloadPluginTests.cs +++ b/dotnet/src/Plugins/Plugins.UnitTests/Web/WebFileDownloadPluginTests.cs @@ -77,13 +77,46 @@ public async Task DownloadToFileFailsForInvalidDomainAsync() } [Fact] - public void ValidatePluginProperties() + public async Task DownloadToFileDeniesAllWithDefaultConfigAsync() { // Arrange + this._messageHandlerStub.AddImageResponse(File.ReadAllBytes(SKLogoPng)); + var uri = new Uri("https://raw.githubusercontent.com/microsoft/semantic-kernel/refs/heads/main/docs/images/sk_logo.png"); + var folderPath = Path.Combine(Path.GetTempPath(), Guid.NewGuid().ToString()); + var filePath = Path.Combine(folderPath, "sk_logo.png"); + Directory.CreateDirectory(folderPath); + + var webFileDownload = new WebFileDownloadPlugin(); + + try + { + // Act & Assert - default config denies all domains + await Assert.ThrowsAsync(async () => await webFileDownload.DownloadToFileAsync(uri, filePath)); + } + finally + { + if (Path.Exists(folderPath)) + { + Directory.Delete(folderPath, true); + } + } + } + + [Fact] + public void ValidatePluginProperties() + { + // Arrange & Act - verify secure defaults + var defaultPlugin = new WebFileDownloadPlugin(); + + // Assert - defaults are deny-all + Assert.Empty(defaultPlugin.AllowedDomains!); + Assert.Empty(defaultPlugin.AllowedFolders!); + Assert.True(defaultPlugin.DisableFileOverwrite); + Assert.Equal(10 * 1024 * 1024, defaultPlugin.MaximumDownloadSize); + + // Arrange & Act - verify explicit configuration var folderPath = Path.Combine(Path.GetTempPath(), Guid.NewGuid().ToString()); - var filePath = Path.Combine(Path.GetTempPath(), "sk_logo.png"); - // Act var webFileDownload = new WebFileDownloadPlugin() { AllowedDomains = ["raw.githubusercontent.com"], @@ -92,7 +125,7 @@ public void ValidatePluginProperties() DisableFileOverwrite = true }; - // Act & Assert + // Assert Assert.Equal(["raw.githubusercontent.com"], webFileDownload.AllowedDomains); Assert.Equal([folderPath], webFileDownload.AllowedFolders); Assert.Equal(100, webFileDownload.MaximumDownloadSize); @@ -124,7 +157,9 @@ public async Task DownloadToFileFailsForInvalidParametersAsync() await Assert.ThrowsAsync(async () => await webFileDownload.DownloadToFileAsync(validUri, invalidFilePath)); await Assert.ThrowsAsync(async () => await webFileDownload.DownloadToFileAsync(validUri, "\\\\UNC\\server\\folder\\myfile.txt")); await Assert.ThrowsAsync(async () => await webFileDownload.DownloadToFileAsync(validUri, "")); - await Assert.ThrowsAsync(async () => await webFileDownload.DownloadToFileAsync(validUri, "myfile.txt")); + // Relative paths are now canonicalized to absolute paths via Path.GetFullPath, + // so they are caught by the AllowedFolders check rather than the "fully qualified" check. + await Assert.ThrowsAsync(async () => await webFileDownload.DownloadToFileAsync(validUri, "myfile.txt")); } /// diff --git a/dotnet/src/Plugins/Plugins.Web/WebFileDownloadPlugin.cs b/dotnet/src/Plugins/Plugins.Web/WebFileDownloadPlugin.cs index 6681a2081bb2..b420b07008a1 100644 --- a/dotnet/src/Plugins/Plugins.Web/WebFileDownloadPlugin.cs +++ b/dotnet/src/Plugins/Plugins.Web/WebFileDownloadPlugin.cs @@ -17,6 +17,19 @@ namespace Microsoft.SemanticKernel.Plugins.Web; /// /// Plugin to download web files. /// +/// +/// +/// This plugin is secure by default. Both and +/// must be explicitly configured before any downloads are permitted. By default, all domains and all +/// file paths are denied. +/// +/// +/// When exposing this plugin to an LLM via auto function calling, ensure that +/// and are restricted to trusted +/// values only. Unrestricted configuration may allow unintended downloads to +/// local paths. +/// +/// public sealed class WebFileDownloadPlugin { /// @@ -47,6 +60,10 @@ public WebFileDownloadPlugin(HttpClient httpClient, ILoggerFactory? loggerFactor /// /// List of allowed domains to download from. /// + /// + /// Defaults to an empty collection (no domains allowed). Must be explicitly populated + /// with trusted domains before any downloads will succeed. + /// public IEnumerable? AllowedDomains { get => this._allowedDomains; @@ -54,8 +71,13 @@ public IEnumerable? AllowedDomains } /// - /// List of allowed folders to download to. + /// List of allowed folders to download to. Subdirectories of allowed folders are also permitted. /// + /// + /// Defaults to an empty collection (no folders allowed). Must be explicitly populated + /// with trusted directory paths before any downloads will succeed. + /// Paths are canonicalized before validation to prevent directory traversal. + /// public IEnumerable? AllowedFolders { get => this._allowedFolders; @@ -63,14 +85,20 @@ public IEnumerable? AllowedFolders } /// - /// Set to true to disable overwriting existing files. + /// Set to false to allow overwriting existing files. /// - public bool DisableFileOverwrite { get; set; } = false; + /// + /// Defaults to true (overwriting is disabled). + /// + public bool DisableFileOverwrite { get; set; } = true; /// /// Set the maximum allowed download size. /// - public long MaximumDownloadSize { get; set; } = long.MaxValue; + /// + /// Defaults to 10 MB. + /// + public long MaximumDownloadSize { get; set; } = 10 * 1024 * 1024; /// /// Downloads a file to a local file path. @@ -94,12 +122,17 @@ public async Task DownloadToFileAsync( throw new InvalidOperationException("Downloading from the provided location is not allowed."); } - var expandedFilePath = Environment.ExpandEnvironmentVariables(filePath); + var expandedFilePath = Path.GetFullPath(Environment.ExpandEnvironmentVariables(filePath)); if (!this.IsFilePathAllowed(expandedFilePath)) { throw new InvalidOperationException("Downloading to the provided location is not allowed."); } + if (this.DisableFileOverwrite && File.Exists(expandedFilePath)) + { + throw new InvalidOperationException("Overwriting existing files is disabled."); + } + using HttpRequestMessage request = new(HttpMethod.Get, url); using HttpResponseMessage response = await this._httpClient.SendWithSuccessCheckAsync(request, HttpCompletionOption.ResponseHeadersRead, cancellationToken).ConfigureAwait(false); @@ -115,7 +148,7 @@ public async Task DownloadToFileAsync( var fileMode = this.DisableFileOverwrite ? FileMode.CreateNew : FileMode.Create; using Stream source = await response.Content.ReadAsStreamAndTranslateExceptionAsync(cancellationToken).ConfigureAwait(false); - using FileStream destination = new(expandedFilePath, FileMode.Create); + using FileStream destination = new(expandedFilePath, fileMode); int bufferSize = 81920; byte[] buffer = ArrayPool.Shared.Rent(81920); @@ -150,8 +183,8 @@ public async Task DownloadToFileAsync( #region private private readonly ILogger _logger; private readonly HttpClient _httpClient; - private HashSet? _allowedDomains; - private HashSet? _allowedFolders; + private HashSet? _allowedDomains = []; + private HashSet? _allowedFolders = []; /// /// If a list of allowed domains has been provided, the host of the provided uri is checked @@ -161,12 +194,15 @@ private bool IsUriAllowed(Uri uri) { Verify.NotNull(uri); - return this._allowedDomains is null || this._allowedDomains.Contains(uri.Host); + return this._allowedDomains is not null + && this._allowedDomains.Count > 0 + && this._allowedDomains.Contains(uri.Host); } /// /// If a list of allowed folder has been provided, the folder of the provided filePath is checked - /// to verify it is in the allowed folder list. + /// to verify it is in the allowed folder list. Paths are canonicalized before comparison. + /// Subdirectories of allowed folders are also permitted. /// private bool IsFilePathAllowed(string path) { @@ -177,11 +213,6 @@ private bool IsFilePathAllowed(string path) throw new ArgumentException("Invalid file path, UNC paths are not supported.", nameof(path)); } - if (this.DisableFileOverwrite && File.Exists(path)) - { - throw new ArgumentException("Invalid file path, overwriting existing files is disabled.", nameof(path)); - } - string? directoryPath = Path.GetDirectoryName(path); if (string.IsNullOrEmpty(directoryPath)) @@ -195,7 +226,30 @@ private bool IsFilePathAllowed(string path) throw new UnauthorizedAccessException($"File is read-only: {path}"); } - return this._allowedFolders is null || this._allowedFolders.Contains(directoryPath); + if (this._allowedFolders is null || this._allowedFolders.Count == 0) + { + return false; + } + + var canonicalDir = Path.GetFullPath(directoryPath); + + foreach (var allowedFolder in this._allowedFolders) + { + var canonicalAllowed = Path.GetFullPath(allowedFolder); + var separator = Path.DirectorySeparatorChar.ToString(); + if (!canonicalAllowed.EndsWith(separator, StringComparison.OrdinalIgnoreCase)) + { + canonicalAllowed += separator; + } + + if (canonicalDir.StartsWith(canonicalAllowed, StringComparison.OrdinalIgnoreCase) + || (canonicalDir + separator).Equals(canonicalAllowed, StringComparison.OrdinalIgnoreCase)) + { + return true; + } + } + + return false; } #endregion } From 68ea5a9f618ab03000fa6beec68c3d3b43e650cd Mon Sep 17 00:00:00 2001 From: Roger Barreto <19890735+rogerbarreto@users.noreply.github.com> Date: Tue, 10 Mar 2026 12:36:36 +0000 Subject: [PATCH 2/2] .Net: Harden plugin security defaults for WebFileDownloadPlugin, HttpPlugin, and FileIOPlugin - Change AllowedDomains and AllowedFolders defaults from null (allow-all) to empty (deny-all) - Change DisableFileOverwrite default to true - Set MaximumDownloadSize default to 10 MB - Add path canonicalization via Path.GetFullPath() to prevent directory traversal - Switch folder matching from exact to prefix-based with separator handling - Fix fileMode variable not being used in FileStream constructor - Add XML doc security remarks to all three plugins - Update tests for new defaults Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- .../src/Plugins/Plugins.Core/FileIOPlugin.cs | 66 +++++++++++--- dotnet/src/Plugins/Plugins.Core/HttpPlugin.cs | 22 ++++- .../Core/FileIOPluginTests.cs | 31 +++++-- .../Plugins.UnitTests/Core/HttpPluginTests.cs | 20 ++++- .../Web/WebFileDownloadPluginTests.cs | 49 +++++++++-- .../Plugins.Web/WebFileDownloadPlugin.cs | 86 +++++++++++++++---- 6 files changed, 228 insertions(+), 46 deletions(-) diff --git a/dotnet/src/Plugins/Plugins.Core/FileIOPlugin.cs b/dotnet/src/Plugins/Plugins.Core/FileIOPlugin.cs index 6863ca886c41..9a94a08538a0 100644 --- a/dotnet/src/Plugins/Plugins.Core/FileIOPlugin.cs +++ b/dotnet/src/Plugins/Plugins.Core/FileIOPlugin.cs @@ -12,11 +12,26 @@ namespace Microsoft.SemanticKernel.Plugins.Core; /// /// Read and write from a file. /// +/// +/// +/// This plugin is secure by default. must be explicitly configured +/// before any file operations are permitted. By default, all file paths are denied. +/// +/// +/// When exposing this plugin to an LLM via auto function calling, ensure that +/// is restricted to trusted values only. +/// +/// public sealed class FileIOPlugin { /// - /// List of allowed folders to read from or write to. + /// List of allowed folders to read from or write to. Subdirectories of allowed folders are also permitted. /// + /// + /// Defaults to an empty collection (no folders allowed). Must be explicitly populated + /// with trusted directory paths before any file operations will succeed. + /// Paths are canonicalized before validation to prevent directory traversal. + /// public IEnumerable? AllowedFolders { get => this._allowedFolders; @@ -24,9 +39,12 @@ public IEnumerable? AllowedFolders } /// - /// Set to true to disable overwriting existing files. + /// Set to false to allow overwriting existing files. /// - public bool DisableFileOverwrite { get; set; } = false; + /// + /// Defaults to true (overwriting is disabled). + /// + public bool DisableFileOverwrite { get; set; } = true; /// /// Read a file @@ -67,6 +85,11 @@ public async Task WriteAsync( throw new InvalidOperationException("Writing to the provided location is not allowed."); } + if (this.DisableFileOverwrite && File.Exists(path)) + { + throw new InvalidOperationException("Overwriting existing files is disabled."); + } + byte[] text = Encoding.UTF8.GetBytes(content); var fileMode = this.DisableFileOverwrite ? FileMode.CreateNew : FileMode.Create; using var writer = new FileStream(path, fileMode, FileAccess.Write, FileShare.None); @@ -78,11 +101,12 @@ await writer.WriteAsync(text } #region private - private HashSet? _allowedFolders; + private HashSet? _allowedFolders = []; /// - /// If a list of allowed folder has been provided, the folder of the provided path is checked - /// to verify it is in the allowed folder list. + /// If a list of allowed folder has been provided, the folder of the provided filePath is checked + /// to verify it is in the allowed folder list. Paths are canonicalized before comparison. + /// Subdirectories of allowed folders are also permitted. /// private bool IsFilePathAllowed(string path) { @@ -93,11 +117,6 @@ private bool IsFilePathAllowed(string path) throw new ArgumentException("Invalid file path, UNC paths are not supported.", nameof(path)); } - if (this.DisableFileOverwrite && File.Exists(path)) - { - throw new ArgumentException("Invalid file path, overwriting existing files is disabled.", nameof(path)); - } - string? directoryPath = Path.GetDirectoryName(path); if (string.IsNullOrEmpty(directoryPath)) @@ -111,7 +130,30 @@ private bool IsFilePathAllowed(string path) throw new UnauthorizedAccessException($"File is read-only: {path}"); } - return this._allowedFolders is null || this._allowedFolders.Contains(directoryPath); + if (this._allowedFolders is null || this._allowedFolders.Count == 0) + { + return false; + } + + var canonicalDir = Path.GetFullPath(directoryPath); + + foreach (var allowedFolder in this._allowedFolders) + { + var canonicalAllowed = Path.GetFullPath(allowedFolder); + var separator = Path.DirectorySeparatorChar.ToString(); + if (!canonicalAllowed.EndsWith(separator, StringComparison.OrdinalIgnoreCase)) + { + canonicalAllowed += separator; + } + + if (canonicalDir.StartsWith(canonicalAllowed, StringComparison.OrdinalIgnoreCase) + || (canonicalDir + separator).Equals(canonicalAllowed, StringComparison.OrdinalIgnoreCase)) + { + return true; + } + } + + return false; } #endregion } diff --git a/dotnet/src/Plugins/Plugins.Core/HttpPlugin.cs b/dotnet/src/Plugins/Plugins.Core/HttpPlugin.cs index ed3d9635f8a1..89f49d8d4705 100644 --- a/dotnet/src/Plugins/Plugins.Core/HttpPlugin.cs +++ b/dotnet/src/Plugins/Plugins.Core/HttpPlugin.cs @@ -14,6 +14,16 @@ namespace Microsoft.SemanticKernel.Plugins.Core; /// /// A plugin that provides HTTP functionality. /// +/// +/// +/// This plugin is secure by default. must be explicitly configured +/// before any HTTP requests are permitted. By default, all domains are denied. +/// +/// +/// When exposing this plugin to an LLM via auto function calling, ensure that +/// is restricted to trusted values only. +/// +/// [System.Diagnostics.CodeAnalysis.SuppressMessage("Design", "CA1054:URI-like parameters should not be strings", Justification = "Semantic Kernel operates on strings")] public sealed class HttpPlugin @@ -39,8 +49,12 @@ public HttpPlugin(HttpClient? client = null) => this._client = client ?? HttpClientProvider.GetHttpClient(); /// - /// List of allowed domains to download from. + /// List of allowed domains to send requests to. /// + /// + /// Defaults to an empty collection (no domains allowed). Must be explicitly populated + /// with trusted domains before any requests will succeed. + /// public IEnumerable? AllowedDomains { get => this._allowedDomains; @@ -100,7 +114,7 @@ public Task DeleteAsync( this.SendRequestAsync(uri, HttpMethod.Delete, requestContent: null, cancellationToken); #region private - private HashSet? _allowedDomains; + private HashSet? _allowedDomains = []; /// /// If a list of allowed domains has been provided, the host of the provided uri is checked @@ -110,7 +124,9 @@ private bool IsUriAllowed(Uri uri) { Verify.NotNull(uri); - return this._allowedDomains is null || this._allowedDomains.Contains(uri.Host); + return this._allowedDomains is not null + && this._allowedDomains.Count > 0 + && this._allowedDomains.Contains(uri.Host); } /// Sends an HTTP request and returns the response content as a string. diff --git a/dotnet/src/Plugins/Plugins.UnitTests/Core/FileIOPluginTests.cs b/dotnet/src/Plugins/Plugins.UnitTests/Core/FileIOPluginTests.cs index 0d43603f54d3..50b44004233d 100644 --- a/dotnet/src/Plugins/Plugins.UnitTests/Core/FileIOPluginTests.cs +++ b/dotnet/src/Plugins/Plugins.UnitTests/Core/FileIOPluginTests.cs @@ -29,7 +29,7 @@ public void ItCanBeImported() public async Task ItCanReadAsync() { // Arrange - var plugin = new FileIOPlugin(); + var plugin = new FileIOPlugin() { AllowedFolders = [Path.GetTempPath()] }; var path = Path.GetTempFileName(); await File.WriteAllTextAsync(path, "hello world"); @@ -44,7 +44,7 @@ public async Task ItCanReadAsync() public async Task ItCannotReadAsync() { // Arrange - var plugin = new FileIOPlugin(); + var plugin = new FileIOPlugin() { AllowedFolders = [Path.GetTempPath()] }; var path = Path.GetTempFileName(); File.Delete(path); @@ -62,7 +62,11 @@ Task Fn() public async Task ItCanWriteAsync() { // Arrange - var plugin = new FileIOPlugin(); + var plugin = new FileIOPlugin() + { + AllowedFolders = [Path.GetTempPath()], + DisableFileOverwrite = false + }; var path = Path.GetTempFileName(); // Act @@ -76,7 +80,11 @@ public async Task ItCanWriteAsync() public async Task ItCannotWriteAsync() { // Arrange - var plugin = new FileIOPlugin(); + var plugin = new FileIOPlugin() + { + AllowedFolders = [Path.GetTempPath()], + DisableFileOverwrite = false + }; var path = Path.GetTempFileName(); File.SetAttributes(path, FileAttributes.ReadOnly); @@ -90,13 +98,26 @@ Task Fn() _ = await Assert.ThrowsAsync(Fn); } + [Fact] + public async Task ItDeniesAllPathsWithDefaultConfigAsync() + { + // Arrange + var plugin = new FileIOPlugin(); + var path = Path.GetTempFileName(); + + // Act & Assert - default config denies all paths + await Assert.ThrowsAsync(async () => await plugin.ReadAsync(path)); + await Assert.ThrowsAsync(async () => await plugin.WriteAsync(path, "hello world")); + } + [Fact] public async Task ItCannotWriteToDisallowedFoldersAsync() { // Arrange var plugin = new FileIOPlugin() { - AllowedFolders = [Path.GetTempPath()] + AllowedFolders = [Path.GetTempPath()], + DisableFileOverwrite = false }; // Act & Assert diff --git a/dotnet/src/Plugins/Plugins.UnitTests/Core/HttpPluginTests.cs b/dotnet/src/Plugins/Plugins.UnitTests/Core/HttpPluginTests.cs index b64d87671bb0..d867e56c0630 100644 --- a/dotnet/src/Plugins/Plugins.UnitTests/Core/HttpPluginTests.cs +++ b/dotnet/src/Plugins/Plugins.UnitTests/Core/HttpPluginTests.cs @@ -44,7 +44,7 @@ public async Task ItCanGetAsync() // Arrange var mockHandler = this.CreateMock(); using var client = new HttpClient(mockHandler.Object); - var plugin = new HttpPlugin(client); + var plugin = new HttpPlugin(client) { AllowedDomains = ["www.example.com"] }; // Act var result = await plugin.GetAsync(this._uriString); @@ -60,7 +60,7 @@ public async Task ItCanPostAsync() // Arrange var mockHandler = this.CreateMock(); using var client = new HttpClient(mockHandler.Object); - var plugin = new HttpPlugin(client); + var plugin = new HttpPlugin(client) { AllowedDomains = ["www.example.com"] }; // Act var result = await plugin.PostAsync(this._uriString, this._content); @@ -76,7 +76,7 @@ public async Task ItCanPutAsync() // Arrange var mockHandler = this.CreateMock(); using var client = new HttpClient(mockHandler.Object); - var plugin = new HttpPlugin(client); + var plugin = new HttpPlugin(client) { AllowedDomains = ["www.example.com"] }; // Act var result = await plugin.PutAsync(this._uriString, this._content); @@ -92,7 +92,7 @@ public async Task ItCanDeleteAsync() // Arrange var mockHandler = this.CreateMock(); using var client = new HttpClient(mockHandler.Object); - var plugin = new HttpPlugin(client); + var plugin = new HttpPlugin(client) { AllowedDomains = ["www.example.com"] }; // Act var result = await plugin.DeleteAsync(this._uriString); @@ -102,6 +102,18 @@ public async Task ItCanDeleteAsync() this.VerifyMock(mockHandler, HttpMethod.Delete); } + [Fact] + public async Task ItDeniesAllDomainsWithDefaultConfigAsync() + { + // Arrange + var mockHandler = this.CreateMock(); + using var client = new HttpClient(mockHandler.Object); + var plugin = new HttpPlugin(client); + + // Act & Assert - default config denies all domains + await Assert.ThrowsAsync(async () => await plugin.GetAsync(this._uriString)); + } + [Fact] public async Task ItThrowsInvalidOperationExceptionForInvalidDomainAsync() { diff --git a/dotnet/src/Plugins/Plugins.UnitTests/Web/WebFileDownloadPluginTests.cs b/dotnet/src/Plugins/Plugins.UnitTests/Web/WebFileDownloadPluginTests.cs index 98ae88dcee64..7a552dc27529 100644 --- a/dotnet/src/Plugins/Plugins.UnitTests/Web/WebFileDownloadPluginTests.cs +++ b/dotnet/src/Plugins/Plugins.UnitTests/Web/WebFileDownloadPluginTests.cs @@ -77,13 +77,46 @@ public async Task DownloadToFileFailsForInvalidDomainAsync() } [Fact] - public void ValidatePluginProperties() + public async Task DownloadToFileDeniesAllWithDefaultConfigAsync() { // Arrange + this._messageHandlerStub.AddImageResponse(File.ReadAllBytes(SKLogoPng)); + var uri = new Uri("https://raw.githubusercontent.com/microsoft/semantic-kernel/refs/heads/main/docs/images/sk_logo.png"); + var folderPath = Path.Combine(Path.GetTempPath(), Guid.NewGuid().ToString()); + var filePath = Path.Combine(folderPath, "sk_logo.png"); + Directory.CreateDirectory(folderPath); + + var webFileDownload = new WebFileDownloadPlugin(); + + try + { + // Act & Assert - default config denies all domains + await Assert.ThrowsAsync(async () => await webFileDownload.DownloadToFileAsync(uri, filePath)); + } + finally + { + if (Path.Exists(folderPath)) + { + Directory.Delete(folderPath, true); + } + } + } + + [Fact] + public void ValidatePluginProperties() + { + // Arrange & Act - verify secure defaults + var defaultPlugin = new WebFileDownloadPlugin(); + + // Assert - defaults are deny-all + Assert.Empty(defaultPlugin.AllowedDomains!); + Assert.Empty(defaultPlugin.AllowedFolders!); + Assert.True(defaultPlugin.DisableFileOverwrite); + Assert.Equal(10 * 1024 * 1024, defaultPlugin.MaximumDownloadSize); + + // Arrange & Act - verify explicit configuration var folderPath = Path.Combine(Path.GetTempPath(), Guid.NewGuid().ToString()); - var filePath = Path.Combine(Path.GetTempPath(), "sk_logo.png"); - // Act var webFileDownload = new WebFileDownloadPlugin() { AllowedDomains = ["raw.githubusercontent.com"], @@ -92,7 +125,7 @@ public void ValidatePluginProperties() DisableFileOverwrite = true }; - // Act & Assert + // Assert Assert.Equal(["raw.githubusercontent.com"], webFileDownload.AllowedDomains); Assert.Equal([folderPath], webFileDownload.AllowedFolders); Assert.Equal(100, webFileDownload.MaximumDownloadSize); @@ -122,9 +155,13 @@ public async Task DownloadToFileFailsForInvalidParametersAsync() await Assert.ThrowsAsync(async () => await webFileDownload.DownloadToFileAsync(validUri, validFilePath)); await Assert.ThrowsAsync(async () => await webFileDownload.DownloadToFileAsync(invalidUri, validFilePath)); await Assert.ThrowsAsync(async () => await webFileDownload.DownloadToFileAsync(validUri, invalidFilePath)); - await Assert.ThrowsAsync(async () => await webFileDownload.DownloadToFileAsync(validUri, "\\\\UNC\\server\\folder\\myfile.txt")); + // UNC paths are rejected as ArgumentException on Windows; on Linux Path.GetFullPath + // canonicalizes them to a regular path, so they are caught by the AllowedFolders check instead. + await Assert.ThrowsAnyAsync(async () => await webFileDownload.DownloadToFileAsync(validUri, "\\\\UNC\\server\\folder\\myfile.txt")); await Assert.ThrowsAsync(async () => await webFileDownload.DownloadToFileAsync(validUri, "")); - await Assert.ThrowsAsync(async () => await webFileDownload.DownloadToFileAsync(validUri, "myfile.txt")); + // Relative paths are now canonicalized to absolute paths via Path.GetFullPath, + // so they are caught by the AllowedFolders check rather than the "fully qualified" check. + await Assert.ThrowsAsync(async () => await webFileDownload.DownloadToFileAsync(validUri, "myfile.txt")); } /// diff --git a/dotnet/src/Plugins/Plugins.Web/WebFileDownloadPlugin.cs b/dotnet/src/Plugins/Plugins.Web/WebFileDownloadPlugin.cs index 6681a2081bb2..b420b07008a1 100644 --- a/dotnet/src/Plugins/Plugins.Web/WebFileDownloadPlugin.cs +++ b/dotnet/src/Plugins/Plugins.Web/WebFileDownloadPlugin.cs @@ -17,6 +17,19 @@ namespace Microsoft.SemanticKernel.Plugins.Web; /// /// Plugin to download web files. /// +/// +/// +/// This plugin is secure by default. Both and +/// must be explicitly configured before any downloads are permitted. By default, all domains and all +/// file paths are denied. +/// +/// +/// When exposing this plugin to an LLM via auto function calling, ensure that +/// and are restricted to trusted +/// values only. Unrestricted configuration may allow unintended downloads to +/// local paths. +/// +/// public sealed class WebFileDownloadPlugin { /// @@ -47,6 +60,10 @@ public WebFileDownloadPlugin(HttpClient httpClient, ILoggerFactory? loggerFactor /// /// List of allowed domains to download from. /// + /// + /// Defaults to an empty collection (no domains allowed). Must be explicitly populated + /// with trusted domains before any downloads will succeed. + /// public IEnumerable? AllowedDomains { get => this._allowedDomains; @@ -54,8 +71,13 @@ public IEnumerable? AllowedDomains } /// - /// List of allowed folders to download to. + /// List of allowed folders to download to. Subdirectories of allowed folders are also permitted. /// + /// + /// Defaults to an empty collection (no folders allowed). Must be explicitly populated + /// with trusted directory paths before any downloads will succeed. + /// Paths are canonicalized before validation to prevent directory traversal. + /// public IEnumerable? AllowedFolders { get => this._allowedFolders; @@ -63,14 +85,20 @@ public IEnumerable? AllowedFolders } /// - /// Set to true to disable overwriting existing files. + /// Set to false to allow overwriting existing files. /// - public bool DisableFileOverwrite { get; set; } = false; + /// + /// Defaults to true (overwriting is disabled). + /// + public bool DisableFileOverwrite { get; set; } = true; /// /// Set the maximum allowed download size. /// - public long MaximumDownloadSize { get; set; } = long.MaxValue; + /// + /// Defaults to 10 MB. + /// + public long MaximumDownloadSize { get; set; } = 10 * 1024 * 1024; /// /// Downloads a file to a local file path. @@ -94,12 +122,17 @@ public async Task DownloadToFileAsync( throw new InvalidOperationException("Downloading from the provided location is not allowed."); } - var expandedFilePath = Environment.ExpandEnvironmentVariables(filePath); + var expandedFilePath = Path.GetFullPath(Environment.ExpandEnvironmentVariables(filePath)); if (!this.IsFilePathAllowed(expandedFilePath)) { throw new InvalidOperationException("Downloading to the provided location is not allowed."); } + if (this.DisableFileOverwrite && File.Exists(expandedFilePath)) + { + throw new InvalidOperationException("Overwriting existing files is disabled."); + } + using HttpRequestMessage request = new(HttpMethod.Get, url); using HttpResponseMessage response = await this._httpClient.SendWithSuccessCheckAsync(request, HttpCompletionOption.ResponseHeadersRead, cancellationToken).ConfigureAwait(false); @@ -115,7 +148,7 @@ public async Task DownloadToFileAsync( var fileMode = this.DisableFileOverwrite ? FileMode.CreateNew : FileMode.Create; using Stream source = await response.Content.ReadAsStreamAndTranslateExceptionAsync(cancellationToken).ConfigureAwait(false); - using FileStream destination = new(expandedFilePath, FileMode.Create); + using FileStream destination = new(expandedFilePath, fileMode); int bufferSize = 81920; byte[] buffer = ArrayPool.Shared.Rent(81920); @@ -150,8 +183,8 @@ public async Task DownloadToFileAsync( #region private private readonly ILogger _logger; private readonly HttpClient _httpClient; - private HashSet? _allowedDomains; - private HashSet? _allowedFolders; + private HashSet? _allowedDomains = []; + private HashSet? _allowedFolders = []; /// /// If a list of allowed domains has been provided, the host of the provided uri is checked @@ -161,12 +194,15 @@ private bool IsUriAllowed(Uri uri) { Verify.NotNull(uri); - return this._allowedDomains is null || this._allowedDomains.Contains(uri.Host); + return this._allowedDomains is not null + && this._allowedDomains.Count > 0 + && this._allowedDomains.Contains(uri.Host); } /// /// If a list of allowed folder has been provided, the folder of the provided filePath is checked - /// to verify it is in the allowed folder list. + /// to verify it is in the allowed folder list. Paths are canonicalized before comparison. + /// Subdirectories of allowed folders are also permitted. /// private bool IsFilePathAllowed(string path) { @@ -177,11 +213,6 @@ private bool IsFilePathAllowed(string path) throw new ArgumentException("Invalid file path, UNC paths are not supported.", nameof(path)); } - if (this.DisableFileOverwrite && File.Exists(path)) - { - throw new ArgumentException("Invalid file path, overwriting existing files is disabled.", nameof(path)); - } - string? directoryPath = Path.GetDirectoryName(path); if (string.IsNullOrEmpty(directoryPath)) @@ -195,7 +226,30 @@ private bool IsFilePathAllowed(string path) throw new UnauthorizedAccessException($"File is read-only: {path}"); } - return this._allowedFolders is null || this._allowedFolders.Contains(directoryPath); + if (this._allowedFolders is null || this._allowedFolders.Count == 0) + { + return false; + } + + var canonicalDir = Path.GetFullPath(directoryPath); + + foreach (var allowedFolder in this._allowedFolders) + { + var canonicalAllowed = Path.GetFullPath(allowedFolder); + var separator = Path.DirectorySeparatorChar.ToString(); + if (!canonicalAllowed.EndsWith(separator, StringComparison.OrdinalIgnoreCase)) + { + canonicalAllowed += separator; + } + + if (canonicalDir.StartsWith(canonicalAllowed, StringComparison.OrdinalIgnoreCase) + || (canonicalDir + separator).Equals(canonicalAllowed, StringComparison.OrdinalIgnoreCase)) + { + return true; + } + } + + return false; } #endregion }