-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
Please stop packaging the Desktop client with a setuid root chrome-sandbox #5153
Comments
Hi there, This has been escalated for further investigation. If you have more information that can help us, please add it below. Thanks! |
Hi @jbillingredhat, We use the default installation template for electron-builder, and we've recently upgraded electron builder to the latest version. However it seems they still |
I think that the electron-builder package tries to solve the problem for all distributions. RHEL8 and later have User namespaces enabled by default, as well as Fedora. There's no need to have a setuid root executable. A lot of the electron-builder issues assume debian-specific kernel sysctls too, so it is unlikely that they've even considered running on any other platform. It doesn't sound like you want to deviate from electron-builder, so I'll just recommend we not use this package. |
While it is possible to diverge from the default electron-builder settings to do so would require us to investigate the issue further and make certain there are no degraded experiences on any environments using the Would you be willing to open an upstream issue with electron-builder? That would be a good confirmation that we can safely change it on our side or if we can resolve it upstream. Can you expand a bit deeper into how this breaks rpm integrity validation? |
I opened electron-userland/electron-builder#7545. I hope it has enough information, I'm not deeply familiar with how electron-builder works. |
Reposting from the electron-builder issue. I took a look at it and might have a recommendation. I'm not familiar enough with linux distros to test this myself, I just found related code via stackoverflow To test this solution, you'd want to copy the default
Pulled the code from https://stackoverflow.com/a/40155124 I don't know enough linux to be able to assist much here, but happy to open a PR for it the change above works for you. You'd allocate the new
|
I opened #9105 to report that running
After reading https://ubuntu.com/blog/ubuntu-23-10-restricted-unprivileged-user-namespaces, it seems that the workaround is to create a file
And then run The only annoying part will be keeping the executable path up-to-date with the latest version of Bitwarden, but this could be scripted. |
@jtherrmann Unfortunately it doesn't work for me on Ubuntu 24.04 UPDATE: see EDIT below. It actually works, but
It seems that since Electron 5, Relevant
Until now I was using the "workaround" of enabling User Namespaces. EDIT 1: actually it was caused by some The answer was in
So the workaround for that is:
After that I can confirm that @jtherrmann Solution works fine. EDIT 2: Updated
|
Steps To Reproduce
Check out the RPM scripts for the latest (as of today) RPM:
$ rpm -qp --scripts https://github.com/bitwarden/clients/releases/download/desktop-v2023.3.1/Bitwarden-2023.3.1-x86_64.rpm
Expected Result
An RPM scriptlet that doesn't run
chmod 4755 '/opt/Bitwarden/chrome-sandbox'
Actual Result
Screenshots or Videos
No response
Additional Context
Electron has long since stopped needing a setuid root chrome sandbox. The AppImage doesn't appear to have a setuid root chrome-sandbox. I believe its only electron 5 and less that needed it, but it still warns about it. Please see:
https://bugs.chromium.org/p/chromium/issues/detail?id=598454
I also checked the .deb file, and I see a very similar script that sets chrome-sandbox as 4755 in the deb postinst script.
Removing the setuid bits on the RPM will still permit the package to run, and also changing the permissions on a packaged RPM causes it to fail RPM integrity verification, which will show up on some security scans. It really doesn't make sense to do this for all RPM and DEB distros just to support an out of date method of setting up namespaces.
Operating System
Linux
Operating System Version
Fedora Linux 37 x86_64
Installation method
Other
Build Version
2023.3.1
Issue Tracking Info
The text was updated successfully, but these errors were encountered: