1567623 - Update AssertEvalNotUsingSystemPrincipal to account for in-the-wild behavior

Status: RESOLVED FIXED

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

Description

[Tom Ritter [:tjr] (OOTO until 4/30 at least)]

Our brief enabling (Bug 1564527) of this (subsequently disabled in Bug 1567499) showed us maybe situations where this gets triggered in the wild. We should update it to account for:

• Not applying to Thunderbird
• Not applying if general.config.filename is set [0]
• Not applying to shield studies or similar [1]
• If someone enters it into the Browser Toolbox or similar. [2]

We also saw a single instance of a chrome:// url that corresponded to a legacy extension. The particular instance also had userChromeJS involved so our theory is that it was some crazy userChromeJS hackery.

We should also modify the behavior so that it doesn't crash the browser and instead does something better... Sending telemetry with no (or better sanitized) data is an option. As is refusing to execute the JS but otherwise continuing execution.

[0] This is a super scary pref that basically says "Go execute this js file in the system context" and it in turn can load and execute more files. It is used by userChromeJS

[1] I'm not sure the extent of this. Maybe we need to exclude all privileged Mozilla extensions. For sure the shield studies include some code (that may come from webpack) that triggers this:

try {
    // This works if eval is allowed (see CSP)
    g = g || Function("return this")() || (1,eval)("this");
} catch(e) {
    // This works if the window reference is available
    if(typeof window === "object")
        g = window;
}

I wonder if we can allowlist those particular strings....

[2] All of these may have the source string of 'debugger'

Comment 1

[Tom Ritter [:tjr] (OOTO until 4/30 at least)]

Attachment: data-review.txt

Comment 2

[Tom Ritter [:tjr] (OOTO until 4/30 at least)]

Attachment: Bug 1567623 - Change doContentSecurityCheck logging from Debug to Verbose r?ckerschb

They are too frequent and too verbose to see any other logging statements.

Comment 3

[Tom Ritter [:tjr] (OOTO until 4/30 at least)]

Attachment: Bug 1567623 - Rename whitelist to allowlist r?ckerschb

Depends on D39555

Comment 4

[Tom Ritter [:tjr] (OOTO until 4/30 at least)]

Attachment: Bug 1567623 - Update AssertEvalNotUsingSystemPrincipal and re-enable it r?ckerschb

We now correctly handle the following cases:

• Thunderbird
• the Browser Toolbox/Console
• Two safe and common idioms
• when general.config.filename is set and userChromeJS does shenanigans

We also change the function to only crash in Debug mode, and for Release channels
we report diagnostic information in a way that does not reveal user data.

Depends on D39556

Comment 5

[Tom Ritter [:tjr] (OOTO until 4/30 at least)]

Attachment: Bug 1567623 - Add Event Telemetry for cases where eval is triggered as System Principal r?chutten

Depends on D39557

Comment 6

[Chris H-C :chutten]

Comment on attachment 9081006 [details]
data-review.txt

PRELIMINARY NOTE:

I recommend putting an expiry on this collection to start with. Maybe 5 versions or so. That way we can re-evaluate in six months whether we still want to monitor it permanently.

DATA COLLECTION REVIEW RESPONSE:

    Is there or will there be documentation that describes the schema for the ultimate data set available publicly, complete and accurate?

Yes. This collection is Telemetry so is documented in its definitions file [Events.yaml](https://hg.mozilla.org/mozilla-central/file/tip/toolkit/components/telemetry/Events.yaml) and the [Probe Dictionary](https://telemetry.mozilla.org/probe-dictionary/).

    Is there a control mechanism that allows the user to turn the data collection on and off?

Yes. This collection is Telemetry so can be controlled through Firefox's Preferences.

    If the request is for permanent data collection, is there someone who will monitor the data over time?

Yes,  Christoph Kerschbaumer is responsible.

    Using the category system of data types on the Mozilla wiki, what collection type of data do the requested measurements fall under?

Category 1, Technical.

    Is the data collection request for default-on or default-off?

Default on for all channels.

    Does the instrumentation include the addition of any new identifiers?

No.

    Is the data collection covered by the existing Firefox privacy notice?

Yes.

    Does there need to be a check-in in the future to determine whether to renew the data?

No. This collection is permanent.

---
Result: datareview+

Comment 7

[Chris H-C :chutten]

Also: I trust that the sanitization methods have been covered liberally with unit tests.

Comment 8

[Tom Ritter [:tjr] (OOTO until 4/30 at least)]

Bug 1570212 will require a rebase so I'll just wait until that lands....

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/d6ad25df5422
Change doContentSecurityCheck logging from Debug to Verbose r=ckerschb
https://hg.mozilla.org/integration/autoland/rev/a245a37bd7fa
Rename whitelist to allowlist r=ckerschb
https://hg.mozilla.org/integration/autoland/rev/e76733f6668b
Update AssertEvalNotUsingSystemPrincipal and re-enable it r=ckerschb
https://hg.mozilla.org/integration/autoland/rev/66cc044a4df2
Add Event Telemetry for cases where eval is triggered as System Principal r=chutten,bzbarsky,tcampbell

Comment 10

[Cosmin Sabou [:CosminS]]

https://hg.mozilla.org/mozilla-central/rev/d6ad25df5422
https://hg.mozilla.org/mozilla-central/rev/a245a37bd7fa
https://hg.mozilla.org/mozilla-central/rev/e76733f6668b
https://hg.mozilla.org/mozilla-central/rev/66cc044a4df2

Comment 11

[Mathew Hodson]

This exception was re-added by this bug.

// The following files are NOT supposed to stay on this whitelist.
// Bug numbers indicate planned removal of each file.

// Bug 1498560
NS_LITERAL_CSTRING("chrome://global/content/bindings/autocomplete.xml"),

Was that intentional? The mentioned bug is marked fixed.

Comment 12

[Tom Ritter [:tjr] (OOTO until 4/30 at least)]

No. I will file a bug and fix it next week (unless you want to :) )