Improper Electron Security Practices (CSP) #442

night · 2020-09-07T08:26:59Z

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.

intrnl · 2020-09-07T09:13:57Z

I wonder what's with the sudden reach out?

ref:

ObserverOfTime · 2020-09-07T12:32:23Z

If someone finds XSS in Discord, that's Discord's fault.

intrnl · 2020-09-07T15:14:40Z

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.

intrnl · 2020-09-07T15:36:14Z

The label is from the issue template.

I think playing the blame game isn't the right thing to do here, please calm down.

rauenzi · 2020-09-07T15:38:09Z

@cookie1599 @Sagishii @SpicyTakis offtopic comments are unnecessary and clutter the issue. Please refrain from doing this.

ObserverOfTime · 2020-09-07T16:17:16Z

Seriously... If Discord wanted to be at least somewhat secure, they'd install into %ProgramFiles%

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.

Curtis-D · 2020-09-07T16:56:17Z

@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.

kotx · 2020-09-07T17:36:56Z

@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?

night · 2020-09-07T20:22:07Z

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.

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.

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.

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.

Curtis-D · 2020-09-07T22:39:10Z

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

It's a public repo, feel free to contribute

intrnl · 2020-09-07T23:01:11Z

I understand.

While I'm not sure some of these are possible for us to do at the moment, like:

Disabling Node integration, since the beginning our code has been running off a <script> element, there are better ways to go around this while still allowing remote updates perhaps.
Enabling context isolation, tons of work for both the client mod and the plugins ecosystem. I think it may very well can cause a heavy backlash from some people.
Removal of CSP bypass, I think themes will be suffering the most here.

At the very least, this issue was filed with good intent on mind, so thank you.

rauenzi · 2020-09-08T01:24:36Z

First, I want to apologize for some of the more.. antagonistic responses. Their feelings do not reflect my own.

It's unfortunate that you believe security and client mods to be mutually exclusive.

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.

