1523813 - Remove `new Function` from utilityOverlay.js

Status: RESOLVED FIXED

(Core :: DOM: Security, enhancement, P2)

Description

[Jed Davis [:jld] ⟨⏰|UTC-7⟩ ⟦he/him⟧]

STR: with a debug build, middle-click the reload button (duplicate current tab)

This sets off the assertion added in bug 1512949, “do not use eval with system privileges”. The JS stack points to this use of new Function in utilityOverlay.js, to eval a string from what I'm assuming is the XUL DOM.

Comment 1

[Christoph Kerschbaumer [:ckerschb]]

Jonas, can you please take a look at this one? If things turn out to take longer to fix, then probably we just need to whitelist utilityOverlay.js for now and file a follow up bug to investigate.

Comment 2

[Jonas Allmann [:jallmann]]

I will have a look.

Comment 3

[Jonas Allmann [:jallmann]]

This instance of new Function appears in a function called checkForMiddleClick() in

https://searchfox.org/mozilla-central/rev/c07aaf12f13037c1f5a343d31f8291549e57373f/browser/base/content/utilityOverlay.js#597

that handles middle mouse clicks on a number of navigation elements in the browser. E.g. here

https://searchfox.org/mozilla-central/source/browser/base/content/browser.xul#844

There is roughly 20 occurences of checkForMiddleClick() in prominent places around the codebase.
Some buttons (like the home button, in contrast to the other three navigation buttons) appear to handle middle clicks differently.

The use of new Function is also apparently related to the very old bug 246720 that was never fixed.

I'm unsure which is the best way to get rid of this new Function.
Would it invole fixing bug 246720, as commented in the code?
Or could checkForMiddleClick() disappear entirely and be replaced by a different way to handle middle clicks? I don't see an obvious reason why middle clicks are not handled regularly along with other click events.

Could I get some hints on how to tackle this bug?

Comment 4

[:Gijs (he/him)]

The most obvious way of fixing this that I can think of would be to make XULElement.doCommand() take an optional event argument, and have that create the command event with the relevant click event as the source event. In that case though, we'd need to update all the consumers to check the sourceEvent of the command event.

If you aren't set up for C++ builds, you could potentially create and dispatch the new command event "manually" and still do the rest of the work. It will be a bit tedious to update all the consumers of checkForMiddleClick but it should work? Does that sound sensible to you, Jonas? Jared, want to sanity check me?

Off-hand I don't see an obvious other way of fixing this... we shouldn't, ideally, switch to click events given that in principle command events should also be handling keyboard activations after that work is done, and the use of command here helps us ignore other mouse button click events, drag events, etc.

Comment 5

[Jonas Allmann [:jallmann]]

Attachment: Work in progress diff.

I'm still tring to wrap my head around this and how commands work in the first place.

I attached a diff of what I'm doing so far. It seems to work at firt glance, the middle-click funtionality is preserved for the buttons I've tested. But I have a few doubts and my approach seems too easy to be correct.

1. Can I just change the behaviour of the oncommand attribute of these elements without breaking their behaviour when the command is not triggered by checkForMiddleClick() but some other event that is not carrying the correct sourceEvent?

2. Related to the first question: Some elements using checkForMiddleClick() do not have an oncommand attribute themselves but reference a command. Changing this command to use event.sourceEvent instead of event will break correct handling of situations where the command is not triggered by a middle click, am I correct? Example:

<toolbarbutton id="back-button" class="toolbarbutton-1 chromeclass-toolbar-additional"
                       label="&backCmd.label;"
                       removable="false" overflows="false"
                       keepbroadcastattributeswhencustomizing="true"
                       command="Browser:BackOrBackDuplicate"
                       onclick="checkForMiddleClick(this, event);"
                       tooltip="back-button-tooltip"
                       context="backForwardMenu"/>

<command id="Browser:BackOrBackDuplicate" oncommand="BrowserBack(event);" disabled="true">

I assume changing this to

<command id="Browser:BackOrBackDuplicate" oncommand="BrowserBack(event.sourceEvent);" disabled="true">

would break the command's behaviour in situations other than middle-clicks.
Would it be a solution to check if event carries a sourceEvent and use the latter only if present?

Or do I need to adapt each of the funcions called in any of the commands, like BrowserBack() in the example, to handle both types of situations correctly?

I hope I'm making myself clear.

Comment 6

[Jonas Allmann [:jallmann]]

Attachment: Bug 1523813, Remove new Function from utilityOverlay.js, r=gijs

Replaced new Function() by CustomEvent carrying original click event as sourceEvent.
Adapted all oncommand listeners to use event.sourceEvent instead of event.

Comment 7

[:Gijs (he/him)]

(In reply to Jonas Allmann [:jallmann] from comment #5)

Created attachment 9041802 [details] [diff] [review]
Work in progress diff.

I'm still tring to wrap my head around this and how commands work in the first place.

I attached a diff of what I'm doing so far. It seems to work at firt glance, the middle-click funtionality is preserved for the buttons I've tested. But I have a few doubts and my approach seems too easy to be correct.

1. Can I just change the behaviour of the oncommand attribute of these elements without breaking their behaviour when the command is not triggered by checkForMiddleClick() but some other event that is not carrying the correct sourceEvent?

Unfortunately probably no, because then we'll pass null as the event in the other cases, which is likely to break things in some places. It would be better to change the contents of the event handler (perhaps not directly in the XUL markup but in the thing they call, e.g. gContextMenu.reload() implementation in nsContextMenu.js ) to check event.sourceEvent instead of just event. In some cases it might be OK to pass the event as event.sourceEvent || event from the oncommand handler, though that feels a bit hacky.

2. Related to the first question: Some elements using checkForMiddleClick() do not have an oncommand attribute themselves but reference a command. Changing this command to use event.sourceEvent instead of event will break correct handling of situations where the command is not triggered by a middle click, am I correct? Example:

<toolbarbutton id="back-button" class="toolbarbutton-1 chromeclass-toolbar-additional"
                       label="&backCmd.label;"
                       removable="false" overflows="false"
                       keepbroadcastattributeswhencustomizing="true"
                       command="Browser:BackOrBackDuplicate"
                       onclick="checkForMiddleClick(this, event);"
                       tooltip="back-button-tooltip"
                       context="backForwardMenu"/>

<command id="Browser:BackOrBackDuplicate" oncommand="BrowserBack(event);" disabled="true">

I assume changing this to

<command id="Browser:BackOrBackDuplicate" oncommand="BrowserBack(event.sourceEvent);" disabled="true">

would break the command's behaviour in situations other than middle-clicks.
Would it be a solution to check if event carries a sourceEvent and use the latter only if present?

Yeah, so the if condition I suggested would work, but like I said earlier, I think it'd be neater to put that check in the BrowserBack implementation (in this case) than inline in the oncommand handler.

Or do I need to adapt each of the funcions called in any of the commands, like BrowserBack() in the example, to handle both types of situations correctly?

So this, ideally. :-)

Comment 8

[Jared Wein [:jaws] (please needinfo? me)]

I don't think I can give enough time right now to think through the proposal and vet its correctness. I trust Gijs' judgement here, so I'll clear the needinfo. Please needinfo me again if you want me to come back to this and prioritize it higher.

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/56e28681fc9b
Remove new Function from utilityOverlay.js, r=Gijs

Comment 10

[Daniel Varga [:dvarga]]

https://hg.mozilla.org/mozilla-central/rev/56e28681fc9b