Possible UAFs with AutoRestore in SMIL code

RESOLVED FIXED in Firefox 52

Status

()

defect
RESOLVED FIXED
3 years ago
11 months ago

People

(Reporter: mccr8, Assigned: birtles)

Tracking

({csectype-uaf, sec-moderate})

unspecified
mozilla54
Points:
---
Bug Flags:
qe-verify -

Firefox Tracking Flags

(firefox-esr45 wontfix, firefox51 wontfix, firefox52+ fixed, firefox-esr5252+ fixed, firefox53+ fixed, firefox54+ fixed)

Details

(Whiteboard: [post-critsmash-triage][adv-main52+])

Attachments

(1 attachment)

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.
Similarly in nsSMILInstanceTime::HandleChangedInterval.
Summary: Possible UAF in nsSMILAnimationController::DoSample → Possible UAFs with AutoRestore in SMIL code
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.)
Posted patch PatchSplinter Review
Assignee: nobody → bbirtles
Status: NEW → ASSIGNED
Attachment #8837443 - Flags: review?(continuation)
Attachment #8837443 - Flags: review?(continuation) → review+
https://hg.mozilla.org/mozilla-central/rev/33f49f751b63
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla54
Please request Aurora/Beta approval on this when you get a chance.
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
Flags: needinfo?(bbirtles)
Attachment #8837443 - Flags: approval-mozilla-beta?
Attachment #8837443 - Flags: approval-mozilla-aurora?
Comment on attachment 8837443 [details] [diff] [review]
Patch

Fix a security issue. Aurora53+.
Attachment #8837443 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Comment on attachment 8837443 [details] [diff] [review]
Patch

fix UAF in SMIL, beta52+
Attachment #8837443 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Group: layout-core-security → core-security-release
Flags: qe-verify-
Whiteboard: [post-critsmash-triage]
Whiteboard: [post-critsmash-triage] → [post-critsmash-triage][adv-main52+]
Group: core-security-release
Product: Core → Core Graveyard
Component: Layout: Misc Code → Layout
Product: Core Graveyard → Core
You need to log in before you can comment on or make changes to this bug.