Remove --no-sandbox from Linux builds for better security

[zx2c4]

Contributor checklist:

• My contribution is not related to translations. Please submit translation changes via our Signal Desktop Transifex project.
• My commits are in nice logical chunks with good commit messages
• My changes are rebased on the latest development branch
• A yarn ready run passes successfully (more about tests here)
• My changes are ready to be shipped to users

Description

Remove --no-sandbox from Linux builds for better security

Including `--no-sandbox` explicitly reduces security and is very much
not recommended by upstream Electron project. This commit removes that
part of the patch.

Some folks will experience issues because their kernels don't have user
namespaces and because that patch also disables the suid sandbox. You
have two options for that: shift the burden to package managers to
enable the suid bit on the suid sandbox, so that ones without user
namespaces can choose to do that, or simply reenable it.

Either way, shipping with --no-sandbox is irresponsible.

CC @trevp-signal

[scottnonnenberg-signal]

Thanks for reaching out. This is a complex topic.

We believe most of the benefits from the Chromium sandbox would depend on us also creating sandboxed renderers as described here: https://www.electronjs.org/docs/api/sandbox-option

This sandboxing of renderer processes seems much less useful in an Electron app than in a web browser, and would involve a great deal of effort, potential performance and deployment problems, and new security risks.

This sandboxing is less useful for a typical Electron app because the renderers should not be handling untrusted web content. Of course, media exploits or XSS are possible – sandboxing would still have value – but the attack surface is much reduced compared to untrusted web content. The documentation (above) discusses this sandbox as intended for untrusted content.

Using the sandbox outside of this context would add implementation complexity and performance cost as we'd have to re-architect the app to move Node operations from the renderer process into the main process, adding IPC latency. Also, from the documentation: "Another difference is that sandboxed renderers don't modify any of the default JavaScript APIs."

So turning on this feature would cause lots of differences in the renderer environment which would require significant re-architecture, re-implementation, and testing.

Perhaps you're raising this now because of Slack's recent announcement on using the sandbox? Note that the Slack Electron app is essentially a wrapper around their web app, so it fits into a "browser" model more easily than most custom-built Electron apps.

Regarding security risks:

• There have been security problems caused by Electron's sandbox feature. Not long ago the will-navigate event was not fired when in sandbox mode (will-navigate event not fired when in sandbox mode electron/electron#8841). The issue was reproducible in every Electron version since the introduction of the sandbox flag (in v1.4.2) up to Electron v8.0. Because of this, implementations relying on will-navigate to abort navigation for security reasons would have been insecure. The current version of Electron documentation (above) still describes sandboxing as an “experimental feature”. I see they may be updating this text soon, but we still have concerns about the degree to which this has seen real-world testing.

• On some Linux distributions we would have to use suid, asking users to lower the security of their system to install our software (and adding installation/update complexity). On other platforms we'd be relying on exposing user namespaces to unprivileged users, which exposes more kernel attack surface to these users (e.g. https://lwn.net/Articles/673597/).

Of course, we will continue to monitor this situation, to see whether this feature develops to a point where the benefits outweigh the costs and risks.

If we misunderstood your suggestion or its benefits feel free to explain further. We do believe there is much more involved here than just changing a command-line argument.

[zx2c4]

This sandboxing is less useful for a typical Electron app because the renderers should not be handling untrusted web content. Of course, media exploits or XSS are possible – sandboxing would still have value – but the attack surface is much reduced compared to untrusted web content. The documentation (above) discusses this sandbox as intended for untrusted content.

Actually, it turns out that this is the crux of the issue. The concern is that XSS-->RCE, or mediadecoder-->RCE, or that {otherrendererbug}-->RCE. I think your claim about "the attack surface is much reduced compared to untrusted web content" isn't uniformly the case for many of the threat models Signal cares about. One big difference is that Signal messages are directed toward individuals, trivially (and exploit payloads are private thanks to signal's crypto, which is sort of nice from attacker perspective). This means that adversaries can target individuals without the need for sophisticated phishing or network redirection mechanisms. For many, that's a big deal.

And, if you admit that XSS is a real potential coding mistake that every web developer makes from time to time, it's not clear to me that Signal's model is much different from a browser. If you don't admit that XSS is a potential coding issue, because of strict practices or regular audits or something, then you must still admit that the media decoding alone is quite a bit of attack surface, and a surface with a storied history of bugs. Why would you not want to sandbox that?

With regards to your concerns about application rearchitecting, that indeed might be so, and I believe you when you say it's more involved than just flipping off the command line switch. Nonetheless, moving access to Node APIs to outside of the renderer process seems like a very very good thing to do. You mentioned performance, but "performance" has clearly never been a top design goal of this application (see: choice of technology used, endless bug reports about slow loading, ...), and getting fundamental security down seems like it'd trump any speculative performance hits, which might not even be that bad in the end. In other words, if I say, "security" and you say "but our architecture!", it might be a good opportunity to rethink the architecture. That mentality obviously doesn't apply for all projects, but I would think it would apply to one like Signal, considering that security is basically its one and only selling point.

You mentioned the issue with SUID sandbox vs userns sandbox: how about you let distributions decide on this the way they always have? Remember: Chrome/Chromium ships in basically every distro now, and those packagers decide whether they want to enable user namespaces or set the SUID bit. That's the current state of the art in Linux sandboxing APIs today. It includes this choice. Different distros choose in different ways. Complaining that it's unclear which tradeoff is worse is not actually a justification for choosing neither. For, an escape from Signal would be far worse than whatever LPE issue you have in mind, which already would require escaping from a sandboxed process. Besides, there are more interesting ways to do LPE on Linux than attacking the Chrome suid sandbox. So, I'm not sure that's a super valid complaint, or one that you're in a position to make. Leave it to the distros to work out, and for Linux upstream to figure out how to improve. But don't deprive us of sandboxing because you're unimpressed with its current deployment complexities on Linux.

Perhaps you're raising this now because of Slack's recent announcement on using the sandbox?

No, actually. I noticed a different scarier electron app wasn't using the sandbox, and mentioned it to somebody saying "good thing Signal doesn't disable it anymore," and was swiftly corrected, motivating me to send in this PR.

[F30]

Nonetheless, moving access to Node APIs to outside of the renderer process seems like a very very good thing to do. You mentioned performance, but "performance" has clearly never been a top design goal of this application (see: choice of technology used, endless bug reports about slow loading, ...), and getting fundamental security down seems like it'd trump any speculative performance hits, which might not even be that bad in the end.

It might be worth noting that VSCode is in the process of moving to a Node.js-free, sandboxed renderer over the course of 2021: microsoft/vscode#92164. If they can do it without performance being a show-stopper, I'm tempted to think it would be possible for Signal.

[stale]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

[zx2c4]

The patch in this PR is clearly wrong, but the discussion might be worth considering. Also relates to #2375. CC @indutny-signal

[stale]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

[zx2c4]

Please don't close.