1227289 - Default to the NullPrincipal rather than SystemPrincipal for Favicons

Status: RESOLVED FIXED

(Core :: DOM: Security, defect)

Description

[Christoph Kerschbaumer [:ckerschb]]

Within Bug 1119386 we started to use the document's principal as the loadingPrincipal of the channel. There are 2 functions where we default to using the systemPrincipal in case the principal argument is omitted:
* setAndFetchFaviconForPage()
* replaceFaviconDataFromDataURL()

We should give addon authors a chance to update their code for 2 or 3 versions and then default to the nullPrinipcal instead of the systemPrincipal.

See also:
+++ b/toolkit/components/places/mozIAsyncFavicons.idl
+   * @param aLoadingPrincipal
+   *        Principal of the page whose favicon is being set. If this argument
+   *        is omitted, the loadingPrincipal defaults to the systemPrincipal
+   *        and we cannot guarantee security checks anymore.

Comment 1

[Jorge Villalobos [:jorgev] (he/him)]

What would add-on devs need to change in their code?

Comment 2

[Christoph Kerschbaumer [:ckerschb]]

Within Bug 1119386 I set the addon-compat flag. Once Bug 1119386 has landed we would like addon authors to provide aLoadingPrincipal for

   void setAndFetchFaviconForPage(in nsIURI aPageURI,
                                  in nsIURI aFaviconURI,
                                  in boolean aForceReload,
                                  in unsigned long aFaviconLoadType,
-                                 [optional] in nsIFaviconDataCallback aCallback);
+                                 [optional] in nsIFaviconDataCallback aCallback,
+                                 [optional] in nsIPrincipal aLoadingPrincipal);


and


   void replaceFaviconDataFromDataURL(in nsIURI aFaviconURI,
                                      in AString aDataURL,
-                                     [optional] in PRTime aExpiration);
+                                     [optional] in PRTime aExpiration,
+                                     [optional] in nsIPrincipal aLoadingPrincipal);



At the moment we fall back to using the systemPrincipal, but after a few versions from now we should default to using the nullPrincipal and block such favicon loads.

Comment 3

[Christoph Kerschbaumer [:ckerschb]]

Jorge, we should update that soon, but we should probably land a warning for addon devs at least for one cycle. Potentially land a warning in FF49 and than convert to the code to default to the nullPrincipal for FF50.

Those are the places we need to udpate:
http://mxr.mozilla.org/mozilla-central/search?string=1227289

What is the standard way to log a deprecation warning these days so addon devs will also see it?

Comment 4

[Jorge Villalobos [:jorgev] (he/him)]

I don't know what the standard way of logging these errors is, but as long as it shows up in the Browser Console, it should be easy for developers and reviewers to pick up on it. Other than that, the message should explain clearly what they need to do, and/or point to this bug.

Comment 5

[Tanvi Vyas[:tanvi]]

Is this only an issue for addons?  Or does gecko code sometimes not pass in a principal, forcing us to fallback to system?

Comment 6

[Christoph Kerschbaumer [:ckerschb]]

(In reply to Tanvi Vyas [:tanvi] from comment #5)
> Is this only an issue for addons?  Or does gecko code sometimes not pass in
> a principal, forcing us to fallback to system?

Taniv, we added an assertion to make sure that Gecko always passes a principal [1]. Addons, that don't pass a principal explicitly will use a systemPrincipal as a fallback. Please note that it might as well be that Gecko passes a SystemPrincipal. If that is of relevance for what you are trying to accomplish, then someone would have to verify that.

[1] https://dxr.mozilla.org/mozilla-central/search?q=1227289&redirect=false

Comment 7

[Christoph Kerschbaumer [:ckerschb]]

Attachment: bug_1227289_default_to_nullprincipal_for_favicon_loads.patch

Bill, Bug 1119386 landed in FF45, I suppose we can now default to using the nullPrincipal instead of using the systemPrincipal in case addons don't provide an explicit loadingPrincipal. Thanks!

Comment 8

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

Comment on attachment 8782409 [details] [diff] [review]
bug_1227289_default_to_nullprincipal_for_favicon_loads.patch

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

Hmm, OK. I did a search in the addons dxr. Out of about 30 add-ons that use these functions, none have been updated. I'm fine doing this, but we may want to push on outreach a little more.

Comment 9

[Christoph Kerschbaumer [:ckerschb]]

(In reply to Bill McCloskey (:billm) from comment #8)
> Hmm, OK. I did a search in the addons dxr. Out of about 30 add-ons that use
> these functions, none have been updated. I'm fine doing this, but we may
> want to push on outreach a little more.

Thanks, Bill. I already set the addon-compat flag for this bug.

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/f0faa3abd17e
Default to the NullPrincipal rather than SystemPrincipal for Favicons. r=billm

Comment 11

[Ryan VanderMeulen [:RyanVM][PTO June 24-28]]

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

Comment 12

[JustOff]

Could anyone give me particular example how to update setAndFetchFaviconForPage call to achieve the result as it was before this bug has landed?

Comment 13

[Jorge Villalobos [:jorgev] (he/him)]

Which add-on are you referring to? How is the function called in your code?

Comment 14

[Christoph Kerschbaumer [:ckerschb]]

(In reply to Jorge Villalobos [:jorgev] from comment #13)
> Which add-on are you referring to? How is the function called in your code?

Setting ni? for JustOff.

Comment 15

[JustOff]

I'm using setAndFetchFaviconForPage to prefetch and/or refresh favicons (https://addons.mozilla.org/en-US/firefox/files/browse/475401/file/content/thumbnail.js#L229). Can I simple add Services.scriptSecurityManager.getSystemPrincipal() to the args?

Comment 16

[Jorge Villalobos [:jorgev] (he/him)]

Since it looks like you're doing this from chrome code, I think the system principal is right.