Closed Bug 1497200 Opened 6 years ago Closed 5 years ago

Apply Meta CSP to about:downloads

Categories

(Core :: DOM: Security, enhancement, P3)

Product:
Core
Component:
DOM: Security
Type:
enhancement

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla71
Tracking Flags:
Tracking Status
firefox71 --- fixed

People

(Reporter: ckerschb, Assigned: ckerschb)

References

Details

(Whiteboard: [domsecurity-backlog1])

Attachments

(2 files, 1 obsolete file)

bug_1497200_csp_about_downloads.patch
11.42 KB, patch
Details | Diff | Splinter Review
Bug 1497200: Apply Meta CSP to about:downloads. r=gijs
47 bytes, text/x-phabricator-request
Details | Review 
      No description provided.
Assignee: nobody → ckerschb
Status: NEW → ASSIGNED
Henri, Smaug,

ultimately we would like to apply a strong CSP to all of our about: pages. And while that works fine for all of the about: pages that are .xhtml, I am not sure how to move forward for about: pages relying on .xul files. E.g. I tried to apply a meta CSP to about:downloads [1] but I couldn't figure out how that would work. E.g. I tried various things similar to:
>  <html:head>
>  <html:meta http-equiv="Content-Security-Policy" content="default-src http:"/>
>  </html:head>
but all of my attempts seemed to fail. Please note that meta CSPs outside the <head> element are ignored by our parser.

I was wondering if you have any insights/suggestions on how we could accomplish adding a meta CSP to .xul files. Do we need to add a new XUL element?

Alternatively, would it make sense to basically add some variation of an .ini file which would be loaded whenever we load any of the xul pages? Maybe in the future we also want to apply a feature policy or whatever other security mechanism to .xul files!

Thanks for your help!

