Last Comment Bug 360529 - Arbitrary code execution using XSS hole and feed preview page
: Arbitrary code execution using XSS hole and feed preview page
Status: RESOLVED FIXED
[sg:critical]
: verified1.8.1.17
Product: Firefox
Classification: Client Software
Component: RSS Discovery and Preview (show other bugs)
: unspecified
: x86 Windows XP
: P2 normal (vote)
: Firefox 3
Assigned To: Mano (::mano, needinfo? for any questions; not reading general bugmail)
:
Mentors:
Depends on: 393762 425010 452689 CVE-2008-5504
Blocks: 430658
  Show dependency treegraph
 
Reported: 2006-11-12 22:23 PST by moz_bug_r_a4
Modified: 2008-11-16 20:45 PST (History)
22 users (show)
mbeltzner: blocking‑firefox3+
samuel.sidler+old: blocking1.8.1.13-
samuel.sidler+old: blocking1.8.1.15-
samuel.sidler+old: blocking1.8.1.17+
dveditz: wanted1.8.1.x+
asac: blocking1.8.0.next-
asac: wanted1.8.0.x-
sdwilsh: in‑testsuite?
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
patch (8.13 KB, patch)
2008-03-01 12:41 PST, Mano (::mano, needinfo? for any questions; not reading general bugmail)
sayrer: review+
jst: superreview+
mbeltzner: approval1.9b4+
Details | Diff | Splinter Review
untested branch patch (5.54 KB, patch)
2008-03-03 07:42 PST, Mano (::mano, needinfo? for any questions; not reading general bugmail)
no flags Details | Diff | Splinter Review
wip (21.88 KB, patch)
2008-03-21 12:20 PDT, Mano (::mano, needinfo? for any questions; not reading general bugmail)
no flags Details | Diff | Splinter Review
patch (31.43 KB, patch)
2008-03-22 12:03 PDT, Mano (::mano, needinfo? for any questions; not reading general bugmail)
mconnor: review+
mconnor: approval1.9b5+
Details | Diff | Splinter Review
for checkin (31.43 KB, patch)
2008-03-24 05:42 PDT, Mano (::mano, needinfo? for any questions; not reading general bugmail)
no flags Details | Diff | Splinter Review
combined branch patch (29.81 KB, patch)
2008-08-20 14:56 PDT, Mano (::mano, needinfo? for any questions; not reading general bugmail)
mconnor: review+
jst: review+
samuel.sidler+old: approval1.8.1.17+
Details | Diff | Splinter Review

Description moz_bug_r_a4 2006-11-12 22:23:16 PST
Please see bug 353266.

Array.prototype methods trick (bug 344495) and document.(open|write) trick (bug
346659) and XSS holes (bug 351370, bug 359137) are available on fx2.0. (bug
359137 was already fixed.)

In a feed preview page, an event handler that was registered by content script
can be called by chrome script via elem.doCommand() or elem.dispatchEvent(). 
In this case, the event handler's scripted caller is chrome script.  Thus, an
attacker can run arbitrary code with chrome privileges by using Array.prototype
methods trick or document.write trick.

--
  document.getElementById("handlersMenuList")
   .addEventListener("command", x, true);

  SubscribeHandler._feedWriter.write(window);
   or
  SubscribeHandler._feedWriter
   .QueryInterface(Components.interfaces.nsIObserver)
   .observe({}, "nsPref:changed", "browser.feeds.handler.default");

x() (if x is a function) or x.handleEvent() is called in
FeedWriter._setSelectedHandler() via handlers[0].doCommand(),
selectedAppMenuItem.doCommand() or liveBookmarksMenuItem.doCommand().

  document.getElementById("alwaysUse")
   .addEventListener("CheckboxStateChange", y, true);

  SubscribeHandler._feedWriter.write(window);
   or
  SubscribeHandler._feedWriter
   .QueryInterface(Components.interfaces.nsIObserver)
   .observe({}, "nsPref:changed", "browser.feeds.handler");

y() or y.handleEvent() is called in FeedWriter._setCheckboxCheckedState() via
aCheckbox.dispatchEvent(event).
--

* Fx1.5.0.x is not affected since it does not have feed preview feature.
* The trunk should be exploitable if an XSS hole existed.
Comment 3 moz_bug_r_a4 2007-01-12 21:03:44 PST
The old testcases no longer work, since those try to use the XSS hole (bug
351370) that has been already fixed.  I'm attaching new testcases that use
another XSS hole (Bug 363597).
Comment 6 moz_bug_r_a4 2007-01-12 21:11:39 PST
Is there any chance this can be fixed without relying on fixing bug 344495?
Comment 7 Martijn Wargers [:mwargers] (not working for Mozilla) 2008-01-21 02:08:54 PST
Is this still a problem in current trunk/branch build?
Comment 8 moz_bug_r_a4 2008-01-21 22:09:30 PST
Yes, at least on 1.8 branch.  It's still possible to use the feed preview page
to turn an XSS bug into an arbitrary code execution bug.
Comment 11 Olli Pettay [:smaug] (vacation Aug 25-28) 2008-01-22 14:45:26 PST
Repeating the question:
(In reply to comment #7)
> Is this still a problem in current trunk/branch build?

If this is branch only, could the bug be marked as gecko-1.8branch/ff-2.0branch?

Comment 12 Daniel Veditz [:dveditz] 2008-02-04 18:04:28 PST
Whether or not it's a trunk problem depends on whether there are any known universal-xss vulnerabilities on the trunk, or will be any found (or made) in the future. We've gone to a lot of trouble to make sure content can't open any chrome-privileged URIs precisely because we didn't trust that there were no such holes, but being able to open frames on arbitrary feeds means that web content _can_ still open chrome-privileged frames.

The only true solution is to make the Feed preview non-privileged. The privileged part could be a separate chrome-privileged sheet-like thing much like an oversized infobar.
Comment 13 Mike Beltzner [:beltzner, not reading bugmail] 2008-02-08 10:45:59 PST
Not gonna block Firefox 3 on this for now, but if you find that potential XSS vuln or figure out a fix for branch, please renom.
Comment 14 Samuel Sidler (old account; do not CC) 2008-02-12 18:37:13 PST
sayrer, is this something you can look at?
Comment 15 Robert Sayre 2008-02-12 18:48:27 PST
(In reply to comment #14)
> sayrer, is this something you can look at?
> 

Mano dealt with this stuff last time, and came up with the Fx2 solution. I am really busy with other stuff--a summary of the attributes and elements used for the attack would be much appreciated.
Comment 16 Samuel Sidler (old account; do not CC) 2008-02-28 17:15:40 PST
Mano, do you have time to look at this in the next week? We're about a week away from our branch freeze.
Comment 17 Daniel Veditz [:dveditz] 2008-02-29 16:27:27 PST
(In reply to comment #13)
> Not gonna block Firefox 3 on this for now, but if you find that potential XSS
> vuln or figure out a fix for branch, please renom.

We find XSS fairly regularly, it needs to get fixed.

Comment 18 Mano (::mano, needinfo? for any questions; not reading general bugmail) 2008-03-01 12:41:03 PST
Created attachment 306777 [details] [diff] [review]
patch
Comment 19 Mike Beltzner [:beltzner, not reading bugmail] 2008-03-02 22:09:35 PST
Comment on attachment 306777 [details] [diff] [review]
patch

a1.9b4=beltzner
Comment 20 Mano (::mano, needinfo? for any questions; not reading general bugmail) 2008-03-03 00:16:03 PST
mozilla/browser/components/feeds/src/FeedWriter.js 1.53
Comment 21 Samuel Sidler (old account; do not CC) 2008-03-03 07:32:40 PST
Mano, does this same patch apply to the branch? If so, can you request approval? If not, a branch-specific one would be great.
Comment 22 Mano (::mano, needinfo? for any questions; not reading general bugmail) 2008-03-03 07:42:25 PST
Created attachment 307041 [details] [diff] [review]
untested branch patch

I don't have a tree to this one on.
Comment 23 moz_bug_r_a4 2008-03-03 16:41:57 PST
It seems that appendChild(), removeAttribute(), etc. are also exploitable,
since those methods fire mutation events.
Comment 25 Brendan Eich [:brendan] 2008-03-03 17:07:35 PST
Followup bug would be better unless the landed patch is backed out.

/be
Comment 26 Mano (::mano, needinfo? for any questions; not reading general bugmail) 2008-03-03 23:01:57 PST
Let's keep using this one.
Comment 27 Samuel Sidler (old account; do not CC) 2008-03-07 11:52:55 PST
This needs a complete trunk patch before we can take it on the branch. Punting to the next release.
Comment 28 Mano (::mano, needinfo? for any questions; not reading general bugmail) 2008-03-19 19:12:45 PDT
Mike and I agreed to push this to early-post-beta given the scale of my upcoming rewrite.
Comment 29 Mano (::mano, needinfo? for any questions; not reading general bugmail) 2008-03-21 09:42:07 PDT
So, after thinking about this a little more, I'm going to just rely on the fact that DOMAttrModified and other mutation events are not being dispatched for nodes which are not yet inserted into the document (and call appendChild within the sandbox).
Comment 30 Mano (::mano, needinfo? for any questions; not reading general bugmail) 2008-03-21 09:47:18 PDT
In the js console:
var a=function() { alert('foo'); }; document.addEventListener("DOMAttrModified", a, true); var b = document.createElement("b"); b.setAttribute("f", "b"); document.documentElement.appendChild(b); b.setAttribute("m", "f");

alerts only for the second setAttribute call. Boris, Jonas,  Johnny: Can the feedwriter code rely on this? W3C's documentation is pretty vague about mutation events behavior for nodes which are not yet inserted into the document.
Comment 31 Olli Pettay [:smaug] (vacation Aug 25-28) 2008-03-21 09:49:40 PDT
Mutation events do get dispatched, but in your testcase the event can't 
propagate to document, because element isn't in document.
But if you add mutation event listener to the element, the listener will be
called.
Comment 32 Mano (::mano, needinfo? for any questions; not reading general bugmail) 2008-03-21 09:54:56 PDT
I'm fine with that, we create the elements and set the attributes on the same time. Would it be enough than to call appendChild and all  _post_ node-insertion, attribute-modifications methods  in the sandbox?
Comment 33 Mano (::mano, needinfo? for any questions; not reading general bugmail) 2008-03-21 12:20:53 PDT
Created attachment 311020 [details] [diff] [review]
wip
Comment 34 Mano (::mano, needinfo? for any questions; not reading general bugmail) 2008-03-22 12:03:51 PDT
Created attachment 311175 [details] [diff] [review]
patch
Comment 35 Mike Connor [:mconnor] 2008-03-23 17:15:37 PDT
Comment on attachment 311175 [details] [diff] [review]
patch

>-      feedTitleLink.setAttribute("title", titleText);
>+      this._contentSandbox.feedTitleLink = feedTitleLink;
>+      this._contentSandbox.titleText = titleText;
>+      var codeStr = "feedTitleLink.setAttribute('title', titleText);";
>+      Cu.evalInSandbox(codeStr, this._contentSandbox);
>       this._safeSetURIAttribute(feedTitleLink, "href", 
>                                 parts.getPropertyAsAString("link"));

can null these after to clean up

>   _setSubscribeUsingLabel: function FW__setSubscribeUsingLabel() {
>-    var stringLabel = null;
>-
>-    switch (this._getFeedType()) {
>+    var stringLabel;
>+    var feedType = this._getFeedType();
>+    switch (feedType) {

you don't use feedType elsewhere, don't understand the change here.

>       case Ci.nsIFeed.TYPE_VIDEO:
>         stringLabel = "subscribeVideoPodcastUsing";
>         break;
> 
>       case Ci.nsIFeed.TYPE_AUDIO:
>         stringLabel = "subscribeAudioPodcastUsing";
>         break;
> 
>       default:
>         stringLabel = "subscribeFeedUsing";
>     }

you could just init stringLabel to this and it'd be a little less code overall, but that's being nitpicky.

r=me, this should be pretty safe, just needs to get tested thoroughly by QA tomorrow...
Comment 36 Mike Connor [:mconnor] 2008-03-23 18:10:33 PDT
Comment on attachment 311175 [details] [diff] [review]
patch

a1.9b5 on this patch, as its fixing an sg:critical bug and has potential regression impact in the wild.

Please make sure that the existing litmus tests cover the functionality we're touching here, and send mail to qa@mozilla.org asking that they be run ASAP on all platforms.  If there are missing tests, please tell QA what the tests should cover.
Comment 37 Shawn Wilsher :sdwilsh 2008-03-23 18:36:58 PDT
It seems to me that this wouldn't be too hard to automate testing on? (not that I'm volunteering - my plate is plenty full)

The testcases seem to basically be already written (some modification so they don't use an alert box maybe, but that shouldn't be too hard either...)
Comment 38 Mano (::mano, needinfo? for any questions; not reading general bugmail) 2008-03-23 18:45:42 PDT
No, you cannot test this, we generally fix XSS holes :p
Comment 39 Shawn Wilsher :sdwilsh 2008-03-23 18:51:14 PDT
right but, if we can exploit said vulnerability in a test, the test should fail, right?
Comment 40 Mano (::mano, needinfo? for any questions; not reading general bugmail) 2008-03-23 18:56:31 PDT
But we cannot. As far as i can tell, none of the tests attached here can be used to test this bug on current trunk.
Comment 41 Boris Zbarsky [:bz] 2008-03-23 20:00:30 PDT
XSS-like conditions can be created by code with UniversalXPConnect privileges, no?
Comment 42 Mano (::mano, needinfo? for any questions; not reading general bugmail) 2008-03-24 05:31:21 PDT
I couldn't find a way (even with privileges enabled) to call about:feed's SubscribeHandler.init on trunk without using the console / urlbar as described in testcase 6. 
Comment 43 Mano (::mano, needinfo? for any questions; not reading general bugmail) 2008-03-24 05:42:53 PDT
Created attachment 311366 [details] [diff] [review]
for checkin

I'll write some tests later as we figure out how to do so...
Comment 44 Mano (::mano, needinfo? for any questions; not reading general bugmail) 2008-03-24 05:44:25 PDT
mozilla/browser/components/feeds/src/FeedWriter.js 1.54

Mail sent to qa@m.c.
Comment 45 Boris Zbarsky [:bz] 2008-03-24 12:18:01 PDT
> without using the console / urlbar as described in testcase 6. 

So if you inject code into the Firefox window that calls eval(content.code), it doesn't work?
Comment 46 Mano (::mano, needinfo? for any questions; not reading general bugmail) 2008-03-24 12:23:53 PDT
Firefox window as in browser.xul? I guess that might work, though it requires the chrome-testing framework....
Comment 47 Boris Zbarsky [:bz] 2008-03-24 12:50:27 PDT
Why?  You can do that from inside a mochitest if you really want to...

Doing it from chrome mochitests might work as long as you can get the feed preview page loaded first.
Comment 48 Samuel Sidler (old account; do not CC) 2008-06-04 16:11:10 PDT
Is there any work being done here for an updated branch patch, presumably one that also includes a fix for bug 430658?
Comment 49 Samuel Sidler (old account; do not CC) 2008-06-05 15:12:45 PDT
Still don't have a branch patch and given the size of the trunk patch we don't want to take it at the last minute. Moving to 1.8.1.16, but we need to get a patch soon so we can land it early in the cycle.
Comment 52 Samuel Sidler (old account; do not CC) 2008-08-11 20:05:56 PDT
Mano, we could really use a branch patch here...
Comment 53 Mano (::mano, needinfo? for any questions; not reading general bugmail) 2008-08-20 14:56:42 PDT
Created attachment 334770 [details] [diff] [review]
combined branch patch
Comment 54 Mike Connor [:mconnor] 2008-08-20 16:41:34 PDT
Comment on attachment 334770 [details] [diff] [review]
combined branch patch

This feels like pain.
Comment 55 Daniel Veditz [:dveditz] 2008-08-22 11:16:15 PDT
qanote: testcase 7 can't be used to verify because the stepping-stone bug was fixed in FF2.0.0.15. Either we need a 2.0.0.16-working testcase that uses another yet-unfixed XSS bug, or this patch needs to be verified by applying the patch to a 2.0.0.14 source tree.
Comment 56 Daniel Veditz [:dveditz] 2008-08-22 11:21:27 PDT
Comment on attachment 334770 [details] [diff] [review]
combined branch patch

It'd be good to get a second review given the size of the patch and the limited testing time we've got left for this release. Johnny: you reviewed an earlier iteration, you up for it?
Comment 58 Samuel Sidler (old account; do not CC) 2008-08-25 23:11:02 PDT
Comment on attachment 334770 [details] [diff] [review]
combined branch patch

Approved for 1.8.1.17. Please land ASAP on the branch. a=ss
Comment 59 Mano (::mano, needinfo? for any questions; not reading general bugmail) 2008-08-26 00:45:08 PDT
Checking in src/FeedWriter.js;
/cvsroot/mozilla/browser/components/feeds/src/FeedWriter.js,v  <--  FeedWriter.js
new revision: 1.2.2.33; previous revision: 1.2.2.32
done
Comment 60 moz_bug_r_a4 2008-08-27 04:12:56 PDT
The fx2 patch caused a syntax error on Windows.

Here is an extra "}".
http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/browser/components/feeds/src/FeedWriter.js&rev=1.2.2.33&mark=788#782


Error: missing catch or finally after try
Source File: file:///C:/test/fx-2.0.0.17pre-2008-08-26-03/firefox/components/FeedWriter.js
Line: 736, Column: 6
Source Code:
      else {

Error: BrowserFeedWriter is not defined
Source File: chrome://browser/content/feeds/subscribe.js
Line: 45
Comment 61 Mike Shaver (:shaver -- probably not reading bugmail closely) 2008-08-27 07:59:07 PDT
Preeeetty sure we need the syntax error fixed before we can ship 2.0.0.17, so unmarking it as branch-fixed until that's addressed.

How did this code ever work in testing the #ifdef XP_WIN block?  We really, *really* can't be checking untested code into the stable branches, so I'm very much hoping that something else happened. :(
Comment 62 Mano (::mano, needinfo? for any questions; not reading general bugmail) 2008-08-27 10:47:27 PDT
Bad merge :-/
Comment 63 :Gavin Sharp [email: gavin@gavinsharp.com] 2008-08-27 10:47:47 PDT
With that fixed I get:
Error: uncaught exception: [Exception... "'[JavaScript Error: "result has no properties" {file: "file:///I:/mozb/mozilla/ff-opt/dist/bin/components/FeedWriter.js" line: 381}]' when calling method: [nsIFeedWriter::write]"  nsresult: "0x80570021 (NS_ERROR_XPC_JAVASCRIPT_ERROR_WITH_DETAILS)"  location: "JS frame :: chrome://browser/content/feeds/subscribe.js :: SH_init :: line 46"  data: yes]

with all of the testcases, along with:
Security Error: Content at about:feeds may not load or link to chrome://global/content/mozilla.xhtml.

I assume this is the expected failure (I get the same on trunk).
Comment 64 :Gavin Sharp [email: gavin@gavinsharp.com] 2008-08-27 10:52:22 PDT
mozilla/browser/components/feeds/src/FeedWriter.js 	1.2.2.34 
Comment 65 juan becerra [:juanb] 2008-08-29 18:33:46 PDT
Verified on Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8.1.17) Gecko/2008082909 Firefox/2.0.0.17 using test cases here. This is the output in the error console while running test case 8, in comment #57:

Error: result has no properties
Source File: file:///C:/Program%20Files/Mozilla%20Firefox/components/FeedWriter.js
Line: 354

Error: uncaught exception: [Exception... "'[JavaScript Error: "result has no properties" {file: "file:///C:/Program%20Files/Mozilla%20Firefox/components/FeedWriter.js" line: 354}]' when calling method: [nsIFeedWriter::write]"  nsresult: "0x80570021 (NS_ERROR_XPC_JAVASCRIPT_ERROR_WITH_DETAILS)"  location: "JS frame :: chrome://browser/content/feeds/subscribe.js :: SH_init :: line 46"  data: yes]

Error: [Exception... "Component returned failure code: 0x80004005 (NS_ERROR_FAILURE) [nsIDOMLocation.href]"  nsresult: "0x80004005 (NS_ERROR_FAILURE)"  location: "native frame :: <unknown filename> :: <TOP_LEVEL> :: line 0"  data: no]
Comment 66 Alexander Sack 2008-08-31 16:46:47 PDT
doesnt apply on 1.8.0 from what i can see. If i am mistaking flip wanted1.8.0.x or blocking1.8.0.15 to ? again. Thanks!
Comment 67 juan becerra [:juanb] 2008-09-02 17:28:39 PDT
In 20015 test case #8 opens up an alert window, and in 20017 it doesn't.
Comment 70 :Gavin Sharp [email: gavin@gavinsharp.com] 2008-09-03 11:48:37 PDT
moz_bug, could you file a new bug on the omissions? Tracking multiple patches in one bug just leads to confusion.
Comment 71 Martijn Wargers [:mwargers] (not working for Mozilla) 2008-09-03 12:28:38 PDT
And it seems to me that should block the fx2.0.0.17 release, right?
Comment 72 Mike Beltzner [:beltzner, not reading bugmail] 2008-09-03 14:09:02 PDT
(In reply to comment #70)
> moz_bug, could you file a new bug on the omissions? Tracking multiple patches
> in one bug just leads to confusion.

I have been convinced that's the right thing to do, instead of continually expanding the scope of this single bug. We'll leave this part of the issue as fixed & verified, and stamp out the others in future releases.

(In reply to comment #71)
> And it seems to me that should block the fx2.0.0.17 release, right?

No, we've waited before, and can continue to do so again.
Comment 75 Mano (::mano, needinfo? for any questions; not reading general bugmail) 2008-09-04 05:29:13 PDT
CC me on it please if you wanst me to fix it.

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