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.

we need to re-write how the feed processor works instead of playing whack-a-mole.

Who can own this bug?

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?

Could you actually pass a native object that implements (explicitly or implicitly) nsIPrefBranch2 and still be an untrusted caller?

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.

I totally forgot observe take nsISupports and not nsIPrefBrnach2, I should fix that. Thanks a ton.

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.

Can we at least get this fix landed on the trunk so it can be tested? Or are we waiting for something else?

I'll take care of this today.

Has this landed?

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.

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.

This bug is on our "Top Security Bugs" list. Please treat as top priority.

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?

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.

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

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

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

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?

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.

Mano: you were expecting a patch last week; what's our progress, here?

Ooops, I accidentally landed the last patch as part of bug 459323, oops oops. I'm attaching the other part now.

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

Otherwise these look good.

We don't call the getter for getInterface AFAICT.

(the getter for retval.value in getInterface, that is).

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

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

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.

I'm pretty sure both Brendan and Blake told me that's not the case (also see boris's comments).

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?

Thanks moz_bug_r_a4.

I suppose it's possible to alter content's toString as well?

// 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?

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.

Is this now the same as bug 479560?

Could you please cc me on it?

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

Reassigning this to Blake, per comment 49.

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

Blake? Progress? WIP? ETA?

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.

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?

Blake tells me that once bug 480205 is done, this will be done.

I'm told this now depends on bug 479560, not actually bug 480205

Now that bug 479560 is fixed191, is this? Mano? Blake?

Yes, this was fixed by the fix for bug 479560.

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 on attachment 337613 It still works on Linux/1.9.0.11.

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

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

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.

Mano, can you figure out what needs to land in FeedWriter land for 1.9.0? Thanks!

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

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.

In 24 hours, yes.

Mano: We're past the 96 hour mark... any work here?

In the next couple of hours. Sorry for the delay.

I think that's it. Please land this for me if i'm not around.

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?

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