Disabled context isolation is a security hole/please enable

[Thaodan]

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

• Related issue: Can't share screen on Ubuntu #492
• Commit in question: 6e8782b
• Electron on Context Isolation
• Electron on contextBridge
• WebRTC-hid-SDK on Context Isolation

[IsmaelMartinez]

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]

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]

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]

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]

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]

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

[nfp0]

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

[IsmaelMartinez]

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]

From what I understand it is not directly about an attacker to the app directly but any attacker or bug that attacks the teams web app. However since the chromium sandbox is still active it is not as bad.

[IsmaelMartinez]

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)

[github-actions]

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]

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

[Thaodan]

No idea what to write here. It's an ongoing issue the whole stale bot thing is just pointless here and only sabotage's the issue tracking.

[IsmaelMartinez]

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]

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]

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]

IsmaelMartinez ***@***.***> writes:

Would you be able to test the [pre-release 1.12.0](https://github.com/IsmaelMartinez/teams-for-linux/releases/tag/v1.12.0) and confirm that the screensharing works? Thanks in advance and apologies it took longer than I hoped for.

I can try next week.

[cpfeiffer]

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

[IsmaelMartinez]

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]

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]

I don't use custom backgrounds no.

[cpfeiffer]

I tried with custom backgrounds and that worked as well.

[IsmaelMartinez]

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.