Closed Bug 1861736 Opened 2 years ago Closed 2 years ago

AddressSanitizer: heap-use-after-free [@ NotifyEvictedFromRenderingObserverSet] with WRITE of size 1

Categories

(Core :: SVG, defect)

Product:
Core
Component:
SVG
Platform:
x86_64
Linux
Type:
defect

Tracking

()

Status:
VERIFIED FIXED
Milestone:
121 Branch
Tracking Flags:
Tracking Status
firefox-esr115 --- unaffected
firefox119 --- unaffected
firefox120 + verified
firefox121 + verified

People

(Reporter: jkratzer, Assigned: longsonr)

References

(Blocks 1 open bug, Regression)

Details

(5 keywords, Whiteboard: [bugmon:bisected,confirmed])

Crash Data

Attachments

(6 files)

Testcase
655 bytes, text/plain
Details
Screenshot 2023-10-30 at 12.05.10.png
73.93 KB, image/png
Details
Bug 1861736 part 1 - preserve renderingobserverset across node adoptions r=emilio
48 bytes, text/x-phabricator-request
pascalc
: approval-mozilla-beta+
tjr
: sec-approval+
Details | Review
Bug 1861736 - don't invalidate when SVGRenderingObserverSet is destroyed r=emilio
48 bytes, text/x-phabricator-request
diannaS
: approval-mozilla-beta+
tjr
: sec-approval+
Details | Review
Bug 1861736 - crashtest r=emilio
48 bytes, text/x-phabricator-request
tjr
: sec-approval+
Details | Review
Bug 1861736 part 2 - explanation r=emilio
48 bytes, text/x-phabricator-request
tjr
: sec-approval+
Details | Review

Testcase found while fuzzing mozilla-central rev 03d61eb955a9 (built with: --enable-address-sanitizer --enable-fuzzing).

Testcase can be reproduced using the following commands:

$ pip install fuzzfetch grizzly-framework
$ python -m fuzzfetch --build 03d61eb955a9 --asan --fuzzing -n firefox
$ python -m grizzly.replay ./firefox/firefox testcase.html

