Closed
Bug 353734
Opened 18 years ago
Closed 18 years ago
RSS icon will load javascript:/data:/chrome: URLs
Categories
(Firefox Graveyard :: RSS Discovery and Preview, defect)
Tracking
(Not tracked)
RESOLVED
FIXED
Firefox 2
People
(Reporter: philor, Assigned: sayrer)
References
()
Details
(Keywords: fixed1.8.1, Whiteboard: [sg:moderate?] post ff1.5)
Attachments
(3 files, 2 obsolete files)
320 bytes,
text/html
|
Details | |
2.33 KB,
patch
|
asaf
:
review+
dveditz
:
approval1.8.0.8-
mtschrep
:
approval1.8.1+
|
Details | Diff | Splinter Review |
23.14 KB,
image/png
|
Details |
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.
Reporter | ||
Updated•18 years ago
|
Flags: blocking-firefox2?
Reporter | ||
Comment 1•18 years ago
|
||
Three <link>s, for a javascript: alert, some data: HTML, and chrome://global/content/filepicker.xul
Assignee | ||
Updated•18 years ago
|
Assignee: nobody → sayrer
Assignee | ||
Comment 2•18 years ago
|
||
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)
Reporter | ||
Comment 3•18 years ago
|
||
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).
Assignee | ||
Comment 4•18 years ago
|
||
(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?
Reporter | ||
Comment 5•18 years ago
|
||
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?
Assignee | ||
Comment 6•18 years ago
|
||
(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•18 years ago
|
||
> 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•18 years ago
|
||
And yes, don't wait on bug 120373 on trunk; just remind me to merge after you land?
Assignee | ||
Comment 9•18 years ago
|
||
(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•18 years ago
|
||
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.
Assignee | ||
Comment 11•18 years ago
|
||
Attachment #239609 -
Attachment is obsolete: true
Attachment #239619 -
Flags: review?(mano)
Attachment #239609 -
Flags: review?(mano)
Assignee | ||
Comment 12•18 years ago
|
||
Attachment #239619 -
Attachment is obsolete: true
Attachment #239620 -
Flags: review?(mano)
Attachment #239619 -
Flags: review?(mano)
Comment 13•18 years ago
|
||
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.
Assignee | ||
Comment 14•18 years ago
|
||
(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•18 years ago
|
||
Not sure what comment 14 means...
Assignee | ||
Comment 16•18 years ago
|
||
(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.
Assignee | ||
Comment 17•18 years ago
|
||
Comment 18•18 years ago
|
||
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...
Assignee | ||
Comment 19•18 years ago
|
||
(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•18 years ago
|
||
Robert, why do you need to run security checks in both subscribeToFeed and in onLinkAdded? Isn't the later enough?
Assignee | ||
Comment 21•18 years ago
|
||
(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•18 years ago
|
||
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+
Updated•18 years ago
|
Attachment #239620 -
Flags: approval1.8.1?
Updated•18 years ago
|
Flags: blocking-firefox2? → blocking-firefox2+
Comment 23•18 years ago
|
||
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 24•18 years ago
|
||
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+
Comment 25•18 years ago
|
||
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
Updated•18 years ago
|
Whiteboard: [sg:moderate?] → [sg:moderate?] post ff1.5
Assignee | ||
Comment 26•18 years ago
|
||
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: 18 years ago
Resolution: --- → FIXED
Assignee | ||
Comment 27•18 years ago
|
||
*** Bug 312108 has been marked as a duplicate of this bug. ***
Assignee | ||
Updated•18 years ago
|
Attachment #239620 -
Flags: approval1.8.0.8?
Comment 28•18 years ago
|
||
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-
Updated•18 years ago
|
Group: security
Updated•6 years ago
|
Product: Firefox → Firefox Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•