Skip to content

Commit 1a5065e

Browse files
SergeyMenshykhCopilot
SergeyMenshykh
and
Copilot
authored
.Net: Fix DocumentPlugin path validation order (#13956)
### Motivation and Context Fix path validation order in DocumentPlugin to canonicalize paths before validating against AllowedDirectories. Also removes redundant path transformation from LocalFileSystemConnector. ### Description - Canonicalize file paths (expand environment variables and resolve to absolute form) **before** validating against AllowedDirectories in both `ReadTextAsync` and `AppendTextAsync` - Remove environment variable expansion from `LocalFileSystemConnector` the connector should operate on the path it receives, not transform it - Add regression tests ### Contribution Checklist - [x] The code builds clean without any errors or warnings - [x] The PR follows the [SK Contribution Guidelines](https://github.com/microsoft/semantic-kernel/blob/main/CONTRIBUTING.md) - [x] The existing tests pass - [x] New tests added --------- Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
1 parent 2a719ca commit 1a5065e

3 files changed

Lines changed: 179 additions & 29 deletions

File tree

‎dotnet/src/Plugins/Plugins.Document/DocumentPlugin.cs‎

Lines changed: 49 additions & 21 deletions
Original file line numberDiff line numberDiff line change
@@ -4,6 +4,7 @@
44
using System.Collections.Generic;
55
using System.ComponentModel;
66
using System.IO;
7+
using System.Runtime.InteropServices;
78
using System.Threading;
89
using System.Threading.Tasks;
910
using Microsoft.Extensions.Logging;
@@ -87,14 +88,16 @@ public async Task<string> ReadTextAsync(
8788
[Description("Path to the file to read")] string filePath,
8889
CancellationToken cancellationToken = default)
8990
{
90-
this._logger.LogDebug("Reading text from {0}", filePath);
91+
var canonicalPath = CanonicalizePath(filePath);
9192

92-
if (!this.IsFilePathAllowed(filePath))
93+
this._logger.LogDebug("Reading text from {0}", canonicalPath);
94+
95+
if (!this.IsFilePathAllowed(canonicalPath))
9396
{
9497
throw new InvalidOperationException("Reading from the provided location is not allowed.");
9598
}
9699

97-
using var stream = await this._fileSystemConnector.GetFileContentStreamAsync(filePath, cancellationToken).ConfigureAwait(false);
100+
using var stream = await this._fileSystemConnector.GetFileContentStreamAsync(canonicalPath, cancellationToken).ConfigureAwait(false);
98101
return this._documentConnector.ReadText(stream);
99102
}
100103

@@ -107,25 +110,27 @@ public async Task AppendTextAsync(
107110
[Description("Destination file path")] string filePath,
108111
CancellationToken cancellationToken = default)
109112
{
110-
if (!this.IsFilePathAllowed(filePath))
113+
var canonicalPath = CanonicalizePath(filePath);
114+
115+
if (!this.IsFilePathAllowed(canonicalPath))
111116
{
112117
throw new InvalidOperationException("Writing to the provided location is not allowed.");
113118
}
114119

115120
// If the document already exists, open it. If not, create it.
116-
if (await this._fileSystemConnector.FileExistsAsync(filePath, cancellationToken).ConfigureAwait(false))
121+
if (await this._fileSystemConnector.FileExistsAsync(canonicalPath, cancellationToken).ConfigureAwait(false))
117122
{
118-
this._logger.LogDebug("Writing text to file {0}", filePath);
119-
using Stream stream = await this._fileSystemConnector.GetWriteableFileStreamAsync(filePath, cancellationToken).ConfigureAwait(false);
123+
this._logger.LogDebug("Writing text to file {0}", canonicalPath);
124+
using Stream stream = await this._fileSystemConnector.GetWriteableFileStreamAsync(canonicalPath, cancellationToken).ConfigureAwait(false);
120125
this._documentConnector.AppendText(stream, text);
121126
}
122127
else
123128
{
124-
this._logger.LogDebug("File does not exist. Creating file at {0}", filePath);
125-
using Stream stream = await this._fileSystemConnector.CreateFileAsync(filePath, cancellationToken).ConfigureAwait(false);
129+
this._logger.LogDebug("File does not exist. Creating file at {0}", canonicalPath);
130+
using Stream stream = await this._fileSystemConnector.CreateFileAsync(canonicalPath, cancellationToken).ConfigureAwait(false);
126131
this._documentConnector.Initialize(stream);
127132

128-
this._logger.LogDebug("Writing text to {0}", filePath);
133+
this._logger.LogDebug("Writing text to {0}", canonicalPath);
129134
this._documentConnector.AppendText(stream, text);
130135
}
131136
}
@@ -134,11 +139,10 @@ public async Task AppendTextAsync(
134139
private HashSet<string>? _allowedDirectories = [];
135140

136141
/// <summary>
137-
/// If a list of allowed directories has been provided, the directory of the provided filePath is checked
138-
/// to verify it is in the allowed directory list. Paths are canonicalized before comparison.
139-
/// Subdirectories of allowed directories are also permitted.
142+
/// Expands environment variables and resolves the path to its canonical form.
143+
/// This must be called before validation to prevent validate/use mismatches.
140144
/// </summary>
141-
private bool IsFilePathAllowed(string path)
145+
private static string CanonicalizePath(string path)
142146
{
143147
Verify.NotNullOrWhiteSpace(path);
144148

@@ -147,31 +151,55 @@ private bool IsFilePathAllowed(string path)
147151
throw new ArgumentException("Invalid file path, UNC paths are not supported.", nameof(path));
148152
}
149153

150-
string? directoryPath = Path.GetDirectoryName(path);
154+
// Expand environment variables first, then canonicalize — so that
155+
// validation and I/O operate on the same resolved path.
156+
var expanded = Environment.ExpandEnvironmentVariables(path);
157+
158+
// Re-check after expansion: an env var could have expanded to a UNC
159+
// or extended-path prefix (e.g., %NETSHARE% → \\server\share).
160+
if (expanded.StartsWith("\\\\", StringComparison.OrdinalIgnoreCase))
161+
{
162+
throw new ArgumentException("Invalid file path, UNC paths are not supported.", nameof(path));
163+
}
164+
165+
return Path.GetFullPath(expanded);
166+
}
167+
168+
// Use case-insensitive comparison on Windows (case-insensitive FS), case-sensitive on Linux/macOS.
169+
private static readonly StringComparison s_pathComparison =
170+
RuntimeInformation.IsOSPlatform(OSPlatform.Windows)
171+
? StringComparison.OrdinalIgnoreCase
172+
: StringComparison.Ordinal;
173+
174+
/// <summary>
175+
/// Checks whether a canonicalized file path falls within one of the allowed directories.
176+
/// Subdirectories of allowed directories are also permitted.
177+
/// </summary>
178+
private bool IsFilePathAllowed(string canonicalPath)
179+
{
180+
string? directoryPath = Path.GetDirectoryName(canonicalPath);
151181

152182
if (string.IsNullOrEmpty(directoryPath))
153183
{
154-
throw new ArgumentException("Invalid file path, a fully qualified file location must be specified.", nameof(path));
184+
throw new ArgumentException("Invalid file path, a fully qualified file location must be specified.", nameof(canonicalPath));
155185
}
156186

157187
if (this._allowedDirectories is null || this._allowedDirectories.Count == 0)
158188
{
159189
return false;
160190
}
161191

162-
var canonicalDir = Path.GetFullPath(directoryPath);
163-
164192
foreach (var allowedDirectory in this._allowedDirectories)
165193
{
166194
var canonicalAllowed = Path.GetFullPath(allowedDirectory);
167195
var separator = Path.DirectorySeparatorChar.ToString();
168-
if (!canonicalAllowed.EndsWith(separator, StringComparison.OrdinalIgnoreCase))
196+
if (!canonicalAllowed.EndsWith(separator, s_pathComparison))
169197
{
170198
canonicalAllowed += separator;
171199
}
172200

173-
if (canonicalDir.StartsWith(canonicalAllowed, StringComparison.OrdinalIgnoreCase)
174-
|| (canonicalDir + separator).Equals(canonicalAllowed, StringComparison.OrdinalIgnoreCase))
201+
if (directoryPath.StartsWith(canonicalAllowed, s_pathComparison)
202+
|| (directoryPath + separator).Equals(canonicalAllowed, s_pathComparison))
175203
{
176204
return true;
177205
}

‎dotnet/src/Plugins/Plugins.Document/FileSystem/LocalFileSystemConnector.cs‎

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -32,7 +32,7 @@ public Task<Stream> GetFileContentStreamAsync(string filePath, CancellationToken
3232
{
3333
try
3434
{
35-
return Task.FromResult<Stream>(File.Open(Environment.ExpandEnvironmentVariables(filePath), FileMode.Open, FileAccess.Read));
35+
return Task.FromResult<Stream>(File.Open(filePath, FileMode.Open, FileAccess.Read));
3636
}
3737
catch (Exception e)
3838
{
@@ -58,7 +58,7 @@ public Task<Stream> GetWriteableFileStreamAsync(string filePath, CancellationTok
5858
{
5959
try
6060
{
61-
return Task.FromResult<Stream>(File.Open(Environment.ExpandEnvironmentVariables(filePath), FileMode.Open, FileAccess.ReadWrite));
61+
return Task.FromResult<Stream>(File.Open(filePath, FileMode.Open, FileAccess.ReadWrite));
6262
}
6363
catch (Exception e)
6464
{
@@ -82,7 +82,7 @@ public Task<Stream> CreateFileAsync(string filePath, CancellationToken cancellat
8282
{
8383
try
8484
{
85-
return Task.FromResult<Stream>(File.Create(Environment.ExpandEnvironmentVariables(filePath)));
85+
return Task.FromResult<Stream>(File.Create(filePath));
8686
}
8787
catch (Exception e)
8888
{
@@ -95,7 +95,7 @@ public Task<bool> FileExistsAsync(string filePath, CancellationToken cancellatio
9595
{
9696
try
9797
{
98-
return Task.FromResult(File.Exists(Environment.ExpandEnvironmentVariables(filePath)));
98+
return Task.FromResult(File.Exists(filePath));
9999
}
100100
catch (Exception e)
101101
{

‎dotnet/src/Plugins/Plugins.UnitTests/Document/DocumentPluginTests.cs‎

Lines changed: 126 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -239,15 +239,137 @@ public async Task ItAllowsSubdirectoriesOfAllowedFoldersAsync()
239239
[Fact]
240240
public async Task ItDeniesRelativePathsAsync()
241241
{
242-
// Arrange
242+
// Arrange — use a unique subfolder so this test is deterministic regardless of CWD
243+
var allowedFolder = Path.Combine(Path.GetTempPath(), "unique-allowed-" + Guid.NewGuid().ToString("N")[..8]);
243244
var fileSystemConnectorMock = new Mock<IFileSystemConnector>();
244245
var documentConnectorMock = new Mock<IDocumentConnector>();
245246
var target = new DocumentPlugin(documentConnectorMock.Object, fileSystemConnectorMock.Object)
246247
{
247-
AllowedDirectories = [Path.GetTempPath()]
248+
AllowedDirectories = [allowedFolder]
248249
};
249250

250-
// Act & Assert — relative paths are caught by the "fully qualified" check
251-
await Assert.ThrowsAsync<ArgumentException>(async () => await target.ReadTextAsync("myfile.docx"));
251+
// Act & Assert — relative paths resolve to CWD after canonicalization,
252+
// which will be outside the allowed directories
253+
await Assert.ThrowsAsync<InvalidOperationException>(async () => await target.ReadTextAsync("myfile.docx"));
254+
await Assert.ThrowsAsync<InvalidOperationException>(async () => await target.AppendTextAsync("text", "myfile.docx"));
255+
}
256+
257+
[Fact]
258+
public async Task ItDeniesEnvVarExpansionBypassOnReadAsync()
259+
{
260+
// Arrange — use a test-specific env var that expands to a value containing
261+
// a path separator + ".." which creates a traversal after expansion.
262+
var allowedFolder = Path.Combine(Path.GetTempPath(), "allowed-sandbox");
263+
var envVarName = "SK_TEST_EXPAND_" + Guid.NewGuid().ToString("N")[..8];
264+
265+
try
266+
{
267+
// The env var value starts with a separator + ".." so that after expansion
268+
// the path becomes: allowed-sandbox/<sep>..<sep>elsewhere<sep>secret.docx
269+
Environment.SetEnvironmentVariable(envVarName,
270+
$"{Path.DirectorySeparatorChar}..{Path.DirectorySeparatorChar}elsewhere");
271+
var maliciousPath = Path.Combine(allowedFolder, $"%{envVarName}%", "secret.docx");
272+
273+
var fileSystemConnectorMock = new Mock<IFileSystemConnector>();
274+
var documentConnectorMock = new Mock<IDocumentConnector>();
275+
var target = new DocumentPlugin(documentConnectorMock.Object, fileSystemConnectorMock.Object)
276+
{
277+
AllowedDirectories = [allowedFolder]
278+
};
279+
280+
// Act & Assert — the path should be denied because env vars are expanded
281+
// before validation, so the canonical path lands outside the allowed directory.
282+
await Assert.ThrowsAsync<InvalidOperationException>(async () => await target.ReadTextAsync(maliciousPath));
283+
}
284+
finally
285+
{
286+
Environment.SetEnvironmentVariable(envVarName, null);
287+
}
288+
}
289+
290+
[Fact]
291+
public async Task ItDeniesEnvVarExpansionBypassOnWriteAsync()
292+
{
293+
// Arrange — same pattern as read test, for the write path.
294+
var allowedFolder = Path.Combine(Path.GetTempPath(), "allowed-sandbox");
295+
var envVarName = "SK_TEST_EXPAND_W_" + Guid.NewGuid().ToString("N")[..8];
296+
297+
try
298+
{
299+
Environment.SetEnvironmentVariable(envVarName,
300+
$"{Path.DirectorySeparatorChar}..{Path.DirectorySeparatorChar}elsewhere");
301+
var maliciousPath = Path.Combine(allowedFolder, $"%{envVarName}%", "secret.docx");
302+
303+
var fileSystemConnectorMock = new Mock<IFileSystemConnector>();
304+
var documentConnectorMock = new Mock<IDocumentConnector>();
305+
var target = new DocumentPlugin(documentConnectorMock.Object, fileSystemConnectorMock.Object)
306+
{
307+
AllowedDirectories = [allowedFolder]
308+
};
309+
310+
// Act & Assert
311+
await Assert.ThrowsAsync<InvalidOperationException>(async () => await target.AppendTextAsync("text", maliciousPath));
312+
}
313+
finally
314+
{
315+
Environment.SetEnvironmentVariable(envVarName, null);
316+
}
317+
}
318+
319+
[Fact]
320+
public async Task ItDeniesEnvVarExpansionToAbsolutePathAsync()
321+
{
322+
// Arrange — env var that expands to an absolute path outside the sandbox
323+
var allowedFolder = Path.Combine(Path.GetTempPath(), "sandbox");
324+
var envVarName = "SK_TEST_ABS_" + Guid.NewGuid().ToString("N")[..8];
325+
var outsidePath = Path.Combine(Path.GetTempPath(), "outside");
326+
327+
try
328+
{
329+
Environment.SetEnvironmentVariable(envVarName, outsidePath);
330+
var maliciousPath = Path.Combine($"%{envVarName}%", "secret.docx");
331+
332+
var fileSystemConnectorMock = new Mock<IFileSystemConnector>();
333+
var documentConnectorMock = new Mock<IDocumentConnector>();
334+
var target = new DocumentPlugin(documentConnectorMock.Object, fileSystemConnectorMock.Object)
335+
{
336+
AllowedDirectories = [allowedFolder]
337+
};
338+
339+
// Act & Assert — after env-var expansion, the path resolves outside the sandbox
340+
await Assert.ThrowsAsync<InvalidOperationException>(async () => await target.ReadTextAsync(maliciousPath));
341+
}
342+
finally
343+
{
344+
Environment.SetEnvironmentVariable(envVarName, null);
345+
}
346+
}
347+
348+
[Fact]
349+
public async Task ItDeniesUncPathsIntroducedViaEnvVarExpansionAsync()
350+
{
351+
// Arrange — env var that expands to a UNC path
352+
var allowedFolder = Path.Combine(Path.GetTempPath(), "sandbox");
353+
var envVarName = "SK_TEST_UNC_" + Guid.NewGuid().ToString("N")[..8];
354+
355+
try
356+
{
357+
Environment.SetEnvironmentVariable(envVarName, @"\\evil-server\share");
358+
var maliciousPath = $"%{envVarName}%{Path.DirectorySeparatorChar}secret.docx";
359+
360+
var fileSystemConnectorMock = new Mock<IFileSystemConnector>();
361+
var documentConnectorMock = new Mock<IDocumentConnector>();
362+
var target = new DocumentPlugin(documentConnectorMock.Object, fileSystemConnectorMock.Object)
363+
{
364+
AllowedDirectories = [allowedFolder]
365+
};
366+
367+
// Act & Assert — expanded path is UNC, should be rejected
368+
await Assert.ThrowsAsync<ArgumentException>(async () => await target.ReadTextAsync(maliciousPath));
369+
}
370+
finally
371+
{
372+
Environment.SetEnvironmentVariable(envVarName, null);
373+
}
252374
}
253375
}

0 commit comments

Comments
 (0)