Skip to content

Fix Windows VM helper rejecting setgid source directories - #9783

Open
ekollof wants to merge 12 commits into
omacom:quattrofrom
ekollof:fix/windows-vm-setgid-chmod
Open

ekollof wants to merge 12 commits into
omacom:quattrofrom
ekollof:fix/windows-vm-setgid-chmod

Conversation

@ekollof

@ekollof ekollof commented Sep 2, 2026

Copy link
Copy Markdown

Fixes #9698
Fixes #9374
Fixes #9334
Fixes #9540
Fixes #9567
Fixes #9884
Fixes #9943
Fixes #9746

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 after the ISO download. The helper re-hardens the protected bind anchor (never $HOME/Windows by pathname) after the guest reports ready, on up_wait timeout, and on stop. Fresh install does not hold a polkit session for the download: it backgrounds an owner-side wait for 2777 and then restores.

@omarchybot

Copy link
Copy Markdown
Collaborator

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

ekollof commented Sep 2, 2026

Copy link
Copy Markdown
Author

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.

@ekollof
ekollof force-pushed the fix/windows-vm-setgid-chmod branch from c194fcf to e3d3190 Compare September 3, 2026 10:03
@omarchybot

Copy link
Copy Markdown
Collaborator

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 fixedensure_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

ekollof commented Sep 3, 2026

Copy link
Copy Markdown
Author

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.

@omarchybot

Copy link
Copy Markdown
Collaborator

Re-reviewed at b50d539b, by Claude Opus 5 and independently by Codex (gpt-5.6-sol) at xhigh reasoning. Tests and probes ran on a disposable VM, never on the machine that holds credentials. Nothing pushed this round.

Everything from the last review is closed. The $LEGACY_SHARED arbitrary-chmod hole stayed fixed across your push (bacb9085 is intact at this head), and __priv_down (:953), the __priv_up_wait timeout (:983) and __priv_up (:946) now all re-harden, so ~/Windows no longer stays world-writable indefinitely — a stop or the next launch brings it back. That is a real improvement over where this branch was.

Worth being precise about what the re-hardening is buying, because it is not the launch failure. The silent-launch bug is fixed by chmod_private_dir on its own: when a sudoless user's mounts_ready gate fails on a 2777 share, priv() falls through to pkexec rather than refusing, and root's __priv_up_wait runs assert_mounts_safeprepare_caller_mounts, which chmods the pinned fds before the exact-mode check. So the container re-applying 2777 after the preflight does not cause a recurring rejection; the next launch repairs it. What restore_shared_privacy uniquely adds is privacy while the VM runs — ~/Windows at 2777 (world-writable, setgid) instead of 700 for the life of the container. That is a genuine gap none of the other five PRs closes, and it is the reason this one is worth more than its size suggests.

Which makes the new function's failure matter, because it is exactly that gap on the install path. wait_then_restore_shared_privacy (:580) waits at most 60 × 0.25s. __priv_up is reached only from install_windows (:1391) — launch goes through up_wait (:1472) — and on a fresh install dockur has not got anywhere near samba.sh by the fifteen-second mark. Its entry.sh sources install.sh at line 24 and samba.sh at line 29; install.sh ends in a top-level startWindows that synchronously calls downloadImage and then unpacks and rebuilds the Windows ISO. That is the ten to fifteen minutes your own script tells the user to expect. The loop expires long before, restores a directory that is still 700, and returns; dockur then reaches addShare and chmods the still-empty share to 2777, with nothing left on the host to undo it until the user next stops or launches. Measured on the worker with a stubbed docker, against the real install shape (share at 700, container log containing no match): 15367 ms elapsed, share left to dockur.

The docker logs escape hatch at :594 does not rescue that, because nothing dockur prints in a normal run matches shared folder|samba. html "Initializing shared folder..." runs before addShare, but upstream html() writes $PAGE and $INFO — the web status page on 8006 — not stdout. The readme echos are redirected into readme.txt and the config echos into smb.conf. The only stdout line that would match is echo "Starting Samba daemon...", gated on DEBUG, which the generated compose does not set. The remaining matches are all dockur error paths, and one of them (Failed to reserve Samba ports!) is emitted from install.sh, i.e. before samba.sh — so in the rare case the grep fires, it fires early. That is benign only because that particular error is fatal to the container.

The rest is smaller:

  • The new test at test/shell.d/windows-vm-test.sh:55 passes, but it does not cover the loop. docker inspect on a nonexistent container fails, started_at is empty, and the function short-circuits at :583 — confirmed on the worker, elapsed 3 ms. The polling, the mode check and the log grep would all still pass this test if they were deleted.
  • __priv_down calling resolve_caller (:953) changes behaviour under a direct sudo omarchy-windows-vm stop: root can write the docker socket, so priv() takes the unelevated branch, PKEXEC_UID is unset, and resolve_caller prints cannot identify the user who authorized this action to stderr and skips the restore — while stop still returns dc down's status and prints "Windows VM stopped." Under pkexec, the normal path, this does not arise.
  • Same line, on a multi-user machine: EXPECTED_SHARED is per-uid but the container and compose are global, so a stop by a second user restores their own (nonexistent) anchor rather than the share the VM was actually using.
  • The PR body still describes the .omarchy-keep sentinel, which e3d31903 removed. Worth correcting: there are six open PRs on this file and the body is what a maintainer compares.

To be explicit about the design question rather than pushing a guess at it — closing the install gap means either waiting for readiness on the install path, which blocks the elevated session for the whole download, or re-hardening from something that outlives the command. Both are your call, so nothing was pushed.

The TOCTOU reasoning in the comment above restore_shared_privacy was re-verified and holds. $EXPECTED_SHARED resolves under /var/lib/omarchy/windows/mounts/users/$uid/; root refuses any other RUNTIME_DIR (:275), assert_boundary_dir requires each component canonical, root-owned and neither group- nor other-writable, and the 0711 root-owned parent means an unprivileged caller cannot rename or replace the leaf between the ! -L test and the chmod. OMARCHY_WINDOWS_DIR does not survive pkexec's environment sanitisation. Codex reached the same conclusion and found no caller-swappable root path either.

Codex agreed with the install-path finding; its independence is not currently guaranteed, and I had reached and measured it first, so treat that as agreement rather than confirmation. What it contributed on its own terms is the sudo/multi-user resolve_caller behaviour in __priv_down, which I verified against the source before repeating it here.

Checked on a disposable VM: ./test/cli and test/shell.d/windows-vm-test.sh (4), windows-vm-mount-boundary-test.sh (10), windows-vm-compose-test.sh (22), windows-key-test.sh (7) — all green at this head. Also re-derived the mode arithmetic there on coreutils 9.11: from 2755, 6755, 7777, 4700 and 2777, chmod 0700 leaves the set-ID bits and u=rwx,go= alone leaves them too, so the a-s clause in chmod_private_dir is load-bearing. Waiting on you for the install-path gap and the stale body, and on the maintainer for which of #9564, #9605, #9989, #10046, #10113 and this one lands.

@ekollof

ekollof commented Sep 4, 2026

Copy link
Copy Markdown
Author

The 15s privileged wait on up was the wrong tool: samba.sh runs only after dockur's ISO download, html "Initializing shared folder..." does not go to stdout, and the unit test short-circuited before the loop.

This revision:

  • leaves priv up as start-only, so install does not hold pkexec for 10–15 minutes
  • backgrounds an owner-side watcher after install that chmods ~/Windows once it becomes 2777 (user chmod of their own tree, not a root pathname)
  • restores every RUNTIME_DIR/mounts/users/*/shared on down, so sudo ... stop without PKEXEC_UID and a second-user stop still hit the real share
  • keeps the post-ready restore on up_wait (and on its timeout)

PR body updated: sentinel text is gone, and the extra setgid issues are listed.

@omarchybot

Copy link
Copy Markdown
Collaborator

Re-reviewed at c2ca99a9 by Claude Opus 5, with a second opinion from Codex (gpt-5.6-sol) at xhigh reasoning pinned to that same SHA. Everything measured on a disposable VM, never on the machine holding credentials. One fix pushed as e75738ec.

Where the last round's findings stand. The install-path gap is closed in design — priv up no longer holds a polkit session, and the 15-second wait that could never reach samba.sh is gone. __priv_down no longer calls resolve_caller, so the sudo ... stop and second-user cases are answered. The PR body is current. Two things came back inside the replacement.

Pushed as e75738ec: the install watcher died with its terminal, so it never restored anything. disown (:608) only stops bash from hupping a job when the shell itself exits. Job control is off in a script, so the watcher stays in the install terminal's foreground process group, and the kernel hangs that group up when the pty closes — disown does not change that. install is launched by omarchy-launch-floating-terminal-with-presentation, which ends in omarchy-show-done's "Press any key to close" prompt, so the terminal goes away the moment the user dismisses it, minutes before dockur reaches chmod 2777. And because __priv_up (:951) no longer restores, nothing else covers that window. Measured on the worker with script(1) reproducing the shape — a pty whose controlling process exits: with the code as written the share is still 2777 four seconds after the terminal closes; with trap '' HUP inside the subshell the watcher survives and restores 700. An asynchronous command already ignores SIGINT and SIGQUIT when job control is off (SigIgn: ...6 on the live process), so HUP was the only gap. The commit adds the one-line trap and a regression test that reproduces the terminal shape; removing the trap makes it fail with left the share at 2777.

Still open, and yours rather than a patch from me: restore_all_shared_privacy does nothing at all for a sudoless-Docker user. priv() routes down to an unelevated with_vm_lock __priv_down for anyone who can reach the Docker socket (:89-100, and $VM_LOCK_DIR is 0750 root:docker). $RUNTIME_DIR/mounts/users is root-owned 0711 (:302) — traversable but not listable — so the glob at :582 cannot expand, $dir stays the literal .../*/shared, [[ -d $dir ]] is false, and the function returns success having restored nothing, including the caller's own anchor. Measured with the real function and production ownership and modes: as root both 1000 and 1001 go 2777700; as uid 1000 both stay 2777. With the same tree at 0755 the glob expands and uid 1000 restores its own (and silently fails on the other user's, which is right). That is a regression against b50d539b, where naming the anchor directly worked unprivileged. The new test at test/shell.d/windows-vm-test.sh:60-69 cannot catch it because mkdir -p gives the fixture 0755. I have not pushed a fix because you deliberately moved this function off resolve_caller and there is more than one reasonable answer — a non-root branch that names $(id -u)'s own anchor, or routing down through pkexec unconditionally — and which one you want is a design choice rather than a defect with one repair.

Two smaller things, both in the watcher and neither pushed:

  • The 1800-second budget (:599) is a hard stop with no message anywhere; the process writes to /dev/null. Your own install text says the download "may take 10-15 minutes", so 30 minutes is about 2x margin — thin on a slow link, and when it expires the share stays 2777 until the next stop or launch.
  • Codex found this one: if the watcher sees 2777 but its chmod fails, :602 swallows the error and :603 exits the watcher permanently instead of trying again. A ~/Windows that was swapped for a symlink to something the caller does not own is enough to reach it.

On the rest of the diff, which I re-checked in full. chmod_private_dir is right and the a-s clause is load-bearing, measured again on coreutils 9.11: from 2700, 2777, 6700, 1700, 1777, 3777 and 7777 it gives exactly 700 every time, while chmod 0700 leaves 2700/6700 and — this is the part worth knowing — so does u=rwx,go= without the a-s. A setgid parent does not put the bit back after a chmod; Linux applies that inheritance at mkdir only, confirmed on the worker. Related: install -d -m 0700 under a setgid parent still produces 2700, so line 1074-1075 depends on the chmod_private_dir two lines below it — which it now has. The ! -L / chmod TOCTOU in restore_shared_privacy (:572-573) holds: $EXPECTED_SHARED's parents are root-owned and neither group- nor other-writable, so an unprivileged caller cannot replace that directory entry between the two syscalls, and OMARCHY_WINDOWS_DIR does not survive pkexec's environment or sudo's env_reset (measured: zero occurrences in sudo env). restore_all_shared_privacy's symlink guard is sound — a users/1000 -> /elsewhere component makes realpath differ from $dir and the entry is skipped, verified on the worker — and an empty glob skips rather than chmodding anything. One gap worth naming: that function reads $RUNTIME_DIR without the [[ $RUNTIME_DIR == /var/lib/omarchy/windows ]] refusal prepare_runtime_tree applies at :276, and does not check that prefix is itself unsymlinked. Not reachable today for the environment reasons above, so defence in depth rather than a finding.

Codex. It reached the same two conclusions about the terminal hangup and the sudoless glob, with the kernel tty-hangup and fork(2) documentation to back them; I had reached and measured both first and its independence is not currently guaranteed, so read that as agreement rather than confirmation. What it contributed on its own terms is the permanent exit 0 on a failed chmod above, which I verified against the source before repeating it.

Cross-references, since several reports of this same defect are open. #10315 is fixed by this PR on both counts it asks for — the symbolic clear, and the missing echo ... >&2 on the post-chmod mode check, which a5d52413 added at :645. #10256 is the same defect and the same fix. #10035 is worth flagging because its diagnosis is wrong: it blames $BASHPID inside $(stat ...) reading "a different descriptor table", but a command-substitution subshell is a fork, so its descriptor table is a copy and /proc/<subshell-pid>/fd/N names the same open file. Measured on the worker — the same %d:%i via the parent's pid and via the subshell's own BASHPID — and the reporter's own trace shows stat returning a mode rather than an error, which is only possible if the descriptor resolved. What their trace actually shows is chmod 0700 failing to clear setgid, which is what this PR fixes. All three could be added to the Fixes list. #10262 rewrites those same lines to a snapshotted self_pid; that is a behaviour no-op, since BASHPID is constant within a process and every use site is in the process that opened the descriptor, so it does not fix #10035 — and it collides with this PR on the exact chmod 0700 -- "/proc/$BASHPID/fd/..." line, which this one replaces with chmod_private_dir. Its static test also forbids $BASHPID inside $(...), which :642 still does. Whichever lands second needs care; landed carelessly after this one, #10262's hunk would put chmod 0700 back and reopen the bug.

Checked on a disposable VM: ./test/cli (116 ok), and through the session wrapper windows-vm-test.sh (5 passed, including the new one), windows-vm-mount-boundary-test.sh (10), windows-vm-compose-test.sh (22), windows-key-test.sh (7) — all green at e75738ec. Waiting on you for the sudoless-Docker restore, and on the maintainer for which of the eight open pull requests on this root cause lands.

@ekollof

ekollof commented Sep 5, 2026

Copy link
Copy Markdown
Author

Addressing the review at e75738ec:

Sudoless-Docker restore (the main open finding): took the non-root-branch option. __priv_down now restores by EUID — root walks the mounts tree (restore_all_shared_privacy, now root-only), and a sudoless caller goes through resolve_caller + restore_shared_privacy for its own anchor. That works unprivileged because the anchor sits under the root-owned 0711 tree the caller cannot rename, and while the bind exists it is the caller's own inode. The restore stays best-effort: a failed resolve_caller cannot fail a dc down that succeeded. A second user's unprivileged stop still restores only their own anchor — the most an unelevated caller can do without pkexec — and the next owner action covers the rest.

Watcher: exits only after verifying the mode is really 700, so a failed chmod no longer ends the watcher permanently; budget raised to 3600s, and on expiry it applies a final best-effort restore and logs via logger -t omarchy-windows-vm (the subshell's output goes to /dev/null, so the journal entry is the visible trace).

Defence in depth: restore_all_shared_privacy now applies the same RUNTIME_DIR == /var/lib/omarchy/windows refusal as prepare_runtime_tree.

Tests: the unit test now pins that an unprivileged restore_all_shared_privacy does nothing and that the owner-side restore fixes only the caller's own anchor; the boundary suite gains a root walk (both anchors to 700), a setpriv --reuid=1000 case for the sudoless shape, and a planted non-standard-runtime tree proving the refusal — reverting the script makes the new cases fail. All suites green at this head: windows-vm-test (5), mount-boundary (11), compose (22), windows-key (7), ./test/cli.

@omarchybot

Copy link
Copy Markdown
Collaborator

Re-reviewed the delta e75738ec..ab2d8e9b by Claude Opus 5, with a second opinion from Codex (gpt-5.6-sol) at xhigh reasoning pinned to ab2d8e9b. Everything measured on a disposable VM, never on the machine holding credentials. One commit pushed as fb1e500f.

The open finding is fixed, and I did not take the description's word for it. Built the production shape on a worker — mounts, users, users/1000 all root-owned 0711, the caller's anchor owned by uid 1000 — confirmed uid 1000 cannot list users/ and that the glob stays the literal .../*/shared, then drove __priv_down unelevated with dc stubbed. At this head the caller's anchor goes 2777700 and uid 9999's stays 2777, which is right: an unelevated caller has no business chmodding another account's inode. Reverting the same run to the e75738ec shape on the same fixture leaves it at 2777. The 0711 chain is traversable, so [[ -d $EXPECTED_SHARED ]] and the chmod both work on a complete pathname even though enumeration is denied — that is the part the design turns on and it holds. launch without --keep-alive gets the same fix through its own priv down.

Pushed as fb1e500f: the new cases did not pin the fix. They call restore_all_shared_privacy and restore_shared_privacy directly, never the dispatch. Reverting only __priv_down's EUID split — the branch that is the fix — leaves the boundary suite at 11 passed, 0 failed; measured. The commit routes both the setpriv child and the root case through __priv_down with dc stubbed, and the same revert now fails with rc=15, the caller's anchor still 2777. Codex found this gap; I confirmed it by mutation before acting on it. Nothing else in your tests changed.

The watcher's other two fixes check out. A ~/Windows pointing at a directory the caller does not own makes every chmod fail: the watcher stayed alive through four seconds of failures, then restored 700 within a second of the ownership being fixed and exited. Under e75738ec the first failure ended it permanently. logger is present, and the helper sets no -e, so the [[ ... ]] && exit 0 cannot take the subshell down on a false comparison.

Three small things left to you, none blocking.

  • bin/omarchy-windows-vm:986 — the comment says a fresh-reboot stop is covered because "the next launch re-hardens through prepare_user_mount_sources". It does not: launch_windows goes priv up_waitassert_mounts_safeprepare_caller_mounts (:935, :964). prepare_user_mount_sources is reached only from install and migrate_legacy_compose, never from a plain launch. The behaviour is right, the name is wrong.
  • :623 — the expiry chmod is unconditional, where the loop body only fires on 777/2777. If ~/Windows was renamed or replaced during the hour, that hardens whatever the pathname now resolves to. Same uid throughout, so it is a footgun rather than a boundary problem, but it is new: before this commit the watcher expired without touching anything. Codex raised this one.
  • :626 — the journal line says "applied a final restore" whether or not the chmod on the line above succeeded, and nothing re-checks the mode. That is the one visible trace of an expiry, so it can tell a maintainer the share was restored while it is still 2777. Also Codex's.

Codex. It agreed on the central claim and its independence is not currently guaranteed, so read that as agreement rather than confirmation — I had reached and measured the same conclusion first. What it contributed on its own terms is all three items above: the test gap, the unconditional expiry chmod, and the log overclaim. I verified each against the source before repeating it, and rejected nothing.

Checked on a disposable VM at fb1e500f: ./test/cli (116 ok), and through the session wrapper windows-vm-mount-boundary-test.sh (11), windows-vm-test.sh (5), windows-vm-compose-test.sh (22), windows-key-test.sh (7) — all green.

Where this sits against the field, re-derived today. Nine open pull requests now fix the chmod 0700 half of this: #10411 (new today), #10046, #9564, #10338, #9605, #9504, #9414, #9322 and this one. Every one of them makes launch stop failing. Only this one closes the other half — dockur's samba.sh chmods the caller's ~/Windows to 2777 through the bind, and nothing put it back — which is the reason the setgid bit keeps arriving in the first place. #10411 additionally clears set-ID in prepare_boundary_component, a site this PR leaves alone; that one is cosmetic, since assert_boundary_dir tests & 022 rather than an exact mode, so a setgid root-owned boundary dir passes either way. #10262 still collides on the exact chmod 0700 -- "/proc/$BASHPID/fd/..." line this PR replaces, and landed carelessly after this one it would put chmod 0700 back and reopen the bug. #10113 goes the wrong way — it teaches the check to accept 2777 rather than clearing it. Which of these lands is the maintainer's call, not mine.

@ekollof

ekollof commented Sep 6, 2026

Copy link
Copy Markdown
Author

Two additions while reproducing a launch failure on a dev-linked machine (~/Windows left at 2700, Failed to start Windows VM!):

e9d5d535 — refuse to elevate a mismatched packaged copy. pkexec can only run /usr/bin/omarchy-windows-vm, which omarchy dev link never shadows: the unprivileged half runs the checkout while the elevated half runs the package. On the machine I reproduced on, the packaged copy still shipped chmod 0700 -- "/proc/$BASHPID/fd/...", so root's prepare_caller_mounts turned a 2777 share into 2700 (setgid survives numeric modes), failed the exact-700 check, and left no diagnostic. priv() now compares the elevated target against the running script (same-inode fast path, sha256 otherwise) and refuses with an actionable message before any polkit prompt. Docker-group users are unaffected: their privileged half is the same file.

7a5fa2ee — boundary suite is no longer host-state-sensitive. The suite failed on that machine for an unrelated reason worth fixing on this branch: a host VM leaves anchors mounted at exactly the production paths the test re-creates. The user-ns copy of the mount table keeps them listed in /proc/self/mountinfo (they are MNT_LOCKED — the kernel refuses umount2 with EINVAL), the tmpfs over-mount hides them from the filesystem but not from mountinfo, and mountpoint(1) matches entries by path — so mountpoint -q reported the fresh anchor dirs as existing mounts, prepare_mount_anchor short-circuited, and the suite failed whenever the host had ever run the VM. The fixture uids are now chosen at runtime to avoid any uid the host already has anchors for.

All suites green at this head: windows-vm-test (6), mount-boundary (11), compose (22), windows-key (7).

emielkollof-cs and others added 9 commits September 16, 2026 02:06
GNU chmod leaves setuid/setgid on directories for numeric modes of four
digits or fewer, so chmod 0700 cannot satisfy the exact-700 mount check
when ~/Windows was created with g+s. Harden with a-s,u=rwx,go= and
print the observed modes when the check still fails.
samba.sh treats an empty /shared bind as uninitialized and chmod 2777s
it at container start, undoing the host 700 privacy check after every
launch. Keep a hidden sentinel in the share and re-harden the directory
after docker compose up.
Creating ~/.omarchy-keep as root in a caller-owned directory is a
symlink-follow write primitive. Restore mode 700 on the pinned
directory inodes after the guest reports ready, and never fail a
successful start on that chmod.
restore_shared_privacy also chmodded $LEGACY_SHARED, which is $HOME/Windows: a pathname the unprivileged caller owns. The [[ -d && ! -L ]] test and the chmod are two syscalls, so the caller can swap the directory for a symlink in between and make the root half of __priv_up_wait chmod an arbitrary path to 0700 with the set-ID bits cleared. On a worker a swapper loop won that race on its 260th iteration, taking a root-owned 4755 binary outside the caller's home to root:700.

The loop's other element already covers the case. $EXPECTED_SHARED sits in the root-owned 0711 boundary tree 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 — so chmodding the anchor is what ~/Windows ends up at, measured rather than assumed.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
install uses priv up, which returned as soon as the container started
and never re-hardened ~/Windows after samba.sh chmod 2777. Wait for
that 2777 (or the shared-folder log line) before restoring, and also
restore after dc down and when the guest-ready wait times out.
The 15s wait on priv up expires before dockur finishes the ISO
download, so samba.sh still chmod 2777s afterwards. Watch the
caller's share in the background after install and restore as the
owner. On stop, re-harden every protected per-uid share under the
runtime mounts tree instead of resolve_caller (no PKEXEC_UID under
direct sudo, and a second user would restore the wrong anchor).
disown only stops bash from hupping a background job when the shell itself exits. The watcher runs in the install terminal's foreground process group, so the kernel hangs it up when that terminal goes away — and install is launched by omarchy-launch-floating-terminal-with-presentation, which closes as soon as the user dismisses the "Press any key" prompt, minutes before dockur's samba.sh reaches the chmod 2777 the watcher exists to undo. Ignoring SIGHUP in the subshell is what disown was reaching for. An asynchronous command already ignores SIGINT and SIGQUIT when job control is off, so HUP is the only gap.

Measured under a pty: without the trap the share is still 2777 four seconds after the terminal exits; with it the watcher restores 700. The new test reproduces that shape with script(1) and fails when the trap is removed.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@ekollof
ekollof force-pushed the fix/windows-vm-setgid-chmod branch from fc0833d to 4f2ba28 Compare September 16, 2026 00:06
@ekollof

ekollof commented Sep 16, 2026

Copy link
Copy Markdown
Author

Rebased onto current quattro (2fbac0c). All 11 commits replayed cleanly, no conflicts. windows-vm-test.sh and windows-vm-mount-boundary-test.sh pass.

@ekollof

ekollof commented Sep 16, 2026

Copy link
Copy Markdown
Author

Fix pushed for a style violation this PR introduced: bin-style-test.sh ("bin commands use command helpers") flags the raw command -v systemd-run probe in schedule_share_privacy_restore. Found while triaging full ./test/shell failures — upstream's copy of this file has no such probe, so this would have failed CI on merge.

2270df6a switches the probe to omarchy-cmd-present systemd-run. Behavior is unchanged: the background-subshell fallback for tests and stripped sessions without a user bus is preserved exactly as before.

Verified: bin-style-test.sh, windows-vm-test.sh, windows-vm-mount-boundary-test.sh, and windows-vm-compose-test.sh all green.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment