The default bug view has changed. See this FAQ.

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

RESOLVED FIXED in Firefox 52

Status

()

Core
DOM
--
critical
RESOLVED FIXED
6 months ago
3 months ago

People

(Reporter: kanru, Assigned: Ehsan)

Tracking

({addon-compat, crash})

unspecified
mozilla52
Unspecified
Windows 8
addon-compat, crash
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(platform-rel ?, firefox52 fixed)

Details

(Whiteboard: [platform-rel-Youtube], crash signature)

Attachments

(1 attachment)

(Reporter)

Description

6 months ago
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.
Flags: needinfo?(ehsan)
(Assignee)

Comment 1

6 months ago
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/.
Assignee: nobody → ehsan
Flags: needinfo?(ehsan)
Summary: Crash in nsNodeInfoManager::SetDocumentPrincipal from XMLHttpRequestMainThread::OnStartRequest → Crash in nsNodeInfoManager::SetDocumentPrincipal from XMLHttpRequestMainThread::OnStartRequest with Youtube Subtiltle Downloader
(Assignee)

Updated

6 months ago
Blocks: 1301123
See Also: bug 1301123
(Assignee)

Comment 2

6 months ago
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?
Blocks: 1300908
Flags: needinfo?(bzbarsky)
Flags: needinfo?(bobbyholley)
(Assignee)

Comment 3

6 months ago
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?
> 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).
Flags: needinfo?(bzbarsky)
platform-rel: --- → ?
Whiteboard: [platform-rel-Youtube]
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.
Flags: needinfo?(bobbyholley)
(Assignee)

Comment 6

6 months ago
(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.
Flags: needinfo?(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".
Flags: needinfo?(bzbarsky)
(Assignee)

Comment 8

6 months ago
Marking this as addon-compat because we're going to make CORS requests from add-ons using an expanded principal not work.
Keywords: addon-compat
(Assignee)

Comment 9

6 months ago
Created attachment 8798568 [details] [diff] [review]
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.
(Assignee)

Comment 10

6 months ago
Boris, do you mind reviewing this please?  Thanks!
Flags: needinfo?(bzbarsky)
Comment on attachment 8798568 [details] [diff] [review]
Disallow CORS fetches when we have an expanded principal; r=bzbarsky

r=me
Flags: needinfo?(bzbarsky)
Attachment #8798568 - Flags: review+

Comment 12

6 months ago
Pushed by eakhgari@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/997a19a765c8
Disallow CORS fetches when we have an expanded principal; r=bzbarsky

Comment 13

6 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/997a19a765c8
Status: NEW → RESOLVED
Last Resolved: 6 months ago
status-firefox52: --- → fixed
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla52

Updated

4 months ago
Depends on: 1320201
(Assignee)

Updated

3 months ago
No longer depends on: 1320201
You need to log in before you can comment on or make changes to this bug.