1307730 - Crash in nsNodeInfoManager::SetDocumentPrincipal from XMLHttpRequestMainThread::OnStartRequest with Youtube Subtiltle Downloader

Status: RESOLVED FIXED

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

Description

[Kan-Ru Chen [:kanru] (UTC+9)]

This bug was filed from the Socorro interface and is 
report bp-55793737-abbb-4e98-820c-a88362161004.
=============================================================

A startup crash from single installation.
Ehsan, you added this MOZ_DIAGNOSTIC_ASSERT so you might want to take a look.

Comment 1

[(no longer active)]

Hmm, I can reproduce this by installing <https://addons.mozilla.org/en-US/firefox/addon/youtube-subtitle-downloader/> and visiting a Youtube video.

The expanded principal in question has two codebase principals inside it, the first one is the codebase principal associated with the youtube page, and the second one is a codebase principal for moz-extension://ff7ba6dc-7090-dd4d-81e2-723cfc97aebf/.

Comment 2

[(no longer active)]

What's happening here is that the add-on is trying to load a resource from https://video.google.com/, and the CheckMayLoad check on the expanded principal fails, and we end up doing a CORS fetch with |Origin: null|, coming from <http://searchfox.org/mozilla-central/source/netwerk/protocol/http/nsCORSListenerProxy.cpp#961>, since nsContentUtils::GetASCIIOrigin() throws up its hands and returns "null" if the principal passed to it is an expanded principal which returns null from its GetURI() function.  The server happily accepts our |Origin: null| header and responds with a |Access-Control-Allow-Origin: null| header, so the CORS checks pass and we proceed to XMLHttpRequestMainThread::OnStartRequest() where we end up calling NS_NewDOMDocument() and then fatally assert.

This makes it clear that our basic idea in bug 1300908 wasn't correct, since it's possible for none of the whitelisted principals in the expanded principal to have access to load the XHRed URL in case the fetch is allowed through CORS.

Unfortunately I can't think of a good way to fix this.  Even if we remember that the load was allowed through CORS, by the time we are about to use the channel's resulting principal, we still have the problem of none of the whitelisted principals having access to the document.  I can't really think of any solutions besides picking the first whitelisted principal or something like that, but I'm not sure if that's sensible.

Bobby, Boris, what do you think?

Comment 3

[(no longer active)]

Also the fact that we end up making a CORS request with |Origin: null| is pretty broken in and of itself.  Are expanded principals supposed to be able to perform CORS fetches?  Should we close this hole?

Comment 4

[Boris Zbarsky [:bzbarsky]]

> I can't really think of any solutions besides picking the first whitelisted principal or something like that

That seems like the simplest option if we want to allow this load at all.

It seems like expanded principals should be allowed to perform CORS fetches in general, just like web pages should be able to.  But if not, we could in fact close it off (e.g. by inspecting the response state of the returned thing and dropping it on the floor if it was only allowed due to CORS).

Comment 5

[Bobby Holley (:bholley)]

I don't have any great ideas here. However, AFAIK nothing in the codebase thus far treats the order of principals in an nsEP as significant, and it seems worth trying to preserve that. So I might prefer to just disallow the CORS load from XHR-in-nsEP. For things like WebExt the code can/should use the web page's XHR anyway.

Comment 6

[(no longer active)]

(In reply to Boris Zbarsky [:bz] (still a bit busy) from comment #4)
> > I can't really think of any solutions besides picking the first whitelisted principal or something like that
> 
> That seems like the simplest option if we want to allow this load at all.
> 
> It seems like expanded principals should be allowed to perform CORS fetches
> in general, just like web pages should be able to.

I can't imagine how that would work though.  By definition, such code won't really have an origin (in the HTML sense of the term), which we do need to have in order to perform a CORS.  Arguably if Youtube's CORS implementation actually checked the requesting origin, it should've rejected the CORS request.

Can you think of how we'd get an origin string when we have an expanded principal?

> But if not, we could in
> fact close it off (e.g. by inspecting the response state of the returned
> thing and dropping it on the floor if it was only allowed due to CORS).

Hmm, so this means sending a CORS request with |Origin: null| or something, and then just pretending as if the request didn't happen?  If we want to break CORS requests, why not just return NS_ERROR_DOM_BAD_URI here <http://searchfox.org/mozilla-central/source/netwerk/protocol/http/nsCORSListenerProxy.cpp#955> when we detect "null" being returned, and treat this the same way as any other CORS failure (and not send out the request in the first place)?


After thinking about this a bit, and given comment 5's great point around the order of the whitelist items not mattering, I'm really inclined to break CORS requests when we detect an expanded principal.

Comment 7

[Boris Zbarsky [:bzbarsky]]

> I can't imagine how that would work though.

The only reasonable use case is for "Access-Control-Allow-Origin: *" resources.  I agree Youtube is just broken here.  ;)

> and then just pretending as if the request didn't happen? 

Yes.

> why not just return NS_ERROR_DOM_BAD_URI here <http://searchfox.org/mozilla-central/source/netwerk/protocol/http/nsCORSListenerProxy.cpp#955> when we detect "null" being returned,

Because that would web-observably break XHR from a sandboxed iframe to an "Access-Control-Allow-Origin: *" resource.

> I'm really inclined to break CORS requests when we detect an expanded principal.

I can probably live with that, if we scope it down to expanded principals, not all cases where the origin serialization is "null".

Comment 8

[(no longer active)]

Marking this as addon-compat because we're going to make CORS requests from add-ons using an expanded principal not work.

Comment 9

[(no longer active)]

Attachment: Disallow CORS fetches when we have an expanded principal; r=bzbarsky

It's not possible to construct a useful Origin header when we have
an expanded principal and are about to perform a CORS fetch.  Therefore,
instead of sending a CORS fetch with an |Origin: null| header, we
must fail the request.

Comment 10

[(no longer active)]

Boris, do you mind reviewing this please?  Thanks!

Comment 11

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8798568 [details] [diff] [review]
Disallow CORS fetches when we have an expanded principal; r=bzbarsky

r=me

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/997a19a765c8
Disallow CORS fetches when we have an expanded principal; r=bzbarsky

Comment 13

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/997a19a765c8