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

VERIFIED FIXED in Firefox 55

Status

()

Firefox
Security
P2
normal
VERIFIED FIXED
6 months ago
2 months ago

People

(Reporter: callahad, Assigned: Prathiksha, Mentored)

Tracking

unspecified
Firefox 55
Points:
---

Firefox Tracking Flags

(firefox52 wontfix, firefox53 wontfix, firefox54 fix-optional, firefox55 fixed, firefox-esr52 affected)

Details

(Whiteboard: [fxprivacy], URL)

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(2 attachments)

(Reporter)

Description

6 months ago
Created attachment 8829984 [details]
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)
Also happens on 50.1.0.

Comment 2

6 months ago
Panos, radaring this to you as it looks like something that might be related to your team's work on our security UI.
Flags: needinfo?(past)
Certainly, thanks for notifying me. Brindusa, can your team figure out when this broke?
Flags: needinfo?(past) → needinfo?(brindusa.tot)
Keywords: regressionwindow-wanted
Whiteboard: [fxprivacy][triage]
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

6 months ago
(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.
Flags: needinfo?(ckerschb)
(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.
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
Blocks: 671389
Flags: needinfo?(pacaro)

Updated

6 months ago
Has Regression Range: --- → yes
Keywords: regressionwindow-wanted

Comment 8

6 months ago
(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).
Flags: needinfo?(brindusa.tot)
(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.
Flags: needinfo?(ckerschb)
Cancelling ni per comment #8.
Flags: needinfo?(pacaro)

Updated

6 months ago
Priority: -- → P2
Whiteboard: [fxprivacy][triage] → [fxprivacy]
See Also: → bug 1284986
Duplicate of this bug: 1349205

Comment 12

4 months ago
This problem consistently reproduces on https://vcsjones.com for 52.0.1 (64-bit). It too has "sandbox" in the CSP policy.
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
Mentor: jhofmann@mozilla.com
status-firefox52: --- → wontfix
status-firefox53: --- → affected
status-firefox54: --- → affected
status-firefox55: --- → affected
status-firefox-esr52: --- → affected
Keywords: DevAdvocacy
Summary: Can't inspect TLS certificates, Page Info "View Certificate" does nothing → Page Info dialog doesn't work on pages that are sandboxed via CSP
(Assignee)

Updated

4 months ago
Assignee: nobody → prathikshaprasadsuman
Comment hidden (mozreview-request)

Comment 15

4 months ago
mozreview-review
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.
Attachment #8853748 - Flags: review?(jhofmann) → review-
(Assignee)

Comment 16

4 months ago
Okay!
Comment hidden (mozreview-request)

Comment 18

4 months ago
mozreview-review
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. :)
Attachment #8853748 - Flags: review?(jhofmann) → review-
(Assignee)

Comment 19

4 months ago
I'll make the changes. :)
Comment hidden (mozreview-request)

Comment 21

4 months ago
mozreview-review
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
Attachment #8853748 - Flags: review?(jhofmann) → review+
Comment hidden (mozreview-request)

Updated

4 months ago
Iteration: --- → 55.3 - Apr 17
Comment hidden (mozreview-request)
(Assignee)

Updated

4 months ago
Keywords: checkin-needed

Comment 24

4 months ago
Pushed by ihsiao@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/634ada49cadb
Changed the permissions.js file to make the Page Info dialog work. r=johannh
Keywords: checkin-needed

Comment 25

4 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/634ada49cadb
Status: NEW → RESOLVED
Last Resolved: 4 months ago
status-firefox55: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 55
Is this worth fixing in beta54?
status-firefox53: affected → wontfix
status-firefox54: affected → fix-optional

Comment 27

3 months ago
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]
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.