779821 - (CVE-2012-4205) XHR created from sandboxes end up having system principal instead of principal of the sandbox

Status: RESOLVED FIXED

(Core :: XPConnect, defect)

Description

[Gabor Krizsanits (INACTIVE)]

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.

Comment 1

[Boris Zbarsky [:bzbarsky]]

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?

Comment 2

[Bobby Holley (:bholley)]

(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?

Comment 3

[Gabor Krizsanits (INACTIVE)]

(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

[Olli Pettay [:smaug][[e-mail]]]

(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.

Comment 5

[Gabor Krizsanits (INACTIVE)]

(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?

Comment 6

[Bobby Holley (:bholley)]

(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

[Olli Pettay [:smaug][[e-mail]]]

    (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.

Comment 8

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Seems strange to special-case XHR; doesn't anyone else care about the principal?

Comment 9

[Olli Pettay [:smaug][[e-mail]]]

XHR is very much special cased in bug 734891

Comment 10

[Daniel Veditz [:dveditz]]

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.

Comment 11

[Boris Zbarsky [:bzbarsky]]

> Er, why?

Because that's what hg blame claimed, but maybe people were just lying in their checkin comments...

Comment 12

[Lukas Blakk [:lsblakk] use ?needinfo]

Is this a significant enough security issue that we should also consider tracking for 15?

Comment 13

[Daniel Veditz [:dveditz]]

>>> 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).

Comment 14

[Bobby Holley (:bholley)]

(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.

Comment 15

[Boris Zbarsky [:bzbarsky]]

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.

Comment 16

[Gabor Krizsanits (INACTIVE)]

Attachment: safe XHR creation for sandboxes

Sorry for the lag here, but I was on vacation.

Comment 17

[Bobby Holley (:bholley)]

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.

Comment 18

[Olli Pettay [:smaug][[e-mail]]]

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.

Comment 19

[Gabor Krizsanits (INACTIVE)]

https://tbpl.mozilla.org/?tree=Try&rev=79c162c06815

Comment 20

[Gabor Krizsanits (INACTIVE)]

(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.

Comment 21

[Gabor Krizsanits (INACTIVE)]

http://hg.mozilla.org/integration/mozilla-inbound/rev/d9092bbda533

Comment 22

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/d9092bbda533

Comment 23

[Al Billings [:abillings - ex-MoCo]]

Shouldn't this be resolved fixed since it was checked into mozilla-central?

Comment 24

[Ryan VanderMeulen [:RyanVM]]

Yep, just forgot to do it :)

Comment 25

[Alex Keybl [:akeybl]]

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.

Comment 26

[Gabor Krizsanits (INACTIVE)]

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...

Comment 27

[Bobby Holley (:bholley)]

(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.

Comment 28

[Gabor Krizsanits (INACTIVE)]

(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.

Comment 29

[Lukas Blakk [:lsblakk] use ?needinfo]

We've already gone to build with beta6 so the window on 16 landings has closed. Wontfixing for 16.