1868901 - (CVE-2023-6863) Undefined behavior in ShutdownObserver()

Status: RESOLVED FIXED

(Core :: Graphics, defect)

Description

[mozillabugs]

ShutdownObserver (widget/GfxInfoBase.cpp) invokes undefined behavior by deleting an object whose static type lacks a virtual destructor, where the object has a derived-class dynamic type. [1]

The issue concerns the three arrays in widget/GfxDriverInfo.h (line numbers are from trunk):

    351: static nsAString* sWindowProtocol[static_cast<size_t>(WindowProtocol::Max)];
    ...
    355: static nsAString* sDeviceVendors[static_cast<size_t>(DeviceVendor::Max)];
    ...
    358: static nsAString* sDriverVendors[static_cast<size_t>(DriverVendor::Max)];

These arrays' elements are assigned nsString* pointers by GfxDriverInfo::GetWindowProtocol(), GfxDriverInfo::GetDeviceVendor(), and GfxDriverInfo::GetDriverVendor(), respectively, e.g. from widget/GfxDriverInfo.cpp:

    602: const nsAString& GfxDriverInfo::GetWindowProtocol(WindowProtocol id) {
    ...
    613:  sWindowProtocol[idx] = new nsString();

and are deleted by ShutdownObserver::Observe() (widget/GfxInfoBase.cpp):

71: NS_IMETHOD Observe(nsISupports* subject, const char* aTopic,
72:                     const char16_t* aData) override {
...
83:    for (auto& windowProtocol : GfxDriverInfo::sWindowProtocol) {
84:      delete windowProtocol;
85:      windowProtocol = nullptr;
86:    }
87:
88:    for (auto& deviceVendor : GfxDriverInfo::sDeviceVendors) {
89:      delete deviceVendor;
90:      deviceVendor = nullptr;
91:    }
92:
93:    for (auto& driverVendor : GfxDriverInfo::sDriverVendors) {
94:      delete driverVendor;
95:      driverVendor = nullptr;
96:    }

But nsString is an alias for nsTString<char16_t>, which isa nsTSubstring<char16_t>. Whereas, nsAString is an alias for nsTSubstring<char16_t>. So the assignment is of a derived class pointer to a base class pointer. But the base class nsTSubstring<char16_t> lacks a virtual destructor, despite the quizzical comment from xpcom/string/nsTSubstring.h:

    325:  // this acts like a virtual destructor
    326:  ~nsTSubstring() { Finalize(); }

[1] See C++ 20 Standard (n4750) s.8.5.2.5(3) : "In a single-object delete expression, if the static type of the object to be deleted is different from its dynamic type, the static type shall be a base class of the dynamic type of the object to be deleted and the static type shall have a virtual destructor or the behavior is undefined."

Comment 1

[Andrew McCreight [:mccr8]]

This code is in the widget/ directory, but it looks like it is all about the graphics blocklist, so I'll move it to graphics.

Comment 2

[Andrew Osmond [:aosmond] (he/him)]

This seems easily solved by just using the actual type in the static variable declarations.

Comment 3

[Andrew Osmond [:aosmond] (he/him)]

While in general we pass around references to nsAString/nsACString, I don't think we normally store pointers to nsAString/nsACString requiring a delete after. Instead we generally use the nsString/nsCString/nsAutoString/nsAutoCString objects when declaring the actual storage.

Comment 4

[Andrew Osmond [:aosmond] (he/him)]

Attachment: Bug 1868901.

Comment 5

[Andrew McCreight [:mccr8]]

I'm going to assume this isn't particularly dangerous in practice.

Comment 6

[Andrew Osmond [:aosmond] (he/him)]

Comment on attachment 9367662 [details]
Bug 1868901.

Beta/Release Uplift Approval Request

• User impact if declined: Minor shutdown security issue
• Is this code covered by automated tests?: Yes
• 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): We just change the declaration to use the concrete derived class instead of the base class. Trivial change.
• String changes made/needed:
• Is Android affected?: Yes

ESR Uplift Approval Request

• If this is not a sec:{high,crit} bug, please state case for ESR consideration: Minor shutdown security issue
• User impact if declined: Minor shutdown security issue
• Fix Landed on Version: 122
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): We just change the declaration to use the concrete derived class instead of the base class. Trivial change.

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/42f4ed33c46d
r=gfx-reviewers,lsalzman

Comment 8

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

https://hg.mozilla.org/mozilla-central/rev/42f4ed33c46d

Comment 9

[Ryan VanderMeulen [:RyanVM][PTO June 24-28]]

Comment on attachment 9367662 [details]
Bug 1868901.

Approved for 121.0rc1 and 115.6esr.

Comment 10

[Pulsebot]

https://hg.mozilla.org/releases/mozilla-beta/rev/ab6d9d1dae75

Comment 11

[Pulsebot]

https://hg.mozilla.org/releases/mozilla-esr115/rev/3c6e8e9d0084

Comment 12

[Jason Kratzer [:jkratzer]]

Attachment: advisory.txt

Comment 13

[Daniel Veditz [:dveditz]]

Bulk-unhiding security bugs fixed in Firefox 119-121 (Fall 2023). Use "moo-doctrine-subsidy" to filter