Closed Bug 1339566 Opened 6 years ago Closed 6 years ago

Use-after-free in nsDocShell::CreateAboutBlankViewer


(Core :: Layout, defect)

Not set



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


(Reporter: mccr8, Assigned: dholbert)



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


(1 file)

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:

I'll take this; seems trivial.
Assignee: nobody → dholbert
Attached 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 , 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:

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-
Closed: 6 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.