no mention in the breaking changes doc of the planned removal of worldSafeExecuteJavaScript

[bughit]

per #24114 it's planned to be removed.

this flag will be turned on by default in Electron 12 (and removed)

This is a breaking change so it ought to be mentioned in the breaking changes doc. Breaking changes should not be casually and quietly introduced, so also some discussion is warranted. Isn't the default change sufficient?

I would like to request that the option be kept, I have code that depends on it, retrieves the main world window. Defines anon functions for later execution.

const contentWin = await webFrame.executeJavaScript('window')
const contentFn = await webFrame.executeJavaScript('() => {...}')

I have no issue with maximally secure defaults, but there shouldn't be a need to completely block this, breaking working code, forcing redesigns. This can be used safely, judiciously. Considering security can be disabled completely, it's not unreasonable to optionally allow EIC to retrieve content world objects, especially functions that EIC itself defines. Please don't remove the choice.

[MarshallOfSound]

worldSafeExecuteJavaScript only takes effect when contextIsolation is enabled. If contextIsolation is disabled then the original behavior will remain.

This removal is not in the breaking changes doc as it isn't done yet. It will land into that doc at some point during the Electron 11 or 12 cycle when the PR goes up to remove it.

There are incredibly good reasons to remove this option. If you want to be insecure (which I don't completely understand why you'd consciously be making that choice) then disable contextIsolation. Having contextIsolation enabled and this option disabled is functionally equivalent to having context isolation disabled. The option will still be removed as originally indicated to fix this security gap.

[bughit]

@nornagon @MarshallOfSound

There are incredibly good reasons to remove this option.

What are they?

If you want to be insecure

Please be concrete:

const contentFn = await webFrame.executeJavaScript('() => {...}')

What is insecure here? This is no less secure than immediately executing the function. I just happen to have a design that separates in time declaration and execution. There's a potential for assigning node objects to the content world prototypes, but that's an intentional and not accidental opening of a hole.

Having contextIsolation enabled and this option disabled is functionally equivalent to having context isolation disabled.

You are now promoting insecurity in the name of security. The status quo with contextIsolation allows me to contain and not leak all actually insecure things in the EIC. My EIC window is full of insecure stuff not meant for content. The EIC code does webFrame.executeJavaScript('() => {...}') without leaking anything to content. So no, disabling contextIsolation is definitly not equivalent and not good advice.

[MarshallOfSound]

What is insecure here?

You said it yourself

"There's a potential for assigning node objects to the content world prototypes, but that's an intentional and not accidental opening of a hole."

That is a massive security hole and completely negates context isolation, sharing objects created in world A over to world B is by definition a security vulnerability.

You are now promoting insecurity in the name of security. The status quo with contextIsolation allows me to contain and not leak all actually insecure things in the EIC.

What you are doing is insecure, you are leaking your entire isolated context whether you think you are or not. Leaking the main world into the EIC or the EIC into the main world doesn't matter, whichever way the leak is it is a security issue. Which is why we are plugging it.

We are promoting security, your app is secure if you have contextIsolation enabled. Having random options to punch holes in context isolation is not secure and we aren't going to make those available to users. Either have your app be secure, or insecure. Those are your options. Anything in the middle is by definition "insecure".

[bughit]

@MarshallOfSound @nornagon

What you are doing is insecure, you are leaking your entire isolated context whether you think you are or not.

You keep asserting this without any explanation. How am I leaking without realizing? If you show it, I will concede and thank you for the trouble.

I said there is potential for me to leak. But I have no intention to do so and so don't. That's called having a choice. I am genuinely trying to understand your POV, so can you please explain what I am leaking with:

const contentFn = await webFrame.executeJavaScript('() => {...}')

To leak anything I would have to assign an EIC object to a content object property, if I don't there's no leak.

[bughit]

@MarshallOfSound @nornagon

Another significant loss I haven't yet underscored is that returning functions injected into the main world allows secure dependency injection of EIC objects into these functions

const jobThatNeedsToRunInContentAndHaveAccessToEicCode = await webFrame.executeJavasScript('eicObj => {... eicObj.doSomething(); ...}');

...

jobThatNeedsToRunInContentAndHaveAccessToEicCode(someEicObj);

• the function thus defined is not accessible to content
• the eicObj passed to it is not accessible to content
• please explain what this is leaking "whether you think you are or not"
• since you want to take it away, what alternative is there to this secure dependency injection?
  • contextBridge is far worse as it's global
  • contextIsolation:false is far worse as it makes it much easier to accidentally leak

[MarshallOfSound]

You keep asserting this without any explanation. How am I leaking without realizing? If you show it, I will concede and thank you for the trouble.

It's not my job to explain things, literally, not my job. We have documentation on Context Isolation, there are public facing documents on Prototype Pollution and blog posts with sample example exploits that demonstrate why sandboxing is needed.

