Jesse Ruderman Reporter
	Description • 14 years ago

1. Install 'DOM Fuzz Lite' from
    https://www.squarefree.com/extensions/domFuzzLite.xpi
2. Load the testcase.

Result: crash [@ mozilla::DOMSVGPathSegList::ItemAt]

Security-sensitive because anything that involves GC scares me.

	Jesse Ruderman Reporter
	Comment 2 • 14 years ago

Debug builds crash.

Something really weird happens when I load the testcase in opt. I wish it would crash :(

	Marcia Knous [:marcia]
	Comment 3 • 14 years ago

https://crash-stats.mozilla.com/report/index/bp-f5c8a064-4f76-4b31-969f-a590a2110310 is my report.

	Johnny Stenback (:jst)
	Comment 4 • 14 years ago

Blake, care to have a look?

	Daniel Holbert [:dholbert] Assignee
	Comment 5 • 14 years ago

I just took a look in GDB, and it looks like basically we're never calling the ~DOMSVGPathSegList() destructor (which is responsible for removing the DOMSVGPathSegList from an internal table).

So during GC, Release() decrements the DOMSVGPathSegList's refcount (to 0), and then calls delete(), which fills the DOMSVGPathSegList with 0x5a5a5a5a -- and in the meantime, we **never touch the destructor.**

This leaves the pointer around in the table, just waiting for us to use it and explode.

I'm not sure why delete() isn't triggering the destructor, but I think that's the main thing that's broken here.

	Daniel Holbert [:dholbert] Assignee
	Comment 6 • 14 years ago

(In reply to comment #5)
> I'm not sure why delete() isn't triggering the destructor, but I think that's
> the main thing that's broken here.

Gah, so it *did* trigger the destructor this time, so Comment 5 might be bogus. (sorry)

Investigating more thoroughly.  Stealing bug from mrbkap, per IRC discussion.

	Daniel Holbert [:dholbert] Assignee
	Comment 7 • 14 years ago

So even though the destructor seems to be triggered, the problem is still that the DOMSVGPathSegList is remaining in the table somehow -- so the call

>  DOMSVGPathSegList *baseValWrapper =
>    DOMSVGPathSegList::GetDOMWrapperIfExists(GetBaseValKey());

in SVGAnimatedPathSegList::ClearBaseValue() is returning non-null* when it should be returning null...

* (the deleted DOMSVGPathSegList that used to be associated with that key)

(I've verified that the key we use to do the removal in the destructor matches the key we use for the lookup in the quoted lines above.)

	Daniel Holbert [:dholbert] Assignee
	Comment 8 • 14 years ago

Ah, ok - so it turns out the table-removal happens *after* the table-lookup. (but we continue to use the value that we looked up earlier).

Specifically, we've got these lines in DOMSVGPathSegList::InternalListWillChangeTo:
>        ItemAt(index)->RemovingFromList();
>        ItemAt(index) = nsnull;

The first line there removes the last nsRefPtr that points to |this|, so it **deletes |this|**.  And after that, we're hosed, and we crash almost immediately (within the second line above).

So, we probably need a nsRefPtr "kung-fu death-grip" on |this|, for the duration of InternalListWillChangeTo.

	Daniel Holbert [:dholbert] Assignee
	Comment 9 • 14 years ago

(In reply to comment #8)
> So, we probably need a nsRefPtr "kung-fu death-grip" on |this|, for the
> duration of InternalListWillChangeTo.

Here's that as a patch.  Confirmed that it fixes this locally.  We might need something like this for other SVG list classes, too.

(The "if (length)" check is necessary because the DOMSVGPathSegList constructor calls "InternalListWillChangeTo", at which point we have a refcount of 0.  So if we were to add & remove the deathgrip at that point, then we get deleted (as soon as we're constructed).

	Daniel Holbert [:dholbert] Assignee
	Comment 10 • 14 years ago

Here's a testcase that uses a length list instead.  It crashes in DOMSVGLengthList::InternalListLengthWillChange, on the line right after
>   mItems[i]->RemovingFromList();

(essentially the same as with the original testcase)

At that point, the DOMSVGLengthList (|this|) and its DOMSVGAnimatedLengthList (|this| up one stack-level) have both just been deleted.

	Daniel Holbert [:dholbert] Assignee
	Comment 11 • 14 years ago

This testcase also crashes using a length-list (like testcase 2a).

In this new version, we simply retain a reference to an item in the list, rather than creating & inserting an item -- and we set the attribute to an invalid value, rather than clearing it.

So, it seems like we're vulnerable to this whenever some chunk of script retains a reference to a list-item past the time when the list gets cleared. ("cleared" = unset, or set to be empty, or set to an invalid value)

	Daniel Holbert [:dholbert] Assignee
	Comment 12 • 14 years ago

Just ran through this with jwatt, and he agrees we just need to add some death-grip nsRefPtrs.

FWIW, we were able to trigger this *without* Jesse's fuzzer extension -- just needed to replace his "fuzzPrivGC()" call with a setTimeout, to allow time for a GC.  (Presumably, the crucial object that gets GC'd is a reference to |text.x| or |text.x.baseVal|).

This SVG item-list code was all rewritten for Gecko 2.0, so this is unlikely to affect Gecko 1.9.x.  This bug should probably block an early 2.0.x release, though.  (I'm not sure if there's a way to request that yet, though -- I think blocking2.0 still means "blocking the Firefox 4 release"...?)

	Johnny Stenback (:jst)
	Comment 13 • 14 years ago

blocking2.0 .x+ means blocking a security/stability release, so marking as such.

	Daniel Holbert [:dholbert] Assignee
	Comment 14 • 14 years ago

Here's a more comprehensive fix.

I've audited the code for this sort of problem, and I think this catches all of the areas affected by this issue. (more detail coming up.)

	Daniel Holbert [:dholbert] Assignee
	Comment 15 • 14 years ago

THINGS WE NEED TO AVOID:
 SITUATION A: DOMSVGXxxList::Foo() calls Bar() on its only remaining Item.  Inside Bar(), we clear the last strong reference to the list, deleting it.

 SITUATION B: DOMSVGAnimatedXxxList::Foo() calls DOMSVGXxxList::Bar() on one of its "child" lists.  Inside Bar(), we clear the last strong reference to the "parent" animated list, deleting it.

In both cases, when control returns to Foo(), we've got a deleted |this| pointer, so we'll crash as soon as we touch any member variables or methods.

NOTE: This is only a problem when it's possible for there to be no references to the "parent" object in script.  So if Foo() is a DOM method called by script, then script necessarily holds an owning reference to the object, and it won't be deleted before Foo() completes.  (I've flagged these cases below as "OK; DOM method".)

AUDIT FOR PLACES WE COULD HIT SITUATIONS A & B IN LENGTH LISTS:
- Places where DOMSVGLength drops its nsRefPtr "mList" (Situation A):
 * DOMSVGLength::RemovingFromList (sets "mList = nsnull;")
   Called from:
   - DOMSVGLengthList::InternalListLengthWillChange --> FIXED IN PATCH
   - DOMSVGLengthList::ReplaceItem --> OK; DOM method
   - DOMSVGLengthList::RemoveItem  --> OK; DOM method
   - DOMSVGLengthList::MaybeRemoveItemFromAnimValListAt --> FIXED IN PATCH

 * DOMSVGLength::InsertingIntoList (sets "mList = aList;")
   - OK -- called by the *new* list, not by the old one.
     (old list may be deleted, and that's ok)

 * DOMSVGLength destructor (implicit)
   Automatically called when we clear last strong ref to a DOMSVGLength.
   In DOMSVGLengthList, this only happens when a local nsCOMPtr<DOMSVGLength>
   goes out of scope, which happens in:
   - DOMSVGLengthList::Initialize       --> OK; DOM method
   - DOMSVGLengthList::InsertItemBefore --> OK; DOM method
   - DOMSVGLengthList::ReplaceItem      --> OK; DOM method

- Places where DOMSVGLengthList drops its nsRefPtr "mAList" (Situation B):
 * DOMSVGLengthList destructor
   Automatically called when we clear last strong ref to a DOMSVGLengthList.
   That now happens when DOMSVGLengthList::InternalListLengthWillChange
   returns, becuase our newly-added kungFuDeathGrip goes out of scope.
   - And that method is called by:
      DOMSVGAnimatedLengthList::InternalBaseValListWillChangeTo
      --> FIXED IN PATCH (kungFuDeathGrip added)

Other list types are basically the same, or safer (due to not having "AnimatedList" forms, in the case of points-list & path-seg-list).

More detail on these other list-types coming soon.

	Daniel Holbert [:dholbert] Assignee
	Comment 16 • 14 years ago

NUMBER LISTS:
- Exactly the same as "length lists" case.  (DOMSVG*Number* and DOMSVG*Length* files are essentially identical)  So, just see Comment 15.

PATH SEG LISTS:
- Places where DOMSVGPathSeg drops its nsRefPtr "mList" (Situation A):
  * Exact same methods as for DOMSVGLength.  Those methods have same callers as described in Comment 15, so that all applies here.
- No threat of Situation B, since there's no animated list type for PathSegLists

POINT LISTS:
- Places where DOMSVGPoint drops its nsRefPtr "mList" (Situation A):
  * Exact same methods as for DOMSVGLength.  Those methods have same callers as described in Comment 15, so that all applies here.
- No threat of Situation B, since there's no animated list type for PointLists

	Daniel Holbert [:dholbert] Assignee
	Comment 17 • 14 years ago

This patch adds some mochitests for this bug, but it shouldn't land for a little while yet because:
 (a) we want to wait until the patch has landed on all affected branches (nightlies and 4.x)
 (b) the test currently relies on a
         netscape.security.PrivilegeManager.enablePrivilege
to force GC, but enablePrivilege is no longer kosher in mochitests.  I've tried to work around this using SpecialPowers, but haven't been successful yet -- see bug 642993. (That bug is talking about my test-writing for this bug, but I've tried to use generic language over there to avoid giving anything away.)

	Jonathan Watt [:jwatt]
	Comment 18 • 14 years ago

Comment on attachment 521889 [details] [diff] [review]
fix v2

Good job. r=jwatt

	Daniel Holbert [:dholbert] Assignee
	Comment 19 • 14 years ago

Landed fix on m-c: http://hg.mozilla.org/mozilla-central/rev/43467a3a651d

(Still needs to land on Firefox 4.0 branch, though.)

	Daniel Holbert [:dholbert] Assignee
	Comment 20 • 13 years ago

Comment on attachment 521889 [details] [diff] [review]
fix v2

At dveditz's suggestion, I'm requesting approval2.0, to land this security fix for some 4.0 security release.

This has zero expected behavior-change, aside from fixing crashes.  It just adds an extra kungFuDeathGrip to a few (non-recursive) functions, to keep something alive until the function returns.

	Daniel Veditz [:dveditz]
	Comment 21 • 13 years ago

Comment on attachment 521889 [details] [diff] [review]
fix v2

Approved for the mozilla2.0 repository, a=dveditz for release-drivers

	Daniel Holbert [:dholbert] Assignee
	Comment 22 • 13 years ago

http://hg.mozilla.org/releases/mozilla-2.0/rev/caa938935590

	Daniel Holbert [:dholbert] Assignee
	Comment 23 • 13 years ago

The test for this is now clear to land, safety-wise, but it still can't land due to part (b) of Comment 17 still being unresolved. (The test for this relies on enablePrivilege instead of SpecialPowers, for triggering a GC at exactly the right time, and last I checked, the straightforward method of extending SpecialPowers didn't work.)

	Daniel Holbert [:dholbert] Assignee
	Comment 24 • 13 years ago

bug 642993 comment 2 suggests that the testcase here may be workable with SpecialPowers now...