Last Comment Bug 454363 - Arbitrary code execution using FeedWriter.onPageChanged() method
: Arbitrary code execution using FeedWriter.onPageChanged() method
Status: VERIFIED FIXED
[sg:critical][partially fixed by bug ...
: fixed1.9.0.14, testcase, verified1.9.1
Product: Firefox
Classification: Client Software
Component: RSS Discovery and Preview (show other bugs)
: Trunk
: All All
: P1 normal (vote)
: Firefox 3.6a1
Assigned To: Mano (::mano, needinfo? for any questions; not reading general bugmail)
:
Mentors:
Depends on: 477128 CVE-2009-1841
Blocks:
  Show dependency treegraph
 
Reported: 2008-09-08 23:50 PDT by moz_bug_r_a4
Modified: 2009-10-28 12:44 PDT (History)
16 users (show)
mbeltzner: blocking‑firefox3.5+
dveditz: blocking1.9.0.4-
dveditz: blocking1.9.0.5-
dveditz: blocking1.9.0.6-
samuel.sidler+old: blocking1.9.0.7-
dveditz: blocking1.9.0.12-
dveditz: blocking1.9.0.14+
dveditz: wanted1.9.0.x+
samuel.sidler+old: wanted1.8.1.x-
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
patch (2.27 KB, patch)
2008-10-14 08:29 PDT, Mano (::mano, needinfo? for any questions; not reading general bugmail)
no flags Details | Diff | Review
patch (2.17 KB, patch)
2008-10-14 08:36 PDT, Mano (::mano, needinfo? for any questions; not reading general bugmail)
gavin.sharp: review+
Details | Diff | Review
trunk+1.9.1 patch (depends on the js bug) (1.53 KB, patch)
2009-02-10 08:17 PST, Mano (::mano, needinfo? for any questions; not reading general bugmail)
gavin.sharp: review-
Details | Diff | Review
combined patch for 1.9.0 (depends on the js bug) (2.58 KB, patch)
2009-02-10 08:21 PST, Mano (::mano, needinfo? for any questions; not reading general bugmail)
gavin.sharp: review-
Details | Diff | Review
190 (1.61 KB, patch)
2009-08-11 17:15 PDT, Mano (::mano, needinfo? for any questions; not reading general bugmail)
samuel.sidler+old: approval1.9.0.14+
Details | Diff | Review

Description moz_bug_r_a4 2008-09-08 23:50:51 PDT
This is the same issue as bug 352124.

onPageChanged() method accepts an untrusted JS object as aURI, thus, it's
possible to run arbitrary code with chrome privileges in the same way as bug
352124.

fx2 is not affected, since only trunk and fx3.0.x have onPageChanged() method.
Comment 1 moz_bug_r_a4 2008-09-08 23:52:02 PDT
Created attachment 337613 [details]
testcase
Comment 3 Daniel Veditz [:dveditz] 2008-09-10 15:28:20 PDT
we need to re-write how the feed processor works instead of playing whack-a-mole.
Comment 4 Samuel Sidler (old account; do not CC) 2008-09-27 10:27:58 PDT
Who can own this bug?
Comment 5 Mano (::mano, needinfo? for any questions; not reading general bugmail) 2008-10-14 08:29:09 PDT
Created attachment 343054 [details] [diff] [review]
patch
Comment 6 Mano (::mano, needinfo? for any questions; not reading general bugmail) 2008-10-14 08:36:58 PDT
Created attachment 343055 [details] [diff] [review]
patch
Comment 7 Boris Zbarsky [:bz] 2008-10-14 09:36:33 PDT
Aren't there other issues lurking here?  For example, in observe() we have:

     if (topic == "nsPref:changed") {

Won't that call toString() on topic if topic is an object?
Comment 8 Mano (::mano, needinfo? for any questions; not reading general bugmail) 2008-10-14 09:46:15 PDT
Could you actually pass a native object that implements (explicitly or implicitly) nsIPrefBranch2 and still be an untrusted caller?
Comment 9 Boris Zbarsky [:bz] 2008-10-14 09:52:04 PDT
Why does it need to implement nsIPrefBranch2?  You're not ensuring that.  You're just ensuring it's a real XPCOM object.  I could pass a DOM node for |subject|, say.  It wouldn't have any of the prefbranch properties on it, but you don't check for those or anything.

And even if you did, if someone happens to hand a prefbranch to content code it could then pass it to you with its objects of choice for the other args.
Comment 10 Mano (::mano, needinfo? for any questions; not reading general bugmail) 2008-10-14 10:03:59 PDT
I totally forgot observe take nsISupports and not nsIPrefBrnach2, I should fix that. Thanks a ton.
Comment 11 Daniel Veditz [:dveditz] 2008-10-22 15:55:31 PDT
Mano: where are we with this blocking bug?

I can't see how a fix for this sg:critical bug can safely land in time for an Oct 24 code-freeze so we're going to have to bump this to the next release. But the next release is going to be an extremely short cycle (tree open for about a week) so we can get it out mid December so please don't put it off.
Comment 12 Daniel Veditz [:dveditz] 2008-10-29 14:48:42 PDT
Can we at least get this fix landed on the trunk so it can be tested? Or are we waiting for something else?
Comment 13 Mano (::mano, needinfo? for any questions; not reading general bugmail) 2008-11-07 09:58:16 PST
I'll take care of this today.
Comment 14 Mike Beltzner [:beltzner, not reading bugmail] 2008-11-09 19:53:40 PST
Has this landed?
Comment 15 Mike Beltzner [:beltzner, not reading bugmail] 2008-11-10 15:09:19 PST
Gavin tells me that attachment 343055 [details] [diff] [review] is a partial patch, so trunk landing won't help here. Moving this off the beta2 radar.
Comment 16 Daniel Veditz [:dveditz] 2008-11-18 13:20:30 PST
Since we can't seem to fix feedhandling bugs in a timely manner, how about a pref that let's users turn it off in order to protect themselves? That's got to be a small, easy patch, right?

Moving out, again.
Comment 17 Brandon Sterne (:bsterne) 2008-12-04 10:44:19 PST
This bug is on our "Top Security Bugs" list.  Please treat as top priority.
Comment 18 Samuel Sidler (old account; do not CC) 2009-01-05 07:47:32 PST
I'm guessing that, because this is a P1, blocking-firefox3.1+ bug that it needs to get done by 3.1 beta 3? Any update? We really would like a fix for this for 1.9.0.6 and have pushed it out quite a bit... Mano?
Comment 19 Daniel Veditz [:dveditz] 2009-01-07 15:44:35 PST
Apparently people don't give a rats ass for the Top Security Bugs list. We should remove the Feeds feature until it can get rewritten safely.
Comment 20 Mike Beltzner [:beltzner, not reading bugmail] 2009-01-21 13:45:20 PST
Mano, any ETA on a fix here? I'm considering moving this off the P1 list, but would like to know if you think a fix would require beta testing or if it would be safe for RC
Comment 21 Mike Beltzner [:beltzner, not reading bugmail] 2009-01-21 14:14:03 PST
(In reply to comment #3)
> we need to re-write how the feed processor works instead of playing
> whack-a-mole.

(In reply to comment #19)
> Apparently people don't give a rats ass for the Top Security Bugs list. We
> should remove the Feeds feature until it can get rewritten safely.

Is there a bug on rewriting the parser (or the Feed Preview UI) that explains what, precisely, about it is causing this class of security problem and what needs to be done?

You find yourself with a willing product driver, who shares your frustration and empathizes with your plight. He is unwilling to remove the component within a release (and so, whack-a-mole it will be) but is willing to do what he can to stop this class of attack in future versions of Firefox.
Comment 22 Mike Beltzner [:beltzner, not reading bugmail] 2009-01-21 15:14:24 PST
(In reply to comment #21)
> Is there a bug on rewriting the parser (or the Feed Preview UI) that explains
> what, precisely, about it is causing this class of security problem and what
> needs to be done?

Bug 474698 was filed, as if by magic.
Comment 23 Mike Beltzner [:beltzner, not reading bugmail] 2009-01-22 07:36:31 PST
Mano, although I've moved this to P2 (blocking RC1) it still blocks release, and we should try to get it fixed early next week on trunk. Can you please give us an ETA on a patch, or list your ideas for a fix so someone else can take it?
Comment 24 Samuel Sidler (old account; do not CC) 2009-02-02 07:41:51 PST
Minusing for the branch release, again. Any fix we take is going to need baking on m-c and/or 1.9.1 anyway... Code freeze for 1.9.0.8 will likely be early March. Please get a patch worked up and landed in m-c/1.9.1 before then.
Comment 25 Mike Beltzner [:beltzner, not reading bugmail] 2009-02-09 08:18:42 PST
Mano: you were expecting a patch last week; what's our progress, here?
Comment 26 Mano (::mano, needinfo? for any questions; not reading general bugmail) 2009-02-10 08:06:23 PST
Ooops, I accidentally landed the last patch as part of bug 459323, oops oops. I'm attaching the other part now.
Comment 27 Mano (::mano, needinfo? for any questions; not reading general bugmail) 2009-02-10 08:17:07 PST
Created attachment 361522 [details] [diff] [review]
trunk+1.9.1 patch (depends on the js bug)
Comment 28 Mano (::mano, needinfo? for any questions; not reading general bugmail) 2009-02-10 08:21:38 PST
Created attachment 361525 [details] [diff] [review]
combined patch for 1.9.0 (depends on the js bug)
Comment 29 :Gavin Sharp [email: gavin@gavinsharp.com] 2009-02-10 11:01:52 PST
The testcase no longer works, presumably because bug 352124 was fixed, which makes this a bit hard to verify.

Do we need to worry about nsIClassInfo? GetInterfaces takes an aValue parameter and gets .value off of it, and xpconnect seems to allow my custom object with a .value getter. Was worried about the same being possible with QI, but it seems to check for valid IID objects (not sure whether there's a way around that).
Comment 30 :Gavin Sharp [email: gavin@gavinsharp.com] 2009-02-10 11:02:50 PST
Otherwise these look good.
Comment 31 Mano (::mano, needinfo? for any questions; not reading general bugmail) 2009-02-10 11:06:27 PST
We don't call the getter for getInterface AFAICT.
Comment 32 Mano (::mano, needinfo? for any questions; not reading general bugmail) 2009-02-10 11:07:04 PST
(the getter for retval.value in getInterface, that is).
Comment 33 :Gavin Sharp [email: gavin@gavinsharp.com] 2009-02-10 11:12:10 PST
Testing implies otherwise:

> var fw=Components.classes["@mozilla.org/browser/feeds/result-writer;1"].getService(Components.interfaces.nsIFeedWriter)

> fw.getInterfaces({get value (){print("foo");}})
foo
{67003393-018c-4e96-af10-c6c51a049fad},{986c11d0-f340-11d4-9075-0010a4e73d9a},{00000000-0000-0000-c000-000000000046}
Comment 34 :Gavin Sharp [email: gavin@gavinsharp.com] 2009-02-18 14:02:28 PST
Comment on attachment 361522 [details] [diff] [review]
trunk+1.9.1 patch (depends on the js bug)

This is sufficient to fix all the cases apart from my concern in the second paragraph of comment 29, but since partial fixes don't really get us much, r- until we figure that out.
Comment 35 :Gavin Sharp [email: gavin@gavinsharp.com] 2009-02-18 14:03:42 PST
moz_bug_r_a4, it would be useful if you could provide a testcase that doesn't rely on 352124 (i.e. that works on current trunk or 1.9.1 branch).
Comment 36 moz_bug_r_a4 2009-02-19 09:24:04 PST
The testcase in this bug doesn't rely on bug 352124.  The reason the testcase
no longer works is that attachment 343055 [details] [diff] [review] has already landed (see comment #26).

fw.getInterfaces() seems to be safe.

function x() {
  o = { set value() { ... } };
  fw.getInterfaces(o);
}

In FW_getInterfaces(), countRef is not o itself, and thus the o.value setter is
not called in FW_getInterfaces().  It seems that xpconnect calls the o.value
setter after returning FW_getInterfaces().  When the o.value setter is called,
caller is |function getInterfaces() {[native code]}| and caller.caller is x().

Hmm, when JS calls fw.observe(subject, topic, data), it seems that xpconnect
calls topic.toString() and data.toString() before entering FW_observe().  Is
this right?  If so, attachment 343055 [details] [diff] [review] should be a sufficient patch.
Comment 37 Mano (::mano, needinfo? for any questions; not reading general bugmail) 2009-02-19 12:17:01 PST
I'm pretty sure both Brendan and Blake told me that's not the case (also see boris's comments).
Comment 40 :Gavin Sharp [email: gavin@gavinsharp.com] 2009-02-19 23:45:51 PST
Er, sorry - I meant "that does not depend on bug 344495", per comment 2. Is that bug no longer relevant?

I'd like to better understand whether getInterfaces needs similar protection, but I think that might require feedback from someone who's more familiar with the wrapper situation. Blake?
Comment 43 :Gavin Sharp [email: gavin@gavinsharp.com] 2009-02-20 21:18:24 PST
Thanks moz_bug_r_a4.
Comment 44 Mano (::mano, needinfo? for any questions; not reading general bugmail) 2009-02-21 03:34:43 PST
I suppose it's possible to alter content's toString as well?
Comment 45 moz_bug_r_a4 2009-02-23 11:13:05 PST
  // nsIObserver
  observe: function FW_observe(subject, topic, data) {
dump("in FW_observe()\n");
dump("subject: " + typeof subject + " " + subject + "\n");
dump("topic: " + typeof topic + " " + topic + "\n");
dump("data: " + typeof data + " " + data + "\n");
    // see init()
    subject = new XPCNativeWrapper(subject);

Even when content calls fw.observe({}, {}, {}), at this point, topic and data
are already string primitives.  Content's toString does not affect it.

I can pass an untrusted object as |subject| to FW_observe(), but I can't think
of a way to pass objects as topic/data to FW_observe().  Am I missing
something?
Comment 46 Mike Beltzner [:beltzner, not reading bugmail] 2009-02-24 14:20:00 PST
mrbkap: Mano's gonna try and see if we can come up with a front end fix, but as a heads-up, we might need some of your time on this if we can't get that to work.

This bug should also be morphed, I think, to cover the more general issue.
Comment 47 Mike Beltzner [:beltzner, not reading bugmail] 2009-02-25 00:02:01 PST
Is this now the same as bug 479560?
Comment 48 Mano (::mano, needinfo? for any questions; not reading general bugmail) 2009-02-25 06:32:39 PST
Could you please cc me on it?
Comment 49 Blake Kaplan (:mrbkap) (please use needinfo!) 2009-02-25 15:16:27 PST
(In reply to comment #47)
> Is this now the same as bug 479560?

Yes. Note that I filed bug 480205 on tracking the work being done for the wrapper. This way we can use these other bugs for whack-a-mole patches if we need to.
Comment 50 Samuel Sidler (old account; do not CC) 2009-03-05 18:58:34 PST
Reassigning this to Blake, per comment 49.
Comment 51 Mike Beltzner [:beltzner, not reading bugmail] 2009-03-10 15:25:09 PDT
--> P1, as this bug will require the wider feedback of a beta release or is of sufficient complexity that we should be looking at it sooner, not later.
Comment 52 Mike Beltzner [:beltzner, not reading bugmail] 2009-03-18 00:16:00 PDT
Blake? Progress? WIP? ETA?
Comment 53 Blake Kaplan (:mrbkap) (please use needinfo!) 2009-03-18 01:28:20 PDT
I have the skeleton written and compiling. Now I'm working on the real guts (all of the auto wrapping/unwrapping logic) and once that's debugged, I have to hook the wrappers up to the global namespace stuff. I'm guessing around 3 days.
Comment 54 Mike Beltzner [:beltzner, not reading bugmail] 2009-04-13 12:47:00 PDT
Blake: once the wrapper is done in bug 480205, will there be an additional patch to write here? Can we parallelize the work a bit by getting someone else to write that patch now?
Comment 55 Mike Beltzner [:beltzner, not reading bugmail] 2009-04-14 11:13:54 PDT
Blake tells me that once bug 480205 is done, this will be done.
Comment 56 Mike Beltzner [:beltzner, not reading bugmail] 2009-04-17 04:36:54 PDT
I'm told this now depends on bug 479560, not actually bug 480205
Comment 57 Mike Beltzner [:beltzner, not reading bugmail] 2009-04-17 08:29:46 PDT
Now that bug 479560 is fixed191, is this? Mano? Blake?
Comment 58 Johnny Stenback (:jst, jst@mozilla.com) 2009-04-17 10:46:37 PDT
Yes, this was fixed by the fix for bug 479560.
Comment 59 Henrik Skupin (:whimboo) 2009-04-21 07:13:10 PDT
Verified fixed on trunk, 1.9.1, and 1.9.0.10 with builds on OS X and Windows:

Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.2a1pre) Gecko/20090420 Minefield/3.6a1pre ID:20090420031158

Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.1b4pre) Gecko/20090420 Shiretoko/3.5b4pre ID:20090420031111

Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.0.10pre) Gecko/2009042104 GranParadiso/3.0.10pre ID:2009042104
Comment 60 Martin Stránský 2009-05-27 03:57:29 PDT
Comment on attachment 337613 [details]
testcase

It still works on Linux/1.9.0.11.
Comment 61 moz_bug_r_a4 2009-05-27 08:54:37 PDT
It seems that the fix for the first testcase did not land on 1.9.0 branch. 
(The fix for bug 479560 fixed only testcase 4.)
Comment 62 Samuel Sidler (old account; do not CC) 2009-05-27 10:00:06 PDT
Blake?

Does that mean this isn't really fixed on 1.9.1 either? I don't see where a fix for that branch landed, certainly not in this bug either... Removing keywords for now, which will put it back on the blocker list. (/me hides)
Comment 63 Mano (::mano, needinfo? for any questions; not reading general bugmail) 2009-05-27 10:54:28 PDT
Comment on attachment 343055 [details] [diff] [review]
patch

It was fixed by this patch, which was landed way before we branched for 1.9.1. We should land this on 1.9.0.
Comment 64 Samuel Sidler (old account; do not CC) 2009-05-27 11:13:27 PDT
Asaf, does that patch apply to the 1.9.0 branch? I also note that you said you landed it with bug 459323 (comment 26). Do we need that patch too?

Adding back the verified1.9.1 keyword, but leaving off the 1.9.0.11 one, for obvious reasons.
Comment 65 Blake Kaplan (:mrbkap) (please use needinfo!) 2009-06-11 17:12:47 PDT
Mano, can you figure out what needs to land in FeedWriter land for 1.9.0? Thanks!
Comment 66 Samuel Sidler (old account; do not CC) 2009-06-16 07:55:54 PDT
Mano: Can we get a status update? Code freeze is today and it'd be good to know at least which patch we need to take...
Comment 67 Samuel Sidler (old account; do not CC) 2009-08-05 16:22:56 PDT
Mano: We *really* need an update here. Can you *please* get us a patch to land on 1.9.0? Code freeze is on August 11.
Comment 68 Mano (::mano, needinfo? for any questions; not reading general bugmail) 2009-08-06 15:52:57 PDT
In 24 hours, yes.
Comment 69 Samuel Sidler (old account; do not CC) 2009-08-10 17:10:25 PDT
Mano: We're past the 96 hour mark... any work here?
Comment 70 Mano (::mano, needinfo? for any questions; not reading general bugmail) 2009-08-11 15:42:55 PDT
In the next couple of hours. Sorry for the delay.
Comment 71 Mano (::mano, needinfo? for any questions; not reading general bugmail) 2009-08-11 17:15:48 PDT
Created attachment 393944 [details] [diff] [review]
190

I think that's it. Please land this for me if i'm not around.
Comment 72 Samuel Sidler (old account; do not CC) 2009-08-11 20:05:01 PDT
Comment on attachment 393944 [details] [diff] [review]
190

Approved for 1.9.0.14. a=ss

dietrich: Can you make sure this gets landed tomorrow if Mano hasn't landed it by then?
Comment 73 Mano (::mano, needinfo? for any questions; not reading general bugmail) 2009-08-12 01:34:18 PDT
mozilla/browser/components/feeds/src/FeedWriter.js 1.58

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