AddressSanitizer: heap-use-after-free [@ NotifyEvictedFromRenderingObserverSet] with WRITE of size 1

    =================================================================
    ==392022==ERROR: AddressSanitizer: heap-use-after-free on address 0x50d000073dac at pc 0x7fdb52ff7c24 bp 0x7ffde7a07db0 sp 0x7ffde7a07da8
    WRITE of size 1 at 0x50d000073dac thread T0 (Isolated Web Co)
        #0 0x7fdb52ff7c23 in NotifyEvictedFromRenderingObserverSet /layout/svg/SVGObserverUtils.cpp:257:18
        #1 0x7fdb52ff7c23 in mozilla::SVGRenderingObserverSet::InvalidateAll() /layout/svg/SVGObserverUtils.cpp:1146:15
        #2 0x7fdb52ffe86d in ~SVGRenderingObserverSet /layout/svg/SVGObserverUtils.cpp:1099:5
        #3 0x7fdb52ffe86d in void nsINode::DeleteProperty<mozilla::SVGRenderingObserverSet>(void*, nsAtom*, void*, void*) /builds/worker/workspace/obj-build/dist/include/nsINode.h:1020:5
        #4 0x7fdb4b0b52e7 in nsPropertyTable::PropertyList::Destroy() /dom/base/nsPropertyTable.cpp:239:7
        #5 0x7fdb4b0b4e73 in nsPropertyTable::RemoveAllProperties() /dom/base/nsPropertyTable.cpp:61:10
        #6 0x7fdb4b060c29 in nsINode::LastRelease() /dom/base/nsINode.cpp:716:19
        #7 0x7fdb4ab771dd in mozilla::dom::Document::Release() /dom/base/Document.cpp:2436:1
        #8 0x7fdb4b0b27c5 in nsNodeInfoManager::RemoveNodeInfo(mozilla::dom::NodeInfo*) /dom/base/nsNodeInfoManager.cpp:330:20
        #9 0x7fdb4add9d3f in mozilla::dom::NodeInfo::~NodeInfo() /dom/base/NodeInfo.cpp:38:18
        #10 0x7fdb4addc208 in mozilla::dom::NodeInfo::DeleteCycleCollectable() /dom/base/NodeInfo.cpp:177:3
        #11 0x7fdb46bfc136 in SnowWhiteKiller::~SnowWhiteKiller() /xpcom/base/nsCycleCollector.cpp:2471:7
        #12 0x7fdb46bfaee5 in nsCycleCollector::FreeSnowWhite(bool) /xpcom/base/nsCycleCollector.cpp:2661:3
        #13 0x7fdb46c06947 in nsCycleCollector::BeginCollection(mozilla::CCReason, ccIsManual, nsICycleCollectorListener*) /xpcom/base/nsCycleCollector.cpp:3647:3
        #14 0x7fdb46c057df in nsCycleCollector::Collect(mozilla::CCReason, ccIsManual, js::SliceBudget&, nsICycleCollectorListener*, bool) /xpcom/base/nsCycleCollector.cpp:3471:9
        #15 0x7fdb46c0510a in nsCycleCollector::ShutdownCollect() /xpcom/base/nsCycleCollector.cpp:3410:20
        #16 0x7fdb46c082f6 in nsCycleCollector::Shutdown(bool) /xpcom/base/nsCycleCollector.cpp:3709:5
        #17 0x7fdb46c0a9f0 in nsCycleCollector_shutdown(bool) /xpcom/base/nsCycleCollector.cpp:4033:18
        #18 0x7fdb46eb3012 in mozilla::ShutdownXPCOM(nsIServiceManager*) /xpcom/build/XPCOMInit.cpp:702:3
        #19 0x7fdb57991e82 in XRE_InitChildProcess(int, char**, XREChildData const*) /toolkit/xre/nsEmbedFunctions.cpp:660:16
        #20 0x5571c026413c in content_process_main /browser/app/../../ipc/contentproc/plugin-container.cpp:57:28
        #21 0x5571c026413c in main /browser/app/nsBrowserApp.cpp:375:18
        #22 0x7fdb6e735d8f in __libc_start_call_main csu/../sysdeps/nptl/libc_start_call_main.h:58:16
        #23 0x7fdb6e735e3f in __libc_start_main csu/../csu/libc-start.c:392:3
        #24 0x5571c0188448 in _start (/home/jkratzer/builds/m-c-20231025162820-fuzzing-asan-opt/firefox+0xdc448) (BuildId: f2c4d3f115b8c54fe14fa9e58f42500a52813c4d)
    
    0x50d000073dac is located 28 bytes inside of 144-byte region [0x50d000073d90,0x50d000073e20)
    freed by thread T0 (Isolated Web Co) here:
        #0 0x5571c0224006 in free /builds/worker/fetches/llvm-project/compiler-rt/lib/asan/asan_malloc_linux.cpp:52:3
        #1 0x7fdb46bfc136 in SnowWhiteKiller::~SnowWhiteKiller() /xpcom/base/nsCycleCollector.cpp:2471:7
        #2 0x7fdb46bfaee5 in nsCycleCollector::FreeSnowWhite(bool) /xpcom/base/nsCycleCollector.cpp:2661:3
        #3 0x7fdb46c06947 in nsCycleCollector::BeginCollection(mozilla::CCReason, ccIsManual, nsICycleCollectorListener*) /xpcom/base/nsCycleCollector.cpp:3647:3
        #4 0x7fdb46c057df in nsCycleCollector::Collect(mozilla::CCReason, ccIsManual, js::SliceBudget&, nsICycleCollectorListener*, bool) /xpcom/base/nsCycleCollector.cpp:3471:9
        #5 0x7fdb46c0510a in nsCycleCollector::ShutdownCollect() /xpcom/base/nsCycleCollector.cpp:3410:20
        #6 0x7fdb46c082f6 in nsCycleCollector::Shutdown(bool) /xpcom/base/nsCycleCollector.cpp:3709:5
        #7 0x7fdb46c0a9f0 in nsCycleCollector_shutdown(bool) /xpcom/base/nsCycleCollector.cpp:4033:18
        #8 0x7fdb46eb3012 in mozilla::ShutdownXPCOM(nsIServiceManager*) /xpcom/build/XPCOMInit.cpp:702:3
        #9 0x7fdb57991e82 in XRE_InitChildProcess(int, char**, XREChildData const*) /toolkit/xre/nsEmbedFunctions.cpp:660:16
        #10 0x5571c026413c in content_process_main /browser/app/../../ipc/contentproc/plugin-container.cpp:57:28
        #11 0x5571c026413c in main /browser/app/nsBrowserApp.cpp:375:18
        #12 0x7fdb6e735d8f in __libc_start_call_main csu/../sysdeps/nptl/libc_start_call_main.h:58:16
    
    previously allocated by thread T0 (Isolated Web Co) here:
        #0 0x5571c02242ae in malloc /builds/worker/fetches/llvm-project/compiler-rt/lib/asan/asan_malloc_linux.cpp:69:3
        #1 0x5571c02693c5 in moz_xmalloc /memory/mozalloc/mozalloc.cpp:52:15
        #2 0x7fdb52ff5765 in operator new /builds/worker/workspace/obj-build/dist/include/mozilla/cxxalloc.h:33:10
        #3 0x7fdb52ff5765 in mozilla::SVGFilterObserverList::SVGFilterObserverList(mozilla::Span<mozilla::StyleGenericFilter<mozilla::StyleAngle, float, float, mozilla::StyleCSSPixelLength, mozilla::StyleGenericSimpleShadow<mozilla::StyleGenericColor<mozilla::StylePercentage>, mozilla::StyleCSSPixelLength, mozilla::StyleCSSPixelLength>, mozilla::StyleComputedUrl> const, 18446744073709551615ul>, nsIContent*, nsIFrame*) /layout/svg/SVGObserverUtils.cpp:905:9
        #4 0x7fdb52ff9886 in SVGFilterObserverListForCanvasContext /layout/svg/SVGObserverUtils.cpp:941:9
        #5 0x7fdb52ff9886 in mozilla::SVGObserverUtils::ObserveFiltersForCanvasContext(mozilla::dom::CanvasRenderingContext2D*, mozilla::dom::Element*, mozilla::Span<mozilla::StyleGenericFilter<mozilla::StyleAngle, float, float, mozilla::StyleCSSPixelLength, mozilla::StyleGenericSimpleShadow<mozilla::StyleGenericColor<mozilla::StylePercentage>, mozilla::StyleCSSPixelLength, mozilla::StyleCSSPixelLength>, mozilla::StyleComputedUrl> const, 18446744073709551615ul>) /layout/svg/SVGObserverUtils.cpp:1381:24
        #6 0x7fdb4d50d74b in mozilla::dom::CanvasRenderingContext2D::SetFilter(nsTSubstring<char> const&, mozilla::ErrorResult&) /dom/canvas/CanvasRenderingContext2D.cpp:2672:11
        #7 0x7fdb4bf42789 in mozilla::dom::CanvasRenderingContext2D_Binding::set_filter(JSContext*, JS::Handle<JSObject*>, void*, JSJitSetterCallArgs) /builds/worker/workspace/obj-build/dom/bindings/./CanvasRenderingContext2DBinding.cpp:5243:24
        #8 0x7fdb4d2e0b7d in bool mozilla::dom::binding_detail::GenericSetter<mozilla::dom::binding_detail::NormalThisPolicy>(JSContext*, unsigned int, JS::Value*) /dom/bindings/BindingUtils.cpp:3275:8
        #9 0x7fdb57dda925 in CallJSNative /js/src/vm/Interpreter.cpp:472:13
        #10 0x7fdb57dda925 in js::InternalCallOrConstruct(JSContext*, JS::CallArgs const&, js::MaybeConstruct, js::CallReason) /js/src/vm/Interpreter.cpp:566:12
        #11 0x7fdb57ddca16 in InternalCall /js/src/vm/Interpreter.cpp:633:10
        #12 0x7fdb57ddca16 in 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:665:8
        #13 0x7fdb57ddea93 in js::CallSetter(JSContext*, JS::Handle<JS::Value>, JS::Handle<JS::Value>, JS::Handle<JS::Value>) /js/src/vm/Interpreter.cpp:796:10
        #14 0x7fdb58191447 in SetExistingProperty(JSContext*, JS::Handle<JS::PropertyKey>, JS::Handle<JS::Value>, JS::Handle<JS::Value>, JS::Handle<js::NativeObject*>, js::PropertyResult const&, JS::ObjectOpResult&) /js/src/vm/NativeObject.cpp:2588:8
        #15 0x7fdb5818e150 in bool js::NativeSetProperty<(js::QualifiedBool)1>(JSContext*, JS::Handle<js::NativeObject*>, JS::Handle<JS::PropertyKey>, JS::Handle<JS::Value>, JS::Handle<JS::Value>, JS::ObjectOpResult&) /js/src/vm/NativeObject.cpp:2622:14
        #16 0x7fdb57df999e in SetProperty /js/src/vm/ObjectOperations-inl.h:305:10
        #17 0x7fdb57df999e in SetObjectElementOperation /js/src/vm/Interpreter.cpp:1586:10
        #18 0x7fdb57df999e in js::Interpret(JSContext*, js::RunState&) /js/src/vm/Interpreter.cpp:2799:12
        #19 0x7fdb57dd96a5 in MaybeEnterInterpreterTrampoline /js/src/vm/Interpreter.cpp:386:10
        #20 0x7fdb57dd96a5 in js::RunScript(JSContext*, js::RunState&) /js/src/vm/Interpreter.cpp:444:13
        #21 0x7fdb57ddaa8e in js::InternalCallOrConstruct(JSContext*, JS::CallArgs const&, js::MaybeConstruct, js::CallReason) /js/src/vm/Interpreter.cpp:598:13
        #22 0x7fdb57ddca16 in InternalCall /js/src/vm/Interpreter.cpp:633:10
        #23 0x7fdb57ddca16 in 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:665:8
        #24 0x7fdb57f371fb in JS::Call(JSContext*, JS::Handle<JS::Value>, JS::Handle<JS::Value>, JS::HandleValueArray const&, JS::MutableHandle<JS::Value>) /js/src/vm/CallAndConstruct.cpp:119:10
        #25 0x7fdb4cd3d9ef in mozilla::dom::EventListener::HandleEvent(mozilla::dom::BindingCallContext&, JS::Handle<JS::Value>, mozilla::dom::Event&, mozilla::ErrorResult&) /builds/worker/workspace/obj-build/dom/bindings/./EventListenerBinding.cpp:62:8
        #26 0x7fdb4e119fe7 in void mozilla::dom::EventListener::HandleEvent<mozilla::dom::EventTarget*>(mozilla::dom::EventTarget* const&, mozilla::dom::Event&, mozilla::ErrorResult&, char const*, mozilla::dom::CallbackObject::ExceptionHandling, JS::Realm*) /builds/worker/workspace/obj-build/dist/include/mozilla/dom/EventListenerBinding.h:65:12
        #27 0x7fdb4e11928f in mozilla::EventListenerManager::HandleEventSingleListener(mozilla::EventListenerManager::Listener*, nsAtom*, mozilla::WidgetEvent*, mozilla::dom::Event*, mozilla::dom::EventTarget*, bool) /dom/events/EventListenerManager.cpp:1342:43
        #28 0x7fdb4e11bf1a in mozilla::EventListenerManager::HandleEventWithListenerArray(mozilla::EventListenerManager::ListenerArray*, nsAtom*, mozilla::EventMessage, nsPresContext*, mozilla::WidgetEvent*, mozilla::dom::Event**, mozilla::dom::EventTarget*, bool) /dom/events/EventListenerManager.cpp:1663:12
        #29 0x7fdb4e11a976 in mozilla::EventListenerManager::HandleEventInternal(nsPresContext*, mozilla::WidgetEvent*, mozilla::dom::Event**, mozilla::dom::EventTarget*, nsEventStatus*, bool) /dom/events/EventListenerManager.cpp:1560:35
        #30 0x7fdb4e0fddd2 in mozilla::EventTargetChainItem::HandleEvent(mozilla::EventChainPostVisitor&, mozilla::ELMCreationDetector&) /dom/events/EventDispatcher.cpp:363:17
        #31 0x7fdb4e0fb2f8 in mozilla::EventTargetChainItem::HandleEventTargetChain(nsTArray<mozilla::EventTargetChainItem>&, mozilla::EventChainPostVisitor&, mozilla::EventDispatchingCallback*, mozilla::ELMCreationDetector&) /dom/events/EventDispatcher.cpp:610:18
        #32 0x7fdb4e102d89 in mozilla::EventDispatcher::Dispatch(mozilla::dom::EventTarget*, nsPresContext*, mozilla::WidgetEvent*, mozilla::dom::Event*, nsEventStatus*, mozilla::EventDispatchingCallback*, nsTArray<mozilla::dom::EventTarget*>*) /dom/events/EventDispatcher.cpp:1225:11
        #33 0x7fdb4e10bd35 in mozilla::EventDispatcher::DispatchDOMEvent(mozilla::dom::EventTarget*, mozilla::WidgetEvent*, mozilla::dom::Event*, nsPresContext*, nsEventStatus*) /dom/events/EventDispatcher.cpp
        #34 0x7fdb4b0691bb in nsINode::DispatchEvent(mozilla::dom::Event&, mozilla::dom::CallerType, mozilla::ErrorResult&) /dom/base/nsINode.cpp:1401:17
        #35 0x7fdb4a786485 in nsContentUtils::DispatchEvent(mozilla::dom::Document*, mozilla::dom::EventTarget*, nsTSubstring<char16_t> const&, mozilla::CanBubble, mozilla::Cancelable, mozilla::Composed, mozilla::Trusted, bool*, mozilla::ChromeOnlyDispatch) /dom/base/nsContentUtils.cpp:4635:29
        #36 0x7fdb4a786134 in nsContentUtils::DispatchTrustedEvent(mozilla::dom::Document*, mozilla::dom::EventTarget*, nsTSubstring<char16_t> const&, mozilla::CanBubble, mozilla::Cancelable, mozilla::Composed, bool*) /dom/base/nsContentUtils.cpp:4601:10
        #37 0x7fdb4abd8d1f in mozilla::dom::Document::DispatchContentLoadedEvents() /dom/base/Document.cpp:8035:3
    
    SUMMARY: AddressSanitizer: heap-use-after-free /layout/svg/SVGObserverUtils.cpp:257:18 in NotifyEvictedFromRenderingObserverSet
    Shadow bytes around the buggy address:
      0x50d000073b00: fa fa fa fa fd fd fd fd fd fd fd fd fd fd fd fd
      0x50d000073b80: fd fd fd fd fd fa fa fa fa fa fa fa fa fa fd fd
      0x50d000073c00: fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd fa
      0x50d000073c80: fa fa fa fa fa fa fa fa fd fd fd fd fd fd fd fd
      0x50d000073d00: fd fd fd fd fd fd fd fd fd fa fa fa fa fa fa fa
    =>0x50d000073d80: fa fa fd fd fd[fd]fd fd fd fd fd fd fd fd fd fd
      0x50d000073e00: fd fd fd fd fa fa fa fa fa fa fa fa fd fd fd fd
      0x50d000073e80: fd fd fd fd fd fd fd fd fd fd fd fd fd fd fa fa
      0x50d000073f00: fa fa fa fa fa fa fd fd fd fd fd fd fd fd fd fd
      0x50d000073f80: fd fd fd fd fd fd fd fd fa fa fa fa fa fa fa fa
      0x50d000074000: fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd
    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
    ==392022==ABORTING
