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
Please give this a sec rating too :)
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)
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.
Comment on attachment 8837295 [details] [diff] [review] fix v1 fix a possible UAF, aurora53+, beta52+
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
Whiteboard: [post-critsmash-triage] → [post-critsmash-triage][adv-main52+]
You need to log in before you can comment on or make changes to this bug.