1848694 - AddressSanitizer: heap-use-after-free [@ nsScriptSecurityManager::Shutdown] with READ of size 8

Status: RESOLVED FIXED

(Core :: Networking, defect, P2)

Description

[Christian Holler (:decoder)]

The attached crash information was submitted via the ASan Nightly Reporter on mozilla-central-asan-nightly revision 118.0a1-20230731215815-https://hg.mozilla.org/mozilla-central/rev/bb8659c8d955208146039b711eefdeb74a253973.

For detailed crash information, see attachment.

Comment 1

[Christian Holler (:decoder)]

Attachment: Detailed Crash Information

Comment 3

[Andrew McCreight [:mccr8]]

The use is on this line of nsScriptSecurityManager::Shutdown():
NS_IF_RELEASE(sIOService);

Looking back at the old version, the free is on this line of nsContentUtils::Shutdown():
NS_IF_RELEASE(sIOService);

That's two different sIOService. Both have the type nsIIOService*.

nsScriptSecurityManager does:
CallGetService(NS_IOSERVICE_CONTRACTID, &sIOService);

nsContentUtils does:
nsresult rv = CallGetService(NS_IOSERVICE_CONTRACTID, &sIOService);

That all looks fine to me (though I'd prefer if this had some kind of refptr), so I'm not sure what can be going wrong here. The IO service uses threadsafe refcounting, but the setting and clearing of the two sIOService is only done in various statics things so I can't imagine those being touched from multiple threads. Maybe there's some weird error state? I noticed that nsScriptSecurityManager doesn't clear on error, and that's the one that is reading, but I'm not sure how we'd be setting sIOService with an error and not getting an addref.

I'll move this to XPCOM because it looks related to component manager shutdown stuff. This is very late in shutdown so it doesn't seem super exploitable, in any event.

Comment 4

[Andrew McCreight [:mccr8]]

Another possibility is that some other place in the code base did a release without an addref. That could be exploitable, if it could be done more than once to actually destroy the object prior to shutdown. I'm not sure how to find somewhere like that.

Comment 5

[Andrew McCreight [:mccr8]]

Actually, I'll move this over to networking as it feels like it is more of a problem with something handling nsIOService than with these two specific variables, but I could be wrong.

Comment 6

[Valentin Gosu [:valentin] (he/him)]

Not really sure how the refcount for the IO service could get messed up.
I think we definitely want to convert the pointers in nsScriptSecurityManager and nsContentUtils to staticRefPtr.

The other thing would be we might want to make this return an already addreffed
https://searchfox.org/mozilla-central/rev/a7e33b7f61e7729e2b1051d2a7a27799f11a5de6/dom/base/nsContentUtils.h#829

static nsIIOService* GetIOService() { return sIOService; }

Comment 7

[Valentin Gosu [:valentin] (he/him)]

Attachment: Bug 1848694 - Remove/avoid global references to nsIIOService r=#necko,mccr8

This patch removes the static pointer to nsIIOService in nsContentUtils,
replacing it to calls to mozilla::components::IO::Service.

It also makes nsScriptSecurityManager::sIOService a StaticRefPtr.

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/b858a0740582
Remove/avoid global references to nsIIOService r=mccr8,necko-reviewers,kershaw

Comment 9

[Sandor Molnar[:smolnar]]

Backed out for causing build bustages on dom/base/Element.cpp

Backout link: https://hg.mozilla.org/integration/autoland/rev/942c70f5f13417a12774f1375bc1998f770ae8f7

Push with failures

Failure log -> /builds/worker/checkouts/gecko/dom/base/Element.cpp:3267:30: error: no member named 'components' in namespace 'mozilla'

Comment 10

[Valentin Gosu [:valentin] (he/him)]

Some of the crashes for this signature actually hang on NS_IF_RELEASE(sIOService) so I think we should be tracking this signature.
Very low volume, though, so I think we can't call it fixed until it gets to release.

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/a92935d3b90b
Remove/avoid global references to nsIIOService r=mccr8,necko-reviewers,kershaw

Comment 12

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

https://hg.mozilla.org/mozilla-central/rev/a92935d3b90b

Comment 13

[Ryan VanderMeulen [:RyanVM]]

Please nominate this for ESR115 approval when you get a chance.

Comment 14

[Valentin Gosu [:valentin] (he/him)]

Comment on attachment 9354107 [details]
Bug 1848694 - Remove/avoid global references to nsIIOService r=#necko,mccr8

ESR Uplift Approval Request

• If this is not a sec:{high,crit} bug, please state case for ESR consideration: Speculative fix for UAF on global pointer.
• User impact if declined: It will take longer to confirm if this change succeeded in fixing the crash.
  If the root cause of this bug is actually a wrong reference count, it might not actually fix the crash or it might just move it to a different location.
• Fix Landed on Version: 119
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): The change replaces the use of manual reference counting with a StaticRefPtr, and removes nsContentUtils::sIOService which could have potentially been accessed on multiple threads.

Comment 15

[Donal Meehan [:dmeehan]]

Comment on attachment 9354107 [details]
Bug 1848694 - Remove/avoid global references to nsIIOService r=#necko,mccr8

Approved for 115.4esr.

Comment 16

[Pulsebot]

https://hg.mozilla.org/releases/mozilla-esr115/rev/66aba039ef62

Comment 17

[Daniel Veditz [:dveditz]]

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