Last Comment Bug 430658 - Remaining attack vectors in FeedWriter.js
: Remaining attack vectors in FeedWriter.js
Status: RESOLVED FIXED
[sg:critical][fixed in 360529]
: testcase, verified1.8.1.17
Product: Firefox
Classification: Client Software
Component: RSS Discovery and Preview (show other bugs)
: unspecified
: x86 Windows XP
: -- normal (vote)
: Firefox 3
Assigned To: Mano (::mano, needinfo? for any questions; not reading general bugmail)
:
Mentors:
Depends on: 360529
Blocks:
  Show dependency treegraph
 
Reported: 2008-04-24 06:39 PDT by moz_bug_r_a4
Modified: 2009-01-05 12:11 PST (History)
13 users (show)
mbeltzner: blocking‑firefox3+
mbeltzner: wanted‑firefox3+
samuel.sidler+old: blocking1.8.1.15-
samuel.sidler+old: blocking1.8.1.17+
mbeltzner: wanted1.8.1.x+
asac: blocking1.8.0.next-
asac: wanted1.8.0.x-
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
patch (3.52 KB, patch)
2008-05-01 14:04 PDT, Mano (::mano, needinfo? for any questions; not reading general bugmail)
mconnor: review+
mconnor: approval1.9+
Details | Diff | Review

Description moz_bug_r_a4 2008-04-24 06:39:49 PDT
This is a follow up to bug 360529.


395   _setTitleText: function FW__setTitleText(container) {
396     if (container.title) {
397       this._setContentText(TITLE_ID, container.title.plainText());
398       this._document.title = container.title.plainText();
399     }

Line 398 is exploitable since it fires DOMTitleChanged event.


411   _setTitleImage: function FW__setTitleImage(container) {
...
437       var feedTitleText = this._document.getElementById("feedTitleText");
438       var titleImageWidth = parseInt(parts.getPropertyAsAString("width")) + 15;
439       feedTitleText.style.marginRight = titleImageWidth + "px";

Line 439 is exploitable since it sets style attribute and thus fires mutation
events.


940   _initSubscriptionUI: function FW__initSubscriptionUI() {
...
960       default:
961         codeStr = "header.className = 'feedBackground'; ";
962         header.className = "feedBackground";

Line 962 is unnecessary (and exploitable).
Comment 1 moz_bug_r_a4 2008-04-24 06:41:18 PDT
Created attachment 317532 [details]
testcase 1 - document.title

This uses bug 428672's XSS trick.  This works on trunk.
Comment 2 moz_bug_r_a4 2008-04-24 06:42:08 PDT
Created attachment 317533 [details]
testcase 2 - style.marginRight

This uses bug 428672's XSS trick.  This works on trunk.
Comment 3 moz_bug_r_a4 2008-04-24 06:42:54 PDT
Created attachment 317534 [details]
testcase 3 - header.className

This uses bug 428672's XSS trick.  This works on trunk and fx2.0.0.x.
Comment 4 Mike Beltzner [:beltzner, not reading bugmail] 2008-04-25 02:20:16 PDT
Dan, sayrer, should we block on this, or can we get it on a dot-dot release?
Comment 5 Daniel Veditz [:dveditz] 2008-04-25 11:50:43 PDT
We should block on this. Now if we fix the specific vector used here (and we already have a patch in bug 428672) that buys us some time, but then we're just crossing our fingers that there aren't any more such bugs.

We need to fix the underlying "content can open a chrome-privileged frame" issue. Despite this bug's summary ("remaining attack vectors") I have absolutely no confidence that there aren't others.
Comment 6 Mike Beltzner [:beltzner, not reading bugmail] 2008-04-28 10:34:53 PDT
(In reply to comment #5)
> We should block on this. Now if we fix the specific vector used here (and we
> already have a patch in bug 428672) that buys us some time, but then we're just
> crossing our fingers that there aren't any more such bugs.

I'm confused - are you saying we should block on fixing this outright, or taking the fix that's in bug 428672?

Assuming the latter.
Comment 7 Daniel Veditz [:dveditz] 2008-04-29 16:01:24 PDT
We should most definitely take bug 428672, that is an independent problem.

That won't fix the underlying issue though -- that just about any same-origin violation can be elevated from an XSS bug to an arbitrary code execution bug because of the feed writer. Don't be fooled by the 430xxx bug number here, this is still basically bug 360529 using a different stepping-stone bug.

Is hiding the skeleton in the closet again by fixing bug 428672 good enough? I'm not willing to say we're not going to find more same-origin violations, and fixing the feedwriter might involve the kind of re-writing you'd want to get out before the final release rather than risk it in a point release. On that ground I think it should block.
Comment 8 Mike Beltzner [:beltzner, not reading bugmail] 2008-04-30 09:07:02 PDT
(In reply to comment #7)
> Is hiding the skeleton in the closet again by fixing bug 428672 good enough?

This close to ship of Firefox 3, without a published exploit, and without any indication that someone's close it those on the Firefox 3 call felt like the answer was "yes".

> I'm not willing to say we're not going to find more same-origin violations, and
> fixing the feedwriter might involve the kind of re-writing you'd want to get
> out before the final release rather than risk it in a point release. On that
> ground I think it should block.

I'm inclined to disagree, but would like to hear from someone what we think the "right fix" involves in terms of time to code up, and what we think the chances of regression are (I'd expect pretty small, since it's just our feed parsing stuff, but maybe add-ons?)

As we've asked before: there are other sg:critical bugs that we're not blocking on, so why do we think this is worse than those? Feed previewing isn't really even a common operation amongst the general net population.
Comment 9 :Gavin Sharp [email: gavin@gavinsharp.com] 2008-04-30 10:32:53 PDT
(In reply to comment #8)
> As we've asked before: there are other sg:critical bugs that we're not blocking
> on, so why do we think this is worse than those? Feed previewing isn't really
> even a common operation amongst the general net population.

Most other sg:critical bugs are marked "sg:critical" because of the potential for remote code execution - this one has a working testcase. I think it's one of the scariest security bugs we have open.

An attacker can force a feed preview page to appear without the user knowing, so whether or not feed previewing is common among the general net population isn't relevant.
Comment 10 Mike Beltzner [:beltzner, not reading bugmail] 2008-04-30 10:36:29 PDT
Fine, so we'll block.

ETA and route to fix? Who can help us out here; Mano's overloaded elsewhere?
Comment 11 Samuel Sidler (old account; do not CC) 2008-04-30 11:07:14 PDT
Marking this blocking1.8.1.15+ again.
Comment 12 Daniel Veditz [:dveditz] 2008-04-30 16:52:49 PDT
:jst, mrbkap: is there any fundamental mis-wrappering we're doing in FeedWriter.js at the heart of these bugs?
Comment 13 Mano (::mano, needinfo? for any questions; not reading general bugmail) 2008-05-01 14:04:52 PDT
Created attachment 318889 [details] [diff] [review]
patch
Comment 14 Mike Connor [:mconnor] 2008-05-01 14:14:47 PDT
Comment on attachment 318889 [details] [diff] [review]
patch

this._contentSandbox.titleImageWidth should get nulled as well, unless there's a reason not to...

oy.  we should rearch this next cycle, this whack a mole has its limits.
Comment 15 Mano (::mano, needinfo? for any questions; not reading general bugmail) 2008-05-01 14:38:58 PDT
mozilla/browser/components/feeds/src/FeedWriter.js 1.56
Comment 16 Samuel Sidler (old account; do not CC) 2008-06-05 15:13:15 PDT
Moving to 1.8.1.16 for the same reasons in bug 360529 comment 49.
Comment 17 [On PTO until 6/29] 2008-09-02 16:07:53 PDT
(In reply to comment #2)
> Created an attachment (id=317533) [details]
> testcase 2 - style.marginRight
> 
> This uses bug 428672's XSS trick.  This works on trunk.

I just examined this on Windows XP using Firefox 2.0.0.16 and the 2.0.0.17 candidate build. This testcase behaves exactly the same in both, which is to show a feed preview inside the iframe. So either the case no longer works with 2.0.0.16, 2.0.0.17 isn't fixed, or I'm completely stupid.
Comment 18 :Gavin Sharp [email: gavin@gavinsharp.com] 2008-09-02 17:36:34 PDT
(In reply to comment #17)
> (In reply to comment #2)
> > Created an attachment (id=317533) [details] [details]
> > testcase 2 - style.marginRight
> > 
> > This uses bug 428672's XSS trick.  This works on trunk.
> 
> I just examined this on Windows XP using Firefox 2.0.0.16 and the 2.0.0.17
> candidate build.

testcases 1 and 2 only ever worked on trunk. You should be able to use testcase 3 to verify on the branch.
Comment 19 [On PTO until 6/29] 2008-09-02 17:38:45 PDT
I did but replied to the wrong comment. :-) The commentary from me is about testcase #3. I'm not seeing a difference in behavior between 2.0.0.16 and 2.0.0.17 on XP.
Comment 20 juan becerra [:juanb] 2008-09-02 17:40:55 PDT
I also didn't see any difference in behavior between 20015, 20016 and 20017
using the three test cases listed here, even test case 3.

Comment #7 mentions that this bug is similar to bug 360529, and test case #8 in that bug can be used to see the difference in behavior between 20015 and 20017 (for that bug).

Asaf, could we apply test case #8 in bug 360529 to verify this bug as well?
Comment 21 moz_bug_r_a4 2008-09-03 02:50:18 PDT
The reason the testcases do not work on fx2.0.0.16 (and fx2.0.0.17) is that the
testcases rely on the XSS hole that was fixed on fx2.0.0.15.  I'll attach
modified testcases for verification.
Comment 22 moz_bug_r_a4 2008-09-03 02:52:37 PDT
Created attachment 336633 [details]
testcase 4 (testcase 1)

This works on fx2.0.0.16, but, does not work on fx2.0.0.17.

This uses bug 451680's XSS trick.
Comment 23 moz_bug_r_a4 2008-09-03 02:54:00 PDT
Created attachment 336635 [details]
testcase 5 (testcase 2)

This works on fx2.0.0.16, but, does not work on fx2.0.0.17.

This uses bug 451680's XSS trick.
Comment 24 moz_bug_r_a4 2008-09-03 02:55:13 PDT
Created attachment 336636 [details]
testcase 6 (testcase 3)

This works on fx2.0.0.16, but, does not work on fx2.0.0.17.

This uses bug 451680's XSS trick.
Comment 25 [On PTO until 6/29] 2008-09-03 15:49:32 PDT
Verified using next testcases for 2.0.0.17 with Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8.1.17) Gecko/2008082909 Firefox/2.0.0.17.

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