Skip to content

[codex] OM-SEC-15: Keep mixed-trust installers outside sudo lifetime - #9470

Draft
AFOliveira wants to merge 10 commits into
omacom:quattrofrom
AFOliveira:codex/om-sec-15-cold-install-chains
Draft

[codex] OM-SEC-15: Keep mixed-trust installers outside sudo lifetime#9470
AFOliveira wants to merge 10 commits into
omacom:quattrofrom
AFOliveira:codex/om-sec-15-cold-install-chains

Conversation

@AFOliveira

@AFOliveira AFOliveira commented Aug 31, 2026

Copy link
Copy Markdown
Contributor

Mixed installers now finish and revoke their privileged work before language managers, Composer, downloaded vendor code or font selection run. Finding: OM-SEC-15. This branch builds on #9457's exact shared helper; canonical entrypoint resolution and startup checks protect all six affected entrypoints.

PHP, Laravel and Symfony use one fixed packaged system helper for their complete PHP prerequisite package set and configuration. The helper accepts only php or symfony, requires root/privileged Bash, runs in a clean environment, and accepts no caller paths, scripts or package names. One sudo -N call covers that phase, then authorization is revoked before user work. This resolves the earlier per-extension prompts and the incomplete package/configuration consolidation.

The new unprivileged regression exercises actual fixed configuration on temporary files, exact complete package sets, one total authentication per PHP flow, failures and revocation. Every protected installer and the package helper has a decoy-startup regression. Font tests now verify cold revocation, no-update package mode, revocation before font selection and failure cleanup. Namespace tests use login identity for subgid and probe the exact requested capability.

At d5d40fec, independent Daybreak review accepts the code; focused installer/font suites, CLI112, syntax and whitespace pass. In the disposable Omarchy VM, the actual PHP flow completed with exactly one password and the required PHP/Xdebug modules loaded. The Symfony system phase also completed with one password and installed the official symfony-cli package. Both left authorization revoked. No Laravel/vendor downloads, full GPU/vendor-install matrix or interactive prompt cancellation were exercised; safe orchestration fixtures cover those relevant boundaries.

Depends on #9457 and its coordinated runtime/settings release. Keep the new system helper packaged alongside the installers. No reusable timestamp or arbitrary root-command broker was added.

@AFOliveira
AFOliveira marked this pull request as draft September 1, 2026 09:49
@ErikMelton
ErikMelton requested a balanced review from Copilot September 1, 2026 19:42

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Hardens mixed-trust installation flows to prevent downloaded or user-owned code from reusing cached sudo authorization.

Changes:

  • Adds command-scoped sudo and credential revocation boundaries.
  • Hardens package, browser, language, font, and gaming installers.
  • Adds privilege-reuse regression coverage and documentation updates.

Reviewed changes

Copilot reviewed 3 out of 11 changed files in this pull request and generated 1 comment.

Show a summary per file
File Description
test/shell.d/install-chain-sudo-security-test.sh Adds installer-chain security regression tests.
manual/18-development-tools.md Documents Docker database credentials.
default/omarchy/sudo-no-update/sudo Adds a no-update sudo wrapper.
bin/omarchy-pkg-install Hardens interactive package installation.
bin/omarchy-pkg-add Supports command-scoped sudo.
bin/omarchy-install-gaming-gpu-lib32 Hardens graphics prerequisite installation.
bin/omarchy-install-gaming-geforce-now Isolates downloaded installer execution.
bin/omarchy-install-gaming-battlenet Separates prerequisites from vendor execution.
bin/omarchy-install-font Adds sudo revocation around font installation.
bin/omarchy-install-dev-env Separates privileged and user tooling phases.
bin/omarchy-install-browser Adds source validation and credential cleanup.

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment thread manual/18-development-tools.md Outdated
@AFOliveira

Copy link
Copy Markdown
Contributor Author

Addressed both review findings in a46f469:

  • Removed the Docker database credential documentation; that belongs only with OM-SEC-09 and no longer appears in this PR.
  • Fixed desktop-entry-launch-test.sh to execute the real #!/bin/bash -p font entrypoint while replacing only its fixed external command paths with deterministic stand-ins. The existing quoting, failure, and injection assertions all pass.
  • Removed the unrelated trusted-source browser rewrite (the full suite exposed it as a default-browser regression) and removed the OM-SEC-21 package-picker delta from this branch.
  • Documented the narrow privileged-Bash shebang exception in AGENTS.md.

Local validation on a46f469:

  • changed-shell bash -n and git diff --check: pass
  • desktop-entry-launch-test.sh: pass
  • default-apps-test.sh: pass
  • install-chain-sudo-security-test.sh: all 13 security cases pass
  • CLI: all 116 cases pass
  • full suite: 221/222 shell files pass; only launch-about-test.sh fails, with the identical a roomy window animates failure already reproduced on exact base 945af75

The PR remains draft while the remaining repeated startup/sudo helpers are reduced onto the shared foundation.

@AFOliveira
AFOliveira force-pushed the codex/om-sec-15-cold-install-chains branch from a46f469 to 5b74b46 Compare September 1, 2026 21:46
@AFOliveira

Copy link
Copy Markdown
Contributor Author

Shared-helper reduction is now published in 5b74b46. This removes the copied startup/sudo implementations and the temporary PATH wrapper, adds one 54-line source-only install helper, and keeps the #9457 core byte-identical. Local validation on the pushed commit: standalone baseline exploit succeeds while the candidate leaves no root target; focused suite 14/14 including the sudo -N mutation; desktop/default-app tests pass; CLI 116/116; full shell suite 221/222 with only the exact baseline-identical launch-about UI failure. The PR remains draft solely for #9457 sequencing.

@ErikMelton

Copy link
Copy Markdown
Member

A few more changes needed;

PHP installs require repeated authentication. install_php (

install_php() {
install_packages php composer php-sqlite xdebug
# Enable some extensions
local php_ini_path="/etc/php/php.ini"
local extensions_to_enable=(
"bcmath"
"intl"
"iconv"
"openssl"
"pdo_sqlite"
"pdo_mysql"
)
/usr/bin/sudo -N -- /usr/bin/sed -i \
-e 's/^;zend_extension=xdebug.so/zend_extension=xdebug.so/' \
-e 's/^;xdebug.mode=debug/xdebug.mode=debug/' \
/etc/php/conf.d/xdebug.ini
local ext
for ext in "${extensions_to_enable[@]}"; do
/usr/bin/sudo -N -- /usr/bin/sed -i "s/^;extension=${ext}/extension=${ext}/" "$php_ini_path"
done
) runs seven separate sudo -N edits after package installation. -N ignores cached credentials and does not update them, so a fresh PHP/Laravel install prompts about eight times; Symfony about nine. Consolidate privileged edits or otherwise authenticate once for a specific trusted phase. See sudo(8) (https://man.archlinux.org/man/sudo.8.en).

Desktop test still reaches host sudo. desktop-entry-launch-test. (

font_script="$test_tmp/omarchy-install-font"
cp "$ROOT/bin/omarchy-security-functions" "$ROOT/bin/omarchy-install-security-functions" "$test_tmp/"
sed \
-e "s#/usr/bin/omarchy-launch-floating-terminal-with-presentation#$mock_bin/omarchy-launch-floating-terminal-with-presentation#g" \
-e "s#/usr/bin/omarchy-pkg-add#$mock_bin/omarchy-pkg-add#g" \
-e "s#/usr/bin/omarchy-font-set#$mock_bin/omarchy-font-set#g" \
-e "s#/usr/bin/sudo#$mock_bin/sudo#g" \
-e "s#/usr/bin/sleep#/usr/bin/true#g" \
"$ROOT/bin/omarchy-install-font" >"$font_script"
chmod 0755 "$font_script"
) rewrites /usr/bin/sudo only in the font script, not in the copied sourced helpers. Those helpers still call the host’s /usr/bin/sudo, causing the test to fail in the sandbox instead of using its mock.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Changes recommended

The GPU helper still exposes inherited startup injection before package operations, and required regression coverage remains incomplete.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details
  • Files reviewed: 3/11 changed files
  • Comments generated: 3
  • Review effort level: Balanced

pass "install-app does not run extra commands from a quote in the display name"

bash "$ROOT/bin/omarchy-install-font" "Cascadia Mono" "ttf-cascadia-mono-nerd" "CaskaydiaMono Nerd Font"
"$font_script" "Cascadia Mono" "ttf-cascadia-mono-nerd" "CaskaydiaMono Nerd Font"
outer_uid=$(id -u)
outer_gid=$(id -g)
subuid=$(awk -F: -v user="$(id -un)" '$1 == user { print $2; exit }' /etc/subuid)
subgid=$(awk -F: -v group="$(id -gn)" '$1 == group { print $2; exit }' /etc/subgid)
Comment thread AGENTS.md
- Prefer a full `if`/`else` conditional for simple two-path control flow; don't rely on `exec` or `exit` in one branch to make following statements unreachable
- For strings/paths with spaces, quote them instead of escaping spaces with `\ ` (e.g., `"$APP_DIR/Disk Usage.desktop"`, not `$APP_DIR/Disk\ Usage.desktop`)
- Shebangs must use `#!/bin/bash` consistently (never `#!/usr/bin/env bash`)
- Shebangs must use `#!/bin/bash` consistently (never `#!/usr/bin/env bash`). A security-sensitive entrypoint may use the exact `#!/bin/bash -p` form only when it must suppress `BASH_ENV` and exported-function startup injection before its first command; that exception must be explained at the boundary and covered by a regression that rejects an ordinary Bash launch with a decoy `-p` argument.
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.

3 participants