1453012 - Need to block chrome URLS when we block about: URLs

Status: RESOLVED FIXED

(Firefox :: Enterprise Policies, enhancement)

Description

[Mike Kaply [:mkaply]]

We currently block about: URLS in the redirector, but these can be bypassed by typing the chrome URL directly in the URL bar (chrome://global/content/config.xul).

We need some mechanism to block that as well.

For 60, maybe we should just have the JS files for the various feature bail early if the policy is not enabled?

While we think of a longer term solution?

Affects

about:config
about:profiles
about:support
about:debugging
about:telemetry

Comment 1

[Mike Kaply [:mkaply]]

Attachment: Blocking chrome pages using content policy

This is a Firefox 60 only fix for this problem because content policy changed.

Comment 2

[Mike Kaply [:mkaply]]

Comment on attachment 8967104 [details] [diff] [review]
Blocking chrome pages using content policy

obviously I need to do something different with the "registerContentPolicy"

Right now it's only in about:config.

This is for feedback, not review

Comment 3

[:Felipe Gomes (needinfo for replies!)]

Comment on attachment 8967104 [details] [diff] [review]
Blocking chrome pages using content policy

Review of attachment 8967104 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/components/enterprisepolicies/Policies.jsm
@@ +848,5 @@
> +
> +let ContentPolicy = {
> +  shouldLoad: function(aContentType, aContentLocation, aRequestOrigin, aContext, aMimeTypeGuess, aExtra) {
> +    if (aContentLocation.scheme == "chrome") {
> +      if (aRequestOrigin && aRequestOrigin.spec == "chrome://browser/content/browser.xul") {

I think you can replace checking the aRequestOrigin, by checking if aContentType is TYPE_DOCUMENT or TYPE_SUBDOCUMENT.  (so that it means it's being attempted to loaded in a tab or an iframe).

@@ +849,5 @@
> +let ContentPolicy = {
> +  shouldLoad: function(aContentType, aContentLocation, aRequestOrigin, aContext, aMimeTypeGuess, aExtra) {
> +    if (aContentLocation.scheme == "chrome") {
> +      if (aRequestOrigin && aRequestOrigin.spec == "chrome://browser/content/browser.xul") {
> +        if ((!Services.policies.isAllowed("about:config") &&

No need to list all these options. I think that once at least one about: page is blocked, we can activate this content policy to block *all* chrome:// URLs.

so your registerContentPolicy() entry point function could be called something like blockAllChromeURLs().

And make the ContentPolicy object name be more specific about its purpose.. And add at least a small comment about it

@@ +891,5 @@
> +                            ContentPolicy.contractID,
> +                            ContentPolicy);
> +
> +  let cm = Cc["@mozilla.org/categorymanager;1"].getService(Ci.nsICategoryManager);
> +  cm.addCategoryEntry("content-policy", ContentPolicy.contractID, ContentPolicy.contractID, false, true);

(I'm not an specialist on this, but it looks on par with other stuff in the tree)

Comment 4

[Mike Kaply [:mkaply]]

Attachment: Bug 1453012 - Block all chrome URLS if about: policy is active.

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

Comment 5

[Code Review Bot [:reviewbot]]

Comment on attachment 8967115 [details]
Bug 1453012 - Block all chrome URLS if about: policy is active.

https://reviewboard.mozilla.org/r/235764/#review241564


Code analysis found 4 defects in this patch:
 - 4 defects found by mozlint

You can run this analysis locally with:
 - `./mach lint path/to/file` (JS/Python)


If you see a problem in this automated review, please report it here: http://bit.ly/2y9N9Vx


::: browser/components/enterprisepolicies/EnterprisePolicies.js:470
(Diff revision 1)
>  }
>  
> +// This policy blocks all chrome:// URLs from being loaded in the browser window.
> +// It is only activated when an about: page is blocked.
> +let ChromeURLBlockPolicy = {
> +  shouldLoad: function(aContentType, aContentLocation, aRequestOrigin, aContext, aMimeTypeGuess, aExtra) {

Error: Expected method shorthand. [eslint: object-shorthand]

::: browser/components/enterprisepolicies/EnterprisePolicies.js:478
(Diff revision 1)
> +        aRequestOrigin.spec == "chrome://browser/content/browser.xul") {
> +      return Ci.nsIContentPolicy.REJECT_REQUEST;
> +    }
> +    return Ci.nsIContentPolicy.ACCEPT;
> +  },
> +  shouldProcess: function(aContentType, aContentLocation, aRequestOrigin, aContext, aMimeTypeGuess, aExtra) {

Error: Expected method shorthand. [eslint: object-shorthand]

::: browser/components/enterprisepolicies/EnterprisePolicies.js:483
(Diff revision 1)
> +  shouldProcess: function(aContentType, aContentLocation, aRequestOrigin, aContext, aMimeTypeGuess, aExtra) {
> +    return Ci.nsIContentPolicy.ACCEPT;
> +  },
> +  classDescription: "Policy Engine Content Policy",
> +  contractID: "@mozilla-org/policy-engine-content-policy-service;1",
> +  classID: Components.ID('{ba7b9118-cabc-4845-8b26-4215d2a59ed7}'),

Error: Strings must use doublequote. [eslint: quotes]

::: browser/components/enterprisepolicies/EnterprisePolicies.js:485
(Diff revision 1)
> +  },
> +  classDescription: "Policy Engine Content Policy",
> +  contractID: "@mozilla-org/policy-engine-content-policy-service;1",
> +  classID: Components.ID('{ba7b9118-cabc-4845-8b26-4215d2a59ed7}'),
> +  QueryInterface: XPCOMUtils.generateQI([Ci.nsIContentPolicy]),
> +  createInstance: function(outer, iid) {

Error: Expected method shorthand. [eslint: object-shorthand]

Comment 6

[Mike Kaply [:mkaply]]

Comment on attachment 8967115 [details]
Bug 1453012 - Block all chrome URLS if about: policy is active.

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

Comment 7

[:Felipe Gomes (needinfo for replies!)]

Comment on attachment 8967115 [details]
Bug 1453012 - Block all chrome URLS if about: policy is active.

https://reviewboard.mozilla.org/r/235764/#review241600

Comment 8

[Mike Kaply [:mkaply]]

Comment on attachment 8967115 [details]
Bug 1453012 - Block all chrome URLS if about: policy is active.

Approval Request Comment
[Feature/Bug causing the regression]: Enterprise Policy
[User impact if declined]: User can bypass blocked pages
[Is this code covered by automated tests?]: No
[Has the fix been verified in Nightly?]: Not in nightly
[Needs manual test from QE? If yes, steps to reproduce]: No
[List of other uplifts needed for the feature/fix]: Non
[Is the change risky?]: No
[Why is the change risky/not risky?]: Adds new content policy only in cases where about: is blocked
[String changes made/needed]: None


This change is only going on beta right now because nsIContentPolicy was completely rewritten for 61, so any patch we do won't be close.

So this patch is only intended as a stopgap on ESR 60.

Comment 9

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

Comment on attachment 8967115 [details]
Bug 1453012 - Block all chrome URLS if about: policy is active.

enterprise policies fix, approved for 60.0b12

Comment 10

[Eliza Balazs [:ebalazs_]]

https://hg.mozilla.org/releases/mozilla-beta/rev/8f64d3c436b3

Comment 11

[Ryan VanderMeulen [:RyanVM]]

Did we ever sort anything out for 61+ here?

Comment 12

[Mike Kaply [:mkaply]]

No, it fell off my radar. I honestly wasn't terribly stressed because things are fine on the ESR and we're still getting RR to the point where it's fully usable for enterprise.

I'll open a new bug and target for 62.

Comment 13

[Liz Henry (:lizzard) (relman/hg->git project)]

Anything left to do here in this bug or can we close it?

Comment 14

[Mike Kaply [:mkaply]]

We're all done here. Additional work was done in https://bugzilla.mozilla.org/show_bug.cgi?id=1471355 and it's already uplifted to 62.