Closed Bug 454363 Opened 16 years ago Closed 15 years ago

Arbitrary code execution using FeedWriter.onPageChanged() method

Categories

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

defect

Tracking

(Not tracked)

VERIFIED FIXED
Firefox 3.6a1

People

(Reporter: moz_bug_r_a4, Assigned: asaf)

References

Details

(Keywords: fixed1.9.0.14, testcase, verified1.9.1, Whiteboard: [sg:critical][partially fixed by bug 479560])

Attachments

(4 files, 1 obsolete file)

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.
Attached file testcase
Flags: blocking1.9.0.3?
Flags: blocking-firefox3.1?
Keywords: testcase
OS: Windows XP → All
Hardware: PC → All
Whiteboard: [sg:critical]
Version: unspecified → Trunk
we need to re-write how the feed processor works instead of playing whack-a-mole.
Flags: blocking1.9.0.3? → blocking1.9.0.3+
Who can own this bug?
Whiteboard: [sg:critical] → [sg:critical] needs owner
Attached patch patch (obsolete) — Splinter Review
Assignee: nobody → mano
Status: NEW → ASSIGNED
Attachment #343054 - Flags: review?(gavin.sharp)
Priority: -- → P1
Whiteboard: [sg:critical] needs owner → [sg:critical] needs review gavin
Target Milestone: --- → Firefox 3.1b2
Attached patch patchSplinter Review
Attachment #343054 - Attachment is obsolete: true
Attachment #343055 - Flags: review?(gavin.sharp)
Attachment #343054 - Flags: review?(gavin.sharp)
Attachment #343055 - Flags: review?(gavin.sharp) → review+
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.
Flags: blocking1.9.0.5+
Flags: blocking1.9.0.4-
Flags: blocking1.9.0.4+
Whiteboard: [sg:critical] needs review gavin → [sg:critical]
Can we at least get this fix landed on the trunk so it can be tested? Or are we waiting for something else?
Whiteboard: [sg:critical] → [sg:critical] needs trunk landing
Flags: blocking-firefox3.1? → blocking-firefox3.1+
I'll take care of this today.
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.
Whiteboard: [sg:critical] needs trunk landing → [sg:critical][needs patch]
Target Milestone: Firefox 3.1b2 → Firefox 3.1
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.
Flags: blocking1.9.0.6+
Flags: blocking1.9.0.5-
Flags: blocking1.9.0.5+
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.
Flags: blocking1.9.0.7+
Flags: blocking1.9.0.6-
Flags: blocking1.9.0.6+
Flags: wanted1.8.1.x-
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.
Whiteboard: [sg:critical][needs patch] → [sg:critical][eta?][needs patch][really P1?]
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?
Priority: P1 → P2
Whiteboard: [sg:critical][eta?][needs patch][really P1?] → [sg:critical][eta?][needs patch]
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.
Flags: blocking1.9.0.8+
Flags: blocking1.9.0.7-
Flags: blocking1.9.0.7+
Depends on: 477128
Mano: you were expecting a patch last week; what's our progress, here?
Whiteboard: [sg:critical][eta?][needs patch] → [sg:critical][waiting for the landing of bug 477128, patch ready]
Ooops, I accidentally landed the last patch as part of bug 459323, oops oops. I'm attaching the other part now.
Attachment #361522 - Flags: review?(gavin.sharp)
Attachment #361525 - Flags: review?(gavin.sharp)
Attachment #361522 - Attachment description: depends on the js bug → trunk+1.9.1 patch (depends on the js bug)
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}
Whiteboard: [sg:critical][waiting for the landing of bug 477128, patch ready] → [sg:critical][needs review mconnor]
Attachment #361522 - Flags: review?(gavin.sharp) → review?(mconnor)
Attachment #361525 - Flags: review?(gavin.sharp) → review?(mconnor)
Attachment #361522 - Flags: review?(mconnor) → review-
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.
Attachment #361525 - Flags: review?(mconnor) → review-
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).
Whiteboard: [sg:critical][needs review mconnor] → [sg:critical]
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.
Whiteboard: [sg:critical] → [sg:critical][needs new patch]
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.
Could you please cc me on it?
Depends on: cow
(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.
Flags: wanted1.9.0.x+
Reassigning this to Blake, per comment 49.
Assignee: mano → mrbkap
--> 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.
Priority: P2 → P1
Flags: blocking1.9.0.8+ → blocking1.9.0.9+
Blake? Progress? WIP? ETA?
Whiteboard: [sg:critical][needs new patch] → [sg:critical][needs ETA mrbkap]
Target Milestone: Firefox 3.1 → Firefox 3
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.
Whiteboard: [sg:critical][needs ETA mrbkap] → [sg:critical][ETA 3 days?]
Whiteboard: [sg:critical][ETA 3 days?] → [sg:critical][needs new wrapper, poor mrbkap]
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.
Whiteboard: [sg:critical][needs new wrapper, poor mrbkap] → [sg:critical][to be fixed by bug 480205]
I'm told this now depends on bug 479560, not actually bug 480205
Depends on: CVE-2009-1841
No longer depends on: cow
Whiteboard: [sg:critical][to be fixed by bug 480205] → [sg:critical][to be fixed by bug 479560]
Now that bug 479560 is fixed191, is this? Mano? Blake?
Yes, this was fixed by the fix for bug 479560.
Status: ASSIGNED → RESOLVED
Closed: 15 years ago
Resolution: --- → FIXED
Whiteboard: [sg:critical][to be fixed by bug 479560] → [sg:critical][fixed by 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
Status: RESOLVED → VERIFIED
Target Milestone: Firefox 3 → Firefox 3.6a1
Comment on attachment 337613 [details]
testcase

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.
Keywords: verified1.9.1
Whiteboard: [sg:critical][fixed by bug 479560] → [sg:critical][partially fixed by bug 479560]
Flags: blocking1.9.0.12?
Flags: blocking1.9.0.12?
Flags: blocking1.9.0.12+
Flags: blocking1.9.0.11+
Mano, can you figure out what needs to land in FeedWriter land for 1.9.0? Thanks!
Assignee: mrbkap → mano
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...
Flags: blocking1.9.0.13+
Flags: blocking1.9.0.12-
Flags: blocking1.9.0.12+
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.
Attached patch 190Splinter Review
I think that's it. Please land this for me if i'm not around.
Attachment #393944 - Flags: approval1.9.0.15?
Attachment #393944 - Flags: approval1.9.0.14?
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?
Attachment #393944 - Flags: approval1.9.0.15?
Attachment #393944 - Flags: approval1.9.0.14?
Attachment #393944 - Flags: approval1.9.0.14+
mozilla/browser/components/feeds/src/FeedWriter.js 1.58
Keywords: fixed1.9.0.14
Group: core-security
Product: Firefox → Firefox Graveyard
You need to log in before you can comment on or make changes to this bug.