Skip to content

Clear set-ID bits on Windows VM directories - #10046

Open
0bsolescence wants to merge 1 commit into
omacom:quattrofrom
0bsolescence:fix/windows-vm-clear-setgid
Open

0bsolescence wants to merge 1 commit into
omacom:quattrofrom
0bsolescence:fix/windows-vm-clear-setgid

Conversation

@0bsolescence

Copy link
Copy Markdown

Fixes #9374.\n\nGNU chmod preserves set-ID bits on directories when applying a numeric mode, so a shared directory left at mode 2700 remained 2700 after chmod 0700. The following exact-mode check then rejected it and prevented the Windows VM from launching.\n\nUse an explicit symbolic mode that clears special bits while setting private 0700 permissions. Add a mount-boundary regression case starting with a setgid shared directory.\n\nTests:\n- bash test/shell.d/windows-vm-mount-boundary-test.sh (passes in an isolated privileged namespace)\n- ./test/all (Windows VM tests pass; five unrelated environment-dependent files fail because omarchy-pkgs is not checked out and the live desktop differs from test expectations)

@spuder

spuder commented Sep 3, 2026

Copy link
Copy Markdown

possible duplicate of #9783 #9605 #9564

@omarchybot

Copy link
Copy Markdown
Collaborator

Reviewed at head cd26237. The production change is correct, and the test line you added is a real regression test rather than a restatement.

Checked the mode arithmetic by running it rather than by reading the manual. On GNU coreutils 9.11, chmod 0700 on a 2755 directory gives 2700; chmod a-s,u=rwx,go= gives 700; and chmod u=rwx,go= without the a-s clause gives 2700. So a-s is load-bearing, not belt-and-braces — = does not clear the set-ID bits on a directory either. go= clears the sticky bit through o=, so the symbolic form is exactly what 0700 was meant to do, and clause order does not matter: a-s,u=rwx,go=, u=rwx,go=,a-s and u=rwx,a-s,go= all produce 700 from 2755, 6755, 7777 and 4700. It also clears setuid, not just setgid.

Tests ran on a disposable VM, not on the machine doing the review. test/shell.d/windows-vm-mount-boundary-test.sh (9 passed), windows-vm-compose-test.sh (22 passed) and windows-vm-test.sh (3 passed) are green at this head. Reverting only the production line back to chmod 0700 and leaving your test line in place fails at test/shell.d/windows-vm-mount-boundary-test.sh:110, "root could not create verified production bind anchors".

Also ran the mode the open issues actually describe. dockur/windows' src/samba.sh runs chmod 2777 "$dir" on the share at container start, so ~/Windows comes back setgid and world-writable after every VM start. Setting both sources to 2777 instead of 2755 in the boundary test still passes at this head: the sources normalize to 700, the leaves assert 1000:700, and the setpriv checks confirm another local account cannot read through the anchors. With chmod 0700 restored, that same run fails at line 110. That is the recurring launch failure reported in #10004, #10047, #10114, #10149 and #10151, and this closes it, not just the first launch.

What it does not cover: bin/omarchy-windows-vm:1018, in prepare_user_mount_sources(), still uses chmod 0700 -- "$storage" "$shared" on those same two directories. Probed on the worker at this head with ~/.windows at 2700 and ~/Windows at 2755, the function returns 0 and leaves both at 2700. It blocks nothing, because install and the legacy-compose migration both reach the elevated prepare_caller_mounts() on the same run and that now repairs the mode — but it is the same bug at the site whose only job is to make those two directories private. bin/omarchy-windows-vm:1061 (write_credentials, the directory holding the plaintext RDP password) and bin/omarchy-windows-vm:267 (prepare_boundary_component) use the same idiom; neither is checked for an exact mode, so neither can fail. Nothing pushed to your branch for these: whether this PR should grow to cover them is a scope decision for the maintainer, and other open PRs on this line make that call differently.

Separately, the rejection at bin/omarchy-windows-vm:588-594 still returns 1 with no message, which is why one root cause has arrived as #9374, #10004, #10047, #10114, #10149 and #10151.

Second opinion from Codex (gpt-5.6-sol) at xhigh reasoning: no defect found. It agreed on the mode arithmetic, on lines 1018/1061/267 being defence-in-depth rather than launch blockers, and on which assertion breaks without the fix — but that agreement is with conclusions already written down in this session and its independence is not currently guaranteed, so it is worth less than the mutation runs above. It did add one thing that was not already checked: the mounted_leaf_matches calls at lines 447 and 958 belong to the removal path only, so they are not a second launch-time rejection route.

Waiting on the maintainer. This is one of six open pull requests touching this line — #9564, #9605, #9783, #9989, #10113 and this one — and choosing between them is not triage's call. Worth knowing when you read the thread: #9564 changes the identical line to the identical symbolic mode.

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

windows-vm: chmod 0700 preserves directory setgid bit, causing launch to fail safety check

3 participants