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

Status: RESOLVED FIXED

(Core :: DOM: Security, defect)

Comment 1

[Christoph Kerschbaumer [:ckerschb]]

Attachment: 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.

Comment 2

[Christoph Kerschbaumer [:ckerschb]]

(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.

Comment 3

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

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?

Comment 4

[Christoph Kerschbaumer [:ckerschb]]

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

Comment 5

[Christoph Kerschbaumer [:ckerschb]]

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

Comment 6

[Christoph Kerschbaumer [:ckerschb]]

Attachment: 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?

Comment 7

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

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.

Comment 8

[Christoph Kerschbaumer [:ckerschb]]

Attachment: 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.

Comment 9

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

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.

Comment 10

[Christoph Kerschbaumer [:ckerschb]]

(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()??

Comment 11

[Christoph Kerschbaumer [:ckerschb]]

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 12

[Marco Bonardo [:mak]]

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.

Comment 13

[Christoph Kerschbaumer [:ckerschb]]

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!

Comment 14

[Christoph Kerschbaumer [:ckerschb]]

Attachment: bug_1196013_favicons.patch

Billm and Mak already took a look at those changes. Jonas, can you sign off on the changes?

Comment 15

[Christoph Kerschbaumer [:ckerschb]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=b12c368a08ad

Comment 16

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

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!

Comment 17

[Christoph Kerschbaumer [:ckerschb]]

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

Comment 18

[Christoph Kerschbaumer [:ckerschb]]

Attachment: 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?

Comment 19

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

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.

Comment 20

[Christoph Kerschbaumer [:ckerschb]]

Attachment: 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!

Comment 21

[Christoph Kerschbaumer [:ckerschb]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=f03d1be99628

Comment 22

[Christoph Kerschbaumer [:ckerschb]]

Attachment: 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

Comment 23

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

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?

Comment 24

[Christoph Kerschbaumer [:ckerschb]]

Attachment: 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.

Comment 25

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/bbfbadb75603

Comment 26

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/bbfbadb75603