Closed Bug 758990 (CVE-2012-1965) Opened 8 years ago Closed 8 years ago

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

Categories

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

defect
Not set

Tracking

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

RESOLVED FIXED
Firefox 15
Tracking Status
firefox14 + fixed
firefox15 + fixed
firefox16 + fixed
firefox-esr10 14+ fixed

People

(Reporter: jruderman, Assigned: philor)

References

Details

(Keywords: sec-moderate, testcase, Whiteboard: [advisory-tracking+][qa-])

Attachments

(2 files)

Attached file 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.)
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
Why can't I see bug 758907?
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
Attached patch fix v.1Splinter Review
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 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+
(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.
Blocks: 759382
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 on attachment 627634 [details] [diff] [review]
fix v.1

>+    const URI_INHERITS_SECURITY_CONTEXT = Ci.nsIHttpProtocolHandler
>+                                            .URI_INHERITS_SECURITY_CONTEXT;
Nit: nsIProtocolHandler
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.
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
Closed: 8 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.
@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.
Reconsidering that ESR wontfix -- this looks low-risk enough to take and may endanger some sites.
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
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+
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+
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?]
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-]
Product: Firefox → Firefox Graveyard
You need to log in before you can comment on or make changes to this bug.