Skip to content
This repository has been archived by the owner on Jun 27, 2021. It is now read-only.

Improper Electron Security Practices #143

Open
night opened this issue Sep 7, 2020 · 3 comments
Open

Improper Electron Security Practices #143

night opened this issue Sep 7, 2020 · 3 comments
Labels
category: core Something to do with loading plugins or pre-included features help wanted If you have the time, pitch in and make a pull request. Otherwise it'll take a long time for this. priority: low Not as important compared to other issues or enhancements. status: delayed Don't have the time/resources to implement now. will be worked on, but not in the near future. type: tweak A suggestion to edit something that is neither a bug nor a feature request

Comments

@night
Copy link

night commented Sep 7, 2020

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

  1. Remote Module Enabled
        originalOptions.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.

  1. Context Isolation Disabled
        originalOptions.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.

  1. Content Security Policy (CSP) Disabled
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});
});

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.

@joe27g
Copy link
Owner

joe27g commented Sep 7, 2020

This may all be true, but what other choice do we have? If Discord cared to make an easy way for client mods to inject, or just plugin/theme support in general, this wouldn't be a problem. But by fighting client mods, they give users the choice to allow the Discord app to have a fraction of its usefulness or have reduced security. Not to mention client mods have to use the same injection methods as malware, which hurts our reputation and blurs the line between good and bad Discord modifications. I can't tell y'all what to do, but if you really care about client mod users' security, you could just add official support for some of the things we've had to awkwardly patch in for years.

@joe27g joe27g added category: core Something to do with loading plugins or pre-included features help wanted If you have the time, pitch in and make a pull request. Otherwise it'll take a long time for this. priority: low Not as important compared to other issues or enhancements. status: delayed Don't have the time/resources to implement now. will be worked on, but not in the near future. type: tweak A suggestion to edit something that is neither a bug nor a feature request labels Sep 8, 2020
@joe27g
Copy link
Owner

joe27g commented Sep 8, 2020

Note: I don't mean to be hostile to you (night) or any other Discord employees, I was just trying to point out that it's a bit ironic to give suggestions to projects that you generally don't support/advocate against using. I just assumed that the reason for creating this issue was to call out client mods as being insecure, but I understand now you were just trying to help. No hard feelings.

Anyway, as for the issues themselves, I'm not interested in fixing them for a few reasons:

  • These are mostly carried over from BD, so I don't remember why exactly each of these options were changed.
  • The CSP is probably the most likely to be abused, so I'd love to just re-enable it and prevent any non-Discord remote scripts from loading, but that would break themes and many BD plugins.
  • Reverting these changes or implementing alternative solutions would require testing with many different plugins, including BD plugins. I don't have time to figure out what the alternatives are or do said testing, hence the "help wanted" label.

For reference, powercord-org/powercord#386 has more info and discussion about these issues.

@MasicoreLord
Copy link
Contributor

Accounted for and partially fixed by last update, EnhancedDiscord has cut off updates, and all remaining official support will terminate April 12th.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
category: core Something to do with loading plugins or pre-included features help wanted If you have the time, pitch in and make a pull request. Otherwise it'll take a long time for this. priority: low Not as important compared to other issues or enhancements. status: delayed Don't have the time/resources to implement now. will be worked on, but not in the near future. type: tweak A suggestion to edit something that is neither a bug nor a feature request
Projects
None yet
Development

No branches or pull requests

3 participants