1811327 - (CVE-2023-28162) cfi-derived-cast: Invalid downcast in ExecutionRunnable::RunOnWorkletThread

Status: RESOLVED FIXED

(Core :: Audio/Video, defect)

Description

[lukas.bernhard]

Steps to reproduce:

When compiling Firefox with cfi-derived-cast, crashtest dom/media/test/crashtests/audioworkletnode-after-unload-1.html fails due to the downcasting here:
https://searchfox.org/mozilla-central/rev/daf613efc5c358f3a94961d73b90472c00703838/dom/worklet/Worklet.cpp#410
The dynamic type of the casted object is nsThread, a parent class of WorkletThread.

This might not be s-s, just flagging as a precaution.

Comment 1

[Olli Pettay [:smaug][[e-mail]]]

The relevant code has been implemented for web audio, so moving this bug there.

I'm not familiar with the setup, see https://searchfox.org/mozilla-central/rev/7426a35738cd542b9488c7b67f4f6d21edfeda0a/dom/worklet/Worklet.cpp#370-375

Comment 2

[Daniel Veditz [:dveditz]]

Andrew looked and it appears to only be touched by static methods rather than virtual and is probably OK until a compiler decides to go optimize some undefined behavior. Should fix.

Comment 3

[Andrew McCreight [:mccr8]]

Specifically, we first call EnsureCycleCollectedJSContext on it, which is a non-static non-virtual method that does not touch any fields. It passes this into the closure argument of JS::InitDispatchToEventLoop, which eventually ends up getting passed back in to the static method DispatchToEventLoop, which casts it back to WorkletThread and calls DispatchRunnable on it, which is a non-static, non-virtual method, that also does not touch any fields.

Comment 4

[Andrew McCreight [:mccr8]]

I started throwing together a patch to fix this, and it looks like I was wrong. In WorkletThread::DispatchRunnable(), nsThread::Dispatch() is actually a virtual method on the parent class nsThread. I guess it works out anyways because NS_GetCurrentThread() returns an nsThread and I suppose there's no weird multiple inheritance so the vtables are compatible at least for that one method?

Comment 5

[Andrew McCreight [:mccr8]]

I'm guessing it must be okay because otherwise it would just crash every time.

Comment 6

[Andrew McCreight [:mccr8]]

Attachment: Bug 1811327 - Be more precise in the types passed around to WorkletThread methods.

Comment 7

[Andrew McCreight [:mccr8]]

I think my patch here reflects why we don't go awry on this test case, but maybe I'm missing the forest for the trees. If we're running ExecutionRunnable::RunOnWorkletThread() on a thread that apparently isn't a worklet thread, that doesn't sound good. Maybe some error condition should have been detected earlier and bailed out?

Comment 8

[Andrew McCreight [:mccr8]]

It looks like the thread name for the thread we're calling ExecutionRunnable::RunOnWorkletThread() on is "GraphRunner".

Comment 9

[Andrew McCreight [:mccr8]]

Across all of the plain mochitests in dom/worklet/tests/, dom/worklet/tests/test_paintWorklet.html seems to be the only one where the name of the thread is "DOM Worklet" and not "GraphRunner", so I suppose that is normal.

Comment 10

[Karl Tomlinson (:karlt back Feb 10)]

https://phabricator.services.mozilla.com/D61406?id=223895#inline-371664

Comment 11

[BugBot [:suhaib / :marco/ :calixte]]

Set release status flags based on info from the regressing bug 1612570

Comment 12

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

Be more precise in the types passed around to WorkletThread methods. r=karlt
https://hg.mozilla.org/integration/autoland/rev/19576e8a0027d40601ba7f985108d8ad66cdffa8
https://hg.mozilla.org/mozilla-central/rev/19576e8a0027

Comment 13

[Andrew McCreight [:mccr8]]

Comment on attachment 9314151 [details]
Bug 1811327 - Be more precise in the types passed around to WorkletThread methods.

ESR Uplift Approval Request

• If this is not a sec:{high,crit} bug, please state case for ESR consideration: This fixes up a minor bit of undefined behavior.
• User impact if declined: Undefined behavior could in theory lead to security problems if the compiler changes.
• Fix Landed on Version: 111
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): This really just shuffles around some static casts to make them more accurate.

Comment 14

[Donal Meehan [:dmeehan]]

Comment on attachment 9314151 [details]
Bug 1811327 - Be more precise in the types passed around to WorkletThread methods.

Approved for 102.9esr.

Comment 15

[Donal Meehan [:dmeehan]]

https://hg.mozilla.org/releases/mozilla-esr102/rev/452c03b24737

Comment 16

[Frederik Braun [:freddy]]

Attachment: advisory.txt

Comment 17

[Daniel Veditz [:dveditz]]

Sorry for the burst of bugspam: filter on tinkling-glitter-filtrate
Adding reporter-external keyword to security bugs found by non-employees for accounting reasons