1582512 - Restrict Javascript loads in the parent process, initially logging Telemetry

Status: RESOLVED FIXED

(Firefox :: Security, enhancement)

Description

[Tom Ritter [:tjr] (ni? for response to CVE/sec-approval/advisories/etc)]

This bug is going to make use of the callback from Bug 1577280 to examine all javascript loads in the parent process; and if they are not otherwise allowed - send a Telemetry Event.

The allowlist is still being developed, but will include chrome:// and resource://

We will need to disable enforcement in many tests that we have.

Comment 1

[Tom Ritter [:tjr] (ni? for response to CVE/sec-approval/advisories/etc)]

Attachment: Bug 1582512 - Register a ScriptValidationCallback to examine script loads in the parent process r?Gijs

If we don't recognize something; send a Telemetry Event so we can figure out where it
comes from and refactor the code.

Comment 2

[Tom Ritter [:tjr] (ni? for response to CVE/sec-approval/advisories/etc)]

Attachment: Bug 1582512 - Disable the javascript filename load restriction on specific tests r?jdescottes

Certain tests load data: URIs, blob: URIs, or otherwise do unusual things
that mean we need to disable the restriction for them.

Depends on D46500

Comment 3

[Tom Ritter [:tjr] (ni? for response to CVE/sec-approval/advisories/etc)]

Okay, while I'm not done, I'm pretty close so I'm ready to get a first review.

Comment 4

[Tom Ritter [:tjr] (ni? for response to CVE/sec-approval/advisories/etc)]

Attachment: Bug 1582512 - Disable javascript filename validation on all entry points from xpcshell r?jandem

Depends on D46501

Comment 5

[Tom Ritter [:tjr] (ni? for response to CVE/sec-approval/advisories/etc)]

Attachment: Bug 1582512 - Switch test-actor-registry.js to use a chrome:// URL r?jdescottes

Depends on D46501

Comment 6

[Tom Ritter [:tjr] (ni? for response to CVE/sec-approval/advisories/etc)]

Attachment: Bug 1582512 - Disable script filename restrictions in Marionette r?jandem

Depends on D51336

Comment 7

[Tom Ritter [:tjr] (ni? for response to CVE/sec-approval/advisories/etc)]

Attachment: Bug 1582512 - Rename FilenameType to FilenameTypeAndDetails r?Gijs

Depends on D46500

Comment 8

[Tom Ritter [:tjr] (ni? for response to CVE/sec-approval/advisories/etc)]

Attachment: Bug 1582512 - Rename FilenameToEvalType to FilenameToFilenameType r?Gijs

Depends on D53231

Comment 9

[Tom Ritter [:tjr] (ni? for response to CVE/sec-approval/advisories/etc)]

Attachment: Bug 1582512 - Record a Telemetry Event if we receive a request to load a script filename we aren't expecting r?chutten

While we're here, we also extend the collection period for evalUsage, as it is taking longer to deploy than expected.

Depends on D53232

Comment 10

[Tom Ritter [:tjr] (ni? for response to CVE/sec-approval/advisories/etc)]

Attachment: Bug 1582512 - Disable script filename validation in Browser Toolbox console debugging r?jdescottes

Depends on D46501

Comment 11

[Tom Ritter [:tjr] (ni? for response to CVE/sec-approval/advisories/etc)]

Attachment: data-review.txt

Comment 12

[Tom Ritter [:tjr] (ni? for response to CVE/sec-approval/advisories/etc)]

Attachment: Bug 1582512 - Add in exception cases where we disable the javascript load restrictions r?jkt

Comment 13

[Chris H-C :chutten|PTO (back Nov 3)]

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

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?

No. This collection will expire in Firefox 77.

    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?

Yes. :tjr is responsible for renewing or removing the collection before it expires in Firefox 77.

---
Result: datareview+

Comment 14

[Tom Ritter [:tjr] (ni? for response to CVE/sec-approval/advisories/etc)]

Try run free of reports: https://treeherder.mozilla.org/#/jobs?repo=try&revision=65ddd3129dcc5eb4afc14197688cda672ed36ce0&selectedJob=276486673

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/2787043f1fab
Register a ScriptValidationCallback to examine script loads in the parent process r=Gijs,ckerschb
https://hg.mozilla.org/integration/autoland/rev/1e0dd57b8041
Rename FilenameType to FilenameTypeAndDetails r=Gijs,ckerschb
https://hg.mozilla.org/integration/autoland/rev/bf81985c33b7
Rename FilenameToEvalType to FilenameToFilenameType r=Gijs,ckerschb
https://hg.mozilla.org/integration/autoland/rev/379318a35b20
Record a Telemetry Event if we receive a request to load a script filename we aren't expecting r=chutten
https://hg.mozilla.org/integration/autoland/rev/5fb3e489c31f
Disable javascript filename validation on all entry points from xpcshell r=jandem
https://hg.mozilla.org/integration/autoland/rev/40a0f6c6cd61
Disable script filename restrictions in Marionette r=jandem,marionette-reviewers,whimboo
https://hg.mozilla.org/integration/autoland/rev/47045fa2ffd2
Switch test-actor-registry.js to use a chrome:// URL r=jdescottes
https://hg.mozilla.org/integration/autoland/rev/232d5735d404
Disable the javascript filename load restriction on specific tests r=jdescottes
https://hg.mozilla.org/integration/autoland/rev/fa91b085eb59
Disable script filename validation in Browser Toolbox console debugging r=jimb

Comment 16

[Tom Ritter [:tjr] (ni? for response to CVE/sec-approval/advisories/etc)]

Attachment: Bug 1582512 - Fixup: correct filemode

Comment 17

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/07d3631e609a
Add in exception cases where we disable the javascript load restrictions r=ckerschb
https://hg.mozilla.org/integration/autoland/rev/8b850fd66bf5
Fixup: correct filemode

Comment 18

[Cosmin Sabou [:CosminS]]

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&resultStatus=testfailed%2Cbusted%2Cexception&revision=fa91b085eb59a57d0a00068ea9695cbf92264f47&selectedJob=277195411

Failure logs: https://treeherder.mozilla.org/logviewer.html#?job_id=277195411&repo=autoland

This started to permafail: https://treeherder.mozilla.org/#/jobs?repo=autoland&resultStatus=success%2Ctestfailed%2Cbusted%2Cexception&tochange=e2ffc40a763fdce74279c6e6d631212bf0d47e87&searchStr=Windows%2C7%2CMinGW%2Copt%2CMochitests%2Ctest-windows7-32-mingwclang%2Fopt-mochitest-gpu-e10s%2CM%28gpu%29&fromchange=c08a99db4c1ace6de3705d819210807b98cc773e&selectedJob=277203169
https://treeherder.mozilla.org/logviewer.html#?job_id=277203169&repo=autoland

Backout link: https://hg.mozilla.org/integration/autoland/rev/03c0b53410a6ed897408a374a712a5b275ff8e13

[task 2019-11-20T16:06:02.181Z] 16:06:02 INFO - 5:50.94 TEST-UNEXPECTED-FAIL | valgrind-test | Syscall param sendmsg(msg.msg_iov[0]) points to uninitialised byte(s) at ??? / IPC::Channel::ChannelImpl::ProcessOutgoingMessages / IPC::Channel::ChannelImpl::Send / applyImpl
[task 2019-11-20T16:06:02.182Z] 16:06:02 INFO - 5:50.94 ==15873== Syscall param sendmsg(msg.msg_iov[0]) points to uninitialised byte(s)
[task 2019-11-20T16:06:02.182Z] 16:06:02 INFO - 5:50.94 ==15873== at 0x4E4372D: ??? (syscall-template.S:82)
[task 2019-11-20T16:06:02.182Z] 16:06:02 INFO - 5:50.94 ==15873== by 0xFED5EB3: IPC::Channel::ChannelImpl::ProcessOutgoingMessages()+867 (ipc/chromium/src/chrome/common/ipc_channel_posix.cc:693)
[task 2019-11-20T16:06:02.182Z] 16:06:02 INFO - 5:50.94 ==15873== by 0xFED757A: IPC::Channel::ChannelImpl::Send(IPC::Message*)+250 (ipc/chromium/src/chrome/common/ipc_channel_posix.cc:803)
[task 2019-11-20T16:06:02.182Z] 16:06:02 INFO - 5:50.94 ==15873== by 0xFF0FE6A: applyImpl<IPC::Channel, bool (IPC::Channel::)(IPC::Message ), StorePtrPassByPtr<IPC::Message> , 0> (dist/include/nsThreadUtils.h:1124)
[task 2019-11-20T16:06:02.182Z] 16:06:02 INFO - 5:50.94 ==15873== by 0xFF0FE6A: apply<IPC::Channel, bool (IPC::Channel::)(IPC::Message )> (dist/include/nsThreadUtils.h:1130)
[task 2019-11-20T16:06:02.182Z] 16:06:02 INFO - 5:50.94 ==15873== by 0xFF0FE6A: mozilla::detail::RunnableMethodImpl<IPC::Channel, bool (IPC::Channel::)(IPC::Message*), false, (mozilla::RunnableKind)0, IPC::Message*>::Run()+42 (dist/include/nsThreadUtils.h:1176)
[task 2019-11-20T16:06:02.182Z] 16:06:02 INFO - 5:50.94 ==15873== by 0xFECAC1D: RunTask (ipc/chromium/src/base/message_loop.cc:442)
[task 2019-11-20T16:06:02.182Z] 16:06:02 INFO - 5:50.94 ==15873== by 0xFECAC1D: DeferOrRunPendingTask (ipc/chromium/src/base/message_loop.cc:450)
[task 2019-11-20T16:06:02.182Z] 16:06:02 INFO - 5:50.94 ==15873== by 0xFECAC1D: MessageLoop::DoWork()+845 (ipc/chromium/src/base/message_loop.cc:523)
[task 2019-11-20T16:06:02.182Z] 16:06:02 INFO - 5:50.94 ==15873== by 0xFECBA88: base::MessagePumpLibevent::Run(base::MessagePump::Delegate*)+184 (ipc/chromium/src/base/message_pump_libevent.cc:321)
[task 2019-11-20T16:06:02.183Z] 16:06:02 INFO - 5:50.94 ==15873== by 0xFECA255: RunInternal (ipc/chromium/src/base/message_loop.cc:315)
[task 2019-11-20T16:06:02.183Z] 16:06:02 INFO - 5:50.94 ==15873== by 0xFECA255: RunHandler (ipc/chromium/src/base/message_loop.cc:308)
[task 2019-11-20T16:06:02.183Z] 16:06:02 INFO - 5:50.94 ==15873== by 0xFECA255: MessageLoop::Run()+85 (ipc/chromium/src/base/message_loop.cc:290)
[task 2019-11-20T16:06:02.183Z] 16:06:02 INFO - 5:50.94 ==15873== by 0xFED3210: base::Thread::ThreadMain()+464 (ipc/chromium/src/base/thread.cc:192)
[task 2019-11-20T16:06:02.183Z] 16:06:02 INFO - 5:50.94 ==15873== by 0xFED02A9: ThreadFunc(void*)+9 (ipc/chromium/src/base/platform_thread_posix.cc:40)
[task 2019-11-20T16:06:02.183Z] 16:06:02 INFO - 5:50.94 ==15873== by 0x4E3BB4F: start_thread+207 (pthread_create.c:304)
[task 2019-11-20T16:06:02.183Z] 16:06:02 INFO - 5:50.94 ==15873== by 0x5CD9FBC: clone+108 (clone.S:112)
[task 2019-11-20T16:06:02.183Z] 16:06:02 INFO - 5:50.94 ==15873== Address 0x1abc4329 is 1,129 bytes inside a block of size 8,192 alloc'd

