835613 - Child process passes security sensitive information to parent process when creating WYCIWYG channel during document.open

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

When creating a WYCIWYG channel during a document.open()/document.write() in a child process, the Document creates the WYCIWYG channel by getting the security-sensitive information (e.g. mSecurityInfo) from the parent, creating a new WYCIWYG channel, and then configuring the WYGIWYG channel with the security-sensitive information it retrieved from the HTTP channel.

Instead, the security-sensitive information needs to be managed within the parent process. For example, we could add a "createWyciwygChannel" message (sent from the child to the parent) to the PHttpChannel protocol, which would create a new WYCIWYG channel based on the information in the HTTP channel, such that the copying of the security-sensitive state (securityInfo in particular) is done wholly within the parent process.

Comment 1

[Josh Aas]

We need an owner here.

Comment 2

[Johnny Stenback (:jst)]

jdm, could you look into this one?

Comment 3

[Josh Matthews [:jdm]]

Attachment: Make wyciwyg channels determine security info from related parent-side HTTP channel.

Comment 4

[Josh Matthews [:jdm]]

This is not my favourite patch I have ever written. Coupling wyciwyg and http in this way is awkward, but this is the way that minimizes the number of Makefile changes required. If someone could suggest a site that can be used to test my changes, that would be handy.

Comment 5

[Jason Duell]

Brian,  Honza has an idea for a cleaner architecture here, but we're wondering how much of a rush this is (not flagged currently for b2g tracking).  What are the downsides of not fixing this?

Comment 6

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

If it isn't blocking-b2g then it isn't something we should rush to fix if the fix is bad.

I am not sure what the process architecture is for the B2G browser or what the current security goals for it are. If the goal is to prevent untrusted content from screwing with the security indicators in the B2G browser, then this is important to fix soon. If that isn't a goal (yet) then it can have a lower priority.

I think it would be good to document Honza's idea in this bug now though so that we don't forget about it.

Comment 7

[Honza Bambas (:mayhemer)]

(In reply to Brian Smith (:bsmith) from comment #6)
> I think it would be good to document Honza's idea in this bug now though so
> that we don't forget about it.

It's bug 839684:

> My proposal is to instead of serialization send an IPC actor
> (SecInfoParent/SecInfoChild).  That would carry the serialization to make
> secinfo work on the child process (I believe this is needed, but maybe not!)
> and, the main goal, children may send it back to parent just by the actor's
> reference.  On the parent process we can then get the existing original raw
> secinfo object securely from SecInfoParent.

Comment 8

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

Comment on attachment 711833 [details] [diff] [review]
Make wyciwyg channels determine security info from related parent-side HTTP channel.

Review of attachment 711833 [details] [diff] [review]:
-----------------------------------------------------------------

(In reply to Honza Bambas (:mayhemer) from comment #7)
> It's bug 839684:
> 
> > My proposal is to instead of serialization send an IPC actor
> > (SecInfoParent/SecInfoChild).  That would carry the serialization to make
> > secinfo work on the child process (I believe this is needed, but maybe not!)

In the long term, nsISecurityInfo should NOT be needed in the child processes. It is used for the secure UI indicators in the address bar and ideally they should be in the parent process. However, I don't think the B2G browser has implemented process separation between the security UI indicators and the child process; if it hasn't yet, then we'll still need to support the sending of nsISecurityInfo from Parent -> Child.

Anyway, I am not sure it is possible to fix this bug yet without a clearer understanding of how the B2G browser can be made multi-process in a way that enforces a useful security boundary.

::: content/html/document/src/nsHTMLDocument.cpp
@@ +2441,5 @@
>    mWyciwygChannel = do_QueryInterface(channel);
>  
> +  nsCOMPtr<nsIHttpChannelChild> childChan = do_QueryInterface(aCallerChannel);
> +  if (childChan) {
> +    mWyciwygChannel->SetSecurityInfoFrom(childChan);

The point of this bug is that the child process shouldn't be able to choose which securityInfo that the parent gets. Here, the child is still deciding which channels are associated with each other and how. For example, with this code, the child process could associate the WYGIWYG channel for the main document with the securityInfo of any subresource HTTP channel. Also, the nsISecurityInfo isn't the only security-sensitive information. wcwgURI is just as sensitive, for example.

Comment 9

[Jason Duell]

Lucas, do you have a sense of whether this is something important enough to take this one-off fix in B2G v1.0.x, or should we wait for the rearchitecture in bug 839684?

Comment 10

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

One question is whether it is possible for this to be abused to allow a compromised app to change its principal (origin) during navigation, due to the fact that the child process chooses the URI to use for the WYCIWYG channel. For example, if I am the http://foo.com hosted app, and I get compromised, can the code that compromises me create a WYGIWYG channel for https://marketplace.firefox.com, that will then be used during back<->forth navigation? If so, the compromise of one app could be easily expanded to compromise any/all apps. But, I assume it must be the case that the parent process already enforces that the principal of a hosted app cannot change during the execution of the app. If so, I don't think that we should try to fix this for B2G 1.0.x.

Comment 11

[Lucas Adamski [:ladamski]]

It depends on the severity.  If this has potential for an sg:high or sg:critical bug (which it might be per comment 10) then I think we need to fix this.  needinfo on Jonas to provide feedback on Brian's concern.

Comment 12

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Basically all the security checks we do in the parent are based on the appid, not the origin. Child processes can already claim to do actions for any origin and we generally believe that. But it can't claim to do actions for another appid.

So claiming to do some action for https://marketplace.firefox.com doesn't really buy the attacker anything, since we still know which appid it's coming from.

In the case of normal web pages in the browser app, we further know that the action is coming from a web page rather than from somewhere in the browser app, which further restricts what the attacker can do.


So I don't think there are any bad security problems requiring immediate action here. Lying about origins is not a problem. Well, it's not something we're trying to prevent currently.

Comment 13

[Honza Bambas (:mayhemer)]

Comment on attachment 711833 [details] [diff] [review]
Make wyciwyg channels determine security info from related parent-side HTTP channel.

Dropping r request based on previous comment.

Josh, please rerequst the review when this gets attention again (or whenever you feel to do it).

Comment 14

[Andrew McCreight [:mccr8]]

Any progress here, jdm?

Comment 15

[Josh Matthews [:jdm]]

None at all.

Comment 16

[Josh Matthews [:jdm]]

Let us not keep lying to ourselves that this has someone working on it.

Comment 17

[Andrew McCreight [:mccr8]]

WYCIWYG is going away in bug 1489308.

Comment 18

[Boris Zbarsky [:bzbarsky]]

Yep, that entire data flow described in comment 0 no longer exists.