Closed Bug 1729642 (CVE-2021-38498) Opened 3 years ago Closed 3 years ago

SUMMARY: AddressSanitizer: heap-use-after-free PLDHashTable.cpp:619 in PLDHashTable::MakeEntryHandle

Categories

(Core :: Internationalization, task)

task

Tracking

()

RESOLVED FIXED
94 Branch
Tracking Status
firefox-esr78 --- wontfix
firefox-esr91 93+ fixed
firefox92 --- wontfix
firefox93 + fixed
firefox94 + fixed

People

(Reporter: m.cooolie, Assigned: jfkthame)

References

Details

(Keywords: csectype-uaf, sec-moderate, Whiteboard: [reporter-external] [client-bounty-form] [verif?][adv-main93+][adv-esr91.2+])

Crash Data

Attachments

(5 files)

Attached file 0908.zip

#Summary
SUMMARY: AddressSanitizer: heap-use-after-free PLDHashTable.cpp:619 in PLDHashTable::MakeEntryHandle

#Reproduce
OS:Win10 X64
Firefox: Nightly 94.0a1 (2021-09-07) (64-bit)

step:

  1. python -m http.server 80
  2. install node&puppeteer-core (ffpuppet not work on windows)
  3. node ff.test.js D:\firefox_asan\target\firefox\firefox.exe http://localhost/poc.htm
  4. wait for 30s if not crashes try again

#Type of crash
Tab process

#Analysis
Not yet

#Patch
Not Yet

#ASAN

==3780==ERROR: AddressSanitizer: heap-use-after-free on address 0x1275b70c0d70 at pc 0x7ffe09934350 bp 0x00d37d5f9880 sp 0x00d37d5f98c8
READ of size 4 at 0x1275b70c0d70 thread T0
#0 0x7ffe0993434f in PLDHashTable::MakeEntryHandle /builds/worker/checkouts/gecko/xpcom/ds/PLDHashTable.cpp:619
#1 0x7ffe09934c5b in PLDHashTable::MakeEntryHandle /builds/worker/checkouts/gecko/xpcom/ds/PLDHashTable.cpp:673
#2 0x7ffe09cf2113 in nsLanguageAtomService::GetLanguageGroup /builds/worker/checkouts/gecko/intl/locale/nsLanguageAtomService.cpp:146
#3 0x7ffe1344b863 in mozilla::StaticPresData::GetFontPrefsForLang /builds/worker/checkouts/gecko/layout/base/StaticPresData.cpp:219
#4 0x7ffe132c18af in nsStyleFont::nsStyleFont /builds/worker/checkouts/gecko/layout/style/nsStyleStruct.cpp:240
#5 0x7ffe187c3330 in style::gecko_properties::ComputedValues::default_values /builds/worker/workspace/obj-build/x86_64-pc-windows-msvc/release/build/style-4ff881e47c64df5b/out/gecko_properties.rs:315
#6 0x7ffe181cf3fa in geckoservo::glue::Servo_StyleSet_Init /builds/worker/checkouts/gecko/servo/ports/geckolib/glue.rs:4095
#7 0x7ffe1325536a in mozilla::ServoStyleSet::ServoStyleSet /builds/worker/checkouts/gecko/layout/style/ServoStyleSet.cpp:117
#8 0x7ffe0cfde456 in mozilla::dom::Document::Init /builds/worker/checkouts/gecko/dom/base/Document.cpp:2639
#9 0x7ffe123d40d9 in NS_NewDOMDocument /builds/worker/checkouts/gecko/dom/xml/XMLDocument.cpp:75
#10 0x7ffe0cec6f50 in mozilla::dom::DOMImplementation::CreateDocument /builds/worker/checkouts/gecko/dom/base/DOMImplementation.cpp:100
#11 0x7ffe0cec77ed in mozilla::dom::DOMImplementation::CreateDocument /builds/worker/checkouts/gecko/dom/base/DOMImplementation.cpp:128
#12 0x7ffe0efefef1 in mozilla::dom::DOMImplementation_Binding::createDocument /builds/worker/workspace/obj-build/dom/bindings/DOMImplementationBinding.cpp:166
#13 0x7ffe0f858269 in mozilla::dom::binding_detail::GenericMethod<mozilla::dom::binding_detail::NormalThisPolicy,mozilla::dom::binding_detail::ThrowExceptions> /builds/worker/checkouts/gecko/dom/bindings/BindingUtils.cpp:3300
#14 0x24bd57e396e (<unknown module>)

0x1275b70c0d70 is located 32 bytes inside of 48-byte region [0x1275b70c0d50,0x1275b70c0d80)
freed by thread T0 here:
#0 0x7ffe33555afb in free Z:\task_163036031364766\fetches\llvm-project\llvm\projects\compiler-rt\lib\asan\asan_malloc_win.cpp:82
#1 0x7ffe09834114 in mozilla::KillClearOnShutdown /builds/worker/checkouts/gecko/xpcom/base/ClearOnShutdown.cpp:55
#2 0x7ffe09b9473d in mozilla::ShutdownXPCOM /builds/worker/checkouts/gecko/xpcom/build/XPCOMInit.cpp:650
#3 0x7ffe16f70200 in XRE_TermEmbedding /builds/worker/checkouts/gecko/toolkit/xre/nsEmbedFunctions.cpp:218
#4 0x7ffe0aef9368 in mozilla::ipc::ScopedXREEmbed::Stop /builds/worker/checkouts/gecko/ipc/glue/ScopedXREEmbed.cpp:90
#5 0x7ffe16f71064 in XRE_InitChildProcess /builds/worker/checkouts/gecko/toolkit/xre/nsEmbedFunctions.cpp:753
#6 0x7ff7897e1f49 in NS_internal_main /builds/worker/checkouts/gecko/browser/app/nsBrowserApp.cpp:327
#7 0x7ff7897e14d4 in wmain /builds/worker/checkouts/gecko/toolkit/xre/nsWindowsWMain.cpp:131
#8 0x7ff7898de217 in __scrt_common_main_seh d:\agent_work\1\s\src\vctools\crt\vcstartup\src\startup\exe_common.inl:288
#9 0x7ffe61627033 in BaseThreadInitThunk+0x13 (C:\Windows\System32\KERNEL32.DLL+0x180017033)
#10 0x7ffe63602650 in RtlUserThreadStart+0x20 (C:\Windows\SYSTEM32\ntdll.dll+0x180052650)

previously allocated by thread T0 here:
#0 0x7ffe33555c0b in malloc Z:\task_163036031364766\fetches\llvm-project\llvm\projects\compiler-rt\lib\asan\asan_malloc_win.cpp:98
#1 0x7ffe33ec139d in moz_xmalloc /builds/worker/checkouts/gecko/memory/mozalloc/mozalloc.cpp:52
#2 0x7ffe09cf1bf7 in nsLanguageAtomService::GetService /builds/worker/checkouts/gecko/intl/locale/nsLanguageAtomService.cpp:87
#3 0x7ffe1344a428 in mozilla::StaticPresData::Init /builds/worker/checkouts/gecko/layout/base/StaticPresData.cpp:20
#4 0x7ffe13f33dd5 in nsLayoutStatics::Initialize /builds/worker/checkouts/gecko/layout/build/nsLayoutStatics.cpp:178
#5 0x7ffe13f33cbd in nsLayoutModuleInitialize /builds/worker/checkouts/gecko/layout/build/nsLayoutModule.cpp:104
#6 0x7ffe09a8abf9 in nsComponentManagerImpl::Init /builds/worker/checkouts/gecko/xpcom/components/nsComponentManager.cpp:408
#7 0x7ffe09b936e1 in NS_InitXPCOM /builds/worker/checkouts/gecko/xpcom/build/XPCOMInit.cpp:446
#8 0x7ffe16f7013e in XRE_InitEmbedding2 /builds/worker/checkouts/gecko/toolkit/xre/nsEmbedFunctions.cpp:192
#9 0x7ffe0aef98ac in mozilla::ipc::ScopedXREEmbed::Start /builds/worker/checkouts/gecko/ipc/glue/ScopedXREEmbed.cpp:83
#10 0x7ffe120ffe0f in mozilla::dom::ContentProcess::Init /builds/worker/checkouts/gecko/dom/ipc/ContentProcess.cpp:202
#11 0x7ffe16f70f01 in XRE_InitChildProcess /builds/worker/checkouts/gecko/toolkit/xre/nsEmbedFunctions.cpp:715
#12 0x7ff7897e1f49 in NS_internal_main /builds/worker/checkouts/gecko/browser/app/nsBrowserApp.cpp:327
#13 0x7ff7897e14d4 in wmain /builds/worker/checkouts/gecko/toolkit/xre/nsWindowsWMain.cpp:131
#14 0x7ff7898de217 in __scrt_common_main_seh d:\agent_work\1\s\src\vctools\crt\vcstartup\src\startup\exe_common.inl:288
#15 0x7ffe61627033 in BaseThreadInitThunk+0x13 (C:\Windows\System32\KERNEL32.DLL+0x180017033)
#16 0x7ffe63602650 in RtlUserThreadStart+0x20 (C:\Windows\SYSTEM32\ntdll.dll+0x180052650)

SUMMARY: AddressSanitizer: heap-use-after-free /builds/worker/checkouts/gecko/xpcom/ds/PLDHashTable.cpp:619 in PLDHashTable::MakeEntryHandle
Shadow bytes around the buggy address:
0x04bc6de98150: fa fa fd fd fd fd fd fd fa fa fd fd fd fd fd fa
0x04bc6de98160: fa fa 00 00 00 00 00 00 fa fa 00 00 00 00 00 00
0x04bc6de98170: fa fa 00 00 00 00 00 04 fa fa 00 00 00 00 00 00
0x04bc6de98180: fa fa 00 00 00 00 00 00 fa fa 00 00 00 00 00 00
0x04bc6de98190: fa fa 00 00 00 00 00 00 fa fa 00 00 00 00 00 00
=>0x04bc6de981a0: fa fa 00 00 00 00 00 00 fa fa fd fd fd fd[fd]fd
0x04bc6de981b0: fa fa fd fd fd fd fd fa fa fa 00 00 00 00 00 fa
0x04bc6de981c0: fa fa fd fd fd fd fd fa fa fa fd fd fd fd fd fa
0x04bc6de981d0: fa fa fd fd fd fd fd fa fa fa fd fd fd fd fd fa
0x04bc6de981e0: fa fa fd fd fd fd fd fa fa fa fd fd fd fd fd fa
0x04bc6de981f0: fa fa 00 00 00 00 00 fa fa fa 00 00 00 00 00 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
==3780==ABORTING

Flags: sec-bounty?

Looks like some kind of shutdown issue, where content is somehow running after we clear a pref. There was a similar one from the same reporter. As with before, I think in practice this won't happen because we exit content processes early. Maybe we need to change fuzzing builds to not build with NS_FREE_PERMANENT_DATA to avoid seeing these.

Group: firefox-core-security → layout-core-security
Component: Security → DOM: CSS Object Model
Product: Firefox → Core
See Also: → 1727259

CC'ing Dan. I don't think there's anything actionable for the i18n team here - the LanguageAtomService stack entry seems like it just is one of the things not freed by the process, but in case it bubbles up, Dan is architecting the LanguageTag unification.

(In reply to Zibi Braniecki [:zbraniecki][:gandalf] from comment #2)

CC'ing Dan. I don't think there's anything actionable for the i18n team here - the LanguageAtomService stack entry seems like it just is one of the things not freed by the process, but in case it bubbles up, Dan is architecting the LanguageTag unification.

StaticPresData keeps a raw weak pointer to nsLanguageAtomService, which is freed on shutdown.

So the main question is why are we running content script after shutdown? But also StaticPresData::Shutdown() seems improperly ordered withnsLanguageAtomService` shutdown.

I see some similar UAFs in Thunderbird. bp-24bf3c84-7106-4a71-87f1-c1ee30210903

(The Firefox one is in the main process.)

Seems like a shutdown issue in i18n.

Component: DOM: CSS Object Model → Internationalization

Hi Jonathan, do you have some time to look at this? It appears that you were the last to think about how to handle shutdown for nsLanguageAtomService, although it was many years ago [1].

[1] https://searchfox.org/mozilla-central/rev/1761c710b035d7dc8892dfa8e56589b4e095221f/intl/locale/nsLanguageAtomService.cpp#88

Flags: needinfo?(jkew)

Hmm, given that nsLayoutStatics can survive past the xpcom-shutdown notification, I guess it's wrong for nsLanguageAtomService to rely on ClearOnShutdown to clean up.

Instead of using ClearOnShutdown, maybe we could give nsLanguageAtomService a static Shutdown method that we explicitly call from nsLayoutStatics::Shutdown (like many other things).

Or we could make it refcounted, and let StaticPresData hold a strong reference, but maybe that's overkill here. I'll see if I can reproduce this locally and test a fix. Leaving ni? on myself to look into it.

Flags: needinfo?(jkew) → needinfo?(jfkthame)

Hmm, I think you may have attached the wrong archive; there's no ff.test.js in the zipfile here, but I do see a file ch.test.js which looks like it is intended for Chrome.

Flags: needinfo?(jfkthame) → needinfo?(m.cooolie)
Crash Signature: [@ PLDHashTable::Search | nsLanguageAtomService::GetLanguageGroup ]
Status: UNCONFIRMED → NEW
Ever confirmed: true
Attached file ff.test.js

sorry for that,my mistake.

node ff.test.js D:\firefox_asan\target\firefox\firefox.exe http://localhost/poc.html

Flags: needinfo?(m.cooolie)

(In reply to Jonathan Kew (:jfkthame) from comment #8)

Instead of using ClearOnShutdown, maybe we could give nsLanguageAtomService a static Shutdown method that we explicitly call from nsLayoutStatics::Shutdown (like many other things).

This seems to work as expected; with such a change, I no longer see the error here.

However, ASAN then reports a similar issue with nsNameSpaceManager:

==128681==ERROR: AddressSanitizer: heap-use-after-free on address 0x608000002fc0 at pc 0x7f1339d1798c bp 0x7ffd033176c0 sp 0x7ffd033176b8
READ of size 8 at 0x608000002fc0 thread T0 (Web Content)
    #0 0x7f1339d1798b in PLDHashTable::EntryStore::IsAllocated() const /home/jkew/mozdev/mozilla-unified/xpcom/ds/PLDHashTable.h:325:41
    #1 0x7f1339d1798b in PLDHashTable::Search(void const*) const /home/jkew/mozdev/mozilla-unified/xpcom/ds/PLDHashTable.cpp:494:20
    #2 0x7f133dee1533 in nsTHashtable<nsBaseHashtableET<nsRefPtrHashKey<nsAtom>, int> >::GetEntry(nsAtom*) const /home/jkew/mozdev/mozilla-unified/objdir-ff-asan/dist/include/nsTHashtable.h:289:16
    #3 0x7f133dee1533 in nsBaseHashtable<nsRefPtrHashKey<nsAtom>, int, int, nsDefaultConverter<int, int> >::Get(nsAtom*, int*) const /home/jkew/mozdev/mozilla-unified/xpcom/ds/nsBaseHashtable.h:328:28
    #4 0x7f133dee1533 in nsNameSpaceManager::RegisterNameSpace(already_AddRefed<nsAtom>, int&) /home/jkew/mozdev/mozilla-unified/dom/base/nsNameSpaceManager.cpp:103:22
    #5 0x7f133dee1533 in nsNameSpaceManager::RegisterNameSpace(nsTSubstring<char16_t> const&, int&) /home/jkew/mozdev/mozilla-unified/dom/base/nsNameSpaceManager.cpp:91:10
    #6 0x7f133d7b97de in nsContentUtils::GetNodeInfoFromQName(nsTSubstring<char16_t> const&, nsTSubstring<char16_t> const&, nsNodeInfoManager*, unsigned short, mozilla::dom::NodeInfo**) /home/jkew/mozdev/mozilla-unified/dom/base/nsContentUtils.cpp:3363:22
    #7 0x7f133db27fc9 in mozilla::dom::Document::CreateElementNS(nsTSubstring<char16_t> const&, nsTSubstring<char16_t> const&, mozilla::dom::ElementCreationOptionsOrString const&, mozilla::ErrorResult&) /home/jkew/mozdev/mozilla-unified/dom/base/Document.cpp:8282:8
    #8 0x7f1343127c2a in NS_NewDOMDocument(mozilla::dom::Document**, nsTSubstring<char16_t> const&, nsTSubstring<char16_t> const&, mozilla::dom::DocumentType*, nsIURI*, nsIURI*, nsIPrincipal*, bool, nsIGlobalObject*, DocumentFlavor) /home/jkew/mozdev/mozilla-unified/dom/xml/XMLDocument.cpp:157:12
    #9 0x7f133d9d63a1 in mozilla::dom::DOMImplementation::CreateDocument(nsTSubstring<char16_t> const&, nsTSubstring<char16_t> const&, mozilla::dom::DocumentType*, mozilla::dom::Document**) /home/jkew/mozdev/mozilla-unified/dom/base/DOMImplementation.cpp:100:8
    #10 0x7f133d9d69aa in mozilla::dom::DOMImplementation::CreateDocument(nsTSubstring<char16_t> const&, nsTSubstring<char16_t> const&, mozilla::dom::DocumentType*, mozilla::ErrorResult&) /home/jkew/mozdev/mozilla-unified/dom/base/DOMImplementation.cpp:128:9
    #11 0x7f133fc2a184 in mozilla::dom::DOMImplementation_Binding::createDocument(JSContext*, JS::Handle<JSObject*>, void*, JSJitMethodCallArgs const&) /home/jkew/mozdev/mozilla-unified/objdir-ff-asan/dom/bindings/DOMImplementationBinding.cpp:166:75
    #12 0x7f13404f290a in bool mozilla::dom::binding_detail::GenericMethod<mozilla::dom::binding_detail::NormalThisPolicy, mozilla::dom::binding_detail::ThrowExceptions>(JSContext*, unsigned int, JS::Value*) /home/jkew/mozdev/mozilla-unified/dom/bindings/BindingUtils.cpp:3300:13
    #13 0x3d8d3096c401  (<unknown module>)

0x608000002fc0 is located 32 bytes inside of 96-byte region [0x608000002fa0,0x608000003000)
freed by thread T0 (Web Content) here:
    #0 0x5636077c6e22 in free /builds/worker/fetches/llvm-project/llvm/projects/compiler-rt/lib/asan/asan_malloc_linux.cpp:127:3
    #1 0x7f1339c35e8b in mozilla::KillClearOnShutdown(mozilla::ShutdownPhase) /home/jkew/mozdev/mozilla-unified/xpcom/base/ClearOnShutdown.cpp:55:19
    #2 0x7f1339f567ac in mozilla::ShutdownXPCOM(nsIServiceManager*) /home/jkew/mozdev/mozilla-unified/xpcom/build/XPCOMInit.cpp:650:5
    #3 0x7f1348a70d8c in XRE_TermEmbedding() /home/jkew/mozdev/mozilla-unified/toolkit/xre/nsEmbedFunctions.cpp:218:3
    #4 0x7f133b5df3e5 in mozilla::ipc::ScopedXREEmbed::Stop() /home/jkew/mozdev/mozilla-unified/ipc/glue/ScopedXREEmbed.cpp:90:5
    #5 0x7f1348a71808 in XRE_InitChildProcess(int, char**, XREChildData const*) /home/jkew/mozdev/mozilla-unified/toolkit/xre/nsEmbedFunctions.cpp:753:16
    #6 0x5636077fb781 in content_process_main(mozilla::Bootstrap*, int, char**) /home/jkew/mozdev/mozilla-unified/browser/app/../../ipc/contentproc/plugin-container.cpp:57:28
    #7 0x5636077fb781 in main /home/jkew/mozdev/mozilla-unified/browser/app/nsBrowserApp.cpp:327:18
    #8 0x7f13546130b2 in __libc_start_main /build/glibc-eX1tMB/glibc-2.31/csu/../csu/libc-start.c:308:16

previously allocated by thread T0 (Web Content) here:
    #0 0x5636077c708d in malloc /builds/worker/fetches/llvm-project/llvm/projects/compiler-rt/lib/asan/asan_malloc_linux.cpp:145:3
    #1 0x5636077fffdd in moz_xmalloc /home/jkew/mozdev/mozilla-unified/memory/mozalloc/mozalloc.cpp:52:15
    #2 0x7f133dedf8c2 in operator new(unsigned long) /home/jkew/mozdev/mozilla-unified/objdir-ff-asan/dist/include/mozilla/cxxalloc.h:33:10
    #3 0x7f133dedf8c2 in nsNameSpaceManager::GetInstance() /home/jkew/mozdev/mozilla-unified/dom/base/nsNameSpaceManager.cpp:39:17
    #4 0x7f133d79c681 in nsContentUtils::Init() /home/jkew/mozdev/mozilla-unified/dom/base/nsContentUtils.cpp:738:23
    #5 0x7f1344baf411 in nsLayoutStatics::Initialize() /home/jkew/mozdev/mozilla-unified/layout/build/nsLayoutStatics.cpp:163:8
    #6 0x7f1344baf2b5 in nsLayoutModuleInitialize() /home/jkew/mozdev/mozilla-unified/layout/build/nsLayoutModule.cpp:104:7
    #7 0x7f1339e6c588 in nsComponentManagerImpl::Init() /home/jkew/mozdev/mozilla-unified/xpcom/components/nsComponentManager.cpp:408:5
    #8 0x7f1339f55636 in NS_InitXPCOM /home/jkew/mozdev/mozilla-unified/xpcom/build/XPCOMInit.cpp:446:51
    #9 0x7f1348a70c97 in XRE_InitEmbedding2(nsIFile*, nsIFile*, nsIDirectoryServiceProvider*) /home/jkew/mozdev/mozilla-unified/toolkit/xre/nsEmbedFunctions.cpp:192:8
    #10 0x7f133b5df8c6 in mozilla::ipc::ScopedXREEmbed::Start() /home/jkew/mozdev/mozilla-unified/ipc/glue/ScopedXREEmbed.cpp
    #11 0x7f1342e4f8ca in mozilla::dom::ContentProcess::Init(int, char**) /home/jkew/mozdev/mozilla-unified/dom/ipc/ContentProcess.cpp:202:13
    #12 0x7f1348a717ac in XRE_InitChildProcess(int, char**, XREChildData const*) /home/jkew/mozdev/mozilla-unified/toolkit/xre/nsEmbedFunctions.cpp:715:21
    #13 0x5636077fb781 in content_process_main(mozilla::Bootstrap*, int, char**) /home/jkew/mozdev/mozilla-unified/browser/app/../../ipc/contentproc/plugin-container.cpp:57:28
    #14 0x5636077fb781 in main /home/jkew/mozdev/mozilla-unified/browser/app/nsBrowserApp.cpp:327:18
    #15 0x7f13546130b2 in __libc_start_main /build/glibc-eX1tMB/glibc-2.31/csu/../csu/libc-start.c:308:16

SUMMARY: AddressSanitizer: heap-use-after-free /home/jkew/mozdev/mozilla-unified/xpcom/ds/PLDHashTable.h:325:41 in PLDHashTable::EntryStore::IsAllocated() const
Shadow bytes around the buggy address:
  0x0c107fff85a0: fa fa fa fa 00 00 00 00 00 00 00 00 00 00 00 fa
  0x0c107fff85b0: fa fa fa fa 00 00 00 00 00 00 00 00 00 00 00 00
  0x0c107fff85c0: fa fa fa fa 00 00 00 00 00 00 00 00 00 00 00 00
  0x0c107fff85d0: fa fa fa fa 00 00 00 00 00 00 00 00 00 00 06 fa
  0x0c107fff85e0: fa fa fa fa 00 00 00 00 00 00 00 00 00 00 00 00
=>0x0c107fff85f0: fa fa fa fa fd fd fd fd[fd]fd fd fd fd fd fd fd
  0x0c107fff8600: fa fa fa fa fd fd fd fd fd fd fd fd fd fd fd fd
  0x0c107fff8610: fa fa fa fa 00 00 00 00 00 00 00 00 00 00 00 fa
  0x0c107fff8620: fa fa fa fa 00 00 00 00 00 00 00 00 00 00 02 fa
  0x0c107fff8630: fa fa fa fa 00 00 00 00 00 00 00 00 00 00 00 02
  0x0c107fff8640: fa fa fa fa 00 00 00 00 00 00 00 00 00 00 00 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
==128681==ABORTING

So we should fix that at the same time. I see that nsNameSpaceManager is refcounted, but nsContentUtils only holds a raw pointer to it. Making that a strong reference (and then dropping it in nsContentUtils::Shutdown, like it already does for a number of others) looks like it should resolve the problem.

Assignee: nobody → jfkthame
Status: NEW → ASSIGNED

(In reply to Jonathan Kew (:jfkthame) from comment #11)

So we should fix that at the same time. I see that nsNameSpaceManager is refcounted, but nsContentUtils only holds a raw pointer to it. Making that a strong reference (and then dropping it in nsContentUtils::Shutdown, like it already does for a number of others) looks like it should resolve the problem.

That sounds like the same issue that has been reported in bug 1727259, which I just CCed you on. Comment 5 has a different fix that that basically gets rid of the nsContentUtils pointer entirely.

Hah - yes, same issue (and the same reporter/essentially the same testcase as here).

I think (if I'm understanding correctly) the fix suggested there wouldn't ensure quite such tidy cleanup. The issue arises when a Document, via nsContentUtils, accesses sNameSpaceManager after the KillOnShutdown handler has cleared nsNameSpaceManager's reference. If we replace use of nsContentUtils::sNameSpaceManager with a call to nsNameSpaceManager::GetInstance(), won't that mean that in such a case, a new instance gets created and returned? I believe that will then hit this assertion when it tries to use InsertIntoShutdownList, as it's too late for that.

None of this would matter in a release build, AFAICS, but it would still leave a non-clean shutdown in debug builds.

patch 1 - Make shutdown of nsLanguageAtomService explicit instead of relying on ClearOnShutdown. r=platform-i18n-reviewers,dminor
https://hg.mozilla.org/integration/autoland/rev/51359ee5adf3773a4c7da805b62d9d692cf6993a
patch 2 - Make nsContentUtils hold a strong reference to the nsNameSpaceManager. r=emilio
https://hg.mozilla.org/integration/autoland/rev/7e31b3768b1e208ba8a5e241d6b3443a0d181c7b

https://hg.mozilla.org/mozilla-central/rev/51359ee5adf3
https://hg.mozilla.org/mozilla-central/rev/7e31b3768b1e

Group: layout-core-security → core-security-release
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → 94 Branch

Not worth an ESR78 uplift IMO given that it's nearly EOL this bug is rated sec-moderate, but please nominate this for Beta and ESR91 approval when you get a chance. It grafts cleanly as-landed.

Comment on attachment 9241185 [details]
Bug 1729642 - patch 1 - Make shutdown of nsLanguageAtomService explicit instead of relying on ClearOnShutdown. r=#platform-i18n-reviewers

Beta/Release Uplift Approval Request

  • User impact if declined: Possible use-after-free (during shutdown), which could result in a crash (or possibly other effects?).
  • 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: (fixes verified in ASAN build using reporter's testcase; no known STR for a standard build)
  • List of other uplifts needed: None
  • Risk to taking this patch: Low
  • Why is the change risky/not risky? (and alternatives if risky): Just defers shutdown of the service, in case there's still a Document that tries to use it
  • String changes made/needed:
Attachment #9241185 - Flags: approval-mozilla-esr91?
Attachment #9241185 - Flags: approval-mozilla-beta?
Attachment #9241186 - Flags: approval-mozilla-beta?

Comment on attachment 9241185 [details]
Bug 1729642 - patch 1 - Make shutdown of nsLanguageAtomService explicit instead of relying on ClearOnShutdown. r=#platform-i18n-reviewers

Approved for 93 beta 8, thanks.

Attachment #9241185 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Attachment #9241186 - Flags: approval-mozilla-beta? → approval-mozilla-beta+

Comment on attachment 9241185 [details]
Bug 1729642 - patch 1 - Make shutdown of nsLanguageAtomService explicit instead of relying on ClearOnShutdown. r=#platform-i18n-reviewers

Approved for 91.2esr.

Attachment #9241185 - Flags: approval-mozilla-esr91? → approval-mozilla-esr91+
Attachment #9241186 - Flags: approval-mozilla-esr91+
QA Whiteboard: [post-critsmash-triage]
Flags: qe-verify-
Flags: sec-bounty? → sec-bounty+
Whiteboard: [reporter-external] [client-bounty-form] [verif?] → [reporter-external] [client-bounty-form] [verif?][adv-main93+]
Whiteboard: [reporter-external] [client-bounty-form] [verif?][adv-main93+] → [reporter-external] [client-bounty-form] [verif?][adv-main93+][adv-esr91.2+]
Attached file advisory.txt
Alias: CVE-2021-38498
Blocks: 1727259
See Also: 1727259
Regressions: 1748520
Group: core-security-release
Regressions: 1856084
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: