Use channel.asyncOpen2() within toolkit/components/places

RESOLVED FIXED in Firefox 48

Status

()

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

People

(Reporter: ckerschb, Assigned: ckerschb)

Tracking

(Blocks: 1 bug)

unspecified
mozilla48
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox48 fixed)

Details

(Whiteboard: [domsecurity-active])

Attachments

(1 attachment, 3 obsolete attachments)

Comment hidden (empty)
(Assignee)

Updated

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

Updated

2 years ago
Summary: Use channel.asyncOpen2() within toolkit/components/places/nsLivemarkService.js → Use channel.asyncOpen2() within toolkit/components/places
(Assignee)

Comment 1

2 years ago
Created attachment 8712014 [details] [diff] [review]
bug_1242857_js_asyncopen2_componets_places.patch
Attachment #8712014 - Flags: review?(mak77)
Attachment #8712014 - Flags: review?(jonas)
Comment on attachment 8712014 [details] [diff] [review]
bug_1242857_js_asyncopen2_componets_places.patch

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

off-hand it looks OK, but honestly I don't know enough about these changes, their effect and what we could improve... so I'm just giving a f+ from a code point of view. Jonas can do better evaluation.

In the json case we load a local file, in the livemarks case we load a single xml file from a server.

::: toolkit/components/places/BookmarkJSONUtils.jsm
@@ +209,5 @@
>        let uri = NetUtil.newURI(spec);
>        let channel = NetUtil.newChannel({
>          uri,
>          loadingPrincipal: Services.scriptSecurityManager.createCodebasePrincipal(uri, {}),
> +        securityFlags:  Ci.nsILoadInfo.SEC_REQUIRE_CORS_DATA_INHERITS,

there are 2 spaces after the colon, when there should only be one.

::: toolkit/components/places/nsLivemarkService.js
@@ +530,5 @@
>                        createInstance(Ci.nsILoadGroup);
>        let channel = NetUtil.newChannel({
>          uri: this.feedURI.spec,
>          loadingPrincipal: Services.scriptSecurityManager.createCodebasePrincipal(this.feedURI, {}),
> +        securityFlags:  Ci.nsILoadInfo.SEC_REQUIRE_CORS_DATA_INHERITS,

ditto
Attachment #8712014 - Flags: review?(mak77) → feedback+
Comment on attachment 8712014 [details] [diff] [review]
bug_1242857_js_asyncopen2_componets_places.patch

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

::: toolkit/components/places/BookmarkJSONUtils.jsm
@@ -208,5 @@
>  
>        let uri = NetUtil.newURI(spec);
>        let channel = NetUtil.newChannel({
>          uri,
>          loadingPrincipal: Services.scriptSecurityManager.createCodebasePrincipal(uri, {}),

This is almost certainly wrong. Any time you're calling createCodebasePrincipal you almost always are.

What URI are we loading here, and why are we loading it? Who decide which URI to load? Is that decided by a website? Who is getting access to the the data returned by the server?

::: toolkit/components/places/nsLivemarkService.js
@@ -528,5 @@
>        // cancel the channel.
>        let loadgroup = Cc["@mozilla.org/network/load-group;1"].
>                        createInstance(Ci.nsILoadGroup);
>        let channel = NetUtil.newChannel({
>          uri: this.feedURI.spec,

Is this.feedURI an nsIURI object? If so, why not just pass that object directly rather than stringify (which the newChannel function will then parse into a nsIURI)

@@ +529,5 @@
>        let loadgroup = Cc["@mozilla.org/network/load-group;1"].
>                        createInstance(Ci.nsILoadGroup);
>        let channel = NetUtil.newChannel({
>          uri: this.feedURI.spec,
>          loadingPrincipal: Services.scriptSecurityManager.createCodebasePrincipal(this.feedURI, {}),

Again, this is almost certainly wrong. See questions for BookmarkJSONUtils.jsm.
(Assignee)

Comment 4

2 years ago
Mak, can you help us answer the questions from Jonas within Comment 3?
Flags: needinfo?(mak77)
(In reply to Jonas Sicking (:sicking) from comment #3)
> ::: toolkit/components/places/BookmarkJSONUtils.jsm
> @@ -208,5 @@
> >  
> >        let uri = NetUtil.newURI(spec);
> >        let channel = NetUtil.newChannel({
> >          uri,
> >          loadingPrincipal: Services.scriptSecurityManager.createCodebasePrincipal(uri, {}),
> 
> This is almost certainly wrong. Any time you're calling
> createCodebasePrincipal you almost always are.
> 
> What URI are we loading here, and why are we loading it? Who decide which
> URI to load? Is that decided by a website? Who is getting access to the the
> data returned by the server?

No, we're not loading anything from the Internet in Firefox. This is just loading from a file URI, it's a local file. The reason is historical, basically we need to import html from an url cause the default bookmarks file is in omni.ja and we can only access it through an url. We planned for a long time to convert that to json, so the 2 apis (html and json) are pretty much identical.
Now, add-ons could ideally also invoke that with a remote uri, I don't know if we care about that? We could even restrict it to only file uris if needed.

> ::: toolkit/components/places/nsLivemarkService.js

> @@ +529,5 @@
> >        let loadgroup = Cc["@mozilla.org/network/load-group;1"].
> >                        createInstance(Ci.nsILoadGroup);
> >        let channel = NetUtil.newChannel({
> >          uri: this.feedURI.spec,
> >          loadingPrincipal: Services.scriptSecurityManager.createCodebasePrincipal(this.feedURI, {}),
> 
> Again, this is almost certainly wrong.

Here we are reading a feed xml from the network indeed. We only have the xml url we got from the database. Not sure what we can build out of that?
Flags: needinfo?(mak77)
For BookmarkJSONUtils.jsm it sounds like we should just set loadUsingSystemPrincipal: true. No need to set a loadingPrincipal, securityFlags or contentPolicyType. And, by extension, no need to create a principal. Easy as pie :)

For nsLivemarkService.js, does a website decide which URL to load? I.e. did we get the URL to load from a website?

Do we know which website? Is it the website that we've bookmarked?
(Assignee)

Comment 7

2 years ago
Mak, thanks for your feedback, can you please answer Jonas's question from Comment 6 than we can get that fixed and updated!
Flags: needinfo?(mak77)
(In reply to Jonas Sicking (:sicking) from comment #6)
> For nsLivemarkService.js, does a website decide which URL to load? I.e. did
> we get the URL to load from a website?
> 
> Do we know which website? Is it the website that we've bookmarked?

When visiting a website we have detection that sniffs <link> tags pointing to rss/atom feeds, this happens in browser code. From there we open the feed preview page (implemented in browser/feeds) (either through a link on the page itself or through the RSS toolbar button in Firefox UI).
At this point the user can add the feed to livemarks, but we don't pass-through nor save the original page that contained the feed. So the information you are looking for is likely lost.
The only thing we have in bookmarks storage is the feed URI (that is the xml uri) and eventually (not mandatory) a site URI that is ideally the originating URI, but it's taken from a tag in the xml contents themselves, so it could be easily forged and I don't think it's reliable at all.

If we want to store the original address that contained the feed, we would need to modify browser code to pass that through the preview page and then modify livemarks APIs to store it.

But I sort of think we may just rely on a check done when we open the feed preview page? if that's unreachable then there's no way to add the unsafe livemark from the UI.
Flags: needinfo?(mak77)
(Assignee)

Comment 9

2 years ago
Created attachment 8713734 [details] [diff] [review]
bug_1242857_js_asyncopen2_componets_places.patch

Mak, how hard would it actually be to make sure we pass a non null siteURI to writeSiteURI()? I kind of feel like that's what we should do. Then we could also remove the two checkLoadURIWithPrincipal checks within that patch.

Btw, the first two arguments to CheckLoadURI need to be switched in my opinion.
Attachment #8712014 - Attachment is obsolete: true
Attachment #8712014 - Flags: review?(jonas)
Flags: needinfo?(mak77)
(In reply to Marco Bonardo [::mak] from comment #8)
> (In reply to Jonas Sicking (:sicking) from comment #6)
> > For nsLivemarkService.js, does a website decide which URL to load? I.e. did
> > we get the URL to load from a website?
> > 
> > Do we know which website? Is it the website that we've bookmarked?
> 
> When visiting a website we have detection that sniffs <link> tags pointing
> to rss/atom feeds, this happens in browser code. From there we open the feed
> preview page (implemented in browser/feeds) (either through a link on the
> page itself or through the RSS toolbar button in Firefox UI).
> At this point the user can add the feed to livemarks, but we don't
> pass-through nor save the original page that contained the feed. So the
> information you are looking for is likely lost.
> The only thing we have in bookmarks storage is the feed URI (that is the xml
> uri) and eventually (not mandatory) a site URI that is ideally the
> originating URI, but it's taken from a tag in the xml contents themselves,
> so it could be easily forged and I don't think it's reliable at all.

Yeah, we should definitely not use that URI for any security checks. I.e. we should *not* use that URI to create any of the values passed to newChannel.

> But I sort of think we may just rely on a check done when we open the feed
> preview page? if that's unreachable then there's no way to add the unsafe
> livemark from the UI.

Yeah, using the URI of the feed to create a principal here sounds ok. We'll likely end up doing something similar for bookmarks.

We should add a comment saying that this is not a great solution though, so that this pattern doesn't get copied elsewhere.

We should also comment on that we'll need to figure out how to handle user contexts. I.e. how to grab the correct cookie jar. That's what the second argument to createCodebasePrincipal is. Dave Huseby is currently auditing all of our code to make sure that this is handled correctly.
(In reply to Christoph Kerschbaumer [:ckerschb] from comment #9)
> Created attachment 8713734 [details] [diff] [review]
> bug_1242857_js_asyncopen2_componets_places.patch
> 
> Mak, how hard would it actually be to make sure we pass a non null siteURI
> to writeSiteURI()

It's not reliable, it directly comes from the feed contents.
Flags: needinfo?(mak77)
(Assignee)

Comment 12

2 years ago
(In reply to Jonas Sicking (:sicking) from comment #10)
> Yeah, using the URI of the feed to create a principal here sounds ok. We'll
> likely end up doing something similar for bookmarks.
> 
> We should add a comment saying that this is not a great solution though, so
> that this pattern doesn't get copied elsewhere.
> 
> We should also comment on that we'll need to figure out how to handle user
> contexts. I.e. how to grab the correct cookie jar. That's what the second
> argument to createCodebasePrincipal is. Dave Huseby is currently auditing
> all of our code to make sure that this is handled correctly.

@Jonas: so in other words, we should keep what we have in the current patch right? (I will add those comments obviously).

@Mak: I suppose we can remove the checkLoadURI-checks in the patch, right, because those are done now whenever we call asyncOpen2(), right?
Flags: needinfo?(mak77)
Flags: needinfo?(jonas)
Comment on attachment 8713734 [details] [diff] [review]
bug_1242857_js_asyncopen2_componets_places.patch

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

::: toolkit/components/places/BookmarkJSONUtils.jsm
@@ +208,5 @@
>  
>        let uri = NetUtil.newURI(spec);
>        let channel = NetUtil.newChannel({
> +        uri: uri,
> +        loadUsingSystemPrincipal: true

Looks good

::: toolkit/components/places/nsLivemarkService.js
@@ +492,1 @@
>        secMan.checkLoadURIWithPrincipal(feedPrincipal, aSiteURI,

before commenting further on this code, can someone fill me in on what aSiteURI and this.feedURI comes from?
(In reply to Jonas Sicking (:sicking) from comment #13)
> ::: toolkit/components/places/nsLivemarkService.js
> @@ +492,1 @@
> >        secMan.checkLoadURIWithPrincipal(feedPrincipal, aSiteURI,
> 
> before commenting further on this code, can someone fill me in on what
> aSiteURI and this.feedURI comes from?

I thought comment 8 was enough. feedURI is the xml url, it comes from the browser RSS preview page. siteURI is one of the values written inside the xml (it's the "main" site the rss refers to). We don't load from it, we provide sort-of a bookmark to it the user can click to visit.

(In reply to Christoph Kerschbaumer [:ckerschb] from comment #12)
> @Mak: I suppose we can remove the checkLoadURI-checks in the patch, right,
> because those are done now whenever we call asyncOpen2(), right?

I think so? I assume the reason for these changes is to centralize checks.
What we do currently is build a principal out of feedURI and use DISALLOW_INHERIT_PRINCIPAL, if the channel checks are on par or better, we are fine. But I don't know, I didn't work on asyncOpen2, nor had the time to go reading its code yet.
Flags: needinfo?(mak77)
(In reply to Marco Bonardo [::mak] from comment #14)
> (In reply to Jonas Sicking (:sicking) from comment #13)
> > ::: toolkit/components/places/nsLivemarkService.js
> > @@ +492,1 @@
> > >        secMan.checkLoadURIWithPrincipal(feedPrincipal, aSiteURI,
> > 
> > before commenting further on this code, can someone fill me in on what
> > aSiteURI and this.feedURI comes from?
> 
> I thought comment 8 was enough.

Sorry, comment 8 was very helpful. It just wasn't clear to me where the writeSiteURI function fit in.

> feedURI is the xml url, it comes from the
> browser RSS preview page. siteURI is one of the values written inside the
> xml (it's the "main" site the rss refers to). We don't load from it, we
> provide sort-of a bookmark to it the user can click to visit.

Ok. We can remove this check *if* that check is performed when the user clicks the link. I.e. if we end up correctly passing the XML feed as the referrer to the docshell code when the user clicks the link.

Basically what we want to ensure is that if the feed contains a link to a file:// URL, that that link doesn't work.

Note that switching to AsyncOpen2 when loading the XML feed doesn't help with that at all.


The other thing I was wondering, independent of the discussion in this bug, is do we have any security checks which prevent a page from pointing to an XML feed from file://? I.e. do we do any checkLoadURI checks before we start the process of processing the XML feed <link>?
Flags: needinfo?(jonas)
(In reply to Jonas Sicking (:sicking) from comment #15)
> > feedURI is the xml url, it comes from the
> > browser RSS preview page. siteURI is one of the values written inside the
> > xml (it's the "main" site the rss refers to). We don't load from it, we
> > provide sort-of a bookmark to it the user can click to visit.
> 
> Ok. We can remove this check *if* that check is performed when the user
> clicks the link. I.e. if we end up correctly passing the XML feed as the
> referrer to the docshell code when the user clicks the link.

Clicking on a bookmark just opens a url, no referrer has ever been supported there.
Supporting a referrer when clicking bookmarks would require a separate bug and it would also require an assignee, since there's no Places team anymore.

Unless by clicking the link you mean from the RSS feed preview page, but there the siteURI is not made visible, when clicking the article links we send the referer.

> The other thing I was wondering, independent of the discussion in this bug,
> is do we have any security checks which prevent a page from pointing to an
> XML feed from file://? I.e. do we do any checkLoadURI checks before we start
> the process of processing the XML feed <link>?

I think that's it:
http://mxr.mozilla.org/mozilla-central/source/browser/modules/ContentLinkHandler.jsm#64
64             if (Feeds.isValidFeed(link, link.ownerDocument.nodePrincipal, "feed" in rels)) {
65               chromeGlobal.sendAsyncMessage("Link:AddFeed",
66                                             {type: link.type,
67                                              href: link.href,
68                                              title: link.title});

http://mxr.mozilla.org/mozilla-central/source/browser/modules/Feeds.jsm#90
90         BrowserUtils.urlSecurityCheck(aLink.href, principalToCheck,
91                                       Ci.nsIScriptSecurityManager.DISALLOW_INHERIT_PRINCIPAL);
(In reply to Marco Bonardo [::mak] from comment #16)
> (In reply to Jonas Sicking (:sicking) from comment #15)
> > > feedURI is the xml url, it comes from the
> > > browser RSS preview page. siteURI is one of the values written inside the
> > > xml (it's the "main" site the rss refers to). We don't load from it, we
> > > provide sort-of a bookmark to it the user can click to visit.
> > 
> > Ok. We can remove this check *if* that check is performed when the user
> > clicks the link. I.e. if we end up correctly passing the XML feed as the
> > referrer to the docshell code when the user clicks the link.
> 
> Clicking on a bookmark just opens a url, no referrer has ever been supported
> there.

The docshell code does various security checks if you pass it the "correct" parameters. Mainly it's there to prevent doing things like linking to file://. I forget which exact arguments you should pass it, and how to do that.

> Supporting a referrer when clicking bookmarks would require a separate bug
> and it would also require an assignee, since there's no Places team anymore.

I agree that passing the correct values to the docshell code when the URL is opened sounds like a separate bug.

However until we've done that we can't remove the security checks in writeSiteURI, since those are currently the only thing that prevents a RSS feed from containing a file:// URL and then sending the user to that.

> Unless by clicking the link you mean from the RSS feed preview page, but
> there the siteURI is not made visible, when clicking the article links we
> send the referer.

Sorry, I don't know what the feed UI looks like well enough to use the right vocabulary here.

It sounded like the RSS feed has a feature which enables it to say "this is the URL of the webpage I'm coming from", and then our UI had some way of enabling the user to navigate to that URL.

What I mean is that we need some way to prevent that URL (as well as other URLs that the user can click on in the feed UI) from linking to file:// URLs.

> > The other thing I was wondering, independent of the discussion in this bug,
> > is do we have any security checks which prevent a page from pointing to an
> > XML feed from file://? I.e. do we do any checkLoadURI checks before we start
> > the process of processing the XML feed <link>?
> 
> I think that's it:
> http://mxr.mozilla.org/mozilla-central/source/browser/modules/
> ContentLinkHandler.jsm#64
> 64             if (Feeds.isValidFeed(link, link.ownerDocument.nodePrincipal,
> "feed" in rels)) {
> 65               chromeGlobal.sendAsyncMessage("Link:AddFeed",
> 66                                             {type: link.type,
> 67                                              href: link.href,
> 68                                              title: link.title});
> 
> http://mxr.mozilla.org/mozilla-central/source/browser/modules/Feeds.jsm#90
> 90         BrowserUtils.urlSecurityCheck(aLink.href, principalToCheck,
> 91                                      
> Ci.nsIScriptSecurityManager.DISALLOW_INHERIT_PRINCIPAL);

Cool, yeah, this sounds good. We should keep these checks.
(In reply to Jonas Sicking (:sicking) from comment #17)
> It sounded like the RSS feed has a feature which enables it to say "this is
> the URL of the webpage I'm coming from", and then our UI had some way of
> enabling the user to navigate to that URL.

The feed xml contains a url that "should" be the main website url (siteURI), but could be anything (nsIFeedContainer::link). Then it contains links to articles, that again could be anything (nsIFeed::items).
The livemarks folders at the top have an entry pointing to that url, followed by the article links (also coming from the xml)
You can try the UI in bugzilla bugs lists using the rss button and adding to the bookmarks toolbar, for example.

> What I mean is that we need some way to prevent that URL (as well as other
> URLs that the user can click on in the feed UI) from linking to file:// URLs.

Off-hand I don't think it's duty of the livemarks/bookmarks service to handle that, in the end it's just a storage component.
Probably it's the feed processor (http://mxr.mozilla.org/mozilla-central/source/toolkit/components/feeds/FeedProcessor.js) that should remove any url that doesn't pass security checks, before passing back the data.
Unfortunately that component is fairly over-engineered, lots of pointless XPCOM inheritance, when a simple jsm could likely do the same in half the code, and I don't think anyone will spend time on it.
But if we are going to look into securing feeds, I think FeedProcessor.js is the file to look at.
(Assignee)

Comment 19

2 years ago
Created attachment 8727937 [details] [diff] [review]
bug_1242857_js_asyncopen2_componets_places.patch

Jonas, Mak, sorry this bug has been sitting around for so long.

@BookmarkJSONUtils.jsm:
As agreed in Comment 7, we should use NetUtil.newChannel({loadUsingSystemPrincipal: true}) for BookmarkJSONUtils - easy :-)

@nsLivemarkService.js:
To sum it up we should keep the securityChecks within writeSiteURI() to make sure feeds can't link to file:// URIs. Further, we agreed to use the feedURI to generate a codeBasePrincipal which we then use as a loadingPrincipal (See Comment 10). Even though that's not best practice, we should make an exception for nsLivemarkService.js but add a comment so this pattern does not get copied around in the codebase.

Everything else discussed in this bug should handled in a separate bug and seems outside the scope of the changeset here.

Do we all agree?
Attachment #8713734 - Attachment is obsolete: true
Attachment #8727937 - Flags: review?(mak77)
Attachment #8727937 - Flags: review?(jonas)

Updated

2 years ago
Attachment #8727937 - Flags: review?(mak77) → review+
Comment on attachment 8727937 [details] [diff] [review]
bug_1242857_js_asyncopen2_componets_places.patch

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

::: toolkit/components/places/nsLivemarkService.js
@@ +537,2 @@
>          loadingPrincipal: Services.scriptSecurityManager.createCodebasePrincipal(this.feedURI, {}),
> +        securityFlags: Ci.nsILoadInfo.SEC_REQUIRE_CORS_DATA_INHERITS,

Use SEC_REQUIRE_SAME_ORIGIN_DATA_IS_BLOCKED.

Since we're creating the principal using the URI that we're loading, they'll for sure be same-origin. So only if a redirect happens will we block anything.
Attachment #8727937 - Flags: review?(jonas) → review+
(Assignee)

Comment 21

2 years ago
Created attachment 8732280 [details] [diff] [review]
bug_1242857_js_asyncopen2_componets_places.patch

(In reply to Jonas Sicking (:sicking) from comment #20)
> Comment on attachment 8727937 [details] [diff] [review]
> bug_1242857_js_asyncopen2_componets_places.patch
> 
> Review of attachment 8727937 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: toolkit/components/places/nsLivemarkService.js
> @@ +537,2 @@
> >          loadingPrincipal: Services.scriptSecurityManager.createCodebasePrincipal(this.feedURI, {}),
> > +        securityFlags: Ci.nsILoadInfo.SEC_REQUIRE_CORS_DATA_INHERITS,
> 
> Use SEC_REQUIRE_SAME_ORIGIN_DATA_IS_BLOCKED.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=825c37e6ee62
Attachment #8727937 - Attachment is obsolete: true
Attachment #8732280 - Flags: review+
(Assignee)

Comment 22

2 years ago
That is ready to go in now!
Keywords: checkin-needed

Comment 23

2 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/5c7786b4bec5
Keywords: checkin-needed
(Assignee)

Updated

2 years ago
Whiteboard: [domsecurity-active]

Comment 24

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