1432323 - UBSan: member access within address 0x6030002a4d80 which does not point to an object of type 'nsCOMArrayEnumerator' in /xpcom/ds/nsArrayEnumerator.cpp:197

Status: RESOLVED FIXED

(Core :: XPCOM, defect)

Description

[Tyson Smith [:tsmith]]

This is triggered on startup when built with -fsanitize=vptr

changeset: 400160:20e194b34185

/xpcom/ds/nsArrayEnumerator.cpp:197:26: runtime error: member access within address 0x6030002a4d80 which does not point to an object of type 'nsCOMArrayEnumerator'
0x6030002a4d80: note: object has invalid vptr
 0c 00 00 2b  e4 e4 e4 e4 e4 e4 e4 e4  e4 e4 e4 e4 e4 e4 e4 e4  e4 e4 e4 e4 e4 e4 e4 e4  e4 e4 e4 e4
              ^~~~~~~~~~~~~~~~~~~~~~~
              invalid vptr
    #0 0x7f0b44ab650d in nsCOMArrayEnumerator::operator new(unsigned long, nsCOMArray_base const&) /xpcom/ds/nsArrayEnumerator.cpp:197:26
    #1 0x7f0b44ab65f1 in NS_NewArrayEnumerator(nsISimpleEnumerator**, nsCOMArray_base const&) /xpcom/ds/nsArrayEnumerator.cpp:210:45
    #2 0x7f0b53a13859 in nsXREDirProvider::GetFilesInternal(char const*, nsISimpleEnumerator**) /toolkit/xre/nsXREDirProvider.cpp:891:10
    #3 0x7f0b53a12f5d in nsXREDirProvider::GetFiles(char const*, nsISimpleEnumerator**) /toolkit/xre/nsXREDirProvider.cpp:645:8
    #4 0x7f0b44b353f2 in FindProviderFile(nsIDirectoryServiceProvider*, FileData*) /xpcom/io/nsDirectoryService.cpp:198:19
    #5 0x7f0b44b3410f in nsDirectoryService::Get(char const*, nsID const&, void**) /xpcom/io/nsDirectoryService.cpp:249:10
    #6 0x7f0b44cff58d in mozilla::Preferences::InitInitialObjects() /modules/libpref/Preferences.cpp:4292:11
    #7 0x7f0b44cfe262 in mozilla::Preferences::GetInstanceForService() /modules/libpref/Preferences.cpp:3331:33
    #8 0x7f0b44d08c18 in PreferencesConstructor(nsISupports*, nsID const&, void**) /modules/libpref/Preferences.cpp:5084:1
    #9 0x7f0b44bb5d5f in nsComponentManagerImpl::CreateInstanceByContractID(char const*, nsISupports*, nsID const&, void**) /xpcom/components/nsComponentManager.cpp:1086:19
    #10 0x7f0b44badb4d in nsComponentManagerImpl::GetServiceByContractID(char const*, nsID const&, void**) /xpcom/components/nsComponentManager.cpp:1446:10
    #11 0x7f0b44bbc01c in nsGetServiceByContractID::operator()(nsID const&, void**) const /xpcom/components/nsComponentManagerUtils.cpp:280:21
    #12 0x7f0b449f0014 in nsCOMPtr_base::assign_from_gs_contractid(nsGetServiceByContractID, nsID const&) /xpcom/base/nsCOMPtr.cpp:95:7
    #13 0x7f0b44caa2b1 in nsChromeRegistryChrome::Init() /chrome/nsChromeRegistryChrome.cpp:117:28
    #14 0x7f0b44ca9b67 in nsChromeRegistry::GetSingleton() /chrome/nsChromeRegistry.cpp:683:7
    #15 0x7f0b44cd668c in nsChromeRegistryConstructor(nsISupports*, nsID const&, void**) /xpcom/build/XPCOMInit.cpp:278:1
    #16 0x7f0b44bb5d5f in nsComponentManagerImpl::CreateInstanceByContractID(char const*, nsISupports*, nsID const&, void**) /xpcom/components/nsComponentManager.cpp:1086:19
    #17 0x7f0b44badb4d in nsComponentManagerImpl::GetServiceByContractID(char const*, nsID const&, void**) /xpcom/components/nsComponentManager.cpp:1446:10
    #18 0x7f0b44bbc01c in nsGetServiceByContractID::operator()(nsID const&, void**) const /xpcom/components/nsComponentManagerUtils.cpp:280:21
    #19 0x7f0b449f0014 in nsCOMPtr_base::assign_from_gs_contractid(nsGetServiceByContractID, nsID const&) /xpcom/base/nsCOMPtr.cpp:95:7
    #20 0x7f0b44cc5a9f in mozilla::services::GetChromeRegistryService() /xpcom/build/ServiceList.h:8:1
    #21 0x7f0b44ba1a1c in ParseManifest(NSLocationType, mozilla::FileLocation&, char*, bool, bool) /xpcom/components/ManifestParser.cpp:752:11
    #22 0x7f0b44bb150f in DoRegisterManifest(NSLocationType, mozilla::FileLocation&, bool, bool) /xpcom/components/nsComponentManager.cpp:547:5
    #23 0x7f0b44bb17eb in nsComponentManagerImpl::ManifestManifest(nsComponentManagerImpl::ManifestProcessingContext&, int, char* const*) /xpcom/components/nsComponentManager.cpp:569:3
    #24 0x7f0b44ba1e1f in ParseManifest(NSLocationType, mozilla::FileLocation&, char*, bool, bool) /xpcom/components/ManifestParser.cpp:769:9
    #25 0x7f0b44bb150f in DoRegisterManifest(NSLocationType, mozilla::FileLocation&, bool, bool) /xpcom/components/nsComponentManager.cpp:547:5
    #26 0x7f0b44bb01e5 in nsComponentManagerImpl::RereadChromeManifests(bool) /xpcom/components/nsComponentManager.cpp:721:5
    #27 0x7f0b44baecaa in nsComponentManagerImpl::Init() /xpcom/components/nsComponentManager.cpp:352:5
    #28 0x7f0b44cc9c77 in NS_InitXPCOM2 /xpcom/build/XPCOMInit.cpp:673:51
    #29 0x7f0b539dffe6 in ScopedXPCOMStartup::Initialize() /toolkit/xre/nsAppRunner.cpp:1567:8
    #30 0x7f0b539f2c4c in XREMain::XRE_main(int, char**, mozilla::BootstrapConfig const&) /toolkit/xre/nsAppRunner.cpp:4837:22
    #31 0x7f0b539f3c41 in XRE_main(int, char**, mozilla::BootstrapConfig const&) /toolkit/xre/nsAppRunner.cpp:4933:21
    #32 0x518e1e in do_main(int, char**, char**) /browser/app/nsBrowserApp.cpp:231:22
    #33 0x5183ac in main /browser/app/nsBrowserApp.cpp:304:16
    #34 0x7f0b7b2f71c0 in __libc_start_main /build/glibc-itYbWN/glibc-2.26/csu/../csu/libc-start.c:308
    #35 0x420c99 in _start (/objdir-ff-ubsan/dist/bin/firefox+0x420c99)