Attached file Testcase —
Group: core-security → layout-core-security

Verified bug as reproducible on mozilla-central 20231027211343-ec7d4cb306bc.
The bug appears to have been introduced in the following build range:

Start: e557eb7d97ce665fd81c680ed136877f12572c13 (20231004205448)
End: d1dad83a0db5ef928894999742e0325d1dac544a (20231004213608)
Pushlog: https://hg.mozilla.org/integration/autoland/pushloghtml?fromchange=e557eb7d97ce665fd81c680ed136877f12572c13&tochange=d1dad83a0db5ef928894999742e0325d1dac544a

Successfully recorded a pernosco session. A link to the pernosco session will be added here shortly.

Keywords: pernosco-wanted → pernosco, regression
Whiteboard: [bugmon:confirm] → [bugmon:bisected,confirmed]

A pernosco session for this bug can be found here.

Here we're destroying the rendering observer set (14) (call that set 1) but we add the observer back again to another set (set 20. I think this confuses things because we lose track of that set and put the observer in yet another set (set 3). Then we clean up set 3 it deletes the observer and when we clean up set 2 it has a dead observer. This all originates from cloneAndAdopt which is why you need the adoptNode to cause a problem.

From the stacks, it looks like the use and free could be in the same cycle collection, which would reduce exploitability, but I'm not sure about that, and maybe messing around with the test case somehow would delay the use to another CC, so I'll mark this sec-high.

