Closed
Bug 1339566
Opened 8 years ago
Closed 8 years ago
Use-after-free in nsDocShell::CreateAboutBlankViewer
Categories
(Core :: Layout, defect)
Core
Layout
Tracking
()
People
(Reporter: mccr8, Assigned: dholbert)
References
Details
(Keywords: csectype-uaf, sec-moderate, Whiteboard: [post-critsmash-triage][adv-main52+])
Attachments
(1 file)
(deleted),
patch
|
mccr8
:
review+
jcristau
:
approval-mozilla-aurora+
jcristau
:
approval-mozilla-beta+
|
Details | Diff | Splinter Review |
AutoRestore<bool> creatingDocument(mCreatingDocument);
mCreatingDocument = true;
// mContentViewer->PermitUnload may release |this| docshell.
nsCOMPtr<nsIDocShell> kungFuDeathGrip(this);
The grip gets released before the AutoRestore.
Assignee | ||
Comment 1•8 years ago
|
||
(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
Assignee | ||
Comment 2•8 years ago
|
||
Attachment #8837295 -
Flags: review?(continuation)
Comment 3•8 years ago
|
||
Please give this a sec rating too :)
status-firefox51:
--- → affected
status-firefox52:
--- → affected
status-firefox53:
--- → affected
status-firefox54:
--- → affected
status-firefox-esr45:
--- → affected
status-firefox-esr52:
--- → affected
tracking-firefox51:
--- → ?
tracking-firefox52:
--- → ?
tracking-firefox53:
--- → ?
tracking-firefox54:
--- → ?
tracking-firefox-esr45:
--- → ?
tracking-firefox-esr52:
--- → ?
Reporter | ||
Updated•8 years ago
|
Attachment #8837295 -
Flags: review?(continuation) → review+
Reporter | ||
Comment 4•8 years ago
|
||
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.
Keywords: csectype-uaf,
sec-moderate
Assignee | ||
Comment 5•8 years ago
|
||
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")
Assignee | ||
Comment 6•8 years ago
|
||
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-
Assignee | ||
Updated•8 years ago
|
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla54
Comment 8•8 years ago
|
||
Please request Aurora/Beta approval on this when you get a chance.
tracking-firefox51:
? → ---
Flags: needinfo?(dholbert)
Assignee | ||
Comment 9•8 years ago
|
||
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?
Updated•8 years ago
|
Comment 10•8 years ago
|
||
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+
Assignee | ||
Comment 11•8 years ago
|
||
uplift |
Assignee | ||
Comment 12•8 years ago
|
||
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.
Comment 13•8 years ago
|
||
uplift |
Updated•8 years ago
|
Group: layout-core-security → core-security-release
Updated•8 years ago
|
Flags: qe-verify-
Whiteboard: [post-critsmash-triage]
Updated•8 years ago
|
Whiteboard: [post-critsmash-triage] → [post-critsmash-triage][adv-main52+]
Updated•7 years ago
|
Group: core-security-release
You need to log in
before you can comment on or make changes to this bug.
Description
•