1126004 - Make JS callers of ios.newChannel call ios.newChannel2 in toolkit/components/places

Status: RESOLVED FIXED

(Core :: DOM: Security, defect)

Description

[Christoph Kerschbaumer [:ckerschb]]

Splitting up Bug 1087744 before it becomes too messy.

Comment 1

[Christoph Kerschbaumer [:ckerschb]]

Attachment: js_15_toolkit_components_places.patch

Comment 2

[Christoph Kerschbaumer [:ckerschb]]

Attachment: js_15_toolkit_components_places_tests.patch

Comment 3

[Tanvi Vyas[:tanvi]]

Copying over Marco's comment 25 from bug 1087744 so we don't lose context:


Comment on attachment 8553427 [details] [diff] [review] [diff] [review]
js_15_toolkit_components_places.patch

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

::: toolkit/components/places/nsLivemarkService.js
@@ +671,5 @@
>        let loadgroup = Cc["@mozilla.org/network/load-group;1"].
>                        createInstance(Ci.nsILoadGroup);
> +      let feedPrincipal =
> +        secMan.getSimpleCodebasePrincipal(this.feedURI);
> +      let channel = NetUtil.newChannel2(this.feedURI.spec,

how much safe is this? What does getSimpleCodebasePrincipal do?
The truth here is that we can't get a principal from a document or such, we just have a feed uri and we want to read its contents.
It's effectively loading data from the Web. do we need any additional safety?

Comment 4

[Marco Bonardo [:mak]]

My question is still unanswered

Comment 5

[Christoph Kerschbaumer [:ckerschb]]

Attachment: js_15_toolkit_components_places.patch

(In reply to Tanvi Vyas [:tanvi] from comment #3)
> Copying over Marco's comment 25 from bug 1087744 so we don't lose context:
> 
> 
> Comment on attachment 8553427 [details] [diff] [review]
> js_15_toolkit_components_places.patch
> 
> Review of attachment 8553427 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: toolkit/components/places/nsLivemarkService.js
> @@ +671,5 @@
> >        let loadgroup = Cc["@mozilla.org/network/load-group;1"].
> >                        createInstance(Ci.nsILoadGroup);
> > +      let feedPrincipal =
> > +        secMan.getSimpleCodebasePrincipal(this.feedURI);
> > +      let channel = NetUtil.newChannel2(this.feedURI.spec,
> 
> how much safe is this? What does getSimpleCodebasePrincipal do?
> The truth here is that we can't get a principal from a document or such, we
> just have a feed uri and we want to read its contents.
> It's effectively loading data from the Web. do we need any additional safety?

Sorry it took as so long to reply. We discussed the scenarios for feeds in our meeting today. Jonas, Tanvi and myself concluded that we should just create a principal from the feed uri using getNoAppCodebasePrincipal(). We know that we don't have a document available. The big difference between getNoAppCodebasePrincipal() and getSimpleCodebasePrincipal() is that getSimpleCodebasePrincipal() uses UNKNOWN_APP_ID whereas getNoAppCodebasePrincipal() uses NO_APP_ID. We should not make any network requests with an unknown appid, so I think the change we incorporated in that patch makes sense.

Comment 6

[Marco Bonardo [:mak]]

Comment on attachment 8554775 [details] [diff] [review]
js_15_toolkit_components_places_tests.patch

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

::: toolkit/components/places/tests/favicons/test_moz-anno_favicon_mime_type.js
@@ +95,5 @@
> +                                  null,      // aLoadingNode
> +                                  Services.scriptSecurityManager.getSystemPrincipal(),
> +                                  null,      // aTriggeringPrincipal
> +                                  Ci.nsILoadInfo.SEC_NORMAL,
> +                                  Ci.nsIContentPolicy.TYPE_OTHER);

why not TYPE_IMAGE?

Comment 7

[Christoph Kerschbaumer [:ckerschb]]

Attachment: js_15_toolkit_components_places_tests.patch

(In reply to Marco Bonardo [::mak] (needinfo? me) from comment #6)
> Comment on attachment 8554775 [details] [diff] [review]
> js_15_toolkit_components_places_tests.patch
> > +                                  Ci.nsIContentPolicy.TYPE_OTHER);
> 
> why not TYPE_IMAGE?

TYPE_IMAGE it is obviously - thanks!

Comment 8

[Christoph Kerschbaumer [:ckerschb]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/ea852054980a
https://hg.mozilla.org/integration/mozilla-inbound/rev/a62c7b38ad6b

Comment 9

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/ea852054980a
https://hg.mozilla.org/mozilla-central/rev/a62c7b38ad6b