758990 - (CVE-2012-1965) Don't allow feed: URLs with an innerURI that inherits the page's security context

Status: RESOLVED FIXED

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

Description

[Jesse Ruderman]

Attachment: 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 2

[Giorgio Maone [:ma1]]

Why can't I see bug 758907?

Comment 3

[Phil Ringnalda (:philor)]

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)]

Attachment: 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

[[e-mail]]

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: [e-mail]]]

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

[[e-mail]]

(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)]

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

[[e-mail]]

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)]

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)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/a8e98b1326b9

Comment 12

[Mario Gomes]

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 13

[(no longer active)]

https://hg.mozilla.org/mozilla-central/rev/a8e98b1326b9

Comment 14

[Mario Gomes]

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]

@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]]

Reconsidering that ESR wontfix -- this looks low-risk enough to take and may endanger some sites.

Comment 18

[Soroush Dalili]

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]

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: [e-mail]]]

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]

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: [e-mail]]]

https://hg.mozilla.org/releases/mozilla-beta/rev/31db0495d56a

Comment 24

[:Gavin Sharp [email: [e-mail]]]

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

(this was already on aurora)

Comment 25

[:Gavin Sharp [email: [e-mail]]]

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]

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: [e-mail]]]

https://hg.mozilla.org/releases/mozilla-esr10/rev/a612c98a839e

Comment 28

[u279076]

Can this testcase be put in-testsuite? Does it need a manual test?

Comment 29

[Phil Ringnalda (:philor)]

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

[u279076]

Thanks Phil. Marking qa- since this is in-testsuite. If you think this needs a manual test please nominate for in-moztrap.