1339591 - Possible UAFs with AutoRestore in SMIL code

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

[Andrew McCreight [:mccr8]]

This contains:

AutoRestore<bool> autoRestoreRunningSample(mRunningSample);
...
nsCOMPtr<nsIDocument> document(mDocument);  // keeps 'this' alive too

If |document| keeps this alive, then it is possible that destroying |document| will destroy |this|, which means the restore will be a use after free.

Comment 1

[Andrew McCreight [:mccr8]]

Similarly in nsSMILInstanceTime::HandleChangedInterval.

Comment 2

[Andrew McCreight [:mccr8]]

The only code we're running in between the two is destroying two nsTHashtables, so it would be hard to exploit it. (Well, aside from whatever is going on with other threads.)

Comment 3

[Brian Birtles (:birtles)]

Attachment: Patch

Comment 4

[Brian Birtles (:birtles)]

sec-moderate so landing this myself:
https://hg.mozilla.org/integration/mozilla-inbound/rev/33f49f751b63cde40ffe16d4c5fee0b4a64a6a79

Comment 5

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

https://hg.mozilla.org/mozilla-central/rev/33f49f751b63

Comment 6

[Ryan VanderMeulen [:RyanVM]]

Please request Aurora/Beta approval on this when you get a chance.

Comment 7

[Brian Birtles (:birtles)]

Comment on attachment 8837443 [details] [diff] [review]
Patch

Approval Request Comment
[Feature/Bug causing the regression]: bug 814921
[User impact if declined]: Unpatched user-after-free (although it appears hard to exploit)
[Is this code covered by automated tests?]: The SMIL functionality is well-tested but there is not specific test case for this vulnerability.
[Has the fix been verified in Nightly?]: Yes
[Needs manual test from QE? If yes, steps to reproduce]:  No
[List of other uplifts needed for the feature/fix]: None
[Is the change risky?]: No
[Why is the change risky/not risky?]: It just re-orders two stack based classes so that their destructors are run in a safe order.
[String changes made/needed]: None

Comment 8

[Gerry Chang [:gchang]]

Comment on attachment 8837443 [details] [diff] [review]
Patch

Fix a security issue. Aurora53+.

Comment 9

[Julien Cristau [:jcristau]]

Comment on attachment 8837443 [details] [diff] [review]
Patch

fix UAF in SMIL, beta52+

Comment 10

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/e2244b774969
https://hg.mozilla.org/releases/mozilla-beta/rev/758af211353c

Comment 11

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-esr52/rev/758af211353c