I'm going to make a blanket statement here that is 100% correct.

If a JavaScript object created in World A is accessible, used, or called from World B you have a security vulnerability and at that point it is game over

Whether it's through prototype pollution (the easy way), direct access to internals (the slightly harder way) or some other magic that we haven't seen before (see new tricks constantly) the second one of those objects crosses between contexts, it's over.

const contentFn = await webFrame.executeJavaScript('() => {...}')

Using your example above, contentFn is a leak. That is a function that was created in the main world and you now have access to it (and I assume call it) in the isolated world. You can see this by checking if contentFn.constructor === Function the answer will be false.

I said there is potential for me to leak. But I have no intention to do so and so don't. That's called having a choice. I am genuinely trying to understand your POV, so can you please explain what I am leaking with:

Just to re-iterate again, just the fact that that object is accessible / exists at all in the wrong world is a leak.

Another significant loss I haven't yet underscored is that returning functions injected into the main world allows secure dependency injection of EIC objects into these functions

As outlined above, this is just a security vulnerability, it isn't "secure" at all. In fact this is the opposite of secure.

contextIsolation:false is far worse as it makes it much easier to accidentally leak

Correct, contextIsolation: false is something you should strive to avoid.

contextBridge is far worse as it's global

Incorrect, contextBridge is the way to safely expose an API from the isolated world into the main world. If for whatever reason (there isn't really a good one) you don't want it to be global you can implement some kind of "take" system whereby you have some kind that returns your API but only to the first thing that asks for it.

E.g.

let myAPI = { doThing: () => {} };

contextBridge.exposeInMainWorld('api', {
  take: () => {
    const api = myAPI;
    myAPI = null;
    return api;
  },
});

To be clear this changes nothing but if for whatever reason you want to avoid globals that's how you'd do it 🤷

[bughit]

@MarshallOfSound @nornagon

It's not my job to explain things, literally, not my job.

I am not asking you for a lecture on general security principles. If you make an assertion that a specific example is a security hole the onus is on you to show how, particularly when it's far from obvious. If it's true it should be trivial. You still have not, and instead appeal to unknown magic. Sorry, that's hand-waving.

If a JavaScript object created in World A is accessible, used, or called from World B you have a security vulnerability and at that point it is game over

show me an actual security vulnerability in this very concrete example.

// from EIC
const jobThatNeedsToRunInContentAndHaveAccessToEicCode = await webFrame.executeJavasScript('eicObj => {eicObj.doSomething();}');
jobThatNeedsToRunInContentAndHaveAccessToEicCode({doSomething() {}});

@nornagon @zcbenz I have no problem accepting reality. It's entirely possible that I'm missing something. What is the specific vulnerability in the above code, @MarshallOfSound won't say. I am not seeing how prototype pollution can interfere with this function definition and invocation.

[MarshallOfSound]

You're being unreasonable, I'm not going to type out exploits into a GitHub comment. The good news for the electron ecosystem is there isn't any onus on me to prove anything. This option will be removed as planned and I won't be engaging in this comment thread any longer.

[MarshallOfSound]

But ya know what, before I duck and run, maybe this is a legitimate teachable moment 😄 So here is a minimalistic exploit on code very similar to what you posted. Specifically didn't demonstrate the generic exploit rather reconfigured your code to allow for a targeted one.

Please take it from me that prototype pollution is a massive threat and what you are doing is making contextIsolation pointless. contextBridge is the only way to do what you want to do safely

[bughit]

So here is a minimalistic exploit on code very similar to what you posted.

This exploit requires exposing EIC objects to untrusted code. This is something which untrusted code can't force, it is under your control, and it can be avoided. My example did not do that, and there is no inherent vulnerability in it, you literally have to insert bad code into it to demonstrate a security hole.

You could have just as easily done this:

webFrame.executeJavasScript('eicObj => {window.eicObj = eicObj;}');

You've demonstrated what was obvious from the start, exposing privileged objects to untrusted code is bad. And I never claimed otherwise, only that it's possible to do useful things without doing so.

[bughit]

I'll grant that it's obviously possible to slip up if you're not explicitly thinking and guarding against exposing privileged objects to untrusted code. Still I'd say balancing these considerations should be up to the dev, and it's sufficient to have maximally secure defaults.

[bughit]

@nornagon @zcbenz

Nothing posted by @MarshallOfSound here falsifies my point, which is, it is possible to do this safely:

// from EIC
const jobThatNeedsToRunInContentAndHaveAccessToEicCode = await webFrame.executeJavasScript('eicObj => {eicObj.doSomething();}');
jobThatNeedsToRunInContentAndHaveAccessToEicCode({doSomething() {}});

The above code does not expose privileged EIC objects to untrusted code and does not have a security hole. So why do you insist on taking away this capability as an opt-in, which is a breaking change? If doing this always resulted in a vulnerability, then of course, but it does not.