1914106 - global-buffer-overflow in [@ mozilla::StyleSheet::StyleSheetLoaded]

Status: RESOLVED FIXED

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

Description

[Tyson Smith [:tsmith]]

Found with m-c 20240820-da046994a08d (--enable-address-sanitizer --enable-fuzzing)

This was found by visiting a live website with an ASan build.

STR:

• Launch browser and visit site

This issue was triggered by visiting http://nicbrain.com.br/.

==1424==ERROR: AddressSanitizer: global-buffer-overflow on address 0x7ff93b641c68 at pc 0x7ff9364813df bp 0x006f731fd270 sp 0x006f731fd2b8
READ of size 8 at 0x7ff93b641c68 thread T0
    #0 0x7ff9364813de in nsTArray_SafeElementAtHelper<mozilla::dom::CSSImportRule *,nsTArray_Impl<mozilla::dom::CSSImportRule *,nsTArrayInfallibleAllocator> >::SafeElementAt /builds/worker/workspace/obj-build/dist/include/nsTArray.h:320
    #1 0x7ff9364813de in mozilla::StyleSheet::GetOwnerRule /builds/worker/workspace/obj-build/dist/include/mozilla/StyleSheet.h:277
    #2 0x7ff9364813de in mozilla::StyleSheet::StyleSheetLoaded(class mozilla::StyleSheet *, bool, enum nsresult) /builds/worker/checkouts/gecko/layout/style/StyleSheet.cpp:852
    #3 0x7ff9364375e3 in mozilla::css::Loader::NotifyObservers(class mozilla::css::SheetLoadData &, enum nsresult) /builds/worker/checkouts/gecko/layout/style/Loader.cpp:1732
    #4 0x7ff93646bc7e in mozilla::SharedStyleSheetCache::LoadCompleted(class mozilla::SharedStyleSheetCache *, class mozilla::css::SheetLoadData &, enum nsresult) /builds/worker/checkouts/gecko/layout/style/SharedStyleSheetCache.cpp:68
    #5 0x7ff9364513d1 in mozilla::css::Loader::SheetComplete /builds/worker/checkouts/gecko/layout/style/Loader.cpp:1760
    #6 0x7ff9364513d1 in mozilla::css::SheetLoadData::SheetFinishedParsingAsync /builds/worker/workspace/obj-build/dist/include/mozilla/css/SheetLoadData.h:260
    #7 0x7ff9364513d1 in mozilla::css::Loader::ParseSheet::<lambda_19>::operator() /builds/worker/checkouts/gecko/layout/style/Loader.cpp:1693
    #8 0x7ff9364513d1 in mozilla::MozPromise<bool,bool,1>::InvokeMethod /builds/worker/workspace/obj-build/dist/include/mozilla/MozPromise.h:652
    #9 0x7ff9364513d1 in mozilla::MozPromise<bool,bool,1>::InvokeCallbackMethod /builds/worker/workspace/obj-build/dist/include/mozilla/MozPromise.h:683
    #10 0x7ff9364513d1 in mozilla::MozPromise<bool,bool,1>::ThenValue<`lambda at /builds/worker/checkouts/gecko/layout/style/Loader.cpp:1691:11',`lambda at /builds/worker/checkouts/gecko/layout/style/Loader.cpp:1695:11'>::DoResolveOrRejectInternal /builds/worker/workspace/obj-build/dist/include/mozilla/MozPromise.h:857
    #11 0x7ff92ff3660b in mozilla::MozPromise<bool, bool, 1>::ThenValueBase::ResolveOrRejectRunnable::Run(void) /builds/worker/workspace/obj-build/dist/include/mozilla/MozPromise.h:488
    #12 0x7ff92c536c4e in mozilla::RunnableTask::Run(void) /builds/worker/checkouts/gecko/xpcom/threads/TaskController.cpp:618
    #13 0x7ff92c518fc1 in mozilla::TaskController::DoExecuteNextTaskOnlyMainThreadInternal(class mozilla::detail::BaseAutoLock<class mozilla::Mutex &> const &) /builds/worker/checkouts/gecko/xpcom/threads/TaskController.cpp:945
    #14 0x7ff92c51597a in mozilla::TaskController::ExecuteNextTaskOnlyMainThreadInternal(class mozilla::detail::BaseAutoLock<class mozilla::Mutex &> const &) /builds/worker/checkouts/gecko/xpcom/threads/TaskController.cpp:768
    #15 0x7ff92c516274 in mozilla::TaskController::ProcessPendingMTTask(bool) /builds/worker/checkouts/gecko/xpcom/threads/TaskController.cpp:554
    #16 0x7ff92c53a6c1 in mozilla::TaskController::TaskController::<lambda_5>::operator() /builds/worker/checkouts/gecko/xpcom/threads/TaskController.cpp:268
    #17 0x7ff92c53a6c1 in mozilla::detail::RunnableFunction<`lambda at /builds/worker/checkouts/gecko/xpcom/threads/TaskController.cpp:268:7'>::Run /builds/worker/checkouts/gecko/xpcom/threads/nsThreadUtils.h:548
    #18 0x7ff92c56168c in nsThread::ProcessNextEvent(bool, bool *) /builds/worker/checkouts/gecko/xpcom/threads/nsThread.cpp:1204
    #19 0x7ff92c570170 in NS_ProcessNextEvent(class nsIThread *, bool) /builds/worker/checkouts/gecko/xpcom/threads/nsThreadUtils.cpp:480
    #20 0x7ff92d985047 in mozilla::ipc::MessagePump::Run(class base::MessagePump::Delegate *) /builds/worker/checkouts/gecko/ipc/glue/MessagePump.cpp:85
    #21 0x7ff92d8b2ba3 in MessageLoop::RunInternal /builds/worker/checkouts/gecko/ipc/chromium/src/base/message_loop.cc:370
    #22 0x7ff92d8b2ba3 in MessageLoop::RunHandler(void) /builds/worker/checkouts/gecko/ipc/chromium/src/base/message_loop.cc:363
    #23 0x7ff92d8b2a06 in MessageLoop::Run(void) /builds/worker/checkouts/gecko/ipc/chromium/src/base/message_loop.cc:345
    #24 0x7ff935d5632c in nsBaseAppShell::Run(void) /builds/worker/checkouts/gecko/widget/nsBaseAppShell.cpp:148
    #25 0x7ff935f61959 in nsAppShell::Run(void) /builds/worker/checkouts/gecko/widget/windows/nsAppShell.cpp:655
    #26 0x7ff937c8be8c in XRE_RunAppShell(void) /builds/worker/checkouts/gecko/toolkit/xre/nsEmbedFunctions.cpp:710
    #27 0x7ff92d8b2ba3 in MessageLoop::RunInternal /builds/worker/checkouts/gecko/ipc/chromium/src/base/message_loop.cc:370
    #28 0x7ff92d8b2ba3 in MessageLoop::RunHandler(void) /builds/worker/checkouts/gecko/ipc/chromium/src/base/message_loop.cc:363
    #29 0x7ff92d8b2a06 in MessageLoop::Run(void) /builds/worker/checkouts/gecko/ipc/chromium/src/base/message_loop.cc:345
    #30 0x7ff937c8b583 in XRE_InitChildProcess(int, char **const, struct XREChildData const *) /builds/worker/checkouts/gecko/toolkit/xre/nsEmbedFunctions.cpp:645
    #31 0x7ff7194b20cb in NS_internal_main(int, char **, char **) /builds/worker/checkouts/gecko/browser/app/nsBrowserApp.cpp:403
    #32 0x7ff7194b142b in wmain /builds/worker/checkouts/gecko/toolkit/xre/nsWindowsWMain.cpp:151
    #33 0x7ff71957fe17 in invoke_main D:\a\_work\1\s\src\vctools\crt\vcstartup\src\startup\exe_common.inl:90
    #34 0x7ff71957fe17 in __scrt_common_main_seh D:\a\_work\1\s\src\vctools\crt\vcstartup\src\startup\exe_common.inl:288
    #35 0x7ff971a14caf  (C:\Windows\System32\KERNEL32.DLL+0x180014caf)
    #36 0x7ff97305ecea  (C:\Windows\SYSTEM32\ntdll.dll+0x18007ecea)

0x7ff93b641c68 is located 56 bytes before global variable '"mData"' defined in 'Unified_cpp_xpcom_ds1.cpp' (0x7ff93b641ca0) of size 6
  '"mData"' is ascii string 'mData'
0x7ff93b641c68 is located 24 bytes before global variable '"%c"' defined in 'Unified_cpp_xpcom_ds1.cpp' (0x7ff93b641c80) of size 3
  '"%c"' is ascii string '%c'
0x7ff93b641c68 is located 0 bytes after global variable 'sEmptyTArrayHeader' defined in 'Unified_cpp_xpcom_ds1.cpp' (0x7ff93b641c60) of size 8
SUMMARY: AddressSanitizer: global-buffer-overflow /builds/worker/workspace/obj-build/dist/include/nsTArray.h:320 in nsTArray_SafeElementAtHelper<mozilla::dom::CSSImportRule *,nsTArray_Impl<mozilla::dom::CSSImportRule *,nsTArrayInfallibleAllocator> >::SafeElementAt
Shadow bytes around the buggy address:
  0x7ff93b641980: 00 00 f9 f9 00 00 00 00 00 00 00 f9 f9 f9 f9 f9
  0x7ff93b641a00: 00 00 00 00 00 00 00 00 f9 f9 f9 f9 00 00 f9 f9
  0x7ff93b641a80: 00 00 00 00 00 00 00 00 00 f9 f9 f9 f9 f9 f9 f9
  0x7ff93b641b00: 00 00 04 f9 f9 f9 f9 f9 00 00 00 00 00 03 f9 f9
  0x7ff93b641b80: f9 f9 f9 f9 00 00 00 00 00 00 00 00 f9 f9 f9 f9
