Stop making browser managed-policy directories world-writable - #6531
Stop making browser managed-policy directories world-writable#6531merdiofriviaisherebitch wants to merge 3 commits into
Conversation
The policy directories were chmod a+rw so omarchy-theme-set-browser could write color.json as the user, but that lets any local user or process drop arbitrary managed policies into a root-owned browser directory. Keep the directories at the default 0755 and pre-create a user-owned color.json at each creation site instead, so theme updates keep working without write access to the directory itself. A migration repairs existing installs. Fixes basecamp#5547
There was a problem hiding this comment.
Pull request overview
Hardens browser managed-policy setup by replacing world-writable directories with pre-created theme policy files.
Changes:
- Creates policy directories with restricted permissions.
- Adds migration logic for existing installations.
- Adds migration permission tests.
Tip
If you aren't ready for review, convert to a draft PR.
Click "Convert to draft" or run gh pr ready --undo.
Click "Ready for review" or run gh pr ready to reengage.
Reviewed changes
Copilot reviewed 3 out of 5 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
install/config/theme-system.sh |
Seeds Chromium’s theme policy during installation. |
bin/omarchy-install-browser |
Initializes policies for supported browsers. |
bin/omarchy-upgrade-to-quattro |
Updates the legacy upgrade path. |
migrations/1785760301.sh |
Repairs existing policy directories and files. |
test/shell.d/browser-policy-perms-test.sh |
Tests migration behavior and permissions. |
Suppressed comments (3)
install/config/theme-system.sh:15
- Chromium does not scope policy keys by filename; it parses every policy in each JSON file in this directory. Making
color.jsonuser-owned therefore still lets that user—or any process running as them—replace it with forced-extension, proxy, or security policies that affect every browser user. Keep the managed-policy file root-owned (for example, mode 0644) and update the theme through a narrowly scoped privileged helper or a non-managed user-level mechanism.
chown "$OMARCHY_INSTALL_USER:" /etc/chromium/policies/managed/color.json
migrations/1785760301.sh:24
- Affected directories are currently world-writable, so
color.jsonmay already be an attacker-created symlink or FIFO. Bothstat/chownhere andteein the other branch follow such entries; for example, a symlink to a root-owned file causes the migration to hand that file to the user. After changing the directory to 0755, remove any symlink or non-regular entry before inspecting or creatingcolor.json.
if [[ -f $policy_dir/color.json ]]; then
# Only repair files still owned by root; another user may have already
# claimed ownership, and that is the intended end state.
if [[ $(stat -c %U "$policy_dir/color.json") == "root" ]]; then
sudo chown "$USER:" "$policy_dir/color.json"
test/shell.d/browser-policy-perms-test.sh:82
- The root-owned branch's replacement stub logs
chown tester:but then executes it, so this test still fails on runners that are not root or do not have atesteraccount. Return after recording the call instead of invoking the real ownership change.
cat >"$stub_bin/sudo" <<STUB
#!/bin/bash
if [[ \$1 == chown ]]; then echo "\$*" >>"$chown_log"; fi
exec "\$@"
STUB
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| @@ -8,7 +8,11 @@ gtk-update-icon-cache /usr/share/icons/Yaru &>/dev/null || true | |||
|
|
|||
| # Chromium policy directory for theme | |||
| mkdir -p /etc/chromium/policies/managed | |||
There was a problem hiding this comment.
Fixed in 1d9db13. All creation sites now use explicit modes: install -d -m 0755 for the directory and chmod 0644 for color.json (in theme-system.sh, omarchy-install-browser, and the upgrade path), so a permissive umask can't reproduce the world-writable layout.
| policy_dirs="${OMARCHY_BROWSER_POLICY_DIRS:-/etc/chromium/policies/managed | ||
| /etc/opt/chrome/policies/managed | ||
| /etc/opt/edge/policies/managed | ||
| /etc/brave/policies/managed}" |
There was a problem hiding this comment.
The override is a test-only seam mirroring the existing precedent in the zram migration (OMARCHY_ZRAM_CONF / OMARCHY_ZRAM_DROPIN, exercised by test/shell.d/zram-migration-test.sh); without it the migration can only be exercised against real /etc paths. On the risk: exploiting it requires controlling the environment of a omarchy-migrate invocation that also passes the sudo password gate — i.e. the attacker already has the user's code execution and credentials, at which point the migration is not the escalation vector. Happy to hard-code the paths and drop the override if maintainers want the seam gone, but then this migration loses its automated coverage.
| if [[ $(stat -c %U "$policy_dir/color.json") == "root" ]]; then | ||
| sudo chown "$USER:" "$policy_dir/color.json" |
There was a problem hiding this comment.
You're right that Chromium merges every key from every file in managed/, so a user-owned color.json still lets the owning user — or a process running as them — inject other policies through that one file. I deliberately did not make it root-owned because of a functional constraint, and I'd like a maintainer steer on the trade-off.
What this PR fixes. The reported vulnerability (#5547) is a world-writable, root-owned policy directory: any local user, and any process, could add, edit, or delete arbitrary policy files. This PR restores the boundary — the directory becomes root-owned 0755, and only a single color.json is writable, and only by its owning user. Other local users are fully locked out.
Why the file is user-owned. omarchy-theme-set-browser updates the theme color at runtime, runs as the unprivileged user, and is launched from the graphical theme menu (no terminal). Keeping color.json root-owned would force privilege escalation on every theme change. The two options there are a pkexec/polkit prompt each time (a real UX regression for a frequent, frictionless action) or a new scoped passwordless-sudo rule — which the project is actively trying to shrink (see #5708).
The trade-off, stated plainly.
- This PR: fixes the reported world-writable bug and the multi-user case with no new privilege mechanism. Residual risk: a compromised process already running as the primary user can rewrite that one file. On a single-user desktop such a process already owns the user's browser profile, so the incremental reach is limited — but it is not zero.
- Root-owned + scoped privileged helper: closes user-level injection entirely, at the cost of a new polkit/sudo surface (and possibly a prompt per theme change).
I went with the first because it fixes the reported issue without introducing privilege machinery I can't validate here, but I'm happy to rework this to a root-owned file behind a narrowly scoped helper in a follow-up if maintainers prefer that guarantee. Which direction do you want?
| cat >"$stub_bin/sudo" <<'STUB' | ||
| #!/bin/bash | ||
| exec "$@" | ||
| STUB |
There was a problem hiding this comment.
Fixed in 1d9db13. The sudo stub now records chown invocations and returns without executing them (while still executing the fixture-safe chmod/tee/rm), so the test no longer depends on the runner's user or privileges. Also added an explicit symlink-replacement case.
Create policy directories and color.json with explicit modes so a permissive umask cannot reproduce a world-writable layout, and treat any non-regular color.json (symlink or FIFO planted while the directory was world-writable) as untrusted: replace it instead of letting stat, chown, or tee follow it. The test's chown handling is now recorded rather than executed so it runs on any user.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 5 changed files in this pull request and generated 1 comment.
Suppressed comments (2)
migrations/1785760301.sh:36
- Handing
color.jsonto the user still allows any process running as that user to write arbitrary Chromium managed-policy keys into it. The directory mode prevents new filenames but does not constrain the contents Chromium accepts. Repair this as a root-owned file and change theme synchronization to use a narrowly scoped privileged writer for only the color policy.
sudo chown "$USER:" "$color_json"
install/config/theme-system.sh:17
- This user-owned policy file remains writable by every process running as the desktop user, and Chromium does not restrict accepted policy keys based on the
color.jsonfilename. A compromised process can therefore still inject arbitrary managed policies. Keep this file root-owned and route fixed-schema color updates through a narrowly scoped privileged helper instead.
chown "$OMARCHY_INSTALL_USER:" /etc/chromium/policies/managed/color.json
| if [[ $(stat -c %a "$policy_dir") != "755" ]]; then | ||
| sudo chmod 755 "$policy_dir" | ||
| fi |
There was a problem hiding this comment.
Fixed in 0c13271. After restoring 0755, the migration now walks every entry in the directory: anything that is not the theme's color.json and not a trusted root-owned policy is removed. Root-owned entries (administrator or package-managed policies) are preserved, so a non-root-owned forced-extension.json planted during the world-writable period is cleaned out rather than left active. Covered by new test cases (attacker-planted file removed, root-owned policy preserved, color.json kept); the removal assertions fail without this change.
Fixing the directory mode only stops future additions; regular policy files planted during the world-writable period would remain active indefinitely. After restoring 0755, drop every entry that is not the theme's color.json and not a trusted root-owned policy, preserving administrator and package-managed policies.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 5 changed files in this pull request and generated no new comments.
Suppressed comments (1)
migrations/1785760301.sh:54
- This only repairs root-owned
color.json. While the directory was0777, another local user could create a regularcolor.json; that attacker-owned file is excluded from cleanup and remains writable and active after this migration. A retained file may also still be mode0666. Recreate/reseed files owned by neither root nor the migrating user, and normalize every retainedcolor.jsonto0644.
elif [[ $(stat -c %U "$color_json") == "root" ]]; then
# Only repair files still owned by root; another user may have already
# claimed ownership, and that is the intended end state.
sudo chown "$USER:" "$color_json"
Summary
Fixes #5547.
The Chromium/Chrome/Edge/Brave managed-policy directories were created
chmod a+rw(mode 0777) so thatomarchy-theme-set-browsercould writecolor.jsonas the unprivileged user. But a world-writable, root-owned browser policy directory lets any local user — or any process running as that user — drop or edit arbitrary managed policies (forced extensions, proxies, disabled security features). The browser only needs to read these files.Approach
Keep the directories at the default
0755(root-writable, world-readable) and instead pre-create a single user-ownedcolor.jsonat every creation site. A user can still update the contents of a file they own even inside a root-owned, non-writable directory, so theme syncing keeps working — but nothing can create, rename, or delete policy files anymore.Creation sites updated:
install/config/theme-system.sh— fresh installs (owns the file to$OMARCHY_INSTALL_USER)bin/omarchy-install-browser— Chrome/Edge/Brave/Brave Origin get a seededcolor.json; Firefox/Zen distribution dirs only need the mkdir since theirpolicies.jsonis copied as rootbin/omarchy-upgrade-to-quattro— the 3.x→4.0 upgrade path (wasinstall -d -m 0777)A new migration repairs existing installs: restores
0755, hands the user ownership of an existing root-ownedcolor.json, and seeds the file when it is missing (without directory write access the theme command can only update an existing file). It no-ops when another user already claimed ownership.Testing
Added
test/shell.d/browser-policy-perms-test.shcovering the migration: world-writable repair, missing-file seeding, idempotent re-runs, missing directories, and the root-owned reclaim branch. All pass; full./test/allsuite shows no regressions.