Keywords: csectype-uaf, sec-high

This bug has been marked as a regression. Setting status flag for Nightly to affected.

status-firefox121: --- → affected

Removing InvalidateAll() from https://searchfox.org/mozilla-central/source/layout/svg/SVGObserverUtils.cpp#1099 i.e. the destructor of the SVGRenderingObserverSet fixes the heap-use-after-free. Need to check somehow whether that change would cause reftest/WPT failures.

Alternatively we'd have to have some flag to say we're removing properties from the element as part of clone and adopt and we shouldn't add them back as part of that call stack.

So 2 approaches seem viable.

  1. Remove InvalidateAll() from the destructor of SVGRenderingObserverSet - the most direct approach.

  2. Add a true parameter to the SetProperty for nsGkAtoms::renderingobserverset so that the property isn't deleted when the element is adopted.

Both work to fix this bug, although I can't be sure whether either will result in invalidation failures unless I push one or other of them to try.

Assignee: nobody → longsonr
Status: NEW → ASSIGNED

don't invalidate when SVGRenderingObserverSet is destroyed is clearly also going to fix 1862026, as it simply removes the codepath that we see in that callstack.

preserve renderingobserverset across node adoptions r=emilio is less likely to cause regressions as it will only activate during node adoptions.

Either patch may cause rendering regressions i.e. oranges. I can't tell without pushing one or other to try. I could try to hide one or other in some other code but I really ought to push to try before trying to land either of these.

Either patch on it's own fixes this bug and ought to fix bug 1862026 too.

Blocks: 1862026

Based on comment #2, this bug contains a bisection range found by bugmon. However, the Regressed by field is still not filled.

:longsonr, if possible, could you fill the Regressed by field and investigate this regression?

For more information, please visit BugBot documentation.

Flags: needinfo?(longsonr)
Flags: needinfo?(longsonr)
Regressed by: 1494263

Set release status flags based on info from the regressing bug 1494263

status-firefox119: --- → unaffected
status-firefox120: --- → affected
status-firefox-esr115: --- → unaffected
Duplicate of this bug: 1862026

Copying crash signatures from duplicate bugs.

Crash Signature: [@ operator bool]
Attachment #9361311 - Attachment description: Bug 1861736 - preserve renderingobserverset across node adoptions r=emilio → Bug 1861736 part 1 - preserve renderingobserverset across node adoptions r=emilio

Comment on attachment 9361312 [details]
Bug 1861736 - don't invalidate when SVGRenderingObserverSet is destroyed r=emilio

Security Approval Request

  • How easily could an exploit be constructed based on the patch?: Seems quite hard, the change doesn't really point you to the cause of the issue. There's nothing that tells you that you need to call adoptNode to cause a problem.
  • 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?: 120
  • If not all supported branches, which bug introduced the flaw?: Bug 1494263
  • Do you have backports for the affected branches?: Yes
  • If not, how different, hard to create, and risky will they be?: Patch should apply to 120 without changes.
  • How likely is this patch to cause regressions; how much testing does it need?: Could well cause rendering regressions. Only landing on try would tell. I could try to hide it in bug 1862411
  • Is Android affected?: Yes
Attachment #9361312 - Flags: sec-approval?

Comment on attachment 9361311 [details]
Bug 1861736 part 1 - preserve renderingobserverset across node adoptions r=emilio

Security Approval Request

  • How easily could an exploit be constructed based on the patch?: More easy to determine that you need to adoptNode with this patch. You'd have to guess that you need a filter on a canvas though.
  • 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?: 120
  • If not all supported branches, which bug introduced the flaw?: Bug 1494263
  • Do you have backports for the affected branches?: Yes
  • If not, how different, hard to create, and risky will they be?: this patch should apply unchanged
  • How likely is this patch to cause regressions; how much testing does it need?: less likely to cause rendering regressions than the other patch. Again I'd really like to land on try first and hide it via bug 1862411

Note that ideally I'd land both patches so that we have belt and braces.

  • Is Android affected?: Yes
Attachment #9361311 - Flags: sec-approval?
Attachment #9361312 - Flags: sec-approval? → sec-approval+
Attachment #9361311 - Flags: sec-approval? → sec-approval+

I'm fine with you landing both under the cover bug, or sending to try under the cover bug

Looks like don't invalidate when SVGRenderingObserverSet is destroyed r=emilio is OK.

https://treeherder.mozilla.org/jobs?repo=try&revision=939fbb1de794aa2e6aaef31cd264438901f1875a

