The default bug view has changed. See this FAQ.
Bug 779821 (CVE-2012-4205)

XHR created from sandboxes end up having system principal instead of principal of the sandbox

RESOLVED FIXED in Firefox 17

Status

()

Core
XPConnect
RESOLVED FIXED
5 years ago
2 years ago

People

(Reporter: krizsa, Assigned: krizsa)

Tracking

({regression, sec-high})

unspecified
mozilla17
x86_64
All
regression, sec-high
Points:
---
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox14 wontfix, firefox15 wontfix, firefox16+ wontfix, firefox17+ fixed, firefox-esr10 unaffected)

Details

(Whiteboard: [qa-][adv-track-main17+])

Attachments

(1 attachment)

(Assignee)

Description

5 years ago
I have not yet tested this just noticed the change in nsXHR init method. It is now using system principal, while previously it was using the current principal (fetched from the context stack if I'm not mistaken that has been removed...). CreateXMLHttpRequest method in XPCComponents.cpp should create an XHR object with the principal of the sandbox.
(Assignee)

Updated

5 years ago
Assignee: nobody → gkrizsanits
Looks like a regression from bug 754202.  This looks pretty bad for cases when add-ons are running untrusted code in a sandbox...

Bobby, can you look please?
Blocks: 754202
tracking-firefox16: --- → ?
tracking-firefox17: --- → ?
(In reply to Boris Zbarsky (:bz) [In and out Aug 1 - 10, out Aug 11-20] from comment #1)
> Looks like a regression from bug 754202.

Er, why? AFAICT this is a regression from bug 756277, which sets subject principal to system without examining the JS context stack. Whether we pull the principal off cx or cx->compartment is kind of irrelevant if we don't even try to find the right cx. ;-)

But yes, this is bad. Gabor, is this currently a security issue given the situations in which we create XHRs in sandboxes? I'm going to conservatively mark this s-s for now, but feel free to unmark if you think it's appropriate.

We should also definitely get test coverage for this stuff.

smaug, given the changes over in bug 756277, what should we be doing here to create the XHR?
Blocks: 756277
No longer blocks: 754202
Group: core-security
(Assignee)

Comment 3

5 years ago
(In reply to Bobby Holley (:bholley) from comment #2)
> Er, why? AFAICT this is a regression from bug 756277, which sets subject
> principal to system without examining the JS context stack. Whether we pull
> the principal off cx or cx->compartment is kind of irrelevant if we don't
> even try to find the right cx. ;-)

Yeah, that's the patch I was trying to find thanks. But the problem indeed that I failed to add a test case that checks if the xhr got too much privs. 

> 
> But yes, this is bad. Gabor, is this currently a security issue given the
> situations in which we create XHRs in sandboxes? I'm going to conservatively
> mark this s-s for now, but feel free to unmark if you think it's appropriate.

The reason why I wasn't that conservative, that I think that patch is not released yet (bug 734891 patch 5), it is not used yet by jetpack sdk, and it's undocumented. That being said I don't mind the orange background on this bug.

> smaug, given the changes over in bug 756277, what should we be doing here to
> create the XHR?

eh... this is tricky I would be really curious what would be the right way now to create an XHR for sandboxes that does not have chrome privs, nor any window. Now that we don't have the context stack can we still check somehow the principal used by the current call context?

Comment 4

5 years ago
(In reply to Bobby Holley (:bholley) from comment #2)
> smaug, given the changes over in bug 756277, what should we be doing here to
> create the XHR?
Initialize XHR using the right principal. That is why nsIXMLHttpRequest has [noscript] init(...).
Only aPrincipal needs to be non-null.
(Assignee)

Comment 5

5 years ago
(In reply to Olli Pettay [:smaug] from comment #4)
> Initialize XHR using the right principal. That is why nsIXMLHttpRequest has
> [noscript] init(...).
> Only aPrincipal needs to be non-null.

Right, so I can simply include nsIXMLHttpRequest.h in xpconnect? This function is implemented and used in XPCComponents.cpp, and it's not clear to me what are the policies about including interfaces from other modules like in this case. Bobby?
(In reply to Gabor Krizsanits [:krizsa :gabor] (out until Aug 13) from comment #5)

> Right, so I can simply include nsIXMLHttpRequest.h in xpconnect? This
> function is implemented and used in XPCComponents.cpp, and it's not clear to
> me what are the policies about including interfaces from other modules like
> in this case. Bobby?

I say go for it, but Ms2ger can tell you if there's some more elegant solution.

Comment 7

5 years ago
    (In reply to Gabor Krizsanits [:krizsa :gabor] (out until Aug 13) from comment #5)
    > (In reply to Olli Pettay [:smaug] from comment #4)
    > Right, so I can simply include nsIXMLHttpRequest.h in xpconnect?
    Yes

    > This
    > function is implemented and used in XPCComponents.cpp, and it's not clear to
    > me what are the policies about including interfaces from other modules like
    > in this case. Bobby?
    Nothing "political" there. XPConnect code uses stuff from content/ all the time.
Seems strange to special-case XHR; doesn't anyone else care about the principal?

Comment 9

5 years ago
XHR is very much special cased in bug 734891
Setting branch status flags based on when bug 756277 was fixed (see comment 2). If this is incorrect please fix up the status flags accordingly.
status-firefox-esr10: --- → unaffected
status-firefox14: --- → wontfix
status-firefox15: --- → affected
status-firefox16: --- → affected
status-firefox17: --- → affected
Keywords: regression
> Er, why?

Because that's what hg blame claimed, but maybe people were just lying in their checkin comments...
Is this a significant enough security issue that we should also consider tracking for 15?
tracking-firefox16: ? → +
tracking-firefox17: ? → +
>>> Looks like a regression from bug 754202.

>>Er, why? AFAICT this is a regression from bug 756277, 

> Because that's what hg blame claimed,

Who's right? One way to determine is to see whether Firefox 15 is affected or not (754202 landed in Fx16, 756277 landed in 14).
Keywords: regressionwindow-wanted, testcase-wanted
Keywords: sec-high
(In reply to Daniel Veditz [:dveditz] from comment #13)
> Who's right? One way to determine is to see whether Firefox 15 is affected
> or not (754202 landed in Fx16, 756277 landed in 14).

I still don't quite understand what Boris is getting at. Unless I'm confused, the bug here is that sandboxes push a principal and then do_CreateInstance an XHR, expecting the XHR to use the principal from the stack. Bug 756277 explicitly changed XHR construction to ignore the context stack and use system principal.
Bobby is right.  I was trying to track down the hg blame for the change that bug 756277 made, and hg was lying to me as a result of all the backouts that happened.
(Assignee)

Comment 16

5 years ago
Created attachment 651351 [details] [diff] [review]
safe XHR creation for sandboxes

Sorry for the lag here, but I was on vacation.
Attachment #651351 - Flags: review?(bobbyholley+bmo)
Comment on attachment 651351 [details] [diff] [review]
safe XHR creation for sandboxes

> static JSBool
> CreateXMLHttpRequest(JSContext *cx, unsigned argc, jsval *vp)
> {
>     JSObject *global = JS_GetGlobalForScopeChain(cx);
>     MOZ_ASSERT(global);
> 
>-    nsCOMPtr<nsISupports> inst;
>-    nsresult rv;
>-    inst = do_CreateInstance("@mozilla.org/xmlextras/xmlhttprequest;1", &rv);
>+    JSPrincipals *jsprin = JS_GetCompartmentPrincipals(GetObjectCompartment(global));
>+    nsJSPrincipals *nsjsprin = nsJSPrincipals::get(jsprin);
>+    MOZ_ASSERT(nsjsprin);

This is pretty roundabout. How about we just call ssm->GetCxSubjectPrincipal instead?

>+      return false;
>+
>+    rv = xhr->Init(nsjsprin, NULL, NULL, uri);
>+    if (NS_FAILED(rv))
>+      return false;

Hm, why do we need to pass aBaseURI now? The old behavior implicitly leaves it null. Please check with smaug to figure out what the right thing to do there is.

r=bholley with those issues addressed.
Attachment #651351 - Flags: review?(bobbyholley+bmo) → review+
Yeah, to fix the security issue, passing null for the baseuri should be ok.

However it is possible that the XHR should use the base uri to have better behavior.
That change should be done in a different bug.
(Assignee)

Comment 19

5 years ago
https://tbpl.mozilla.org/?tree=Try&rev=79c162c06815
(Assignee)

Comment 20

5 years ago
(In reply to Olli Pettay [:smaug] from comment #18)
> However it is possible that the XHR should use the base uri to have better
> behavior.
> That change should be done in a different bug.

I can do that. Originally I wanted this feature for expanded principals specifically where this is a none issue. But since it can be used for sandboxes with any principals, this feels like the right thing to do. Although it is a bit unclear to me what difference does it make. I mean the principal should already contain that piece of information right? If there is a case where it makes a difference I'll create a bug for that and fix it.
(Assignee)

Comment 21

5 years ago
http://hg.mozilla.org/integration/mozilla-inbound/rev/d9092bbda533
https://hg.mozilla.org/mozilla-central/rev/d9092bbda533
status-firefox17: affected → fixed
Flags: in-testsuite+
Target Milestone: --- → mozilla17
Shouldn't this be resolved fixed since it was checked into mozilla-central?
Yep, just forgot to do it :)
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Has this not been nominated for uplift due to risk? Given this was unfixed in FF14/15, we likely will not take extra risk in FF16 at this point.
(Assignee)

Comment 26

5 years ago
There are two patches needed to expose this issue one is bug 734891, which lands only in FF16, so the fix would be great if could land in the same release. Do we have a guide about these patch uplifting thing? I'm not familiar with the process I'm afraid. Don't want to make the same mistake next time...
(In reply to Gabor Krizsanits [:krizsa :gabor] from comment #26)
> Do we have a guide about these patch uplifting thing? I'm not
> familiar with the process I'm afraid. Don't want to make the same mistake
> next time...

Flag the patch for approval on the relevant branches. That will pop up a "risk analysis" template that you can fill out. Make sure to indicate which branches are affected and what the risks are. But keep in mind that we're pretty late in the cycle, so stuff has to be pretty dire to land right now.
(Assignee)

Comment 28

5 years ago
(In reply to Bobby Holley (:bholley) from comment #27)
> Flag the patch for approval on the relevant branches. That will pop up a
> "risk analysis" template that you can fill out. Make sure to indicate which
> branches are affected and what the risks are. But keep in mind that we're
> pretty late in the cycle, so stuff has to be pretty dire to land right now.

Thanks, I'll keep that in mind. So in this case I think 17 is good. The jetpack API that will use and expose this feature (the XHR ctor in the sandbox) will be released right after ff17. Before that it's very unlikely that anyone will use this undocumented feature, and abuse it in a way that passes through the addon review process.
We've already gone to build with beta6 so the window on 16 landings has closed. Wontfixing for 16.
status-firefox15: affected → wontfix
status-firefox16: affected → wontfix
Whiteboard: [qa-]
Whiteboard: [qa-] → [qa-][adv-track-main17+]
Alias: CVE-2012-4205
Group: core-security
Keywords: regressionwindow-wanted, testcase-wanted
You need to log in before you can comment on or make changes to this bug.