Closed Bug 1339591 Opened 3 years ago Closed 3 years ago

Possible UAFs with AutoRestore in SMIL code

Categories

(Core :: Layout, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla54
Tracking Status
firefox-esr45 --- wontfix
firefox51 --- wontfix
firefox52 + fixed
firefox-esr52 52+ fixed
firefox53 + fixed
firefox54 + fixed

People

(Reporter: mccr8, Assigned: birtles)

Details

(Keywords: csectype-uaf, sec-moderate, Whiteboard: [post-critsmash-triage][adv-main52+])

Attachments

(1 file)

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.)
Attached 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.