Comment 19

[Tom Ritter [:tjr] (ni? for response to CVE/sec-approval/advisories/etc)]

:jandem - it appears that the callback is triggering valgrind failures.

I did two try runs with this patchset:

One where the callback is disabled - this passed Valgrind.
https://treeherder.mozilla.org/#/jobs?repo=try&revision=f853998537378f6ee3af1834483d4bb99899583f

One where the callback is enabled; but the callback does virtually nothing - this failed Valgrind
https://treeherder.mozilla.org/#/jobs?repo=try&revision=2f32c50710eacae37c6371f9072029cba0e811b9&selectedJob=277264322

I know you have some tests for the callbacks, but I don't imagine they're run under valgrind, so I think the issue may be with how the callback is set up, rather than this set of patches....

Comment 20

[Jan de Mooij [:jandem]]

The Valgrind log doesn't make sense to me; the callback itself doesn't involve memory allocation and is similar to other callbacks...

I'll do some digging today.

Comment 21

[Jan de Mooij [:jandem]]

(In reply to Jan de Mooij [:jandem] from comment #20)

I'll do some digging today.

Ongoing CI trouble is making this quite painful today :/ However I confirmed the problem happens with just your first patch, so I'm going to reduce that more to a minimal change and then reduce what this does on the JS engine side.

Comment 22

[Jan de Mooij [:jandem]]

The plot thickens, this triggers the failure too:

   if (XRE_IsE10sParentProcess()) {
     JS::SetFilenameValidationCallback(
         nsContentSecurityUtils::ValidateScriptFilename);
+    JS::SetFilenameValidationCallback(nullptr);
   }

Doesn't make sense because SetFilenameValidationCallback just sets a global pointer.

I'll reduce more.

Comment 23

[Jan de Mooij [:jandem]]

Valgrind is green if I change XRE_IsE10sParentProcess() to true like this.

Maybe calling XRE_IsE10sParentProcess() there affects IPC somehow and exposes a pre-existing issue? CC'ing jseward, the Valgrind author, in case he has seen something like this before.

Comment 24

[Jan de Mooij [:jandem]]

I wonder if we're reading some prefs too early in startup. In that case bug 1579367 might help because it makes the code in XPCJSRuntime run later in the parent process. I'll try with that patch to see if that fixes this...

Comment 25

[Jan de Mooij [:jandem]]

(In reply to Jan de Mooij [:jandem] from comment #24)

I wonder if we're reading some prefs too early in startup. In that case bug 1579367 might help

It does indeed fix this. I'll rebase that and try to land it this week.

Comment 26

[Jan de Mooij [:jandem]]

Bug 1579367 landed so this should be green now, but please verify on Try.

Comment 27

[Tom Ritter [:tjr] (ni? for response to CVE/sec-approval/advisories/etc)]

Okay, this is good to go; but at this point I'm going to wait until after the freeze.

Comment 28

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/40dd63b6910a
Register a ScriptValidationCallback to examine script loads in the parent process r=Gijs,ckerschb
https://hg.mozilla.org/integration/autoland/rev/1374c08302f9
Rename FilenameType to FilenameTypeAndDetails r=Gijs,ckerschb
https://hg.mozilla.org/integration/autoland/rev/017582048986
Rename FilenameToEvalType to FilenameToFilenameType r=Gijs,ckerschb
https://hg.mozilla.org/integration/autoland/rev/02865605c1c3
Record a Telemetry Event if we receive a request to load a script filename we aren't expecting r=chutten
https://hg.mozilla.org/integration/autoland/rev/391ed11326fb
Disable javascript filename validation on all entry points from xpcshell r=jandem
https://hg.mozilla.org/integration/autoland/rev/1eb6f6b02149
Disable script filename restrictions in Marionette r=jandem,marionette-reviewers,whimboo
https://hg.mozilla.org/integration/autoland/rev/59db30e1915f
Switch test-actor-registry.js to use a chrome:// URL r=jdescottes
https://hg.mozilla.org/integration/autoland/rev/dfdefc6ede97
Disable the javascript filename load restriction on specific tests r=jdescottes
https://hg.mozilla.org/integration/autoland/rev/7dd0266da0a1
Disable script filename validation in Browser Toolbox console debugging r=jimb
https://hg.mozilla.org/integration/autoland/rev/a615a2c07523
Add in exception cases where we disable the javascript load restrictions r=ckerschb

Comment 29

[Andrei Ciure[:aciure]]

Backed out 10 changesets (bug 1582512) for causing browser_preferences_usage.js to permafail

push that caused the backout: https://treeherder.mozilla.org/#/jobs?repo=autoland&resultStatus=testfailed%2Cbusted%2Cexception%2Cretry%2Cusercancel%2Crunnable&revision=a615a2c0752306657fb81260e1967628822fdbee&selectedJob=279101828

backout: https://hg.mozilla.org/integration/autoland/rev/2bc04d118e225666caa5b8b09de45728529a005b

Comment 30

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/fb2e0191992c
Register a ScriptValidationCallback to examine script loads in the parent process r=Gijs,ckerschb
https://hg.mozilla.org/integration/autoland/rev/f76cb5caaaf5
Rename FilenameType to FilenameTypeAndDetails r=Gijs,ckerschb
https://hg.mozilla.org/integration/autoland/rev/350db18bda88
Rename FilenameToEvalType to FilenameToFilenameType r=Gijs,ckerschb
https://hg.mozilla.org/integration/autoland/rev/a7c2818664b0
Record a Telemetry Event if we receive a request to load a script filename we aren't expecting r=chutten
https://hg.mozilla.org/integration/autoland/rev/2c3e20596423
Disable javascript filename validation on all entry points from xpcshell r=jandem
https://hg.mozilla.org/integration/autoland/rev/9fd51e8501a6
Disable script filename restrictions in Marionette r=jandem,marionette-reviewers,whimboo
https://hg.mozilla.org/integration/autoland/rev/3634a5f6b7e2
Switch test-actor-registry.js to use a chrome:// URL r=jdescottes
https://hg.mozilla.org/integration/autoland/rev/fb72c8bc04b2
Disable the javascript filename load restriction on specific tests r=jdescottes
https://hg.mozilla.org/integration/autoland/rev/7b88b317577e
Disable script filename validation in Browser Toolbox console debugging r=jimb
https://hg.mozilla.org/integration/autoland/rev/ac2a45259a35
Add in exception cases where we disable the javascript load restrictions r=ckerschb

Comment 31

[Natalia Csoregi [:nataliaCs]]

https://hg.mozilla.org/mozilla-central/rev/fb2e0191992c
https://hg.mozilla.org/mozilla-central/rev/f76cb5caaaf5
https://hg.mozilla.org/mozilla-central/rev/350db18bda88
https://hg.mozilla.org/mozilla-central/rev/a7c2818664b0
https://hg.mozilla.org/mozilla-central/rev/2c3e20596423
https://hg.mozilla.org/mozilla-central/rev/9fd51e8501a6
https://hg.mozilla.org/mozilla-central/rev/3634a5f6b7e2
https://hg.mozilla.org/mozilla-central/rev/fb72c8bc04b2
https://hg.mozilla.org/mozilla-central/rev/7b88b317577e
https://hg.mozilla.org/mozilla-central/rev/ac2a45259a35