Improper Electron Security Practices (CSP)

[night]

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.

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

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

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

4. 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]

I wonder what's with the sudden reach out?

ref:

• powercord-org/powercord#386
• AryToNeX/Glasscord#56
• joe27g/EnhancedDiscord#143
• leovoel/BeautifulDiscord#94

[ObserverOfTime]

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

[cookie1599]

what happening ?

[Inve1951]

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.

[intrnl]

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]

The label is from the issue template.

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

[SpicyTakis]

😂

[zerebos]

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

[ObserverOfTime]

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]

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

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

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]

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]

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 Githubissues.
• Githubissues is a development platform for aggregating issues.