Comment 1

[Eric Rahm [:erahm]]

This looks like we're poking uninitialized memory which looks plausible as we have a somewhat crazy operator new overload going on [1].

[1] https://searchfox.org/mozilla-central/rev/9a8d3f8191b15cdca89a7ce044c7bea2dd0462dc/xpcom/ds/nsArrayEnumerator.cpp#179-204

Comment 2

[Eric Rahm [:erahm]]

Marking sec for now, there are other issues.

Comment 3

[Eric Rahm [:erahm]]

Attachment: Refactor operator new in nsArrayEnumerator

It looks like we were allocating memory but never actually calling nsCOMArrayEnumerator's constructor. This refactors things a bit to actually call the constructor and prevent using the constructor directly.

Addtionally a potential integer overflow is fixed for the case where the array being enumerated has a size of zero.

Comment 4

[Eric Rahm [:erahm]]

Attachment: Refactor operator new in nsArrayEnumerator

Tyson pointed out a signed unsigned issue as well.

Comment 5

[Nathan Froyd [:froydnj]]

Comment on attachment 8953297 [details] [diff] [review]
Refactor operator new in nsArrayEnumerator

Review of attachment 8953297 [details] [diff] [review]:
-----------------------------------------------------------------

WFM.

::: xpcom/ds/nsArrayEnumerator.cpp
@@ +191,5 @@
>    // enough value. Note that the initial value of aSize gives us
>    // space for mValueArray[0], so we must subtract
> +  size_t size = sizeof(nsCOMArrayEnumerator);
> +  uint32_t count;
> +  if (aArray.Count() > 0) {

Heh.

Comment 6

[Eric Rahm [:erahm]]

Comment on attachment 8953297 [details] [diff] [review]
Refactor operator new in nsArrayEnumerator

This is currently unrated, I feel like it's a sec-moderate or sec-low. There are three issues:

#1 - We were doing a half-baked placement new without calling a real constructor so that left uninitialized variables. I think worst case scenario the index would be junk so we would start iterating at a random point. There are existing checks to make sure we don't iterate past the array size.

#2 - Integer overflow if the array size is 0. This would most likely result in an OOM, but otherwise okay.

#3 - Maybe OOB access if the array size is *negative*. This shouldn't happen but it's type is |int32_t| so yolo. That's more of a latent issue.

[Security approval request comment]
How easily could an exploit be constructed based on the patch?

Not super easy, it looks like I'm refactoring some code.

Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?

No.

Which older supported branches are affected by this flaw?

All.

If not all supported branches, which bug introduced the flaw?

N/A

Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be?

No, but the patch should apply.

How likely is this patch to cause regressions; how much testing does it need?

Pretty minimal, our test suites should exercise the code.

Comment 7

[Al Billings [:abillings - ex-MoCo]]

Calling this a sec-moderate and clearing sec-approval. Check it into trunk!

Comment 8

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

https://hg.mozilla.org/mozilla-central/rev/209ecda9242c

Comment 9

[Ryan VanderMeulen [:RyanVM]]

Too late for 59.