[1] https://searchfox.org/mozilla-central/source/browser/components/downloads/content/contentAreaDownloadsView.xul#1
Flags: needinfo?(hsivonen)
Flags: needinfo?(bugs)
Can we make about: channels generate the headers as if they were HTTP headers from the network?
Flags: needinfo?(hsivonen)
(In reply to Henri Sivonen (:hsivonen) from comment #2)
> Can we make about: channels generate the headers as if they were HTTP
> headers from the network?

I personally would prefer the meta CSP solution (if easily possible), because it's easily pinpoint- and update-able from within the .xul file.

Obviously we can play all sorts of tricks and apply a CSP within nsDocument.cpp when we initialize the CSP. E.g. we could add some sort of URI checking mechanism and if the URI is about:downloads then we apply whatever CSP we want.

Regarding, the channel and the HTTP headers; I don't think we can apply those headers in the AboutRedirector when we actually create the channel since http headers are only available for nsIHttpChannel implementations. about: channels do not implement that interface, but to double check, let's consult :valentin if there would be an alternative solution to that idea.
Flags: needinfo?(valentin.gosu)
(In reply to Christoph Kerschbaumer [:ckerschb] from comment #3)
> (In reply to Henri Sivonen (:hsivonen) from comment #2)
> Regarding, the channel and the HTTP headers; I don't think we can apply
> those headers in the AboutRedirector when we actually create the channel
> since http headers are only available for nsIHttpChannel implementations.
> about: channels do not implement that interface, but to double check, let's
> consult :valentin if there would be an alternative solution to that idea.

That is correct. Moreover, nsIHttpChannel has a lot of methods and attributes that don't really apply to file/chrome/resource channels, so if we wanted headers for these channels we'd have to split that functionality into a different interface.
I'd rather we didn't do that, but if there is no other way... we can.

Btw, I think the reason the meta tag didn't work is this:
https://searchfox.org/mozilla-central/rev/3d989d097fa35afe19e814c6d5bc2f2bf10867e1/dom/security/nsCSPContext.cpp#146
Flags: needinfo?(valentin.gosu)
(In reply to Valentin Gosu [:valentin] from comment #4)
> That is correct. Moreover, nsIHttpChannel has a lot of methods and
> attributes that don't really apply to file/chrome/resource channels, so if
> we wanted headers for these channels we'd have to split that functionality
> into a different interface.
> I'd rather we didn't do that, but if there is no other way... we can.

Actually I am all with you valentin, it makes no sense to change any of the nsIChannel/nsIHttpChannel interfaces just because of that. I would prefer if we could modify some the nsXULElement implementation.

Gijs suggested we might add an attribute to the window which we check for within dom/xul/nsXULElement.cpp when we call bindToTree. Even though it's not standard, that sounds like a good solution to me. I am currently verifying if that works.
 
> Btw, I think the reason the meta tag didn't work is this:
> https://searchfox.org/mozilla-central/rev/
> 3d989d097fa35afe19e814c6d5bc2f2bf10867e1/dom/security/nsCSPContext.cpp#146

FWIW, the code you pointed to is just enforcement of CSP. The reason it's not working at the moment is because XUL doesn't really know about CSP and I guess also not meta elements, or a combination of both.
Hey Henry,

would we accept a patch that basically adds a custom attribute "csp" to the root element?

In this patch I verified that it works and to me this approach seems like a good solution and not too much of a hack.

What do you think, acceptable approach?
Attachment #9017891 - Flags: feedback?(hsivonen)
Adding an attribute to the <window> element sounds reasonable.
But please check that the element actually is <xul:window> element. And GetAttr returns false if the attribute isn't there, so that is a place have an 'if'.


(And Henri is Henri, not Henry :p )
Flags: needinfo?(bugs)
(In reply to Olli Pettay [:smaug] from comment #7)
> Adding an attribute to the <window> element sounds reasonable.
> But please check that the element actually is <xul:window> element. And
> GetAttr returns false if the attribute isn't there, so that is a place have
> an 'if'.

Sounds good, but I am not sure we only want to expose that attribute on window. E.g. extensions.xul which is about:addons uses <page> as the root, and we want to apply a CSP there too.

> (And Henri is Henri, not Henry :p )

Oh, so sorry, in comment 1 I still had it correct and used Henri.
Comment on attachment 9017891 [details] [diff] [review]
bug_1497200_csp_about_downloads.patch

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

I'm not a XUL owner, so I don't fully understand the range of issues that need to be considered from a XUL perspective. Since XUL is not Web-exposed, I'm OK with us just making stuff up. Hence, feedback+.

I am, however, a bit concerned about the future path of migrating these pages away from XUL. I think we shouldn't just make stuff up if we migrate these pages to HTML or XHTML. In that case, considering the issues that prevent us from emulating HTTP headers in about: channels, if these pages were to migrate to HTML or XHTML, I'd prefer us to hard-code "as if HTTP headers were set" in the about: channel case in the code that normally interprets the headers when the channel is an HTTP channel.
Attachment #9017891 - Flags: feedback?(hsivonen) → feedback+

Gijs, I am stuck on that patch and I already spent quite so much time on it. Any feedback is highly appreciated. In particular, I have 4 questions which are marked "XXX Q1" to "XXX Q4". If you can help me resolve/provide feedback on any of those I would appreciate it.

A bit of background:
On XUL pages we can't simply apply a Meta CSP hence we are adding a custom attribute "csp" on XULElement which then feeds that attribute into the CSP machinery. Applying the CSP seems to work fine, but I am running into problems where the CSP blocks things which now need to be resolved.

Thanks for your time!

Attachment #9017891 - Attachment is obsolete: true
Attachment #9080342 - Flags: feedback?(gijskruitbosch+bugs)
Comment on attachment 9080342 [details] [diff] [review]
bug_1497200_csp_about_downloads.patch

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

::: browser/components/downloads/DownloadsViewUI.jsm
@@ +197,5 @@
>      this.element.setAttribute("orient", "horizontal");
> +    // XXX Q1: The following produces
> +    // JavaScript error: resource:///modules/DownloadsViewUI.jsm, line 199: ReferenceError: DownloadsView is not defined
> +    this.element.addEventListener("click", ev => {
> +      DownloadsView.onDownloadClick(ev);

We're in a JSM, and the attribute is defined in a DOM somewhere. The contents of the attribute event listener then run in the context of that DOM, instead of the JSM. But your listener runs in the context of the JSM.

Looks like `DownloadsView` could be https://searchfox.org/mozilla-central/source/browser/components/downloads/content/downloads.js#587 or the one defined in allDownloadsView.js (so either the browser downloads panel or the about:downloads page). So I suspect that `ev.target.ownerGlobal.DownloadsView.onDownloadClick(ev)` might work.

@@ +205,2 @@
>      this.element.appendChild(
>        document.importNode(downloadListItemFragment, true)

What specifically gets blocked how? `document` and `this.element` should both be in the same document. I don't know why importNode itself would be blocked.

If I had to guess, then the issue is the `oncommand` attribute in the markup that's used to generate the DOM fragment, at https://searchfox.org/mozilla-central/rev/40e889be8ff926e32f7567957f4c316f14f6fbef/browser/components/downloads/DownloadsViewUI.jsm#192 . Using a command event handler for that should work.

::: dom/xul/nsXULElement.cpp
@@ +679,5 @@
>          "Unexpected XUL element in non-XUL doc");
>    }
>  #endif
>  
> +  // XXX Q4: What do you think, good approach?

Might be worth talking to bgrins or bdahl to see whether we can prioritize converting these pages to (x)html instead. Especially if they get loaded in content anyway, it shouldn't be super difficult.

Otherwise, this seems fine.

::: toolkit/content/editMenuOverlay.js
@@ +57,5 @@
> +        <command id="cmd_switchTextDirection" />
> +      </commandset>
> +    `);
> +
> +    // XXX Q3: Can we simplify that somehow?

Yeah, so something like this:

```
fragment.firstElementChild.addEventListener("command", event => {
  let commandID = event.target.id;
  goDoCommand(commandID);
});
```

looks like it can replace all the command event listeners.

For the commandupdate event listeners, I think what you have is fine.
Attachment #9080342 - Flags: feedback?(gijskruitbosch+bugs)

For future reference, you can put things up in phabricator and not request review, and then needinfo me on the bug or tag me in a phabricator comment, and that'll also work even if it's not ready for final review - and has the benefit of better context availability and interdiffs for the final patch (and working markdown in comments... I asked the BMO team about this already and it's a known issue, AIUI). :-)

https://hg.mozilla.org/mozilla-central/rev/1572223bd5ec

Status: ASSIGNED → RESOLVED
Closed: 5 years ago
status-firefox71: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla71
Regressions: 1582532
Depends on: 1584282
Regressions: 1584539
No longer regressions: 1582532
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: