Fix Windows VM helper rejecting setgid source directories

[ekollof]

Fixes #9698
Fixes #9374

GNU chmod 0700 leaves setuid/setgid on directories when the numeric mode is four digits or fewer. prepare_caller_mounts then requires mode 700 exactly, so a leftover g+s on ~/Windows (often 2700 or 2777) makes every privileged VM action fail closed with no diagnostic.

This hardens those directories with chmod a-s,u=rwx,go= (which does clear the special bits) and prints the observed modes if the check still fails.

dockur samba.sh also chmod 2777s an empty /shared bind at container start, so a successful launch immediately undoes the host privacy mode. A hidden .omarchy-keep sentinel keeps the share non-empty, and the helper re-hardens ~/Windows after docker compose up.

Covered by a user-side prepare_user_mount_sources test and a root-namespace case that starts from 2700/2777 sources.

[omarchybot]

Reviewed by Claude Opus 5, with an independent second opinion from Codex at xhigh. Both reached the same verdict, and Codex contributed two mechanisms the first pass had not worked out; its independence is not currently guaranteed, so the agreement is worth less than those two findings.

What was checked. The diff against quattro at 8915169; every place in bin/omarchy-windows-vm that reads or asserts a mode (prepare_caller_mounts 609/616, mounted_leaf_matches 518, mounts_ready 650, assert_mounts_safe 919, and the unprivileged fast path at 89-100); the chmod semantics themselves, measured rather than assumed; and dockur/windows src/samba.sh addShare(), because the sentinel rests on a claim about it. Tests ran on a disposable VM, not locally: windows-vm-test.sh (4 passed), windows-vm-compose-test.sh (22), windows-vm-mount-boundary-test.sh (10), windows-key-test.sh (7) and ./test/cli, all green. Reverting chmod_private_dir to chmod 0700 on that VM makes both of your new cases fail — storage mode is 2700, expected 700, and root could not harden setgid VM source directories — so they are regression tests that actually regress.

The mode fix is correct, and it is the right kind of repair. On coreutils 9.11, chmod a-s,u=rwx,go= yields exactly 700 from 2700, 2777, 6700, 1700, 1777 and 3777; plain chmod 0700 yields 2700, 2700, 6700, 700, 700 and 2700 from the same starts. Nothing in the diff loosens a check: the exact-700 comparisons at 616 and 518 are untouched, and 617 adds a diagnostic rather than a tolerance. Extending the hardener to write_credentials (1099) is right as well.

The sentinel is where the defects are. Its premise holds — dockur's addShare() decides empty="Y" with [ -z "$(ls -A "$dir")" ], and ls -A counts a dotfile, so a .omarchy-keep does suppress the chmod 2777. The implementation is the problem.

1. ensure_shared_sentinel gives the caller a root-write primitive (high). At line 574 it does [[ -e $sentinel ]] || : >"$sentinel", and at line 622 that runs as root inside a directory the unprivileged caller owns. : > opens with O_CREAT|O_TRUNC and no O_NOFOLLOW, so a dangling symlink at ~/Windows/.omarchy-keep is followed. Demonstrated in the mount-boundary test's own namespace on the worker: with .omarchy-keep -> /var/secret/created-by-root planted by uid 1000, prepare_caller_mounts created that file root:root 0644 outside the caller's home. An existing target is not clobbered, because -e short-circuits first — but -e and the redirect are two syscalls with an attacker-controlled path between them, so swapping the symlink in that window turns it into a truncate of any root-owned file. Pinning the parent by FD does not make the child lookup safe. This is the only place in the file that reaches outside the boundary the rest of the function is built to enforce, and it is worth weighing against everything else the function does to avoid exactly this.

2. On an existing install the sentinel is created by root, in the user's home (low). launch does not call prepare_user_mount_sources: migrate_legacy_compose returns at 1135 as soon as $COMPOSE_FILE exists, so the first launch after this lands goes straight to priv up_wait and the root half creates the file. Confirmed on the worker: root:root 0644. The user can delete it, and the next privileged run puts it back root-owned. remove also leaves it behind, since rm -rf at 1418 covers ~/.windows but ~/Windows is deliberately kept.

3. A sentinel that cannot be written now fails every privileged action (low). Line 574 returns 1 and line 622 aborts prepare_caller_mounts, which up (1368), up_wait (1449), remove (1410) and write_compose (1078) all reach through assert_mounts_safe. A full or read-only ~/Windows previously did not stop removal; now it does. Losing a defensive sentinel probably should not be fatal, least of all on the cleanup path.

4. The sudoless-Docker path never creates a sentinel, and restore_shared_privacy races (low). Codex traced this one: on a box with sudoless Docker and an already-empty share with valid 700 anchors, the fast path at 95 finds mounts_ready true and runs __priv_up as the caller; assert_mounts_safe skips prepare_caller_mounts at 919 because EUID != 0, so ensure_shared_sentinel never runs before Docker starts. The same holds after the user or the guest deletes the file — the share is exported to the guest at 754-756, so it is deletable from inside Windows. That leaves only restore_shared_privacy, and dc up -d returns when the container is started, not when its entrypoint has reached samba.sh, so the restore can land before dockur's chmod 2777 rather than after it. The comment at 565-567 reads as though the ordering were guaranteed.

5. restore_shared_privacy supplies __priv_up's exit status (low). It is the last command at 931, so a failed chmod makes launch print ❌ Failed to start Windows VM! at 1450 over a container that started fine and is left running.

Neither of the two behaviours that distinguish this PR — the sentinel under the privileged path, and restore_shared_privacy — is covered by a test. The boundary suite's shared directory is already non-empty from shared.txt at line 99, and the unit test only asserts the sentinel exists; both would still pass if restore_shared_privacy were deleted.

Nothing was pushed to your branch. Finding 1 needs a design decision rather than a patch — drop the privileged creation, or open with O_NOFOLLOW/O_EXCL — and there are five open pull requests fixing this same root cause, so editing one of them ahead of that choice would not help. #9322, #9414 and #9504 change the same two chmod sites and stop there; #9605 also covers write_credentials and adds the diagnostic; this one is the only one that addresses the container re-adding the bit after start, which is what #9746 reports. On the shared question of which moments are covered: all five clear the bits at both the user preflight and the privileged mount preparation, and all five therefore fix #9334, #9374, #9540, #9567 and #9698, and also fix the #9746 launch failure, because the next privileged run re-clears before the exact-700 check. What the other four leave standing is the exposure rather than the failure — ~/Windows sits at 2777, world-writable, from container start until the next privileged action, which is the thing the 0700 hardening exists to prevent, and a sudoless-Docker user drops out of the fast path into a polkit prompt on every launch after the container has run.

Waiting on the maintainer, both for that choice and for a decision on the sentinel.

[ekollof]

Addressing the review by dropping privileged sentinel creation.

ensure_shared_sentinel ran as root in a caller-owned directory with a create/truncate and no O_NOFOLLOW, which is a write primitive the rest of prepare_caller_mounts exists to prevent. A missing sentinel also should not fail remove.

This revision:

• keeps chmod_private_dir and the exact-700 checks
• removes all privileged (and user-side) sentinel creation
• restores share mode after windows started successfully, so it lands after dockur samba.sh rather than racing dc up -d
• treats that restore as best-effort so a chmod failure cannot report a successful start as Failed to start Windows VM

The world-writable window is now only during guest boot, not until the next privileged action.

[omarchybot]

Re-reviewed at e3d31903 by Claude Opus 5, with a second opinion from Codex at xhigh pinned to that same SHA. Dropping the sentinel closed three of the four things the last round raised, and it was the right call. One defect came back in the replacement, in a weaker form; I pushed the fix.

Where the previous findings stand. Finding 1 (root create/truncate through ~/Windows/.omarchy-keep) is fixed — ensure_shared_sentinel is gone, and nothing privileged writes into the caller-owned share any more. Findings 2 and 3 (the sentinel landing root-owned in $HOME, and an unwritable sentinel failing remove) are moot for the same reason. Finding 5 is fixed: restore_shared_privacy no longer supplies an exit status, so a failed chmod cannot report a running container as Failed to start Windows VM. Finding 4's race is fixed — Codex traced dockur's entry.sh, which sources samba.sh synchronously before QEMU starts, so the windows started successfully line really does come after the chmod 2777 and the comment at 937-938 is accurate. That is not a permanent guarantee, since 721 pins the moving dockurr/windows tag rather than a digest.

New, and pushed as bacb9085: restore_shared_privacy handed root an arbitrary-path chmod. The loop at 571-573 ran over "$EXPECTED_SHARED" and "$LEGACY_SHARED", and $LEGACY_SHARED is $CALLER_HOME/Windows (225) — a pathname the unprivileged caller owns. [[ -d $dir && ! -L $dir ]] and chmod are two syscalls, and GNU chmod follows a command-line symlink, so the caller can swap the directory for a link in the window and steer the chmod anywhere. It runs as root at 939, so this is the same defect class as the sentinel with a different payload. Demonstrated on a disposable VM against a copy of the exact loop body: a swapper running as uid 1000, restricted to its own home, took a root-owned 4755 binary outside that home to root:700 — race won on iteration 260, well under a second, and freely retriable. Impact is a host denial of service rather than an escalation (ownership is unchanged and go= grants nobody anything), but chmod 0700 / or a desetuid'd sudo is not a small one.

The other loop element already covers the case, so the fix is to delete the legacy one. $EXPECTED_SHARED sits inside the root-owned 0711 boundary tree that prepare_runtime_tree builds and assert_boundary_dir re-checks (300-303), which the caller cannot write, and assert_mounts_safe has just proved through mounts_ready that it is a bind of the same inode as $LEGACY_SHARED. Chmodding the anchor is therefore what ~/Windows ends up at — measured on the worker rather than assumed: 2777 on the source, chmod on the bind anchor alone, 700 on both. The commit also adds the regression test, which the existing one could not carry because it passes the same safe path as both variables; reverting the loop makes the new case fail.

Still open, for you rather than a patch from me: __priv_up does not restore, only __priv_up_wait does. install_windows starts the VM through priv up (1354), and on a fresh install the share is empty, so dockur sets ~/Windows to 2777 and nothing brings it back. __priv_down (919) does not either. It stays world-writable through the 10-15 minute Windows install the script tells the user to sit and watch, and then until the first launch — indefinitely if they never launch. The obvious fix is the wrong one: dc up -d returns when the container is started, not when its entrypoint has reached samba.sh, so hardening there would race exactly the way this revision stopped racing. A readiness wait on the install path, or a re-harden at the top of stop, would both work; which one is yours to pick. On the same note, if up_wait times out at 943-945 it returns without restoring, so a guest that never logs the ready line leaves the share at 2777 too.

What was checked. The delta only — everything under 8915169 was reviewed last round. git diff against the merge-base f99d33a8; every mode assertion the change touches (prepare_caller_mounts 608-615, mounted_leaf_matches 518, mounts_ready 639-665, the unprivileged fast path at 89-100); chmod_private_dir's semantics, which Codex confirms clear setuid, setgid and sticky for both directories and regular files on GNU coreutils; and the set -- help trick in the new unit test, which is not vacuous — the dispatcher runs show_usage and the production function definitions stay available to call directly. Tests ran on a disposable VM, never locally: windows-vm-test.sh, windows-vm-compose-test.sh (22), windows-vm-mount-boundary-test.sh (10), windows-key-test.sh (7) and ./test/cli, all green both before and after the pushed commit. Your new root-namespace boundary case genuinely regresses when chmod_private_dir is reverted to chmod 0700, and leaves the binds intact for the eight tests that follow it.

Codex reached the arbitrary-chmod finding independently and rated it High; its independence is not currently guaranteed, so treat that as agreement rather than confirmation. What it contributed on its own terms is the dockur entry.sh ordering above, the moving-tag caveat, and the __priv_up gap including that stop does not close it either.

Related. This closes #9374 and #9698, which the body names, and also #9334, #9540, #9567, #9884 and #9943, which are the same setgid symptom — worth adding to the body so they close together. #9746 and #9943 are the container-re-adds-the-bit variant, and this is the only one of the five open PRs on this root cause that addresses it: #9322, #9414 and #9504 change the two chmod sites and stop; #9605 adds write_credentials and the diagnostic; this one also shrinks the window in which ~/Windows sits world-writable after a launch. Which of the five lands is the maintainer's call, and nothing here is a request to close any of them.

Waiting on you for the __priv_up gap, and on the maintainer for the choice between the five.

[ekollof]

Picked both of the remaining options.

install goes through priv up, so it never hit the guest-ready restore and left ~/Windows at 2777 for the whole first-boot install (and forever if you never launch). Hardening immediately after dc up -d would race samba.sh again.

This revision:

• waits on up until the share is 2777 (or the shared-folder/samba log line, or a short timeout), then restores through the protected anchor
• restores on down after the container is gone, so samba cannot chmod it back
• restores when up_wait times out without the ready line

Still never chmod $LEGACY_SHARED by pathname, and still never fail a successful start/stop on that restore.