=>0x7ff93b641c00: 00 00 00 00 00 00 00 f9 f9 f9 f9 f9 00[f9]f9 f9
  0x7ff93b641c80: 03 f9 f9 f9 06 f9 f9 f9 00 01 f9 f9 00 00 00 00
  0x7ff93b641d00: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x7ff93b641d80: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x7ff93b641e00: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x7ff93b641e80: 00 00 00 00 00 00 00 f9 f9 f9 f9 f9 f9 f9 f9 f9
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

Comment 1

[Andrew McCreight [:mccr8]]

Visiting that site in a local debug build on MacOS, I hit an assertion:

Assertion failure: aSheet->GetOwnerRule(), at layout/style/StyleSheet.cpp:851
#01: mozilla::StyleSheet::StyleSheetLoaded(mozilla::StyleSheet*, bool, nsresult) (layout/style/StyleSheet.cpp:0)
#02: mozilla::css::Loader::NotifyObservers(mozilla::css::SheetLoadData&, nsresult) (layout/style/Loader.cpp:1729)
#03: mozilla::SharedStyleSheetCache::LoadCompleted(mozilla::SharedStyleSheetCache*, mozilla::css::SheetLoadData&, nsresult) (layout/style/SharedStyleSheetCache.cpp:66)
#04: mozilla::MozPromise<bool, bool, true>::ThenValue<mozilla::css::Loader::ParseSheet(nsTSubstring<char> const&, RefPtr<nsMainThreadPtrHolder<mozilla::css::SheetLoadData> > const&, mozilla::css::Loader::AllowAsyncParse)::$_0, mozilla::css::Loader::ParseSheet(nsTSubstring<char> const&, RefPtr<nsMainThreadPtrHolder<mozilla::css::SheetLoadData> > const&, mozilla::css::Loader::AllowAsyncParse)::$_1>::DoResolveOrRejectInternal(mozilla::MozPromise<bool, bool, true>::ResolveOrRejectValue&) (obj-dbg.noindex/dist/include/mozilla/MozPromise.h:870)
#05: mozilla::MozPromise<bool, bool, true>::ThenValueBase::ResolveOrRejectRunnable::Run() (obj-dbg.noindex/dist/include/mozilla/MozPromise.h:489)

Comment 2

[Andrew McCreight [:mccr8]]

The crash is on this line:

return mReferencingRules.SafeElementAt(0);

The log has this:
is located 0 bytes after global variable 'sEmptyTArrayHeader'

So I guess this is an empty array? Seems unfortunate that a use of SafeElementAt is causing a buffer overflow of some sort. Nika, is that expected?

Comment 3

[Nika Layzell [:nika] (ni? for response)]

(In reply to Andrew McCreight [:mccr8] from comment #2)

The crash is on this line:

return mReferencingRules.SafeElementAt(0);

The log has this:
is located 0 bytes after global variable 'sEmptyTArrayHeader'

So I guess this is an empty array? Seems unfortunate that a use of SafeElementAt is causing a buffer overflow of some sort. Nika, is that expected?

Intuitively this shouldn't happen, as sEmptyTArrayHeader should always have a length of 0 (and is const, so is mapped in a read-only page). SafeElementAt should then notice that 0 < 0 is false, and return the default value (which is nullptr).

I'm pretty sure that in this case, compiler undefined-behaviour optimizations are what are responsible.

The call-site, looks like this:

  NOTIFY(ImportRuleLoaded, (*aSheet->GetOwnerRule(), *aSheet));

In this case, it immediately dereferences the return value from GetOwnerRule(). Clang therefore infers due to UB that GetOwnerRule must return a non-null value, and the entire length check is omitted, leading to an unsafe buffer overrun access replacing a safe null pointer dereference sEmptyTArrayHeader.

This is one of those fun unintuitive undefined-behaviour optimizations that leads to bounds checks being omitted.

Comment 4

[Emilio Cobos Álvarez (:emilio)]

Attachment: Reduced test-case

We kick of the (valid) @import load, but then we throw because we have garbage afterwards, so we never actually insert a DOM rule...

Comment 5

[Emilio Cobos Álvarez (:emilio)]

Given none of the called functions use that read, I think this shouldn't be exploitable.

Comment 6

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1914106 - Deal with insertRule edge-case. r=#style

When there's trailing garbage after an @import rule we throw, but we
still trigger the load (that's not great but not trivial to change).

Deal with that case before calling ImportRuleLoaded().

Comment 7

[Andrew McCreight [:mccr8]]

We're trying to access the first element of the empty array, which is placed in a read-only page as a hardening measure, so I don't think any real harm can come of this.

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/2ec3ea09d581
Deal with insertRule edge-case. r=jwatt

Comment 9

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/2ec3ea09d581

Comment 10

[Ryan VanderMeulen [:RyanVM]]

Please nominate this for ESR128 approval when you get a chance. It grafts cleanly.

Comment 11

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 9420203 [details]
Bug 1914106 - Deal with insertRule edge-case. r=#style

ESR Uplift Approval Request

• If this is not a sec:{high,crit} bug, please state case for ESR consideration: Low risk sec-low
• User impact if declined: none
• Fix Landed on Version:
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): Trivial fix

Comment 12

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 9420203 [details]
Bug 1914106 - Deal with insertRule edge-case. r=#style

Approved for 128.3esr.

Comment 13

[Pulsebot]

https://hg.mozilla.org/releases/mozilla-esr128/rev/ad96b60231e8