"ASSERTION: must be purple" with setUserData

VERIFIED FIXED in Firefox 13

Status

()

Core
XPCOM
--
critical
VERIFIED FIXED
5 years ago
5 years ago

People

(Reporter: Jesse Ruderman, Assigned: mccr8)

Tracking

(Blocks: 1 bug, 4 keywords)

Trunk
mozilla13
assertion, crash, regression, testcase
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox12 unaffected, firefox13 verified, firefox14 verified, firefox-esr10 unaffected)

Details

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

Attachments

(3 attachments)

(Reporter)

Description

5 years ago
Created attachment 608837 [details]
testcase (crashes after about 10 seconds)
(Reporter)

Comment 1

5 years ago
Created attachment 608838 [details]
stack trace
(Assignee)

Comment 2

5 years ago
Thanks!
Assignee: nobody → continuation
(Reporter)

Comment 3

5 years ago
Also crashes in opt nightly: bp-8d859490-dc5a-497e-b3d8-60cd52120323
(Assignee)

Comment 4

5 years ago
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.
status-firefox12: --- → unaffected
status-firefox13: --- → affected
status-firefox14: --- → affected
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.
(Assignee)

Comment 6

5 years ago
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]
(Assignee)

Comment 7

5 years ago
I audited all calls to nsCycleCollectingAutoRefCnt::RemovePurple and nsCycleCollectingAutoRefCnt::unmarkPurple and with my patch they should be okay.
(Assignee)

Comment 8

5 years ago
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.
(Assignee)

Comment 9

5 years ago
Created attachment 608926 [details] [diff] [review]
ensure that participants are purple before unpurpling them
(Assignee)

Comment 10

5 years ago
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.
(Reporter)

Updated

5 years ago
Blocks: 728460
Keywords: regression
(Assignee)

Comment 11

5 years ago
Comment on attachment 608926 [details] [diff] [review]
ensure that participants are purple before unpurpling them

Try run looked good.
Attachment #608926 - Flags: review?(bugs)

Updated

5 years ago
Attachment #608926 - Flags: review?(bugs) → review+
(Assignee)

Comment 12

5 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/f01aac728984
Target Milestone: --- → mozilla14
https://hg.mozilla.org/mozilla-central/rev/f01aac728984
Status: NEW → RESOLVED
Last Resolved: 5 years ago
status-firefox14: affected → fixed
Resolution: --- → FIXED
(Assignee)

Comment 14

5 years ago
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+

Updated

5 years ago
status-firefox-esr10: --- → unaffected
(Assignee)

Comment 16

5 years ago
https://hg.mozilla.org/releases/mozilla-aurora/rev/28722c954913
status-firefox13: affected → fixed
Target Milestone: mozilla14 → mozilla13
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.
status-firefox13: fixed → 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
status-firefox14: fixed → verified
You need to log in before you can comment on or make changes to this bug.