diff --git a/docs/linux-port/investigations/security-review-2026-10.md b/docs/linux-port/investigations/security-review-2026-10.md index 02e5ff16468b..907e543c37ae 100644 --- a/docs/linux-port/investigations/security-review-2026-10.md +++ b/docs/linux-port/investigations/security-review-2026-10.md @@ -46,6 +46,10 @@ Copy verification checks only output length and the source's current size. `Veri **Suggested fix:** Carry the opened source's identity, size, nanosecond mtime and ctime through publication and recheck the pinned inode and source name immediately before unlink. Verify copied content against the source and preserve the original on detected changes. Use the helper's content/metadata checks as a reference while acknowledging the unavoidable final check/unlink race; the present gap accepts changes made before the final check. +**Remediation and threat model:** Cross-device deletion protects against accidental concurrent modification, such as an application saving during a move: data differing from the copied version must be retained. Before each quarantine rename, fstat of the held descriptor must match the copied identity, size, mtime and ctime; writes bump ctime, which a writer cannot restore by setting mtime. Only after that check passes may post-rename verification accept rename's ctime update, while still requiring size, mtime, device/inode and link count to match. Immediately before unlink, recheck the held version and lstat the quarantine name against its device/inode. Unsupported atomic rename uses the same final held-fd/version and original-name identity checks. Verification failure retains or restores the source without overwriting a newly created original name. Regular-file tag re-keying runs only after successful deletion. + +A hostile process running as the same user and racing the quarantine name is outside this threat model. POSIX offers no unlink-by-inode; a writer using an open descriptor can still write in the final stat/rename or stat/unlink window. These checks reject observed changes, but cannot guarantee exclusion of writes during those unavoidable syscall gaps. + ### R5 — P2: Direct thumbnail cache hits bypass image validation **Location:** `src/Files.Platform.Linux/Thumbnails/LinuxThumbnailService.cs:179`, `:239`, `:249`, `:259`; contrast external output validation at `:481`. diff --git a/src/Files.Platform.Linux/FileOperations/LinuxFileOperationsHooks.cs b/src/Files.Platform.Linux/FileOperations/LinuxFileOperationsHooks.cs index 7a79d1ab5c3d..37859607cebb 100644 --- a/src/Files.Platform.Linux/FileOperations/LinuxFileOperationsHooks.cs +++ b/src/Files.Platform.Linux/FileOperations/LinuxFileOperationsHooks.cs @@ -42,5 +42,14 @@ public sealed class LinuxFileOperationsHooks /// Invoked after an entry was classified and right before it is deleted or descended into. public Action? BeforeDeleteEntry { get; init; } + + /// Invoked after move source verification, before the final quarantine checks and rename. + public Action? AfterMoveSourceVerified { get; init; } + + /// Invoked after an atomic quarantine rename, before verification and deletion. + public Action? MoveSourceQuarantined { get; init; } + + /// Invoked before the final held-fd and entry checks immediately preceding move source unlink. + public Action? BeforeMoveSourceUnlink { get; init; } } } diff --git a/src/Files.Platform.Linux/FileOperations/LinuxFileOperationsService.Move.cs b/src/Files.Platform.Linux/FileOperations/LinuxFileOperationsService.Move.cs index 60a41493891a..8546a90586df 100644 --- a/src/Files.Platform.Linux/FileOperations/LinuxFileOperationsService.Move.cs +++ b/src/Files.Platform.Linux/FileOperations/LinuxFileOperationsService.Move.cs @@ -69,6 +69,7 @@ private async Task MoveEntryAsync(string source, string destination, Di if (stat.IsSpecial) throw new FileOperationException(FileOperationErrorKind.UnsupportedFileType, "FIFOs, sockets and device files cannot be moved across file systems.", source); + using var moveSourceHandle = sourceKind == EntryKind.Symlink ? OpenMoveSource(sourceParent, name, source) : null; VerifyMoveSource(sourceParent, name, stat, source); var copy = sourceKind == EntryKind.Symlink ? CopyLink(source, destination, resolved.Replace, context, sourceParent, destinationParent) @@ -77,19 +78,14 @@ private async Task MoveEntryAsync(string source, string destination, Di { context.Hooks?.BeforeDeleteEntry?.Invoke(source); context.CancellationToken.ThrowIfCancellationRequested(); - if (!PosixNative.TryStat((int)input.SafeFileHandle.DangerousGetHandle(), out var current) - || !PosixNative.SameFileVersion(openedStat, current)) - throw new FileOperationException(FileOperationErrorKind.VerificationFailed, "The source changed while it was being moved.", source); - VerifyMoveSource(sourceParent, name, openedStat, source); - PosixNative.UnlinkAt(sourceParent.Descriptor, name, 0, source); + DeleteMoveSource(sourceParent, name, openedStat, source, (int)input.SafeFileHandle.DangerousGetHandle(), context); context.PathChanged(source, destination, xattrStale: true); }, stat).ConfigureAwait(false); if (sourceKind == EntryKind.Symlink && copy.Status == FileOperationStatus.Succeeded) { context.Hooks?.BeforeDeleteEntry?.Invoke(source); context.CancellationToken.ThrowIfCancellationRequested(); - VerifyMoveSource(sourceParent, name, stat, source); - PosixNative.UnlinkAt(sourceParent.Descriptor, name, 0, source); + DeleteMoveSource(sourceParent, name, stat, source, (int)moveSourceHandle!.DangerousGetHandle(), context); context.PathChanged(source, destination, xattrStale: true); } return copy; @@ -133,12 +129,14 @@ private async Task MoveDirectoryContentsAsync(string source, string des if (result.Status == FileOperationStatus.Succeeded) { + if (!PosixNative.TryStat(sourceDirectory.Descriptor, out var removalStat)) + throw PosixNative.CreateException(5, source); context.Hooks?.BeforeDeleteEntry?.Invoke(source); context.CancellationToken.ThrowIfCancellationRequested(); VerifyMoveSource(sourceParent, Path.GetFileName(source), stat, source); try { - PosixNative.UnlinkAt(sourceParent.Descriptor, Path.GetFileName(source), PosixNative.AtRemoveDir, source); + DeleteMoveSource(sourceParent, Path.GetFileName(source), removalStat, source, sourceDirectory.Descriptor, context); context.PathChanged(source, created ? destination : null, xattrStale: created); } catch (IOException ex) when (ex.HResult == 39 && sourceDirectory.ListNames().Count > 0) @@ -151,6 +149,77 @@ private async Task MoveDirectoryContentsAsync(string source, string des return result; } + private static Microsoft.Win32.SafeHandles.SafeFileHandle OpenMoveSource(DirectoryHandle parent, string name, string source) + { + var fd = PosixNative.OpenAt(parent.Descriptor, name, PosixNative.ReadOnlyFlags | PosixNative.ONofollow | 0x200000, out var errno); // O_PATH + if (fd < 0) + throw PosixNative.CreateException(errno, source); + return new Microsoft.Win32.SafeHandles.SafeFileHandle(fd, true); + } + + private static void DeleteMoveSource(DirectoryHandle parent, string name, PosixStat expected, string source, int fd, FileOperationContext context) + { + void Changed() => throw new FileOperationException(FileOperationErrorKind.VerificationFailed, "The source changed while it was being moved.", source); + bool Matches(PosixStat current) => PosixNative.SameFileVersion(expected, current) && expected.LinkCount == current.LinkCount; + VerifyMoveSource(parent, name, expected, source); + context.Hooks?.AfterMoveSourceVerified?.Invoke(source); + if (!PosixNative.TryStat(fd, out var before) || !Matches(before) || !Matches(PosixNative.StatAt(parent.Descriptor, name, source))) + Changed(); + void VerifyBeforeRename() + { + if (!PosixNative.TryStat(fd, out var current) || !Matches(current) + || !Matches(PosixNative.StatAt(parent.Descriptor, name, source))) + Changed(); + } + // Protect accidental concurrent saves: never knowingly delete a version absent from the copy. + // Writes change ctime even when mtime is restored. Only a verified pre-rename version may + // accept rename's ctime update. Hostile same-user quarantine-name races are out of scope: + // POSIX has no unlink-by-inode, so final stat/rename and stat/unlink windows remain. + var quarantine = ".linuxfiles-move-" + Guid.NewGuid().ToString("N"); + while (!PosixNative.TryQuarantineRename(parent.Descriptor, name, quarantine, out var errno, context.Hooks, VerifyBeforeRename)) + { + if (errno == 17) + { + quarantine = ".linuxfiles-move-" + Guid.NewGuid().ToString("N"); + continue; + } + if (errno is 22 or 38 or 95) // RENAME_NOREPLACE unsupported; recheck the held version and name. + { + context.Hooks?.BeforeMoveSourceUnlink?.Invoke(source, source); + var fallbackEntry = PosixNative.StatAt(parent.Descriptor, name, source); + if (!PosixNative.TryStat(fd, out var fallback) || !Matches(fallback) + || !Matches(fallbackEntry) || !PosixNative.SameEntry(fallback, fallbackEntry)) + Changed(); + PosixNative.UnlinkAt(parent.Descriptor, name, expected.IsDirectory ? PosixNative.AtRemoveDir : 0, source); + return; + } + throw PosixNative.CreateException(errno, source); + } + try + { + context.Hooks?.MoveSourceQuarantined?.Invoke(source, Path.Combine(Path.GetDirectoryName(source)!, quarantine)); + var entry = PosixNative.StatAt(parent.Descriptor, quarantine, source); + // The pre-rename fd check passed; only rename's ctime update is permitted. + var renamedExpected = expected with { ChangedSeconds = entry.ChangedSeconds, ChangedNanoseconds = entry.ChangedNanoseconds }; + var held = default(PosixStat); + if (!PosixNative.SameFileVersion(renamedExpected, entry) || expected.LinkCount != entry.LinkCount + || !PosixNative.TryStat(fd, out held) || !PosixNative.SameFileVersion(entry, held) || entry.LinkCount != held.LinkCount) + Changed(); + context.Hooks?.BeforeMoveSourceUnlink?.Invoke(source, Path.Combine(Path.GetDirectoryName(source)!, quarantine)); + if (!PosixNative.TryStat(fd, out var finalHeld) || !PosixNative.SameFileVersion(held, finalHeld) + || held.LinkCount != finalHeld.LinkCount + || !PosixNative.SameFileVersion(finalHeld, PosixNative.StatAt(parent.Descriptor, quarantine, source))) + Changed(); + PosixNative.UnlinkAt(parent.Descriptor, quarantine, expected.IsDirectory ? PosixNative.AtRemoveDir : 0, source); + } + catch + { + // Never overwrite a newly created source name. If restoration fails, retain quarantine. + PosixNative.TryQuarantineRename(parent.Descriptor, quarantine, name, out _, context.Hooks); + throw; + } + } + private static void VerifyMoveSource(DirectoryHandle parent, string name, PosixStat expected, string source) { var current = PosixNative.StatAt(parent.Descriptor, name, source); diff --git a/src/Files.Platform.Linux/Native/PosixFileOperationsNative.cs b/src/Files.Platform.Linux/Native/PosixFileOperationsNative.cs index b190d10b8538..c6e7d5bf06c7 100644 --- a/src/Files.Platform.Linux/Native/PosixFileOperationsNative.cs +++ b/src/Files.Platform.Linux/Native/PosixFileOperationsNative.cs @@ -33,6 +33,23 @@ public static PosixStat StatAt(int dirfd, string name, string displayPath) return stat; } + // Strict atomic rename: quarantine must never use the compatibility check/rename or link/unlink paths. + public static bool TryQuarantineRename(int dirfd, string source, string destination, out int errno, LinuxFileOperationsHooks? hooks, Action? beforeRename = null) + { + try + { + var injected = hooks?.RenameError?.Invoke(source, destination, false); + beforeRename?.Invoke(); + errno = injected + ?? (renameat2(dirfd, source, dirfd, destination, 1) == 0 ? 0 : Marshal.GetLastPInvokeError()); + } + catch (EntryPointNotFoundException) + { + errno = 38; + } + return errno == 0; + } + public static bool TryRenameAt(int sourceFd, string source, int destinationFd, string destination, bool replace, out int errno, LinuxFileOperationsHooks? hooks = null) { var cache = hooks?.UnsupportedRenameMounts ?? s_noRename2; diff --git a/src/Files.Platform.Linux/Native/PosixNative.cs b/src/Files.Platform.Linux/Native/PosixNative.cs index 37819abb5425..216a8f94345b 100644 --- a/src/Files.Platform.Linux/Native/PosixNative.cs +++ b/src/Files.Platform.Linux/Native/PosixNative.cs @@ -15,7 +15,7 @@ namespace Files.Platform.Linux.Native /// Mount-related statx fields: attribute bits with their support mask, the mount id (when reported) and the device. public readonly record struct MountInfo(ulong Attributes, ulong AttributesMask, bool MountIdValid, ulong MountId, uint DevMajor, uint DevMinor); - internal readonly record struct PosixStat(uint Mode, ulong Size, uint OwnerUserId, long ModifiedSeconds, uint ModifiedNanoseconds, ulong Inode = 0, uint DevMajor = 0, uint DevMinor = 0, ulong? MountId = null, long ChangedSeconds = 0, uint ChangedNanoseconds = 0) + internal readonly record struct PosixStat(uint Mode, ulong Size, uint OwnerUserId, long ModifiedSeconds, uint ModifiedNanoseconds, ulong Inode = 0, uint DevMajor = 0, uint DevMinor = 0, ulong? MountId = null, long ChangedSeconds = 0, uint ChangedNanoseconds = 0, uint LinkCount = 0) { public uint FileType => Mode & 0xF000; @@ -45,6 +45,7 @@ internal static unsafe partial class PosixNative private const uint StatxType = 0x1; private const uint StatxMode = 0x2; + private const uint StatxNlink = 0x4; private const uint StatxUid = 0x8; private const uint StatxMtime = 0x40; private const uint StatxCtime = 0x80; @@ -89,7 +90,7 @@ public static bool TryStat(int dirfd, string path, int flags, out PosixStat stat try { - const uint mask = StatxType | StatxMode | StatxUid | StatxMtime | StatxCtime | StatxSize | StatxIno | StatxMountId; + const uint mask = StatxType | StatxMode | StatxUid | StatxMtime | StatxCtime | StatxSize | StatxIno | StatxMountId | StatxNlink; if (!TryStatx(dirfd, path, flags, mask, out var buffer, out errno)) return false; @@ -106,7 +107,8 @@ public static bool TryStat(int dirfd, string path, int flags, out PosixStat stat (returned & StatxIno) != 0 ? BitConverter.ToUInt64(buffer, 32) : 0, BitConverter.ToUInt32(buffer, 136), BitConverter.ToUInt32(buffer, 140), (returned & StatxMountId) != 0 ? BitConverter.ToUInt64(buffer, 144) : null, (returned & StatxCtime) != 0 ? BitConverter.ToInt64(buffer, 96) : 0, - (returned & StatxCtime) != 0 ? BitConverter.ToUInt32(buffer, 104) : 0); + (returned & StatxCtime) != 0 ? BitConverter.ToUInt32(buffer, 104) : 0, + (returned & StatxNlink) != 0 ? BitConverter.ToUInt32(buffer, 16) : 0); return true; } catch (Exception ex) when (ex is DllNotFoundException or EntryPointNotFoundException) diff --git a/tests/Files.Platform.Tests/FileOperations/MoveQuarantineTests.cs b/tests/Files.Platform.Tests/FileOperations/MoveQuarantineTests.cs new file mode 100644 index 000000000000..ce50a4dd73e0 --- /dev/null +++ b/tests/Files.Platform.Tests/FileOperations/MoveQuarantineTests.cs @@ -0,0 +1,233 @@ +// Copyright (c) Files Community +// Licensed under the MIT License. + +using System.IO; +using System.Runtime.Versioning; +using System.Threading.Tasks; +using Files.Platform.Abstractions.FileOperations; +using Files.Platform.Linux.FileOperations; +using Microsoft.VisualStudio.TestTools.UnitTesting; + +namespace Files.Platform.Tests.FileOperations +{ + [TestClass] + [SupportedOSPlatform("linux")] + public sealed class MoveQuarantineTests : FileOperationsTestBase + { + [TestMethod] + [DataRow(false)] + [DataRow(true)] + public async Task SuccessfulFileDeletion_ReportsPathChangeAfterUnlink(bool child) + { + var source = Write(Path.Combine(Src, child ? "tree/a" : "a"), "OLD"); + var destination = Path.Combine(Dst, child ? "tree/a" : "a"); + var reported = false; + var result = await new LinuxFileOperationsService((_, _) => false).MoveAsync( + [child ? Path.GetDirectoryName(source)! : source], Dst, new FileOperationOptions + { + PathChanged = (from, to, recursive, stale) => + { + if (from != source) + return; + reported = true; + Assert.AreEqual(destination, to); + Assert.IsFalse(recursive); + Assert.IsTrue(stale); + Assert.IsFalse(File.Exists(source)); + Assert.IsEmpty(Directory.GetFileSystemEntries(Path.GetDirectoryName(source)!, ".linuxfiles-move-*")); + }, + }); + Assert.IsTrue(result[0].Succeeded, result[0].ErrorMessage); + Assert.IsTrue(reported); + } + + [TestMethod] + [DataRow(false)] + [DataRow(true)] + public async Task ChangedAfterVerification_PreservesSource(bool directory) + { + var source = directory ? Path.Combine(Src, "tree") : Write(Path.Combine(Src, "a"), "OLD"); + if (directory) + Write(Path.Combine(source, "child"), "OLD"); + var invoked = false; + var service = new LinuxFileOperationsService((_, _) => false, new LinuxFileOperationsHooks + { + AfterMoveSourceVerified = path => + { + if (path != source) + return; + invoked = true; + Write(directory ? Path.Combine(source, "new") : source, "CHANGED"); + }, + }); + var result = await service.MoveAsync([source], Dst); + Assert.IsTrue(invoked); + Assert.AreEqual(FileOperationErrorKind.VerificationFailed, result[0].ErrorKind); + Assert.AreEqual("CHANGED", File.ReadAllText(directory ? Path.Combine(source, "new") : source)); + } + + [TestMethod] + public async Task ReplacedBySymlinkDuringRename_RestoresReplacement() + { + var source = Write(Path.Combine(Src, "a"), "OLD"); + var precious = Write(Path.Combine(Root, "precious"), "KEEP"); + var swapped = false; + var service = new LinuxFileOperationsService((_, _) => false, new LinuxFileOperationsHooks + { + RenameError = (from, to, _) => + { + if (from == "a" && to.StartsWith(".linuxfiles-move-") && !swapped) + { + swapped = true; + File.Move(source, source + ".saved"); + File.CreateSymbolicLink(source, precious); + } + return null; + }, + }); + var result = await service.MoveAsync([source], Dst); + Assert.IsTrue(swapped); + Assert.AreEqual(FileOperationErrorKind.VerificationFailed, result[0].ErrorKind); + Assert.AreEqual(precious, new FileInfo(source).LinkTarget); + Assert.AreEqual("KEEP", File.ReadAllText(precious)); + Assert.AreEqual("OLD", File.ReadAllText(source + ".saved")); + Assert.IsEmpty(Directory.GetFileSystemEntries(Src, ".linuxfiles-move-*")); + } + + [TestMethod] + [DataRow(false, false)] + [DataRow(true, false)] + [DataRow(false, true)] + public async Task QuarantineMismatch_RestoresOrRetainsWithoutOverwriting(bool directory, bool occupied) + { + var source = directory ? Path.Combine(Src, "tree") : Write(Path.Combine(Src, "a"), "OLD"); + if (directory) + Write(Path.Combine(source, "child"), "OLD"); + string? quarantined = null; + var service = new LinuxFileOperationsService((_, _) => false, new LinuxFileOperationsHooks + { + MoveSourceQuarantined = (path, hidden) => + { + if (path != source) + return; + quarantined = hidden; + Write(directory ? Path.Combine(hidden, "new") : hidden, "CHANGED"); + if (occupied) + Write(source, "KEEP"); + }, + }); + var result = await service.MoveAsync([source], Dst); + Assert.IsNotNull(quarantined); + Assert.AreEqual(FileOperationErrorKind.VerificationFailed, result[0].ErrorKind); + var retained = occupied ? quarantined : source; + Assert.AreEqual("CHANGED", File.ReadAllText(directory ? Path.Combine(retained, "new") : retained)); + if (occupied) + Assert.AreEqual("KEEP", File.ReadAllText(source)); + else + Assert.IsEmpty(Directory.GetFileSystemEntries(Src, ".linuxfiles-move-*")); + } + + [TestMethod] + public async Task EditedBetweenFinalStatAndRename_RestoresChangedSource() + { + var source = Write(Path.Combine(Src, "a"), "OLD"); + var edited = false; + var service = new LinuxFileOperationsService((_, _) => false, new LinuxFileOperationsHooks + { + RenameError = (from, to, _) => + { + if (from == "a" && to.StartsWith(".linuxfiles-move-")) + { + edited = true; + Write(source, "CHANGED AFTER STAT"); + } + return null; + }, + }); + var result = await service.MoveAsync([source], Dst); + Assert.IsTrue(edited); + Assert.AreEqual(FileOperationErrorKind.VerificationFailed, result[0].ErrorKind); + Assert.AreEqual("CHANGED AFTER STAT", File.ReadAllText(source)); + Assert.AreEqual("OLD", File.ReadAllText(Path.Combine(Dst, "a"))); + Assert.IsEmpty(Directory.GetFileSystemEntries(Src, ".linuxfiles-move-*")); + } + + [TestMethod] + public async Task EqualLengthEditWithRestoredMtimeBeforeRename_PreservesSource() + { + var source = Write(Path.Combine(Src, "a"), "OLD"); + var mtime = File.GetLastWriteTimeUtc(source); + var edited = false; + var service = new LinuxFileOperationsService((_, _) => false, new LinuxFileOperationsHooks + { + RenameError = (from, to, _) => + { + if (from == "a" && to.StartsWith(".linuxfiles-move-")) + { + edited = true; + Write(source, "NEW"); + File.SetLastWriteTimeUtc(source, mtime); + } + return null; + }, + }); + var result = await service.MoveAsync([source], Dst); + Assert.IsTrue(edited); + Assert.AreEqual(FileOperationErrorKind.VerificationFailed, result[0].ErrorKind); + Assert.AreEqual("NEW", File.ReadAllText(source)); + Assert.AreEqual("OLD", File.ReadAllText(Path.Combine(Dst, "a"))); + Assert.IsEmpty(Directory.GetFileSystemEntries(Src, ".linuxfiles-move-*")); + } + + [TestMethod] + [DataRow(0, false)] + [DataRow(0, true)] + [DataRow(22, false)] + [DataRow(38, false)] + [DataRow(95, false)] + [DataRow(22, true)] + public async Task ChangedImmediatelyBeforeUnlink_PreservesNewData(int errno, bool replacement) + { + var source = Write(Path.Combine(Src, "a"), "OLD"); + var invoked = false; + var service = new LinuxFileOperationsService((_, _) => false, new LinuxFileOperationsHooks + { + RenameError = (_, to, _) => errno != 0 && to.StartsWith(".linuxfiles-move-") ? errno : null, + BeforeMoveSourceUnlink = (_, entry) => + { + invoked = true; + var mtime = File.GetLastWriteTimeUtc(entry); + if (replacement) + File.Move(entry, source + ".saved"); + Write(entry, "NEW"); + File.SetLastWriteTimeUtc(entry, mtime); + }, + }); + var result = await service.MoveAsync([source], Dst); + Assert.IsTrue(invoked); + Assert.AreEqual(FileOperationErrorKind.VerificationFailed, result[0].ErrorKind); + Assert.AreEqual("NEW", File.ReadAllText(source)); + Assert.AreEqual("OLD", File.ReadAllText(Path.Combine(Dst, "a"))); + if (replacement) + Assert.AreEqual("OLD", File.ReadAllText(source + ".saved")); + Assert.IsEmpty(Directory.GetFileSystemEntries(Src, ".linuxfiles-move-*")); + } + + [TestMethod] + [DataRow(22)] + [DataRow(38)] + [DataRow(95)] + public async Task UnsupportedRename_UsesVerifiedFallback(int errno) + { + var source = Write(Path.Combine(Src, "a"), "OLD"); + var service = new LinuxFileOperationsService((_, _) => false, new LinuxFileOperationsHooks + { + RenameError = (_, to, _) => to.StartsWith(".linuxfiles-move-") ? errno : null, + }); + var result = await service.MoveAsync([source], Dst); + Assert.IsTrue(result[0].Succeeded, result[0].ErrorMessage); + Assert.IsFalse(File.Exists(source)); + Assert.AreEqual("OLD", File.ReadAllText(Path.Combine(Dst, "a"))); + } + } +}