1799692 - Track TriggeringRemoteType for worker-triggered navigations

Status: RESOLVED FIXED

(Core :: DOM: Workers, defect, P3)

Description

[Nika Layzell [:nika] (ni? for response)]

In bug 1538028, I'm adding the concept of TriggeringRemoteType which is being set on nsDocShellLoadState instances when they're created in the parent, and then bounced down into the child. Unfortunately, this is quite difficult to do for navigations triggered by clients.

In the case of ClientOpenWindow, this would require tracking the remoteType which triggered the load from ClientManagerOpParent (which would need to pull it from the PBackground instance, likely by adding another method to BackgroundParent to access RemoteType like we have for GetChildID). This would need to be passed all of the way down into WaitForLoad, where it would be set on the nsDocShellLoadState. Right now, by not setting the triggering remote type, it is treated as being triggered by the parent process.

It's a bit more complex in the case of ClientNavigateOpChild, which currently starts the navigation in the client's content process. The load will be considered to have the triggering remote type of the managed client. This is probably fine for now (as I don't think a service worker should be able to manage a document loaded in a more-privileged process?), but is probably worth noting. Fixing this would likely require moving where the navigation is started into the parent process, so that it's possible to set TriggeringRemoteType (as it's not OK to configure it in a content process).

In the case of ClientOpenWindow, not correctly specifying TriggeringRemoteType means this API could be used as part of a content process privilege escalation, where the compromised content process writes a temporary file to disk, and then navigates to it, loading attacker controlled content in a file content process.

Comment 1

[Daniel Veditz [:dveditz]]

I thought content processes weren't able to write files? But if we can it sounds like you're describing a possible sandbox escape. Normally that would be "sec-high" but I'm not sure that's appropriate here.

Comment 2

[Harveer Singh]

Since this is on the workers and content processes, I am setting this as needinfo from @asuth.

Comment 3

[Nika Layzell [:nika] (ni? for response)]

(In reply to Daniel Veditz [:dveditz] from comment #1)

I thought content processes weren't able to write files? But if we can it sounds like you're describing a possible sandbox escape. Normally that would be "sec-high" but I'm not sure that's appropriate here.

Content processes generally cannot write files to arbitrary places, but they do have access to write files to a temporary directory for that process, as well as to trigger things such as downloads, which will write a file to the downloads folder. This issue I'm describing here is another instance of the sandbox escape in bug 1538028 which I wanted to split out into a separate bug.

Comment 4

[Andrew Sutherland [:asuth] (he/him)]

I've assigned this a severity/priority consistent with bug 1491018 which this is a variation on, with this being strongly related to bug 1491113 which I'm marking a dependency relation on. From discussion with :nika via other channels:

• It would be very good for the parent process to be validating all the principals / principal consistencies involved here!
• IPDL explicitly has a Tainted message annotation which manifests as a newtype wrapper which we can use, although as I look at it now, it seems like we'd really want to be able to put the annotation on the ipdlh struct fields in ClientIPCTypes.ipdlh and being able to annotate the message itself is probably not going to do be useful for us. But we can obviously also use custom types and probably want to obsolete taking PrincipalInfos at all instead depending on more of an object-capability approach.

Comment 5

[BugBot [:suhaib / :marco/ :calixte]]

The severity field for this bug is set to S3. However, the bug is flagged with the sec-high keyword.
:hsingh, could you consider increasing the severity of this security bug?

For more information, please visit auto_nag documentation.

Comment 6

[Nika Layzell [:nika] (ni? for response)]

(In reply to Andrew Sutherland [:asuth] (he/him) from comment #4)

I've assigned this a severity/priority consistent with bug 1491018 which this is a variation on, with this being strongly related to bug 1491113 which I'm marking a dependency relation on.

This is mostly filed as a fork off of bug 1538028. That bug is being approached without doing principal validation, as content processes can load things with the system principal, which generally has full permissions. In this case the big thing I want to do is track the triggering remote type, not validate principals, so that we can block that specific case from bug 1538028. We should also do the bigger work in bug 1491018 eventually, though it'll be a larger project.

Comment 7

[Nika Layzell [:nika] (ni? for response)]

Attachment: Bug 1799692 - Part 1: Introduce ThreadsafeContentParentHandle, r=asuth!

This introduces a new type to ContentParent which acts as a weak handle to the
actor and is safe to hold and manipulate from any thread.

This replaces accesses of the ContentParent type from the background thread,
as they were error-prone due to ContentParent not being threadsafe-refcounted.

The bulk of this patch is piping the new type through to the places it is
required, and removing now-unecessary extra complexity.

Comment 8

[Nika Layzell [:nika] (ni? for response)]

Attachment: Bug 1799692 - Part 2: Specify TriggeringRemoteType for ClientOpenWindow loads, r=asuth!

This builds on the changes in part 1 to specify the triggering remote type for
loads created from ClientOpenWindow.

Depends on D162346

Comment 9

[Jens Stutte [:jstutte]]

(In reply to Andrew Sutherland [:asuth] (he/him) from comment #4)

I've assigned this a severity/priority consistent with bug 1491018 which this is a variation on, with this being strongly related to bug 1491113 which I'm marking a dependency relation on. From discussion with :nika via other channels:

• It would be very good for the parent process to be validating all the principals / principal consistencies involved here!
• IPDL explicitly has a Tainted message annotation which manifests as a newtype wrapper which we can use, although as I look at it now, it seems like we'd really want to be able to put the annotation on the ipdlh struct fields in ClientIPCTypes.ipdlh and being able to annotate the message itself is probably not going to do be useful for us. But we can obviously also use custom types and probably want to obsolete taking PrincipalInfos at all instead depending on more of an object-capability approach.

:dveditz, would you consider lowering the sec rating in this light? Otherwise we might need to raise the severity here.

Comment 10

[Daniel Veditz [:dveditz]]

Since this blocks bug 1538028 we don't need to double-count the sec-high

Comment 11

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Part 1: Introduce ThreadsafeContentParentHandle, r=asuth
https://hg.mozilla.org/integration/autoland/rev/56bdce9421f30b02578b5e7f51c0f35438b714b6
https://hg.mozilla.org/mozilla-central/rev/56bdce9421f3

Part 2: Specify TriggeringRemoteType for ClientOpenWindow loads, r=asuth
https://hg.mozilla.org/integration/autoland/rev/73b3372a88c5894eec2125711abcaa11f0e87552
https://hg.mozilla.org/mozilla-central/rev/73b3372a88c5