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

feat: enable setuid sandbox on linux #17269

Merged
merged 1 commit into from Mar 8, 2019
Merged

feat: enable setuid sandbox on linux #17269

merged 1 commit into from Mar 8, 2019

Conversation

nornagon
Copy link
Member

@nornagon nornagon commented Mar 7, 2019

Description of Change

This change enables the setuid sandbox on Linux. This allows Electron to run sandboxed in environments that disable CLONE_NEWUSER for unprivileged users (e.g. docker without CAP_SYS_ADMIN, and Arch Linux).

Closes #16631.

Checklist

Release Notes

Notes: Enabled the setuid sandbox on Linux, allowing Electron to launch sandboxed processes in environments that disable CLONE_NEWUSER for unprivileged users.

@electron-cation electron-cation bot added the new-pr 🌱 PR opened in the last 24 hours label Mar 7, 2019
@nornagon
Copy link
Member Author

nornagon commented Mar 7, 2019

This is a draft PR because it doesn't yet address the distribution side of this change. Specifically, in order to work correctly, the chrome_sandbox executable must be:

  • named chrome-sandbox, in the same directory as the electron executable,
  • owned by root, and
  • have permission mask 4755 set.

If the above conditions are not met, the sandbox will not function. Chrome solves this problem during installation: the deb file distribution has those permissions encoded. We'll need to do something similar for both npm installed versions of Electron and versions that are packaged for distribution.

@nornagon
Copy link
Member Author

nornagon commented Mar 7, 2019

@nornagon
Copy link
Member Author

nornagon commented Mar 7, 2019

@nornagon nornagon marked this pull request as ready for review March 7, 2019 22:51
@nornagon nornagon requested review from a team and zcbenz March 7, 2019 22:53
@nornagon
Copy link
Member Author

nornagon commented Mar 7, 2019

Requesting review from @electron/wg-security and @zcbenz

Copy link
Member

@MarshallOfSound MarshallOfSound left a comment

Choose a reason for hiding this comment

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

Is it possible to support renaming of this binary to something static like electron_sandbox or even dynamic and based on the current binaries name?

E.g. myapp_sandbox

Not a deal breaker and this is literally just aesthetics

Copy link
Member

@deepak1556 deepak1556 left a comment

Choose a reason for hiding this comment

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

LGTM on build config changes.

@deepak1556
Copy link
Member

Is it possible to support renaming of this binary to something static like electron_sandbox

Its hardcoded in upstream SetuidSandboxHost::GetSandboxBinaryPath(), will be a patch.

@nornagon
Copy link
Member Author

nornagon commented Mar 7, 2019

it would be relatively easy but questionably valuable to rename the binary to something like electron-sandbox. it would be an unreasonable amount of work to make it dynamic.

@MarshallOfSound
Copy link
Member

If it requires a patch to make it electron and too much work + patches to make it dynamic then I'm 👍 on this change. Worth asking the question 😄

@electron-cation electron-cation bot removed the new-pr 🌱 PR opened in the last 24 hours label Mar 8, 2019
@nornagon nornagon merged commit 652e232 into master Mar 8, 2019
@release-clerk
Copy link

release-clerk bot commented Mar 8, 2019

Release Notes Persisted

Enabled the setuid sandbox on Linux, allowing Electron to launch sandboxed processes in environments that disable CLONE_NEWUSER for unprivileged users.

@nornagon nornagon deleted the enable-suid-sandbox branch March 8, 2019 21:10
@malept
Copy link
Member

malept commented Mar 11, 2019

Can we get this backported to 5-0-x?

@nornagon
Copy link
Member Author

/trop run backport-to 5-0-x

@trop
Copy link
Contributor

trop bot commented Mar 11, 2019

The backport process for this PR has been manually initiated,
sending your 1's and 0's to "5-0-x" here we go! :D

@trop
Copy link
Contributor

trop bot commented Mar 11, 2019

I have automatically backported this PR to "5-0-x", please check out #17343

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.

None yet

4 participants