moz_bug_r_a4 Reporter
	Description • 16 years ago

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

	moz_bug_r_a4 Reporter
	Comment 1 • 16 years ago

This uses bug 428672's XSS trick.  This works on trunk.

	moz_bug_r_a4 Reporter
	Comment 2 • 16 years ago

This uses bug 428672's XSS trick.  This works on trunk.

	moz_bug_r_a4 Reporter
	Comment 3 • 16 years ago

This uses bug 428672's XSS trick.  This works on trunk and fx2.0.0.x.

	Mike Beltzner [:beltzner, not reading bugmail]
	Comment 4 • 16 years ago

Dan, sayrer, should we block on this, or can we get it on a dot-dot release?

	Daniel Veditz [:dveditz]
	Comment 5 • 16 years ago

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.

	Mike Beltzner [:beltzner, not reading bugmail]
	Comment 6 • 16 years ago

(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.

	Daniel Veditz [:dveditz]
	Comment 7 • 16 years ago

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.

	Mike Beltzner [:beltzner, not reading bugmail]
	Comment 8 • 16 years ago

(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.

	:Gavin Sharp [email: gavin@gavinsharp.com]
	Comment 9 • 16 years ago

(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.

	Mike Beltzner [:beltzner, not reading bugmail]
	Comment 10 • 16 years ago

Fine, so we'll block.

ETA and route to fix? Who can help us out here; Mano's overloaded elsewhere?

	Samuel Sidler (old account; do not CC)
	Comment 11 • 16 years ago

Marking this blocking1.8.1.15+ again.

	Daniel Veditz [:dveditz]
	Comment 12 • 16 years ago

:jst, mrbkap: is there any fundamental mis-wrappering we're doing in FeedWriter.js at the heart of these bugs?

	Mike Connor [:mconnor]
	Comment 14 • 16 years ago

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.

	Asaf Romano (gone) Assignee
	Comment 15 • 16 years ago

mozilla/browser/components/feeds/src/FeedWriter.js 1.56

	Samuel Sidler (old account; do not CC)
	Comment 16 • 16 years ago

Moving to 1.8.1.16 for the same reasons in bug 360529 comment 49.

	Al Billings [:abillings - ex-MoCo]
	Comment 17 • 16 years ago

(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.

	:Gavin Sharp [email: gavin@gavinsharp.com]
	Comment 18 • 16 years ago

(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.

	Al Billings [:abillings - ex-MoCo]
	Comment 19 • 16 years ago

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.

	juan becerra [:juanb]
	Comment 20 • 16 years ago

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?

	moz_bug_r_a4 Reporter
	Comment 21 • 16 years ago

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.

	moz_bug_r_a4 Reporter
	Comment 22 • 16 years ago

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

This uses bug 451680's XSS trick.

	moz_bug_r_a4 Reporter
	Comment 23 • 16 years ago

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

This uses bug 451680's XSS trick.

	moz_bug_r_a4 Reporter
	Comment 24 • 16 years ago

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

This uses bug 451680's XSS trick.

	Al Billings [:abillings - ex-MoCo]
	Comment 25 • 16 years ago

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.