Use channel->ascynOpen2 for favicons in toolkit/components/places

RESOLVED FIXED in Firefox 49

Status

()

Core
DOM: Security
RESOLVED FIXED
2 years ago
9 months ago

People

(Reporter: ckerschb, Assigned: ckerschb)

Tracking

unspecified
mozilla49
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox49 fixed)

Details

(Whiteboard: [domsecurity-active])

Attachments

(1 attachment, 7 obsolete attachments)

Comment hidden (empty)
(Assignee)

Updated

2 years ago
Assignee: nobody → mozilla
Blocks: 1182535
(Assignee)

Comment 1

2 years ago
Created attachment 8649534 [details] [diff] [review]
bug_1196013_favicons.patch

Jonas, we still haven't finished bug 1119386 which passes the right principal along all the way to the place where we create the new channel. Do you want me to mark bug 1119386 blocking this bug? Or do you want to land this patch first and then pass the right principal along. Up to you.
(Assignee)

Comment 2

2 years ago
(In reply to Christoph Kerschbaumer [:ckerschb] from comment #1)
> Created attachment 8649534 [details] [diff] [review]
> bug_1196013_favicons.patch
> 
> Jonas, we still haven't finished bug 1119386 which passes the right
> principal along all the way to the place where we create the new channel. Do
> you want me to mark bug 1119386 blocking this bug? Or do you want to land
> this patch first and then pass the right principal along. Up to you.
Flags: needinfo?(jonas)
I don't know what the decision in bug 1119386 was? Is *this* code supposed to use the content principal? Or did we "only" need to set the loadingPrincipal in some <xul:image> load?
Flags: needinfo?(jonas)
Status: NEW → ASSIGNED
(Assignee)

Updated

2 years ago
Depends on: 1119386
(Assignee)

Comment 4

2 years ago
When converting callsites here we should also investigate:
http://mxr.mozilla.org/mozilla-central/source/browser/base/content/tabbrowser.xml#895
(Assignee)

Updated

2 years ago
Summary: Use channel->ascynOpen2 in toolkit/components/places → Use channel->ascynOpen2 for favicons in toolkit/components/places
(Assignee)

Comment 5

2 years ago
Adding a few gentleman who can help sorting things out and most importantly what security checks we can probably remove/refactor within this bug.
(Assignee)

Comment 6

2 years ago
Created attachment 8709739 [details] [diff] [review]
bug_1196013_favicons.patch

Hey Bill, since you already provided so much guidance on Bug 1119386 I was wondering if you can help convert callsites for favicons to use asyncOpen2 as well.

Basically there are 3 questions:
1) Can we simplify (or maybe even remove) shouldLoadFavIcon?
2) What is getLinkIconURI() doing? How does it interact with favicon loading from the network and can we potentially remove the contentPolicy check from within that function?
3) Are you fine with using SEC_ALLOW_CROSS_ORIGIN_DATA_INHERITS as the security flag when creating the new channel?
Attachment #8709739 - Flags: feedback?(wmccloskey)
(Assignee)

Updated

2 years ago
Attachment #8649534 - Attachment is obsolete: true
Hi Chris,

It does seem that we need to keep checking the prefs in shouldLoadFavIcon. Also, I read through bug 453442, and it seems like we need to keep the http/https checks because of that bug.

getLinkIconURI handles pages that have <link rel="icon" href="foobar">. If we switch the favicon service to asyncOpen2, then we shouldn't need the checks you commented out. That code causes us to send Link:SetIcon to the main process, which then calls setAndFetchFaviconForPage via tabbrowser.setIcon. I'm a little worried about the special cases we have for about: pages. It would be worth testing to make sure about:home has the correct favicon.

I don't feel qualified to say whether SEC_ALLOW_CROSS_ORIGIN_DATA_INHERITS is a good idea. At the least it seems like it should be a separate bug. Favicons seem a lot like image data, so I guess it makes sense to do this. But I know nothing about this stuff.
Attachment #8709739 - Flags: feedback?(wmccloskey)
(Assignee)

Comment 8

2 years ago
Created attachment 8713384 [details] [diff] [review]
bug_1196013_favicons.patch

@Bill: Thanks for all your answers - would you be willing to sign off on those changes?

@Jonas: Bill and I already sorted out what security checks we can remove from callsites. Favicons are in fact images, so I am pretty confident using SEC_ALLOW_CROSS_ORIGIN_DATA_INHERITS is the security flag which we should use for asyncOpen2(), but I would like you to confirm.
Attachment #8709739 - Attachment is obsolete: true
Attachment #8713384 - Flags: review?(wmccloskey)
Attachment #8713384 - Flags: review?(jonas)
Comment on attachment 8713384 [details] [diff] [review]
bug_1196013_favicons.patch

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

I don't really feel qualified to review some of this stuff. I'm redirecting to Marco.

::: browser/base/content/tabbrowser.xml
@@ +912,5 @@
>          <parameter name="aURI"/>
>          <body>
>            <![CDATA[
> +            // favicons channels use asyncOpen2() but we still have to perform that
> +            // shouldLoad check because of bug 453442

Please capitalize the sentence and end it with a period. Also, I'm not sure what you mean by "that shouldLoad check". We're just checking some prefs and the scheme here.

::: browser/modules/ContentLinkHandler.jsm
@@ -144,5 @@
>      var isAllowedPage = [
>        /^about:neterror\?/,
>        /^about:blocked\?/,
>        /^about:certerror\?/,
>        /^about:home$/,

Did you test how your patch affects favicon loads for these pages? Naively I would assume they can load whatever they want, but it would be nice to be sure.

::: dom/security/nsContentSecurityManager.cpp
@@ +142,5 @@
>        requestingContext = aLoadInfo->LoadingNode();
>        break;
>      }
>  
> +    case nsIContentPolicy::TYPE_IMAGE: {

I'm not really familiar with this code. Hopefully Jonas knows it better.
Attachment #8713384 - Flags: review?(wmccloskey) → review?(mak77)
(Assignee)

Comment 10

2 years ago
(In reply to Bill McCloskey (:billm) from comment #9)
> ::: browser/modules/ContentLinkHandler.jsm
> @@ -144,5 @@
> >      var isAllowedPage = [
> >        /^about:neterror\?/,
> >        /^about:blocked\?/,
> >        /^about:certerror\?/,
> >        /^about:home$/,
> 
> Did you test how your patch affects favicon loads for these pages? Naively I
> would assume they can load whatever they want, but it would be nice to be
> sure.

Actually, I just had another look here
>  (!isAllowedPage || !uri.schemeIs("chrome")) {
which makes me think we need to change the security flags to
> nsILoadInfo::SEC_ALLOW_CROSS_ORIGIN_DATA_INHERITS | nsILoadInfo::SEC_ALLOW_CHROME

to allow chrome: access, right Jonas?

The other thing I am wondering, we currently don't account for DISALLOW_SCRIPT

> ssm.checkLoadURIWithPrincipal(targetDoc.nodePrincipal, uri,
>                                Ci.nsIScriptSecurityManager.DISALLOW_SCRIPT);

Do we have to introduce another securityflag for that within LoadInfo which we can then forward to chckLoadURIWithPrincipal()??
(Assignee)

Comment 11

2 years ago
For the record, here are the important arguments within doContentSecurityCheck when loading the icon for about:blocked:

doContentSecurityCheck {
  channelURI: chrome://global/skin/icons/blacklist_favicon.png
  loadingPrincipal: about:blocked
  triggeringPrincipal: about:blocked
  contentPolicyType: 37
  securityMode: 4
  initalSecurityChecksDone: no
  enforceSecurity: no
}
Comment on attachment 8713384 [details] [diff] [review]
bug_1196013_favicons.patch

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

the changes look ok to me, doesn't look like there's anything requiring a specific knowledge in the Places internals and Bill's review has good comments already.
Attachment #8713384 - Flags: review?(mak77) → feedback+
(Assignee)

Comment 13

2 years ago
Comment on attachment 8713384 [details] [diff] [review]
bug_1196013_favicons.patch

This patch became slightly bitrotted and depends on the changes within the imageLoader. Clearing review-request for now. Once we landed Bug 1206961, I will re-flag you. Thanks!
Attachment #8713384 - Flags: review?(jonas)
(Assignee)

Updated

2 years ago
Whiteboard: [domsecurity-active]
(Assignee)

Comment 14

2 years ago
Created attachment 8746679 [details] [diff] [review]
bug_1196013_favicons.patch

Billm and Mak already took a look at those changes. Jonas, can you sign off on the changes?
Attachment #8713384 - Attachment is obsolete: true
Attachment #8746679 - Flags: review?(jonas)
Attachment #8746679 - Flags: feedback+
(Assignee)

Comment 15

2 years ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=b12c368a08ad
Blocks: 1270678
Comment on attachment 8746679 [details] [diff] [review]
bug_1196013_favicons.patch

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

I won't have time to review this one for a little bit, and I also don't feel that I know the code well enough to really safely review, so you should probably find another reviewer for this patch.

The only thing that I'd say is that in the code paths that we know we're loading from the favicon cache, we can simply use the system principal to load. No need to worry about loadingPrincipal/triggeringPrincipal/security-flags.

That might be what the patch is already doing though. If so, great!
Attachment #8746679 - Flags: review?(jonas)
(Assignee)

Comment 17

2 years ago
Comment on attachment 8746679 [details] [diff] [review]
bug_1196013_favicons.patch

Bill, Mak, anyone of the two of you willing to finally sign off on the patch?

I think we are passing the right loadingPrincipal in all of the cases where we can, and where we can't, e.g. sessionstore we use the systemPrincipal [1,2] which seems reasonable to me.

[1] https://bugzilla.mozilla.org/show_bug.cgi?id=1119386#c55
[2] http://mxr.mozilla.org/mozilla-central/source/browser/base/content/tabbrowser.xml#869
Attachment #8746679 - Flags: review?(wmccloskey)
Attachment #8746679 - Flags: review?(mak77)
(Assignee)

Comment 18

2 years ago
Created attachment 8753240 [details] [diff] [review]
bug_1196013_favicons.patch

Rebased the patch since the changes within AsyncFaviconHelpers.cpp do not apply anymore.

Bill, Mak, still the same question, any of the two of you can accept this changeset?
Attachment #8746679 - Attachment is obsolete: true
Attachment #8746679 - Flags: review?(wmccloskey)
Attachment #8746679 - Flags: review?(mak77)
Attachment #8753240 - Flags: review?(wmccloskey)
Attachment #8753240 - Flags: review?(mak77)
Comment on attachment 8753240 [details] [diff] [review]
bug_1196013_favicons.patch

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

So what about the chrome: URIs for about:home and stuff? Comment 10 seems to say we need SEC_ALLOW_CHROME, but the patch doesn't include that.

::: browser/base/content/tabbrowser.xml
@@ +908,5 @@
>          <parameter name="aURI"/>
>          <body>
>            <![CDATA[
> +            // favicons channels use asyncOpen2() but we still have to perform that
> +            // shouldLoad check because of bug 453442

Again, I don't understand the purpose of this comment. What does it mean?

::: browser/modules/ContentLinkHandler.jsm
@@ -171,2 @@
>      // To be on the safe side, only allow chrome:// favicons.
>      var isAllowedPage = [

isAllowedPage is now dead I think.
Attachment #8753240 - Flags: review?(wmccloskey)
(Assignee)

Comment 20

2 years ago
Created attachment 8754291 [details] [diff] [review]
bug_1196013_favicons.patch

Hey Bill, thanks for the review comments!

(In reply to Bill McCloskey (:billm) from comment #19)
> So what about the chrome: URIs for about:home and stuff? Comment 10 seems to
> say we need SEC_ALLOW_CHROME, but the patch doesn't include that.

That is indeed a good catch - added SEC_ALLOW_CHROME to the security flags!

> ::: browser/base/content/tabbrowser.xml
> > +            // favicons channels use asyncOpen2() but we still have to perform that
> > +            // shouldLoad check because of bug 453442
> Again, I don't understand the purpose of this comment. What does it mean?

Initially I thought we should add a comment there explaining why there is still a security check on the callsite even though asyncOpen2() advertises to handle *all* security checks. But then again, one could look at Bug 453442 to figure it out. Anyway, I removed the comment now!

> ::: browser/modules/ContentLinkHandler.jsm
> isAllowedPage is now dead I think.

Correct - removed!
Attachment #8753240 - Attachment is obsolete: true
Attachment #8753240 - Flags: review?(mak77)
Attachment #8754291 - Flags: review?(wmccloskey)
(Assignee)

Comment 21

2 years ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=f03d1be99628
(Assignee)

Comment 22

2 years ago
Created attachment 8754314 [details] [diff] [review]
bug_1196013_favicons.patch

Hey Bill, I just realized that the changes within FaviconHelpers.cpp got lost when rebasing the patch last time. Sorry about that!

I hope I answered all other questions within Comment 20 - thanks!

https://treeherder.mozilla.org/#/jobs?repo=try&revision=4e894743bd43
Attachment #8754291 - Attachment is obsolete: true
Attachment #8754291 - Flags: review?(wmccloskey)
Attachment #8754314 - Flags: review?(wmccloskey)
I'm sorry to keep this going, but now I'm worried about the DISALLOW_SCRIPT script issue you brought up in comment 10. Since favicons are fetched in the parent process, it's pretty important that we don't run javascript: URIs there. Can you add that flag?
(Assignee)

Comment 24

2 years ago
Created attachment 8755605 [details] [diff] [review]
bug_1196013_favicons.patch

(In reply to Bill McCloskey (:billm) from comment #23)
> I'm sorry to keep this going, but now I'm worried about the DISALLOW_SCRIPT
> script issue you brought up in comment 10. Since favicons are fetched in the
> parent process, it's pretty important that we don't run javascript: URIs
> there. Can you add that flag?

Bill, thanks for being so conscious about that patch. I added SEC_DISALLOW_SCRIPT to make sure we don't run javascript: URIs.
Attachment #8754314 - Attachment is obsolete: true
Attachment #8754314 - Flags: review?(wmccloskey)
Attachment #8755605 - Flags: review?(wmccloskey)
Attachment #8755605 - Flags: review?(wmccloskey) → review+
(Assignee)

Updated

2 years ago
Keywords: checkin-needed

Comment 25

2 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/bbfbadb75603
Keywords: checkin-needed

Comment 26

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/bbfbadb75603
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
status-firefox49: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla49
You need to log in before you can comment on or make changes to this bug.