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

RESOLVED FIXED in Firefox 2

Status

defect
--
major
RESOLVED FIXED
13 years ago
7 months ago

People

(Reporter: philor, Assigned: sayrer)

Tracking

({fixed1.8.1})

Dependency tree / graph
Bug Flags:
blocking-firefox2 +

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [sg:moderate?] post ff1.5, )

Attachments

(3 attachments, 2 obsolete attachments)

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.
Flags: blocking-firefox2?
Posted file Testcase
Three <link>s, for a javascript: alert, some data: HTML, and chrome://global/content/filepicker.xul
Assignee: nobody → sayrer
The wrapper.href attribute seems to have the URL already resolved, so we can just run it through the security manager.
Attachment #239609 - Flags: review?(mano)
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).
(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?
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?
(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?
> 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).
And yes, don't wait on bug 120373 on trunk; just remind me to merge after you land?
(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.
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.
Attachment #239609 - Attachment is obsolete: true
Attachment #239619 - Flags: review?(mano)
Attachment #239609 - Flags: review?(mano)
Attachment #239619 - Attachment is obsolete: true
Attachment #239620 - Flags: review?(mano)
Attachment #239619 - Flags: review?(mano)
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.
(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. 
Not sure what comment 14 means...
(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.
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...
(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

Robert, why do you need to run security checks in both subscribeToFeed and in onLinkAdded? Isn't the later enough?
(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 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.
Attachment #239620 - Flags: review?(mano) → review+
Flags: blocking-firefox2? → blocking-firefox2+
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.
Whiteboard: [sg:moderate?]
Comment on attachment 239620 [details] [diff] [review]
check again onclick, allow useless mailnews protocols in link elements

Approved for RC2.
Attachment #239620 - Flags: approval1.8.1? → approval1.8.1+
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
Keywords: fixed1.8.1
Whiteboard: [sg:moderate?] → [sg:moderate?] post ff1.5
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
Status: NEW → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
*** Bug 312108 has been marked as a duplicate of this bug. ***
Attachment #239620 - Flags: approval1.8.0.8?
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
Attachment #239620 - Flags: approval1.8.0.8? → approval1.8.0.8-
Group: security
Product: Firefox → Firefox Graveyard
You need to log in before you can comment on or make changes to this bug.