Skip to content
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

Disabled context isolation is a security hole/please enable #986

Open
Thaodan opened this issue Oct 19, 2023 · 24 comments
Open

Disabled context isolation is a security hole/please enable #986

Thaodan opened this issue Oct 19, 2023 · 24 comments
Labels
help wanted Extra attention is needed known issue/workaround Workaround available in the know_issues.md file

Comments

@Thaodan
Copy link

Thaodan commented Oct 19, 2023

Describe the bug
For #434 and further #492 context isolation was disabled. This is a security issue as it exposes
all electron api to websites running inside Electron.

If I understand correctly one has to implement contextBridges to be able to use ipcRender which
is required for WebRTC.
I've linked a few documentation pages around this below.

Desktop :

  • OS: any
  • Installation package: any
  • Version: >1.0.0.17

Additional context

@IsmaelMartinez
Copy link
Owner

I am sorry but currently I don't have time to look at this in more detail. It does look quite complex to re-place unfortunately.

@IsmaelMartinez
Copy link
Owner

I had a look and it does look a few changes are needed but it should be hopefully double. It might take me a few tries and might break some functionality but I will try 😄 . Thanks for reporting

@IsmaelMartinez
Copy link
Owner

Ok, this is going to be a much bigger task. It will require to re-work all the parts in the UI. Ideally we can get rid of that one, nodeIntegration and the sandbox, but is a mamouth task.

Electron has deprecated the BrowserWindow so I will try to start the work to move to the BaseWindow and WebContentView, but it might take some time.

In the meantime, I will make this values as config options so people can disable them. They will break functionality for sure, but at least you can still use the app. Not sure when I will have the time to make those changes (the enable them as flags) but hopefully soon!

Thanks for reporting again.

@IsmaelMartinez
Copy link
Owner

by the way, we do disable eval so that should, in theory, block most of the bad actors. But agree better to make sure we only expose what we have to.

@IsmaelMartinez
Copy link
Owner

IsmaelMartinez commented Jul 8, 2024

1.8.0 have the ability to disable the contextIsolation and sandboxing in the main BrowserWindow. Currently not doing this until is thoroughly tested by multiple users.

@Thaodan , are you able to test this and see what functionality you see missing? Difficult for me to test from a MacOS it looks like most things are working (all that I tried, but I haven't tried things like custom backgrounds)

Thanks!

@IsmaelMartinez
Copy link
Owner

another option included is running the app with firejail https://github.com/IsmaelMartinez/teams-for-linux/blob/develop/README.md#running-teams-for-linux-in-a-firejail

contextIsolation and sandbox seem to be breaking screensharing. To fix that we would need to re-architect the app (something that would happen over time, but would require a lot of work).

@IsmaelMartinez IsmaelMartinez added the known issue/workaround Workaround available in the know_issues.md file label Aug 23, 2024
@nfp0
Copy link

nfp0 commented Sep 6, 2024

How severe is this hole? Could you please give an example of what a bad actor could do in this case?

@IsmaelMartinez
Copy link
Owner

In theory you could use some of the electron APIs, but I did try to see if I could exploit it and couldn't get much of it, but I am no security expert.

You can disable context isolation etc, there is a config option. That would break screen sharing, but the rest should still work.

I wasn't able to see the "exposed" APIs anywhere, what I assume is that you need to still expose them, something we don't do.

If you are concerned, use firejail, or snap/flathub, that do run on their own context.

I have been trying to re-write all that area, but the logo trademark, and other user issues/requests has kept me busy. Happy to help anyone interested in fixing this. Otherwise it will need to wait until I get time.

@Thaodan
Copy link
Author

Thaodan commented Sep 6, 2024 via email

@IsmaelMartinez
Copy link
Owner

Aye, we do get monitored by snyc, and github, so if there is a security issue in one of the libraries I will get alarms. (anyone is welcome to subscribe to them, you just need to go to the snyc link in the project)

Copy link

This issue is stale because it has been open 30 days with no activity. Remove stale label or comment or this will be closed in 5 days.

@github-actions github-actions bot added the Stale label Oct 16, 2024
Copy link

This issue was closed because it has been stalled for 5 days with no activity.

@github-actions github-actions bot closed this as not planned Won't fix, can't repro, duplicate, stale Oct 21, 2024
@Thaodan
Copy link
Author

Thaodan commented Oct 23, 2024 via email

@IsmaelMartinez
Copy link
Owner

I would disagree on the stale bot. It might need some tuning but it does help keep the list manageable. It did close a few issues where people then indicated it was now solved, when I was still looking for an answer a couple of weeks ago.

Also, that this was closed by the stale bot doesn't mean I can reopen it. Also, I haven't forgotten about it, it is just a dropped priority as there are workarounds and it is a fairly complex change and I need someone to help testing.

Are you able to do tests running the app with the options I added a few months ago? I need to understand what parts break to see what I need to fix. Thanks!

@IsmaelMartinez IsmaelMartinez added help wanted Extra attention is needed and removed Stale labels Oct 24, 2024
@IsmaelMartinez
Copy link
Owner

This is blocked until we move away from BrowserView (now deprecated) to webcontentsview.

A few other changes are needed before doing this including removing the, I believe, unused electron-remote Library. I am looking into it

@IsmaelMartinez
Copy link
Owner

IsmaelMartinez commented Nov 15, 2024

Hi @Thaodan, I have now removed the deprecation that was blocking for on this area, and removed the disabling of context isolation for the streamSelector window.

As this is a fairly big change to the whole of the front en part of the application, I prefer to split it into small releases when possible, to avoid breaking everything.

Would you be able to test the pre-release 1.12.0 and confirm that the screensharing works? Thanks in advance and apologies it took longer than I hoped for.

@Thaodan
Copy link
Author

Thaodan commented Nov 16, 2024 via email

@cpfeiffer
Copy link

I tried and was successful. Screensharing worked fine with 1.12.0 pre-release.

@IsmaelMartinez
Copy link
Owner

Thanks! That validates it will work, I just now need to make this change in a billion places, but at least now that contextIsonlation is only applying at loading time inside the preload script, meaning risk is much lower (it was already fairly low)

@IsmaelMartinez
Copy link
Owner

hi @Thaodan and @cpfeiffer , do you use the customBackgrounds? I think those are now broken for this changes. Added further changes in this case to the login form (that I believe is not in use) in 1.12.1.

I started making changes to the rest of the app and, OMG, as I suspected a lot of it needs re-written.

As such, I am going to start a teams-v2 version and remove the v1 logic (as add lots of complexity and MS is stopping support mid next year). Hopefully I am fully ready by then with that new version with some heavy refactoring.

@Thaodan
Copy link
Author

Thaodan commented Nov 25, 2024 via email

@cpfeiffer
Copy link

I tried with custom backgrounds and that worked as well.

@IsmaelMartinez
Copy link
Owner

I have been trying for a few weeks to move the app to not use contentIsolation and I am hitting dead ends. This is mainly because we don't own the url, as it is an external URL we open.

I will check but I think on that case there isn't a problem as the external URL will NOT have access to the electron options.

Sharing to keep this up to date. I have re-written most but can't get it to work because I can safely inject scripts into the microsoft page. I might be able to do it on a dirty way by lowering the security of the origins etc, and then execute scripts, but that will be really fragile and need to execute in any page reload, what is normally quite fragile.

@Thaodan
Copy link
Author

Thaodan commented Dec 15, 2024 via email

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
help wanted Extra attention is needed known issue/workaround Workaround available in the know_issues.md file
Projects
None yet
Development

No branches or pull requests

4 participants