Crash in nsFrameLoader::GetOwnerDoc

RESOLVED FIXED in Firefox 53

Status

()

defect
--
critical
RESOLVED FIXED
2 years ago
4 months ago

People

(Reporter: philipp, Assigned: kats)

Tracking

(4 keywords)

53 Branch
mozilla55
Unspecified
Windows
Points:
---
Bug Flags:
qe-verify -

Firefox Tracking Flags

(firefox-esr45 unaffected, firefox52 unaffected, firefox-esr52 unaffected, firefox53+ fixed, firefox54+ fixed, firefox55+ fixed)

Details

(Whiteboard: [post-critsmash-triage], crash signature)

Attachments

(1 attachment)

This bug was filed from the Socorro interface and is 
report bp-eca3256c-ac2c-41b7-8b71-e11452170308.
=============================================================
Crashing Thread (0)
Frame 	Module 	Signature 	Source
0 	xul.dll 	nsFrameLoader::GetOwnerDoc() 	dom/base/nsFrameLoader.h:156
1 	xul.dll 	mozilla::layout::GetFrom(nsFrameLoader*) 	layout/ipc/RenderFrameParent.cpp:83
2 	xul.dll 	mozilla::layout::RenderFrameParent::GetTextureFactoryIdentifier(mozilla::layers::TextureFactoryIdentifier*) 	layout/ipc/RenderFrameParent.cpp:289
3 	xul.dll 	mozilla::dom::TabParent::InitRenderFrame() 	dom/ipc/TabParent.cpp:641
4 	xul.dll 	nsFrameLoader::TryRemoteBrowser() 	dom/base/nsFrameLoader.cpp:2941
5 	xul.dll 	nsFrameLoader::ShowRemoteFrame(mozilla::gfx::IntSizeTyped<mozilla::ScreenPixel> const&, nsSubDocumentFrame*) 	dom/base/nsFrameLoader.cpp:1225
6 	xul.dll 	nsFrameLoader::Show(int, int, int, int, nsSubDocumentFrame*) 	dom/base/nsFrameLoader.cpp:1095
7 	xul.dll 	nsSubDocumentFrame::ShowViewer() 	layout/generic/nsSubDocumentFrame.cpp:191
8 	xul.dll 	AsyncFrameInit::Run() 	layout/generic/nsSubDocumentFrame.cpp:91
9 	xul.dll 	nsContentUtils::RemoveScriptBlocker() 	dom/base/nsContentUtils.cpp:5225
10 	xul.dll 	nsDocument::EndUpdate(unsigned int) 	dom/base/nsDocument.cpp:4890
11 	xul.dll 	mozilla::dom::XULDocument::EndUpdate(unsigned int) 	dom/xul/XULDocument.cpp:3186
12 	xul.dll 	mozAutoDocUpdate::~mozAutoDocUpdate() 	dom/base/mozAutoDocUpdate.h:40
13 	xul.dll 	nsINode::ReplaceOrInsertBefore(bool, nsINode*, nsINode*, mozilla::ErrorResult&) 	dom/base/nsINode.cpp:2524
14 	xul.dll 	mozilla::dom::NodeBinding::appendChild 	obj-firefox/dom/bindings/NodeBinding.cpp:855
15 	xul.dll 	js::InternalCallOrConstruct(JSContext*, JS::CallArgs const&, js::MaybeConstruct) 	js/src/vm/Interpreter.cpp:460
16 	xul.dll 	InternalCall 	js/src/vm/Interpreter.cpp:505
17 	xul.dll 	Interpret 	js/src/vm/Interpreter.cpp:2956
18 	xul.dll 	js::RunScript(JSContext*, js::RunState&) 	js/src/vm/Interpreter.cpp:406
19 	xul.dll 	js::InternalCallOrConstruct(JSContext*, JS::CallArgs const&, js::MaybeConstruct) 	js/src/vm/Interpreter.cpp:478
20 	xul.dll 	js::Call(JSContext*, JS::Handle<JS::Value>, JS::Handle<JS::Value>, js::AnyInvokeArgs const&, JS::MutableHandle<JS::Value>) 	js/src/vm/Interpreter.cpp:524
21 	xul.dll 	JS::Call(JSContext*, JS::Handle<JS::Value>, JS::Handle<JS::Value>, JS::HandleValueArray const&, JS::MutableHandle<JS::Value>) 	js/src/jsapi.cpp:2847
22 	xul.dll 	JS::StructGCPolicy<JS::GCVector<jsid, 8, js::TempAllocPolicy> >::trace(JSTracer*, JS::GCVector<jsid, 8, js::TempAllocPolicy>*, char const*) 	obj-firefox/dist/include/js/GCPolicyAPI.h:87
23 	xul.dll 	mozilla::dom::EventBinding::Wrap<mozilla::dom::Event>(JSContext*, mozilla::dom::Event*, JS::Handle<JSObject*>) 	obj-firefox/dist/include/mozilla/dom/EventBinding.h:97
24 	xul.dll 	mozilla::dom::EventListener::HandleEvent(JSContext*, JS::Handle<JS::Value>, mozilla::dom::Event&, mozilla::ErrorResult&) 	obj-firefox/dom/bindings/EventListenerBinding.cpp:47
25 	xul.dll 	mozilla::EventListenerManager::HandleEventSubType(mozilla::EventListenerManager::Listener*, nsIDOMEvent*, mozilla::dom::EventTarget*) 	dom/events/EventListenerManager.cpp:1132

it looks like this may be regressing in 53 (maybe related to bug 1331509?) - so far there are only a handful of those crashes, but it's still early days for 53 on beta. 

marking this as security sensitive due to the uaf crashing address...
I saw one UAF in 50.1, but the frequency did increase starting in Fx53.
Flags: needinfo?(bugmail)
Thanks, I'll look into this.
Assignee: nobody → bugmail
Flags: needinfo?(bugmail)
First possibility I see is that at [1] the return value is null because the function fails to send properly. However we ignore that return value and continue using `renderFrame` which I guess might be freed at that point? I'm not sure where it gets freed in that scenario. However that would certainly explain the UAF.

Prior to my change at [2] we wouldn't do anything with that `renderFrame`, hence no UAF. In the old code, when we needed to call GetTextureFactoryIdentifier, we would re-obtain the renderframe pointer via GetRenderFrame() [3], which would return null in this scenario, and we had a null guard to catch it. Hence also no UAF.

[1] http://searchfox.org/mozilla-central/rev/7cb75d87753de9103253e34bc85592e26378f506/dom/ipc/TabParent.cpp#656
[2] https://hg.mozilla.org/integration/autoland/rev/edcd2bd8340d#l4.12
[3] https://hg.mozilla.org/integration/autoland/rev/edcd2bd8340d#l4.33
Group: core-security → dom-core-security
Tracking for 53 and 54 since this is rated sec-high.
poke to review.  Kats - we'll want uplift requests.
Flags: needinfo?(dvander)
Flags: needinfo?(bugmail)
Yup, just waiting on dvander.
Flags: needinfo?(bugmail)
Comment on attachment 8845657 [details] [diff] [review]
Patch

Review of attachment 8845657 [details] [diff] [review]:
-----------------------------------------------------------------

I never liked how IPDL immediately destroys "expert-mode" actors like this. Maybe we should change that.
Attachment #8845657 - Flags: review?(dvander) → review+
Comment on attachment 8845657 [details] [diff] [review]
Patch

[Security approval request comment]
How easily could an exploit be constructed based on the patch?
- Not very easily, I think

Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?
- There's no comments, check-in comment, or tests.

Which older supported branches are affected by this flaw?
- 53 and 54

If not all supported branches, which bug introduced the flaw?
- 53

Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be?
- This patch should apply pretty easily to the affected branches.

How likely is this patch to cause regressions; how much testing does it need?
- Not likely to cause regressions, but I'm also not 100% sure it will fix the problem. It certainly seems to fit the symptoms but I've been wrong about these kinds of bugs before.
Attachment #8845657 - Flags: sec-approval?
Comment on attachment 8845657 [details] [diff] [review]
Patch

Approval Request Comment
[Feature/Bug causing the regression]: bug 1331509
[User impact if declined]: crash in some edge cases where an IPDL message fails
[Is this code covered by automated tests?]: yes
[Has the fix been verified in Nightly?]: no, hasn't landed yet
[Needs manual test from QE? If yes, steps to reproduce]: no STR
[List of other uplifts needed for the feature/fix]: none
[Is the change risky?]: not really
[Why is the change risky/not risky?]: pretty small localized change, not much that it can do wrong
[String changes made/needed]: none
Attachment #8845657 - Flags: approval-mozilla-beta?
Attachment #8845657 - Flags: approval-mozilla-aurora?
Comment on attachment 8845657 [details] [diff] [review]
Patch

Giving approvals.
Attachment #8845657 - Flags: sec-approval?
Attachment #8845657 - Flags: sec-approval+
Attachment #8845657 - Flags: approval-mozilla-beta?
Attachment #8845657 - Flags: approval-mozilla-beta+
Attachment #8845657 - Flags: approval-mozilla-aurora?
Attachment #8845657 - Flags: approval-mozilla-aurora+
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Group: dom-core-security → core-security-release
Looking at crash-stats, I see crashes in 53.0b1 and 53.0b2 but nothing in b3, b4, or b5. Since b3 is the first beta this patch made it into, I think we can conclude the patch fixed the bug.
Flags: qe-verify-
Whiteboard: [post-critsmash-triage]
Group: core-security-release
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.