The default bug view has changed. See this FAQ.
Bug 758990 (CVE-2012-1965)

Don't allow feed: URLs with an innerURI that inherits the page's security context

RESOLVED FIXED in Firefox 14

Status

()

Firefox
RSS Discovery and Preview
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: Jesse Ruderman, Assigned: philor)

Tracking

({sec-moderate, testcase})

Trunk
Firefox 15
sec-moderate, testcase
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox14+ fixed, firefox15+ fixed, firefox16+ fixed, firefox-esr1014+ fixed)

Details

(Whiteboard: [advisory-tracking+][qa-])

Attachments

(2 attachments)

(Reporter)

Description

5 years ago
Created attachment 627593 [details]
testcase

"feed:javascript:" URLs can be used to XSS sites that just blacklist "javascript:" and "data:".

(Issue raised by Mario Gomes in bug 758907 comment 5, and brought to my attention by Reed Loden.)
(Reporter)

Updated

5 years ago
Duplicate of this bug: 758989
Component: Networking → RSS Discovery and Preview
OS: Mac OS X → All
Product: Core → Firefox
QA Contact: networking → rss.preview
Hardware: x86_64 → All

Comment 2

5 years ago
Why can't I see bug 758907?
(Assignee)

Comment 3

5 years ago
I'm not terribly excited about it, but since they'll never actually be useful for anything, because the parser will refuse to parse them and the FeedConverter makes the foolish decision that if it can't be parsed then it won't even look at the user's prefs to see if he wants it handed off to someone else, it will instead preview it except it won't preview it because it can't be parsed, we might as well just not create the URI.
Assignee: nobody → philringnalda
Status: NEW → ASSIGNED
Summary: Don't allow "feed:javascript:" URLs → Don't allow feed: URLs with an innerURI that inherits the page's security context
(Assignee)

Comment 4

5 years ago
Created attachment 627634 [details] [diff] [review]
fix v.1

The bits touching test_355473.js are because bug 408599 left some things behind when it changed the meaning of the test, after it stopped being a test for "don't create feed: URIs from these things" and became a "create sane feed: URIs from these other things" test.

The checking for inherits part and the test I'm reasonably confident about, since I just stole the former from nsBrowserContentHandler.js and the latter from test_bug261425.js. Cr.NS_ERROR_MALFORMED_URI for Cr.NS_JUST_DON'T_WANT_TO_CREATE_THAT is a little iffy, but... every feed: URI is a malformed URI by spec, that's part of why you can't even find the spec anymore. Close enough for things we'll only be saying to the malicious and to foolish dreamers whose hopes would soon be dashed anyway.

https://tbpl.mozilla.org/?tree=Try&rev=d75b554103db
Attachment #627634 - Flags: review?(gavin.sharp)

Comment 5

5 years ago
Comment on attachment 627634 [details] [diff] [review]
fix v.1

>+    var netutil = Cc["@mozilla.org/network/util;1"].getService(Ci.nsINetUtil);
...
>     var uri = Cc["@mozilla.org/network/util;1"].

>-  var ftpFeedURI = ios.newURI("feed:ftp://example.com/feed.xml", null, null);
>-  var fileFeedURI = ios.newURI("feed:file:///var/feed.xml", null, null);
What's wrong with these two?
Comment on attachment 627634 [details] [diff] [review]
fix v.1

>diff --git a/browser/components/feeds/test/unit/test_355473.js b/browser/components/feeds/test/unit/test_355473.js

>-  var dataFeedURI = ios.newURI("feed:data:text/xml,<rss/>", null, null);
>-  var ftpFeedURI = ios.newURI("feed:ftp://example.com/feed.xml", null, null);
>-  var fileFeedURI = ios.newURI("feed:file:///var/feed.xml", null, null);

(In reply to Phil Ringnalda (:philor) from comment #4)
> The bits touching test_355473.js are because bug 408599 left some things
> behind when it changed the meaning of the test, after it stopped being a
> test for "don't create feed: URIs from these things" and became a "create
> sane feed: URIs from these other things" test.

Not sure I understand - as Neil suggests, seems like it doesn't hurt to continue testing that creating feed:file and feed:ftp URIs works. The rest looks good.
Attachment #627634 - Flags: review?(gavin.sharp) → review+

Comment 7

5 years ago
(In reply to comment #5)
> (From update of attachment 627634 [details] [diff] [review])
> >+    var netutil = Cc["@mozilla.org/network/util;1"].getService(Ci.nsINetUtil);
> ...
> >     var uri = Cc["@mozilla.org/network/util;1"].
Sorry, I should have said that this should now use the netutil service above.

Updated

5 years ago
Blocks: 759382
(Assignee)

Comment 8

5 years ago
Yeah, all that's wrong with the ftpFeedURI and fileFeedURI tests is that they aren't beneath a comment saying "// Even though we don't care about these enough to test what we create from them, we still want to test that we don't throw creating them like we used to. Until someone decides we do want to."

Comment 9

5 years ago
Comment on attachment 627634 [details] [diff] [review]
fix v.1

>+    const URI_INHERITS_SECURITY_CONTEXT = Ci.nsIHttpProtocolHandler
>+                                            .URI_INHERITS_SECURITY_CONTEXT;
Nit: nsIProtocolHandler
(Assignee)

Comment 10

5 years ago
I guess since you've been waiting 4 years, 8 months, and 16 days for someone to heed that nsIProtocolHandler nit, I better fix that.
(Assignee)

Comment 11

5 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/a8e98b1326b9
Target Milestone: --- → Firefox 15
Ah, that's the right place.

Well, Just let me know if this be eligible for a bounty? If so, I'll have to cc the real finder of this bug.
https://hg.mozilla.org/mozilla-central/rev/a8e98b1326b9
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Ccing the real finder.

@irsdl: I have used your "feed:javascript" to exploit anouther bug, and they think that this is a bug, so Ccing real finder.

Please talk about credits and bounty(if be eligible) with @irsdl.

Comment 15

5 years ago
@Mario despite the fact I found this type of XSS, I do not think I am eligible to get the bounty as I have not reported it in this format ;) I am currently using this "feed" protocol to find a more interesting bypasses. For example: https://bugzilla.mozilla.org/show_bug.cgi?id=732654 even reported sooner than this bug.
status-firefox-esr10: --- → wontfix
status-firefox14: --- → wontfix
status-firefox15: --- → fixed
status-firefox16: --- → fixed
tracking-firefox15: --- → +
tracking-firefox16: --- → +
Reconsidering that ESR wontfix -- this looks low-risk enough to take and may endanger some sites.
status-firefox-esr10: wontfix → affected
status-firefox14: wontfix → affected
tracking-firefox-esr10: --- → 14+
tracking-firefox14: --- → +

Comment 18

5 years ago
I will give you my last word, it can lead to bypass a lot of protections in different places.
I will give you several examples:
1- feed protocol can trigger events such as onbeforeunload: https://bugzilla.mozilla.org/show_bug.cgi?id=732654
2- it can cause problem for other applications which are based on blacklist filtering and does not have any information about this unique feature of Firefox: http://www.longtailvideo.com/support/forums/jw-player/bug-reports/27228/xss-vulnerability-by-using-feed-protocol-in-firefox
Blocks: 732654

Comment 20

5 years ago
Please let me know if anyone like myself can get a bounty here. I have not created this report, but I was the finder. At the same time, this issue is public.
Comment on attachment 627634 [details] [diff] [review]
fix v.1

[Approval Request Comment]
Bug caused by (feature/regressing bug #): longstanding security issue
User impact if declined: more potential security bugs
Testing completed (on m-c, etc.): has been on m-c since June 2, no known issues
Risk to taking this patch (and alternatives if risky): very low risk. these URIs don't work anyways, but allowing them can create security bugs.
String or UUID changes made by this patch: none
Attachment #627634 - Flags: approval-mozilla-beta?
Attachment #627634 - Flags: approval-mozilla-aurora?
Comment on attachment 627634 [details] [diff] [review]
fix v.1

[triage comment]
thanks for the risk assessment, this has been around on m-c long enough that I don't think we're taking on too much if we put this in the next beta.
Attachment #627634 - Flags: approval-mozilla-beta?
Attachment #627634 - Flags: approval-mozilla-beta+
Attachment #627634 - Flags: approval-mozilla-aurora?
Attachment #627634 - Flags: approval-mozilla-aurora+
https://hg.mozilla.org/releases/mozilla-beta/rev/31db0495d56a
status-firefox14: affected → fixed
Comment on attachment 627634 [details] [diff] [review]
fix v.1

(this was already on aurora)
Attachment #627634 - Flags: approval-mozilla-aurora+
Comment on attachment 627634 [details] [diff] [review]
fix v.1

[Approval Request Comment]
see comment 21, same applies to ESR (tested patch+test locally)
Attachment #627634 - Flags: approval-mozilla-esr10?
Comment on attachment 627634 [details] [diff] [review]
fix v.1

low risk, please go ahead and land to ESR branch
Attachment #627634 - Flags: approval-mozilla-esr10? → approval-mozilla-esr10+
https://hg.mozilla.org/releases/mozilla-esr10/rev/a612c98a839e
status-firefox-esr10: affected → fixed
Whiteboard: [advisory-tracking+]
Alias: CVE-2012-1965
Group: core-security
Can this testcase be put in-testsuite? Does it need a manual test?
Whiteboard: [advisory-tracking+] → [advisory-tracking+][qa?]
(Assignee)

Comment 29

5 years ago
I probably would have set it at the time, since I did land a test with the patch, but I just had to view-source to even find the silly flag.
Flags: in-testsuite+
Thanks Phil. Marking qa- since this is in-testsuite. If you think this needs a manual test please nominate for in-moztrap.
Whiteboard: [advisory-tracking+][qa?] → [advisory-tracking+][qa-]
You need to log in before you can comment on or make changes to this bug.