Conversation
|
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 The mode fix is correct, and it is the right kind of repair. On coreutils 9.11, The sentinel is where the defects are. Its premise holds — dockur's
Neither of the two behaviours that distinguish this PR — the sentinel under the privileged path, and Nothing was pushed to your branch. Finding 1 needs a design decision rather than a patch — drop the privileged creation, or open with Waiting on the maintainer, both for that choice and for a decision on the sentinel. |
|
Addressing the review by dropping privileged sentinel creation.
This revision:
The world-writable window is now only during guest boot, not until the next privileged action. |
c194fcf to
e3d3190
Compare
|
Re-reviewed at Where the previous findings stand. Finding 1 (root create/truncate through New, and pushed as The other loop element already covers the case, so the fix is to delete the legacy one. Still open, for you rather than a patch from me: What was checked. The delta only — everything under 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 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 Waiting on you for the |
|
Picked both of the remaining options.
This revision:
Still never chmod |
|
Re-reviewed at Everything from the last review is closed. The Worth being precise about what the re-hardening is buying, because it is not the launch failure. The silent-launch bug is fixed by Which makes the new function's failure matter, because it is exactly that gap on the install path. The The rest is smaller:
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 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 Checked on a disposable VM: |
|
The 15s privileged wait on This revision:
PR body updated: sentinel text is gone, and the extra setgid issues are listed. |
|
Re-reviewed at Where the last round's findings stand. The install-path gap is closed in design — Pushed as Still open, and yours rather than a patch from me: Two smaller things, both in the watcher and neither pushed:
On the rest of the diff, which I re-checked in full. Codex. It reached the same two conclusions about the terminal hangup and the sudoless glob, with the kernel tty-hangup and 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 Checked on a disposable VM: |
|
Addressing the review at Sudoless-Docker restore (the main open finding): took the non-root-branch option. 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 Defence in depth: Tests: the unit test now pins that an unprivileged |
|
Re-reviewed the delta The open finding is fixed, and I did not take the description's word for it. Built the production shape on a worker — Pushed as The watcher's other two fixes check out. A Three small things left to you, none blocking.
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 Where this sits against the field, re-derived today. Nine open pull requests now fix the |
fb1e500 to
ab2d8e9
Compare
|
Two additions while reproducing a launch failure on a dev-linked machine (
All suites green at this head: windows-vm-test (6), mount-boundary (11), compose (22), windows-key (7). |
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>
fc0833d to
4f2ba28
Compare
|
Rebased onto current quattro (2fbac0c). All 11 commits replayed cleanly, no conflicts. windows-vm-test.sh and windows-vm-mount-boundary-test.sh pass. |
|
Fix pushed for a style violation this PR introduced:
Verified: |
Fixes #9698
Fixes #9374
Fixes #9334
Fixes #9540
Fixes #9567
Fixes #9884
Fixes #9943
Fixes #9746
GNU
chmod 0700leaves setuid/setgid on directories when the numeric mode is four digits or fewer.prepare_caller_mountsthen requires mode700exactly, so a leftoverg+son~/Windows(often2700or2777) 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.shalsochmod 2777s an empty/sharedbind after the ISO download. The helper re-hardens the protected bind anchor (never$HOME/Windowsby pathname) after the guest reports ready, onup_waittimeout, and onstop. Freshinstalldoes not hold a polkit session for the download: it backgrounds an owner-side wait for2777and then restores.