From 953d9ea26c570b90cf1c3eeaf895da35dfd9a37f Mon Sep 17 00:00:00 2001 From: Edwin Wu Date: Thu, 16 Jul 2026 18:50:16 -0700 Subject: [PATCH] Fix DoS in Quick Share via malformed mDNS on Apple platforms PiperOrigin-RevId: 949295891 --- .../WiFiCommon/GNCNWBrowseResultImpl.m | 5 +- .../apple/Mediums/WiFiCommon/GNCNWFramework.m | 25 +++++- .../WiFiCommon/Tests/GNCFakeNWBrowseResult.h | 1 + .../WiFiCommon/Tests/GNCFakeNWBrowseResult.m | 3 + .../WiFiCommon/Tests/GNCFakeNWFramework.h | 8 +- .../WiFiCommon/Tests/GNCFakeNWFramework.m | 8 +- .../Tests/GNCNWBrowseResultImplTest.m | 13 +++ .../WiFiCommon/Tests/GNCNWFrameworkTest.m | 80 +++++++++++++------ .../apple/Tests/network_utils_test.mm | 26 ++++++ .../implementation/apple/network_utils.mm | 4 +- 10 files changed, 136 insertions(+), 37 deletions(-) diff --git a/internal/platform/implementation/apple/Mediums/WiFiCommon/GNCNWBrowseResultImpl.m b/internal/platform/implementation/apple/Mediums/WiFiCommon/GNCNWBrowseResultImpl.m index 519b5a19..48d4b630 100644 --- a/internal/platform/implementation/apple/Mediums/WiFiCommon/GNCNWBrowseResultImpl.m +++ b/internal/platform/implementation/apple/Mediums/WiFiCommon/GNCNWBrowseResultImpl.m @@ -57,7 +57,10 @@ NS_ASSUME_NONNULL_BEGIN - (nullable NSString *)getBonjourServiceNameFromEndpoint:(nw_endpoint_t)endpoint { const char *name = nw_endpoint_get_bonjour_service_name(endpoint); - return name ? @(name) : nil; + if (name == NULL) return nil; + // @() returns nil on non-UTF-8 input; the wire format does not guarantee UTF-8. + // Round-trip through Latin-1 so callers always get a non-nil NSString. + return @(name) ?: [NSString stringWithCString:name encoding:NSISOLatin1StringEncoding]; } @end diff --git a/internal/platform/implementation/apple/Mediums/WiFiCommon/GNCNWFramework.m b/internal/platform/implementation/apple/Mediums/WiFiCommon/GNCNWFramework.m index 7668a6a1..dc52494c 100644 --- a/internal/platform/implementation/apple/Mediums/WiFiCommon/GNCNWFramework.m +++ b/internal/platform/implementation/apple/Mediums/WiFiCommon/GNCNWFramework.m @@ -63,11 +63,14 @@ NSDictionary *GNCTXTRecordForBrowseResult(nw_browse_resu block:^bool(const char *key, const nw_txt_record_find_key_t found, const uint8_t *value, const size_t value_len) { if (found == nw_txt_record_find_key_non_empty_value) { + NSString *keyString = @(key); NSString *valueString = [[NSString alloc] initWithBytes:value length:value_len encoding:NSUTF8StringEncoding]; - [txtRecords setValue:valueString forKey:@(key)]; + if (keyString != nil && valueString != nil) { + [txtRecords setObject:valueString forKey:keyString]; + } } return YES; }]; @@ -231,6 +234,11 @@ NSDictionary *GNCTXTRecordForBrowseResult(nw_browse_resu [browseResultWrapper copyEndpointFromResult:new_result]; NSString *name = [browseResultWrapper getBonjourServiceNameFromEndpoint:endpoint]; + if (name == nil) { + GNCLoggerInfo( + @"Dropping mDNS result with unrepresentable name."); + break; + } NSDictionary *txtRecords = GNCTXTRecordForBrowseResult(new_result); serviceFoundHandler(name, txtRecords); @@ -250,6 +258,11 @@ NSDictionary *GNCTXTRecordForBrowseResult(nw_browse_resu [browseResultWrapper copyEndpointFromResult:old_result]; NSString *oldName = [browseResultWrapper getBonjourServiceNameFromEndpoint:old_endpoint]; + if (oldName == nil) { + GNCLoggerInfo( + @"Dropping mDNS result with unrepresentable old name."); + break; + } NSDictionary *oldTXTRecords = GNCTXTRecordForBrowseResult(old_result); serviceLostHandler(oldName, oldTXTRecords); @@ -258,6 +271,11 @@ NSDictionary *GNCTXTRecordForBrowseResult(nw_browse_resu [browseResultWrapper copyEndpointFromResult:new_result]; NSString *newName = [browseResultWrapper getBonjourServiceNameFromEndpoint:new_endpoint]; + if (newName == nil) { + GNCLoggerInfo( + @"Dropping mDNS result with unrepresentable new name."); + break; + } NSDictionary *newTXTRecords = GNCTXTRecordForBrowseResult(new_result); serviceFoundHandler(newName, newTXTRecords); @@ -276,6 +294,11 @@ NSDictionary *GNCTXTRecordForBrowseResult(nw_browse_resu [browseResultWrapper copyEndpointFromResult:old_result]; NSString *name = [browseResultWrapper getBonjourServiceNameFromEndpoint:endpoint]; + if (name == nil) { + GNCLoggerInfo( + @"Dropping mDNS result with unrepresentable name."); + break; + } NSDictionary *txtRecords = GNCTXTRecordForBrowseResult(old_result); serviceLostHandler(name, txtRecords); diff --git a/internal/platform/implementation/apple/Mediums/WiFiCommon/Tests/GNCFakeNWBrowseResult.h b/internal/platform/implementation/apple/Mediums/WiFiCommon/Tests/GNCFakeNWBrowseResult.h index 1d86c517..a09119ff 100644 --- a/internal/platform/implementation/apple/Mediums/WiFiCommon/Tests/GNCFakeNWBrowseResult.h +++ b/internal/platform/implementation/apple/Mediums/WiFiCommon/Tests/GNCFakeNWBrowseResult.h @@ -31,6 +31,7 @@ NS_ASSUME_NONNULL_BEGIN @property(nonatomic) nw_browse_result_change_t getChangesFromResult; @property(nonatomic, nullable) nw_endpoint_t endpointFromResultResult; @property(nonatomic, nullable) NSString *getBonjourServiceNameFromEndpointResult; +@property(nonatomic) BOOL returnNilServiceName; @end diff --git a/internal/platform/implementation/apple/Mediums/WiFiCommon/Tests/GNCFakeNWBrowseResult.m b/internal/platform/implementation/apple/Mediums/WiFiCommon/Tests/GNCFakeNWBrowseResult.m index 2355b6c2..11a8b3c0 100644 --- a/internal/platform/implementation/apple/Mediums/WiFiCommon/Tests/GNCFakeNWBrowseResult.m +++ b/internal/platform/implementation/apple/Mediums/WiFiCommon/Tests/GNCFakeNWBrowseResult.m @@ -67,6 +67,9 @@ NS_ASSUME_NONNULL_BEGIN } - (nullable NSString *)getBonjourServiceNameFromEndpoint:(nw_endpoint_t)endpoint { + if (self.returnNilServiceName) { + return nil; + } return self.getBonjourServiceNameFromEndpointResult ?: @"FakeService"; } diff --git a/internal/platform/implementation/apple/Mediums/WiFiCommon/Tests/GNCFakeNWFramework.h b/internal/platform/implementation/apple/Mediums/WiFiCommon/Tests/GNCFakeNWFramework.h index 4d56fbca..eb827f36 100644 --- a/internal/platform/implementation/apple/Mediums/WiFiCommon/Tests/GNCFakeNWFramework.h +++ b/internal/platform/implementation/apple/Mediums/WiFiCommon/Tests/GNCFakeNWFramework.h @@ -53,16 +53,16 @@ NS_ASSUME_NONNULL_BEGIN * @param serviceName The name of the service found. * @param txtRecords The TXT records of the service found. */ -- (void)triggerServiceFound:(NSString*)serviceName - txtRecords:(NSDictionary*)txtRecords; +- (void)triggerServiceFound:(nullable NSString*)serviceName + txtRecords:(nullable NSDictionary*)txtRecords; /** * Triggers the service lost handler with the given service info. * * @param serviceName The name of the service lost. * @param txtRecords The TXT records of the service lost. */ -- (void)triggerServiceLost:(NSString*)serviceName - txtRecords:(NSDictionary*)txtRecords; +- (void)triggerServiceLost:(nullable NSString*)serviceName + txtRecords:(nullable NSDictionary*)txtRecords; @end diff --git a/internal/platform/implementation/apple/Mediums/WiFiCommon/Tests/GNCFakeNWFramework.m b/internal/platform/implementation/apple/Mediums/WiFiCommon/Tests/GNCFakeNWFramework.m index 76f7eb2f..e94a05f0 100644 --- a/internal/platform/implementation/apple/Mediums/WiFiCommon/Tests/GNCFakeNWFramework.m +++ b/internal/platform/implementation/apple/Mediums/WiFiCommon/Tests/GNCFakeNWFramework.m @@ -122,15 +122,15 @@ return serverSocket; } -- (void)triggerServiceFound:(NSString *)serviceName - txtRecords:(NSDictionary *)txtRecords { +- (void)triggerServiceFound:(nullable NSString *)serviceName + txtRecords:(nullable NSDictionary *)txtRecords { if (self.serviceFoundHandler) { self.serviceFoundHandler(serviceName, txtRecords); } } -- (void)triggerServiceLost:(NSString *)serviceName - txtRecords:(NSDictionary *)txtRecords { +- (void)triggerServiceLost:(nullable NSString *)serviceName + txtRecords:(nullable NSDictionary *)txtRecords { if (self.serviceLostHandler) { self.serviceLostHandler(serviceName, txtRecords); } diff --git a/internal/platform/implementation/apple/Mediums/WiFiCommon/Tests/GNCNWBrowseResultImplTest.m b/internal/platform/implementation/apple/Mediums/WiFiCommon/Tests/GNCNWBrowseResultImplTest.m index b415c876..b1192fdb 100644 --- a/internal/platform/implementation/apple/Mediums/WiFiCommon/Tests/GNCNWBrowseResultImplTest.m +++ b/internal/platform/implementation/apple/Mediums/WiFiCommon/Tests/GNCNWBrowseResultImplTest.m @@ -67,4 +67,17 @@ XCTAssertEqualObjects(results[@"key2"], @"value2"); } +- (void)testGetBonjourServiceName_InvalidUTF8 { + GNCNWBrowseResultImpl *browseResult = [[GNCNWBrowseResultImpl alloc] init]; + const char *raw_invalid = "\xc3\x28" + "abc"; + nw_endpoint_t endpoint = + nw_endpoint_create_bonjour_service(raw_invalid, "_servicetype._tcp", "local."); + XCTAssertNotNil(endpoint); + + NSString *serviceName = [browseResult getBonjourServiceNameFromEndpoint:endpoint]; + XCTAssertNotNil(serviceName); + XCTAssertEqualObjects(serviceName, @"�(abc"); +} + @end diff --git a/internal/platform/implementation/apple/Mediums/WiFiCommon/Tests/GNCNWFrameworkTest.m b/internal/platform/implementation/apple/Mediums/WiFiCommon/Tests/GNCNWFrameworkTest.m index e09375c9..4b985539 100644 --- a/internal/platform/implementation/apple/Mediums/WiFiCommon/Tests/GNCNWFrameworkTest.m +++ b/internal/platform/implementation/apple/Mediums/WiFiCommon/Tests/GNCNWFrameworkTest.m @@ -61,7 +61,6 @@ static NSString *const kHostAddress = @"127.0.0.1"; _mockConnectionImpl = OCMClassMock([GNCNWConnectionImpl class]); } - - (void)testGNCNWFrameworkCanBeInstantiated { GNCNWFramework *framework = [[GNCNWFramework alloc] init]; XCTAssertNotNil(framework); @@ -272,6 +271,49 @@ static NSString *const kHostAddress = @"127.0.0.1"; XCTAssertEqualObjects(foundTXTRecords, @{@"key" : @"value"}); } +- (void)testStartDiscoveryForServiceTypeNilName API_AVAILABLE(ios(13.0)) { + GNCNWFramework *framework = [[GNCNWFramework alloc] init]; + GNCFakeNWBrowser *fakeBrowser = [[GNCFakeNWBrowser alloc] init]; + fakeBrowser.createWithDescriptorResult = (nw_browser_t)fakeBrowser; + OCMStub([_mockBrowserImpl alloc]).andReturn(fakeBrowser); + + XCTestExpectation *serviceFoundExpectation = [self expectationWithDescription:@"Service found"]; + serviceFoundExpectation.inverted = YES; + + NSError *error = nil; + BOOL result = [framework startDiscoveryForServiceType:kServiceType + serviceFoundHandler:^(NSString *serviceName, + NSDictionary *txtRecords) { + [serviceFoundExpectation fulfill]; + } + serviceLostHandler:^(NSString *serviceName, + NSDictionary *txtRecords) { + } + includePeerToPeer:NO + error:&error]; + + XCTAssertTrue(result); + XCTAssertNil(error); + + // Simulate a service being found with nil name. + GNCFakeNWBrowseResult *fakeBrowseResult = [[GNCFakeNWBrowseResult alloc] init]; + fakeBrowseResult.txtRecord = @{@"key" : @"value"}; + fakeBrowseResult.getChangesFromResult = nw_browse_result_change_result_added; + nw_endpoint_t fakeEndpoint = + nw_endpoint_create_host("localhost", [[NSString stringWithFormat:@"%ld", kPort] UTF8String]); + fakeBrowseResult.endpointFromResultResult = fakeEndpoint; + fakeBrowseResult.returnNilServiceName = YES; + OCMStub([_mockBrowseResultImpl sharedInstance]).andReturn(fakeBrowseResult); + + if (fakeBrowser.browseResultsChangedHandler) { + GNCFakeNWBrowseResult *oldFakeBrowseResult = [[GNCFakeNWBrowseResult alloc] init]; + fakeBrowser.browseResultsChangedHandler((nw_browse_result_t)oldFakeBrowseResult, + (nw_browse_result_t)fakeBrowseResult, true); + } + + [self waitForExpectations:@[ serviceFoundExpectation ] timeout:0.1]; +} + - (void)testStartDiscoveryForServiceTypeDuplicate API_AVAILABLE(ios(13.0)) { GNCNWFramework *framework = [[GNCNWFramework alloc] init]; GNCFakeNWBrowser *fakeBrowser = [[GNCFakeNWBrowser alloc] init]; @@ -449,12 +491,13 @@ static NSString *const kHostAddress = @"127.0.0.1"; // TODO: b/377543997 - Migrate to dependency injection and remove mocks. OCMStub([_mockBrowserImpl alloc]).andReturn(fakeBrowser); - __block BOOL serviceFound = NO; + XCTestExpectation *serviceFoundExpectation = [self expectationWithDescription:@"Service found"]; + serviceFoundExpectation.inverted = YES; NSError *error = nil; [framework startDiscoveryForServiceType:kServiceType serviceFoundHandler:^(NSString *serviceName, NSDictionary *txtRecords) { - serviceFound = YES; + [serviceFoundExpectation fulfill]; } serviceLostHandler:^(NSString *serviceName, NSDictionary *txtRecords) { @@ -478,14 +521,7 @@ static NSString *const kHostAddress = @"127.0.0.1"; (nw_browse_result_t)fakeBrowseResult, true); } - // Allow async blocks to run. - XCTestExpectation *delay = [[XCTestExpectation alloc] initWithDescription:@"delay"]; - dispatch_after(dispatch_time(DISPATCH_TIME_NOW, 0.1 * NSEC_PER_SEC), dispatch_get_main_queue(), ^{ - [delay fulfill]; - }); - [self waitForExpectations:@[ delay ] timeout:0.5]; - - XCTAssertFalse(serviceFound); + [self waitForExpectations:@[ serviceFoundExpectation ] timeout:0.1]; } - (void)testStartDiscoveryIgnoresLoopbackRemove API_AVAILABLE(ios(13.0)) { @@ -495,7 +531,8 @@ static NSString *const kHostAddress = @"127.0.0.1"; // TODO: b/377543997 - Migrate to dependency injection and remove mocks. OCMStub([_mockBrowserImpl alloc]).andReturn(fakeBrowser); - __block BOOL serviceLost = NO; + XCTestExpectation *serviceLostExpectation = [self expectationWithDescription:@"Service lost"]; + serviceLostExpectation.inverted = YES; NSError *error = nil; [framework startDiscoveryForServiceType:kServiceType serviceFoundHandler:^(NSString *serviceName, @@ -503,7 +540,7 @@ static NSString *const kHostAddress = @"127.0.0.1"; } serviceLostHandler:^(NSString *serviceName, NSDictionary *txtRecords) { - serviceLost = YES; + [serviceLostExpectation fulfill]; } includePeerToPeer:NO error:&error]; @@ -524,14 +561,7 @@ static NSString *const kHostAddress = @"127.0.0.1"; (nw_browse_result_t)newFakeBrowseResult, true); } - // Allow async blocks to run. - XCTestExpectation *delay = [[XCTestExpectation alloc] initWithDescription:@"delay"]; - dispatch_after(dispatch_time(DISPATCH_TIME_NOW, 0.1 * NSEC_PER_SEC), dispatch_get_main_queue(), ^{ - [delay fulfill]; - }); - [self waitForExpectations:@[ delay ] timeout:0.5]; - - XCTAssertFalse(serviceLost); + [self waitForExpectations:@[ serviceLostExpectation ] timeout:0.1]; } - (void)testStopDiscoveryForServiceType API_AVAILABLE(ios(13.0)) { @@ -722,8 +752,8 @@ static NSString *const kHostAddress = @"127.0.0.1"; GNCNWFrameworkSocket *socket = [framework connectToHost:address port:kPort includePeerToPeer:NO - cancelSource:nil - queue:nil + cancelSource:nil + queue:nil error:&error]; XCTAssertNotNil(socket); @@ -742,8 +772,8 @@ static NSString *const kHostAddress = @"127.0.0.1"; GNCNWFrameworkSocket *socket = [framework connectToHost:address port:kPort includePeerToPeer:NO - cancelSource:nil - queue:nil + cancelSource:nil + queue:nil error:&error]; XCTAssertNil(socket); diff --git a/internal/platform/implementation/apple/Tests/network_utils_test.mm b/internal/platform/implementation/apple/Tests/network_utils_test.mm index 00e36665..527132bf 100644 --- a/internal/platform/implementation/apple/Tests/network_utils_test.mm +++ b/internal/platform/implementation/apple/Tests/network_utils_test.mm @@ -157,4 +157,30 @@ static NSString *const kServiceType = @"_test._tcp"; [self waitForExpectationsWithTimeout:1.0 handler:nil]; } +- (void)testStartDiscoveryCallbacksWithNilName { + XCTestExpectation *foundExpectation = [self expectationWithDescription:@"Service found callback"]; + XCTestExpectation *lostExpectation = [self expectationWithDescription:@"Service lost callback"]; + + nearby::apple::network_utils::NetworkDiscoveredServiceCallback callback; + callback.network_service_discovered_cb = [&](const nearby::NsdServiceInfo &service_info) { + XCTAssertEqual(service_info.GetServiceName(), std::string("")); + XCTAssertEqual(service_info.GetServiceType(), kServiceType.UTF8String); + [foundExpectation fulfill]; + }; + callback.network_service_lost_cb = [&](const nearby::NsdServiceInfo &service_info) { + XCTAssertEqual(service_info.GetServiceName(), std::string("")); + XCTAssertEqual(service_info.GetServiceType(), kServiceType.UTF8String); + [lostExpectation fulfill]; + }; + + BOOL result = nearby::apple::network_utils::StartDiscovery( + _fakeNWFramework, kServiceType.UTF8String, std::move(callback), YES); + XCTAssertTrue(result); + + [_fakeNWFramework triggerServiceFound:nil txtRecords:@{}]; + [_fakeNWFramework triggerServiceLost:nil txtRecords:@{}]; + + [self waitForExpectationsWithTimeout:1.0 handler:nil]; +} + @end diff --git a/internal/platform/implementation/apple/network_utils.mm b/internal/platform/implementation/apple/network_utils.mm index 30c06cfb..11d7e14b 100644 --- a/internal/platform/implementation/apple/network_utils.mm +++ b/internal/platform/implementation/apple/network_utils.mm @@ -63,7 +63,7 @@ bool StartDiscovery(GNCNWFramework* medium, const std::string& service_type, serviceFoundHandler:^(NSString* name, NSDictionary* txtRecords) { NsdServiceInfo nsd_service_info; nsd_service_info.SetServiceType([serviceType UTF8String]); - nsd_service_info.SetServiceName([name UTF8String]); + nsd_service_info.SetServiceName(name ? [name UTF8String] : ""); [txtRecords enumerateKeysAndObjectsUsingBlock:[nsd_service_info = &nsd_service_info]( NSString* key, NSString* val, BOOL* stop) { @@ -74,7 +74,7 @@ bool StartDiscovery(GNCNWFramework* medium, const std::string& service_type, serviceLostHandler:^(NSString* name, NSDictionary* txtRecords) { NsdServiceInfo nsd_service_info; nsd_service_info.SetServiceType([serviceType UTF8String]); - nsd_service_info.SetServiceName([name UTF8String]); + nsd_service_info.SetServiceName(name ? [name UTF8String] : ""); [txtRecords enumerateKeysAndObjectsUsingBlock:[nsd_service_info = &nsd_service_info]( NSString* key, NSString* val, BOOL* stop) {