Improper Electron Security Practices

[night]

Upon reviewing this project's injection 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

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

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

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

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.

[cyyynthia]

Hello,

Although this sudden reach out from a Discord employee feels weird, I've done a first quick investigation about the points you brought up.

Enabling Node integration was a heritage from previous mods out there, and it seems to not cause any severe issue if missing. I'll give it a more in-depth look and disable it once I've confirmed it doesn't cause any troubles and/or mitigated the possible issues rising from this.

Remote was forcefully re-enabled because we still have logic depending on it, although we've been working on deprecating this and moving to IPC calls. See #337 & the comment just above the line where we enable remote. We can't remove it just yet as some logic still needs to be moved away from this.

For context isolation the major issue I've encountered is accessing the webpackJsonp global. This is the starting point of any meaningful injection, and not accessing it makes the whole client useless. Unless someone can come up with a decent alternative to get it, we're required to disable this to make the client function. As it is right now, plugins run in the preload script and don't have access to the main world. In some way I think it's a good thing as the vulnerable untrusted side of things can't access plugin' logic and use them as an entry point to do evil, but I might be wrong.

You also didn't catch it in your investigation it seems but we also remove CSP headers. Some people proposed to only whitelist a few domains for theme developers to use external resources without using Discord as a storage bucket, and I'm fine with this. We'll make the change probably at the same time as disabling the Node integration, and make sure to only whitelist those extra domains for resource fetching only (no script-src, to prevent remote scripts from being used this way).

Lastly, I don't know if you'll monitor replies or actually reply but I'm curious: is there any particular reason for the sudden reach out to all client mod developers about potential RCEs? It feels weird to have a Discord employee coming and pointing out security issues out of nowhere (or even give public attention to client mods at all).

[night]

For context isolation the major issue I've encountered is accessing the webpackJsonp global. This is the starting point of any meaningful injection, and not accessing it makes the whole client useless. Unless someone can come up with a decent alternative to get it, we're required to disable this to make the client function. As it is right now, plugins run in the preload script and don't have access to the main world. In some way I think it's a good thing as the vulnerable untrusted side of things can't access plugin' logic and use them as an entry point to do evil, but I might be wrong.

To my knowledge the only access between contexts you can currently have is via executeJavascript as well as the new (experimental) contextBridge. webpackJsonp is still defined in the window, but you will need to executeJavascript in that context to play with it. Typically to make things work in context isolation you need to retool to working with some form of limited, secure IPC between the two contexts.

You also didn't catch it in your investigation it seems but we also remove CSP headers. Some people proposed to only whitelist a few domains for theme developers to use external resources without using Discord as a storage bucket, and I'm fine with this. We'll make the change probably at the same time as disabling the Node integration, and make sure to only whitelist those extra domains for resource fetching only (no script-src, to prevent remote scripts from being used this way).

If users can upload arbitrary, unreviewed styles to a random bucket then it becomes a bit of security theater at that point as far as style CSP rules go. With that being said, it is certainly better than CSP being completely disabled outright (since as you said, no script-src).

Lastly, I don't know if you'll monitor replies or actually reply but I'm curious: is there any particular reason for the sudden reach out to all client mod developers about potential RCEs? It feels weird to have a Discord employee coming and pointing out security issues out of nowhere (or even give public attention to client mods at all).

Everyone always assumes ill intent, for whatever reason. I have an obvious interest in client mods, and while companies will never support them I still am an advocate for them. Someone mentioned to me about Discord mods being insecure, and out of curiosity I decided to take a deeper look.

[cyyynthia]

To my knowledge the only access between contexts you can currently have is via executeJavascript as well as the new (experimental) contextBridge. webpackJsonp is still defined in the window, but you will need to executeJavascript in that context to play with it. Typically to make things work in context isolation you need to retool to working with some form of limited, secure IPC between the two contexts.

I've figured this much so far, however it remains an important problem for how things works, not only for the injection. For example, we make use of Flux for managing settings data and various other Discord internals. We also render a lot of Discord's own React components and I really can't figure out a way of properly injecting there (and that's a problem since React components is where tons of injections are). We can through executeJavaScript inject into those, I have a basic idea on how to pass data back and forth, but they imply to be sync for anything React-related and this'd probably perform super badly.

The problem is that we try to be super close to Discord internals and to re-use Discord's code to our advantage (or libs, like Flux). This context wall makes it way, way more difficult to achieve this. While the world is social distancing, I want to be closer to Discord 😢. Jokes aside, the context isolation is not only affecting the injection but how things are fundamentally handled, which is a bummer. The ideal would be a way to have a reverse exposeInMainWorld to request a global from the other side, but it doesn't seem to be a thing (or I missed it).

If users can upload arbitrary, unreviewed styles to a random bucket then it becomes a bit of security theater at that point as far as style CSP rules go. With that being said, it is certainly better than CSP being completely disabled outright

I don't think importing unreviewed styles is a super big deal. CSS can make the client unusable for sure but it can't generate any other dangerous effects. Restricting resource fetching to some domains will already most likely hit some themes out there, but I'm unsure if disabling remote themes altogether is good either. Powercord doesn't recommend making use of these for fully featured themes, but it can be handy in some scenarios such as snippets (and we have some snippets being shared that way, less than plain css but Discord's 2k chars limit is annoying at times).

Everyone always assumes ill intent, for whatever reason. I have an obvious interest in client mods, and while companies will never support them I still am an advocate for them. Someone mentioned to me about Discord mods being insecure, and out of curiosity I decided to take a deeper look.

I mentioned BetterTTV when we were discussing about why the sudden reach out in private channels. That's good to hear people from the other side are friendly towards client mods.

I'll try to fix both Node integration and CSP really soon (although releases to v2 branch are on hold because we have some updater injections which can go wild, trying to stick around when Discord updates. Should be good but we don't want to break people's installations too much so we're waiting for a host update to see if it blows up or not). For remote we have little code remaining that still relies on this, so it should not be too much of a pain to update. As for the context isolation, there's too much unsolved issues for now to re-enable it or even draft some plans. I'll experiment with it and see if I can pull out some magic tricks out of my hat.

[sylveon]

If users can upload arbitrary, unreviewed styles to a random bucket then it becomes a bit of security theater at that point as far as style CSP rules go. With that being said, it is certainly better than CSP being completely disabled outright (since as you said, no script-src).

I'm no expert in this domain, but perhaps it would be possible to allow users to manually whitelist domains, with an appropriate warning?

[Tropix126]

I'm no expert in this domain, but perhaps it would be possible to allow users to manually whitelist domains, with an appropriate warning?

Might not be a bad idea.

[cyyynthia]

Updates on this regard so far: node integration has been disabled in commit 204d21c, so node integration is officially no more for everyone (both v2 and v2-dev users). As we run inside the preload we still have access to Node APIs anyway. The only issue I've found was that it prevented the use of require within devtools; for now I implemented a mock require function which only allows requiring specific modules (powercord's and electron). This is temporary until I can get a proper fix. (Demo: https://user-images.githubusercontent.com/9999055/92621600-4a2a0100-f2c4-11ea-9974-351cab9348b6.png)

CSP is still removed on both branches. I started making a list of domains we'll be whitelisting, and I'll probably implement, under advanced options, the ability to adding custom domains to the whitelist (for adding own hosts for wallpapers, or whatever people will use this for). Once I get to enabling CSP, it'll first be Report-Only, so we can evaluate if the whitelist needs tweaking or eventually warn theme developers about the policies that are about to get enforced without breaking everything down immediately. This shouldn't last super long and just last a few days (or I may even skip it completely and just throw in actual CSP, will depend on my level of laziness at the moment).

Now regarding context isolation. I've been tinkering with them for a bit and while executeJavaScript works good enough to interact with webpackJsonp, it still causes major issues as of how to handle the context separation while keeping our deep level of integration with Discord internals without compromising the isolation either. I have some plans and I've been exploring ideas such as stub objects, proxies and such but I quickly figured I won't be able to get amazing results without mixing different techniques and do some compromises. It's super tricky to inject into there without creating a massive hole that'd render the context isolation inefficient.

Though, since most (if not all) interactions with Discord internals are done through Powercord-provided methods, we can hide the awful logic there and have little to no change for plugin developers. If everything goes according to plan, plugins should experience no significant impact as the change of internal architecture (or at least I'll try my best to minimize it). It's certainly tricky and will require a lot of effort to get everything to work with context isolation, but should be definitely feasible. I'll continue to tinker with this see if I can get promising results.

[zerebos]

I did some of the testing I talked about on BetterDiscord/BetterDiscord#442 where the majority of the mod (BD in my case) was webpacked, read in as a file, and thrown over the wall using executeJavaScript which surprisingly worked alright. But due to the way BD is structured, this isn't ideal because it would still require node integration being enabled.

@Bowser65 and I had a discussion last night about potential ways to work around or with having context isolation enabled. And he proposed some tactics specifically revolving around accessing webpackJsonp as well as some other specifics like react components and flux which prevent problems due to not being able to pass classes and Symbols using either the bridge or executeJavaScript.

After some back and forth, Bowser proposed "actually doing things properly" meaning truly separating the main process and renderer code and only communicating between them for specific actions using an api exposed through the context bridge. I think this is a good way to do things, and we already discussed some higher level details including implications for our plugins and mods. For Zipcord, the separate mod I mentioned, I can include this type of architecture from the ground up for both the mod and the future plugins. I'm sure Powercord will be able to adapt to this in time. BD and the plugins will be the biggest issue, it will take a significant effort to make not only BD compliant, but the plugins developed for it as well.

That said, while I like this direction, it will take time to work out more details and design as well as of course the implementation.

[night]

Now regarding context isolation. I've been tinkering with them for a bit and while executeJavaScript works good enough to interact with webpackJsonp, it still causes major issues as of how to handle the context separation while keeping our deep level of integration with Discord internals without compromising the isolation either. I have some plans and I've been exploring ideas such as stub objects, proxies and such but I quickly figured I won't be able to get amazing results without mixing different techniques and do some compromises. It's super tricky to inject into there without creating a massive hole that'd render the context isolation inefficient.

Only you know the extent of the userland requirements, but isolation itself cannot be compromised if it's enabled as you can only expose to the main world using exposeInMainWorld. Proxies cannot be used through this to my knowledge, though any function you exposed is already a read-only reference.

Insecurity in context isolation involves exposing APIs which compromise security of the sandbox. In other words, if you have a function to save a file and you allow it to be saved anywhere, then that's exploitable. If you allow the window to execute arbitrary commands/scripts in the isolated context, not just shell commands, then that's exploitable. It's also important not to run user-generated content/scripts in the isolated context.

If you look how DiscordNative is setup, that might give you some ideas for how to restructure your APIs.

I did some of the testing I talked about on BetterDiscord/BetterDiscord#442 where the majority of the mod (BD in my case) was webpacked, read in as a file, and thrown over the wall using executeJavaScript which surprisingly worked alright. But due to the way BD is structured, this isn't ideal because it would still require node integration being enabled.

Generally you want potentially unsafe, remote code to live only in the renderer within the normal window context. Throwing it over the wall seems ideal for running user generated scripts. For native level calls (like file system access, arbitrary http access, script execution access, etc) you will want restricted APIs to perform those specific tasks. So the initial idea of yours I think is what you want, but you will wanna remove that requirement of node integration and move those native pieces out to an API you exposeInMainWorld.

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. This should provide enough time to get updates in order, and prevent others from disabling these important security features in the future.

[cyyynthia]

Generally you want potentially unsafe, remote code to live only in the renderer within the normal window context. Throwing it over the wall seems ideal for running user generated scripts. For native level calls (like file system access, arbitrary http access, script execution access, etc) you will want restricted APIs to perform those specific tasks. So the initial idea of yours I think is what you want, but you will wanna remove that requirement of node integration and move those native pieces out to an API you exposeInMainWorld.

We still need to tackle some aspects on what APIs to provide, and how to design them to be safe from being exploited (since for example some plugins do API calls to specific APIs we can't handle case-by-case). I'm thinking about doing something similar to web browsers and have plugins specify which URLs they may call so we can whitelist only what's necessary. That's the only major additional requirement compared to what was planned in #337.

Overall while discussing with @rauenzi we roughly drafted everything we'll need to deal with regarding getting around the context isolation, and I've started some work in that regard. It'll definitely cut some flexibility and will make some plugins way more difficult to implement, but they are specific enough to justify this. Plus we can maybe provide additional APIs in the future if we see a high enough demand.

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.

I'll do my best to land the security updates as soon as possible to comply with those upcoming policies in time. Huge thanks for the heads up, at least it won't be a surprise update breaking the whole modding ecosystem with no solutions on hand. Appreciate it!