Closed Bug 738769 Opened 12 years ago Closed 12 years ago

"ASSERTION: must be purple" with setUserData

Categories

(Core :: XPCOM, defect)

defect
Not set
critical

Tracking

()

VERIFIED FIXED
mozilla13
Tracking Status
firefox12 --- unaffected
firefox13 --- verified
firefox14 --- verified
firefox-esr10 --- unaffected

People

(Reporter: jruderman, Assigned: mccr8)

References

Details

(4 keywords, Whiteboard: [sg:critical][qa+][advisory-tracking+])

Attachments

(3 files)

      No description provided.
Attached file stack trace
Thanks!
Assignee: nobody → continuation
Also crashes in opt nightly: bp-8d859490-dc5a-497e-b3d8-60cd52120323
Yeah, not surprising.  You end up trying to deref a tagged refcount.

This is a regression from bug 728460.  ChildFinder::NoteXPCOMChild calls CanSkip on its children.  CanSkip can end up removing its children from the purple buffer.  An XPCVariant can being a child of its child, and gets removed from the purple buffer during a CanSkip.

Here's the specific way this happens in this case (the purple node we're examining is the XPCVariant):

0x124e02680 [rc=1] XPCVariant
> 0x118032430 mJSVal
> 0x1250da530 mData

0x1250da530 [rc=5] nsGenericElement (xhtml) html file:///Users/amccreight/mz/tests/purple.html
> 0x120cf1670 mNodeInfo
> 0x124e02680 [user data (or handler)]
> 0x1250da5e0 mAttrsAndChildren[i]
> 0x12507c790 mAttrsAndChildren[i]

XPCVariant does a CanSkip on the nsGenericElement, which is in a currently visible document, so it calls MarkNodeChildren on the XPCVariant, which unpurples it.

The immediate fix is to change that CanSkip to a CanSkipThis or CanSkipInCC, but I think really we should consider changing UnmarkPurple to UnmarkIfPurple or something.  That extra branch won't kill us in CC cleanup code and will add a layer of defense.  We can probably also then tear out some of the weird special casing code.

I think this was hard to reproduce because we only run the ChildFinder some of the time, in the last cleanup slice before a CC.  I filed bug 738813 to allow control of this in fuzzing.  Always-on is probably more useful, but I guess there could be errors that are covered up by running the child finder before the CC, so we might as well support always-off as well.
OS: Mac OS X → All
Hardware: x86_64 → All
Also crashes in opt nightly: bp-8d859490-dc5a-497e-b3d8-60cd52120323

Takes about 10 seconds. Seems reliable.
In terms of vulnerability, what will happen here is something like this:

x = r.mFoo->mBar
r.mFoo = 2 * x + 1

r.mFoo is double a ref count, plus 1.  This is going to be an unaligned load of a 32-bit value (the low bit is always 1), but I don't know offhand if that will always crash or work on x86.

In general, an attacker can control the ref count of an object, but I'm not sure how easy it would be to make the ref count large enough that (times two plus one) it is an address that can actually be read from without crashing.

In this particular case that Jesse found, the object in question is an XPCVariant.  I think that an XPCVariant can only be owned by a single object, capping the ref count at 1.  If that is the case, then the only thing an attacker can do is force a read at address 3.  But I'm not sure, and maybe they can get a higher ref count under some circumstances.  It is also possible that there are other cases that we have not found.
Whiteboard: [sg:critical]
I audited all calls to nsCycleCollectingAutoRefCnt::RemovePurple and nsCycleCollectingAutoRefCnt::unmarkPurple and with my patch they should be okay.
By "okay" I mean they are all tested for being purple, and there's no place in between the check and the unmarking that could unpurple them, even assuming the CanSkip* callbacks unpurple arbitrary things.
Very mechanical patch.  Most of it is just renaming UnmarkPurple to UnmarkIfPurple.  The only interesting part is the very end in nsISupportsImpl.h, where I add a purple check.
Blocks: 728460
Keywords: regression
Comment on attachment 608926 [details] [diff] [review]
ensure that participants are purple before unpurpling them

Try run looked good.
Attachment #608926 - Flags: review?(bugs)
Attachment #608926 - Flags: review?(bugs) → review+
https://hg.mozilla.org/mozilla-central/rev/f01aac728984
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Comment on attachment 608926 [details] [diff] [review]
ensure that participants are purple before unpurpling them

[Approval Request Comment]
Regression caused by (bug #): 728460
User impact if declined: crashes in some situations, potentially security problems
Testing completed (on m-c, etc.): it has been on m-c for a day or two
Risk to taking this patch (and alternatives if risky): should be low. All it does it turn a debug-only assertion into a dynamic check, plus a bunch of renaming.
String changes made by this patch: none
Attachment #608926 - Flags: approval-mozilla-aurora?
Comment on attachment 608926 [details] [diff] [review]
ensure that participants are purple before unpurpling them

[Triage Comment]
sg:crit and low risk, approved for Beta 12 and Aurora 13.
Attachment #608926 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Whiteboard: [sg:critical] → [sg:critical][qa+]
Group: core-security
No crashes loading the testcase on FF 13b4 and FF 2012-05-18-mozilla-beta-debug on Win 7, Ubuntu 12.04 and Mac OS X 10.6. Marking verified.
Whiteboard: [sg:critical][qa+] → [sg:critical][qa+][advisory-tracking+]
No crashes loading the testcase on FF 14 2012-06-07-mozilla-beta-debug on Win 7, Ubuntu 12.04 and Mac OS X 10.6. Marking verified.

I was able to see the crash on Nightly 2012-03-23-mozilla-central-debug.
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: