Closed Bug 1666184 Opened 4 years ago Closed 3 years ago

Defer Loading Object in nsObjectLoadingContent

Categories

(Core :: DOM: Core & HTML, task, P2)

Product:
Core
Component:
DOM: Core & HTML
Type:
task

Tracking

()

Status:
RESOLVED FIXED
Milestone:
92 Branch
Tracking Flags:
Tracking Status
firefox-esr78 91+ fixed
firefox90 --- wontfix
firefox91 + fixed
firefox92 --- fixed

People

(Reporter: ckerschb, Assigned: edgar)

Details

(Keywords: csectype-priv-escalation, sec-high, Whiteboard: [sec-survey][adv-main91+r][adv-esr78.13+r])

Attachments

(1 file)

Bug 1666184 - Defer loading object when setting attribute; r=nika
48 bytes, text/x-phabricator-request
tjr
: approval-mozilla-beta+
tjr
: approval-mozilla-esr78+
tjr
: sec-approval+
Details | Review 
#01: doInvoke(NPObject*, void*, _NPVariant const*, unsigned int, bool, _NPVariant*) [dom/plugins/base/nsJSNPRuntime.cpp:698]
[task 2020-09-17T16:46:13.050Z] 16:46:13     INFO - GECKO(2988) | #02: mozilla::plugins::parent::_invokeDefault(_NPP*, NPObject*, _NPVariant const*, unsigned int, _NPVariant*) [dom/plugins/base/nsNPAPIPlugin.cpp:892]
[task 2020-09-17T16:46:13.052Z] 16:46:13     INFO - GECKO(2988) | #03: mozilla::plugins::PluginScriptableObjectParent::AnswerInvokeDefault(nsTArray<mozilla::plugins::Variant>&&, mozilla::plugins::Variant*, bool*) [dom/plugins/ipc/PluginScriptableObjectParent.cpp:868]
[task 2020-09-17T16:46:13.052Z] 16:46:13     INFO - GECKO(2988) | #04: mozilla::plugins::PPluginScriptableObjectParent::OnCallReceived(IPC::Message const&, IPC::Message*&) [s3:gecko-generated-sources-l1:a4faf643b98b34a0ad1fd0284761655a86e4330a7a4f9f7084e68f0e4a79fd9aec0ae40e93cdb8eba7e60f6022c7edc2fb3ad17e9e9e724161944361cb646d0c/ipc/ipdl/PPluginScriptableObjectParent.cpp::1068]
[task 2020-09-17T16:46:13.055Z] 16:46:13     INFO - GECKO(2988) | #05: mozilla::plugins::PPluginModuleParent::OnCallReceived(IPC::Message const&, IPC::Message*&) [s3:gecko-generated-sources-l1:f88a0ca3641b1a6563122ac32e7a56c00b071fa921f25d4dd5f1a5e98d836710d3ad74c51a3514a4327480fbd0f7d3a4ef1a720f589b51e56b5ac40f54002481/ipc/ipdl/PPluginModuleParent.cpp::1516]
[task 2020-09-17T16:46:13.055Z] 16:46:13     INFO - GECKO(2988) | #06: mozilla::ipc::MessageChannel::DispatchInterruptMessage(mozilla::ipc::ActorLifecycleProxy*, IPC::Message&&, unsigned long) [ipc/glue/MessageChannel.cpp:2182]
[task 2020-09-17T16:46:13.055Z] 16:46:13     INFO - GECKO(2988) | #07: mozilla::ipc::MessageChannel::Call(mozilla::UniquePtr<IPC::Message, mozilla::DefaultDelete<IPC::Message> >, IPC::Message*) [ipc/glue/MessageChannel.cpp:1772]
[task 2020-09-17T16:46:13.055Z] 16:46:13     INFO - GECKO(2988) | #08: mozilla::ipc::IProtocol::ChannelCall(IPC::Message*, IPC::Message*) [ipc/glue/ProtocolUtils.cpp:528]
[task 2020-09-17T16:46:13.055Z] 16:46:13     INFO - GECKO(2988) | #09: mozilla::plugins::PPluginInstanceParent::CallNPP_Destroy(short*) [s3:gecko-generated-sources-l1:3674a9628733dadfb6a1206e279a05a978e5d19663d6ea2fb41f76b8420076e695d66f7f377a4244fe354d114a262f2b75989881d96c3e6364081509508db670/ipc/ipdl/PPluginInstanceParent.cpp::1211]
[task 2020-09-17T16:46:13.055Z] 16:46:13     INFO - GECKO(2988) | #10: mozilla::plugins::PluginModuleParent::NPP_Destroy(_NPP*, _NPSavedData**) [dom/plugins/ipc/PluginModuleParent.cpp:1504]
[task 2020-09-17T16:46:13.058Z] 16:46:13     INFO - GECKO(2988) | #11: nsNPAPIPluginInstance::Stop() [dom/plugins/base/nsNPAPIPluginInstance.cpp:175]
[task 2020-09-17T16:46:13.059Z] 16:46:13     INFO - GECKO(2988) | #12: nsPluginHost::StopPluginInstance(nsNPAPIPluginInstance*) [dom/plugins/base/nsPluginHost.cpp:2406]
[task 2020-09-17T16:46:13.059Z] 16:46:13     INFO - GECKO(2988) | #13: nsObjectLoadingContent::DoStopPlugin(nsPluginInstanceOwner*) [dom/base/nsObjectLoadingContent.cpp:2946]
[task 2020-09-17T16:46:13.059Z] 16:46:13     INFO - GECKO(2988) | #14: nsObjectLoadingContent::StopPluginInstance() [dom/base/nsObjectLoadingContent.cpp:2998]
[task 2020-09-17T16:46:13.060Z] 16:46:13     INFO - GECKO(2988) | #15: nsObjectLoadingContent::LoadObject(bool, bool, nsIRequest*) [dom/base/nsObjectLoadingContent.cpp:1981]
[task 2020-09-17T16:46:13.060Z] 16:46:13     INFO - GECKO(2988) | #16: nsObjectLoadingContent::LoadObject(bool, bool) [dom/base/nsObjectLoadingContent.cpp:1881]
[task 2020-09-17T16:46:13.061Z] 16:46:13     INFO - GECKO(2988) | #17: mozilla::dom::HTMLEmbedElement::AfterMaybeChangeAttr(int, nsAtom*, bool) [dom/html/HTMLEmbedElement.cpp:157]
[task 2020-09-17T16:46:13.061Z] 16:46:13     INFO - GECKO(2988) | #18: mozilla::dom::HTMLEmbedElement::AfterSetAttr(int, nsAtom*, nsAttrValue const*, nsAttrValue const*, nsIPrincipal*, bool) [dom/html/HTMLEmbedElement.cpp:120]
[task 2020-09-17T16:46:13.062Z] 16:46:13     INFO - GECKO(2988) | #19: mozilla::dom::Element::SetAttrAndNotify(int, nsAtom*, nsAtom*, nsAttrValue const*, nsAttrValue&, nsIPrincipal*, unsigned char, bool, bool, bool, mozilla::dom::Document*, mozAutoDocUpdate const&) [dom/base/Element.cpp:2375]
[task 2020-09-17T16:46:13.062Z] 16:46:13     INFO - GECKO(2988) | #20: mozilla::dom::Element::SetAttr(int, nsAtom*, nsAtom*, nsTSubstring<char16_t> const&, nsIPrincipal*, bool) [dom/base/Element.cpp:2232]
[task 2020-09-17T16:46:13.063Z] 16:46:13     INFO - GECKO(2988) | #21: mozilla::dom::Element::SetAttribute(nsTSubstring<char16_t> const&, nsTSubstring<char16_t> const&, nsIPrincipal*, mozilla::ErrorResult&) [dom/base/Element.cpp:1305]
[task 2020-09-17T16:46:13.066Z] 16:46:13     INFO - GECKO(2988) | #22: mozilla::dom::Element_Binding::setAttribute(JSContext*, JS::Handle<JSObject*>, void*, JSJitMethodCallArgs const&) [s3:gecko-generated-sources-l1:9c0fb52ed8e32269411463a10e2eae7aa12ff3897bc1021f4d8b204001bae20b69ab83dba5848dfc8e839a7e2a157cb83d607c2b81e2559fccba8e88905872b4/dom/bindings/ElementBinding.cpp::1348]
[task 2020-09-17T16:46:13.067Z] 16:46:13     INFO - GECKO(2988) | #23: bool mozilla::dom::binding_detail::GenericMethod<mozilla::dom::binding_detail::NormalThisPolicy, mozilla::dom::binding_detail::ThrowExceptions>(JSContext*, unsigned int, JS::Value*) [dom/bindings/BindingUtils.cpp:3231]
[task 2020-09-17T16:46:13.067Z] 16:46:13     INFO - GECKO(2988) | #24: CallJSNative(JSContext*, bool (*)(JSContext*, unsigned int, JS::Value*), js::CallReason, JS::CallArgs const&) [js/src/vm/Interpreter.cpp:508]
[task 2020-09-17T16:46:13.068Z] 16:46:13     INFO - GECKO(2988) | #25: js::InternalCallOrConstruct(JSContext*, JS::CallArgs const&, js::MaybeConstruct, js::CallReason) [js/src/vm/Interpreter.cpp:600]
[task 2020-09-17T16:46:13.068Z] 16:46:13     INFO - GECKO(2988) | #26: InternalCall(JSContext*, js::AnyInvokeArgs const&, js::CallReason) [js/src/vm/Interpreter.cpp:665]
[task 2020-09-17T16:46:13.069Z] 16:46:13     INFO - GECKO(2988) | #27: Interpret(JSContext*, js::RunState&) [js/src/vm/Interpreter.cpp:3337]
[task 2020-09-17T16:46:13.069Z] 16:46:13     INFO - GECKO(2988) | #28: js::RunScript(JSContext*, js::RunState&) [js/src/vm/Interpreter.cpp:469]
[task 2020-09-17T16:46:13.070Z] 16:46:13     INFO - GECKO(2988) | #29: js::InternalCallOrConstruct(JSContext*, JS::CallArgs const&, js::MaybeConstruct, js::CallReason) [js/src/vm/Interpreter.cpp:637]
[task 2020-09-17T16:46:13.071Z] 16:46:13     INFO - GECKO(2988) | #30: InternalCall(JSContext*, js::AnyInvokeArgs const&, js::CallReason) [js/src/vm/Interpreter.cpp:665]
[task 2020-09-17T16:46:13.071Z] 16:46:13     INFO - GECKO(2988) | #31: js::Call(JSContext*, JS::Handle<JS::Value>, JS::Handle<JS::Value>, js::AnyInvokeArgs const&, JS::MutableHandle<JS::Value>, js::CallReason) [js/src/vm/Interpreter.cpp:682]
[task 2020-09-17T16:46:13.072Z] 16:46:13     INFO - GECKO(2988) | #32: JS::Call(JSContext*, JS::Handle<JS::Value>, JS::Handle<JS::Value>, JS::HandleValueArray const&, JS::MutableHandle<JS::Value>) [js/src/jsapi.cpp:2821]
[task 2020-09-17T16:46:13.073Z] 16:46:13     INFO - GECKO(2988) | #33: mozilla::dom::EventHandlerNonNull::Call(mozilla::dom::BindingCallContext&, JS::Handle<JS::Value>, mozilla::dom::Event&, JS::MutableHandle<JS::Value>, mozilla::ErrorResult&) [s3:gecko-generated-sources-l1:9f8f99d15fc940b7590146353491dabf93ce68dc0db37ac8dcc44ae02391da8d09637f6a00c96ad480abb3c8094ea4c38fc4b96607e24d1c3c291947923c72d9/dom/bindings/EventHandlerBinding.cpp::278]
[task 2020-09-17T16:46:13.073Z] 16:46:13     INFO - GECKO(2988) | #34: void mozilla::dom::EventHandlerNonNull::Call<nsCOMPtr<mozilla::dom::EventTarget> >(nsCOMPtr<mozilla::dom::EventTarget> const&, mozilla::dom::Event&, JS::MutableHandle<JS::Value>, mozilla::ErrorResult&, char const*, mozilla::dom::CallbackObject::ExceptionHandling, JS::Realm*) [s3:gecko-generated-sources-l1:5abc72777d30ad0ea7b8b3e1392cd7961bd73b8cd08e5ab209f3b42251def563ee9c9a7cea98a24d0fa9d867edfc0c0297c165b0d167458ac5a2af4b7a0caebe/dist/include/mozilla/dom/EventHandlerBinding.h::367]
[task 2020-09-17T16:46:13.075Z] 16:46:13     INFO - GECKO(2988) | #35: mozilla::JSEventHandler::HandleEvent(mozilla::dom::Event*) [dom/events/JSEventHandler.cpp:201]
[task 2020-09-17T16:46:13.075Z] 16:46:13     INFO - GECKO(2988) | #36: mozilla::EventListenerManager::HandleEventSubType(mozilla::EventListenerManager::Listener*, mozilla::dom::Event*, mozilla::dom::EventTarget*) [dom/events/EventListenerManager.cpp:1088]
[task 2020-09-17T16:46:13.076Z] 16:46:13     INFO - GECKO(2988) | #37: mozilla::EventListenerManager::HandleEventInternal(nsPresContext*, mozilla::WidgetEvent*, mozilla::dom::Event**, mozilla::dom::EventTarget*, nsEventStatus*, bool) [dom/events/EventListenerManager.cpp:1279]
[task 2020-09-17T16:46:13.076Z] 16:46:13     INFO - GECKO(2988) | #38: mozilla::EventTargetChainItem::HandleEvent(mozilla::EventChainPostVisitor&, mozilla::ELMCreationDetector&) [dom/events/EventDispatcher.cpp:359]
[task 2020-09-17T16:46:13.077Z] 16:46:13     INFO - GECKO(2988) | #39: mozilla::EventTargetChainItem::HandleEventTargetChain(nsTArray<mozilla::EventTargetChainItem>&, mozilla::EventChainPostVisitor&, mozilla::EventDispatchingCallback*, mozilla::ELMCreationDetector&) [dom/events/EventDispatcher.cpp:560]
[task 2020-09-17T16:46:13.078Z] 16:46:13     INFO - GECKO(2988) | #40: mozilla::EventDispatcher::Dispatch(nsISupports*, nsPresContext*, mozilla::WidgetEvent*, mozilla::dom::Event*, nsEventStatus*, mozilla::EventDispatchingCallback*, nsTArray<mozilla::dom::EventTarget*>*) [dom/events/EventDispatcher.cpp:1058]
[task 2020-09-17T16:46:13.079Z] 16:46:13     INFO - GECKO(2988) | #41: nsDocumentViewer::LoadComplete(nsresult) [layout/base/nsDocumentViewer.cpp:1097]
[task 2020-09-17T16:46:13.080Z] 16:46:13     INFO - GECKO(2988) | #42: nsDocShell::EndPageLoad(nsIWebProgress*, nsIChannel*, nsresult) [docshell/base/nsDocShell.cpp:6381]
[task 2020-09-17T16:46:13.080Z] 16:46:13     INFO - GECKO(2988) | #43: nsDocShell::OnStateChange(nsIWebProgress*, nsIRequest*, unsigned int, nsresult) [docshell/base/nsDocShell.cpp:5733]
[task 2020-09-17T16:46:13.081Z] 16:46:13     INFO - GECKO(2988) | #44: {virtual override thunk({offset(-448)}, nsDocShell::OnStateChange(nsIWebProgress*, nsIRequest*, unsigned int, nsresult))} [/builds/worker/workspace/build/application/firefox/libxul.so + 0x7daf64e]
[task 2020-09-17T16:46:13.082Z] 16:46:13     INFO - GECKO(2988) | #45: nsDocLoader::DoFireOnStateChange(nsIWebProgress*, nsIRequest*, int&, nsresult) [uriloader/base/nsDocLoader.cpp:1348]
[task 2020-09-17T16:46:13.083Z] 16:46:13     INFO - GECKO(2988) | #46: nsDocLoader::doStopDocumentLoad(nsIRequest*, nsresult) [uriloader/base/nsDocLoader.cpp:953]
[task 2020-09-17T16:46:13.083Z] 16:46:13     INFO - GECKO(2988) | #47: nsDocLoader::DocLoaderIsEmpty(bool, mozilla::Maybe<nsresult> const&) [uriloader/base/nsDocLoader.cpp:759]
[task 2020-09-17T16:46:13.085Z] 16:46:13     INFO - GECKO(2988) | #48: nsDocLoader::OnStopRequest(nsIRequest*, nsresult) [uriloader/base/nsDocLoader.cpp:640]
[task 2020-09-17T16:46:13.085Z] 16:46:13     INFO - GECKO(2988) | #49: {virtual override thunk({offset(-8)}, nsDocLoader::OnStopRequest(nsIRequest*, nsresult))} [/builds/worker/workspace/build/application/firefox/libxul.so + 0x4ddc263]
[task 2020-09-17T16:46:13.086Z] 16:46:13     INFO - GECKO(2988) | #50: mozilla::net::nsLoadGroup::NotifyRemovalObservers(nsIRequest*, nsresult) [netwerk/base/nsLoadGroup.cpp:615]
[task 2020-09-17T16:46:13.087Z] 16:46:13     INFO - GECKO(2988) | #51: mozilla::net::nsLoadGroup::RemoveRequest(nsIRequest*, nsISupports*, nsresult) [netwerk/base/nsLoadGroup.cpp:522]
[task 2020-09-17T16:46:13.088Z] 16:46:13     INFO - GECKO(2988) | #52: nsBaseChannel::OnStopRequest(nsIRequest*, nsresult) [netwerk/base/nsBaseChannel.cpp:859]
[task 2020-09-17T16:46:13.088Z] 16:46:13     INFO - GECKO(2988) | #53: {virtual override thunk({offset(-144)}, nsBaseChannel::OnStopRequest(nsIRequest*, nsresult))} [/builds/worker/workspace/build/application/firefox/libxul.so + 0x4317ace]

Marking this as security sensitive until we have resolved it because it might hint to a potential vulnerability in Firefox.

FWIW, dom/plugins/test/mochitest/test_src_url_change.html executes the code path.

The problem is that it's not safe to run script at the time we load the object. Applying the patch from 1181918 triggers the assertion and illustrates the problem.

Severity: -- → S2

Christoph, since you are assigned to the bug, what's the status here? Thanks!

Flags: needinfo?(ckerschb)

Overall there are still a lot of instances (see Dependencies of Bug 1181918) which we would need to update before we can arm the assertion in the AutoEntryConstructor. But this project got de-prioritized for now. However we landed an assertion that no new instances violating the rule can be introduced within Bug 1666419 (do not get confused by the name of the bug - it's only because it's a public bug).

Flags: needinfo?(ckerschb)
Assignee: ckerschb → nobody
Status: ASSIGNED → NEW

(In reply to Christoph Kerschbaumer [:ckerschb] from comment #4)

Overall there are still a lot of instances (see Dependencies of Bug 1181918) which we would need to update before we can arm the assertion in the AutoEntryConstructor. But this project got de-prioritized for now. However we landed an assertion that no new instances violating the rule can be introduced within Bug 1666419 (do not get confused by the name of the bug - it's only because it's a public bug).

Does this work on bug 1666419 then mean, we should consider to lower the "sec-*" rating here?

Flags: needinfo?(ckerschb)

(In reply to Jens Stutte [:jstutte] from comment #5)

Does this work on bug 1666419 then mean, we should consider to lower the "sec-*" rating here?

Yes, the sec rating here can be lowered in my opinion - Tom?

Flags: needinfo?(ckerschb) → needinfo?(tom)

Is this particular bug still valid given EOL of Flash?

I don't know the bug is valid given that Flash is EOL; but if it is then no, the security rating for this issue is still (probably) accurate. Bug 1666419 ensures we don't add new instances of this type of vulnerability, but each existing occurrence is a potential UAF. Unless we can conclude this isn't triggerable by content we shouldn't lower the sec rating. And even if we can conclude that, it would generally be better to fix the issue, and remove the exemption.

Of course now that Flash is EOL, this particular issue might be invalid now!

Flags: needinfo?(tom)

This bug probably still exists without Flash, as the nsObjectLoadingContent::LoadObject codepath still exists. We should probably move the calls to LoadObject here: https://searchfox.org/mozilla-central/rev/d7e344e956d9da2808ea33e1fe0f963ed10c142d/dom/html/HTMLObjectElement.cpp#157, and here: https://searchfox.org/mozilla-central/rev/d7e344e956d9da2808ea33e1fe0f963ed10c142d/dom/html/HTMLEmbedElement.cpp#139 behind a script runner.

Edgar, can you fix this please?

Flags: needinfo?(echen)
Assignee: nobody → echen
Flags: needinfo?(echen)

There's a r+ patch which didn't land and no activity in this bug for 2 weeks.
:edgar, could you have a look please?
For more information, please visit auto_nag documentation.

Flags: needinfo?(nika)
Flags: needinfo?(echen)

Nika, can you please help to land this while Edgar is on PTO? Thanks!

Comment on attachment 9225167 [details]
Bug 1666184 - Defer loading object when setting attribute; r=nika

Security Approval Request

  • How easily could an exploit be constructed based on the patch?: Should be not easy, after Flash is EOL, I am not able to not find a case that could cause a similiar crash.
  • Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?: No
  • Which older supported branches are affected by this flaw?: All
  • If not all supported branches, which bug introduced the flaw?: None
  • Do you have backports for the affected branches?: Yes
  • If not, how different, hard to create, and risky will they be?:
  • How likely is this patch to cause regressions; how much testing does it need?: Should be low, this patch dose not change the behaviour, and other element, like imageElement, also put loading behind a script runner for attribute changing. Try result also looks good.
Flags: needinfo?(echen)
Attachment #9225167 - Flags: sec-approval?
Flags: needinfo?(nika)

As we've built RC, this is going to sit until after the release

Changing the priority to p2 as the bug is tracked by a release manager for the current nightly.
See What Do You Triage for more information

Priority: P3 → P2

Comment on attachment 9225167 [details]
Bug 1666184 - Defer loading object when setting attribute; r=nika

Approved to land and uplift

Attachment #9225167 - Flags: sec-approval?
Attachment #9225167 - Flags: sec-approval+
Attachment #9225167 - Flags: approval-mozilla-esr91+
Attachment #9225167 - Flags: approval-mozilla-esr78+
Attachment #9225167 - Flags: approval-mozilla-beta+

Defer loading object when setting attribute; r=nika
https://hg.mozilla.org/integration/autoland/rev/7a4158a7fa28e40338f474060201f9591e96a92d
https://hg.mozilla.org/mozilla-central/rev/7a4158a7fa28

Group: dom-core-security → core-security-release
Status: NEW → RESOLVED
Closed: 3 years ago
status-firefox92: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → 92 Branch
Attachment #9225167 - Flags: approval-mozilla-esr91+
QA Whiteboard: [post-critsmash-triage]
Flags: qe-verify-

As part of a security bug pattern analysis, we are requesting your help with a high level analysis of this bug. It is our hope to develop static analysis (or potentially runtime/dynamic analysis) in the future to identify classes of bugs.

Please visit this google form to reply.

Flags: needinfo?(echen)
Whiteboard: [sec-survey]

(In reply to Release mgmt bot [:sylvestre / :calixte / :marco for bugbug] from comment #22)

Please visit this google form to reply.

Done.

Flags: needinfo?(echen)
Whiteboard: [sec-survey] → [sec-survey][adv-main90+r]
Whiteboard: [sec-survey][adv-main90+r] → [sec-survey][adv-main91+r]
Whiteboard: [sec-survey][adv-main91+r] → [sec-survey][adv-main91+r][adv-esr78.13+r]
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: