diff --git a/backend/src/Taskdeck.Api/FirstRun/FirstRunBootstrapper.cs b/backend/src/Taskdeck.Api/FirstRun/FirstRunBootstrapper.cs index 46524e6b5..8ec8b0f20 100644 --- a/backend/src/Taskdeck.Api/FirstRun/FirstRunBootstrapper.cs +++ b/backend/src/Taskdeck.Api/FirstRun/FirstRunBootstrapper.cs @@ -562,28 +562,45 @@ private static void PreserveCorruptConfig(string path, Exception parseError) var backupPath = $"{path}.corrupt-{DateTime.UtcNow:yyyyMMddHHmmssfff}"; try { - File.Copy(path, backupPath, overwrite: false); + // The .corrupt-* backup holds the same secrets as the original (the connector key in + // particular), so create it ATOMICALLY with owner-only permissions (#1241/#1264) -- a + // copy-then-restrict sequence would briefly expose it via the directory's default ACL + // (File.Copy does not carry the source's security descriptor). Byte-faithful read: the file is + // unparsable, possibly from an interrupted write, so no text decoding is applied. + WriteRestrictedFile(backupPath, File.ReadAllBytes(path)); } - catch (Exception copyEx) + catch (Exception ex) { - Console.Error.WriteLine( - $"[FirstRun] WARNING: {path} contains invalid JSON ({parseError.Message}) and could NOT be " + - $"backed up ({copyEx.Message}); it will be overwritten."); - return; - } + // Recovery-first fallback: a failed primary backup must not cost the ONLY copy of a + // possibly-recoverable key. Fall back to a plain copy + best-effort restriction, warning that + // the backup may be readable by other local users until fixed. Copy to a DISTINCT name: if the + // primary attempt's best-effort cleanup could not remove its partial file (e.g. an AV/indexer + // held a same-user handle), overwrite:false on the same name would always fail here even though + // the source is still perfectly readable. + backupPath = $"{backupPath}.fallback"; + try + { + File.Copy(path, backupPath, overwrite: false); + } + catch (Exception copyEx) + { + Console.Error.WriteLine( + $"[FirstRun] WARNING: {path} contains invalid JSON ({parseError.Message}) and could NOT be " + + $"backed up (primary backup: {ex.Message}; fallback copy: {copyEx.Message}); it will be " + + "overwritten."); + return; + } - // The .corrupt-* backup holds the same secrets as the original (the connector key in particular), so - // lock it to the current user too -- otherwise recovery-preservation would re-expose it via the - // directory's default ACL (#1241). Best-effort: a failed restriction warns but keeps the backup. - try - { - RestrictFileToCurrentUser(backupPath); - } - catch (Exception aclEx) - { - Console.Error.WriteLine( - $"[FirstRun] WARNING: preserved {backupPath} but could not restrict its permissions " + - $"({aclEx.Message}); it may be readable by other local users until fixed."); + try + { + RestrictFileToCurrentUser(backupPath); + } + catch (Exception aclEx) + { + Console.Error.WriteLine( + $"[FirstRun] WARNING: preserved {backupPath} but could not restrict its permissions " + + $"({aclEx.Message}); it may be readable by other local users until fixed."); + } } Console.Error.WriteLine( @@ -684,22 +701,20 @@ or WaitHandleCannotBeOpenedException Directory.CreateDirectory(dir); var tempPath = Path.Combine(dir, $".{Path.GetFileName(path)}.{Guid.NewGuid():N}.tmp"); - // Create the file empty and lock it to the current user BEFORE writing the secret payload, - // closing the window where the JWT secret + connector key would sit on disk readable by other - // local users: under the Unix default umask (022) the file would be 0644, and on Windows it would - // inherit the directory's default ACL (e.g. BUILTIN\Users read). #1241. A narrow race remains: - // the empty temp file briefly exists with default permissions, so a racer could pre-open a read - // handle that survives the restriction (tightening ACLs does not revoke open handles) -- creating - // the file atomically WITH the restricted ACL closes that too and is tracked in #1264. If the file - // cannot be locked down, RestrictFileToCurrentUser throws (as IOException) and the secret is never - // written to it -- the caller falls back to an in-memory value rather than leaving it world-readable. - // The whole create -> restrict -> write -> move sequence is inside the try so any failure (incl. a - // failed restrict) deletes the staged temp file rather than leaking it. + // Create the temp file ATOMICALLY with owner-only permissions and FileShare.None (#1241/#1264): + // under the Unix default umask (022) a plainly-created file would be 0644, and on Windows it + // would inherit the directory's default ACL (e.g. BUILTIN\Users read). Supplying the restrictive + // mode/DACL at creation leaves no instant where the file is openable by another local user -- + // the previous create-then-restrict sequence had a window where a racer's pre-opened handle on + // the empty file survived the tightened ACL (ACL changes do not revoke open handles) and could + // read the later-written secret. FileMode.CreateNew also refuses to adopt a pre-created file. + // If the restricted create fails, WriteRestrictedFile throws (normalized to IOException) and the + // secret is never written -- the caller falls back to an in-memory value rather than leaving it + // world-readable. The create+write -> move sequence is inside the try so any failure deletes the + // staged temp file rather than leaking it. try { - File.Create(tempPath).Dispose(); - RestrictFileToCurrentUser(tempPath); - File.WriteAllText(tempPath, payload); + WriteRestrictedFile(tempPath, payload); ReplaceFileWithRetry(tempPath, path); } catch @@ -746,13 +761,150 @@ internal static void RestrictExistingLocalConfigFile() } /// - /// Restricts a freshly-created (still-empty) file to the current user only, BEFORE secrets are written - /// into it, so appsettings.local.json (JWT secret + connector key) is never readable by other - /// local users (#1241). On Unix this is 0600; on Windows it replaces the DACL with a single - /// owner-only ACE and disables inheritance (so the directory's default ACEs -- e.g. BUILTIN\Users read -- - /// do not apply). Any failure is normalized to so the first-run callers' - /// existing catch (IOException) falls back to an in-memory value -- the secret is never written to - /// a file we could not lock down. + /// Creates ATOMICALLY with owner-only permissions and + /// , then writes through that same handle + /// (#1264). On Unix the file is born 0600 () and + /// the exact mode is then pinned through the open handle (umask-proof); on Windows the protected + /// owner-only DACL is supplied to CreateFile itself and read back through the open handle, so a + /// filesystem that silently ignores security descriptors (FAT32/exFAT, some SMB shares) fails closed + /// instead of persisting the secret unprotected. Unlike create-then-restrict there is no instant at + /// which another local user can open the file, and no pre-opened handle can survive into the written + /// secret; additionally refuses to adopt a file someone pre-created at + /// the path. Any failure is normalized to + /// (matching ) so first-run callers' + /// catch (IOException) falls back to an in-memory value; a partially-written file is + /// best-effort deleted so callers never observe a half-written secret file. + /// + internal static void WriteRestrictedFile(string path, string contents) + => WriteRestrictedFile(path, Encoding.UTF8.GetBytes(contents)); + + internal static void WriteRestrictedFile(string path, byte[] contents) + { + FileStream stream; + try + { + stream = CreateRestrictedNewFile(path); + } + catch (IOException) + { + // Creation failed -> nothing of ours remains at the path (CreateRestrictedNewFile removes its + // own file when the post-create lockdown pin/verification fails); never delete what we did not + // create (CreateNew fails precisely when the path is already occupied). + throw; + } + catch (Exception ex) + { + // Normalize (e.g. UnauthorizedAccessException, PlatformNotSupportedException) so the callers' + // catch(IOException) handles it uniformly -- the secret is never written to an unprotected file. + throw new IOException( + $"Could not create {path} restricted to the current user; refusing to write the secret to it.", ex); + } + + try + { + using (stream) + { + stream.Write(contents, 0, contents.Length); + } + } + catch (Exception ex) + { + // We created the file, so a failed/partial write cleans it up (it was restricted from birth, so + // this is consistency hygiene, not exposure). Best-effort; the original failure propagates. + try { File.Delete(path); } catch { /* ignore */ } + if (ex is IOException) + { + throw; + } + + throw new IOException( + $"Could not write the restricted file {path}; refusing to leave a partial secret file.", ex); + } + } + + private static FileStream CreateRestrictedNewFile(string path) + { + if (OperatingSystem.IsWindows()) + { + return CreateOwnerOnlyFileWindows(path); + } + + var stream = new FileStream(path, new FileStreamOptions + { + Mode = FileMode.CreateNew, + Access = FileAccess.Write, + Share = FileShare.None, + UnixCreateMode = UnixFileMode.UserRead | UnixFileMode.UserWrite, + }); + try + { + // open(2)'s mode argument is umask-masked (the mask can only STRIP bits from 0600, never widen + // it) and is silently ignored on non-POSIX filesystems (e.g. vfat). Pin exactly 0600 through the + // open handle (fchmod -- race-free): this restores the exact-mode guarantee regardless of umask, + // and it FAILS on filesystems that cannot store the mode exactly where the pre-#1264 + // SetUnixFileMode(path) call failed -- keeping the fail-closed contract instead of silently + // persisting the secret with whatever mode the mount dictates. + File.SetUnixFileMode(stream.SafeFileHandle, UnixFileMode.UserRead | UnixFileMode.UserWrite); + return stream; + } + catch + { + // We created this file; a failed lockdown must not leave it behind. Best-effort; the original + // exception propagates (normalized to IOException by the caller). + stream.Dispose(); + try { File.Delete(path); } catch { /* ignore */ } + throw; + } + } + + [SupportedOSPlatform("windows")] + private static FileStream CreateOwnerOnlyFileWindows(string path) + { + // ReadPermissions (READ_CONTROL) is requested alongside Write so the DACL VERIFICATION below can + // read the security descriptor back through this same exclusive handle. + var stream = new FileInfo(path).Create( + FileMode.CreateNew, + FileSystemRights.Write | FileSystemRights.ReadPermissions, + FileShare.None, + bufferSize: 4096, + FileOptions.None, + BuildOwnerOnlyFileSecurity()); + try + { + // CreateFileW silently IGNORES the supplied security descriptor on filesystems without ACL + // support (FAT32/exFAT, some SMB shares): the create succeeds and the file is world-readable. + // The pre-#1264 SetAccessControl path FAILED there (fail-closed). Read the DACL back through the + // open handle to restore that contract -- on NTFS this merely confirms the atomically-applied + // descriptor; on a non-ACL volume it throws or reports an unprotected DACL and we refuse. + var applied = stream.GetAccessControl(); + if (!applied.AreAccessRulesProtected) + { + throw new IOException( + $"The filesystem hosting {path} did not honor the owner-only ACL (FAT32/exFAT and some " + + "network shares cannot store it); refusing to write the secret to an unprotected file."); + } + + return stream; + } + catch + { + // We created this file; a failed lockdown verification must not leave it behind. Best-effort; + // the original exception propagates (normalized to IOException by the caller). + stream.Dispose(); + try { File.Delete(path); } catch { /* ignore */ } + throw; + } + } + + /// + /// Restricts an EXISTING file to the current user only (#1241). On Unix this is 0600; on Windows + /// it replaces the DACL with a single owner-only ACE and disables inheritance (so the directory's + /// default ACEs -- e.g. BUILTIN\Users read -- do not apply). Used for forward remediation of files that + /// already exist (, the corrupt-backup fallback); NEW + /// secret files are instead created atomically restricted via + /// (#1264). + /// Any failure is normalized to so callers' + /// catch (IOException) handle it uniformly. /// internal static void RestrictFileToCurrentUser(string path) { @@ -764,7 +916,7 @@ internal static void RestrictFileToCurrentUser(string path) return; } - RestrictFileToCurrentUserWindows(path); + new FileInfo(path).SetAccessControl(BuildOwnerOnlyFileSecurity()); } catch (IOException) { @@ -780,7 +932,7 @@ internal static void RestrictFileToCurrentUser(string path) } [SupportedOSPlatform("windows")] - private static void RestrictFileToCurrentUserWindows(string path) + private static FileSecurity BuildOwnerOnlyFileSecurity() { using var identity = WindowsIdentity.GetCurrent(); var owner = identity.User; @@ -796,7 +948,7 @@ private static void RestrictFileToCurrentUserWindows(string path) // Drop inherited ACEs and grant the current user full control -- the only ACE on the file. security.SetAccessRuleProtection(isProtected: true, preserveInheritance: false); security.AddAccessRule(new FileSystemAccessRule(owner, FileSystemRights.FullControl, AccessControlType.Allow)); - new FileInfo(path).SetAccessControl(security); + return security; } private static void ReplaceFileWithRetry(string tempPath, string path) diff --git a/backend/tests/Taskdeck.Api.Tests/FirstRun/FirstRunBootstrapperTests.cs b/backend/tests/Taskdeck.Api.Tests/FirstRun/FirstRunBootstrapperTests.cs index aaaeb4bbc..7ed208538 100644 --- a/backend/tests/Taskdeck.Api.Tests/FirstRun/FirstRunBootstrapperTests.cs +++ b/backend/tests/Taskdeck.Api.Tests/FirstRun/FirstRunBootstrapperTests.cs @@ -309,16 +309,38 @@ public void PersistValue_MutexConstructorIsGuarded_StructuralCheck() } [Fact] - public void PersistValue_SetsUnixFileMode_StructuralCheck() + public void PersistValue_WritesSecretsViaAtomicRestrictedCreate_StructuralCheck() { - // Verify that the temp file gets 0600 permissions on Unix before being - // moved into place, matching the CLI's CliFirstRunBootstrapper pattern. + // #1264 load-bearing wiring: the behavioral tests cannot distinguish atomic create-with-permissions + // from a regression back to create-then-restrict (the final file state is identical on NTFS), so pin + // the construction structurally. PersistValue's temp file AND the corrupt-config backup must go + // through WriteRestrictedFile; the helper must create with the permissions supplied at creation + // (UnixCreateMode / FileSecurity passed to Create) rather than restrict post-hoc. var source = File.ReadAllText( FindSourceFile("FirstRunBootstrapper.cs")); - Assert.Contains("SetUnixFileMode", source); - Assert.Contains("UnixFileMode.UserRead | UnixFileMode.UserWrite", source); - Assert.Contains("!OperatingSystem.IsWindows()", source); + Assert.Contains("WriteRestrictedFile(tempPath, payload)", source); + Assert.Contains("WriteRestrictedFile(backupPath", source); + Assert.Contains("UnixCreateMode = UnixFileMode.UserRead | UnixFileMode.UserWrite", source); + Assert.Contains("BuildOwnerOnlyFileSecurity())", source); + // The pre-#1264 sequence must not come back. + Assert.DoesNotContain("File.Create(tempPath)", source); + Assert.DoesNotContain("RestrictFileToCurrentUser(tempPath)", source); + } + + [Fact] + public void WriteRestrictedFile_FailsClosedOnNonAclFilesystems_StructuralCheck() + { + // On FAT32/exFAT/some SMB shares CreateFileW silently IGNORES the supplied security descriptor, and + // on non-POSIX mounts open(2)'s mode is ignored -- where the pre-#1264 restrict calls FAILED + // (fail-closed). Pin the two post-create guards that restore that contract: the Windows DACL + // read-back through the open handle and the Unix exact-mode pin (also umask-proof) through the + // open handle. + var source = File.ReadAllText( + FindSourceFile("FirstRunBootstrapper.cs")); + + Assert.Contains("AreAccessRulesProtected", source); + Assert.Contains("File.SetUnixFileMode(stream.SafeFileHandle", source); } [Fact] @@ -396,12 +418,13 @@ public void RestrictFileToCurrentUser_LocksFileToCurrentUserOnly() } [Fact] - public void RestrictFileToCurrentUser_LockdownSurvivesAtomicMove() + public void WriteRestrictedFile_LockdownSurvivesAtomicMove() { - // #1241 load-bearing property: PersistValue restricts a temp file, then File.Move(overwrite)s it onto - // the target. This asserts the owner-only lockdown survives that same-directory atomic rename. A - // regression to copy-then-delete or File.Replace (which preserves the DESTINATION's ACL) would - // silently defeat the fix; the in-isolation helper test would not catch it. + // #1241/#1264 load-bearing property: PersistValue atomically creates a restricted temp file, then + // File.Move(overwrite)s it onto the target. This asserts the owner-only lockdown survives that + // same-directory atomic rename. A regression to copy-then-delete or File.Replace (which preserves + // the DESTINATION's ACL) would silently defeat the fix; the in-isolation helper test would not + // catch it. var dir = Path.Combine(Path.GetTempPath(), $"td-acl-move-{Guid.NewGuid():N}"); Directory.CreateDirectory(dir); var tempPath = Path.Combine(dir, ".secrets.tmp"); @@ -411,9 +434,7 @@ public void RestrictFileToCurrentUser_LockdownSurvivesAtomicMove() // Pre-seed an existing (unrestricted) target so the move is a real overwrite. File.WriteAllText(targetPath, "{}"); - File.Create(tempPath).Dispose(); - FirstRunBootstrapper.RestrictFileToCurrentUser(tempPath); - File.WriteAllText(tempPath, "{\"secret\":\"x\"}"); + FirstRunBootstrapper.WriteRestrictedFile(tempPath, "{\"secret\":\"x\"}"); File.Move(tempPath, targetPath, overwrite: true); if (OperatingSystem.IsWindows()) @@ -447,6 +468,118 @@ public void RestrictFileToCurrentUser_LockdownSurvivesAtomicMove() } } + [Fact] + public void WriteRestrictedFile_CreatesFileAtomicallyLockedToCurrentUser() + { + // #1264 behavioral test (each CI runner covers its own platform branch): the file must be BORN with + // owner-only permissions -- Unix 0600 via FileStreamOptions.UnixCreateMode, Windows via the + // protected owner-only DACL supplied to CreateFile -- and hold the exact written content. + var path = Path.Combine(Path.GetTempPath(), $"td-atomic-{Guid.NewGuid():N}.tmp"); + try + { + FirstRunBootstrapper.WriteRestrictedFile(path, "{\"secret\":\"atomic\"}"); + + Assert.Equal("{\"secret\":\"atomic\"}", File.ReadAllText(path)); + if (OperatingSystem.IsWindows()) + { + var security = new FileInfo(path).GetAccessControl(); + Assert.True( + security.AreAccessRulesProtected, + "inheritance should be disabled so the directory's default ACEs do not apply"); + + using var identity = WindowsIdentity.GetCurrent(); + var currentSid = identity.User; + var rules = security.GetAccessRules( + includeExplicit: true, includeInherited: true, typeof(SecurityIdentifier)); + Assert.True(rules.Count > 0, "the file should have at least one explicit access rule"); + foreach (FileSystemAccessRule rule in rules) + { + Assert.Equal(AccessControlType.Allow, rule.AccessControlType); + Assert.Equal(currentSid, rule.IdentityReference); + } + } + else + { + Assert.Equal( + UnixFileMode.UserRead | UnixFileMode.UserWrite, + File.GetUnixFileMode(path)); + } + } + finally + { + if (File.Exists(path)) File.Delete(path); + } + } + + [Fact] + public void WriteRestrictedFile_RefusesToAdoptAPreExistingFile() + { + // #1264: FileMode.CreateNew must fail on an already-occupied path rather than write the secret into + // a file someone else created (whose handle/permissions we do not control) -- and it must not + // delete or overwrite that pre-existing file. + var path = Path.Combine(Path.GetTempPath(), $"td-preexist-{Guid.NewGuid():N}.tmp"); + File.WriteAllText(path, "pre-existing"); + try + { + Assert.Throws(() => FirstRunBootstrapper.WriteRestrictedFile(path, "secret")); + + Assert.True(File.Exists(path), "the pre-existing file must not be deleted on a refused create"); + Assert.Equal("pre-existing", File.ReadAllText(path)); + } + finally + { + File.Delete(path); + } + } + + [Fact] + public void QuarantineCorruptLocalConfigAt_BackupIsRestrictedToCurrentUser() + { + // #1241/#1264: the .corrupt-* backup holds the same secrets as the original, so it must be created + // with owner-only permissions (atomically, not copy-then-restrict) and stay byte-faithful for key + // recovery. + var dir = Path.Combine(Path.GetTempPath(), $"td-corrupt-acl-{Guid.NewGuid():N}"); + Directory.CreateDirectory(dir); + var path = Path.Combine(dir, "appsettings.local.json"); + File.WriteAllText(path, "{ corrupt but holds : the only key copy"); + try + { + FirstRunBootstrapper.QuarantineCorruptLocalConfigAt(path); + + var preserved = Directory.GetFiles(dir, "appsettings.local.json.corrupt-*"); + Assert.Single(preserved); + Assert.Equal("{ corrupt but holds : the only key copy", File.ReadAllText(preserved[0])); + if (OperatingSystem.IsWindows()) + { + var security = new FileInfo(preserved[0]).GetAccessControl(); + Assert.True( + security.AreAccessRulesProtected, + "the backup must carry the protected (non-inherited) owner-only DACL"); + + using var identity = WindowsIdentity.GetCurrent(); + var currentSid = identity.User; + var rules = security.GetAccessRules( + includeExplicit: true, includeInherited: true, typeof(SecurityIdentifier)); + Assert.True(rules.Count > 0); + foreach (FileSystemAccessRule rule in rules) + { + Assert.Equal(AccessControlType.Allow, rule.AccessControlType); + Assert.Equal(currentSid, rule.IdentityReference); + } + } + else + { + Assert.Equal( + UnixFileMode.UserRead | UnixFileMode.UserWrite, + File.GetUnixFileMode(preserved[0])); + } + } + finally + { + try { Directory.Delete(dir, recursive: true); } catch { /* best-effort */ } + } + } + private static string FindSourceFile(string fileName) { // Walk up from the test output directory to find the repo source.