Closed Bug 1582512 Opened 1 year ago Closed 1 year ago

Restrict Javascript loads in the parent process, initially logging Telemetry

Categories

(Firefox :: Security, enhancement)

enhancement
Not set
normal

Tracking

()

RESOLVED FIXED
Firefox 73
Tracking Status
firefox73 --- fixed

People

(Reporter: tjr, Assigned: tjr)

References

Details

Attachments

(11 files, 1 obsolete file)

47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
3.43 KB, text/plain
chutten
: data-review+
Details
47 bytes, text/x-phabricator-request
Details | Review

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.

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

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

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

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

Attachment #9093983 - Attachment description: Bug 1582512 - Disable the javascript filename load restriction on specific tests → Bug 1582512 - Disable the javascript filename load restriction on specific tests r?Gijs

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

Depends on D53232

Attachment #9105682 - Attachment description: Bug 1582512 - Disable javascript filename validation on all entry points from xpcshell r?Gijs → Bug 1582512 - Disable javascript filename validation on all entry points from xpcshell r?jandem
Attachment #9106280 - Attachment description: Bug 1582512 - Disable script filename restrictions in Marionette r?Gijs → Bug 1582512 - Disable script filename restrictions in Marionette r?jandem
Attachment #9106279 - Attachment description: Bug 1582512 - Refactor devtools tests to not pass a URI as a filename in evalInSandbox r?Gijs → Bug 1582512 - Switch test-actor-registry.js to use a chrome:// URL r?jdescottes
Attachment #9093983 - Attachment description: Bug 1582512 - Disable the javascript filename load restriction on specific tests r?Gijs → Bug 1582512 - Disable the javascript filename load restriction on specific tests r?jdescottes
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+
Attachment #9109100 - Flags: data-review?(chutten) → data-review+
Pushed by tritter@mozilla.com:
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
Pushed by csabou@mozilla.com:
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

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

Flags: needinfo?(tom)
Attachment #9110265 - Attachment is obsolete: true

: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....

Flags: needinfo?(tom) → needinfo?(jdemooij)

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.

(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.

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.

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.

Flags: needinfo?(jdemooij) → needinfo?(jseward)

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...

(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.

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

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

Flags: needinfo?(jseward)
Pushed by tritter@mozilla.com:
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
Pushed by tritter@mozilla.com:
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
You need to log in before you can comment on or make changes to this bug.