diff --git a/src/client/Microsoft.Identity.Client.Extensions.Msal/Accessors/FileWithPermissions.cs b/src/client/Microsoft.Identity.Client.Extensions.Msal/Accessors/FileWithPermissions.cs index ba9ebe60c6..e245cd4178 100644 --- a/src/client/Microsoft.Identity.Client.Extensions.Msal/Accessors/FileWithPermissions.cs +++ b/src/client/Microsoft.Identity.Client.Extensions.Msal/Accessors/FileWithPermissions.cs @@ -20,15 +20,27 @@ internal static class FileWithPermissions #region Unix specific /// - /// 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 /// - [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 /// @@ -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 /// + /// + /// must not be a symbolic link. Two layers enforce this: + /// + /// The caller ( with + /// setChmod600: true) runs a pre-check via lstat(2) before entering the + /// retry loop, throwing with a clear message for + /// the non-adversarial case. + /// open(2) is called with O_NOFOLLOW so the kernel atomically rejects + /// a symlink that appears in the race window between the pre-check and the write, + /// returning ELOOP which is also surfaced as . + /// + /// 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); diff --git a/src/client/Microsoft.Identity.Client.Extensions.Msal/FileIOWithRetries.cs b/src/client/Microsoft.Identity.Client.Extensions.Msal/FileIOWithRetries.cs index b24d235d8e..a0cb7f7f15 100644 --- a/src/client/Microsoft.Identity.Client.Extensions.Msal/FileIOWithRetries.cs +++ b/src/client/Microsoft.Identity.Client.Extensions.Msal/FileIOWithRetries.cs @@ -38,6 +38,11 @@ internal static void CreateAndWriteToFile(string filePath, byte[] data, bool set { EnsureParentDirectoryExists(filePath, logger); + if (!SharedUtilities.IsWindowsPlatform()) + { + ThrowIfCacheFileIsSymlink(filePath); + } + logger.LogInformation($"Writing cache file"); TryProcessFile(() => @@ -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(() => @@ -93,6 +104,33 @@ internal static void TouchFile(string filePath, TraceSourceLogger logger) }, logger); } + /// + /// Throws if 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. + /// + /// + /// Uses which maps to lstat(2) on Unix and + /// therefore returns 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. + /// + 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. + } + } + internal static void TryProcessFile(Action action, TraceSourceLogger logger) { for (int tryCount = 0; tryCount <= FileLockRetryCount; tryCount++) diff --git a/tests/Microsoft.Identity.Test.Unit/CacheExtension/FileIOWithRetriesTests.cs b/tests/Microsoft.Identity.Test.Unit/CacheExtension/FileIOWithRetriesTests.cs index a8e4143d87..4b819a8115 100644 --- a/tests/Microsoft.Identity.Test.Unit/CacheExtension/FileIOWithRetriesTests.cs +++ b/tests/Microsoft.Identity.Test.Unit/CacheExtension/FileIOWithRetriesTests.cs @@ -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 { @@ -27,6 +28,105 @@ public void TestInitialize() _logger.Listeners.Add(_testListener); } + [DoNotRunOnWindows] + public void CreateAndWriteToFile_SymlinkAtCachePath_ThrowsInvalidOperationException() + { + 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()); + +#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(() => + 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() + { + 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()); + +#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(() => + 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(() => + 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() {