Last Comment Bug 758990 - (CVE-2012-1965) Don't allow feed: URLs with an innerURI that inherits the page's security context
(CVE-2012-1965)
: Don't allow feed: URLs with an innerURI that inherits the page's security con...
Status: RESOLVED FIXED
[advisory-tracking+][qa-]
: sec-moderate, testcase
Product: Firefox
Classification: Client Software
Component: RSS Discovery and Preview (show other bugs)
: Trunk
: All All
: -- normal (vote)
: Firefox 15
Assigned To: Phil Ringnalda (:philor, back in August)
:
Mentors:
: 758989 (view as bug list)
Depends on:
Blocks: xss 732654 759382
  Show dependency treegraph
 
Reported: 2012-05-27 14:18 PDT by Jesse Ruderman
Modified: 2012-08-14 15:31 PDT (History)
10 users (show)
philringnalda: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
+
fixed
+
fixed
+
fixed
14+
fixed


Attachments
testcase (74 bytes, text/html)
2012-05-27 14:18 PDT, Jesse Ruderman
no flags Details
fix v.1 (4.68 KB, patch)
2012-05-28 01:04 PDT, Phil Ringnalda (:philor, back in August)
gavin.sharp: review+
lukasblakk+bugs: approval‑mozilla‑beta+
lukasblakk+bugs: approval‑mozilla‑esr10+
Details | Diff | Splinter Review

Description Jesse Ruderman 2012-05-27 14:18:00 PDT
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.)
Comment 1 Jesse Ruderman 2012-05-27 14:20:52 PDT
*** Bug 758989 has been marked as a duplicate of this bug. ***
Comment 2 Giorgio Maone [:mao] 2012-05-27 15:24:22 PDT
Why can't I see bug 758907?
Comment 3 Phil Ringnalda (:philor, back in August) 2012-05-27 23:56:36 PDT
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.
Comment 4 Phil Ringnalda (:philor, back in August) 2012-05-28 01:04:07 PDT
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
Comment 5 neil@parkwaycc.co.uk 2012-05-29 00:39:20 PDT
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 6 :Gavin Sharp [email: gavin@gavinsharp.com] 2012-05-29 09:52:27 PDT
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.
Comment 7 neil@parkwaycc.co.uk 2012-05-29 09:59:14 PDT
(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.
Comment 8 Phil Ringnalda (:philor, back in August) 2012-05-29 10:36:02 PDT
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 neil@parkwaycc.co.uk 2012-05-29 11:09:21 PDT
Comment on attachment 627634 [details] [diff] [review]
fix v.1

>+    const URI_INHERITS_SECURITY_CONTEXT = Ci.nsIHttpProtocolHandler
>+                                            .URI_INHERITS_SECURITY_CONTEXT;
Nit: nsIProtocolHandler
Comment 10 Phil Ringnalda (:philor, back in August) 2012-05-30 22:02:40 PDT
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.
Comment 11 Phil Ringnalda (:philor, back in August) 2012-06-01 19:45:21 PDT
https://hg.mozilla.org/integration/mozilla-inbound/rev/a8e98b1326b9
Comment 12 Mario Gomes 2012-06-02 11:32:25 PDT
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.
Comment 14 Mario Gomes 2012-06-02 14:25:56 PDT
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 Soroush Dalili 2012-06-02 14:40:30 PDT
@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.
Comment 17 Daniel Veditz [:dveditz] 2012-06-22 10:53:17 PDT
Reconsidering that ESR wontfix -- this looks low-risk enough to take and may endanger some sites.
Comment 18 Soroush Dalili 2012-06-22 11:43:37 PDT
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
Comment 20 Soroush Dalili 2012-06-25 16:53:40 PDT
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 21 :Gavin Sharp [email: gavin@gavinsharp.com] 2012-06-26 14:30:18 PDT
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
Comment 22 Lukas Blakk [:lsblakk] use ?needinfo 2012-06-26 16:55:13 PDT
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.
Comment 23 :Gavin Sharp [email: gavin@gavinsharp.com] 2012-06-26 17:14:08 PDT
https://hg.mozilla.org/releases/mozilla-beta/rev/31db0495d56a
Comment 24 :Gavin Sharp [email: gavin@gavinsharp.com] 2012-06-26 17:14:28 PDT
Comment on attachment 627634 [details] [diff] [review]
fix v.1

(this was already on aurora)
Comment 25 :Gavin Sharp [email: gavin@gavinsharp.com] 2012-06-26 17:24:24 PDT
Comment on attachment 627634 [details] [diff] [review]
fix v.1

[Approval Request Comment]
see comment 21, same applies to ESR (tested patch+test locally)
Comment 26 Lukas Blakk [:lsblakk] use ?needinfo 2012-06-28 16:15:40 PDT
Comment on attachment 627634 [details] [diff] [review]
fix v.1

low risk, please go ahead and land to ESR branch
Comment 27 :Gavin Sharp [email: gavin@gavinsharp.com] 2012-06-29 09:33:43 PDT
https://hg.mozilla.org/releases/mozilla-esr10/rev/a612c98a839e
Comment 28 Anthony Hughes (:ashughes) [GFX][QA][Mentor] 2012-08-14 15:04:47 PDT
Can this testcase be put in-testsuite? Does it need a manual test?
Comment 29 Phil Ringnalda (:philor, back in August) 2012-08-14 15:19:15 PDT
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.
Comment 30 Anthony Hughes (:ashughes) [GFX][QA][Mentor] 2012-08-14 15:31:37 PDT
Thanks Phil. Marking qa- since this is in-testsuite. If you think this needs a manual test please nominate for in-moztrap.

Note You need to log in before you can comment on or make changes to this bug.