Repository navigation
Conversation
|
Verified: At
Scope limit: With both path variables unset, the intentionally retained fallback still selects Review informationTest scope: Real update entrypoint, util-linux Community review: Independent automated community review, unaffiliated with the Omarchy team, intended to help prepare PRs for their review. Automated AI review: Astra Medium initial inspection and synthesis, independent Opus 5.5 High and GPT 6 Sol Xhigh technical reviews, followed by verification of the stated claims against targeted evidence. |
|
Tested in the Omarchy VM running Omarchy 4.0.4-1 with util-linux 2.42.3:
Works as expected with no issues. |
5dc63e6 to
cf919bc
Compare
Automated AI review
Outcome: At Earlier review: all verified results and the Plain
|
| Earlier claim | Status at cf919bc |
|---|---|
Transcript path survives the script(1) and lock re-execs; analyzer reads it |
Still holds |
Valid private XDG_RUNTIME_DIR keeps transcript and lock there; no /tmp transcript |
Still holds |
Explicit OMARCHY_UPDATE_LOG is passed to script |
Still holds |
| UID 0 refused clearly before any update step or transcript | Holds when OMARCHY_PATH is kept (sudo -E, sudo -i, root shell). Plain sudo is stopped earlier with the source-root message (finding above) |
Missing transcript: analyzer exits 0 without the raw grep error |
Still holds |
| Analyzer warns without the initcpio success marker, quiet with it | Still holds |
Scope limit: both path variables unset still selects /tmp/omarchy-update.log |
Still applies |
Root runs, simulated with mock sudo (UID 0 from fakeroot and from user-namespace root, same results):
| Root environment | base 324f0ba |
head cf919bc |
head, refusal after set -e |
|---|---|---|---|
plain sudo (no OMARCHY_PATH) |
source-root error, no script |
same as base | clear refusal |
sudo -E |
reaches script with /tmp/omarchy-update.log |
clear refusal, after sudo -k and sudo -h |
clear refusal, no sudo calls |
sudo -i |
reaches script with /tmp/omarchy-update.log |
clear refusal, after sudo -k and sudo -h |
clear refusal, no sudo calls |
PR description: at the current base, a plain sudo omarchy update already stops at the source-root check before script runs, so it cannot leave a root-owned /tmp/omarchy-update.log. That failure mode applies to 4.0.4 and, at the base, to sudo -E, sudo -i and root shells, which the new refusal covers.
Related open PRs:
- Point at the running update log when the lock is held #12094 adds a hint that hard-codes
tail -f /tmp/omarchy-update.logtoomarchy-update-lock. After this PR, that hint would point at the wrong file. - [Security] Keep the update transcript out of world-writable /tmp #8429 uses the same runtime-dir path with a state-directory fallback; Stage diagnostics logs privately instead of at fixed /tmp paths #7995 and Keep update/runtime/diagnostics out of world-writable /tmp #12109 move the transcript elsewhere. Keep update/runtime/diagnostics out of world-writable /tmp #12109 also adds
test/shell.d/update-log-path-test.sh, the same path as this PR's new test. Exit cleanly when the update log is missing #11217 is another fix for omarchy update analyze logs fails with a raw grep error when the update log doesn't exist #11204. - Stop treating an initramfs hook banner as a failed rebuild #12835 stops warning on a bare
Updating linux initcpiosbanner, whichupdate-log-path-test.sh:75expects to warn on. - Repair stale boot images before restart #6840 and Warn when root Btrfs spans extra LUKS devices the initramfs cannot unlock #11023 append checks that do not depend on the log to the end of the analyzer. The new early
exit 0would skip those checks when no transcript exists.
Optional notes:
docs/update-process.md:26and:311leave out the retained/tmpfallback. Writing${XDG_RUNTIME_DIR:-/tmp}/omarchy-update.log(override:OMARCHY_UPDATE_LOG), as the lock row does, would match the code.- The new test still passes 8/8 when the analyzer's fallback is changed to
/tmp, or whenexport OMARCHY_UPDATE_LOGand theenvpass-through are both dropped. The root case also never checks thatscriptwas not called. A standalone analyzer case with onlyXDG_RUNTIME_DIRset would catch the fallback change; having thescriptstub also record$OMARCHY_UPDATE_LOGfrom its environment would catch the dropped export; an empty-$script_callsassertion would pin the root case. - The description's
That closes **#11204**is not registered as a closing reference.Closes #11204would link the issue.
Review information
Test scope: Source and sandbox tests at cf919bc (real script(1), lock and analyzer, other steps stubbed); sudo simulated, no cross-user test.
AI process: Opus 5.5 Medium coordination and synthesis, Opus 5.5 Xhigh technical review and final fact check, GPT 6 Sol Xhigh search for related issues, Opus 5.5 Medium editorial check.
Opt out: To stop receiving these reviews, reply to this comment saying so.
omarchy-update re-execs itself through script(1) into a hardcoded
/tmp/omarchy-update.log, and omarchy-update-analyze-logs reads the same
path back. /tmp is world-writable and nothing checks the file's owner.
script opens it with O_CREAT, which fs.protected_regular refuses when the
file belongs to someone else, so whoever owns that file decides who may
run omarchy update.
Two ways that goes wrong, both reproduced on 4.0.4 in a VM:
- another local user creates the file and every other user's update
aborts with EACCES before the confirmation prompt. The sticky bit
means the victim cannot remove it.
- with no attacker at all, one `sudo omarchy update` leaves the
transcript root-owned and every later unprivileged run fails the same
way, with an error naming script(1) rather than Omarchy.
Write the transcript to the per-user runtime directory instead, and
export the path so the consumer reads the same file rather than
hardcoding it twice.
Refuse to run as root. XDG_RUNTIME_DIR is unset under sudo, so the path
change alone would leave the second case unfixed. Every step already
calls sudo for itself, omarchy-dev-link refuses root the same way, and
omarchy channel set could not run under sudo before this either since it
calls omarchy-dev-link.
omarchy-update-analyze-logs now exits quietly when no transcript exists,
which closes omacom#11204. This is not omacom#11630, which is a different symptom on
the same line: exit 0 with no log created at all.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
cf919bc to
047a6af
Compare
 |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| # local user could pre-create it and lock this one out of updating. The path is | ||
| # exported so omarchy-update-analyze-logs reads the same file instead of | ||
| # hardcoding it a second time. | ||
| OMARCHY_UPDATE_LOG="${OMARCHY_UPDATE_LOG:-${XDG_RUNTIME_DIR:-/tmp}/omarchy-update.log}" |
There was a problem hiding this comment.
If a non-root user runs an update without XDG_RUNTIME_DIR, this fallback passes the fixed /tmp/omarchy-update.log path to script. Another local user can pre-create that file and prevent the transcript from opening, blocking the update before confirmation. The new test explicitly preserves this fallback, so the cross-user lockout remains in a supported case.
How this was verified: The unset-runtime path passes the shared /tmp filename directly to script, where another user's pre-created file prevents the transcript from opening.
| root_out=$(fakeroot env -u OMARCHY_UPDATE_LOGGED -u OMARCHY_UPDATE_LOG \ | ||
| SCRIPT_CALLS="$script_calls" "$SUDO_TEST_ROOT/bin/omarchy-update" 2>&1 || true) | ||
| grep -q "not under sudo" <<<"$root_out" || |
There was a problem hiding this comment.
If a test host lacks fakeroot, both root-refusal checks fail because their command cannot start, even when the updater works. The shell suite discovers and runs this test automatically, but the test does not declare or check that dependency.
| # local user could pre-create it and lock this one out of updating. The path is | ||
| # exported so omarchy-update-analyze-logs reads the same file instead of | ||
| # hardcoding it a second time. | ||
| OMARCHY_UPDATE_LOG="${OMARCHY_UPDATE_LOG:-${XDG_RUNTIME_DIR:-/tmp}/omarchy-update.log}" |
There was a problem hiding this comment.
The update now keeps its only transcript in XDG_RUNTIME_DIR, which is removed at logout. A user who returns later to diagnose a failed update can no longer inspect the transcript that the update documentation recommends for debugging. A durable diagnostic copy or a documented retention limit would make that cost clear.
Knowledge Base Used: System updates and migrations
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
|
Closing after maintainer review. We are not accepting this as a security vulnerability that warrants these changes for Omarchy's intended single-user desktop setup. The cross-account disclosure scenario requires an additional untrusted local account, or a separately compromised service account, plus a run of the relevant command. The report has not demonstrated a sensitive credential disclosure or a consequential exploit in the default setup. Root can already read this information, and malicious programs running as the desktop user can still read it after a move to a private file or directory. Hostname, hardware details, and package inventory alone do not establish a security vulnerability. The default systemd protections ( The default Private staging and explicit cleanup can be reasonable housekeeping, but we do not consider the demonstrated behavior a security priority for the default Omarchy threat model. We are declining this family of security-motivated changes on that basis. References: systemd default protections, kernel documentation, default tmpfs mount. The reproduced root-owned transcript after sudo omarchy update is a distinct functional problem, and we acknowledge it can affect a single-user machine. It is not proof of an attacker in the default setup; its /tmp leftover is also cleared on reboot. If pursued, root-invocation handling should be addressed as a focused functional fix rather than as this broader security change. The separate missing-log issue #11204 remains open. Omabot on behalf of DHH |
4 participants
omarchy-updatere-execs itself throughscript(1)into a hardcoded path, as the firstthing it does, and
omarchy-update-analyze-logsreads the same path back:/tmpis world-writable and nothing checks who owns that file.scriptopens it withO_CREAT, whichfs.protected_regular=1refuses when the file belongs to someone else. Sowhoever owns the file decides who may run
omarchy update.This is not the same issue as #11630. That one exits 0 with no log created at all. This one
exits 1 with
script: cannot open /tmp/omarchy-update.log: Permission denied, and needs thefile to already exist owned by another user.
Two ways it goes wrong
Another local user blocks everyone's updates. Verified on 4.0.4 in a VM:
It aborts before the confirmation prompt and before any work. The sticky bit means the
victim cannot remove the file, and mallory can recreate it. The practical effect is that
the machine stops receiving updates.
One
sudo omarchy updatedoes the same thing with no attacker involved. There is no rootcheck today, so the transcript is left root-owned and every later unprivileged run fails the
same way:
The error names
script, not Omarchy, and nothing points at a stale file in/tmp, so thelikely outcome is a user who concludes updates are broken.
The change
Put the transcript in the per-user runtime directory, which is
0700and owned by the user,and export the path so the consumer reads the same file instead of hardcoding it a second
time:
Refuse to run as root.
XDG_RUNTIME_DIRis unset undersudo, so the path change alonewould leave the second case unfixed. Every step of the update already calls
sudofor itselfwhere it needs to, and
omarchy-dev-linkrefuses root in exactly this way, so this is theexisting idiom rather than a new constraint. It also cannot break
omarchy channel set,which calls
omarchy-dev-linkand therefore already could not run undersudo.omarchy-update-analyze-logsexits quietly when there is no transcript. That closes#11204, which reports a raw
greperror when the log does not exist.Scope
Two scripts,
docs/update-process.md(four references), and a new test. No behaviour changeto the update flow itself. The fallback to
/tmpis kept for the case whereXDG_RUNTIME_DIRis genuinely unset, which the root refusal makes much rarer.Verification
Both cases were reproduced and then re-tested against the patched scripts in a VM. With
another user's file planted at the old path,
omarchy updatenow reaches the confirmationprompt normally;
sudo omarchy-updateis refused with a clear message; and the transcriptlands at
/run/user/1000/omarchy-update.logowned by the user.New
test/shell.d/update-log-path-test.shstubsscript(1)and asserts the transcript pathunder a runtime dir, that the shared
/tmppath is no longer used, the explicit override,the fallback, and the root refusal (via
fakeroot), plus the three analyze-logs behaviours.Confirmed it fails when the old behaviour is restored.
🤖 Generated with Claude Code