BetterDiscord: Improper Electron Security Practices (CSP)
Upon reviewing this project’s “injector” code, it appears it disables numerous security features implemented by Discord to ensure remote code is sufficiently sandboxed from the operating system. As it stands, this software is a walking remote code execution waiting to happen.
- Node Integration Enabled
options.webPreferences.nodeIntegration = true;
This software leaks node integration into the main window. This means the window has access to directly modify the file system and execute arbitrary commands.
- Remote Module Enabled
options.webPreferences.enableRemoteModule = true;
This software enables Electron’s remote module in the main window. This means the window has access to send direct IPC commands which can be used to execute arbitrary code. The remote module is also being removed in the next version of Electron, so you will have to fix this anyways when that occurs.
- Context Isolation Disabled
options.webPreferences.contextIsolation = false;
This software disables Electron’s context isolation, which forces browser code to run in a separate context from main window code. This prevents attackers from doing things like polluting prototypes which may expose access to restricted functions that escalate access to execute arbitrary commands.
- Content Security Policy (CSP) Disabled
// Remove the CSP
const removeCSP = () => {
electron.session.defaultSession.webRequest.onHeadersReceived(function(details, callback) {
if (!details.responseHeaders["content-security-policy-report-only"] && !details.responseHeaders["content-security-policy"]) return callback({cancel: false});
delete details.responseHeaders["content-security-policy-report-only"];
delete details.responseHeaders["content-security-policy"];
callback({cancel: false, responseHeaders: details.responseHeaders});
});
};
// Remove CSP immediately on linux since they install to discord_desktop_core still
if (process.platform == "win32" || process.platform == "darwin") electron.app.once("ready", removeCSP);
else removeCSP();
CSP exists to mitigate and prevent attacks around most XSS and content injection. If someone finds XSS in Discord, the lack of 1, 2, and 3 listed above would directly result in remote code execution.
Security of Electron is not to be taken lightly as there are many foot-guns. By releasing software like this and encouraging people to install it, you are putting users at risk without taking proper steps to keep Electron secure. I would strongly encourage you to read up on the best security practices for Electron at https://www.electronjs.org/docs/tutorial/security and apply those to this project.
About this issue
- Original URL
- State: open
- Created 4 years ago
- Reactions: 98
- Comments: 30 (15 by maintainers)
If someone finds XSS in Discord, that’s Discord’s fault.
Discord does have improvements planned for downloading and updating, but I just want to highlight that these are completely different attack vectors.
Discord’s app displays a remote website which is being given direct access to remotely execute code by this software. In an example attack scenario, arbitrary user data which somehow gets access to run JavaScript would essentially be a 0-day. To exploit app updates, Discord’s distribution channels would need to be forced to serve an exploit. Given the attack surfaces listed, the former is more likely to occur since user-generated content is accessible from clients and there is much more surface area for attack.
Arguing that all security should go out the window because of us installing to AppData is also a pretty irresponsible way of thinking as a developer. Security is built in layers, with the end goal being that the attack surface area is sufficiently reduced. It is everyone’s job to think about security.
It’s unfortunate that you believe security and client mods to be mutually exclusive. While making a client mod work properly in a sandboxed environment will require some amount of work, the end result here is for the benefit of users. If you have specific questions regarding Electron I can try to offer you direction/advice, but all of the listed security issues are usually solvable problems.
@night Are you writing on behalf of Discord or as a side project developer?
There’s lots Discord can do to help client modifications and make these security problems a non issue. Instead you create GitHub issues across all the popular client mods as if they can work around these problems without major drawbacks.
Maybe if you worked more as a company with client modifications you wouldn’t have to worry about these issues.
It’s a public repo, feel free to contribute
First, I want to apologize for some of the more… antagonistic responses. Their feelings do not reflect my own.
I agree that they should not be mutually exclusive.
The points you’ve brought up are things I’ve considering changing in the past, however this mod started out as a fork of the original BetterDiscord, it’s only no longer marked as a fork by GitHub due to the limitations it was placing on the repo. Because of that, all of those were ingrained in the original design and removing those would cause backwards-incompatible changes for the mod itself as well as plugins and themes.
I feel similar to @Bowser65 that some of it could be changed.
That said, as someone who not only maintains BetterDiscord, but also has contributed to (in some form or another) EnhancedDiscord as well as Powercord, I think it’s a good idea to put some of the more detailed discussion in one spot for us all. I think the best spot (on GitHub) would be the issue you opened on the Powercord repository as you and Bowser have already had some dialogue there and it’s also not as cluttered as this one seems to have become very quickly. Let me know if you can think of a better spot.
As a side note, I have been working on another (potential) mod separate from BD where security would be a bigger part of the mod and these holes would not exist. The injector I’ve been working on for that one complies with all of the above, save for the context isolation. Hopefully soon I’ll be able to test out some of the ideas I mentioned above and provide an update.
@night I find it hard to believe that you, an employee of Discord, are trying to “help” in fixing security issues in applications that break the TOS.
What is your goal here?
You shouldn’t limit img-src to Imgur. There are many (oftentimes better) image hosting services.
After a long time, BetterDiscord now complies with almost all of the items listed, however a better solution for the CSP is in the works. So this issue will stay open to track that.
I wonder what’s with the sudden reach out?
ref:
I think it’s important to state that these security prefs are disabled on purpose to give freedom to plugins and themes.
Sure, we could enable them back on, but that would have to come with major trade-offs that doesn’t seem to be worth doing to an existing client mod.
Seriously… If Discord wanted to be at least somewhat secure, they’d install into
%ProgramFiles%(like Skype does) instead of some random location where literally any application run with normal user privileges can mess with it. Note that Discord’s forced auto-updates at application startup can also inject code at will. The machine is compromised long before any v8 context isolation would happen.I understand.
While I’m not sure some of these are possible for us to do at the moment, like:
At the very least, this issue was filed with good intent on mind, so thank you.
The label is from the issue template.
I think playing the blame game isn’t the right thing to do here, please calm down.
Update: A new cross-platform installer (in beta) is now available that runs safely inside of these new security measures. https://github.com/BetterDiscord/Installer/releases/latest
Not directly related to electron, but as an additional security measure all plugins from our new website are manually reviewed every update. This further decreases the likeliness of malicious code being distributed through official sources, however security still is an issue when it comes to running untrusted plugins.
It’s being rolled out rather slowly. There is a fix that can be pushed if absolutely needed. You might also have luck with this quick patch if you’re on stable.
Link from image: https://cdn.discordapp.com/attachments/410788958277074944/821690118468272168/patch.zip
I’m assuming that’s what just broke the latest discord update from the AUR for me (canary). Was about to open an issue, but I randomly decided to read this first.
I’m just updating this to let people know: my solution has just been to move over to powercord. No offence to BD, but powercord seems MUCH nicer than BD.
Time for BD to start installing its own version of Electron to replace Discord’s. 🤔 (
discord_arch_electronalready does that using a system-wide Electron installation.)It seems the change night mentioned on the Powercord issue
has finally become reality. See this commit here. Discord is now shipping a modified version of Electron to ensure these features are on and cannot be disabled by BD. Currently, this only affects the Canary release channel, but it may come to others soon.
This requires a major re-architecture of BD (and potentially plugins as well) in order to comply and work with this. I can only say I will do what I can when I can. My free time, and frankly interest, to work on BD has been dwindling in recent days. Especially with work taking up more and more of my time.
Discord packages for Linux do install it system-wide (thanks to the package maintainers). Injections are still possible because Discord then installs its modules locally for the user. At least, as a result of the system-wide installation method, (some) auto-updates are disabled.