Restrict C:\ProgramData\fips to SYSTEM and Administrators

install-service.ps1 created C:\ProgramData\fips without setting an ACL,
so it inherited C:\ProgramData's access: any local user could read the
node's identity key there, and could create a fips.yaml, fips.key,
hosts, peers.allow or peers.deny in the directory before one existed,
for the service, running as LocalSystem, to read. The service log is
written there too.

The installer now builds an ACL that grants only SYSTEM and
Administrators and does not inherit, and creates a new directory with
that ACL already applied, using the .NET call each PowerShell edition
provides. Created first and restricted afterwards, the empty directory
would carry C:\ProgramData's access in between, and any local user
could turn it into a junction in that time with no privilege and
without changing its owner.

An existing directory is refused if it is a link or junction or is
owned by an account other than SYSTEM, Administrators or the installing
administrator, since a user who created it could swap it for a link to
another directory. Otherwise the installer makes Administrators the
owner, refuses the directory if it has since become a link and any link
or folder inside it, applies the ACL, and resets the owner and ACL of
each existing file. That check runs before the ACL is applied, because
applying it rewrites the inherited entries of everything below the
directory, and runs again after it and after the reset. Every icacls
call acts on a link rather than its target and has its exit code
checked. Files created in the directory later, by the service or
fipsctl keygen, take the same ACL. Well-known SIDs are used so the step
works on non-English Windows.

A user who created the directory keeps full control of it through the
entry C:\ProgramData's CREATOR OWNER grant leaves for their account,
and taking ownership changes only the owner. So each refusal tells the
operator to copy out anything needed and delete the directory, taking
ownership only if that is what it takes to delete it, and an icacls
failure says to delete the directory or the file, or simply retry if
the file has gone.

Packaging tests check the order of these steps and that they come
before any path inside the directory is touched; that the ACL is built
with both SIDs and both inheritance flags and passed to the directory's
creation; the refusal conditions themselves (the owner test, the
directory link test at the start and in each recheck, and a link or
folder entry each refused on its own); that every icacls call acts on
links and is checked; and that takeown is never offered as the
recovery. The README and the fips reference say that reading the
directory now needs an elevated prompt (an unelevated foreground run
may skip the config there or fail with an access error), that the
installer should run before fipsctl keygen, and that it should run
again after files are moved into the directory, since a moved file
keeps its old permissions.

docs/reference/cli-fips.md

@@ -81,7 +81,7 @@ writes, on first start) the identity files:
 
 | File | Mode | Purpose |
 | ---- | ---- | ------- |
-| `fips.key` | `0600` | Bech32 nsec for the persistent identity (Unix only; Windows inherits parent ACLs). |
+| `fips.key` | `0600` | Bech32 nsec for the persistent identity (Unix; on Windows the file takes its directory's ACL, which `install-service.ps1` restricts to SYSTEM and Administrators). |
 | `fips.pub` | `0644` | Bech32 npub corresponding to `fips.key`. |
 
 When `node.identity.persistent` is `false` (the default), a fresh

packaging/windows/build-zip.ps1

@@ -105,6 +105,14 @@ Configuration:
   peers.allow and peers.deny beside it. Edit fips.yaml there
   before starting the service.
 
+  install-service.ps1 restricts C:\ProgramData\fips to SYSTEM
+  and Administrators before writing into it. Reading or editing
+  files there, fipsctl keygen, fipsctl address with no argument,
+  and a foreground fips.exe run that relies on
+  C:\ProgramData\fips\fips.yaml all need an elevated prompt.
+  Unelevated, a foreground run may skip that file without
+  saying so, or may fail with an access error.
+
   A foreground run takes -c <file>, or reads
   C:\ProgramData\fips\fips.yaml and then, as per-user overrides
   the service does not read, %APPDATA%\fips\fips.yaml,
@@ -112,7 +120,14 @@ Configuration:
   beside the last config loaded.
 
   fipsctl keygen writes to C:\ProgramData\fips by default and
-  needs an elevated prompt.
+  needs an elevated prompt. Run install-service.ps1 before it:
+  a directory that keygen creates first carries
+  C:\ProgramData's access until the installer restricts it.
+
+  A file moved into C:\ProgramData\fips keeps its old
+  permissions. That includes a fips.yaml or fips.key moved from
+  %APPDATA%\fips as the daemon's warning suggests, so run
+  install-service.ps1 again after moving files there.
 
 Logs:
   The service logs to C:\ProgramData\fips\fips.log, rolled at

packaging/windows/install-service.ps1

@@ -19,9 +19,116 @@ $ConfigDir = "$env:ProgramData\fips"
 
 Write-Host "Installing FIPS service..."
 
-# Create directories
+# Create the install directory
 New-Item -ItemType Directory -Force -Path $InstallDir | Out-Null
-New-Item -ItemType Directory -Force -Path $ConfigDir | Out-Null
+
+# Create the config directory, or restrict an existing one, so that only
+# SYSTEM and Administrators can use it before anything is written into it.
+# The service keeps its identity key, config, hosts file, peer ACL files and
+# log there, and the ACL inherited from C:\ProgramData lets any local user
+# read those files and create missing ones. A user may also have created the
+# directory, or a link in its place, before this script ran, so a link, a
+# directory owned by another account, and a link or folder inside it are
+# refused rather than acted on. A user who created the directory keeps full
+# control of it through an inherited entry even after an administrator takes
+# ownership, so the only recovery offered for a refused directory is to
+# delete it. Well-known SIDs are used because account names are translated on
+# non-English Windows.
+$icacls = "$env:SystemRoot\System32\icacls.exe"
+
+# A new object, so only the DACL is written and any explicit entry an
+# existing directory carried is dropped; inheritance from C:\ProgramData is
+# turned off.
+$acl = New-Object System.Security.AccessControl.DirectorySecurity
+$acl.SetAccessRuleProtection($true, $false)
+$inherit = [System.Security.AccessControl.InheritanceFlags]::ContainerInherit -bor [System.Security.AccessControl.InheritanceFlags]::ObjectInherit
+foreach ($sid in @("S-1-5-18", "S-1-5-32-544")) {
+    $rule = [System.Security.AccessControl.FileSystemAccessRule]::new(
+        [System.Security.Principal.SecurityIdentifier]::new($sid),
+        [System.Security.AccessControl.FileSystemRights]::FullControl,
+        $inherit,
+        [System.Security.AccessControl.PropagationFlags]::None,
+        [System.Security.AccessControl.AccessControlType]::Allow)
+    $acl.AddAccessRule($rule)
+}
+
+# A new directory is created with this ACL in one step. Created first and
+# restricted afterwards, it would carry C:\ProgramData's access in between,
+# and a user could turn the empty directory into a junction in that time.
+# Both calls leave an existing directory, or a link at the path, as it is.
+# Windows PowerShell's .NET Framework takes the ACL in
+# Directory.CreateDirectory; PowerShell 7 takes it in
+# FileSystemAclExtensions.CreateDirectory instead.
+if ($PSVersionTable.PSEdition -eq "Core") {
+    [System.IO.FileSystemAclExtensions]::CreateDirectory($acl, $ConfigDir) | Out-Null
+} else {
+    [System.IO.Directory]::CreateDirectory($ConfigDir, $acl) | Out-Null
+}
+
+$dirItem = Get-Item -LiteralPath $ConfigDir -Force
+if ($dirItem.Attributes -band [System.IO.FileAttributes]::ReparsePoint) {
+    Write-Error "$ConfigDir is a link or junction, not a directory. Remove it, then run install-service.ps1 again."
+    exit 1
+}
+
+try {
+    $ownerSid = (Get-Acl -LiteralPath $ConfigDir).GetOwner([System.Security.Principal.SecurityIdentifier]).Value
+} catch {
+    Write-Error "Cannot read the owner of $ConfigDir. Another account may have created it and placed files in it. Copy out anything you need, delete the directory, then run install-service.ps1 again. If Windows refuses the deletion, take ownership first, but still delete it: taking ownership leaves its creator full control of it."
+    exit 1
+}
+$trustedOwners = @("S-1-5-18", "S-1-5-32-544", [System.Security.Principal.WindowsIdentity]::GetCurrent().User.Value)
+if ($trustedOwners -notcontains $ownerSid) {
+    Write-Error "$ConfigDir is owned by $ownerSid, not by SYSTEM, Administrators or this account. Another account may have created it and placed files in it. Copy out anything you need, delete the directory, then run install-service.ps1 again. If Windows refuses the deletion, take ownership first, but still delete it: taking ownership leaves its creator full control of it."
+    exit 1
+}
+
+& $icacls $ConfigDir /setowner "*S-1-5-32-544" /L /Q
+if ($LASTEXITCODE -ne 0) {
+    Write-Error "icacls could not set the owner of $ConfigDir (exit code $LASTEXITCODE). Another account may have changed its permissions. Copy out anything you need, delete the directory, then run install-service.ps1 again."
+    exit 1
+}
+
+# FIPS keeps only files in the directory. Applying the new ACL propagates
+# into existing entries, so links and folders are refused before it as well
+# as after it and after each file is reset. An existing directory that is
+# still empty can be turned into a junction by any user until the ACL is
+# applied, so each check also refuses the directory itself if it has become
+# a link.
+$refuseEntries = {
+    if ((Get-Item -LiteralPath $ConfigDir -Force).Attributes -band [System.IO.FileAttributes]::ReparsePoint) {
+        Write-Error "$ConfigDir became a link or junction while the installer ran. Another account may have converted it, and the permissions of the folder it points to may have been changed. Remove the link, then run install-service.ps1 again."
+        exit 1
+    }
+    foreach ($item in @(Get-ChildItem -LiteralPath $ConfigDir -Force)) {
+        if (($item.Attributes -band [System.IO.FileAttributes]::ReparsePoint) -or $item.PSIsContainer) {
+            Write-Error "$($item.FullName) is a link or a folder, and FIPS keeps only files in $ConfigDir. Inspect and remove that entry, then run install-service.ps1 again."
+            exit 1
+        }
+    }
+}
+& $refuseEntries
+
+Set-Acl -LiteralPath $ConfigDir -AclObject $acl
+
+& $refuseEntries
+
+foreach ($item in @(Get-ChildItem -LiteralPath $ConfigDir -Force)) {
+    & $icacls $item.FullName /setowner "*S-1-5-32-544" /L /Q
+    if ($LASTEXITCODE -ne 0) {
+        Write-Error "icacls could not set the owner of $($item.FullName) (exit code $LASTEXITCODE). Another account may have placed or changed this file. Delete the file if it is still there, then run install-service.ps1 again."
+        exit 1
+    }
+    & $icacls $item.FullName /reset /L /Q
+    if ($LASTEXITCODE -ne 0) {
+        Write-Error "icacls could not reset the ACL of $($item.FullName) (exit code $LASTEXITCODE). Another account may have placed or changed this file. Delete the file if it is still there, then run install-service.ps1 again."
+        exit 1
+    }
+}
+
+& $refuseEntries
+
+Write-Host "  Restricted $ConfigDir to SYSTEM and Administrators"
 
 # Copy binaries
 $Binaries = @("fips.exe", "fipsctl.exe", "fipstop.exe")

src/config/mod.rs

@@ -541,9 +541,11 @@ fn warn_unmanaged_key_file(path: &Path) {
 /// only) before any key material is written, so an existing file at a looser
 /// mode is corrected rather than inherited.
 ///
-/// Coverage gap: on Windows the file inherits default ACLs from the parent
+/// Coverage gap: on Windows the file takes the ACL inherited from its
 /// directory, and neither the mode enforcement nor the symlink protection
-/// applies. The exclusion is deliberate.
+/// applies. `install-service.ps1` restricts `C:\ProgramData\fips` to SYSTEM
+/// and Administrators, but a key written anywhere else gets whatever that
+/// directory grants.
 pub fn write_key_file(path: &Path, nsec: &str) -> Result<(), ConfigError> {
     use std::io::Write;
 

src/packaging_tests.rs

@@ -417,3 +417,344 @@ fn dns_cleanup_in_postrm_purge_and_uninstall_removes_every_file_fips_dns_setup_w
         missing.join("\n  ")
     );
 }
+
+/// Returns the lines of a PowerShell script, trimmed, without blank lines and
+/// without lines that are only a `#` comment.
+fn ps_lines(ps1: &str) -> Vec<String> {
+    ps1.lines()
+        .map(str::trim)
+        .filter(|l| !l.is_empty() && !l.starts_with('#'))
+        .map(str::to_string)
+        .collect()
+}
+
+/// Guards the order in which install-service.ps1 secures `C:\ProgramData\fips`.
+///
+/// The directory inherits `C:\ProgramData`'s access, under which any local
+/// user can read the files in it and create missing ones, and a user can
+/// create the directory, or a junction in its place, before the installer
+/// runs, or turn an empty one into a junction. So the installer must build
+/// the restricted ACL and create a new directory with it in one step, refuse
+/// a link and a directory owned by another account, take ownership, check the
+/// directory and the entries inside for links and folders before replacing
+/// the ACL (applying it propagates into them) and again after it, reset each
+/// file, check once more, and only then name any path inside the directory.
+#[test]
+fn windows_installer_restricts_config_dir_before_any_path_inside_it() {
+    let lines = ps_lines(&repo_file("packaging/windows/install-service.ps1"));
+    let nth = |what: &str, n: usize, pred: &dyn Fn(&str) -> bool| -> usize {
+        lines
+            .iter()
+            .enumerate()
+            .filter(|(_, l)| pred(l))
+            .nth(n)
+            .map(|(i, _)| i)
+            .unwrap_or_else(|| {
+                panic!(
+                    "install-service.ps1: no {} line {what}",
+                    ["first", "second", "third"][n]
+                )
+            })
+    };
+    let is_check = |l: &str| l == "& $refuseEntries";
+    let is_create = |l: &str| l.contains("CreateDirectory(") && l.contains("$ConfigDir");
+
+    let order = [
+        (
+            "SetAccessRuleProtection",
+            nth(
+                "containing SetAccessRuleProtection($true, $false)",
+                0,
+                &|l| l.contains("SetAccessRuleProtection($true, $false)"),
+            ),
+        ),
+        (
+            "creation of $ConfigDir",
+            nth("containing CreateDirectory( and $ConfigDir", 0, &is_create),
+        ),
+        (
+            "ReparsePoint check on $ConfigDir",
+            nth("containing ReparsePoint", 0, &|l| {
+                l.contains("ReparsePoint")
+            }),
+        ),
+        (
+            "owner check",
+            nth("containing GetOwner", 0, &|l| l.contains("GetOwner")),
+        ),
+        (
+            "directory /setowner",
+            nth("containing /setowner", 0, &|l| l.contains("/setowner")),
+        ),
+        (
+            "$refuseEntries definition",
+            nth("starting $refuseEntries =", 0, &|l| {
+                l.starts_with("$refuseEntries =")
+            }),
+        ),
+        (
+            "check before the lock",
+            nth("that is & $refuseEntries", 0, &is_check),
+        ),
+        (
+            "Set-Acl of $ConfigDir",
+            nth("containing Set-Acl and $ConfigDir", 0, &|l| {
+                l.contains("Set-Acl") && l.contains("$ConfigDir")
+            }),
+        ),
+        (
+            "check after the lock",
+            nth("that is & $refuseEntries", 1, &is_check),
+        ),
+        (
+            "child /setowner",
+            nth("containing /setowner", 1, &|l| l.contains("/setowner")),
+        ),
+        (
+            "child /reset",
+            nth("containing /reset", 0, &|l| l.contains("/reset")),
+        ),
+        (
+            "check after the reset",
+            nth("that is & $refuseEntries", 2, &is_check),
+        ),
+        (
+            "first path inside $ConfigDir",
+            nth("containing $ConfigDir\\", 0, &|l| {
+                l.contains("$ConfigDir\\")
+            }),
+        ),
+    ];
+    for pair in order.windows(2) {
+        let [(a, ia), (b, ib)] = pair else {
+            unreachable!("windows(2) yields pairs")
+        };
+        assert!(
+            ia < ib,
+            "install-service.ps1: {a} (code line {ia}) must come before {b} (code line {ib})"
+        );
+    }
+
+    let (protect, create) = (order[0].1, order[1].1);
+    for needle in [
+        "S-1-5-18",
+        "S-1-5-32-544",
+        "ContainerInherit",
+        "ObjectInherit",
+    ] {
+        assert!(
+            lines[protect..create].iter().any(|l| l.contains(needle)),
+            "install-service.ps1: the ACL built between SetAccessRuleProtection and \
+             the creation of $ConfigDir does not name {needle}"
+        );
+    }
+    for l in lines.iter().filter(|l| l.contains("CreateDirectory")) {
+        assert!(
+            l.contains("$acl") && l.contains("$ConfigDir"),
+            "install-service.ps1: $ConfigDir must be created with $acl in one step: {l}"
+        );
+    }
+    if let Some(l) = lines
+        .iter()
+        .find(|l| l.contains("New-Item") && l.contains("$ConfigDir"))
+    {
+        panic!("install-service.ps1: New-Item creates $ConfigDir without its ACL: {l}");
+    }
+    let set_acl = &lines[order[7].1];
+    assert!(
+        set_acl.contains("-AclObject $acl"),
+        "install-service.ps1: Set-Acl does not apply the ACL built for creation: {set_acl}"
+    );
+
+    let (def, first_check) = (order[5].1, order[6].1);
+    let block = &lines[def..first_check];
+    assert!(
+        block
+            .iter()
+            .any(|l| l.contains("Get-Item -LiteralPath $ConfigDir") && l.contains("ReparsePoint")),
+        "install-service.ps1: the $refuseEntries script block does not test whether \
+         $ConfigDir itself has become a link"
+    );
+    assert!(
+        block.iter().any(|l| l.contains("Get-ChildItem -LiteralPath $ConfigDir")
+            && !l.contains("-Recurse")),
+        "install-service.ps1: the $refuseEntries script block does not list $ConfigDir's entries"
+    );
+    for needle in ["ReparsePoint", "PSIsContainer"] {
+        assert!(
+            block
+                .iter()
+                .any(|l| l.contains("$item") && l.contains(needle)),
+            "install-service.ps1: the $refuseEntries script block does not test {needle} \
+             on each entry"
+        );
+    }
+
+    let checks = lines.iter().filter(|l| is_check(l)).count();
+    assert_eq!(
+        checks, 3,
+        "install-service.ps1: expected exactly three & $refuseEntries lines, found {checks}"
+    );
+    let setowners = lines.iter().filter(|l| l.contains("/setowner")).count();
+    assert_eq!(
+        setowners, 2,
+        "install-service.ps1: expected exactly two /setowner lines, found {setowners}"
+    );
+    if let Some(l) = lines.iter().find(|l| l.contains("-Recurse")) {
+        panic!("install-service.ps1: -Recurse is not allowed: {l}");
+    }
+}
+
+/// Guards the conditions under which install-service.ps1 refuses its config
+/// directory, not only their position.
+///
+/// Each refusal must be an `if` whose body is `Write-Error` then `exit 1`: an
+/// owner outside SYSTEM, Administrators and the installing account; the
+/// directory being a link, both at the start and inside every recheck; and an
+/// entry that is a link or a folder, either of which is enough. A condition
+/// that can never hold, or that needs both a link and a folder, would pass an
+/// ordering check while refusing nothing.
+#[test]
+fn windows_installer_refusal_conditions_stop_the_install() {
+    let lines = ps_lines(&repo_file("packaging/windows/install-service.ps1"));
+    let refuses_at = |i: usize, what: &str| {
+        let cond = &lines[i];
+        assert!(
+            cond.starts_with("if (") && cond.ends_with('{'),
+            "install-service.ps1: the {what} is not an if statement: {cond}"
+        );
+        let body = lines.get(i + 1..i + 3).unwrap_or_default();
+        assert!(
+            body.len() == 2 && body[0].starts_with("Write-Error ") && body[1] == "exit 1",
+            "install-service.ps1: the {what} is not followed by Write-Error then exit 1: \
+             {cond}\n  then: {body:?}"
+        );
+    };
+    let find = |what: &str, pred: &dyn Fn(&str) -> bool| -> Vec<usize> {
+        let found: Vec<usize> = lines
+            .iter()
+            .enumerate()
+            .filter(|(_, l)| pred(l))
+            .map(|(i, _)| i)
+            .collect();
+        assert!(
+            !found.is_empty(),
+            "install-service.ps1: no line found for the {what}"
+        );
+        found
+    };
+
+    let trusted = find("list of trusted owners", &|l| {
+        l.starts_with("$trustedOwners = @(")
+    });
+    for needle in [
+        "\"S-1-5-18\"",
+        "\"S-1-5-32-544\"",
+        "WindowsIdentity]::GetCurrent().User.Value",
+    ] {
+        assert!(
+            lines[trusted[0]].contains(needle),
+            "install-service.ps1: the trusted owners do not include {needle}: {}",
+            lines[trusted[0]]
+        );
+    }
+    for i in find("owner refusal", &|l| {
+        l == "if ($trustedOwners -notcontains $ownerSid) {"
+    }) {
+        refuses_at(i, "owner refusal");
+    }
+
+    let dir_links = find("refusal of $ConfigDir as a link", &|l| {
+        l.starts_with("if (") && l.contains("ReparsePoint") && !l.contains("$item")
+    });
+    assert_eq!(
+        dir_links.len(),
+        2,
+        "install-service.ps1: expected the directory's link check once at the start and \
+         once in $refuseEntries, found {}",
+        dir_links.len()
+    );
+    for i in dir_links {
+        refuses_at(i, "refusal of $ConfigDir as a link");
+    }
+
+    for i in find("refusal of a link or folder entry", &|l| {
+        l.starts_with("if (") && l.contains("$item")
+    }) {
+        let cond = &lines[i];
+        assert!(
+            cond.contains("ReparsePoint")
+                && cond.contains("PSIsContainer")
+                && cond.contains(" -or ")
+                && !cond.contains(" -and "),
+            "install-service.ps1: an entry must be refused if it is a link or a folder, \
+             either one: {cond}"
+        );
+        refuses_at(i, "refusal of a link or folder entry");
+    }
+}
+
+/// Guards the recovery install-service.ps1 gives for a directory it refuses.
+///
+/// A user who created `C:\ProgramData\fips` holds full control of it through
+/// an inherited entry for their own account. Taking ownership changes only the
+/// owner, so the installer's owner check would then pass while that user could
+/// still swap the directory for a junction. The recovery must be to delete the
+/// directory, and the installer must not offer `takeown` as a way through.
+#[test]
+fn windows_installer_refusals_never_offer_takeown_as_the_recovery() {
+    let lines = ps_lines(&repo_file("packaging/windows/install-service.ps1"));
+    if let Some(l) = lines
+        .iter()
+        .find(|l| l.to_ascii_lowercase().contains("takeown"))
+    {
+        panic!("install-service.ps1: a refusal offers takeown as the recovery: {l}");
+    }
+    let owner_refusals = lines
+        .iter()
+        .filter(|l| l.contains("Write-Error") && l.contains("delete the directory"))
+        .count();
+    assert!(
+        owner_refusals >= 2,
+        "install-service.ps1: expected the owner refusals to say to delete the directory, \
+         found {owner_refusals} such lines"
+    );
+}
+
+/// Guards every icacls call in install-service.ps1: each acts on a link itself
+/// rather than its target (`/L`), never walks a tree (`/T`), and has its exit
+/// code checked on the next line, since `$ErrorActionPreference = "Stop"` does
+/// not cover a native command's exit code in Windows PowerShell 5.1.
+#[test]
+fn windows_installer_icacls_calls_act_on_links_and_check_exit_codes() {
+    let lines = ps_lines(&repo_file("packaging/windows/install-service.ps1"));
+    let calls: Vec<usize> = lines
+        .iter()
+        .enumerate()
+        .filter(|(_, l)| l.contains("& $icacls"))
+        .map(|(i, _)| i)
+        .collect();
+    assert!(
+        calls.len() >= 3,
+        "install-service.ps1: expected at least three & $icacls calls, found {}",
+        calls.len()
+    );
+    for i in calls {
+        let call = &lines[i];
+        let tokens: Vec<&str> = call.split_whitespace().collect();
+        assert!(
+            tokens.iter().any(|t| t.eq_ignore_ascii_case("/L")),
+            "install-service.ps1: icacls call without /L follows a link: {call}"
+        );
+        assert!(
+            !tokens.iter().any(|t| t.eq_ignore_ascii_case("/T")),
+            "install-service.ps1: icacls call with /T walks the tree: {call}"
+        );
+        let next = lines.get(i + 1).map(String::as_str).unwrap_or_default();
+        assert!(
+            next.contains("$LASTEXITCODE"),
+            "install-service.ps1: icacls call not followed by a $LASTEXITCODE check: \
+             {call}\n  next line: {next}"
+        );
+    }
+}