Closed Bug 895222 Opened 11 years ago Closed 11 years ago

Crash with expandos, HTMLPropertiesCollection, CC


(Core :: DOM: Core & HTML, defect)

Not set



Tracking Status
firefox22 --- unaffected
firefox23 + fixed
firefox24 + fixed
firefox25 + fixed
firefox-esr17 --- unaffected
b2g18 --- unaffected


(Reporter: jruderman, Assigned: smaug)



(Keywords: crash, sec-high, testcase, Whiteboard: [adv-main23-])

Crash Data


(3 files)

1. Install
2. Load the testcase

Usually crashes [@ mozilla::dom::HTMLPropertiesCollection::EnsureFresh].  The crash is more reliable in debug builds.

Attached file stack

So the crash location is dereferencing mNames in EnsureFresh.

mNames is set in the HTMLPropertiesCollection constructor like so:

  mNames = new PropertyStringList(this);

The only place it's touched after this is in HTMLPropertiesCollection unlink.  And indeed, in the debugger I see us unlinking this particular HTMLPropertiesCollection object... and then doing a call into it from JS!

I added breakpoints to AddRef/Release on HTMLPropertiesCollection, and I see several calls to AddRef: one from the PropertyStringList ctor, one from nsGenericHTMLElement::Properties, and one from wrapping into the DOM binding.  Then I'm seeing the object get CCed.  Something is seriously broken here.  I wonder whether this is a snowhite regression...
(I'd guess it is the proxy handling which causes problems.)
Oh, PropertyStringList has two refcnts. That is bad. Super error prone.
Peter, do you have time to look into this?
Flags: needinfo?(peterv)
Assignee: nobody → continuation
Flags: needinfo?(peterv)
As bz expected, the CC is identifying this object as garbage.

Here's the object, with its refcount of 3:

0x129362e00 [rc=3] HTMLPropertiesCollection
> 0x129362ce0 mRoot
> 0x1293425c0 mNames
> 0x1184cb300 Preserved wrapper

Here are the three references:

0x129362ce0 [rc=1] FragmentOrElement (xhtml) div file:///Users/amccreight/mz/tests/895222.html
> 0x129362e00

0x1293425c0 [rc=1] PropertyStringList
> 0x129362e00 mCollection

0x1184cb300 [gc] JS Object (Proxy)
> 0x1184353e0 extra0
> 0x129362e00 UnwrapDOMObject(obj) 

I'm not sure which of these is unexpected.  I'm guessing the Proxy?

It is rather peculiar that the div element's reference doesn't have a name associated with it, but that could just be a superficial deficiency.
I'm marking this sec-high because when combined with another bug that leaves an unlinked object with a dangling pointer, this could be turned into a UAF.  sec-moderate might also be okay, depending on how confident we are that doesn't happen. ;)
Keywords: sec-high
I don't have much time for this for the next day or two, it sounds like, and Olli said he'd take a look.  Thanks!
Assignee: continuation → bugs
All three references from comment 6 are expected to be references to the object...  The proxy's reference, however, should keep it alive, since the proxy is alive.
The only reference to it in the CC graph is the HTMLPropertiesCollection.

In the GC graph, its references are thus:
via DOM expando object :
0x1184cb300 [Proxy <no private>]

via mExpandoAndGeneration.expando :
0x118435400 [Object <no private>]
    --[y]-> 0x1184cb300 [Proxy <no private>]

So I guess we're not tracing through the mExpandoAndGeneration.expando in the CC graph for some reason?  0x118435400 doesn't show up in the CC graph at all.
Attached patch patchSplinter Review
This should be pretty clear.
We added something new to trace, but didn't change NeedsScriptTraverse which is used in CanSkipInCC (I know, NeedsScriptTraverse was error prone).
This should work for now. If we start to add more complex tracing to various nodes, we
may need to re-think this some more.

Change to nsChildContentList is needed because NeedsScriptTraverse starts to take nsINode as param.
Attachment #778750 - Flags: review?(continuation)
Depends on: 820846
Comment on attachment 778750 [details] [diff] [review]

Review of attachment 778750 [details] [diff] [review]:

Attachment #778750 - Flags: review?(continuation) → review+
Comment on attachment 778750 [details] [diff] [review]

[Security approval request comment]
How easily could an exploit be constructed based on the patch? The crash in this bug is a null-pointer crash.
It is possible, although I think unlikely, that some other type of crash can be found.

Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?
Comment will be about improving skippability optimization

Which older supported branches are affected by this flaw?
beta (ff23), aurora (ff24), nightly (ff25)

If not all supported branches, which bug introduced the flaw?
bug 820846, kind of.

Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be?
The patch applies cleanly to beta/aurora/nightly

How likely is this patch to cause regressions; how much testing does it need?
The patch may cause tiny bit higher cycle collection times. Telemetry would tell the regression quite fast, but
unfortunately telemetry is all broken atm. Hopefully it will be fixed in the next few days.

[Approval Request Comment]
Bug caused by (feature/regressing bug #): bug 820846
User impact if declined: Random crashes because expando properties on document and form element aren't effectively
strong pointers
Testing completed (on m-c, etc.): tryserver looks ok. 
Risk to taking this patch (and alternatives if risky): Low-risky. We're just more careful when to _not_ optimize something
                                                       out from cycle collection graph
String or IDL/UUID changes made by this patch: NA
Attachment #778750 - Flags: sec-approval?
Attachment #778750 - Flags: approval-mozilla-beta?
Attachment #778750 - Flags: approval-mozilla-aurora?
I need release management comments about whether we can take this on beta at this point before sec-approval. It is only a four line patch though!
Flags: needinfo?(release-mgmt)
(In reply to Al Billings [:abillings] from comment #14)
> I need release management comments about whether we can take this on beta at
> this point before sec-approval. It is only a four line patch though!

Since telemetry would be our indicator of regression but telemetry is broken atm, I'm not comfortable taking this to Beta. Given it's a higher likelihood of crashing in debug builds as well as comment 7 suggesting this is closer to moderate in risk (though labeled 'high') I think we should hold off for now. If telemetry is repaired before this Thursday's beta I would reconsider but that's pushing it.  Might be better just to wait for FF24.
Flags: needinfo?(release-mgmt)
telemetry is not broken anymore.
And note, I don't expect there to be any significant regression.
(effectively one virtual call in case there is expando properties on a DOM Node)
(In reply to Olli Pettay [:smaug] from comment #16)
> telemetry is not broken anymore.

OK then :)

Abillings - if you're comfortable with sec-approval+ here, we can take this to branches then.
Flags: needinfo?(abillings)
Comment on attachment 778750 [details] [diff] [review]

Sounds good. Let's do it.
Attachment #778750 - Flags: sec-approval?
Attachment #778750 - Flags: sec-approval+
Attachment #778750 - Flags: approval-mozilla-beta?
Attachment #778750 - Flags: approval-mozilla-beta+
Attachment #778750 - Flags: approval-mozilla-aurora?
Attachment #778750 - Flags: approval-mozilla-aurora+
Flags: needinfo?(abillings)
Closed: 11 years ago
Flags: in-testsuite?
Resolution: --- → FIXED
Target Milestone: --- → mozilla25
Firefox 22 is unaffected by this issue?
yes. bug 820846 landed to FF23
Whiteboard: [adv-main23-]
Group: core-security
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.