Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -20,15 +20,27 @@ internal static class FileWithPermissions
#region Unix specific

/// <summary>
/// Equivalent to calling open() with flags O_CREAT|O_WRONLY|O_TRUNC. O_TRUNC will truncate the file.
/// Calls open(2) with caller-supplied flags and mode. Used instead of creat(2) so that
/// O_NOFOLLOW can be included to atomically reject symlinks at the kernel level.
/// See https://man7.org/linux/man-pages/man2/open.2.html
/// </summary>
[DllImport("libc", EntryPoint = "creat", SetLastError = true)]
private static extern int PosixCreate([MarshalAs(UnmanagedType.LPStr)] string pathname, int mode);
[DllImport("libc", EntryPoint = "open", SetLastError = true)]
private static extern int PosixOpen([MarshalAs(UnmanagedType.LPStr)] string pathname, int flags, int mode);

[DllImport("libc", EntryPoint = "chmod", SetLastError = true)]
private static extern int PosixChmod([MarshalAs(UnmanagedType.LPStr)] string pathname, int mode);

// open(2) flags — values differ between Linux and macOS.
// Linux: O_WRONLY=0x1, O_CREAT=0x40, O_TRUNC=0x200, O_NOFOLLOW=0x20000
// macOS: O_WRONLY=0x1, O_CREAT=0x200, O_TRUNC=0x400, O_NOFOLLOW=0x100
private static readonly int s_openFlags = RuntimeInformation.IsOSPlatform(OSPlatform.OSX)
? 0x1 | 0x200 | 0x400 | 0x100 // macOS
: 0x1 | 0x40 | 0x200 | 0x20000; // Linux

// ELOOP errno — kernel returns this when O_NOFOLLOW encounters a symlink.
// Linux: 40 macOS: 62
private static readonly int s_eloop = RuntimeInformation.IsOSPlatform(OSPlatform.OSX) ? 62 : 40;

#endregion

/// <summary>
Expand Down Expand Up @@ -60,22 +72,41 @@ public static void WriteToNewFileWithOwnerRWPermissions(string path, byte[] data
/// Based on https://stackoverflow.com/questions/45132081/file-permissions-on-linux-unix-with-net-core and on
/// https://github.com/NuGet/NuGet.Client/commit/d62db666c710bf95121fe8f5c6a6cbe01985456f
/// </summary>
/// <remarks>
/// <paramref name="path"/> must not be a symbolic link. Two layers enforce this:
/// <list type="number">
/// <item>The caller (<see cref="FileIOWithRetries.CreateAndWriteToFile"/> with
/// <c>setChmod600: true</c>) runs a pre-check via <c>lstat(2)</c> before entering the
/// retry loop, throwing <see cref="InvalidOperationException"/> with a clear message for
/// the non-adversarial case.</item>
/// <item><c>open(2)</c> is called with <c>O_NOFOLLOW</c> so the kernel atomically rejects
/// a symlink that appears in the race window between the pre-check and the write,
/// returning <c>ELOOP</c> which is also surfaced as <see cref="InvalidOperationException"/>.</item>
/// </list>
/// </remarks>
private static void WriteToNewFileWithOwnerRWPermissionsUnix(string path, byte[] data)
{
int _0600 = Convert.ToInt32("600", 8);

int fileDescriptor = PosixCreate(path, _0600);
int fileDescriptor = PosixOpen(path, s_openFlags, _0600);

// if creat() fails, then try to use File.Create because it will throw a meaningful exception.
if (fileDescriptor == -1)
{
int posixCreateError = Marshal.GetLastWin32Error();
int errno = Marshal.GetLastWin32Error();

if (errno == s_eloop)
{
throw new InvalidOperationException(
$"The cache file path '{path}' is a symbolic link. MSAL cache paths must not be symbolic links.");
}

// For any other error fall back to File.Create which will throw a meaningful exception.
using (File.Create(path))
{
// File.Create() should have thrown an exception with an appropriate error message
}
File.Delete(path);
throw new InvalidOperationException($"libc creat() failed with last error code {posixCreateError}, but File.Create did not");
throw new InvalidOperationException($"libc open() failed with last error code {errno}, but File.Create did not");
}

var safeFileHandle = new SafeFileHandle((IntPtr)fileDescriptor, ownsHandle: true);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,11 @@ internal static void CreateAndWriteToFile(string filePath, byte[] data, bool set
{
EnsureParentDirectoryExists(filePath, logger);

if (!SharedUtilities.IsWindowsPlatform())
{
ThrowIfCacheFileIsSymlink(filePath);
}
Comment thread
iNinja marked this conversation as resolved.

logger.LogInformation($"Writing cache file");

TryProcessFile(() =>
Expand Down Expand Up @@ -76,6 +81,12 @@ private static void EnsureParentDirectoryExists(string filePath, TraceSourceLogg
internal static void TouchFile(string filePath, TraceSourceLogger logger)
{
EnsureParentDirectoryExists(filePath, logger);

if (!SharedUtilities.IsWindowsPlatform())
{
ThrowIfCacheFileIsSymlink(filePath);
}

logger.LogInformation($"Touching file...");

TryProcessFile(() =>
Expand All @@ -93,6 +104,33 @@ internal static void TouchFile(string filePath, TraceSourceLogger logger)
}, logger);
}

/// <summary>
/// Throws <see cref="InvalidOperationException"/> if <paramref name="filePath"/> is a
/// symbolic link. Called on non-Windows platforms before writing or touching the cache
/// file to prevent a symlink from being used as a write-anywhere primitive.
/// </summary>
/// <remarks>
/// Uses <see cref="File.GetAttributes(string)"/> which maps to <c>lstat(2)</c> on Unix and
/// therefore returns <see cref="FileAttributes.ReparsePoint"/> for the symlink itself
/// rather than following it to the target. If the path does not yet exist the method
/// returns silently — the caller will create the file normally.
/// </remarks>
private static void ThrowIfCacheFileIsSymlink(string filePath)
{
try
{
if ((File.GetAttributes(filePath) & FileAttributes.ReparsePoint) != 0)
{
throw new InvalidOperationException(
$"The cache file path '{filePath}' is a symbolic link. MSAL cache paths must not be symbolic links.");
}
}
catch (FileNotFoundException)
{
// File does not exist yet — not a symlink, proceed normally.
}
}
Comment thread
iNinja marked this conversation as resolved.

internal static void TryProcessFile(Action action, TraceSourceLogger logger)
{
for (int tryCount = 0; tryCount <= FileLockRetryCount; tryCount++)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@
using Microsoft.Identity.Client.Extensions.Msal;
using Microsoft.VisualStudio.TestTools.UnitTesting;
using Microsoft.Identity.Client.PlatformsCommon.Shared;
using Microsoft.Identity.Test.Common.Core.Helpers;

namespace Microsoft.Identity.Test.Unit.CacheExtension
{
Expand All @@ -27,6 +28,105 @@ public void TestInitialize()
_logger.Listeners.Add(_testListener);
}

[DoNotRunOnWindows]
public void CreateAndWriteToFile_SymlinkAtCachePath_ThrowsInvalidOperationException()
Comment thread
Copilot marked this conversation as resolved.
{
string dir = Path.Combine(Path.GetTempPath(), Guid.NewGuid().ToString());
Directory.CreateDirectory(dir);
string realTarget = Path.Combine(dir, "real.bin");
string symlinkPath = Path.Combine(dir, "cache.bin");
File.WriteAllBytes(realTarget, Array.Empty<byte>());

#if NET6_0_OR_GREATER
File.CreateSymbolicLink(symlinkPath, realTarget);
#else
// net48 only runs on Windows where this test is already skipped by [DoNotRunOnWindows].
using var proc = Process.Start(new ProcessStartInfo("ln", $"-s \"{realTarget}\" \"{symlinkPath}\""));
proc?.WaitForExit();
#endif

try
{
var ex = AssertException.Throws<InvalidOperationException>(() =>
FileIOWithRetries.CreateAndWriteToFile(
symlinkPath,
new byte[] { 1, 2, 3 },
setChmod600: true,
new TraceSourceLogger(_logger)));

StringAssert.Contains(ex.Message, "symbolic link");
}
finally
{
Directory.Delete(dir, recursive: true);
}
}

[DoNotRunOnWindows]
public void TouchFile_SymlinkAtCachePath_ThrowsInvalidOperationException()
Comment thread
Copilot marked this conversation as resolved.
{
string dir = Path.Combine(Path.GetTempPath(), Guid.NewGuid().ToString());
Directory.CreateDirectory(dir);
string realTarget = Path.Combine(dir, "real.bin");
string symlinkPath = Path.Combine(dir, "cache.bin");
File.WriteAllBytes(realTarget, Array.Empty<byte>());

Comment thread
iNinja marked this conversation as resolved.
#if NET6_0_OR_GREATER
File.CreateSymbolicLink(symlinkPath, realTarget);
#else
using var proc = Process.Start(new ProcessStartInfo("ln", $"-s \"{realTarget}\" \"{symlinkPath}\""));
proc?.WaitForExit();
#endif

try
{
var ex = AssertException.Throws<InvalidOperationException>(() =>
FileIOWithRetries.TouchFile(
symlinkPath,
new TraceSourceLogger(_logger)));

StringAssert.Contains(ex.Message, "symbolic link");
}
finally
{
Directory.Delete(dir, recursive: true);
}
}

[DoNotRunOnWindows]
public void TouchFile_DanglingSymlinkAtCachePath_ThrowsInvalidOperationException()
{
// A dangling symlink (target does not exist) must also be detected and rejected.
// If File.GetAttributes ever threw FileNotFoundException for broken links instead of
// returning ReparsePoint, the catch block in ThrowIfCacheFileIsSymlink would silently
// allow the write. This test pins that the check works for dangling links.
string dir = Path.Combine(Path.GetTempPath(), Guid.NewGuid().ToString());
Directory.CreateDirectory(dir);
string nonExistentTarget = Path.Combine(dir, "ghost.bin");
string symlinkPath = Path.Combine(dir, "cache.bin");

#if NET6_0_OR_GREATER
File.CreateSymbolicLink(symlinkPath, nonExistentTarget);
#else
using var proc = Process.Start(new ProcessStartInfo("ln", $"-s \"{nonExistentTarget}\" \"{symlinkPath}\""));
proc?.WaitForExit();
#endif

try
{
var ex = AssertException.Throws<InvalidOperationException>(() =>
FileIOWithRetries.TouchFile(
symlinkPath,
new TraceSourceLogger(_logger)));

StringAssert.Contains(ex.Message, "symbolic link");
}
finally
{
Directory.Delete(dir, recursive: true);
}
}

[TestMethod]
public async Task Touch_FiresEvent_Async()
{
Expand Down
Loading