Closed Bug 1567623 Opened 1 year ago Closed 1 year ago

Update AssertEvalNotUsingSystemPrincipal to account for in-the-wild behavior

Categories

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

enhancement

Tracking

()

RESOLVED FIXED
mozilla70
Tracking Status
firefox70 --- fixed

People

(Reporter: tjr, Assigned: tjr)

References

(Blocks 1 open bug)

Details

(Whiteboard: [domsecurity-active])

Attachments

(5 files)

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'

Status: NEW → ASSIGNED
Priority: -- → P1
Whiteboard: [domsecurity-active]

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

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 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+
Attachment #9081006 - Flags: data-review?(chutten) → data-review+

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

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

Depends on: 1570212

Pushed by archaeopteryx@coole-files.de:
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

Keywords: checkin-needed

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.

Flags: needinfo?(tom)

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

You need to log in before you can comment on or make changes to this bug.