-
Notifications
You must be signed in to change notification settings - Fork 2.1k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[BUG] Heap-Use-After-Free (UAF) in tv-casting-app when deleting a fabric after casting #36920
Comments
@BoB13-Matter This would be an issue for any fabric removal with a live ReadClient, right? Not specific to tv-casting? |
Just looking at this issue, application would take care of the delete for readClient inside ReadClient::OnDone, in the destructor of readClient, we will take care of the object removal in mpActiveReadClientList via mpImEngine->RemoveReadClient(this); this issue should be fine? maybe tv-casting-app does not delete readClient object in ReadClient::onDone? ReadClient::~ReadClient() one improvement in readClient we can do is to reset mPeer in readClient::Close so that the below GetFabricIndex would get the wrong value. |
The problem is we are holding a reference to the now-deleted thing in the loop in InteractionModelEngine::OnFabricRemoved, no? @yunhanw-google |
@bzbarsky-apple
I think mpImEngine->RemoveReadClient(this) in ~ReadClient() has removed that readClient reference?
|
Yes.
How could it? The reference is held on the stack in the |
So the stack trace shows that Close eventually calls delete:
And the loop is for (auto * readClient = mpActiveReadClientList; readClient != nullptr; readClient = readClient->GetNextClient())
{
if (readClient->GetFabricIndex() == fabricIndex)
{
ChipLogProgress(InteractionModel, "Fabric removed, deleting obsolete read client with FabricIndex: %u", fabricIndex);
readClient->Close(CHIP_ERROR_IM_FABRIC_DELETED, false);
}
} So we are deleting |
Agree, the loop variable is pointing to deleted memory.... And the body of the for will delete the object readClient points to. So then readClient->GetNextClient() will be use-after-free. |
Just create the PR for the fix and corresponding unit test to cover, thanks |
Reproduction steps
Build the
tv-casting-app
with AddressSanitizer (ASAN) enabled.Open Terminal 1 and start the
tv-casting-app
:Open Terminal 2 and start the
tv-app
:In the
tv-casting-app
shell, send a cast request:tv-casting-app > cast request 0
In the
tv-app
shell, respond with:tv-app > controller ux ok
In the
tv-casting-app
, print the list of fabrics and delete a specific fabric:Observe the ASAN output in
tv-casting-app
indicating a heap-use-after-free issue.tv-casting-app UAF.txt
Summary
A heap-use-after-free (UAF) issue occurs in the
tv-casting-app
when thedelete-fabric
command is executed. After removing a fabric, the system attempts to access a freedReadClient
object.Analysis and Description
The issue arises from a lifecycle mismatch between fabric cleanup and the
ReadClient
object references. The sequence of events leading to the issue is as follows:Fabric Removal (FabricTable.cpp):
The
Delete()
method ofFabricTable
is called, which iterates through its delegates and invokesOnFabricRemoved()
:ReadClient Cleanup (InteractionModelEngine.cpp):
The
OnFabricRemoved()
method loops through activeReadClient
objects and callsClose()
:ReadClient Deallocation (ReadClient.cpp):
The
Close()
method triggers the callbackmpCallback.OnDone(this)
, marking theReadClient
for destruction:Use of Freed Memory:
Despite being destroyed, the
ReadClient
remains in thempActiveReadClientList
. Subsequent iterations inOnFabricRemoved()
access the dangling pointer. Specifically, thereadClient->GetNextClient()
call accesses thempNext
pointer of the destroyedReadClient
object, leading to a UAF:This happens because
GetNextClient()
does not verify the validity of thempNext
pointer, which now points to a deallocated memory region.Bug prevalence
always
GitHub hash of the SDK that was being used
04e6a68
Platform
core
Platform Version(s)
all versions
Anything else?
No response
The text was updated successfully, but these errors were encountered: