Skip to content

Commit d1e1ca6

Browse files
westey-mwestey-m
committed
Address PR comments
1 parent 50b10df commit d1e1ca6

5 files changed

Lines changed: 194 additions & 8 deletions

File tree

‎dotnet/src/InternalUtilities/src/System/SecureFileSystem.cs‎

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -217,6 +217,7 @@ private static class UnixFileSystem
217217
private const int ReadOnly = 0;
218218
private const int WriteOnly = 1;
219219
private const int ReadWrite = 2;
220+
private const int LinuxPathOnly = 0x200000;
220221
private const uint DefaultPermissions = 0x1B6; // 0666, reduced by the process umask.
221222
private const int AlreadyExists = 17;
222223

@@ -319,7 +320,9 @@ internal static SafeFileHandle Open(
319320
#pragma warning restore CA2000
320321

321322
private static int GetDirectoryFlags() =>
322-
ReadOnly | GetDirectoryFlag() | GetNoFollowFlag() | GetCloseOnExecFlag();
323+
// Traversal needs search permission, not permission to list each ancestor.
324+
(RuntimeInformation.IsOSPlatform(OSPlatform.Linux) ? LinuxPathOnly : ReadOnly) |
325+
GetDirectoryFlag() | GetNoFollowFlag() | GetCloseOnExecFlag();
323326

324327
private static int GetDirectoryFlag() =>
325328
RuntimeInformation.IsOSPlatform(OSPlatform.OSX) ? 0x100000 : 0x10000;

‎dotnet/src/Plugins/Plugins.MsGraph/CloudDrivePlugin.cs‎

Lines changed: 21 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -5,6 +5,7 @@
55
using System.ComponentModel;
66
using System.IO;
77
using System.Runtime.InteropServices;
8+
using System.Security;
89
using System.Threading;
910
using System.Threading.Tasks;
1011
using Microsoft.Extensions.Logging;
@@ -176,9 +177,7 @@ public async Task UploadFileAsync(
176177

177178
Ensure.NotNullOrWhitespace(filePath, nameof(filePath));
178179

179-
var canonicalPath = CanonicalizePath(filePath);
180-
181-
if (!this.IsUploadPathAllowed(canonicalPath))
180+
if (!this.TryGetAllowedUploadPath(filePath, out var canonicalPath))
182181
{
183182
throw new InvalidOperationException("Uploading from the provided location is not allowed. Configure 'AllowedUploadDirectories' with trusted directory paths to enable uploads.");
184183
}
@@ -293,13 +292,28 @@ private static string NormalizeRemotePath(string path)
293292
}
294293

295294
/// <summary>
296-
/// Expands environment variables and resolves the path to its canonical form.
295+
/// Validates the upload location without exposing caller-path resolution errors.
297296
/// The upload still uses a secure open because this path can change after validation.
298297
/// </summary>
299-
private static string CanonicalizePath(string path)
298+
private bool TryGetAllowedUploadPath(string path, out string canonicalPath)
300299
{
301-
var fullPath = SecureFileSystem.NormalizePath(path, expandEnvironmentVariables: true);
302-
return PathUtilities.GetSafeFullPath(fullPath);
300+
canonicalPath = SecureFileSystem.NormalizePath(path, expandEnvironmentVariables: true);
301+
if (this._allowedUploadDirectories.Count == 0)
302+
{
303+
return false;
304+
}
305+
306+
try
307+
{
308+
canonicalPath = PathUtilities.GetSafeFullPath(canonicalPath);
309+
}
310+
catch (Exception ex) when (ex is IOException or UnauthorizedAccessException or InvalidOperationException or SecurityException)
311+
{
312+
return false;
313+
}
314+
315+
// Keep trusted configuration errors outside the caller-path error handling.
316+
return this.IsUploadPathAllowed(canonicalPath);
303317
}
304318

305319
/// <summary>

‎dotnet/src/Plugins/Plugins.UnitTests/Core/SecureFileSystemTests.cs‎

Lines changed: 48 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -4,6 +4,7 @@
44
using System.Collections.Generic;
55
using System.IO;
66
using System.Runtime.InteropServices;
7+
using System.Runtime.Versioning;
78
using System.Text;
89
using System.Threading.Tasks;
910
using Microsoft.SemanticKernel;
@@ -389,6 +390,53 @@ public void MissingParentIsNotCreated()
389390
Assert.False(Directory.Exists(Path.GetDirectoryName(path)));
390391
}
391392

393+
[LinuxNonRootTheory]
394+
[InlineData(false)]
395+
[InlineData(true)]
396+
[SupportedOSPlatform("linux")]
397+
public void SearchOnlyDirectoriesDoNotPreventFileAccess(bool restrictedAncestorOfAllowedRoot)
398+
{
399+
var restricted = Path.Combine(this._tempDirectory, "search-only");
400+
var writable = Path.Combine(restricted, "writable");
401+
Directory.CreateDirectory(writable);
402+
var allowed = restrictedAncestorOfAllowedRoot ? writable : restricted;
403+
var input = Path.Combine(writable, "input.txt");
404+
var output = Path.Combine(writable, "output.txt");
405+
File.WriteAllText(input, "readable");
406+
var originalMode = File.GetUnixFileMode(restricted);
407+
try
408+
{
409+
File.SetUnixFileMode(restricted,
410+
UnixFileMode.UserExecute | UnixFileMode.GroupExecute | UnixFileMode.OtherExecute);
411+
Assert.Equal("readable", File.ReadAllText(input));
412+
Assert.Throws<UnauthorizedAccessException>(() => Directory.GetFileSystemEntries(restricted));
413+
414+
using (var stream = SecureFileSystem.OpenRead(input, [allowed], out _))
415+
using (var reader = new StreamReader(stream))
416+
{
417+
Assert.Equal("readable", reader.ReadToEnd());
418+
}
419+
420+
using (var stream = SecureFileSystem.OpenWrite(output, [allowed], out _, FileMode.CreateNew))
421+
{
422+
stream.WriteByte(42);
423+
}
424+
425+
using (var stream = SecureFileSystem.OpenUpdate(output, [allowed], out var created))
426+
{
427+
Assert.False(created);
428+
Assert.Equal(42, stream.ReadByte());
429+
stream.WriteByte(43);
430+
}
431+
432+
Assert.Equal(new byte[] { 42, 43 }, File.ReadAllBytes(output));
433+
}
434+
finally
435+
{
436+
File.SetUnixFileMode(restricted, originalMode);
437+
}
438+
}
439+
392440
[UnixFact(Timeout = 5000)]
393441
public async Task NamedPipeIsRejectedWithoutWaitingForAnotherProcessAsync()
394442
{

‎dotnet/src/Plugins/Plugins.UnitTests/MsGraph/CloudDrivePluginTests.cs‎

Lines changed: 90 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -3,9 +3,11 @@
33
using System;
44
using System.IO;
55
using System.Runtime.InteropServices;
6+
using System.Runtime.Versioning;
67
using System.Text;
78
using System.Threading;
89
using System.Threading.Tasks;
10+
using Microsoft.SemanticKernel;
911
using Microsoft.SemanticKernel.Plugins.MsGraph;
1012
using Moq;
1113
using Xunit;
@@ -656,6 +658,94 @@ public async Task UploadDoesNotFollowLinksOutsideAllowedDirectoryAsync()
656658
connector.VerifyNoOtherCalls();
657659
}
658660

661+
[SymbolicLinkTheory]
662+
[InlineData(false)]
663+
[InlineData(true)]
664+
public async Task UploadPathResolutionFailuresReturnTheUsualDenialAsync(bool emptyAllowedDirectories)
665+
{
666+
var allowed = Path.Combine(this._tempDirectory, "allowed");
667+
Directory.CreateDirectory(allowed);
668+
var loop = Path.Combine(this._tempDirectory, "loop");
669+
Directory.CreateSymbolicLink(loop, loop);
670+
Assert.Throws<InvalidOperationException>(() => PathUtilities.GetSafeFullPath(loop));
671+
var connector = new Mock<ICloudDriveConnector>(MockBehavior.Strict);
672+
var plugin = new CloudDrivePlugin(connector.Object)
673+
{
674+
AllowedUploadDirectories = emptyAllowedDirectories ? [] : [allowed],
675+
AllowedUploadDestinationPaths = ["/"]
676+
};
677+
var expected = await Assert.ThrowsAsync<InvalidOperationException>(
678+
() => plugin.UploadFileAsync(Path.Combine(this._tempDirectory, "missing.txt"), "/file.txt"));
679+
680+
var actual = await Assert.ThrowsAsync<InvalidOperationException>(
681+
() => plugin.UploadFileAsync(Path.Combine(loop, "file.txt"), "/file.txt"));
682+
683+
Assert.Equal(expected.Message, actual.Message);
684+
Assert.Null(actual.InnerException);
685+
connector.VerifyNoOtherCalls();
686+
}
687+
688+
[SymbolicLinkFact]
689+
public async Task UploadPathResolutionFailuresDoNotHideConfigurationErrorsAsync()
690+
{
691+
var loop = Path.Combine(this._tempDirectory, "loop");
692+
Directory.CreateSymbolicLink(loop, loop);
693+
var expected = Assert.Throws<InvalidOperationException>(() => PathUtilities.GetSafeFullPath(loop));
694+
var connector = new Mock<ICloudDriveConnector>(MockBehavior.Strict);
695+
var plugin = new CloudDrivePlugin(connector.Object)
696+
{
697+
AllowedUploadDirectories = [loop],
698+
AllowedUploadDestinationPaths = ["/"]
699+
};
700+
701+
var actual = await Assert.ThrowsAsync<InvalidOperationException>(
702+
() => plugin.UploadFileAsync(Path.Combine(this._tempDirectory, "file.txt"), "/file.txt"));
703+
704+
Assert.Equal(expected.Message, actual.Message);
705+
connector.VerifyNoOtherCalls();
706+
}
707+
708+
[LinuxNonRootFact]
709+
[SupportedOSPlatform("linux")]
710+
public async Task InaccessibleUploadPathsDoNotRevealDetailsOrHideConfigurationErrorsAsync()
711+
{
712+
var allowed = Path.Combine(this._tempDirectory, "allowed");
713+
var restricted = Path.Combine(this._tempDirectory, "restricted");
714+
var nested = Path.Combine(restricted, "nested");
715+
Directory.CreateDirectory(allowed);
716+
Directory.CreateDirectory(nested);
717+
var inaccessible = Path.Combine(nested, "file.txt");
718+
await File.WriteAllTextAsync(inaccessible, "outside");
719+
var connector = new Mock<ICloudDriveConnector>(MockBehavior.Strict);
720+
var plugin = new CloudDrivePlugin(connector.Object)
721+
{
722+
AllowedUploadDirectories = [allowed],
723+
AllowedUploadDestinationPaths = ["/"]
724+
};
725+
var expected = await Assert.ThrowsAsync<InvalidOperationException>(
726+
() => plugin.UploadFileAsync(Path.Combine(this._tempDirectory, "missing.txt"), "/file.txt"));
727+
var originalMode = File.GetUnixFileMode(restricted);
728+
try
729+
{
730+
File.SetUnixFileMode(restricted, UnixFileMode.None);
731+
Assert.Throws<UnauthorizedAccessException>(() => PathUtilities.GetSafeFullPath(inaccessible));
732+
733+
var actual = await Assert.ThrowsAsync<InvalidOperationException>(
734+
() => plugin.UploadFileAsync(inaccessible, "/file.txt"));
735+
Assert.Equal(expected.Message, actual.Message);
736+
Assert.Null(actual.InnerException);
737+
738+
plugin.AllowedUploadDirectories = [nested];
739+
await Assert.ThrowsAsync<UnauthorizedAccessException>(
740+
() => plugin.UploadFileAsync(Path.Combine(allowed, "file.txt"), "/file.txt"));
741+
connector.VerifyNoOtherCalls();
742+
}
743+
finally
744+
{
745+
File.SetUnixFileMode(restricted, originalMode);
746+
}
747+
}
748+
659749
public void Dispose()
660750
{
661751
Directory.Delete(this._tempDirectory, recursive: true);

‎dotnet/src/Plugins/Plugins.UnitTests/Support/SymbolicLinkFactAttribute.cs‎

Lines changed: 31 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -38,6 +38,37 @@ public UnixFactAttribute()
3838
}
3939
}
4040

41+
[AttributeUsage(AttributeTargets.Method)]
42+
internal sealed class LinuxNonRootFactAttribute : FactAttribute
43+
{
44+
public LinuxNonRootFactAttribute()
45+
{
46+
this.Skip = LinuxNonRootTestSupport.SkipReason;
47+
}
48+
}
49+
50+
[AttributeUsage(AttributeTargets.Method)]
51+
internal sealed class LinuxNonRootTheoryAttribute : TheoryAttribute
52+
{
53+
public LinuxNonRootTheoryAttribute()
54+
{
55+
this.Skip = LinuxNonRootTestSupport.SkipReason;
56+
}
57+
}
58+
59+
internal static class LinuxNonRootTestSupport
60+
{
61+
internal static string? SkipReason { get; } =
62+
!RuntimeInformation.IsOSPlatform(OSPlatform.Linux)
63+
? "This test requires Linux directory permissions."
64+
: GetEffectiveUserId() == 0
65+
? "Root can bypass the directory permissions this test needs to enforce."
66+
: null;
67+
68+
[DllImport("libc", EntryPoint = "geteuid")]
69+
private static extern uint GetEffectiveUserId();
70+
}
71+
4172
internal static class SymbolicLinkTestSupport
4273
{
4374
internal static string? SkipReason { get; } = GetSkipReason();

0 commit comments

Comments
 (0)