430658 - Remaining attack vectors in FeedWriter.js

Status: RESOLVED FIXED

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

Description

[moz_bug_r_a4]

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]

Attachment: testcase 1 - document.title

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

Comment 2

[moz_bug_r_a4]

Attachment: testcase 2 - style.marginRight

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

Comment 3

[moz_bug_r_a4]

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

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

Comment 5

[Daniel Veditz [:dveditz]]

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

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

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

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

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

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

Marking this blocking1.8.1.15+ again.

Comment 12

[Daniel Veditz [:dveditz]]

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

Comment 13

[Asaf Romano (gone)]

Attachment: patch

Comment 14

[Mike Connor [:mconnor]]

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

[Asaf Romano (gone)]

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

Comment 16

[Samuel Sidler (old account; do not CC)]

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

Comment 17

[Al Billings [:abillings - ex-MoCo]]

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

(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

[Al Billings [:abillings - ex-MoCo]]

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

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]

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]

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

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

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

[Al Billings [:abillings - ex-MoCo]]

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.