1647519 - Reject javascript: requests targeting other content processes

Status: RESOLVED FIXED

(Core :: DOM: Content Processes, task, P3)

Description

[Chris Peterson [:cpeterson]]

Ensure javascript: requests targeting other content processes are rejected and add a test to verify this.

Comment 1

[Chris Peterson [:cpeterson]]

M6c

Comment 2

[Neha Kochar [:neha]]

Nika and Peter said we should write a test that loading javascript URIs in an OOP iframe throws a nice exception in the console to ensure we don't regress it with Fission. Currently, without Fission, it fails silently.

Comment 3

[David Teller [:Yoric] - still alive but not very active]

A few notes from #Fission (thanks, kmag and nika!) :

Cross-process case:

1. A ContentChild issues javascript: load by calling BrowsingContext::LoadURI – for our example, a (forbidden) cross-process load;
2. ... or nsDocShell::LoadURI (or InternalLoad) sends LoadURI (or InternalLoad) to the ContentParent;
3. The BrowsingContext sends LoadURI (or InternalLoad) to the ContentParent (we need to block this with a nice error message).
4. The ContentParent receives the LoadURI and sends a LoadURI to another child (need to block this with IPC_FAIL in ContentParent::RecvLoadURI - RecvInternalLoad, in case the ContentChild has been compromised).
5. The ContentChild receives the LoadURI (nothing to do, with the patch, this has already been blocked in step 3. or 4.).

In-process case:

Some same-process loads may hit nsDocShell::LoadURI directly without going through BrowsingContext, and they should still reject with a sane error if the triggering principal doesn't subsume the principal of the target document.

Also:
@kmag "Incidentally, you probably want to report the error with rv.ThrowSecurityError(...) something like this: https://searchfox.org/mozilla-central/rev/ce21a13035623c1d349980057d09000e70669802/js/xpconnect/wrappers/AccessCheck.cpp#153
But in the case of window.open and window.location = ..., I don't think we're allowed to throw for things like this, so I think we'll just need to create the exception and report it."

Comment 4

[David Teller [:Yoric] - still alive but not very active]

Attachment: Bug 1647519 - Tests for cross-frame javascript: URIs;r?kmag

Comment 5

[David Teller [:Yoric] - still alive but not very active]

Attachment: Bug 1647519 - Reject cross-process javascript: URIs;r?kmag

Comment 6

[David Teller [:Yoric] - still alive but not very active]

Left a few questions in https://phabricator.services.mozilla.com/D89361.

Comment 7

[David Teller [:Yoric] - still alive but not very active]

More questions in https://phabricator.services.mozilla.com/D89361.

Comment 8

[David Teller [:Yoric] - still alive but not very active]

Latest try run: https://treeherder.mozilla.org/#/jobs?repo=try&revision=9233e872b8120a479dbc94377aebb197d7f15421&selectedTaskRun=f_XQtqGSQOGQQvQ64tYKeg.0 .

Comment 9

[David Teller [:Yoric] - still alive but not very active]

I won't have time to finish this.

Comment 10

[Chris Peterson [:cpeterson]]

kmag just needs to rewrite the tests in Yoric's patch.

Keeping in Fission M6c.

Comment 11

[Kris Maglione [:kmag]]

Attachment: Bug 1647519: Reject javascript: requests targeting other content processes. r=nika

Loads targeting cross-process BrowsingContexts are by definition cross-origin,
which should preclude any javascript: loads. While those loads are currently
prevented by principal checks in the final target process, sending IPC
messages for the attempts is unnecessary, and potentially opens a door to
privilege escalation exploits by a compromised content process.

This patch prevents any cross-process load requests from being sent by content
processes, and adds checks in the parent process to kill any (potentially
compromised) content process which attempts to send them.

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/0395717ea76d
Reject javascript: requests targeting other content processes. r=nika

Comment 13

[Cristian Brindusan [:cbrindusan]]

Backed out changeset 0395717ea76d (bug 1647519) for build bustages at BrowsingContext.cpp.
https://hg.mozilla.org/integration/autoland/rev/13b11cf948be1c854403a2c8a53794d201d82e52

Push with failures:
https://treeherder.mozilla.org/jobs?repo=autoland&revision=0395717ea76def6574e2dd06fcbfbd19c0d298d5&selectedTaskRun=DOzYT8Y4SQyQDvwt4ys5wg.0

Failure log:
https://treeherder.mozilla.org/logviewer?job_id=328608947&repo=autoland&lineNumber=13419

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/c0ecccf36d56
Reject javascript: requests targeting other content processes. r=nika

Comment 15

[Alexandru Michis [:malexandru]]

Backed out changeset c0ecccf36d56 (Bug 1647519) for causing bustages in BrowsingContext.cpp

Failure log: https://treeherder.mozilla.org/logviewer?job_id=328614054&repo=autoland&lineNumber=7567

Push with failures: https://treeherder.mozilla.org/jobs?repo=autoland&selectedTaskRun=Ql0lyPKiRnKIJpeTu3XuRQ.0&tochange=598f1502388a58e5c75810fadfe77ff7258e89e2&fromchange=86192d1ed893aac76580c3486eb865cd2378afad&searchStr=build

Comment 16

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/e4cefba9f87e
Reject javascript: requests targeting other content processes. r=nika

Comment 17

[Cristian Brindusan [:cbrindusan]]

Backed out changeset e4cefba9f87e (Bug 1647519) for build bustages in BrowsingContext.cpp.
https://hg.mozilla.org/integration/autoland/rev/11f295953afb1fa4783f7ef2a2f7bab04ac6c6ce

Push with failures:
https://treeherder.mozilla.org/jobs?repo=autoland&revision=e4cefba9f87ed7d4c5e49ae1b0a83875229d3667&selectedTaskRun=HLBUnTF1Rq6sM_bdq3MAuw.0

Failure log:
https://treeherder.mozilla.org/logviewer?job_id=328625001&repo=autoland&lineNumber=13562

Comment 18

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/fcd21b8e5e55
Reject javascript: requests targeting other content processes. r=nika

Comment 19

[Sandor Molnar[:smolnar]]

https://hg.mozilla.org/mozilla-central/rev/fcd21b8e5e55