fix(desktop): keep launching when the setuid sandbox helper cannot be configured - #86718
Open
z80dev wants to merge 1 commit into
Open
fix(desktop): keep launching when the setuid sandbox helper cannot be configured#86718z80dev wants to merge 1 commit into
z80dev wants to merge 1 commit into
Conversation
… configured A desktop-launcher context (rofi, app menu, the post-update relaunch) has no TTY, so the sudo chown/chmod fixup for chrome-sandbox always fails there and the launch died. Probe whether this process can actually create a user namespace (fork + unshare(CLONE_NEWUSER), mirroring Chromium's own CanCreateProcessInNewUserNS) and, when it can, continue with Chromium's namespace sandbox instead of exiting: Chromium only consults the setuid helper when the userns sandbox is unavailable, and when neither is available it aborts rather than running unsandboxed, so this cannot cause an unsandboxed launch. The --no-sandbox fallback for AppArmor-restricted hosts, the ELECTRON_DISABLE_SANDBOX=1 override, and root behavior are unchanged. Absence of apparmor_restrict_unprivileged_userns is not sufficient evidence (kernel.unprivileged_userns_clone=0, user.max_user_namespaces=0, missing CONFIG_USER_NS, seccomp), hence the probe. Restores the fixup-failure tests pruned in 6b81590 and adds coverage for the new branch.
Contributor
There was a problem hiding this comment.
Pull request overview
This PR improves Linux desktop launch reliability by allowing hermes desktop to continue launching when Electron’s setuid chrome-sandbox helper cannot be configured in non-interactive (no-TTY) contexts, as long as the Chromium user-namespace sandbox is actually available.
Changes:
- Adds an AppArmor userns restriction probe and a new
fork + unshare(CLONE_NEWUSER)capability probe to decide whether to continue after a sandbox-helper fixup failure. - Updates the Linux desktop launch path to (a) fall back to
--no-sandboxon AppArmor-restricted hosts, (b) continue with the userns sandbox when available, or (c) exit with an explicit recovery command when neither sandbox mechanism is usable. - Expands
tests/hermes_cli/test_gui_command.pycoverage for fixup-failure and userns-probe branches.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
hermes_cli/main.py |
Adds userns restriction + availability probes and updates Linux desktop launch decision logic after sandbox-helper fixup failure. |
tests/hermes_cli/test_gui_command.py |
Restores/extends regression tests for sandbox-helper gate behavior and adds unit tests for the userns probe. |
Suppressed comments (1)
tests/hermes_cli/test_gui_command.py:918
- Same issue as above: prefer the OS lane marker (
@pytest.mark.linux_only) overskipif(sys.platform != "linux")so this test is correctly selected/executed by the OS-specific CI lanes.
@pytest.mark.skipif(sys.platform != "linux", reason="needs os.fork")
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| # userns-sandbox continuation added for non-TTY launch contexts. | ||
|
|
||
|
|
||
| @pytest.mark.skipif(sys.platform != "linux", reason="Linux sandbox gate") |
Comment on lines
+7154
to
+7172
| import ctypes | ||
|
|
||
| try: | ||
| # Load libc before forking so the child only makes one syscall. | ||
| libc = ctypes.CDLL("libc.so.6", use_errno=True) | ||
| pid = os.fork() | ||
| except (OSError, AttributeError): | ||
| return False | ||
| if pid == 0: | ||
| try: | ||
| rc = libc.unshare(_CLONE_NEWUSER) | ||
| except BaseException: | ||
| os._exit(1) | ||
| os._exit(0 if rc == 0 else 1) | ||
| try: | ||
| _, status = os.waitpid(pid, 0) | ||
| except OSError: | ||
| return False | ||
| return os.WIFEXITED(status) and os.WEXITSTATUS(status) == 0 |
alt-glitch
added
type/security
Contributor
fix(desktop): keep launching when the setuid sandbox helper cannot be configured
|
|
I reproduced this exact no-TTY failure on Ubuntu 24.04 under WSL2 and verified the proposed userns continuation end to end. I opened a focused follow-up into your branch for the outstanding review findings: z80dev#1 The follow-up adds the repo Linux CI markers, Live proof on WSL2 with user-owned |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
On Linux hosts where Chromium's unprivileged user-namespace sandbox works,
hermes desktopfails on every launch whose context has no TTY — the.desktopentry (app grid, rofi, etc.) and the post-update detached relaunch._desktop_linux_sandbox_fixuptriessudo chown/chmodwhenchrome-sandboxisn'troot:root 4755; sudo cannot prompt;cmd_guiexits 1 with nothing visible, because GUI launches have no stdout. An in-app update rebuild resets the helper to user-owned and--build-onlyreturns before the fixup, so nothing re-provisions it — every content-changing update re-manufactures the failure.Fix
When the fixup fails, probe whether this process can actually create a user namespace — fork +
unshare(CLONE_NEWUSER), mirroring Chromium's ownsandbox::Credentials::CanCreateProcessInNewUserNS(~3 ms measured) — and if so, warn and continue with Chromium's namespace sandbox instead of exiting. Absence ofapparmor_restrict_unprivileged_usernsis deliberately not treated as evidence:kernel.unprivileged_userns_clone=0(Debian, Archlinux-hardened),user.max_user_namespaces=0, kernels withoutCONFIG_USER_NS, and container seccomp policy all break userns without that sysctl.The exit arm now prints the exact
sudo chown/chmodrecovery command.Why this is safe
Chromium consults the setuid helper only when the userns sandbox is unavailable, and when neither is available it aborts (
LOG(FATAL)insetuid_sandbox_host.cc, exit 133) rather than running unsandboxed. So this change cannot cause an unsandboxed launch; the worst case of a wrong host check is a crash, never a silent sandbox drop. The setuid helper and the userns sandbox are two implementations of the same layer-1 isolation, not two layers.Unchanged: the
--no-sandboxfallback for AppArmor-restricted hosts (54ea059), theELECTRON_DISABLE_SANDBOX=1override (pinned by a new test), root behavior, and the missing/symlink-helper rejection.Behavior matrix
--no-sandboxfallbacklinux-hardened/ Debianunprivileged_userns_clone=0/ containerOne intentional interactive-path change: on userns-capable hosts, declining the sudo prompt in a terminal launch now warns and continues instead of hard-failing, so the helper may stay unprovisioned. Acceptable — the namespace sandbox provides the same layer-1 isolation.
Testing
tests/hermes_cli/test_gui_command.py: 34 passed, 5 skipped (26/5 before, with zero tests reaching the changed branch)..desktopExec path launched detached with no TTY, sudo failed exactly as in the rofi case, and the app started with sandboxed zygote children whilechrome-sandboxwas user-owned0755. One host confirms the mechanism, not a host class — hence the probe.History
--no-sandboxfallback