353734 - RSS icon will load javascript:/data:/chrome: URLs

Status: RESOLVED FIXED

(Firefox Graveyard :: RSS Discovery and Preview, defect)

Description

[Phil Ringnalda (:philor)]

http://lxr.mozilla.org/mozilla1.8/source/browser/base/content/browser.js#6715 merrily adds whatever link.href it's offered, which used to be the "no real problem" bug 312108, but now with the feed preview parser, we load that URI when the user clicks the icon in the addressbar, whether it's javascript:alert(document.cookie) or chrome://global/content/filepicker.xul, or data:whatever.

I'm pretty sure we don't want to do that, even though we dodged a bullet by accidently "fixing" bug 305472 - we don't discover feeds in frames at all right now, so you can't run script in your third-party parent's context, but you can still run script when the user has it disabled, if you can talk them into clicking your feed icon, or you can load chrome: and data: that would otherwise be forbidden.

Comment 1

[Phil Ringnalda (:philor)]

Attachment: Testcase

Three <link>s, for a javascript: alert, some data: HTML, and chrome://global/content/filepicker.xul

Comment 2

[Robert Sayre]

Attachment: run the feed icon URL through secMan

The wrapper.href attribute seems to have the URL already resolved, so we can just run it through the security manager.

Comment 3

[Phil Ringnalda (:philor)]

From a conversation with bz last night, you don't actually want DISALLOW_FROM_MAIL, which is a no-op unless you somehow get a mailbox:, imap:, or news: URL loaded in the browser and having feeds autodiscovered in it (in which case it would refuse them all).

Comment 4

[Robert Sayre]

(In reply to comment #3)
> From a conversation with bz last night, you don't actually want
> DISALLOW_FROM_MAIL, which is a no-op unless...

Why wouldn't we want to disallow from mail?

Comment 5

[Phil Ringnalda (:philor)]

Two reasons off the top of my head:

1) It will only exist until bz lands his patch on bug 120373, on the trunk at least, at which point you'll be switched to just the new name for DISALLOW_SCRIPT_OR_DATA anyway.

2) It's voodoo security: Firefox can't load any of those schemes, much less parse a <link> tag out of them, so it just does three unnecessary LowerCaseEqualsLiteral() checks for no reason, while making casual readers think we're protecting against something other than the impossible.

But, supposing we did load mailbox: URIs in the browser, and did parse <link> elements in them: why would you want to absolutely refuse feeds from them, rather than allowing just the same protocols you would ordinarily allow?

Comment 6

[Robert Sayre]

(In reply to comment #5)
> Two reasons off the top of my head:
> 
> 1) It will only exist until bz lands his patch on bug 120373, on the trunk at
> least,

This bug should block the branch, too.

> 
> 2) It's voodoo security: Firefox can't load any of those schemes

Why do we want to allow them?

Comment 7

[Boris Zbarsky [:bzbarsky]]

> Why do we want to allow them?

It's not about allowing "them".  It's about allowing things to be loaded from "them".

The point of DISALLOW_FROM_MAIL is to prevent loads that require no user interaction from mail messages (meta refresh, that sort of thing).  The patch in bug 120373 makes this clearer by removing the "MAIL" part and making it clear just what the flag means.  Now in your case, you have user interaction.  The user clicked something.  Modulo actual security issues, the user probably intends something to happen, so there's no reason to pretend that we might not let it happen (even aside from the fact that we don't actually prevent anything with that flag -- it's a complete no-op in Firefox right now).

Comment 8

[Boris Zbarsky [:bzbarsky]]

And yes, don't wait on bug 120373 on trunk; just remind me to merge after you land?

Comment 9

[Robert Sayre]

(In reply to comment #7)
> Now in your case, you have user interaction. 
> The user clicked something.  

No, this event fires when a link element is detected. It's what allows the little feed icon to show up in the first place. hmm, we should probably check this again onclick.

Comment 10

[Boris Zbarsky [:bzbarsky]]

Oh, I see.   But the point is that the load won't happen without user interaction, right?  That is, it's not like we're asking "can we blow away the current page even though the user didn't ask us to and script is disabled?"  That's what the DISALLOW_FROM_MAIL thing is intended for, pretty much.

Comment 11

[Robert Sayre]

Attachment: check again onclick, allow useless mailnews protocols in link elements

Comment 12

[Robert Sayre]

Attachment: check again onclick, allow useless mailnews protocols in link elements

Comment 13

[Boris Zbarsky [:bzbarsky]]

You're not allowing them in link elements.  That's the point.  You're allowing link elements in a page loaded from a useless mailnews protocol to act like link elements in a page loaded from http or file or whatever.  The link element itself is most likely an http link, of course.

Comment 14

[Robert Sayre]

(In reply to comment #7)
>
> (even aside from the fact that we don't actually prevent anything
> with that flag -- it's a complete no-op in Firefox right now).

From the feed icon, we appear to get the technobabble dialog about registered protocol handlers. 

Comment 15

[Boris Zbarsky [:bzbarsky]]

Not sure what comment 14 means...

Comment 16

[Robert Sayre]

(In reply to comment #13)
> You're allowing
> link elements in a page loaded from a useless mailnews protocol 

Oh, I was confused from testing with a file:// URL.

Comment 17

[Robert Sayre]

Attachment: screenshot of protocol handler dialog

Comment 18

[Boris Zbarsky [:bzbarsky]]

Ah, I see.  But the DISALLOW_FROM_MAIL check won't prevent that dialog.  Again, it only has an effect if the _originating_ URI is news:, mailbox:, or imap:.

If you want to prevent that alert, check whether there is actually an exposed protocol handler...

Comment 19

[Robert Sayre]

(In reply to comment #18)
> Ah, I see.  But the DISALLOW_FROM_MAIL check won't prevent that dialog.  Again,
> it only has an effect if the _originating_ URI is news:, mailbox:, or imap:.
> 

yeah, I get it now

> If you want to prevent that alert

different bug

Comment 20

[Asaf Romano (gone)]

Robert, why do you need to run security checks in both subscribeToFeed and in onLinkAdded? Isn't the later enough?

Comment 21

[Robert Sayre]

(In reply to comment #20)
> Robert, why do you need to run security checks in both subscribeToFeed and in
> onLinkAdded? Isn't the later enough?
> 

We don't want to show the feeds in the dropdown if we're not going to allow the click. I added the checks to subscribeToFeed in case someone figures out a way to bypass onLinkAdded.

Comment 22

[Asaf Romano (gone)]

Comment on attachment 239620 [details] [diff] [review]
check again onclick, allow useless mailnews protocols in link elements

I cannot see how would that happen, but it doesn't hurt.

>Index: browser/base/content/browser.js
>===================================================================

> #else
> #ifdef MOZ_PLACES
>+      urlSecurityCheck(href, gBrowser.currentURI.spec,
>+                       Ci.nsIScriptSecurityManager.DISALLOW_SCRIPT_OR_DATA);
>       PlacesCommandHook.addLiveBookmark(feeds[0].href);
> #else
>+      urlSecurityCheck(href, gBrowser.currentURI.spec,
>+                       Ci.nsIScriptSecurityManager.DISALLOW_SCRIPT_OR_DATA);
>       this.addLiveBookmark(feeds[0].href);
> #endif
> #endif

nit: you can put the security check outside of the #ifdef block.


r=mano.

Comment 23

[Daniel Veditz [:dveditz]]

javascript: is not given chrome privs so that's good. Could be a XSS hole if such a feed url was discovered in a 3rd party feed should that ever start working.

chrome: is potentially dangerous, especially if we (or any extension!) has added one that parses query parameters and either has dangerous parameters or is susceptible to script injection. I don't know of any, but there are 1700 extensions out there so I'm sure it's been done.

Comment 24

[Mike Schroepfer]

Comment on attachment 239620 [details] [diff] [review]
check again onclick, allow useless mailnews protocols in link elements

Approved for RC2.

Comment 25

[Asaf Romano (gone)]

Landed this on branch w/o fixing this nit since the code there is all broken anyway (looks like we managed to break #ifndef MOZ_FEEDS builds).

mozilla/browser/base/content/browser.js 1.479.2.208

Comment 26

[Robert Sayre]

Checking in browser/base/content/browser.js;
/cvsroot/mozilla/browser/base/content/browser.js,v  <--  browser.js
new revision: 1.715; previous revision: 1.714
done

Comment 27

[Robert Sayre]

*** Bug 312108 has been marked as a duplicate of this bug. ***

Comment 28

[Daniel Veditz [:dveditz]]

Comment on attachment 239620 [details] [diff] [review]
check again onclick, allow useless mailnews protocols in link elements

This patch doesn't even come close to applying to the 1.8.0 branch