Closed Bug 1295322 (CVE-2017-5382) Opened 8 years ago Closed 8 years ago

In the newest nightly builds, its possible to capture chrome level exceptions at content level.

Categories

(Firefox Graveyard :: RSS Discovery and Preview, defect)

51 Branch
defect
Not set
normal

Tracking

(firefox48 wontfix, firefox49 wontfix, firefox50 wontfix, firefox51 fixed)

RESOLVED FIXED
Firefox 51
Tracking Status
firefox48 --- wontfix
firefox49 --- wontfix
firefox50 --- wontfix
firefox51 --- fixed

People

(Reporter: jerri.rice.001, Assigned: bzbarsky)

References

Details

(Keywords: sec-moderate, Whiteboard: [post-critsmash-triage][adv-main51+])

Attachments

(4 files)

User Agent: Mozilla/5.0 (X11; Ubuntu; Linux x86_64; rv:48.0) Gecko/20100101 Firefox/48.0
Build ID: 20160728205137

Steps to reproduce:

Used a named window and the open function combined with the feed URI preview code changes to capture a chrome level exception at content level.  I know we do not want this.


Actual results:

A chrome level exception is captured at content level, and in this testcase is logged to the console.


Expected results:

It is my understanding that we do not want chrome level errors or exceptions being dispatched directly to content level.
This is basically a dupe of bug 1258071, and I'm not sure if we need to keep it sec-sensitive. The error messages are sometimes leaky in terms of security, but that isn't the case here. I also don't *think* that the exception objects provide content with anything it shouldn't have in terms of privilege. Which isn't to say we shouldn't fix it... I tried to fix this in the past and got stuck trying to make it go anywhere (plus we had more important things to worry about at the time...).
Component: Untriaged → RSS Discovery and Preview
With the other work I've filed, and some things yet to file I was attempting to achieve xraywrapper  bypass, and having things chrome privileged could help I would think.

Having said that I would leave this marked sec sensitive.  I also have some other bugs to file which may not leak anything sensitive but also originate from chrome code.

The point I was making here was that even if in an indirect way we still have content code interacting with chrome code to the point where it's possible to capture an exception obviously never meant for content.
"Great or dead", right? Our feed handling certainly isn't "great".
Status: UNCONFIRMED → NEW
Depends on: 1258071
Ever confirmed: true
Keywords: sec-audit
(In reply to Daniel Veditz [:dveditz] from comment #3)
> "Great or dead", right? Our feed handling certainly isn't "great".

That's a bigger debate, though, and one of the options is "make it great". I don't really use feeds in Firefox anymore so I don't know what "make it great" would boil down to (and it might not be much besides "close all the security holes / transparent bits").


In any case, I think this this is basically a problem we have with any/all (JS-implemented?) URI schemes. What happens if you create an invalid blob or http or moz-icon URI (I think those are all c++-implemented, fwiw) ?

Off-hand the only other JS-implemented ones I can find are from places and they just defer to the substituting URI protocol handler, so they likely don't have issues. But of course there's also add-on provided protocol handlers...

Feels like a general fix should be in the IO service which is abstracting all the URI creation away from the page, or whatever layer of abstraction between there and the page DOM is reflecting these exceptions. I'm not sure where that code lives. Boris, am I missing the point? Who'd be a good person to ensure that URI creation doesn't forward xpcom stuff from chrome to content? As I noted in comment #1, I tried to fix this in the feed component at some point and failed, but maybe I just got it wrong?
Flags: needinfo?(bzbarsky)
When we say "xpcom stuff", what are we talking about?  Are we talking about a chrome-side exception object that gets handed to content (i.e. via COW or whatnot)?  Or are we talking about content reflector created for an existing nsIException, or a new nsIException that then gets a content reflector but has its nsresult/message come from chrome?
Flags: needinfo?(bzbarsky)
(In reply to Boris Zbarsky [:bz] from comment #5)
> When we say "xpcom stuff", what are we talking about?  Are we talking about
> a chrome-side exception object that gets handed to content (i.e. via COW or
> whatnot)?  Or are we talking about content reflector created for an existing
> nsIException, or a new nsIException that then gets a content reflector but
> has its nsresult/message come from chrome?

I don't know what these things are and how to distinguish them. If it's any help, the object that gets passed to content has a QueryInterface method but accessing it gets me a wrapper error (Permission denied to access property "QueryInterface").
OK, so I took a look at the actual testcase.  The object content is getting is in fact a cross-compartment wrapper (an opaque-policy filtering wrapper).  The underlying object is a WebIDL reflector for an xpconnect Exception object.

So what's happening is that we call into the JS-impl protocol handler.  This throws an exception.  Then we unwind out to the binding code, which calls dom::Throw with the nsresult that got handed back.  We check the GetPendingException on the CycleCollectedJSRuntime and find it there, with the same nsresult.  So we reuse the existing exception object.  We try to get a reflector for our compartment, which gives us a cross-compartment wrapper because there is an existing reflector.

Note that Exception/DOMException return a null GetParentObject, so just get wrapped into whatever compartment first tries.

My gut feeling is that we should not reuse the existing exception object if it has a reflector already and that reflector is not subsumed our subject principal.  This would be the principled way to avoid leaking stuff out to content like this.  Bobby, can you think of any reason this would cause problems?
Flags: needinfo?(bobbyholley)
(In reply to Boris Zbarsky [:bz] from comment #7)
> My gut feeling is that we should not reuse the existing exception object if
> it has a reflector already and that reflector is not subsumed our subject
> principal.  This would be the principled way to avoid leaking stuff out to
> content like this.  Bobby, can you think of any reason this would cause
> problems?

This sounds like a great plan. Thanks for investigating this Boris!
Flags: needinfo?(bobbyholley)
Assignee: nobody → bzbarsky
Status: NEW → ASSIGNED
I didn't add tests because of the whole security bug thing and not having a good way to write a test that doesn't basically duplicate the testcase in this bug...  Ideas on this welcome!
Attachment #8784942 - Flags: review?(bobbyholley)
Attachment #8784940 - Flags: review?(bobbyholley) → review+
Comment on attachment 8784942 [details] [diff] [review]
part 2.  Stop propagating through xpconnect exceptions to consumers that won't be able to work with them

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

If it's not too much work, can you convert the testcase into a mochitest, upload it to the bug, and flag it as in-testsuite? That's my standard practice in this case.

If it's too much work don't worry about it - you're already a hero for fixing this.

::: dom/bindings/CallbackObject.cpp
@@ +274,5 @@
>        // to worry about ordering here.
>        if (mErrorResult.IsJSContextException()) {
> +        // XXXkhuey bug 1117269.  When this is fixed, please consider fixing
> +        // ThrowExceptionValueIfSafe over in Exceptions.cpp in the same way.
> +        

Nit: Whitespace

::: dom/bindings/Exceptions.cpp
@@ +30,5 @@
> +// aOriginalException.  The incoming value must be in the compartment of aCx.
> +// This function guarantees that an exception is pending on aCx when it returns.
> +// XXXbz should we even propagate through the original nsresult, or just go
> +// ahead and use NS_ERROR_UNEXPECTED like CallbackObject.cpp does when
> +// sanitizing random exceptions from JS-implemented webidl?

In general, I think we shouldn't get too hung up on propagating proper nsresults up to callers. UNEXPECTED is fine by me.

@@ +47,5 @@
> +  MOZ_ASSERT(js::IsObjectInContextCompartment(exnObj, aCx),
> +             "exnObj needs to be in the right compartment for the "
> +             "CheckedUnwrap thing to make sense");
> +
> +  if (js::CheckedUnwrap(exnObj)) {

I'm sort of torn between using this and wrapperSubsumes. I guess it conceptually depends on whether we want to make the security decision here or delegate that to the WrapperFactory (which made the call when picking the security wrapper). In practice it almost certainly doesn't matter, so this is probably fine.

@@ +57,5 @@
> +
> +  nsresult originalRv;
> +  if (NS_FAILED(aOriginalException->GetResult(&originalRv))) {
> +    originalRv = NS_ERROR_UNEXPECTED;
> +  }

If nobody is depending on us propagating the right thing here, I'm kind of tempted to not do so. This only happens in sketchy cases anyway.

@@ +72,5 @@
> +  }
> +  MOZ_ASSERT(syntheticVal.isObject() &&
> +             js::CheckedUnwrap(&syntheticVal.toObject()) ==
> +               &syntheticVal.toObject(),
> +             "Must have a reflector here, not a wrapper");

Probably better to just assert !js::IsWrapper.
Attachment #8784942 - Flags: review?(bobbyholley) → review+
dveditz, what does sec-audit mean here?  Do I need sec-approval to land this?
Flags: needinfo?(dveditz)
Attached patch testSplinter Review
Attachment #8785415 - Flags: review?(bobbyholley)
Comment on attachment 8785415 [details] [diff] [review]
test

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

Thanks.
Attachment #8785415 - Flags: review?(bobbyholley) → review+
You don't need sec-approval to land this. Changing to sec-moderate after IRC discussion with bz: we don't know how this could be exploited, but it's wrong enough that it's likely to be a good first step to one.
Flags: needinfo?(dveditz) → sec-bounty?
Keywords: sec-auditsec-moderate
https://hg.mozilla.org/mozilla-central/rev/926053fe7f06
https://hg.mozilla.org/mozilla-central/rev/98eba3f5cc51
https://hg.mozilla.org/mozilla-central/rev/94dfc6dd2648
https://hg.mozilla.org/mozilla-central/rev/2c4613b97a3c

Do we want to uplift this?
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Flags: needinfo?(bzbarsky)
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → Firefox 51
I'm not sure it's worth uplifting...  Chances are it's safe to do that, but I'm not very sure of that.
Flags: needinfo?(bzbarsky)
Flags: sec-bounty? → sec-bounty+
Group: firefox-core-security → core-security-release
Based on the recommendation in comment 18, this is now a wontfix for 50.
Flags: qe-verify-
Whiteboard: [post-critsmash-triage]
Whiteboard: [post-critsmash-triage] → [post-critsmash-triage][adv-main51+]
Alias: CVE-2017-5382
Group: core-security-release
Product: Firefox → Firefox Graveyard
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: