-
-
Notifications
You must be signed in to change notification settings - Fork 972
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Improper Electron Security Practices (CSP) #442
Comments
I wonder what's with the sudden reach out? ref: |
If someone finds XSS in Discord, that's Discord's fault. |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
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. |
The label is from the issue template. I think playing the blame game isn't the right thing to do here, please calm down. |
This comment has been minimized.
This comment has been minimized.
@cookie1599 @Sagishii @SpicyTakis offtopic comments are unnecessary and clutter the issue. Please refrain from doing this. |
Discord packages for Linux do install it system-wide (thanks to the package maintainers). |
@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. |
@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? |
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. |
It's a public repo, feel free to contribute |
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. |
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. |
You shouldn't limit img-src to Imgur. There are many (oftentimes better) image hosting services. |
And you aren't allowed to hotlink imgur anyway outside of forums (according to their ToS anyway, or, what they claim is in it) |
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. |
Time for BD to start installing its own version of Electron to replace Discord's. 🤔 |
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. |
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 |
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". |
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 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 |
it's the same for me. I guess I'll just have to wait until the next version of BD releases. Good luck guys |
See #598 (comment) |
Update: A new cross-platform installer (in beta) is now available that runs safely inside of these new security measures. |
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. |
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. |
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.
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.
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.
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.
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.
The text was updated successfully, but these errors were encountered: