1333532 - Page Info dialog doesn't work on pages that are sandboxed via CSP

Status: VERIFIED FIXED

(Firefox :: Security, defect, P2)

Description

[Dan Callahan [:callahad]]

Attachment: screenshot.png

See attached screenshot. Possibly related to Bug 1226600.

STR:

1. Visit https://broker.portier.io/ver.txt
2. Tools > Page Info > Security
3. View Certificate

What happens:

- The entire Security panel is blank
- Clicking "View Certificate" does nothing
- The following error appears in the Browser Console:  NS_ERROR_UNEXPECTED: Component returned failure code: 0x8000ffff (NS_ERROR_UNEXPECTED) [nsIQuotaManagerService.getUsageForPrincipal]  permissions.js:202

I have verified this on OS X and Linux, on Release (51) through Nightly, on three computers, two different network connections, and with old and new profiles alike.

Of note:

- The correct information *does* appear in the DevTools
- The correct issuer *is* shown in the Page Info doorhanger
- View Certificate works fine on other domains (e.g., https://helloworld.letsencrypt.org)

Comment 1

[Erik Rose [:erik][:erikrose]]

Also happens on 50.1.0.

Comment 2

[:Gijs (he/him)]

Panos, radaring this to you as it looks like something that might be related to your team's work on our security UI.

Comment 3

[Panos Astithas (he/him) [:past] (please ni?)]

Certainly, thanks for notifying me. Brindusa, can your team figure out when this broke?

Comment 4

[Johann Hofmann [:johannh]]

So for some reason the principal of that page is a null principal. I presume that's the reason why nsIQuotaManagerService.getUsageForPrincipal doesn't like it. I doubt that this was regressed by the recent Security UI work but we should fix it nonetheless.

Comment 5

[:Gijs (he/him)]

(In reply to Johann Hofmann [:johannh] from comment #4)
> So for some reason the principal of that page is a null principal.

Interesting... maybe because it's a text file? That's still kind of odd though... I would expect an HTML page that frames a text file that is same-origin by the URI to be able to read it from the DOM, and for that they'd need to be same principal, I think. Christoph, do you know why this would be using a null principal?

> I presume
> that's the reason why nsIQuotaManagerService.getUsageForPrincipal doesn't
> like it.

That certainly sounds very plausible.

Comment 6

[Johann Hofmann [:johannh]]

(In reply to :Gijs from comment #5)
> (In reply to Johann Hofmann [:johannh] from comment #4)
> > So for some reason the principal of that page is a null principal.
> 
> Interesting... maybe because it's a text file? That's still kind of odd
> though...

Yes that was my first thought, too, but neither https://raw.githubusercontent.com/kripken/emscripten/master/emscripten-version.txt nor my self-hosted test.txt have a null principal. I was also considering some borked server header e.g. a wrong Content-Type but I haven't found anything suspicious so far.

Comment 7

[Masatoshi Kimura [:emk]]

I'm not sure why nobody did bisection instead of guesswork.
Regression range:
https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=48bd14a01b55464215c0bfe973f5657a7b5b7f9c&tochange=c34ec3e0884c31f128cf051619e78ffd9461a1c4

Comment 8

[:Gijs (he/him)]

(In reply to Masatoshi Kimura [:emk] from comment #7)
> I'm not sure why nobody did bisection instead of guesswork.

Because I wasn't sure it was a regression in the first place. I only flagged up people who work in this area, and they already set the regressionwindow-wanted keyword.


> Regression range:
> https://hg.mozilla.org/integration/mozilla-inbound/
> pushloghtml?fromchange=48bd14a01b55464215c0bfe973f5657a7b5b7f9c&tochange=c34e
> c3e0884c31f128cf051619e78ffd9461a1c4

FWIW, the site sends a CSP header with:

sandbox allow-scripts allow-forms; default-src 'none'; script-src 'self'; style-src 'self'; form-action *

which is presumably the cause of the null principal. That's probably correct. We should just fix browser code to do the right thing with the null principal, which isn't really causally linked to us starting to implement CSP sandboxing (that is, null principals existed before that and this bug could have surfaced before that, too, with a different testcase).

Comment 9

[Christoph Kerschbaumer [:ckerschb]]

(In reply to :Gijs from comment #8)
> sandbox allow-scripts allow-forms; default-src 'none'; script-src 'self';
> style-src 'self'; form-action *
> 
> which is presumably the cause of the null principal. That's probably
> correct. We should just fix browser code to do the right thing with the null
> principal, which isn't really causally linked to us starting to implement
> CSP sandboxing (that is, null principals existed before that and this bug
> could have surfaced before that, too, with a different testcase).

That is entirely correct, if CSP includes 'sandbox' then we set the sandboxflags on the document and later on the loadInfo which in turn causes GetChannelResultPrincipal() to return a freshly created NullPrincipal.

Comment 10

[Masatoshi Kimura [:emk]]

Cancelling ni per comment #8.

Comment 12

[Kevin Jones]

This problem consistently reproduces on https://vcsjones.com for 52.0.1 (64-bit). It too has "sandbox" in the CSP policy.

Comment 13

[Johann Hofmann [:johannh]]

Yes, we should solve this. Making this a mentored bug.

Since sandboxed code does not have access to indexedDB anyway, we should just skip the call to quotaManagerService.getUsageForPrincipal[0] if gPermPrincipal.isNullPrincipal == true (keeping the indexedDB section hidden) .

[0] http://searchfox.org/mozilla-central/source/browser/base/content/pageinfo/permissions.js#190

Comment 14

[:prathiksha]

Attachment: Bug 1333532 - Changed the permissions.js file to make the Page Info dialog work.

Review commit: https://reviewboard.mozilla.org/r/125812/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/125812/

Comment 15

[Johann Hofmann [:johannh]]

Comment on attachment 8853748 [details]
Bug 1333532 - Changed the permissions.js file to make the Page Info dialog work.

https://reviewboard.mozilla.org/r/125812/#review128412

Thanks! This is the right approach, just a couple of things to fix.

Can you please also add a comment why we're doing this? E.g. we're not showing indexedDB information for pages with a null principal such as sandboxed pages, because these pages do not have access to indexedDB.

::: browser/base/content/pageinfo/permissions.js:187
(Diff revision 1)
>    let row = document.getElementById("perm-indexedDB-row");
>    let extras = document.getElementById("perm-indexedDB-extras");
>  
>    row.appendChild(extras);
>  
>    var quotaManagerService =

You can include this part in the if clause (and then please change var to let)

::: browser/base/content/pageinfo/permissions.js:190
(Diff revision 1)
>    row.appendChild(extras);
>  
>    var quotaManagerService =
>      Components.classes["@mozilla.org/dom/quota-manager-service;1"]
>                .getService(nsIQuotaManagerService);
> +  if(gPermPrincipal.isNullPrincipal == false) {

Please add a space after the "if".

This can be simplified to 

if (!gPermPrincipal.isNullPrincipal)

::: browser/base/content/pageinfo/permissions.js:191
(Diff revision 1)
>  
>    var quotaManagerService =
>      Components.classes["@mozilla.org/dom/quota-manager-service;1"]
>                .getService(nsIQuotaManagerService);
> +  if(gPermPrincipal.isNullPrincipal == false) {
>    gUsageRequest =

Please indent the code inside the brackets by two spaces.

Comment 16

[:prathiksha]

Okay!

Comment 17

[:prathiksha]

Comment on attachment 8853748 [details]
Bug 1333532 - Changed the permissions.js file to make the Page Info dialog work.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/125812/diff/1-2/

Comment 18

[Johann Hofmann [:johannh]]

Comment on attachment 8853748 [details]
Bug 1333532 - Changed the permissions.js file to make the Page Info dialog work.

https://reviewboard.mozilla.org/r/125812/#review128436

::: browser/base/content/pageinfo/permissions.js:184
(Diff revision 2)
>  }
>  
>  function initIndexedDBRow() {
> +  // IndexedDB information is not shown for pages with a null principal
> +  // such as sandboxed pages because these pages do not have access to indexedDB.
> +  if (!gPermPrincipal.isNullPrincipal) {

I just realized that this function is only responsible for showing the additional controls related to indexedDB usage and not the row itself, so it should be fine to just return early here and skip the rest of the function if there's a null principal.

Sorry for that. :)

Comment 19

[:prathiksha]

I'll make the changes. :)

Comment 20

[:prathiksha]

Comment on attachment 8853748 [details]
Bug 1333532 - Changed the permissions.js file to make the Page Info dialog work.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/125812/diff/2-3/

Comment 21

[Johann Hofmann [:johannh]]

Comment on attachment 8853748 [details]
Bug 1333532 - Changed the permissions.js file to make the Page Info dialog work.

https://reviewboard.mozilla.org/r/125812/#review128504

Great, thank you!

::: browser/base/content/pageinfo/permissions.js:184
(Diff revision 3)
>  }
>  
>  function initIndexedDBRow() {
> +  // IndexedDB information is not shown for pages with a null principal
> +  // such as sandboxed pages because these pages do not have access to indexedDB.
> +  if (gPermPrincipal.isNullPrincipal) 

Nit: trailing space

Comment 22

[:prathiksha]

Comment on attachment 8853748 [details]
Bug 1333532 - Changed the permissions.js file to make the Page Info dialog work.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/125812/diff/3-4/

Comment 23

[:prathiksha]

Comment on attachment 8853748 [details]
Bug 1333532 - Changed the permissions.js file to make the Page Info dialog work.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/125812/diff/4-5/

Comment 24

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/634ada49cadb
Changed the permissions.js file to make the Page Info dialog work. r=johannh

Comment 25

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/634ada49cadb

Comment 26

[Julien Cristau [:jcristau] (back April 22)]

Is this worth fixing in beta54?

Comment 27

[Sajedul Islam]

I have reproduced this bug with Nightly 54.0a1 (2017-01-24) on Windows 8.1 , 64 Bit ! 

This bug's fix is Verified with latest Nightly !

Build ID 	20170426030329
User Agent 	Mozilla/5.0 (Windows NT 6.3; Win64; x64; rv:55.0) Gecko/20100101 Firefox/55.0

[bugday-20170426]