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

The SUID sandbox helper binary was found, but is not configured correctly #17972

Closed
christianbundy opened this issue Apr 25, 2019 · 104 comments

Comments

@christianbundy
Copy link

commented Apr 25, 2019

Preflight Checklist

  • I have read the Contributing Guidelines for this project.
  • I agree to follow the Code of Conduct that this project adheres to.
  • I have searched the issue tracker for an issue that matches the one I want to file, without success.

Issue Details

  • Electron Version:
    • 5.0.0
  • Operating System:
    • Arch Linux x64
  • Last Known Working Electron version::
    • 4.1.5

Expected Behavior

Running node_modules/.bin/electron --version should output v5.0.0.

To be clear, all commands create this error, but I'm using the --version flag for simplicity.

Actual Behavior

$ node_modules/.bin/electron --version
[2720:0425/142001.775056:FATAL:setuid_sandbox_host.cc(157)] The SUID sandbox helper binary was found, but is not configured correctly. Rather than run without sandboxing I'm aborting now. You need to make sure that /home/christianbundy/src/ssbc/patchwork/node_modules/electron/dist/chrome-sandbox is owned by root and has mode 4755.

Additional Information

$ stat /home/christianbundy/src/ssbc/patchwork/node_modules/electron/dist/chrome-sandbox
  Size: 5185424   	Blocks: 10128      IO Block: 4096   regular file
Device: 802h/2050d	Inode: 1465270     Links: 1
Access: (0755/-rwxr-xr-x)  Uid: ( 1000/christianbundy)   Gid: ( 1000/christianbundy)
Access: 2019-04-25 14:15:10.609279524 -0700
Modify: 2019-04-25 14:15:10.659278929 -0700
Change: 2019-04-25 14:15:10.659278929 -0700
 Birth: 2019-04-25 14:15:10.609279524 -0700

If I chown and chmod the file then it works fine, but my intuition is that npm install electron@latest should work without those commands. Is this expected behavior?

@nornagon

This comment has been minimized.

Copy link
Member

commented Apr 25, 2019

Unfortunately there's no way we can configure this correctly automatically, because setting the appropriate permissions requires root privileges and we don't want to ask for a root password during npm install.

Ideally, Arch would configure its kernel support unprivileged CLONE_NEWUSER and Chromium (and Electron) could use the namespace sandbox instead of relying on the old setuid sandbox. Apps that are distributed on Linux will need to incorporate this into their install process. See electron-userland/electron-installer-debian#184 and electron-userland/electron-installer-redhat#112 for example.

During development, we should probably print something out on npm install though with instructions if we detect that the namespace sandbox isn't available.

@christianbundy

This comment has been minimized.

Copy link
Author

commented Apr 25, 2019

Hey, thanks for the super quick response!

Does unprivileged CLONE_NEWUSER come from CONFIG_USER_NS=y? If so, that should be the current configuration.

Please let me know if there's anything I can do to help, or if this is expected output on Arch I'm happy to close -- I understand that there's some jankiness to be expected when running bleeding-edge distros and I don't mind resolving this in a PKGBUILD rather than expecting it to work perfectly straight out of npm. Cheers!

@nornagon

This comment has been minimized.

Copy link
Member

commented Apr 26, 2019

CONFIG_USER_NS=y enables the user namespaces feature, but they're still restricted to privileged users by default. This suggests sysctl kernel.unprivileged_userns_clone=1

@sofianguy sofianguy added this to Unsorted Issues in 5.0.x Apr 26, 2019

@kitingChris

This comment has been minimized.

Copy link

commented Apr 28, 2019

Is there a possible workaround in the meantime until every linux distro enables those features?

@MarshallOfSound

This comment has been minimized.

Copy link
Member

commented Apr 28, 2019

@kitingChris The setuid sandbox IS the workaround. You just need to ensure that when developing / releasing an electron app your dev / packaging scripts set the permissions of the sandbox helper correctly as @nornagon linked above.

@vladimiry

This comment has been minimized.

Copy link

commented Apr 28, 2019

Is there a possible workaround in the meantime until every linux distro enables those features?

See the original message:

If I chown and chmod the file then it works fine

Also see here #16631 (comment) So to make suid sandbox work you basically have to tweak the chrome-sandbox binary this way:

  • sudo chown root chrome-sandbox
  • chmod 4755 chrome-sandbox

The issue is more severe though if running appimage/snap packages, I have not yet revealed a decent workaround for these cases. It's working if appimage is executed with --no-sandbox arguemnt, but this is a hack.

@vladimiry

This comment has been minimized.

Copy link

commented Apr 28, 2019

@nornagon

because setting the appropriate permissions requires root privileges and we don't want to ask for a root password during npm install.

Executing chmod 4755 node_modules/electron/dist/chrome-sandbox doesn't require root permission and that should be enough to run such command for wrapping the app to deb/pacman/etc packages as when such packages get installed all the files including chrome-sandbox normally owned by root. So it would be great electron does chmod 4755 node_modules/electron/dist/chrome-sandbox automatically during installation process as then there would be no need to handle this case manually like mentioned here.

@vladimiry

This comment has been minimized.

Copy link

commented Apr 28, 2019

CONFIG_USER_NS=y enables the user namespaces feature, but they're still restricted to privileged users by default. This suggests sysctl kernel.unprivileged_userns_clone=1

I confirm executing sudo sysctl kernel.unprivileged_userns_clone=1 is another workaround, related comment.

@burningTyger

This comment has been minimized.

Copy link

commented Apr 28, 2019

@vladimiry I needed to first chown and then chmod. The other way round it didn't work.

@vladimiry

This comment has been minimized.

Copy link

commented Apr 28, 2019

@burningTyger you are right, I just have changed the original message.

@MarshallOfSound

This comment has been minimized.

Copy link
Member

commented Apr 28, 2019

@nornagon If we chmod 4755 out/Release/chrome-sandbox on CI will that permission be persisted once we zip it up or do we have to make this change in electron-download to fix the permission on DL?

vladimiry added a commit to vladimiry/ElectronMail that referenced this issue Apr 29, 2019
@liberodark

This comment has been minimized.

Copy link

commented Apr 29, 2019

same here is bad new fonction for what need to change that... :(

vladimiry added a commit to vladimiry/ElectronMail that referenced this issue Apr 29, 2019
@nornagon

This comment has been minimized.

Copy link
Member

commented Apr 29, 2019

@MarshallOfSound zip does not support permissions, but ultimately the issue is not the chmod permission, but rather the owner. The setuid sandbox helper is suid to root, because it needs to perform functions that are only available to root. If it were possible to set the appropriate permissions without first gaining root privileges, that would be a very serious vulnerability in Linux. Fortunately for Linux, and unfortunately for us, that is not the case.

Here are the options as I see them:

  1. Change nothing in Electron. Recommend that developers enable unprivileged CLONE_USERNS on their kernel to allow the namespace sandbox to run instead of the setuid sandbox.
  2. Boot without sandbox if no sandboxing method is available only when booting an unpackaged app. If Electron is booting a packaged app, refuse to boot without sandbox.
  3. In all cases, if no sandboxing method is available, boot without sandboxing and print a warning.
  4. Disable sandboxing by default on Linux.

I'm leaning towards (2). It would ease development without compromising the security of the deployed app.

I'm not yet sure what to do about snap/flatpak, as I'm not familiar with their workings. It's possible that they already sandbox the app sufficiently that we can disable sandboxing altogether in that situation, as we do when building the Mac App Store version of Electron.

@nornagon nornagon self-assigned this Apr 29, 2019

@vladimiry

This comment has been minimized.

Copy link

commented Apr 29, 2019

As for now, I like more the first option.

  1. Boot without sandbox if no sandboxing method is available only when booting an unpackaged app. If Electron is booting a packaged app, refuse to boot without sandbox.

Such scenario would be somehow misleading. One might be successfully running unpackaged app without sandboxing, but there is a chance that the packaged app won't work the same way with enabled sandboxing. Like, for example, the case when you access the main process from the renderer process not through the remote interface. Or the case of wrapping the app to AppImage / Snap / Flatpak packages.

  1. In all cases, if no sandboxing method is available, boot without sandboxing and print a warning.

So a packaged app that was designed and developed as sandboxed might be executed without sandbox if no sandbox available. This doesn't sound good to me.

  1. Disable sandboxing by default on Linux.

What does it mean exactly? What would be the way to enable sandboxing on Linux in the way the app either starts or fails if no sandboxing available (the current situation, forced sandboxing).

@nornagon

This comment has been minimized.

Copy link
Member

commented Apr 29, 2019

I also like (1), but to defend (2) a little, the API exposed to the app wouldn't change when disabling the sandbox. The only thing we would disable is the OS-level sandbox. We would still load the app the same way, it just wouldn't be protected by the OS.

@vladimiry

This comment has been minimized.

Copy link

commented Apr 29, 2019

We would still load the app the same way, it just wouldn't be protected by the OS.

Then it sounds good to me too. Thanks for the explanation.

@cstayyab cstayyab referenced this issue Apr 30, 2019
@pronebird

This comment has been minimized.

Copy link

commented Aug 30, 2019

@nornagon so we can't even run Electron 6 on Debian and there is no way we're giving a chrome process root perms. The workarounds given are not sufficient from the user perspective.

There are two workarounds:
Enable unprivileged access to CLONE_NEWUSER in your kernel. Some kernels support changing this with sysctl kernel.unprivileged_userns_clone=1.

There is absolutely no way messing with the customers computer is the way to go about this.

Disable sandboxing entirely by launching with --no-sandbox. Adding this argument from JS is unfortunately insufficient, as the GPU process is launched before the main process JS is run.

How about adding a --sandbox flag and turn off the sandbox feature by default? This has a benefit of not screwing 99% of folks using Electron.

@vladimiry

This comment has been minimized.

Copy link

commented Aug 30, 2019

The workarounds given are not sufficient from the user perspective.

Using loader-like script/program which will add --no-sandbox arg can also be considered as a workaround, so no end-user actions needed. Besides such loader could test user namespace support first and add --no-sandbox only if required.

@nornagon

This comment has been minimized.

Copy link
Member

commented Aug 30, 2019

@pronebird giving the chrome-sandbox binary setuid root is much less dangerous than running Electron without a sandbox. Consider: you're the one shipping the Electron binaries, so you can feel confident you're shipping trusted code (e.g. by signing it). However, if your app can be convinced to load untrusted code (possibly intentionally, e.g. by navigating to a URL on the web), then the app will happily execute that code with the user's permission, without any kind of sandbox. I'd be much more concerned about defense against attacks focused on running untrusted JS in an unsandboxed process in your app than I would be about attacks involving exploiting a bug in the same sandboxing system that Chrome uses. (And I'd expect the latter to get fixed very quickly if they were discovered.) If you want to audit the code of the chrome-sandbox binary yourself, you can start here: https://chromium.googlesource.com/chromium/src/+/refs/heads/master/sandbox/linux/suid/sandbox.c#424

If you're committed to running without an Electron sandbox, I'd highly recommend sandboxing some other way, e.g. snapcraft or flatpak, packagers for which I believe include launcher scripts that disable Electron's sandbox inside the container.

@pronebird

This comment has been minimized.

Copy link

commented Aug 30, 2019

@nornagon

giving the chrome-sandbox binary setuid root is much less dangerous than running Electron without a sandbox. Consider: you're the one shipping the Electron binaries, so you can feel confident you're shipping trusted code (e.g. by signing it). However, if your app can be convinced to load untrusted code (possibly intentionally, e.g. by navigating to a URL on the web), then the app will happily execute that code with the user's permission, without any kind of sandbox. I'd be much more concerned about defense against attacks focused on running untrusted JS in an unsandboxed process in your app than I would be about attacks involving exploiting a bug in the same sandboxing system that Chrome uses. (And I'd expect the latter to get fixed very quickly if they were discovered.)

Sure, but running a sandbox makes little sense if you have an app that never loads any remote content. I believe that has to be the case for Electron – for building desktop apps using web stack, loading maybe JSON from the web, but not like just showing web pages like we don't have Safari for that. We're past the times when folks used to wrap websites in phone-gap, aren't we?

I think the overhead is way too much. I've added a bootstrap script today into our app, and removed the chrome-sandbox binary from distribution, which was another 5 megs. Unfortunately electron-builder doesn't support any of this, so we had to add a hook and do it ourselves .

If you want to audit the code of the chrome-sandbox binary yourself, you can start here: https://chromium.googlesource.com/chromium/src/+/refs/heads/master/sandbox/linux/suid/sandbox.c#424

Thanks. It really depends on a threat model, and there is no way we're running anything Google with root permissions.

It can be entirely safe, but It would be also really hard to verify that all sources are intact since electron builds are shipped via npm. We'd have to set up custom build server for Electron and audit the sources ourselves. This would be a tremendous effort and I would rather avoid it.

@MarshallOfSound

This comment has been minimized.

Copy link
Member

commented Aug 30, 2019

@pronebird I'm not gonna get too involved in the sandbox discussion but my stance here is we should have the sandbox enabled by default like we currently do.

It can be entirely safe, but It would be also really hard to verify that all sources are intact since electron builds are shipped via npm

This is fundamentally incorrect, electron builds are not shipped via npm. The electron package on npm is actually about 2 files and ~40 lines of JS. The actual Electron builds are delivered via S3 and have checksums also on S3 so folks can validate that the build they are downloading is actually the build Electron shipped.

@nornagon nornagon referenced this issue Aug 30, 2019
3 of 6 tasks complete
@pronebird

This comment has been minimized.

Copy link

commented Aug 30, 2019

@MarshallOfSound

This is fundamentally incorrect, electron builds are not shipped via npm. The electron package on npm is actually about 2 files and ~40 lines of JS. The actual Electron builds are delivered via S3 and have checksums also on S3 so folks can validate that the build they are downloading is actually the build Electron shipped.

What I meant is, the builds are downloaded during the npm install stage. I didn't mean that they are physically hosted on npm. Let it be they are hosted on S3. Prove me wrong, but I bet that checksums are hosted on the same S3, which lowers the security grade of such system, it's one leg only. But that's not the point. Imagine the scenario where your builds are being tampered and the checksums updated accordingly. Now we have a problem. That's simply a risk and damage control on my side. No root = lower risks of something really bad happening.

@nornagon

This comment has been minimized.

Copy link
Member

commented Aug 30, 2019

@pronebird chrome-sandbox binary definitely shouldn't be 5MB, that's a bug. Opened #20049 to fix it, thanks for pointing it out.

You're certainly welcome to choose to disable sandboxing for your use case. It's unfortunate that Chromium's architecture makes it so that you have to pass --no-sandbox on the command line directly rather than calling app.commandLine.appendSwitch; ideally disabling the sandbox would be possible without necessitating a wrapper script. Hopefully with #20049 the concern of removing the binary will be ameliorated, since 200KB extra is much more reasonable to live with than 5MB.

@gardner

This comment has been minimized.

Copy link

commented Sep 5, 2019

Is there an option for using a system binary? I have reservations about giving a suid root bit to anything in my node_modules/ folder. Thanks.

@vladimiry

This comment has been minimized.

Copy link

commented Sep 5, 2019

@gardner you could pass --no-sandbox arg which is sort of easy workaround when you are in dev mode.

@bpasero

This comment has been minimized.

Copy link
Contributor

commented Sep 18, 2019

VSCode just recently switched to Electron 6 and now we are getting reports that VSCode no longer starts unless --no-sandbox option is provided. What is the way forward here? Refs: microsoft/vscode#81056

@p3x-robot

This comment has been minimized.

Copy link

commented Sep 18, 2019

snap is works now out of the box, for appimage, there is a hard written code:
electron-userland/electron-builder#3872 (comment)

the rest i do not know, i only release NPM, AppImage and SNAP...

@p3x-robot

This comment has been minimized.

Copy link

commented Sep 18, 2019

basically, you have to re-package every type of item to add an argument (--no-sandbox) to the launcher script.

@p3x-robot

This comment has been minimized.

Copy link

commented Sep 18, 2019

there is no solution on Electron 6 I guess, either you have to write it based on your packages or wait for a fix or downgrade...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.