987433 - Use after free with relList attribute of <a> element

Status: VERIFIED FIXED

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

Description

[Nils]

Attachment: crash.html

Testcase crashes the latest ASAN Firefox build. ASAN output attached in crash.txt. The relList attribute is freed while JS references are still alive.

Probably related to changes in bug #968637

Comment 1

[Nils]

Attachment: crash.txt

Comment 2

[Olli Pettay [:smaug][[e-mail]]]

Yes, we don't seem to call DropReference().

Comment 3

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Why aren't we just using cycle collection here?

Comment 4

[Olli Pettay [:smaug][[e-mail]]]

That I can't recall. I think we should just CC here.

Comment 5

[Boris Zbarsky [:bzbarsky]]

Ouch, I'm sorry I missed that.  :(

We should definitely kill off that weak ref.  For one thing, holding on to the token list means we can mutate attributes, which can trigger mutation observers, so should keep the element alive.

And yes, this is a regression from bug 968637.  Not sure whether I should add the dep....

Comment 6

[Olli Pettay [:smaug][[e-mail]]]

Attachment: patch

I can't reproduce this now. I think I did manage yesterday...
But based on the stack and the current code the patch is certainly needed.

Comment 7

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Comment on attachment 8396556 [details] [diff] [review]
patch

Review of attachment 8396556 [details] [diff] [review]:
-----------------------------------------------------------------

r=me

::: content/base/src/nsDOMTokenList.h
@@ +77,5 @@
>    void RemoveInternal(const nsAttrValue* aAttr,
>                        const nsTArray<nsString>& aTokens);
>    inline const nsAttrValue* GetParsedAttr();
>  
> +  nsCOMPtr<Element> mElement;

The tree seems to use both nsCOMPtr and nsRefPtr with Element.  We should clean that up at some point.

Comment 8

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8396556 [details] [diff] [review]
patch

[Security approval request comment]
How easily could an exploit be constructed based on the patch?
The patch itself doesn't actually touch the problematic code, but if someone realizes 
why the patch is needed, then it is quite obvious.

Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?
Check-in comment will be: "DOMTokenList should have a strong reference to its owner so that the API doesn't suddenly start returning empty string."


Which older supported branches are affected by this flaw?
30 (aurora)

If not all supported branches, which bug introduced the flaw?
bug 968637

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 Aurora

How likely is this patch to cause regressions; how much testing does it need?
Not likely to cause regressions.

Comment 9

[Al Billings [:abillings - ex-MoCo]]

Comment on attachment 8396556 [details] [diff] [review]
patch

sec-approval+ and approved for Aurora.

Comment 10

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/fea9904b6dec

Comment 11

[Randell Jesup [:jesup] (needinfo me)]

We're seeing new leaks on the push that included this and two other patches on inbound

Comment 12

[Ryan VanderMeulen [:RyanVM]]

Backed out for mochitest leaks on all platforms.
https://hg.mozilla.org/integration/mozilla-inbound/rev/3620f69143dd

https://tbpl.mozilla.org/php/getParsedLog.php?id=36762599&tree=Mozilla-Inbound
https://tbpl.mozilla.org/php/getParsedLog.php?id=36761050&tree=Mozilla-Inbound
https://tbpl.mozilla.org/php/getParsedLog.php?id=36767576&tree=Mozilla-Inbound
https://tbpl.mozilla.org/php/getParsedLog.php?id=36761690&tree=Mozilla-Inbound

Comment 13

[Boris Zbarsky [:bzbarsky]]

Do we traverse/unlink the token lists created by nsGenericHTMLElement::GetTokenList?  I bet we don't.  :(  So we could get leaks now that those are wrappercached, in fact...

Comment 14

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

We do, for itemtype, itemref, and itemprop.  http://mxr.mozilla.org/mozilla-central/source/content/base/src/FragmentOrElement.cpp#1801

But not for sandbox :(  http://mxr.mozilla.org/mozilla-central/source/content/html/content/src/HTMLIFrameElement.h#83

Comment 15

[Olli Pettay [:smaug][[e-mail]]]

Uh.

Comment 16

[Olli Pettay [:smaug][[e-mail]]]

That means we need this for FF29 too, since that sandbox stuff was added in Bug 845057.

Comment 17

[Olli Pettay [:smaug][[e-mail]]]

(nsGenericHTMLElement::GetTokenList is too error prone)

Comment 18

[Olli Pettay [:smaug][[e-mail]]]

Attachment: hopefully less leaky

https://tbpl.mozilla.org/?tree=Try&rev=7894812fca3f

Comment 19

[Al Billings [:abillings - ex-MoCo]]

(In reply to Olli Pettay [:smaug] from comment #16)
> That means we need this for FF29 too, since that sandbox stuff was added in
> Bug 845057.

If you nominate a patch, I'll approve it.

Comment 20

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

(In reply to Olli Pettay [:smaug] from comment #17)
> (nsGenericHTMLElement::GetTokenList is too error prone)

Indeed.

Comment 21

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Comment on attachment 8397789 [details] [diff] [review]
hopefully less leaky

Review of attachment 8397789 [details] [diff] [review]:
-----------------------------------------------------------------

I don't review Foo*** very often.

Comment 22

[Olli Pettay [:smaug][[e-mail]]]

The previous patch got sec-a+, so I don't think it is worth to ask approval for the fixed patch.

Comment 23

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/c9640661eddc

Comment 24

[Al Billings [:abillings - ex-MoCo]]

(In reply to Olli Pettay [:smaug] from comment #22)
> The previous patch got sec-a+, so I don't think it is worth to ask approval
> for the fixed patch.

ohai.

Can we get a beta and aurora patch though?

Comment 25

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8397789 [details] [diff] [review]
hopefully less leaky

This applies with fuzz

[Approval Request Comment]
Bug caused by (feature/regressing bug #): bug 968637 and Bug 845057
User impact if declined: sec-crit crashes
Testing completed (on m-c, etc.): just landed
Risk to taking this patch (and alternatives if risky): 
Should be rather low risk. Leaks in the previous landing revealed another
cases of this issue, and the current patch is now safer. 
String or IDL/UUID changes made by this patch: NA

Comment 26

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/c9640661eddc

Comment 27

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/92cf8d0f0d58
https://hg.mozilla.org/releases/mozilla-beta/rev/8074caa1fb14

Comment 28

[Olli Pettay [:smaug][[e-mail]]]

Thanks again Ryan!

Comment 29

[Bogdan Maris, Desktop Test Engineering]

I reproduced the crash on ASAN build from 2014-03-24 and verified that the issue is fixed on latest Aurora, latest Nightly and Firefox 29 beta 5 ASAN builds.