From 2c52568075bc9ef78a3b5c8fef956716a9d3f122 Mon Sep 17 00:00:00 2001 From: Karsten Sperling Date: Thu, 30 May 2024 11:36:14 +1200 Subject: [PATCH 1/4] Darwin: Keep MTRCommissionableBrowser around until OnBleScanStopped Because the BLE platform implementation uses a separate dispatch queue, StopBleScan() does not synchrnously stop any further use of the current BleScannerDelegate. Keep the MTRCommissionableBrowser that contains the delegate alive until OnBleScanStopped to avoid a UAF. --- .../CHIP/MTRCommissionableBrowser.mm | 19 +++++++++++++++++-- 1 file changed, 17 insertions(+), 2 deletions(-) diff --git a/src/darwin/Framework/CHIP/MTRCommissionableBrowser.mm b/src/darwin/Framework/CHIP/MTRCommissionableBrowser.mm index 2b3e2814ecd220..c3eda10092999a 100644 --- a/src/darwin/Framework/CHIP/MTRCommissionableBrowser.mm +++ b/src/darwin/Framework/CHIP/MTRCommissionableBrowser.mm @@ -61,7 +61,11 @@ @implementation MTRCommissionableBrowserResult #endif // CONFIG_NETWORK_LAYER_BLE { public: - CHIP_ERROR Start(id delegate, MTRDeviceController * controller, dispatch_queue_t queue) +#if CONFIG_NETWORK_LAYER_BLE + id mBleScannerDelegateOwner; +#endif // CONFIG_NETWORK_LAYER_BLE + + CHIP_ERROR Start(id owner, id delegate, MTRDeviceController * controller, dispatch_queue_t queue) { assertChipStackLockedByCurrentThread(); @@ -77,7 +81,10 @@ CHIP_ERROR Start(id delegate, MTRDeviceControl ResetCounters(); #if CONFIG_NETWORK_LAYER_BLE + mBleScannerDelegateOwner = owner; // retain the owner until OnBleScanStopped is called ReturnErrorOnFailure(PlatformMgrImpl().StartBleScan(this)); +#else + (void)owner; #endif // CONFIG_NETWORK_LAYER_BLE ReturnErrorOnFailure(Resolver::Instance().Init(chip::DeviceLayer::UDPEndPointManager())); @@ -281,6 +288,7 @@ void OnBrowseStop(CHIP_ERROR error) override void OnBleScanAdd(BLE_CONNECTION_OBJECT connObj, const ChipBLEDeviceIdentificationInfo & info) override { assertChipStackLockedByCurrentThread(); + VerifyOrReturn(mDelegate != nil); auto result = [[MTRCommissionableBrowserResult alloc] init]; result.instanceName = [NSString stringWithUTF8String:kBleKey]; @@ -303,6 +311,7 @@ void OnBleScanAdd(BLE_CONNECTION_OBJECT connObj, const ChipBLEDeviceIdentificati void OnBleScanRemove(BLE_CONNECTION_OBJECT connObj) override { assertChipStackLockedByCurrentThread(); + VerifyOrReturn(mDelegate != nil); auto key = [NSString stringWithFormat:@"%@", connObj]; if ([mDiscoveredResults objectForKey:key] == nil) { @@ -319,6 +328,12 @@ void OnBleScanRemove(BLE_CONNECTION_OBJECT connObj) override [mDelegate controller:mController didFindCommissionableDevice:result]; }); } + + void OnBleScanStopped() override + { + mBleScannerDelegateOwner = nil; + } + #endif // CONFIG_NETWORK_LAYER_BLE private: @@ -354,7 +369,7 @@ - (instancetype)initWithDelegate:(id)delegate - (BOOL)start { - VerifyOrReturnValue(CHIP_NO_ERROR == _browser.Start(_delegate, _controller, _queue), NO); + VerifyOrReturnValue(CHIP_NO_ERROR == _browser.Start(self, _delegate, _controller, _queue), NO); return YES; } From c853d0283a393e2b7e4fa1e9b698d8d1a5cdaa69 Mon Sep 17 00:00:00 2001 From: Karsten Sperling Date: Thu, 30 May 2024 12:04:38 +1200 Subject: [PATCH 2/4] restyle --- src/darwin/Framework/CHIP/MTRCommissionableBrowser.mm | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/darwin/Framework/CHIP/MTRCommissionableBrowser.mm b/src/darwin/Framework/CHIP/MTRCommissionableBrowser.mm index c3eda10092999a..513d459a266d5c 100644 --- a/src/darwin/Framework/CHIP/MTRCommissionableBrowser.mm +++ b/src/darwin/Framework/CHIP/MTRCommissionableBrowser.mm @@ -84,7 +84,7 @@ CHIP_ERROR Start(id owner, id delegate, MTRDev mBleScannerDelegateOwner = owner; // retain the owner until OnBleScanStopped is called ReturnErrorOnFailure(PlatformMgrImpl().StartBleScan(this)); #else - (void)owner; + (void) owner; #endif // CONFIG_NETWORK_LAYER_BLE ReturnErrorOnFailure(Resolver::Instance().Init(chip::DeviceLayer::UDPEndPointManager())); From 567192cc90a6ce18430168bb7dbd25411d31712a Mon Sep 17 00:00:00 2001 From: Karsten Sperling Date: Thu, 30 May 2024 13:45:49 +1200 Subject: [PATCH 3/4] Retain owner in Stop() instead of Start() --- .../CHIP/MTRCommissionableBrowser.mm | 20 +++++++++++-------- 1 file changed, 12 insertions(+), 8 deletions(-) diff --git a/src/darwin/Framework/CHIP/MTRCommissionableBrowser.mm b/src/darwin/Framework/CHIP/MTRCommissionableBrowser.mm index 513d459a266d5c..232ad1affdc937 100644 --- a/src/darwin/Framework/CHIP/MTRCommissionableBrowser.mm +++ b/src/darwin/Framework/CHIP/MTRCommissionableBrowser.mm @@ -65,7 +65,7 @@ @implementation MTRCommissionableBrowserResult id mBleScannerDelegateOwner; #endif // CONFIG_NETWORK_LAYER_BLE - CHIP_ERROR Start(id owner, id delegate, MTRDeviceController * controller, dispatch_queue_t queue) + CHIP_ERROR Start(id delegate, MTRDeviceController * controller, dispatch_queue_t queue) { assertChipStackLockedByCurrentThread(); @@ -81,10 +81,7 @@ CHIP_ERROR Start(id owner, id delegate, MTRDev ResetCounters(); #if CONFIG_NETWORK_LAYER_BLE - mBleScannerDelegateOwner = owner; // retain the owner until OnBleScanStopped is called ReturnErrorOnFailure(PlatformMgrImpl().StartBleScan(this)); -#else - (void) owner; #endif // CONFIG_NETWORK_LAYER_BLE ReturnErrorOnFailure(Resolver::Instance().Init(chip::DeviceLayer::UDPEndPointManager())); @@ -97,7 +94,7 @@ CHIP_ERROR Start(id owner, id delegate, MTRDev chip::Inet::InterfaceId::Null(), this); } - CHIP_ERROR Stop() + CHIP_ERROR Stop(id owner) { assertChipStackLockedByCurrentThread(); @@ -116,7 +113,14 @@ CHIP_ERROR Stop() mDiscoveredResults = nil; #if CONFIG_NETWORK_LAYER_BLE - ReturnErrorOnFailure(PlatformMgrImpl().StopBleScan()); + mBleScannerDelegateOwner = owner; // retain the owner until OnBleScanStopped is called + CHIP_ERROR err = PlatformMgrImpl().StopBleScan(); + if (err != CHIP_NO_ERROR) { + mBleScannerDelegateOwner = nil; + return err; + } +#else + (void) owner; #endif // CONFIG_NETWORK_LAYER_BLE return ChipDnssdStopBrowse(this); @@ -369,13 +373,13 @@ - (instancetype)initWithDelegate:(id)delegate - (BOOL)start { - VerifyOrReturnValue(CHIP_NO_ERROR == _browser.Start(self, _delegate, _controller, _queue), NO); + VerifyOrReturnValue(CHIP_NO_ERROR == _browser.Start(_delegate, _controller, _queue), NO); return YES; } - (BOOL)stop { - VerifyOrReturnValue(CHIP_NO_ERROR == _browser.Stop(), NO); + VerifyOrReturnValue(CHIP_NO_ERROR == _browser.Stop(self), NO); _delegate = nil; _controller = nil; _queue = nil; From b6dff312670c90b258609b7161c2fdaf728c6982 Mon Sep 17 00:00:00 2001 From: Karsten Sperling Date: Thu, 30 May 2024 16:58:58 +1200 Subject: [PATCH 4/4] More review comments --- src/darwin/Framework/CHIP/MTRCommissionableBrowser.mm | 6 +----- 1 file changed, 1 insertion(+), 5 deletions(-) diff --git a/src/darwin/Framework/CHIP/MTRCommissionableBrowser.mm b/src/darwin/Framework/CHIP/MTRCommissionableBrowser.mm index 232ad1affdc937..276fb5e3460682 100644 --- a/src/darwin/Framework/CHIP/MTRCommissionableBrowser.mm +++ b/src/darwin/Framework/CHIP/MTRCommissionableBrowser.mm @@ -114,11 +114,7 @@ CHIP_ERROR Stop(id owner) #if CONFIG_NETWORK_LAYER_BLE mBleScannerDelegateOwner = owner; // retain the owner until OnBleScanStopped is called - CHIP_ERROR err = PlatformMgrImpl().StopBleScan(); - if (err != CHIP_NO_ERROR) { - mBleScannerDelegateOwner = nil; - return err; - } + PlatformMgrImpl().StopBleScan(); // doesn't actually fail, and if it did we'd want to carry on regardless #else (void) owner; #endif // CONFIG_NETWORK_LAYER_BLE