Pushed by longsonr@gmail.com: https://hg.mozilla.org/integration/autoland/rev/1a2f03d90ddc don't invalidate when SVGRenderingObserverSet is destroyed r=emilio

https://hg.mozilla.org/mozilla-central/rev/1a2f03d90ddc

Group: layout-core-security → core-security-release
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
status-firefox121: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → 121 Branch

Comment on attachment 9361312 [details]
Bug 1861736 - don't invalidate when SVGRenderingObserverSet is destroyed r=emilio

Beta/Release Uplift Approval Request

  • User impact if declined: specially crafted content such as the testcase can cause a double free crash. That's likely exploitable.
  • Is this code covered by automated tests?: No
  • Has the fix been verified in Nightly?: Yes
  • Needs manual test from QE?: Yes
  • If yes, steps to reproduce: see comment 0
  • List of other uplifts needed: None
  • Risk to taking this patch: Low
  • Why is the change risky/not risky? (and alternatives if risky): it's one line. The worst it could do would be to cause a rendering regression but that would almost certainly have been caught by existing reftests.
  • String changes made/needed: none
  • Is Android affected?: Yes
Attachment #9361312 - Flags: approval-mozilla-beta?
Flags: qe-verify+

Verified bug as fixed on rev mozilla-central 20231104091937-fa8ebe703963.
Removing bugmon keyword as no further action possible. Please review the bug and re-add the keyword for further analysis.

Status: RESOLVED → VERIFIED
status-firefox121: fixed → verified
Keywords: bugmon

Just a few questions:
-During this uplift, is this okay to land with the current commit message or do I need change it to the cover bug?
-the sec-approval stated "Note that ideally I'd land both patches so that we have belt and braces." but were just landing 1a2f03d90ddc.. is that correct or do we need to uplift anything from bug 1862411?

Thank you!

Flags: needinfo?(longsonr)

I landed to try with the cover bug. The problem I had was that if the first patch here didn't work then I'd have to adapt it till it did and I needed the cover bug to hide repeated try pushes with potential fixes. All that didn't happen though because the first patch worked first time. The cover bug is harmless but not required at all any more. All we need to land right now is the first patch one line change from this bug to beta.

I'd still want to uplift the other patch in this bug eventually but I want this one to land on beta first because then I can land the other patch to try and see if it works much more safely. It's easier, though still not straightforward, to see where we have a problem if I land the second patch. If we've landed the first patch everywhere that needs it, disclosure really wouldn't matter any more.

Flags: needinfo?(longsonr)

Comment on attachment 9361312 [details]
Bug 1861736 - don't invalidate when SVGRenderingObserverSet is destroyed r=emilio

Approved for 120.0b8

Attachment #9361312 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
QA Whiteboard: [qa-triaged]

Hi @Robert will this command :" $ python -m fuzzfetch --build 03d61eb955a9 --asan --fuzzing -n firefox " only grab builds from mozilla-central ? is there a way to grab our latest Beta build ?

I downloaded our latest asan fuzzing beta build 120.0b8 manually and added the test case in that folder but the :
python -m grizzly.replay ./firefox testcase.html from that folder will only grab a build from Firefox 115, which is this one I think: 03d61eb955a9

I tried python3 -m fuzzfetch --build 38f5ce2d997a --asan --fuzzing -n firefox in order to grab our latest beta but it wont recognize the "mozilla-central" build because it should be "mozilla-beta".

Also In the 115 build, it would start it, open the grizzly tab and the testcase.html in a new tab but it wont crash, it would return "No results detected" in terminal. Is it suppose to crash ? What is the expected results ? I am not getting the : "==392022==ERROR: AddressSanitizer: heap-use-after-free on address " error in terminal in that build at all.

Flags: needinfo?(longsonr)

I've not tried the grizzly route myself, I just loaded the testcase in the browser via the address bar, waited 30 seconds or so for gc to take place and most of the time (> 50%) the tab would crash.

Flags: needinfo?(longsonr)
Pushed by longsonr@gmail.com: https://hg.mozilla.org/integration/autoland/rev/c709d9994c7c part 1 - preserve renderingobserverset across node adoptions r=emilio

Oh and the testcase never crashed 119 or earlier. Only 120 (beta) and nightly were ever affected

Comment on attachment 9361311 [details]
Bug 1861736 part 1 - preserve renderingobserverset across node adoptions r=emilio

Beta/Release Uplift Approval Request

  • User impact if declined: Not so much since the other patch in this bug landed but given this was a likely exploitable security issue at one time, better to be safe than sorry.
  • Is this code covered by automated tests?: No
  • Has the fix been verified in Nightly?: Yes
  • Needs manual test from QE?: Yes
  • If yes, steps to reproduce: One way is to run the testcase and wait for a garbage collection, or manually invoke one via about:memory. Another way was outlined in comment 0
  • List of other uplifts needed: None
  • Risk to taking this patch: Low
  • Why is the change risky/not risky? (and alternatives if risky): It's one line. The worst it's likely to do is cause invalidation issues. The first patch was more likely to do that though.
  • String changes made/needed: none
  • Is Android affected?: Yes
Flags: needinfo?(longsonr)
Attachment #9361311 - Flags: approval-mozilla-beta?

Comment on attachment 9361311 [details]
Bug 1861736 part 1 - preserve renderingobserverset across node adoptions r=emilio

Approved for out last beta, thanks.

Attachment #9361311 - Flags: approval-mozilla-beta? → approval-mozilla-beta+

Verified as fixed in our latest Beta build 120.0b8, I was able to reproduce the issue in 120.0b7. Thanks @Robert !

QA Whiteboard: [qa-triaged]
status-firefox120: fixed → verified
Flags: qe-verify+

Comment on attachment 9361313 [details]
Bug 1861736 - crashtest r=emilio

Security Approval Request

  • How easily could an exploit be constructed based on the patch?: well this is a crashtest so really really easily, however the fixes have been in for some time so this is no longer exploitable.
  • 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?:
  • 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?: just a test so no chance at all.
  • Is Android affected?: Yes
Attachment #9361313 - Flags: sec-approval?
Attachment #9361578 - Flags: sec-approval?
Attachment #9361313 - Flags: sec-approval? → sec-approval+

Comment on attachment 9361578 [details]
Bug 1861736 part 2 - explanation r=emilio

So official policy is that if a bug fix lands in version N, we can ship the tests/comments in version N+1.5. (That is, if it lands in 120, we can ship the tests half-way through the 121 cycle). Which is about now, so you can land these.

I must not have noticed these when I approved before, because what I will typically do is set a whiteboard tag of [reminder-test 2023-01-08] which will automatically needinfo you on that date to remind you to land the tests.

Attachment #9361578 - Flags: sec-approval? → sec-approval+
Pushed by longsonr@gmail.com: https://hg.mozilla.org/integration/autoland/rev/8241d3b84402 part 2 - explanation r=emilio DONTBUILD
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: