Closed Bug 948574 Opened 11 years ago Closed 10 years ago

[e10s] remote nsISiteSecurityService::IsSecureHost/IsSecureURI

Categories

(Core :: Security: PSM, defect)

Product:
Core
Component:
Security: PSM
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla34
Tracking Flags:
Tracking Status
e10s + ---

People

(Reporter: keeler, Assigned: jimm)

References

Details

Attachments

(2 files, 1 obsolete file)

patch v1.1
8.11 KB, patch
bzbarsky
: review+
Details | Diff | Splinter Review
patch v1.1 (rebased)
8.06 KB, patch
pauljt
: superreview+
Details | Diff | Splinter Review 
Child processes should not have direct access to nsISiteSecurityService.
Attached patch patch (obsolete) — — Splinter Review 
bsmedberg - I'd appreciate if you would have a look at this. If someone else would be more appropriate, feel free to redirect it.
Assignee: nobody → dkeeler
Status: NEW → ASSIGNED
Attachment #8345566 - Flags: review?(benjamin)
Why do these things need to be accessible to the child process at all? These functions are used for making security decisions that, AFAICT, should be made only in the parent process.

It seems like these functions are used for tests, but IMO we shouldn't expose functionality like this just for tests.

I could very well be overlooking something though.
Well, the way it's currently architected, the docshell looks up if a site is HSTS before showing the about:certerror page to know if it should even offer the ability to add an override: https://mxr.mozilla.org/mozilla-central/source/docshell/base/nsDocShell.cpp#4384
(In reply to David Keeler (:keeler) from comment #3)
> Well, the way it's currently architected, the docshell looks up if a site is
> HSTS before showing the about:certerror page to know if it should even offer
> the ability to add an override:
> https://mxr.mozilla.org/mozilla-central/source/docshell/base/nsDocShell.
> cpp#4384

Yes, I understand. But, about:certerror should be loaded in the parent process so that it actually works. I think what we need is a way for the parent process to detect that the root document load failed due to an SSL error, and then have it (the parent process) cancel the document load and navigate to the relevant about:certerror page in a parent-process-loaded tab.
Attachment #8345566 - Flags: review?(benjamin) → review?(bzbarsky)
Comment on attachment 8345566 [details] [diff] [review]
patch

r=me if this was a diff -w (so the actual indentation of the NS_ENSURE_SUCCESS bit after rv = sss->IsSecureURI() in docshell is sane) and if IsSecureURI() can in fact only return a failure result in fatal situations.  Is this last definitely the case?
Attachment #8345566 - Flags: review?(bzbarsky) → review+
Brian - I filed bug 951405 to fix this properly, but I think this is a reasonable step forward in the meantime.

Boris - It wasn't a diff -w -- the indentation was just a mistake on my part. I'm almost certain IsSecureURI cannot fail except in fatal situations at this point in the code.
(In reply to David Keeler (:keeler) from comment #6)
> Boris - It wasn't a diff -w -- the indentation was just a mistake on my
> part. I'm almost certain IsSecureURI cannot fail except in fatal situations
> at this point in the code.

And thank you for the review, by the way.
One more question, though - should I also do error-checking on the SendIsSecureURI call?
Flags: needinfo?(bzbarsky)
I don't know.  Can that call fail without crashing the process?
Flags: needinfo?(bzbarsky)
(In reply to Boris Zbarsky [:bz] from comment #8)
> I don't know.  Can that call fail without crashing the process?

Not as far as I can tell. I'm not extremely familiar with the IPC infrastructure, however.
Calls from a child process on the PContent IPDL subtree do not fail. Error-checking is typically only required in the parent process.
Attached patch patch v1.1 — — Splinter Review 
Ok - I fixed the indentation issue. Also, from my understanding of the discussion in this bug, the error checking is correct.
Attachment #8345566 - Attachment is obsolete: true
Attachment #8356765 - Flags: review?(bzbarsky)
Comment on attachment 8356765 [details] [diff] [review]
patch v1.1

r=me
Attachment #8356765 - Flags: review?(bzbarsky) → review+
Can't we just land this patch without the MOZ_CRASH?
That might be an option. However, since bug 775370 is the main reason for this, and since the correctness of that patch depends on child processes not instantiating their own nsSiteSecurityService, it's going to be necessary either way. As long as we're making progress with bug 959752 (and it seems like we are), I think it's fine to wait.
Oh, I thought this had already landed. I still don't quite understand why we should do this, though. It seems like we need to fix 951405 so that about:certerror actually works in order for e10s to be enabled by default in Nightly. And, if we do that, then we don't need this. So, I don't see how this intermediate step is useful except for making a few more tests pass (sorta wrongly). Now we have an e10s flag to disable tests just for e10s and if we use that it seems like we don't need to resolve this bug at all.

Or, to put this a different way: can't we "fix" this bug just as effectively with a few uses of "skip-of = e10s" in test manifests?
(In reply to Brian Smith (:briansmith, was :bsmith; NEEDINFO? for response) from comment #18)
> Oh, I thought this had already landed. I still don't quite understand why we
> should do this, though. It seems like we need to fix 951405 so that
> about:certerror actually works in order for e10s to be enabled by default in
> Nightly.

In bug 951405 we addressed issues with about pages that load in the child similar to the way we did this in native fennec, by forwarding user interaction over to chrome to handle chrome privileged calls. This fits with our general goal of having all about pages load remotly. (We want as few exceptions as possible, currently I think the only page that does this is newtab, and we plan to fix that.)

With that, can we reconsider landing this patch? I'm curious why we consider an ipdl call like this to be a secuirty risk. I can understand wanting to limit access in the child to interfaces like nsISiteSecurityService, but a forwarded call for isSecureURI seems pretty safe, I don't understand how something like that can be exploited?

I can take this to finish it up if we agree it's ok.
Flags: needinfo?(brian)
Since I'm not working on the e10s/sandboxing stuff at all any more, I don't think it makes sense to block progress on what I think. I suggest that the people designing and prioritizing the security aspects of e10s/sandboxing should decide what to do.
Flags: needinfo?(brian)
One reason I think it's not ideal to load the about:certerror page in the child process is that we're basically doing this (unless I'm misunderstanding):

child tells parent it wants to load a page -> parent does network IO, encounters certificate error, tells child -> child loads about:certerror, user clicks "add exception", child tells parent about it -> parent adds an exception

The problem is that we're trusting the information the child provided (i.e. what certificate/host to add an exception for, and that the user actually clicked the "add exception" button) when we don't have to. Instead, this could be:

child tells parent it wants to load a page -> parent does network IO, encounters a certificate error, presents about:certerror. User clicks "add exception", parent does so, and tells the child it should try again -> child loads page
(In reply to David Keeler (:keeler) [use needinfo?] from comment #21)
> One reason I think it's not ideal to load the about:certerror page in the
> child process is that we're basically doing this (unless I'm
> misunderstanding):
> 
> child tells parent it wants to load a page -> parent does network IO,
> encounters certificate error, tells child -> child loads about:certerror,
> user clicks "add exception", child tells parent about it -> parent adds an
> exception.

I'm not convinced the added back and forth between parent and child is a major issue. We do save overhead with the current approach in that the cert error loads into an existing remote browser, where as the other way we have to swap out browser objects on the fly and transfer state across to get pages like certerror displaying correctly.

> The problem is that we're trusting the information the child provided (i.e.
> what certificate/host to add an exception for, and that the user actually
> clicked the "add exception" button) when we don't have to.

Well, the caller has to provide the site and the dialog handles the rest, including clear display of the site the exception is for. Not sure this is such an issue. You raise an issue though that has me wondering what our policy should be for message manager api exposure in the child. I've never seen this documented before. Poking around in toolkit, I see various apis related to login manager, addon policies, browser security ui, and auto-fill. If we have set a message manager exposure policy, I wonder if it's being applied when people do reviews currently?

billm, blassey, any ideas here? Do we have a restriction on exposing privileged chrome functionality via mm in the child?
Assignee: dkeeler → jmathies
(In reply to Jim Mathies [:jimm] from comment #22)
> Well, the caller has to provide the site and the dialog handles the rest,
> including clear display of the site the exception is for. Not sure this is
> such an issue. You raise an issue though that has me wondering what our
> policy should be for message manager api exposure in the child. I've never
> seen this documented before. Poking around in toolkit, I see various apis
> related to login manager, addon policies, browser security ui, and
> auto-fill. If we have set a message manager exposure policy, I wonder if
> it's being applied when people do reviews currently?
> 
> billm, blassey, any ideas here? Do we have a restriction on exposing
> privileged chrome functionality via mm in the child?

I'm not sure we could have a concrete policy besides "be careful". Reviewers should always be aware that the content process could be compromised. I think :kang has an intern who is working on auditing all of these calls, but that effort might be b2g-specific.
(In reply to Bill McCloskey (:billm) from comment #23)
> (In reply to Jim Mathies [:jimm] from comment #22)
> > Well, the caller has to provide the site and the dialog handles the rest,
> > including clear display of the site the exception is for. Not sure this is
> > such an issue. You raise an issue though that has me wondering what our
> > policy should be for message manager api exposure in the child. I've never
> > seen this documented before. Poking around in toolkit, I see various apis
> > related to login manager, addon policies, browser security ui, and
> > auto-fill. If we have set a message manager exposure policy, I wonder if
> > it's being applied when people do reviews currently?
> > 
> > billm, blassey, any ideas here? Do we have a restriction on exposing
> > privileged chrome functionality via mm in the child?
> 
> I'm not sure we could have a concrete policy besides "be careful". Reviewers
> should always be aware that the content process could be compromised. I
> think :kang has an intern who is working on auditing all of these calls, but
> that effort might be b2g-specific.

To close out this discussion here - we have an open tracking bug on hardening existing mm apis (bug 820202), we've already audited these apis once for b2g (bug 777602), and I've filed bug 1034297 about adding a doc to mdn documentation our current policy for both devs, reviewers and addon developers.
Attachment #8451552 - Flags: superreview?(ptheriault)
(In reply to Jim Mathies [:jimm] from comment #25)
> Created attachment 8451552 [details] [diff] [review]
> patch v1.1 (rebased)
> 
> try push:
> 
> https://tbpl.mozilla.org/?tree=Try&rev=6d8e2462a73f

Good green run here.
No longer depends on: 959752
So we are basically remoting nsISiteSecurityService::IsSecureURI so that the child doesn't have access to nsISiteSecurityService functions. I assume there is some other patch somewhere (or planned) which would prevent using nsISiteSecurityService in the child right? (I.e. this is just the code to remote the service, but I am not familiar with what our plan is to make this functionality actually restricted to the parent.) 

PS I'm probably not appropriate for superreview here, but happy to provide input as far as I can. Maybe Sid can provide this?
Flags: needinfo?(sstamm)
(In reply to Paul Theriault [:pauljt] from comment #27)
> So we are basically remoting nsISiteSecurityService::IsSecureURI so that the
> child doesn't have access to nsISiteSecurityService functions. I assume
> there is some other patch somewhere (or planned) which would prevent using
> nsISiteSecurityService in the child right? 

That's in this patch actually, down at the bottom in nsSiteSecurityService::Init(), we crash if someone tries to to initiate the service in a child process.

> PS I'm probably not appropriate for superreview here, but happy to provide
> input as far as I can. Maybe Sid can provide this?

No problem, I asked you because you did those audits of mm apis and I don't think we have a designated security reviewer for mm apis exposure currently. You seemed like the right person for an sr. :)
(In reply to Jim Mathies [:jimm] from comment #28)
> (In reply to Paul Theriault [:pauljt] from comment #27)
> > So we are basically remoting nsISiteSecurityService::IsSecureURI so that the
> > child doesn't have access to nsISiteSecurityService functions. I assume
> > there is some other patch somewhere (or planned) which would prevent using
> > nsISiteSecurityService in the child right? 
> 
> That's in this patch actually, down at the bottom in
> nsSiteSecurityService::Init(), we crash if someone tries to to initiate the
> service in a child process.

OK that was my concern. Isn't this crash mechanism completely bypass-able in a compromised child scenario? (i.e. where the child process can run arbitrary code, privileged js etc)
IE you can't init the security service, but is there anything stopping the compromised child just making the same IPC calls that the nsSiteSecurityService makes.
The security property we want here is that a compromised child can't remove/modify entries in the parent's data structure that backs the site security service. At the moment, this is the permission manager. Can child processes make arbitrary calls to that service?
(In reply to Paul Theriault [:pauljt] from comment #30)
> IE you can't init the security service, but is there anything stopping the
> compromised child just making the same IPC calls that the
> nsSiteSecurityService makes.

It could make a call to IsSecureURI. But they can't get into the guts of the service itself. I think this is generally the way we want things to be.
(In reply to David Keeler (:keeler) [use needinfo?] from comment #31)
> The security property we want here is that a compromised child can't
> remove/modify entries in the parent's data structure that backs the site
> security service. At the moment, this is the permission manager. Can child
> processes make arbitrary calls to that service?

Looks like we mirror perms but don't allow access to the db, assuming I'm looking at the right src file here for what you're referring to - 

http://mxr.mozilla.org/mozilla-central/source/extensions/cookie/nsPermissionManager.cpp#403
(In reply to Jim Mathies [:jimm] from comment #28)
> > PS I'm probably not appropriate for superreview here, but happy to provide
> > input as far as I can. Maybe Sid can provide this?
> 
> No problem, I asked you because you did those audits of mm apis and I don't
> think we have a designated security reviewer for mm apis exposure currently.
> You seemed like the right person for an sr. :)

Sorry for the delay.  Paul: I agree with jimm that you're a fine sr for this since you seem to be closer to the "what we expose in mm" more than any of us.  I'm happy to provide input, but haven't thought as much about mm api exposure.

FWIW, the patch looks fine to me as long as we're ok with exposing isSecureURI (doesn't seem a significant security risk to know if a site is an HSTS site) and there's no way for the child to reach *around* this control and into whatever DB stores the HSTS data as keeler and paul said.  This means for completeness, we probably need to verify that the permission manager can't be mutated by the child.
Flags: needinfo?(sstamm)
Comment on attachment 8451552 [details] [diff] [review]
patch v1.1 (rebased)

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

Ok great - r+ based on the comments from Jim and David above.
Attachment #8451552 - Flags: superreview?(ptheriault) → superreview+
Note that I think its worth revisiting the previous audits of message manager and IPDL protocols- my team will start looking into this. See bug 1041862 & bug 1040184.
https://hg.mozilla.org/mozilla-central/rev/e945647d2933
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla34
See Also: → 1215723
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: