Use-after-free in nsDocShell::CreateAboutBlankViewer

RESOLVED FIXED in Firefox 52

Status

()

defect
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: mccr8, Assigned: dholbert)

Tracking

({csectype-uaf, sec-moderate})

unspecified
mozilla54
Points:
---
Bug Flags:
in-testsuite -
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)

AutoRestore<bool> creatingDocument(mCreatingDocument);
  mCreatingDocument = true;

  // mContentViewer->PermitUnload may release |this| docshell.
  nsCOMPtr<nsIDocShell> kungFuDeathGrip(this);

The grip gets released before the AutoRestore.
(and the AutoRestore is holding a pointer to this->mCreatingDocument, which means it will stomp on potentially-freed memory when it goes out of scope & tries to do its AutoRestore value-restoration)

Source link:
http://searchfox.org/mozilla-central/rev/ac8a72f2d5204ced2004c93a9c193430c63a6a71/docshell/base/nsDocShell.cpp#8026

I'll take this; seems trivial.
Assignee: nobody → dholbert
Posted patch fix v1Splinter Review
Attachment #8837295 - Flags: review?(continuation)
Attachment #8837295 - Flags: review?(continuation) → review+
I'm just going to mark this sec-moderate. I think almost no code runs between the destructor and the UAF, so it seems benign.
I'll land this tomorrow, then, unless you think I shouldn't (e.g. if there's another similar bug that would become more discoverable after this points an arrow at AutoRestore).

(Per https://wiki.mozilla.org/Security/Bug_Approval_Process , no sec-approval is needed if "the bug has a sec-low, sec-moderate, sec-other, or sec-want rating")
Per bug 1338772 comment 15, sounds like we're good to go. Landed:
https://hg.mozilla.org/integration/mozilla-inbound/rev/0f1afbc59a1cdd3012f1309bc64c0effaa9cae34

Also, marking "in-testsuite-" since we don't have a testcase & we're not aware that testcases could actually trigger bad behavior here, per comment 4 (and this issue was discovered via code inspection)
Flags: in-testsuite-
https://hg.mozilla.org/mozilla-central/rev/0f1afbc59a1c
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla54
Please request Aurora/Beta approval on this when you get a chance.
Comment on attachment 8837295 [details] [diff] [review]
fix v1

Approval Request Comment
[Feature/Bug causing the regression]: bug 1031303 (which added the AutoRestore here)
[User impact if declined]: This fix addresses a potential security issue -- there's a possible use-after-free being fixed here, though we're not aware of any way to get up to mischief in this case, per comment 4.  So: user impact if declined is: users would remain exposed to a small security risk.
[Is this code covered by automated tests?]: I think so. This is in our about:blank setup code, which gets exercised on every pageload (since we start pages at about:blank before loading their actual contents).
[Has the fix been verified in Nightly?]: Yes. (It's landed on Nightly and hasn't caused any problems. "Verified" is a bit of a silly concept here, since we don't have a testcase -- this was discovered via code inspection -- and we don't know of any way for this to actually cause trouble. So this is just fixing a theoretical problem.)
[Needs manual test from QE? If yes, steps to reproduce]: Nope.
[List of other uplifts needed for the feature/fix]: None.
[Is the change risky?]: No.
[Why is the change risky/not risky?]: Extremely small tweak - just keeping an object alive slightly longer, so that we tweak its bool before it's potentially-destroyed instead of after it's potentially-destroyed.
[String changes made/needed]: None.
Flags: needinfo?(dholbert)
Attachment #8837295 - Flags: approval-mozilla-beta?
Attachment #8837295 - Flags: approval-mozilla-aurora?
Comment on attachment 8837295 [details] [diff] [review]
fix v1

fix a possible UAF, aurora53+, beta52+
Attachment #8837295 - Flags: approval-mozilla-beta?
Attachment #8837295 - Flags: approval-mozilla-beta+
Attachment #8837295 - Flags: approval-mozilla-aurora?
Attachment #8837295 - Flags: approval-mozilla-aurora+
ESR52 should get this as well (and IIUC doesn't need separate approval beyond beta52+). 

Based on recent treeherder, looks like RyanVM is merging everything from beta52 to esr52 in periodic large batches, so I assume this will make it over to esr52 that way.
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
You need to log in before you can comment on or make changes to this bug.