Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(linux): Don't setuid chrome-sandbox when not required #8368

Merged
merged 2 commits into from
Jul 22, 2024

Conversation

pimterry
Copy link
Contributor

Fix #7545 (already closed but as stale, not actually fixed)

Setuid for chrome-sandbox is not needed on the vast majority of systems where user namespaces are available, as this more secure mechanism is used for sandboxing instead. Root setuid binaries like this are a general security risk, and appear to cause specific problems in various cases today (e.g. making a deb package unusable on Ubuntu 24.04: httptoolkit/httptoolkit#602, making rpm packages fail validation on Fedora: bitwarden/clients#5153)

User namespaces were implemented in 2013 (kernel 3.8), and have been enabled by default widely for many years. Chrome themselves stopped setting this in 2016, describing it as unnecessary on all supported Linux platforms: https://issues.chromium.org/issues/40462640.

The only other case I'm aware of where setuid is required despite user namespace support is Electron <5. This electron version has been end-of-life since 2019 (more than 4 years ago) so hopefully that's not a concern, but if Electron Builder supports very old electron versions then this may be a breaking change.

This PR checks whether they're supported (i.e. /proc/self/ns/user is a symlink) and briefly tests that they work correctly (running true in a separate user namespace), and then sets the setuid bit only if that fails. This is the same test approach as used to detect sandboxing support in Nix: https://github.com/NixOS/nix/blob/40f80e1b5cf2bebb6d1d8c9efac1d982a540d555/tests/functional/common/vars-and-functions.sh#L180-L182

Copy link

changeset-bot bot commented Jul 22, 2024

🦋 Changeset detected

Latest commit: b7ed5d6

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 8 packages
Name Type
app-builder-lib Patch
dmg-builder Patch
electron-builder-squirrel-windows Patch
electron-builder Patch
electron-forge-maker-appimage Patch
electron-forge-maker-nsis-web Patch
electron-forge-maker-nsis Patch
electron-forge-maker-snap Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

Copy link

netlify bot commented Jul 22, 2024

Deploy Preview for car-park-attendant-cleat-11576 failed.

Name Link
🔨 Latest commit 2c890cd
🔍 Latest deploy log https://app.netlify.com/sites/car-park-attendant-cleat-11576/deploys/669e8abfc4fa1d0008e4292d

@pimterry
Copy link
Contributor Author

I've patched the various commit/changeset warnings here, but also I've just discovered the wider underlying cause of these recent issues: electron/electron#41066. All Electron apps are effectively broken on Ubuntu 24.04 right now due to related user namespacing restrictions, and it may be that while actually skipping this setuid is useful, since that would imply the test here will fail in those environments, this doesn't actually solve my problem... I'll keep digging there.

This isn't a blocker for this though, so this may still be useful to explore and/or merge here regardless, since setuid really shouldn't be required here anyway! I strongly suspect Ubuntu will revert this change from 24.04 (Mint and others have already done so) so it'll work there too soon.

@mmaietta mmaietta changed the title Don't setuid chrome-sandbox when not required fix(linux): Don't setuid chrome-sandbox when not required Jul 22, 2024
This is not necessary in many environments, so we now test for whether
this is required and then enable it only when necessary.
@mmaietta mmaietta merged commit 2acdf65 into electron-userland:master Jul 22, 2024
13 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Do we really need a setuid root chrome-sandbox for every platform?
2 participants