1291967 - Firefox Aurora crashes on github site when clicking certain links

Status: RESOLVED DUPLICATE of bug 1298505

(Core :: DOM: Security, defect, P1)

Description

[Karl Dubost💡 :karlcow]

Preliminary comments:

1. Using Firefox Aurora 49.0a2 (2016-08-01) (if I remember I have this behavior since the start of Firefox 49). 
2. It is reproducible all the time. 
3. Initially I thought it was due to addons, but the problem persists with all add-ons being disabled. 
4. Profile with ~300 tabs

Steps to reproduce:

1. Open a github.com project page
   ex: https://github.com/webcompat/webcompat.com
2. Click on Issues
   https://github.com/webcompat/webcompat.com/issues

Expected:
goes to the issues list

Actual:
Instant crash


Crash signature example:
https://crash-stats.mozilla.com/report/index/4f332d56-a9b9-4382-bb60-c70842160804


The HTML doesn't seem to have anything special without specific events attached to it. 

```
<span itemscope="" itemtype="http://schema.org/ListItem" itemprop="itemListElement">
      <a href="/webcompat/webcompat.com/issues" class="js-selected-navigation-item reponav-item" data-hotkey="g i" data-selected-links="repo_issues repo_labels repo_milestones /webcompat/webcompat.com/issues" itemprop="url">
        <svg aria-hidden="true" class="octicon octicon-issue-opened" height="16" version="1.1" viewBox="0 0 14 16" width="14"><path d="M7 2.3c3.14 0 5.7 2.56 5.7 5.7s-2.56 5.7-5.7 5.7A5.71 5.71 0 0 1 1.3 8c0-3.14 2.56-5.7 5.7-5.7zM7 1C3.14 1 0 4.14 0 8s3.14 7 7 7 7-3.14 7-7-3.14-7-7-7zm1 3H6v5h2V4zm0 6H6v2h2v-2z"></path></svg>
        <span itemprop="name">Issues</span>
        <span class="counter">140</span>
        <meta itemprop="position" content="2">
</a>    </span>
```

Comment 1

[Karl Dubost💡 :karlcow]

Ok I closed the 300 tabs after backing them up. 
restarted Aurora, with a single window, one tab, 
erased my github cookies
went to github, logged in, and did the same steps. 

Crash again, this time on one of the issues. 
https://crash-stats.mozilla.com/report/index/a9ff894d-36c8-481e-b9fe-a18292160804

Comment 2

[:Gijs (he/him)]

(Requests:
- Please file bugs as soon as you see them... 49 is getting close to release by now and we'll need to uplift any fixes to beta - but you said you've been seeing this for a long time?
- Fx::Untriaged gets more love than Fx::General, so it's generally the better place to file if you don't know a specific product/component your issue belongs to.)

FWIW, I can't trivially reproduce myself. I expect there's still a setting in your profile that's somehow different. Can you attach about:support info to help others figure out how to reproduce ?


nullptr-crashes in CSP land (looks like the CSPContext's mLoadingPrincipal is null, and so aProtectedResourcePrincipal is null and so trying to call aProtectedResourcePrincipal->CheckMayLoad doesn't work?) look like something I can ask :ckerschb about...

[Tracking Requested - why for this release]:
I'm assuming this didn't happen in 48, so it's probably a regression, and a crash, so we should track for 49.

Comment 3

[Karl Dubost💡 :karlcow]

1. Navigating through links on GitHub. No sign-in. clean profile: Firefox 50.0a2 (2016-08-04). NO CRASH
2. Navigating through links on GitHub. No sign-in. clean profile: Firefox 50.0a2 (2016-08-04). NO CRASH
3. Navigating through links on GitHub. signed-in. clean profile: Firefox 50.0a2 (2016-08-04). NO CRASH
4. Profile with 304 tabs (45 tabs on github.com domain). Already logged-in. Opening a new tab to https://github.com/webcompat/webcompat.com/ Clicking on issues. INSTANT CRASH. https://crash-stats.mozilla.com/report/index/b83b97ae-9eb4-4967-9ed9-5cf8e2160814
5. Profile with 304 tabs (45 tabs on github.com domain). Already logged-in. Opening a new tab to https://github.com/webcompat/webcompat.com/. SIGN-OUT. Clicking on issues. INSTANT CRASH. https://crash-stats.mozilla.com/report/index/bp-7e4a4f2b-7864-4d1b-969d-003002160815


Some other crash reports.
https://crash-stats.mozilla.com/report/index/bp-a8814171-075d-45a9-8c6b-37f672160814
https://crash-stats.mozilla.com/report/index/bp-735aa49c-bef6-4207-96a5-b3a892160814
https://crash-stats.mozilla.com/report/index/bp-72fccf6c-8770-43e0-8627-d0d862160814

Hope it helps. Tell me if I can do anything more.

Comment 4

[Karl Dubost💡 :karlcow]

Attachment: uMatrix-github.png

My normal profile has uBlock and uMatrix (see screenshot) on.

Breaking on click on dev tools and clicking on "issues".

https://assets-cdn.github.com/assets/github-5a05539a2ab04f10abd2879546db58882d74e2789b0164a23ac4bfdb431a461f.js

  window.addEventListener('click', function (i) {
    var n = void 0;
    if (i.target.closest && (n = i.target.closest('[data-ga-click]'))) {
      var o = e.extractEventParams(n.getAttribute('data-ga-click'));
      t.trackEvent(o)
    }
  }, !0)


And this message in the console:

> Security wrapper denied access to property (void 0) 
> on privileged Javascript object. Support for exposing 
> privileged objects to untrusted content via __exposedProps__ 
> is being gradually removed - use WebIDL bindings or 
> Components.utils.cloneInto instead. Note that only the 
> first denied property access from a given global object 
> will be reported.

Comment 5

[Karl Dubost💡 :karlcow]

ok going on a step-by-step in devtools. 
it starts with n = void 0, then two steps forward n = null, and it crashes.
https://crash-stats.mozilla.com/report/index/bp-e8eaa0ed-937b-4a6c-a83e-c0e8d2160815

Comment 6

[:Gijs (he/him)]

(In reply to Karl Dubost :karlcow from comment #4)
> Created attachment 8780949 [details]
> uMatrix-github.png
> 
> My normal profile has uBlock and uMatrix (see screenshot) on.
> 
> Breaking on click on dev tools and clicking on "issues".
> 
> https://assets-cdn.github.com/assets/github-
> 5a05539a2ab04f10abd2879546db58882d74e2789b0164a23ac4bfdb431a461f.js
> 
>   window.addEventListener('click', function (i) {
>     var n = void 0;
>     if (i.target.closest && (n = i.target.closest('[data-ga-click]'))) {
>       var o = e.extractEventParams(n.getAttribute('data-ga-click'));
>       t.trackEvent(o)
>     }
>   }, !0)
> 
> 
> And this message in the console:
> 
> > Security wrapper denied access to property (void 0) 
> > on privileged Javascript object. Support for exposing 
> > privileged objects to untrusted content via __exposedProps__ 
> > is being gradually removed - use WebIDL bindings or 
> > Components.utils.cloneInto instead. Note that only the 
> > first denied property access from a given global object 
> > will be reported.

Uhhh. That's... surprising. Bobby, how on earth would this happen? "void 0" isn't supposed to be a property ("void" is an operator, and even setting window.void to something doesn't override that lookup in my testing), and none of github's code should be getting its hands on a privileged JS object. Maybe uBlock or uMatrix are doing something they shouldn't be? :-\

Marking sec-sensitive until we know there isn't a security issue here.

Comment 7

[Bobby Holley (:bholley)]

(In reply to :Gijs Kruitbosch (PTO recovery mode) from comment #6)
> (In reply to Karl Dubost :karlcow from comment #4)
> > Created attachment 8780949 [details]
> > uMatrix-github.png
> > 
> > My normal profile has uBlock and uMatrix (see screenshot) on.
> > 
> > Breaking on click on dev tools and clicking on "issues".
> > 
> > https://assets-cdn.github.com/assets/github-
> > 5a05539a2ab04f10abd2879546db58882d74e2789b0164a23ac4bfdb431a461f.js
> > 
> >   window.addEventListener('click', function (i) {
> >     var n = void 0;
> >     if (i.target.closest && (n = i.target.closest('[data-ga-click]'))) {
> >       var o = e.extractEventParams(n.getAttribute('data-ga-click'));
> >       t.trackEvent(o)
> >     }
> >   }, !0)
> > 
> > 
> > And this message in the console:
> > 
> > > Security wrapper denied access to property (void 0) 
> > > on privileged Javascript object. Support for exposing 
> > > privileged objects to untrusted content via __exposedProps__ 
> > > is being gradually removed - use WebIDL bindings or 
> > > Components.utils.cloneInto instead. Note that only the 
> > > first denied property access from a given global object 
> > > will be reported.
> 
> Uhhh. That's... surprising. Bobby, how on earth would this happen? "void 0"
> isn't supposed to be a property ("void" is an operator, and even setting
> window.void to something doesn't override that lookup in my testing)

That's just the string we use to represent "undefined" in jsval stringification:

http://searchfox.org/mozilla-central/rev/9ec085584d7491ddbaf6574d3732c08511709172/js/src/jsstr.cpp#3100

Which gets called here:

http://searchfox.org/mozilla-central/rev/9ec085584d7491ddbaf6574d3732c08511709172/js/xpconnect/wrappers/XrayWrapper.cpp#202

, and
> none of github's code should be getting its hands on a privileged JS object.
> Maybe uBlock or uMatrix are doing something they shouldn't be? :-\
> 
> Marking sec-sensitive until we know there isn't a security issue here.

I'm certainly curious as to how a privileged object is ending up in content scope, and it's probably worth looking into. This message does nonetheless indicate that our security wrappers are doing their duty as a safety-net.

Comment 8

[Christoph Kerschbaumer [:ckerschb]]

Unfortunately I can't reproduce the crash using mc/aurora/beta channels. When calling StripURIForReporting() the argument aProtectedResourcePrincipal should never be null. The only time we set the principal to null within CSP is when we destruct the principal itself [1]. But that only happens when we tear down the page.

What's interesting though is that StripURIForReporting() is only called when CSP blocks resource loads. I visited https://github.com/webcompat/webcompat.com/ and clicked ISSUES. No CSP errors are reported on that page. Are you sure you have no addons installed which might trigger the CRASH?

[1] https://dxr.mozilla.org/mozilla-central/source/dom/security/nsCSPContext.h#80

Comment 9

[:Gijs (he/him)]

(In reply to Christoph Kerschbaumer [:ckerschb] from comment #8)
>  Are you sure you have no addons installed which might trigger the CRASH?

(In reply to Karl Dubost :karlcow from comment #4)
> Created attachment 8780949 [details]
> uMatrix-github.png
> 
> My normal profile has uBlock and uMatrix (see screenshot) on.

Comment 10

[Karl Dubost💡 :karlcow]

Restarted in Safe Mode (to disable add-ons). I can reproduce exactly the same issue.
https://crash-stats.mozilla.com/report/index/a0145786-d47b-48b7-bca6-2a0292160817

Comment 11

[Christoph Kerschbaumer [:ckerschb]]

Kamil, Matt, can you try to help me reproduce the problem? I tried several times (see comment 8) on Mac and Linux and I can't reproduce. Sending CSP reports is commonly used on a variety of sites. It's odd that it only causes problems on Github, but if it is really an issue we should get it fixed.

To reproduce:
(a) Visit https://github.com/webcompat/webcompat.com/
(b) click: ISSUES
(c) Expect CRASH (potentially repeart steps (a) and (b))
(d) Also keep an eye for possible browser console messages (not web console) regarding CSP messages.

Comment 12

[:Gijs (he/him)]

(In reply to Karl Dubost :karlcow from comment #10)
> Restarted in Safe Mode (to disable add-ons). I can reproduce exactly the
> same issue.
> https://crash-stats.mozilla.com/report/index/a0145786-d47b-48b7-bca6-
> 2a0292160817

Can you post your about:support (Help > Troubleshooting Information) content? Maybe it depends on some kind of pref you've flipped.

Comment 13

[Matt Wobensmith [:mwobensmith][:matt:]]

(In reply to Christoph Kerschbaumer [:ckerschb] from comment #11)
> Kamil, Matt, can you try to help me reproduce the problem? 


I followed those steps on a selection of configs and saw no crash. I did see CSP errors in the browser console, which differed by release version. Those are below.

Mac Fx48:
Content Security Policy: The page's settings blocked the loading of a resource at about:blank ("base-uri https://github.com").  (unknown)
Content Security Policy: Directive ‘frame-src’ has been deprecated. Please use directive ‘child-src’ instead.   (unknown)


Mac Fx49/Fx50/Fx51, Linux Fx50/Fx51, Windows 8 Fx50
Content Security Policy: Directive ‘frame-src’ has been deprecated. Please use directive ‘child-src’ instead.   (unknown)

Comment 14

[Karl Dubost💡 :karlcow]

Attachment: firefox-profile.txt

This is the about:support information.
I only edited the name of the profile folder.

Comment 15

[Ethan Tseng [:ethan]]

Dan can reproduce this bug.
https://crash-stats.mozilla.com/report/index/1427c4b4-4818-4260-aab8-a05742160823

Comment 16

[Christoph Kerschbaumer [:ckerschb]]

Dan was able to crash with the same signature:
https://crash-stats.mozilla.com/report/index/1427c4b4-4818-4260-aab8-a05742160823

Comment 17

[Kamil Jozwiak [:kjozwiak]]

Removing myself, as per comment#15, looks like Dan has been able to reproduce the crash.

Comment 18

[Christoph Kerschbaumer [:ckerschb]]

Unfortunately I was only able to reproduce the exact issue from comment 15 once, but it allowed me to perform my own debugging and to add some assertions so I can provide more context.

First, the problem appears only on e10s, and only if the CSP uses a report-uri directive, which is probably also the reason we haven't gotten so many bug reports as of now. The problem starts here [1] where we serialize the principal. Worth mentioning is also that the principal holds a pointer to the CSP and the CSP holds a pointer to the principal. We use that principal as the loadingPrincipal for sending CSP reports. To break up that cycle we added a function called clearLoadingPrincipal() [2] which we call in the destructor of  the principal to avoid mem leakage.

I thought that Principal::Read() and ::Write() is only used for SessionRestore, but apparently not. So now it gets tricky, and I am not entirely sure this is what happens but it's my guess:

(1) The site creates a principal and a CSP which hangs of that principal.
(2) The site serializes the principal here [1] and when we deserialize the principal [3] we overwrite the CSP's principal with the serialized principal [4] because we have to call SetReqeustContext() after deserialization.
(3) That temporary (serialized) principal now gets destructed and within its destructor calls clearLoadingPrincipal() on the CSP.
(4) We encounter a CSP error and send a CSP report, the principal is now null and we crash within StripURIForReporting() [5].

As a first measure we could add a null check for the principal within StripURIForReporting() which would at least prevent crashes, but that's obviously not the right solution. I honestly don't know how to handle because within nsPrincipal::Read() we can't distinguish what kind of serialization we perform, sessionstore/restore or something else.

Smaug, any idea how we could handle that case?

[1] https://hg.mozilla.org/mozilla-central/rev/c27b0ab05958#l1.84
[2] https://hg.mozilla.org/mozilla-central/rev/acc983ca0dec#l6.15
[3] https://dxr.mozilla.org/mozilla-central/source/caps/nsPrincipal.cpp#408
[4] https://dxr.mozilla.org/mozilla-central/source/dom/security/nsCSPContext.cpp#652
[5] https://dxr.mozilla.org/mozilla-central/source/dom/security/nsCSPContext.cpp#754

Comment 19

[Gerry Chang [:gchang]]

Un-track for 49 as the volume of crash number in 49 beta is low.

Comment 20

[Daniel Veditz [:dveditz]]

Looks like the mLoadingPrincipal is only used in reporting. Did I miss a reference?

It must be valid when set because you're able to pull mSelfURI off it without crashing.

It can be used in SendReports when creating a channel, but being set to null should be OK?

It's used in StripURIForReporting as a same-origin check, but CheckMayLoad() is NOT a same-origin check! Any URL of the same scheme will be allowed, for instance. This should have been ->Equals() or ->Subsumes() (not that either would fix the crash). At their core, though, after other complicated checks those just call NS_SecurityCompareURIs(). You have mSelfURI already and are comparing against a URI so just call that.

Comment 21

[Daniel Veditz [:dveditz]]

Filed bug 1298505 on the CheckMayLoad bit because you might find a different solution to this crash. Or you can fix both with one patch (using NS_SecurityCompareURIs) and have that one "depend on" this one.

Comment 22

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

Sounds like my needinfo isn't needed here.

Comment 23

[Christoph Kerschbaumer [:ckerschb]]

Karl, thanks again for reporting the Bug. The fix just landed within Bug 1298505. I don't know if you are willing to build mozilla-central and could verify that the issue is fixed. Alternatively you could download and use tomorrow's Nightly. Either way, would be great if you could verify the issue to be fixed. Thanks!

Comment 24

[Karl Dubost💡 :karlcow]

Tested with Firefox Nightly 51.0a1 (2016-08-31)
https://crash-stats.mozilla.com/report/index/b3613ef1-16df-43b0-a3b5-6a5342160901

Compiled mozilla-central.
76:12.40 We know it took a while, but your build finally finished successfully!

Then installing the profile I usually use where the crash is happening.
And starting.

Get a restore session with some slow script for initializing the 300 tabs.
Eventually it starts. 51.0a1 (2016-09-01)

Among noticeable error messages hundred of times for each actions:

[Parent 58126] ###!!! ASSERTION: Child frames aren't sorted correctly: '(!mFrames.IsEmpty() && mFrames.FirstChild()->GetContent()->GetContainingShadow()) || nsIFrame::IsFrameListSorted<IsOrderLEQWithDOMFallback>(mFrames)', file /Users/karl/code/mozilla-central/layout/generic/nsFlexContainerFrame.cpp, line 2164
[Parent 58126] WARNING: '!IsSelectionValid()', file /Users/karl/code/mozilla-central/widget/ContentCache.cpp, line 651
[Parent 58126] WARNING: '!aEvent.mSucceeded', file /Users/karl/code/mozilla-central/dom/ipc/TabParent.cpp, line 2183


Loading the github.com/webcompat/webcompat.com page
with more error messages including these ones where I replaced the values of the cookies to "*******"

[Child 58129] WARNING: Fetch ignoring illegal header - 'Set-Cookie': 'user_session=******; path=/; expires=Thu, 15 Sep 2016 02:47:58 -0000; secure; HttpOnly
_gh_sess=*******
[Child 58129] WARNING: Cannot know response Content-Length due to presence of Content-Encoding or Transfer-Encoding headers.: file /Users/karl/code/mozilla-central/dom/fetch/FetchDriver.cpp, line 519
[Child 58129] WARNING: NS_ENSURE_SUCCESS(status, status) failed with result 0x804B0002: file /Users/karl/code/mozilla-central/netwerk/protocol/http/nsCORSListenerProxy.cpp, line 553
[Parent 58126] WARNING: NS_ENSURE_SUCCESS(rv, rv) failed with result 0x80070057: file /Users/karl/code/mozilla-central/netwerk/base/nsChannelClassifier.cpp, line 85
[Parent 58126] WARNING: NS_ENSURE_SUCCESS(rv, rv) failed with result 0x80070057: file /Users/karl/code/mozilla-central/netwerk/base/nsChannelClassifier.cpp, line 85
[Child 58129] WARNING: blocked access to response header: file /Users/karl/code/mozilla-central/dom/xhr/XMLHttpRequestMainThread.cpp, line 1069
[Child 58129] WARNING: blocked access to response header: file /Users/karl/code/mozilla-central/dom/xhr/XMLHttpRequestMainThread.cpp, line 1069


Proceeding on clicking on issues


*** NO CRASH **** Yoohoo!


After removing the usual messages mentioned above and keeping the ones which seem to be related.

[Child 58129] WARNING: blocked access to response header: file /Users/karl/code/mozilla-central/dom/xhr/XMLHttpRequestMainThread.cpp, line 1069
[Child 58129] WARNING: blocked access to response header: file /Users/karl/code/mozilla-central/dom/xhr/XMLHttpRequestMainThread.cpp, line 1069
[Parent 58126] WARNING: NS_ENSURE_SUCCESS(rv, rv) failed with result 0x805E0006: file /Users/karl/code/mozilla-central/dom/security/nsContentSecurityManager.cpp, line 445
[Parent 58126] WARNING: NS_ENSURE_SUCCESS(rv, rv) failed with result 0x805E0006: file /Users/karl/code/mozilla-central/netwerk/protocol/http/nsHttpChannel.cpp, line 5675
[Child 58129] WARNING: Cannot know response Content-Length due to presence of Content-Encoding or Transfer-Encoding headers.: file /Users/karl/code/mozilla-central/dom/fetch/FetchDriver.cpp, line 519
[Child 58129] WARNING: blocked access to response header: file /Users/karl/code/mozilla-central/dom/xhr/XMLHttpRequestMainThread.cpp, line 1069
[Child 58129] WARNING: NS_ENSURE_SUCCESS(status, status) failed with result 0x804B0002: file /Users/karl/code/mozilla-central/netwerk/protocol/http/nsCORSListenerProxy.cpp, line 553
[Child 58129] WARNING: blocked access to response header: file /Users/karl/code/mozilla-central/dom/xhr/XMLHttpRequestMainThread.cpp, line 1069
[Child 58129] WARNING: blocked access to response header: file /Users/karl/code/mozilla-central/dom/xhr/XMLHttpRequestMainThread.cpp, line 1069
[Parent 58126] WARNING: attempt to modify an immutable nsStandardURL: file /Users/karl/code/mozilla-central/netwerk/base/nsStandardURL.cpp, line 1554
[Parent 58126] WARNING: NS_ENSURE_TRUE(aSecondURI) failed: file /Users/karl/code/mozilla-central/dom/base/ThirdPartyUtil.cpp, line 97
[Parent 58126] WARNING: NS_ENSURE_TRUE(aSecondURI) failed: file /Users/karl/code/mozilla-central/dom/base/ThirdPartyUtil.cpp, line 97
[Child 58129] WARNING: blocked access to response header: file /Users/karl/code/mozilla-central/dom/xhr/XMLHttpRequestMainThread.cpp, line 1069
[Child 58129] WARNING: blocked access to response header: file /Users/karl/code/mozilla-central/dom/xhr/XMLHttpRequestMainThread.cpp, line 1069


Clicked links around for a little while and 0 crash.


So I guess "it is solved". The log messages seems to mention something interesting.

Comment 25

[Karl Dubost💡 :karlcow]

First of all. 
Thanks Christoph for the patch. \o/ Ace.

It seems it solved the most pressing issue. 
There might be other things to investigate with regards to the logs. 
I don't know if there is plan to uplift to Aurora but that would be great. 

If I can help with any further tests for this bug or clone children of this bug, let me know.

Comment 26

[Christoph Kerschbaumer [:ckerschb]]

(In reply to Karl Dubost :karlcow from comment #25)
> There might be other things to investigate with regards to the logs. 
Thanks Karl! Actually not all of these warnings are actually warnings in the sense that something is going wrong.

> I don't know if there is plan to uplift to Aurora but that would be great. 
Yes, I already filed the uplift requests so this will hopefully end up in the upcoming release.
See also: https://bugzilla.mozilla.org/show_bug.cgi?id=1298505#c7
 
> If I can help with any further tests for this bug or clone children of this
> bug, let me know.

Thanks!

I will close this bug as a duplicate of 1298505, since the actual fix landed within Bug 1298505.

Comment 27

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Too late to fix in 50.1.0 release