For node integration, we could use our own API/IPC to get modules or equivalent functions to plugins.
The CSP could be modified to allow only certain whitelisted sites as Bowser also pointed out. (In our case we would only allow GitHub and Imgur as that's what we limit our plugins and themes to currently.)
And again as Bowser said, the most significant hurdle would be the context isolation. After bouncing some ideas off of Bowser I had a couple ideas on potential workarounds for the context isolation, however I haven't had a chance to test them out.

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.

ObserverOfTime · 2020-09-10T19:34:47Z

You shouldn't limit img-src to Imgur. There are many (oftentimes better) image hosting services.

Chew · 2020-09-12T02:52:46Z

And you aren't allowed to hotlink imgur anyway outside of forums (according to their ToS anyway, or, what they claim is in it)

rauenzi · 2021-01-27T08:47:13Z

It seems the change night mentioned on the Powercord issue

It's also worth noting that these issues have prompted some internal discussion around Electron security. We are now considering force enabling BrowserWindow security options on the Electron level within the next couple months

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.

ObserverOfTime · 2021-01-27T09:04:47Z

Time for BD to start installing its own version of Electron to replace Discord's. 🤔
(discord_arch_electron already does that using a system-wide Electron installation.)

Tropix126 · 2021-01-27T19:33:10Z

discord/electron@c0575e6

solonovamax · 2021-01-27T20:38:10Z

It's also worth noting that these issues have prompted some internal discussion around Electron security. We are now considering force enabling BrowserWindow security options on the Electron level within the next couple months

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.

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.

zany130 · 2021-03-24T01:51:58Z

stable seems to have implemented this change as well

Tropix126 · 2021-03-24T01:53:17Z

stable seems to have implemented this change as well

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

n5q · 2021-03-24T01:57:01Z

Affects stable as well now, attempting to install through betterdiscordctl will cause discord to greet you with a message saying "your discord installation has continuously failed to update and is now very out of date".

zany130 · 2021-03-24T01:58:07Z

thx seems to be for windows though unless I am mistaken so linux users are out of luck I'll try and see if I can apply the solution to the Linux version, I think the equivalent folder on Linux is ~.local/betterdiscord or something, but I'm assuming if they put a notice that says windows only it won't be so easy

EDIT: NVM idk why I thought the patch had to be placed into the betterdiscord folder its the discord folder that it has to go into

svadrutk · 2021-03-24T02:40:34Z

it's the same for me. I guess I'll just have to wait until the next version of BD releases. Good luck guys

ObserverOfTime · 2021-03-24T16:04:53Z

See #598 (comment)

Tropix126 · 2021-04-01T18:08:03Z

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

rauenzi · 2021-04-02T17:12:47Z

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.

Tropix126 · 2021-05-19T00:42:00Z

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.

night added the bug label Sep 7, 2020

This comment has been minimized.

Sign in to view

This was referenced Sep 7, 2020

Improper Electron Security Practices leovoel/BeautifulDiscord#94

Closed

Improper Electron Security Practices joe27g/EnhancedDiscord#143

Open

This comment has been minimized.

Sign in to view

Juby210 mentioned this issue Sep 9, 2020

"Security Update" from Powercord has broken multiple BD Plugins Juby210/bdCompat#5

Closed

rauenzi added enhancement help wanted and removed bug labels Sep 10, 2020

rauenzi mentioned this issue Sep 10, 2020

Improper Electron Security Practices powercord-org/powercord#386

Open

rauenzi pinned this issue Jan 27, 2021

rauenzi mentioned this issue Feb 1, 2021

Canary Discord Imcompatible with BD. #551

Closed

ObserverOfTime mentioned this issue Feb 14, 2021

Discord Canary now thinks im in the Discord website. bb010g/betterdiscordctl#94

Closed

Tropix126 mentioned this issue Feb 26, 2021

[Bug] BandagedBD incompatible with PTB 77645 (6bbb9a6), interfering with settings #561

Closed

rauenzi mentioned this issue Feb 27, 2021

[Bug] BetterDiscord incompatible with Canary & PTB #548

Closed

ObserverOfTime mentioned this issue Mar 2, 2021

BetterDiscord incompatible with Discord's new security measures (BD does not load) bb010g/betterdiscordctl#96

Closed

Technoblazed mentioned this issue Mar 24, 2021

Discord not showing custom background in settings and breaking microphone settings #596

Closed

rauenzi mentioned this issue Mar 24, 2021

[Bug] BetterDiscord incompatible with latest Discord on Linux. #598

Closed

EmeraldSnorlax mentioned this issue Mar 24, 2021

? bb010g/betterdiscordctl#98

Closed

This was referenced Mar 24, 2021

Can't install BBD on latest discord version. #569

Closed

[Bug] No longer working after restart, "continuously failed to update" #562

Closed

[Bug] Multiple different things crash discord. #541

Closed

Kinue72 mentioned this issue Mar 25, 2021

[Bug] BetterDiscord incompatible with latest Stable on Mac #600

Closed

rauenzi changed the title ~~Improper Electron Security Practices~~ Improper Electron Security Practices (CSP) Apr 2, 2021

rauenzi unpinned this issue Apr 2, 2021

C0D3-M4513R mentioned this issue Apr 3, 2021

[Issue] Account switching not working aarondoet/AccountSwitcher#35

Closed

ghost deleted a comment May 13, 2022

rauenzi added the accepted label Aug 29, 2023

rauenzi linked a pull request Aug 29, 2023 that will close this issue

Re-enable CSP via whitelist #1662

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improper Electron Security Practices (CSP) #442

Improper Electron Security Practices (CSP) #442

night commented Sep 7, 2020

intrnl commented Sep 7, 2020 •

edited

ObserverOfTime commented Sep 7, 2020

This comment has been minimized.

This comment has been minimized.

intrnl commented Sep 7, 2020

intrnl commented Sep 7, 2020 •

edited

This comment has been minimized.

rauenzi commented Sep 7, 2020

ObserverOfTime commented Sep 7, 2020 •

edited

Curtis-D commented Sep 7, 2020

kotx commented Sep 7, 2020 •

edited

night commented Sep 7, 2020

Curtis-D commented Sep 7, 2020 •

edited

intrnl commented Sep 7, 2020 •

edited

rauenzi commented Sep 8, 2020

ObserverOfTime commented Sep 10, 2020

Chew commented Sep 12, 2020

rauenzi commented Jan 27, 2021

ObserverOfTime commented Jan 27, 2021

Tropix126 commented Jan 27, 2021

solonovamax commented Jan 27, 2021 •

edited

zany130 commented Mar 24, 2021

Tropix126 commented Mar 24, 2021 •

edited

n5q commented Mar 24, 2021

zany130 commented Mar 24, 2021 •

edited

svadrutk commented Mar 24, 2021

ObserverOfTime commented Mar 24, 2021

Tropix126 commented Apr 1, 2021

rauenzi commented Apr 2, 2021

Tropix126 commented May 19, 2021

Improper Electron Security Practices (CSP) #442

Improper Electron Security Practices (CSP) #442

Comments

night commented Sep 7, 2020

intrnl commented Sep 7, 2020 • edited

ObserverOfTime commented Sep 7, 2020

This comment has been minimized.

This comment has been minimized.

intrnl commented Sep 7, 2020

intrnl commented Sep 7, 2020 • edited

This comment has been minimized.

rauenzi commented Sep 7, 2020

ObserverOfTime commented Sep 7, 2020 • edited

Curtis-D commented Sep 7, 2020

kotx commented Sep 7, 2020 • edited

night commented Sep 7, 2020

Curtis-D commented Sep 7, 2020 • edited

intrnl commented Sep 7, 2020 • edited

rauenzi commented Sep 8, 2020

ObserverOfTime commented Sep 10, 2020

Chew commented Sep 12, 2020

rauenzi commented Jan 27, 2021

ObserverOfTime commented Jan 27, 2021

Tropix126 commented Jan 27, 2021

solonovamax commented Jan 27, 2021 • edited

zany130 commented Mar 24, 2021

Tropix126 commented Mar 24, 2021 • edited

n5q commented Mar 24, 2021

zany130 commented Mar 24, 2021 • edited

svadrutk commented Mar 24, 2021

ObserverOfTime commented Mar 24, 2021

Tropix126 commented Apr 1, 2021

rauenzi commented Apr 2, 2021

Tropix126 commented May 19, 2021

intrnl commented Sep 7, 2020 •

edited

intrnl commented Sep 7, 2020 •

edited

ObserverOfTime commented Sep 7, 2020 •

edited

kotx commented Sep 7, 2020 •

edited

Curtis-D commented Sep 7, 2020 •

edited

intrnl commented Sep 7, 2020 •

edited

solonovamax commented Jan 27, 2021 •

edited

Tropix126 commented Mar 24, 2021 •

edited

zany130 commented Mar 24, 2021 •

edited