1300908 - XHR can end up with an expanded loading principal

Status: RESOLVED FIXED

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

Description

[(no longer active)]

My assertion in bug 1300897 gets triggered for example under this call stack:

#0  mozilla::OriginAttributesHolder<mozilla::BasePrincipal>::GetOriginAttributes (this=0x115c02880) at BasePrincipal.h:259
#1  0x00000001007119e9 in mozilla::BasePrincipal::OriginAttributesRef (this=0x115c02868) at BasePrincipal.h:327
#2  0x00000001006f1c11 in mozilla::net::InheritOriginAttributes (aLoadingPrincipal=0x115c02868, aAttrs=@0x11c7b3328) at LoadInfo.cpp:34
#3  0x00000001006f0e32 in mozilla::net::LoadInfo::LoadInfo (this=0x11c7b32c0, aLoadingPrincipal=0x115c02868, aTriggeringPrincipal=0x0, aLoadingContext=0x0, aSecurityFlags=144, aContentPolicyType=33) at LoadInfo.cpp:190
#4  0x00000001006f1e0d in mozilla::net::LoadInfo::LoadInfo (this=0x11c7b32c0, aLoadingPrincipal=0x115c02868, aTriggeringPrincipal=0x0, aLoadingContext=0x0, aSecurityFlags=144, aContentPolicyType=33) at LoadInfo.cpp:64
#5  0x0000000100766172 in mozilla::net::nsIOService::NewChannelFromURIWithProxyFlags2 (this=0x115e62140, aURI=0x11c7c3d00, aProxyURI=0x0, aProxyFlags=0, aLoadingNode=0x0, aLoadingPrincipal=0x115c02868, aTriggeringPrincipal=0x0, aSecurityFlags=144, aContentPolicyType=33, result=0x7fff5fbf1d60) at nsIOService.cpp:870
#6  0x0000000100766072 in mozilla::net::nsIOService::NewChannelFromURI2 (this=0x115e62140, aURI=0x11c7c3d00, aLoadingNode=0x0, aLoadingPrincipal=0x115c02868, aTriggeringPrincipal=0x0, aSecurityFlags=144, aContentPolicyType=33, result=0x7fff5fbf1d60) at nsIOService.cpp:676
#7  0x0000000100780c03 in NS_NewChannelInternal (outChannel=0x11b1b60b8, aUri=0x11c7c3d00, aLoadingNode=0x0, aLoadingPrincipal=0x115c02868, aTriggeringPrincipal=0x0, aSecurityFlags=144, aContentPolicyType=33, aLoadGroup=0x0, aCallbacks=0x0, aLoadFlags=4194305, aIoService=0x115e62140) at nsNetUtilInlines.h:171
#8  0x000000010076d227 in NS_NewChannel (outChannel=0x11b1b60b8, aUri=0x11c7c3d00, aLoadingPrincipal=0x115c02868, aSecurityFlags=144, aContentPolicyType=33, aLoadGroup=0x0, aCallbacks=0x0, aLoadFlags=4194305, aIoService=0x0) at nsNetUtilInlines.h:262
#9  0x0000000104f3cadc in mozilla::dom::XMLHttpRequestMainThread::CreateChannel (this=0x11b1b6000) at XMLHttpRequestMainThread.cpp:2417
#10 0x0000000104f3c361 in mozilla::dom::XMLHttpRequestMainThread::OpenInternal (this=0x11b1b6000, aMethod=@0x7fff5fbf2650, aUrl=@0x7fff5fbf22c0, aAsync=@0x7fff5fbf2278, aUsername=@0x7fff5fbf2578, aPassword=@0x7fff5fbf24c8) at XMLHttpRequestMainThread.cpp:1532
#11 0x0000000104f3c753 in mozilla::dom::XMLHttpRequestMainThread::Open (this=0x11b1b6000, aMethod=@0x7fff5fbf2650, aUrl=@0x7fff5fbf25b0, aAsync=false, aUsername=@0x7fff5fbf2578, aPassword=@0x7fff5fbf24c8, aRv=@0x7fff5fbf2408) at XMLHttpRequestMainThread.cpp:1420
#12 0x000000010374ef00 in mozilla::dom::XMLHttpRequestBinding::open (cx=0x115ec0000, obj={<js::HandleBase<JSObject *>> = {<No data fields>}, ptr = 0x7fff5fbf2880}, self=0x11b1b6000, args=@0x7fff5fbf2828) at XMLHttpRequestBinding.cpp:406
#13 0x0000000103bcffa9 in mozilla::dom::GenericBindingMethod (cx=0x115ec0000, argc=3, vp=0x11b01b348) at BindingUtils.cpp:2812
#14 0x000000010816810d in js::CallJSNative (cx=0x115ec0000, native=0x103bcfd40 <mozilla::dom::GenericBindingMethod(JSContext*, unsigned int, JS::Value*)>, args=@0x7fff5fbf4c18) at jscntxtinlines.h:235

This is triggered from <http://searchfox.org/mozilla-central/source/js/xpconnect/tests/unit/test_allowedDomainsXHR.js#54>.  This is from a sandbox with XHR exposed to it.  In this case, the loading principal will be an expanded principal.

This makes our house of cards tremble down.  It demonstrates one way in which an expanded principal can flow through the DOM code with hard to predict implications.

Boris, Bobby, I'm out of ideas as to what to do here.  Specifically, what OA should be used when we are in this situation??

Comment 1

[Bobby Holley (:bholley)]

Ah yeah, I guess this is one of those 'DOM constructors exposed to sandboxes" cases. This was actually the original motivation for nsEP IIRC (allowing jetpack content scripts to load from multiple domains), but I'm not sure if it's actually used in the current SDK - would need to ask someone.

This does seem like a problem. The solutions I can think of are:
(a)  Have the machinery figure out which sub-principal was actually allowed to perform the load, and then use that (not the nsEP) as the loading principal on the channel.
(b) Check if we have any addon SDK code that actually relies on creating XHRs for nsEPs, and remove this feature if not.
(c) Teach the Necko machinery how to deal with an nsEP loadingPrincipal.

Preferably not (c).

Comment 2

[Boris Zbarsky [:bzbarsky]]

Can we take a step back and figure out whether we can do anything to associate origin attributes with expanded principals in saner ways?  Because this game of whack-a-mole is not going to end well.  :(

Comment 3

[Bobby Holley (:bholley)]

(In reply to Boris Zbarsky [:bz] from comment #2)
> Can we take a step back and figure out whether we can do anything to
> associate origin attributes with expanded principals in saner ways?

I don't think there's an obvious algebra of how to combine OAs. For example, suppose an nsEP has two principals, one with a PB id and one without (or with another PB id). What should the PB id of the nsEP be?

I suppose we could try requiring that all nsEP principals have precisely the same OAs, and then hoisting that onto the nsEP. This would be a new API restriction that could break addons, but might work. Worth trying.

>  Because
> this game of whack-a-mole is not going to end well.  :(

OAs aside, do we believe that Necko is capable of handling an nsEP for the loading principal?

Comment 4

[Boris Zbarsky [:bzbarsky]]

> OAs aside, do we believe that Necko is capable of handling an nsEP for the loading principal?

Loading or triggering?

The loading principal is pretty much used only for security checks (CheckLoadURI) and maybe content policy stuff addons do.  I expect an nsEP there is fine.

The triggering principal is used for those _and_ the channel result principal when the "inherit" flag is set.  That seems fishier.

A concrete question: if an nsEP sandbox does an XHR, what should the principal of the resulting responseXML document be?

What if it does a location.href navigation to a data: or about:blank URL?  What about a javascript: URL that returns a nonempty string?

Comment 5

[(no longer active)]

(In reply to Bobby Holley (PTO through 9/19/2016) (busy with Stylo) from comment #3)
> (In reply to Boris Zbarsky [:bz] from comment #2)
> > Can we take a step back and figure out whether we can do anything to
> > associate origin attributes with expanded principals in saner ways?
> 
> I don't think there's an obvious algebra of how to combine OAs. For example,
> suppose an nsEP has two principals, one with a PB id and one without (or
> with another PB id). What should the PB id of the nsEP be?

Do we have the option of rejecting to create such principals?

> I suppose we could try requiring that all nsEP principals have precisely the
> same OAs, and then hoisting that onto the nsEP. This would be a new API
> restriction that could break addons, but might work. Worth trying.

Should we try to do that instead of this whack a mole game?  I'm asking because I have a limited amount of time to devote to this (basically only this week) before having to travel for ~10 days.

Comment 6

[(no longer active)]

(In reply to Boris Zbarsky [:bz] from comment #4)
> > OAs aside, do we believe that Necko is capable of handling an nsEP for the loading principal?
> 
> Loading or triggering?
> 
> The loading principal is pretty much used only for security checks
> (CheckLoadURI) and maybe content policy stuff addons do.

And also to inherit the OAs from, which is the problematic case in this bug: <http://searchfox.org/mozilla-central/source/netwerk/base/LoadInfo.cpp#190>

Comment 7

[Boris Zbarsky [:bzbarsky]]

I have no idea what loadinfo's origin attributes are used for....  I guess the idea is to tell which bucket the load is happening in.  And an XHR load in an nsEP extension sandbox really isn't happening in any particular bucket, conceptually....

Comment 8

[(no longer active)]

(In reply to Boris Zbarsky [:bz] from comment #7)
> I have no idea what loadinfo's origin attributes are used for....

They are exposed through nsILoadInfo, and can be consumed by any consumer for any purpose.

> I guess
> the idea is to tell which bucket the load is happening in.  And an XHR load
> in an nsEP extension sandbox really isn't happening in any particular
> bucket, conceptually....

Hmm, not quite following the bucketing analogy.  Especially since loadInfo's OAs can be modified by consumers to modify how loads are performed.

Comment 9

[Boris Zbarsky [:bzbarsky]]

Ah, I thought the point was to examine which context a load is happening in.  If the loadinfo's OAs are mutable, then that's clearly not the (only?) point....

Comment 10

[(no longer active)]

(In reply to Boris Zbarsky [:bz] from comment #9)
> Ah, I thought the point was to examine which context a load is happening in.

What do you mean by "context" here?  If you mean the user context ID, that's _one_ of the things that we need to get OAs right for it to work correctly.

> If the loadinfo's OAs are mutable, then that's clearly not the (only?)
> point....

Not sure if I follow this part...

Comment 11

[Boris Zbarsky [:bzbarsky]]

> What do you mean by "context" here? 

I mean the user context id, private browsing state, etc.  Basically, in my head OA is (perhaps incorrectly?) a mechanism for bucketing otherwise-identical principals/origins/stuff-that-depends-on-origin into separate buckets.  For necko in particular, presumably cookies are added based on the OA for the load, but I can also see us having the cache taking OA into account and whatnot...

> Not sure if I follow this part...

For purposes of "which cookies should I add to the load?" the OA in the LoadInfo doesn't need to be mutable: it would just be set up when the loadinfo is created.  If we allow mutating the OA in the LoadInfo, then presumably we have use cases for effectively "moving" a load from one bucket (in the sense above) to another.  I have no idea what those use cases are, though.

Comment 12

[(no longer active)]

Sometimes setters are trying to copy another OA: <http://searchfox.org/mozilla-central/source/browser/extensions/pdfjs/content/PdfStreamConverter.jsm#1012>

And sometimes they're trying to modify the "bucket" as you described above: <http://searchfox.org/mozilla-central/source/netwerk/test/unit/test_separate_connections.js#30>

This setter was added in bug 1175685, FWIW.

Comment 13

[Bobby Holley (:bholley)]

We had some discussions about this the week before last, let me know if other information is needed here.

Comment 14

[Andrew Overholt [:overholt]]

Do we have a plan of what to do here?

Comment 15

[(no longer active)]

I just closed bug 1297687.  With that and the new fix to bug 1297687, I don't think we need to do anything else here.  Bobby, do you agree?

Comment 16

[Bobby Holley (:bholley)]

(In reply to :Ehsan Akhgari from comment #15)
> I just closed bug 1297687.  With that and the new fix to bug 1297687

Those are the same bug.

>, I
> don't think we need to do anything else here.  Bobby, do you agree?

I'm not sure - is the intention to declare that it's OK to have an nsEP loading principal?

Comment 17

[(no longer active)]

(In reply to Bobby Holley (:bholley) (busy with Stylo) from comment #16)
> (In reply to :Ehsan Akhgari from comment #15)
> > I just closed bug 1297687.  With that and the new fix to bug 1297687
> 
> Those are the same bug.
> 
> >, I
> > don't think we need to do anything else here.  Bobby, do you agree?
> 
> I'm not sure - is the intention to declare that it's OK to have an nsEP
> loading principal?

Let me clarify.  There are different things that people care about here.

* In bug 1297687 what _I_ cared about was to ensure that all expanded principals have a "sane" OA.  We sort of achieved that by banning passing more than one principal with unequal OAs, using the OA of a passed principal when creating codebase principals from the string arguments, and also letting the caller specify the desired OA explicitly.  That fix reduces the risk of EPs flowing through the system for things that rely on OAs being accurate, such as private browsing and containers.  I don't think there is anything more to care about.

* There's also the orthogonal question of whether it's OK to have expanded principals appear as loading principals in general.  The reason I was arguing before that this isn't OK was that EPs could have inaccurate OAs, but now that this has been fixed, I don't personally see a huge downside besides it being nicer if we didn't have to think about EPs appear as document principals (them appearing as the loading principal here exposes them to a smaller surface than if they appear as a document principal.)  I'm fine to close this as WONTFIX if you think that the current setup is good enough.

More on comment 1, this feature is definitely used in the add-on SDK: <http://searchfox.org/mozilla-central/source/addon-sdk/source/lib/sdk/content/sandbox.js#137>.  I haven't checked which one of the other global properties constructors can also lead into similar issues, but based on pushing a patch for bug 1301123 to the try server, it seems that XHR is the only case where we can end up with an EP as a document principal.  I personally think it would be nice if we at least just fixed that, by picking the original principal in the whitelist that makes CheckMayLoad pass.  The simplest way for that would be to use that as the loading principal from the get go.

Comment 18

[(no longer active)]

Attachment: Avoid using expanded principals as the loading principal of XHR

Comment 19

[Bobby Holley (:bholley)]

Comment on attachment 8794278 [details] [diff] [review]
Avoid using expanded principals as the loading principal of XHR

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

This is awesome!

This looks great conceptually, though I think smaug is probably a better reviewer for the network mechanics.

Once we have this, can we also land a release-mode assert against an nsEP document principal?

Comment 20

[(no longer active)]

(In reply to Bobby Holley (:bholley) (busy with Stylo) from comment #19)
> Once we have this, can we also land a release-mode assert against an nsEP
> document principal?

Yes, I was going to do that in bug 1301123.  These two together seem to be green on try.

Do we *really* want a release mode assertion?

Comment 21

[Bobby Holley (:bholley)]

(In reply to :Ehsan Akhgari from comment #20)
> (In reply to Bobby Holley (:bholley) (busy with Stylo) from comment #19)
> > Once we have this, can we also land a release-mode assert against an nsEP
> > document principal?
> 
> Yes, I was going to do that in bug 1301123.  These two together seem to be
> green on try.
> 
> Do we *really* want a release mode assertion?

The most ideal thing, I think, would be to make it a MOZ_DIAGNOSTIC_ASSERT, and add some Telemetry as well. If the telemetry looks good on release, we can convert it to MOZ_RELEASE_ASSERT.

Comment 22

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

Comment on attachment 8794278 [details] [diff] [review]
Avoid using expanded principals as the loading principal of XHR

I tried and failed to understand why nsILoadInfo::SEC_REQUIRE_SAME_ORIGIN_DATA_INHERITS is there.
I guess you just added it there because *_INHERITS, even though secFlags never should have it.
Could you perhaps remove it and instead assert that secFlags never has it.

Comment 23

[(no longer active)]

Yeah, you guessed right.  :-)  I'll add the assertion.

Comment 24

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/fe4efa592a1c
Avoid using expanded principals as the loading principal of XHR; r=smaug

Comment 25

[Carsten Book [:Tomcat]]

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