Skip to content

Clear set-ID bits when securing the Windows VM mount sources - #9605

Closed
omarchybot wants to merge 1 commit into
quattrofrom
fix/windows-vm-setgid
Closed

omarchybot wants to merge 1 commit into
quattrofrom
fix/windows-vm-setgid

Conversation

@omarchybot

Copy link
Copy Markdown
Collaborator

Closes #9374. Closes #9540. Closes #9567.

omarchy-windows-vm launch and install abort — the first with ❌ Failed to start Windows VM!, the second with ❌ Failed to write the Windows VM configuration. — when ~/Windows or ~/.windows carries the setgid bit. Nothing is written to stderr, so status reporting RUNNING and a working web console leave no way to tell what refused.

prepare_caller_mounts() hardens both pinned sources with chmod 0700 and then asserts stat -Lc '%a' is exactly 700. chmod with a numeric mode does not clear S_ISGID on a directory: the kernel preserves it whenever the caller is in the file's group or holds CAP_FSETID, which root always does. A directory at 2700 stays 2700, chmod still returns 0, and the != 700 branch rejects a source the same function had just been asked to secure — then returns 1 without a message.

Symbolic modes clear the set-ID bits, so the check now sees the mode the chmod was meant to produce. The same substitution applies at the two other sites that make these directories private: prepare_user_mount_sources(), which prepares the very directories the elevated path re-checks, and write_credentials(), where a set-ID bit would hand the directory holding the plaintext RDP password a group it was never meant to have.

The mode-verification branch gains a diagnostic. It stays reachable for reasons a user can act on, and returning silently is what made this take three separate reports to characterise.

Reviewed by Codex at xhigh, which confirmed the mode arithmetic across every set-ID combination and pointed out that the existing suites start these directories at 0755 and so pass against the broken chmod too; the regression cases added here start them at 2700/6700 and fail without the fix.

`chmod 0700` does not clear S_ISGID on a directory: the kernel preserves it
whenever the caller is in the file's group or holds CAP_FSETID, which root
always does. A `~/Windows` or `~/.windows` carrying the setgid bit therefore
stayed 2700 through the privacy step, and the exact `!= 700` check that follows
rejected a source the same function had just been asked to secure — aborting
launch and install with no message on stderr.

Symbolic modes clear the set-ID bits, so the check now sees the mode the chmod
was meant to produce. The mode-verification branch gains a diagnostic, because
it is reachable for reasons a user can act on and previously returned silently.

Co-Authored-By: Codex XHigh <codex@openai.com>
@Zhruoshui

Copy link
Copy Markdown

+1 from an affected user — 4.0.3-1, and the analysis here matches what I found independently on my machine.

A few details that might be useful in the thread.

chmod g-s isn't a sufficient workaround, and it's the one people keep recommending across the duplicate issues. dockur's samba doesn't only set 2700 — on my box the share came back as drwxrwsrwx (2777) after a session. g-s on that gives you 777, which still trips the != 700 check, so the next launch fails again and it looks like the workaround "wore off". u=rwx,go=,a-s is the right form precisely because it lands on 700 from either variant. I've seen both 2700 and 2777 on the same install.

The install path breaks the same way, not just launch. prepare_caller_mounts is called from __priv_write_compose as well (line 762), so a first-time omarchy-windows-vm install dies at the same guard and only reports "Failed to write the Windows VM configuration." That one is nastier than the launch failure, because remove_windows() deletes ~/.windows and ~/.config/windows but leaves ~/Windows alone — the set-ID bit survives a remove, so remove + reinstall is guaranteed to fail. Reinstalling is the first thing people try, which is probably feeding the duplicate count.

On the numeric-mode behaviour, for anyone still wondering whether it's the kernel or the filesystem: it's coreutils. 9.11 here, package checksums clean. Against an existing 2700 directory:

chmod 0700              -> 2700
chmod 700               -> 2700
install -d -m 0700      -> 2700
chmod 00700             -> 700
chmod u=rwx,go=,a-s     -> 700
python os.chmod(0o700)  -> 700

The raw syscall clears it, so it's the four-digits-or-fewer rule for directories — nothing btrfs-specific. Same applies to the other two chmod 0700 sites this PR converts (1018 and 1061).

Lastly, the silent return 1 at 590 is doing a lot of the damage on its own. Before I found the guard I had written off docker and polkit, since nothing in the output mentions permissions anywhere. The added echo is arguably worth as much as the chmod fix.

@omarchybot

Copy link
Copy Markdown
Collaborator Author

Reviewed at 1b94d0cc against current quattro (f45461a3). The bug is real and this fix is correct, but #12323 is the better fix for the same bug, and it already carries verified and ready. So this PR was not run on a worker again, and it is not the one to merge.

Why #12323 over this one. Both make the same change at the two sites that matter, prepare_caller_mounts (bin/omarchy-windows-vm:583) and prepare_user_mount_sources (:1018): a symbolic chmod u=rwx,go=,a-s, with the exact-700 check left as it is. Both add a diagnostic where the check used to fail silently. Here is where they differ:

  • Clear setgid when hardening Windows VM directories #12323 tests the privileged path in the root mount-boundary suite. It covers setuid and setgid sources behind live anchors and again after the binds are recreated (the reboot case), and it fault-injects the diagnostic. The test here goes through __priv_write_compose, which is a real path, but it leaves the rebind case untested.
  • Clear setgid when hardening Windows VM directories #12323's diagnostic names both directories as well as their modes. This one gives only the modes.
  • This PR also changes write_credentials (:1061). That directory has no exact-mode check, and the credentials file inside it is written 0600, so a set-ID bit there causes no failure and exposes nothing. That change is extra scope, not part of the fix.

Other open fixes for the same bug: #9783 rewrites far more than this bug needs. #12019 adds no tests. #13154 leaves the user-side chmod at :1018 numeric. #13213 and #13750 are smaller and correct, but their tests are weaker.

Second opinion: Codex at medium effort was given all seven pull requests without our view and asked to pick one. It also picked #12323, for the same reasons. It read the same tree as this review, so that is agreement, not independent confirmation.

This now waits on the maintainer. The recommendation is to merge #12323 and close this one in its favour.

@omarchybot

Copy link
Copy Markdown
Collaborator Author

Closing this alternative after #12323 merged into quattro. The merged fix explicitly clears setuid/setgid in both mount-source hardening paths, keeps the exact mode-700 boundary, adds a diagnostic, and covers this launch/install failure. This alternative is no longer needed for that bug.

Scope checked by GPT-6 in Codex and Codex Medium against the discussion and landed change; review independence is not guaranteed. No tests were rerun for this cleanup.

Omabot on behalf of DHH

@omarchybot omarchybot closed this Oct 4, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working

Projects

None yet

2 participants