Closed Bug 1535612 Opened 5 years ago Closed 5 years ago

SUMMARY: AddressSanitizer: heap-use-after-free /builds/worker/workspace/build/src/obj-firefox/dist/include/nsWrapperCache.h:162:12 in GetWrapperMaybeDead

Categories

(Core :: CSS Parsing and Computation, defect, P1)

Product:
Core
Component:
CSS Parsing and Computation
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla68
Tracking Flags:
Tracking Status
firefox-esr60 67+ fixed
firefox65 --- wontfix
firefox66 - wontfix
firefox67 + fixed
firefox68 + fixed

People

(Reporter: jkratzer, Assigned: emilio)

References

(Blocks 2 open bugs)

Details

(4 keywords, Whiteboard: [adv-main67+][adv-esr60.7+])

Attachments

(4 files)

testcase.html
841 bytes, text/html
Details
test-case that shows the underlying correctness issue
244 bytes, text/html
Details
Bug 1535612 - CSSKeyframeList::RemoveRule should clear parent references when removed. r=heycam
47 bytes, text/x-phabricator-request
pascalc
: approval-mozilla-beta+
RyanVM
: approval-mozilla-release-
abillings
: sec-approval+
Details | Review
ESR rebase.
1.03 KB, patch
lizzard
: approval-mozilla-esr60+
Details | Diff | Splinter Review

Testcase found while fuzzing mozilla-central rev 4d62ab0e31fd.

A build with --enable-fuzzing is required in order to to reproduce this issue.

==8893==ERROR: AddressSanitizer: heap-use-after-free on address 0x6080000dbfb0 at pc 0x7f2dce91e3bb bp 0x7ffee06e6f10 sp 0x7ffee06e6f08
READ of size 8 at 0x6080000dbfb0 thread T0 (file:// Content)
#0 0x7f2dce91e3ba in GetWrapperMaybeDead /builds/worker/workspace/build/src/obj-firefox/dist/include/nsWrapperCache.h:162:12
#1 0x7f2dce91e3ba in GetWrapperPreserveColor /builds/worker/workspace/build/src/obj-firefox/dist/include/nsWrapperCacheInlines.h:14
#2 0x7f2dce91e3ba in nsWrapperCache::GetWrapper() const /builds/worker/workspace/build/src/obj-firefox/dist/include/nsWrapperCacheInlines.h:27
#3 0x7f2dd478a39c in DoGetOrCreateDOMReflector<mozilla::css::Rule, mozilla::dom::binding_detail::eWrapIntoContextCompartment> /builds/worker/workspace/build/src/obj-firefox/dist/include/mozilla/dom/BindingUtils.h:1051:26
#4 0x7f2dd478a39c in GetOrCreateDOMReflector<mozilla::css::Rule> /builds/worker/workspace/build/src/obj-firefox/dist/include/mozilla/dom/BindingUtils.h:1126
#5 0x7f2dd478a39c in mozilla::dom::CSSRule_Binding::get_parentRule(JSContext*, JS::Handle<JSObject*>, mozilla::css::Rule*, JSJitGetterCallArgs) /builds/worker/workspace/build/src/obj-firefox/dom/bindings/CSSRuleBinding.cpp:146
#6 0x7f2dd604fc50 in bool mozilla::dom::binding_detail::GenericGetter<mozilla::dom::binding_detail::NormalThisPolicy, mozilla::dom::binding_detail::ThrowExceptions>(JSContext*, unsigned int, JS::Value*) /builds/worker/workspace/build/src/dom/bindings/BindingUtils.cpp:3040:13
#7 0x7f2ddd77fac7 in CallJSNative /builds/worker/workspace/build/src/js/src/vm/Interpreter.cpp:442:13
#8 0x7f2ddd77fac7 in js::InternalCallOrConstruct(JSContext*, JS::CallArgs const&, js::MaybeConstruct) /builds/worker/workspace/build/src/js/src/vm/Interpreter.cpp:534
#9 0x7f2ddd784380 in InternalCall /builds/worker/workspace/build/src/js/src/vm/Interpreter.cpp:589:10
#10 0x7f2ddd784380 in Call /builds/worker/workspace/build/src/js/src/vm/Interpreter.cpp:605
#11 0x7f2ddd784380 in js::CallGetter(JSContext*, JS::Handle<JS::Value>, JS::Handle<JS::Value>, JS::MutableHandle<JS::Value>) /builds/worker/workspace/build/src/js/src/vm/Interpreter.cpp:729
#12 0x7f2ddddba2fa in CallGetter /builds/worker/workspace/build/src/js/src/vm/NativeObject.cpp:2240:12
#13 0x7f2ddddba2fa in GetExistingProperty<js::CanGC> /builds/worker/workspace/build/src/js/src/vm/NativeObject.cpp:2292
#14 0x7f2ddddba2fa in NativeGetPropertyInline<js::CanGC> /builds/worker/workspace/build/src/js/src/vm/NativeObject.cpp:2541
#15 0x7f2ddddba2fa in js::NativeGetProperty(JSContext*, JS::Handle<js::NativeObject*>, JS::Handle<JS::Value>, JS::Handle<JS::PropertyKey>, JS::MutableHandle<JS::Value>) /builds/worker/workspace/build/src/js/src/vm/NativeObject.cpp:2578
#16 0x7f2ddd78ca67 in GetProperty /builds/worker/workspace/build/src/js/src/vm/ObjectOperations-inl.h:117:10
#17 0x7f2ddd78ca67 in GetProperty /builds/worker/workspace/build/src/js/src/vm/ObjectOperations-inl.h:124
#18 0x7f2ddd78ca67 in js::GetProperty(JSContext*, JS::Handle<JS::Value>, JS::Handle<js::PropertyName*>, JS::MutableHandle<JS::Value>) /builds/worker/workspace/build/src/js/src/vm/Interpreter.cpp:4486
#19 0x7f2ddd7686bb in GetPropertyOperation /builds/worker/workspace/build/src/js/src/vm/Interpreter.cpp:215:10
#20 0x7f2ddd7686bb in Interpret(JSContext*, js::RunState&) /builds/worker/workspace/build/src/js/src/vm/Interpreter.cpp:2762
#21 0x7f2ddd74a5b8 in js::RunScript(JSContext*, js::RunState&) /builds/worker/workspace/build/src/js/src/vm/Interpreter.cpp:422:10
#22 0x7f2ddd780436 in js::InternalCallOrConstruct(JSContext*, JS::CallArgs const&, js::MaybeConstruct) /builds/worker/workspace/build/src/js/src/vm/Interpreter.cpp:562:13
#23 0x7f2ddd782082 in js::Call(JSContext*, JS::Handle<JS::Value>, JS::Handle<JS::Value>, js::AnyInvokeArgs const&, JS::MutableHandle<JS::Value>) /builds/worker/workspace/build/src/js/src/vm/Interpreter.cpp:605:8
#24 0x7f2dde393da9 in JS::Call(JSContext*, JS::Handle<JS::Value>, JS::Handle<JS::Value>, JS::HandleValueArray const&, JS::MutableHandle<JS::Value>) /builds/worker/workspace/build/src/js/src/jsapi.cpp:2623:10
#25 0x7f2dd5870044 in mozilla::dom::Function::Call(JSContext*, JS::Handle<JS::Value>, nsTArray<JS::Value> const&, JS::MutableHandle<JS::Value>, mozilla::ErrorResult&) /builds/worker/workspace/build/src/obj-firefox/dom/bindings/FunctionBinding.cpp:41:8
#26 0x7f2dd29d3dc9 in void mozilla::dom::Function::Call<nsCOMPtr<nsISupports> >(nsCOMPtr<nsISupports> const&, nsTArray<JS::Value> const&, JS::MutableHandle<JS::Value>, mozilla::ErrorResult&, char const*, mozilla::dom::CallbackObject::ExceptionHandling, JS::Realm*) /builds/worker/workspace/build/src/obj-firefox/dist/include/mozilla/dom/FunctionBinding.h:73:12
#27 0x7f2dd29d21f2 in nsGlobalWindowInner::RunTimeoutHandler(mozilla::dom::Timeout*, nsIScriptContext*) /builds/worker/workspace/build/src/dom/base/nsGlobalWindowInner.cpp:5635:17
#28 0x7f2dd2e142e1 in mozilla::dom::TimeoutManager::RunTimeout(mozilla::TimeStamp const&, mozilla::TimeStamp const&, bool) /builds/worker/workspace/build/src/dom/base/TimeoutManager.cpp:980:44
#29 0x7f2dd2e12bbf in mozilla::dom::TimeoutExecutor::MaybeExecute() /builds/worker/workspace/build/src/dom/base/TimeoutExecutor.cpp:177:11
#30 0x7f2dd2e17c5c in Notify /builds/worker/workspace/build/src/dom/base/TimeoutExecutor.cpp:241:5
#31 0x7f2dd2e17c5c in non-virtual thunk to mozilla::dom::TimeoutExecutor::Notify(nsITimer*) /builds/worker/workspace/build/src/dom/base/TimeoutExecutor.cpp
#32 0x7f2dce8d1235 in nsTimerImpl::Fire(int) /builds/worker/workspace/build/src/xpcom/threads/nsTimerImpl.cpp:562:40
#33 0x7f2dce8d0665 in nsTimerEvent::Run() /builds/worker/workspace/build/src/xpcom/threads/TimerThread.cpp:260:11
#34 0x7f2dce90eab2 in mozilla::ThrottledEventQueue::Inner::ExecuteRunnable() /builds/worker/workspace/build/src/xpcom/threads/ThrottledEventQueue.cpp:243:22
#35 0x7f2dce907717 in mozilla::ThrottledEventQueue::Inner::Executor::Run() /builds/worker/workspace/build/src/xpcom/threads/ThrottledEventQueue.cpp:80:15
#36 0x7f2dce8a9595 in mozilla::SchedulerGroup::Runnable::Run() /builds/worker/workspace/build/src/xpcom/threads/SchedulerGroup.cpp:295:32
#37 0x7f2dce8e8c41 in nsThread::ProcessNextEvent(bool, bool*) /builds/worker/workspace/build/src/xpcom/threads/nsThread.cpp:1179:14
#38 0x7f2dce8f104d in NS_ProcessNextEvent(nsIThread*, bool) /builds/worker/workspace/build/src/xpcom/threads/nsThreadUtils.cpp:482:10
#39 0x7f2dcfba07f4 in mozilla::ipc::MessagePump::Run(base::MessagePump::Delegate*) /builds/worker/workspace/build/src/ipc/glue/MessagePump.cpp:110:5
#40 0x7f2dcfa76b4e in RunInternal /builds/worker/workspace/build/src/ipc/chromium/src/base/message_loop.cc:315:10
#41 0x7f2dcfa76b4e in RunHandler /builds/worker/workspace/build/src/ipc/chromium/src/base/message_loop.cc:308
#42 0x7f2dcfa76b4e in MessageLoop::Run() /builds/worker/workspace/build/src/ipc/chromium/src/base/message_loop.cc:290
#43 0x7f2dd8edc0b3 in nsBaseAppShell::Run() /builds/worker/workspace/build/src/widget/nsBaseAppShell.cpp:137:27
#44 0x7f2ddd4a294e in XRE_RunAppShell() /builds/worker/workspace/build/src/toolkit/xre/nsEmbedFunctions.cpp:933:20
#45 0x7f2dcfa76b4e in RunInternal /builds/worker/workspace/build/src/ipc/chromium/src/base/message_loop.cc:315:10
#46 0x7f2dcfa76b4e in RunHandler /builds/worker/workspace/build/src/ipc/chromium/src/base/message_loop.cc:308
#47 0x7f2dcfa76b4e in MessageLoop::Run() /builds/worker/workspace/build/src/ipc/chromium/src/base/message_loop.cc:290
#48 0x7f2ddd4a1adc in XRE_InitChildProcess(int, char**, XREChildData const*) /builds/worker/workspace/build/src/toolkit/xre/nsEmbedFunctions.cpp:771:34
#49 0x560025c1c834 in content_process_main /builds/worker/workspace/build/src/browser/app/../../ipc/contentproc/plugin-container.cpp:56:28
#50 0x560025c1c834 in main /builds/worker/workspace/build/src/browser/app/nsBrowserApp.cpp:265
#51 0x7f2df2133b96 in __libc_start_main /build/glibc-OTsEL5/glibc-2.27/csu/../csu/libc-start.c:310

0x6080000dbfb0 is located 16 bytes inside of 88-byte region [0x6080000dbfa0,0x6080000dbff8)
freed by thread T0 (file:// Content) here:
#0 0x560025be99e2 in free /builds/worker/workspace/moz-toolchain/src/llvm/projects/compiler-rt/lib/asan/asan_malloc_linux.cc:124:3
#1 0x7f2dce6db058 in MaybeKillObject /builds/worker/workspace/build/src/xpcom/base/nsCycleCollector.cpp:2428:29
#2 0x7f2dce6db058 in SnowWhiteKiller::Visit(nsPurpleBuffer&, nsPurpleBufferEntry*) /builds/worker/workspace/build/src/xpcom/base/nsCycleCollector.cpp:2457
#3 0x7f2dce6af3b5 in void nsPurpleBuffer::VisitEntries<SnowWhiteKiller>(SnowWhiteKiller&) /builds/worker/workspace/build/src/xpcom/base/nsCycleCollector.cpp:941:23
#4 0x7f2dce6b0b98 in nsCycleCollector::FreeSnowWhiteWithBudget(js::SliceBudget&) /builds/worker/workspace/build/src/xpcom/base/nsCycleCollector.cpp:2622:14
#5 0x7f2dd0d00d09 in AsyncFreeSnowWhite::Run() /builds/worker/workspace/build/src/js/xpconnect/src/XPCJSRuntime.cpp:142:9
#6 0x7f2dce90c772 in IdleRunnableWrapper::Run() /builds/worker/workspace/build/src/xpcom/threads/nsThreadUtils.cpp:327:22
#7 0x7f2dce8e8c41 in nsThread::ProcessNextEvent(bool, bool*) /builds/worker/workspace/build/src/xpcom/threads/nsThread.cpp:1179:14
#8 0x7f2dce8f104d in NS_ProcessNextEvent(nsIThread*, bool) /builds/worker/workspace/build/src/xpcom/threads/nsThreadUtils.cpp:482:10
#9 0x7f2dcfba07ff in mozilla::ipc::MessagePump::Run(base::MessagePump::Delegate*) /builds/worker/workspace/build/src/ipc/glue/MessagePump.cpp:88:21
#10 0x7f2dcfa76b4e in RunInternal /builds/worker/workspace/build/src/ipc/chromium/src/base/message_loop.cc:315:10
#11 0x7f2dcfa76b4e in RunHandler /builds/worker/workspace/build/src/ipc/chromium/src/base/message_loop.cc:308
#12 0x7f2dcfa76b4e in MessageLoop::Run() /builds/worker/workspace/build/src/ipc/chromium/src/base/message_loop.cc:290
#13 0x7f2dd8edc0b3 in nsBaseAppShell::Run() /builds/worker/workspace/build/src/widget/nsBaseAppShell.cpp:137:27
#14 0x7f2ddd4a294e in XRE_RunAppShell() /builds/worker/workspace/build/src/toolkit/xre/nsEmbedFunctions.cpp:933:20
#15 0x7f2dcfa76b4e in RunInternal /builds/worker/workspace/build/src/ipc/chromium/src/base/message_loop.cc:315:10
#16 0x7f2dcfa76b4e in RunHandler /builds/worker/workspace/build/src/ipc/chromium/src/base/message_loop.cc:308
#17 0x7f2dcfa76b4e in MessageLoop::Run() /builds/worker/workspace/build/src/ipc/chromium/src/base/message_loop.cc:290
#18 0x7f2ddd4a1adc in XRE_InitChildProcess(int, char**, XREChildData const*) /builds/worker/workspace/build/src/toolkit/xre/nsEmbedFunctions.cpp:771:34
#19 0x560025c1c834 in content_process_main /builds/worker/workspace/build/src/browser/app/../../ipc/contentproc/plugin-container.cpp:56:28
#20 0x560025c1c834 in main /builds/worker/workspace/build/src/browser/app/nsBrowserApp.cpp:265
#21 0x7f2df2133b96 in __libc_start_main /build/glibc-OTsEL5/glibc-2.27/csu/../csu/libc-start.c:310

previously allocated by thread T0 (file:// Content) here:
#0 0x560025be9d63 in __interceptor_malloc /builds/worker/workspace/moz-toolchain/src/llvm/projects/compiler-rt/lib/asan/asan_malloc_linux.cc:146:3
#1 0x560025c1e5fd in moz_xmalloc /builds/worker/workspace/build/src/memory/mozalloc/mozalloc.cpp:68:15
#2 0x7f2dd9494fad in operator new /builds/worker/workspace/build/src/obj-firefox/dist/include/mozilla/mozalloc.h:131:10
#3 0x7f2dd9494fad in mozilla::ServoCSSRuleList::GetRule(unsigned int) /builds/worker/workspace/build/src/layout/style/ServoCSSRuleList.cpp:77
#4 0x7f2dd94b525b in InsertRuleInternal /builds/worker/workspace/build/src/layout/style/StyleSheet.cpp:1121:32
#5 0x7f2dd94b525b in mozilla::StyleSheet::InsertRule(nsTSubstring<char16_t> const&, unsigned int, nsIPrincipal&, mozilla::ErrorResult&) /builds/worker/workspace/build/src/layout/style/StyleSheet.cpp:485
#6 0x7f2dd47abfaa in mozilla::dom::CSSStyleSheet_Binding::insertRule(JSContext*, JS::Handle<JSObject*>, mozilla::StyleSheet*, JSJitMethodCallArgs const&) /builds/worker/workspace/build/src/obj-firefox/dom/bindings/CSSStyleSheetBinding.cpp:210:25
#7 0x7f2dd6058ec1 in bool mozilla::dom::binding_detail::GenericMethod<mozilla::dom::binding_detail::NormalThisPolicy, mozilla::dom::binding_detail::ThrowExceptions>(JSContext*, unsigned int, JS::Value*) /builds/worker/workspace/build/src/dom/bindings/BindingUtils.cpp:3144:13
#8 0x7f2ddd77fac7 in CallJSNative /builds/worker/workspace/build/src/js/src/vm/Interpreter.cpp:442:13
#9 0x7f2ddd77fac7 in js::InternalCallOrConstruct(JSContext*, JS::CallArgs const&, js::MaybeConstruct) /builds/worker/workspace/build/src/js/src/vm/Interpreter.cpp:534
#10 0x7f2ddd7671c7 in CallFromStack /builds/worker/workspace/build/src/js/src/vm/Interpreter.cpp:593:10
#11 0x7f2ddd7671c7 in Interpret(JSContext*, js::RunState&) /builds/worker/workspace/build/src/js/src/vm/Interpreter.cpp:3075
#12 0x7f2ddd74a5b8 in js::RunScript(JSContext*, js::RunState&) /builds/worker/workspace/build/src/js/src/vm/Interpreter.cpp:422:10
#13 0x7f2ddd780436 in js::InternalCallOrConstruct(JSContext*, JS::CallArgs const&, js::MaybeConstruct) /builds/worker/workspace/build/src/js/src/vm/Interpreter.cpp:562:13
#14 0x7f2ddd782082 in js::Call(JSContext*, JS::Handle<JS::Value>, JS::Handle<JS::Value>, js::AnyInvokeArgs const&, JS::MutableHandle<JS::Value>) /builds/worker/workspace/build/src/js/src/vm/Interpreter.cpp:605:8
#15 0x7f2dde393da9 in JS::Call(JSContext*, JS::Handle<JS::Value>, JS::Handle<JS::Value>, JS::HandleValueArray const&, JS::MutableHandle<JS::Value>) /builds/worker/workspace/build/src/js/src/jsapi.cpp:2623:10
#16 0x7f2dd56634a9 in mozilla::dom::EventListener::HandleEvent(JSContext*, JS::Handle<JS::Value>, mozilla::dom::Event&, mozilla::ErrorResult&) /builds/worker/workspace/build/src/obj-firefox/dom/bindings/EventListenerBinding.cpp:52:8
#17 0x7f2dd68d14e2 in HandleEvent<mozilla::dom::EventTarget > /builds/worker/workspace/build/src/obj-firefox/dist/include/mozilla/dom/EventListenerBinding.h:66:12
#18 0x7f2dd68d14e2 in mozilla::EventListenerManager::HandleEventSubType(mozilla::EventListenerManager::Listener, mozilla::dom::Event*, mozilla::dom::EventTarget*) /builds/worker/workspace/build/src/dom/events/EventListenerManager.cpp:1038
#19 0x7f2dd68d3b13 in mozilla::EventListenerManager::HandleEventInternal(nsPresContext*, mozilla::WidgetEvent*, mozilla::dom::Event**, mozilla::dom::EventTarget*, nsEventStatus*, bool) /builds/worker/workspace/build/src/dom/events/EventListenerManager.cpp:1239:17
#20 0x7f2dd68b3c90 in HandleEvent /builds/worker/workspace/build/src/obj-firefox/dist/include/mozilla/EventListenerManager.h:355:5
#21 0x7f2dd68b3c90 in mozilla::EventTargetChainItem::HandleEvent(mozilla::EventChainPostVisitor&, mozilla::ELMCreationDetector&) /builds/worker/workspace/build/src/dom/events/EventDispatcher.cpp:351
#22 0x7f2dd68b1eb8 in mozilla::EventTargetChainItem::HandleEventTargetChain(nsTArray<mozilla::EventTargetChainItem>&, mozilla::EventChainPostVisitor&, mozilla::EventDispatchingCallback*, mozilla::ELMCreationDetector&) /builds/worker/workspace/build/src/dom/events/EventDispatcher.cpp:553:16
#23 0x7f2dd68b8b03 in mozilla::EventDispatcher::Dispatch(nsISupports*, nsPresContext*, mozilla::WidgetEvent*, mozilla::dom::Event*, nsEventStatus*, mozilla::EventDispatchingCallback*, nsTArray<mozilla::dom::EventTarget*>) /builds/worker/workspace/build/src/dom/events/EventDispatcher.cpp:1048:11
#24 0x7f2dd97720b7 in nsDocumentViewer::LoadComplete(nsresult) /builds/worker/workspace/build/src/layout/base/nsDocumentViewer.cpp:1102:7
#25 0x7f2ddc61f03c in nsDocShell::EndPageLoad(nsIWebProgress, nsIChannel*, nsresult) /builds/worker/workspace/build/src/docshell/base/nsDocShell.cpp:6587:21

SUMMARY: AddressSanitizer: heap-use-after-free /builds/worker/workspace/build/src/obj-firefox/dist/include/nsWrapperCache.h:162:12 in GetWrapperMaybeDead
Shadow bytes around the buggy address:
0x0c10800137a0: fa fa fa fa 00 00 00 00 00 00 00 00 00 00 05 fa
0x0c10800137b0: fa fa fa fa 00 00 00 00 00 00 00 00 00 00 00 00
0x0c10800137c0: fa fa fa fa fd fd fd fd fd fd fd fd fd fd fd fa
0x0c10800137d0: fa fa fa fa 00 00 00 00 00 00 00 00 00 00 00 fa
0x0c10800137e0: fa fa fa fa 00 00 00 00 00 00 00 00 00 00 00 00
=>0x0c10800137f0: fa fa fa fa fd fd[fd]fd fd fd fd fd fd fd fd fa
0x0c1080013800: fa fa fa fa fd fd fd fd fd fd fd fd fd fd fd fd
0x0c1080013810: fa fa fa fa fd fd fd fd fd fd fd fd fd fd fd fa
0x0c1080013820: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
0x0c1080013830: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
0x0c1080013840: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
Shadow byte legend (one shadow byte represents 8 application bytes):
Addressable: 00
Partially addressable: 01 02 03 04 05 06 07
Heap left redzone: fa
Freed heap region: fd
Stack left redzone: f1
Stack mid redzone: f2
Stack right redzone: f3
Stack after return: f5
Stack use after scope: f8
Global redzone: f9
Global init order: f6
Poisoned by user: f7
Container overflow: fc
Array cookie: ac
Intra object redzone: bb
ASan internal: fe
Left alloca redzone: ca
Right alloca redzone: cb
Shadow gap: cc
==8893==ABORTING

Flags: in-testsuite?
Attached file testcase.html —
Group: core-security → layout-core-security
Keywords: sec-critical

Will take a look. Test-case uses InspectorUtils but I suspect it's not InspectorUtils-specific.

Flags: needinfo?(emilio)
Priority: -- → P1
Whiteboard: [layout:triage-discuss]
Depends on: 1535671
Assignee: nobody → emilio
Blocks: 1345697
Status: NEW → ASSIGNED

Should print null, not the parent object.

Flags: needinfo?(emilio)

Comment on attachment 9051397 [details]
Bug 1535612 - CSSKeyframeList::RemoveRule should clear parent references when removed. r=heycam

Beta/Release Uplift Approval Request

  • Feature/Bug causing the regression: Bug 1345697
  • User impact if declined: sec-crit
  • Is this code covered by automated tests?: No
  • Has the fix been verified in Nightly?: No
  • Needs manual test from QE?: No
  • If yes, steps to reproduce:
  • List of other uplifts needed: None
  • Risk to taking this patch: Low
  • Why is the change risky/not risky? (and alternatives if risky): Doing cleanup before removing children. Will land a test once change is in branches, since there's an underlying correctness issue.
  • String changes made/needed: none

ESR Uplift Approval Request

  • If this is not a sec:{high,crit} bug, please state case for ESR consideration:
  • User impact if declined: sec-crit
  • Fix Landed on Version: Not yet
  • Risk to taking this patch: Low
  • Why is the change risky/not risky? (and alternatives if risky): Doing cleanup before removing children. Will land a test once change is in branches, since there's an underlying correctness issue.
  • String or UUID changes made by this patch: none

Security Approval Request

  • How easily could an exploit be constructed based on the patch?: Relatively easily.
  • Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?: Yes
  • Which older supported branches are affected by this flaw?: All
  • If not all supported branches, which bug introduced the flaw?: Bug 1345697
  • Do you have backports for the affected branches?: No
  • If not, how different, hard to create, and risky will they be?: Should apply cleanly, or maybe with the exception of bug 1451289, but should be a trivial rebase.
  • How likely is this patch to cause regressions; how much testing does it need?: not much
Attachment #9051397 - Flags: sec-approval?
Attachment #9051397 - Flags: approval-mozilla-release?
Attachment #9051397 - Flags: approval-mozilla-esr60?
Attachment #9051397 - Flags: approval-mozilla-beta?

If we commit to fixing this it would delay the 66 release date, which is in 2 days.
I don't want to do that unless it's a serious situation where we know this is being actively exploited.
We could consider taking this for a dot release a bit later in the 66 release cycle.

status-firefox65: --- → wontfix
status-firefox66: --- → affected
status-firefox67: --- → affected
tracking-firefox66: --- → +
tracking-firefox67: --- → +

(In reply to Liz Henry (:lizzard) (use needinfo) from comment #7)

If we commit to fixing this it would delay the 66 release date, which is in 2 days.
I don't want to do that unless it's a serious situation where we know this is being actively exploited.
We could consider taking this for a dot release a bit later in the 66 release cycle.

I'm not sure how much of a deal a delay is, but I think we should fix this, this is really trivially exploitable.

I think we should consider this for a release if we do one for pwn2own later this week. Otherwise, this should wait. It is a bug found internally by a member of my team, not an outside party. Regardless of the trivial exploit nature, if we don't check in a fix yet, it will probably not be rediscovered before we ship 67 (or a point release of 66).

Whiteboard: [layout:triage-discuss]

So far, my plan is not to take this for the potential 66.0.1 chemspill this week. But, we are planning a regular dot release (probably 66.0.2) for mid next week.
So let's deal with pwn2own first, then get this landed and onto m-r afterwards. For now please hold off.

Comment on attachment 9051397 [details]
Bug 1535612 - CSSKeyframeList::RemoveRule should clear parent references when removed. r=heycam

sec-approval+ to land on mozilla-central.

Attachment #9051397 - Flags: sec-approval? → sec-approval+

https://hg.mozilla.org/integration/autoland/rev/208cf2d82f55bf31d951e818ca0e3b29ef2f5ebe
https://hg.mozilla.org/mozilla-central/rev/208cf2d82f55

Group: layout-core-security → core-security-release
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
status-firefox68: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla68

Comment on attachment 9051397 [details]
Bug 1535612 - CSSKeyframeList::RemoveRule should clear parent references when removed. r=heycam

Patch landed on mozilla-central, approved for beta/release/esr

Attachment #9051397 - Flags: approval-mozilla-release?
Attachment #9051397 - Flags: approval-mozilla-release+
Attachment #9051397 - Flags: approval-mozilla-esr60?
Attachment #9051397 - Flags: approval-mozilla-esr60+
Attachment #9051397 - Flags: approval-mozilla-beta?
Attachment #9051397 - Flags: approval-mozilla-beta+

we usually rate use-after-frees in a sandboxed child process as sec-high

Keywords: sec-critical → sec-high

Comment on attachment 9051397 [details]
Bug 1535612 - CSSKeyframeList::RemoveRule should clear parent references when removed. r=heycam

After further discussion, we decided to let this ship in the 67 cycle instead. Still need the ESR rebase, though :).

Attachment #9051397 - Flags: approval-mozilla-release+ → approval-mozilla-release-
Attached patch ESR rebase. — — Splinter Review
Flags: needinfo?(emilio)

Comment on attachment 9053692 [details] [diff] [review]
ESR rebase.

ESR Uplift Approval Request

  • If this is not a sec:{high,crit} bug, please state case for ESR consideration: Sec-high, flagging the rebased patch for uplift
  • User impact if declined:
  • Fix Landed on Version:
  • Risk to taking this patch: Low
  • Why is the change risky/not risky? (and alternatives if risky):
  • String or UUID changes made by this patch:
  • Do you want to request approval of these patches as well?: on
Attachment #9053692 - Flags: approval-mozilla-esr60?
Flags: qe-verify-
Attachment #9051397 - Flags: approval-mozilla-esr60+
Comment on attachment 9053692 [details] [diff] [review]
ESR rebase.

Sec-high issue, fixed in 67/68, let's uplift for 60.7esr.
Attachment #9053692 - Flags: approval-mozilla-esr60? → approval-mozilla-esr60+
Whiteboard: [adv-main67+][adv-esr60.7+]
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: