Minimal Mdns ResolverDelegateProxy use-after-free

[msandstedt]

Problem

Discovery_ImplPlatform.cpp decrements the reference counter for the ResolverDelegateProxy by calling Release, which eventually causes it to be freed. However, this doesn't necessarily un-register the freed delegate from e.g. Resolver_ImplMinimalMdns.cpp. The result is that if more records are discovered than expected, code like Resolver_ImplMinimalMdns.cpp can execute methods on the freed ResolverDelegateProxy.

Proposed Solution

Something like this might work:

diff --git a/src/lib/dnssd/ResolverProxy.h b/src/lib/dnssd/ResolverProxy.h
index 5d3683387..3484366a9 100644
--- a/src/lib/dnssd/ResolverProxy.h
+++ b/src/lib/dnssd/ResolverProxy.h
@@ -28,6 +28,12 @@ class ResolverDelegateProxy : public ReferenceCounted<ResolverDelegateProxy>, pu
 public:
     void SetDelegate(ResolverDelegate * delegate) { mDelegate = delegate; }
 
+    void Release()
+    {
+        chip::Dnssd::Resolver::Instance().SetResolverDelegate(nullptr);
+        ReferenceCounted<ResolverDelegateProxy>::Release();
+    }
+
     /// ResolverDelegate interface
     void OnNodeIdResolved(const ResolvedNodeData & nodeData) override
     {

[msandstedt]

CC @vivien-apple

[msandstedt]

The suggested fix does fix my problem. However, now things like TestCommissionManager are failing. For now, I am calling this in my code after Shutdown:

chip::Dnssd::Resolver::Instance().SetResolverDelegate(nullptr);

[bzbarsky-apple]

Shouldn't ~ResolverDelegateProxy be handling the un-registration?

Removing "deferral candidate" label, since this is a use-after-free bug, hence likely exploitable.

[andy31415]

@msandstedt Trying to understand if this is still an issue and to understand it in general: this mentiones MinimalMdns but also Discovery_ImplPlatform.cpp ... those two should be mutually exclusive.

Is this still an issue? I am unsure how to reproduce. MinMDNS code was updated quite a bit with the usage of address resolver since last year.

[andy31415]

MinMDNS does not currently use ResolverProxy at all.

[andy31415]

Closing: based on discussion with @msandstedt since MinMDNS is not using resolver proxy (anymore - it used to when this bug was reported), this should be fine now.