From a6f799af7f13154ee8d6d3156750d0cfe3f5a788 Mon Sep 17 00:00:00 2001 From: Edwin Wu Date: Wed, 15 Jul 2026 19:45:44 -0700 Subject: [PATCH] Validate file/folder names for UTF-8 and null characters in Nearby Connections. PiperOrigin-RevId: 948685221 --- Package.swift | 3 + connections/implementation/BUILD | 2 + .../mediums/advertisements/BUILD | 7 -- .../offline_frames_validator.cc | 36 ++++--- .../offline_frames_validator_test.cc | 96 +++++++++++++++++-- .../platform/implementation/apple/platform.mm | 22 ++++- .../apple/scheduled_executor.mm | 22 ++++- .../internal/base/utf_string_conversions.h | 8 +- 8 files changed, 160 insertions(+), 36 deletions(-) diff --git a/Package.swift b/Package.swift index d96c9aa4..2dfe92d2 100644 --- a/Package.swift +++ b/Package.swift @@ -535,6 +535,7 @@ let package = Package( .headerSearchPath("third_party/ukey2/ukey2/"), .headerSearchPath("third_party/ukey2/compiled_proto/src/main/proto"), .define("NO_WEBRTC"), + .define("GITHUB_BUILD"), ] ), .target( @@ -553,6 +554,7 @@ let package = Package( .headerSearchPath("compiled_proto/"), .define("NO_WEBRTC"), .define("NC_OSS_BUILD"), + .define("GITHUB_BUILD"), ] ), .target( @@ -566,6 +568,7 @@ let package = Package( .headerSearchPath("./"), .headerSearchPath("compiled_proto/"), .define("NO_WEBRTC"), + .define("GITHUB_BUILD"), ] ), .target( diff --git a/connections/implementation/BUILD b/connections/implementation/BUILD index 0121567f..7c5344f2 100644 --- a/connections/implementation/BUILD +++ b/connections/implementation/BUILD @@ -99,6 +99,8 @@ cc_library( "//internal/platform:base", "//internal/platform:logging", "//internal/platform:mac_address", + "//sharing/internal/base:utf_utils", + "@com_google_absl//absl/strings", "@com_google_absl//absl/strings:string_view", "@com_google_absl//absl/time", ], diff --git a/connections/implementation/mediums/advertisements/BUILD b/connections/implementation/mediums/advertisements/BUILD index 233aad0d..ff31ac4e 100644 --- a/connections/implementation/mediums/advertisements/BUILD +++ b/connections/implementation/mediums/advertisements/BUILD @@ -29,7 +29,6 @@ cc_library( "//internal/platform:base", "//internal/platform:logging", "//internal/platform:util", - "@com_google_absl//absl/strings", ], ) @@ -45,7 +44,6 @@ cc_library( "//internal/platform:logging", "//internal/platform:types", "//internal/platform:util", - "@com_google_absl//absl/strings", ], ) @@ -54,9 +52,7 @@ cc_library( srcs = ["advertisement_util.cc"], hdrs = ["advertisement_util.h"], deps = [ - ":dct_advertisement", "//internal/platform:base", - "//internal/platform:logging", "//internal/platform:util", "@com_google_absl//absl/strings:string_view", ], @@ -79,8 +75,6 @@ cc_test( srcs = ["dct_advertisement_test.cc"], deps = [ ":dct_advertisement", - "//internal/platform:base", - "//internal/platform:util", "//internal/platform/implementation/g3", "@com_github_protobuf_matchers//protobuf-matchers", "@com_google_googletest//:gtest_main", @@ -93,7 +87,6 @@ cc_test( deps = [ ":util", "//internal/platform:base", - "//internal/platform:util", "//internal/platform/implementation/g3", "@com_github_protobuf_matchers//protobuf-matchers", "@com_google_googletest//:gtest_main", diff --git a/connections/implementation/offline_frames_validator.cc b/connections/implementation/offline_frames_validator.cc index 9c54cf9c..ecfe5d5a 100644 --- a/connections/implementation/offline_frames_validator.cc +++ b/connections/implementation/offline_frames_validator.cc @@ -19,6 +19,8 @@ #include //NOLINT #include +#include "absl/strings/escaping.h" +#include "absl/strings/match.h" #include "absl/strings/string_view.h" #include "connections/implementation/internal_payload.h" #include "connections/implementation/offline_frames.h" @@ -27,9 +29,11 @@ #include "internal/platform/exception.h" #include "internal/platform/logging.h" #include "internal/platform/service_address.h" +#include "sharing/internal/base/utf_string_conversions.h" namespace nearby { namespace connections { + namespace parser { namespace { @@ -146,16 +150,25 @@ Exception EnsureValidPayloadTransferControlFrame( return {Exception::kSuccess}; } -bool CheckForIllegalCharacters(std::string toBeValidated, +bool CheckForIllegalCharacters(absl::string_view toBeValidated, const absl::string_view illegalPatterns[], size_t illegalPatternsSize) { if (toBeValidated.empty()) { return false; } + // Null bytes are rejected to prevent null-byte injection attacks. C-style + // APIs (like system file operations) treat '\0' as a string terminator, + // whereas C++ strings can contain them. This discrepancy can lead to + // validation bypasses (e.g., validating "file.sh\0.png" as a PNG but + // creating "file.sh" on disk). + if (absl::StrContains(toBeValidated, '\0') || + !nearby::utils::IsStringUtf8(toBeValidated)) { + return true; + } + for (int index = 0; index < illegalPatternsSize; index++) { - if (toBeValidated.find(std::string(illegalPatterns[index])) != - std::string::npos) { + if (absl::StrContains(toBeValidated, illegalPatterns[index])) { return true; } } @@ -184,20 +197,21 @@ Exception EnsureValidPayloadTransferFrame(const PayloadTransferFrame& frame) { location::nearby::connections::PayloadTransferFrame::PayloadHeader:: FILE) { if (frame.payload_header().has_file_name()) { - if (CheckForIllegalCharacters(frame.payload_header().file_name(), - kIllegalFileNamePatterns, + const std::string& file_name = frame.payload_header().file_name(); + if (CheckForIllegalCharacters(file_name, kIllegalFileNamePatterns, kIllegalFileNamePatternsSize)) { - LOG(ERROR) << "File name " << frame.payload_header().file_name() - << " has illegal characters"; + LOG(ERROR) << "File name (hex) " << absl::BytesToHexString(file_name) + << " has illegal characters or invalid UTF-8"; return {Exception::kIllegalCharacters}; } } if (frame.payload_header().has_parent_folder()) { - if (CheckForIllegalCharacters(frame.payload_header().parent_folder(), - kIllegalParentFolderPatterns, + const std::string& parent_folder = frame.payload_header().parent_folder(); + if (CheckForIllegalCharacters(parent_folder, kIllegalParentFolderPatterns, kIllegalParentFolderPatternsSize)) { - LOG(ERROR) << "Parent folder " << frame.payload_header().parent_folder() - << " has illegal characters"; + LOG(ERROR) << "Parent folder (hex) " + << absl::BytesToHexString(parent_folder) + << " has illegal characters or invalid UTF-8"; return {Exception::kIllegalCharacters}; } } diff --git a/connections/implementation/offline_frames_validator_test.cc b/connections/implementation/offline_frames_validator_test.cc index 59c12271..019847c9 100644 --- a/connections/implementation/offline_frames_validator_test.cc +++ b/connections/implementation/offline_frames_validator_test.cc @@ -180,8 +180,8 @@ TEST(OfflineFramesValidatorTest, OfflineFrame offline_frame; OsInfo os_info; - std::string bytes = ForConnectionResponse(kStatusAccepted, os_info, - "device_name"); + std::string bytes = + ForConnectionResponse(kStatusAccepted, os_info, "device_name"); offline_frame.ParseFromString(bytes); auto ret_value = EnsureValidOfflineFrame(offline_frame); @@ -211,8 +211,7 @@ TEST(OfflineFramesValidatorTest, OfflineFrame offline_frame; OsInfo os_info; - std::string bytes = - ForConnectionResponse(-1, os_info, "device_name"); + std::string bytes = ForConnectionResponse(-1, os_info, "device_name"); offline_frame.ParseFromString(bytes); auto ret_value = EnsureValidOfflineFrame(offline_frame); @@ -369,6 +368,92 @@ TEST(OfflineFramesValidatorTest, EXPECT_EQ(ret_value.value, Exception::kIllegalCharacters); } +TEST(OfflineFramesValidatorTest, ValidatesAsFailedTypeFileWithNonUtf8FilePath) { + PayloadTransferFrame::PayloadHeader header; + PayloadTransferFrame::PayloadChunk chunk; + header.set_id(12345); + header.set_type(PayloadTransferFrame::PayloadHeader::FILE); + header.set_total_size(100); + header.set_file_name(std::string("hello\xffworld")); + header.set_parent_folder(std::string()); + chunk.set_body("payload data"); + chunk.set_offset(0); + chunk.set_flags(1); + + OfflineFrame offline_frame; + std::string bytes = ForDataPayloadTransfer(header, chunk); + offline_frame.ParseFromString(bytes); + + auto ret_value = EnsureValidOfflineFrame(offline_frame); + + EXPECT_EQ(ret_value.value, Exception::kIllegalCharacters); +} + +TEST(OfflineFramesValidatorTest, + ValidatesAsFailedTypeFileWithNonUtf8ParentFolder) { + PayloadTransferFrame::PayloadHeader header; + PayloadTransferFrame::PayloadChunk chunk; + header.set_id(12345); + header.set_type(PayloadTransferFrame::PayloadHeader::FILE); + header.set_total_size(100); + header.set_file_name(std::string("valid.txt")); + header.set_parent_folder(std::string("folder\xff")); + chunk.set_body("payload data"); + chunk.set_offset(0); + chunk.set_flags(1); + + OfflineFrame offline_frame; + std::string bytes = ForDataPayloadTransfer(header, chunk); + offline_frame.ParseFromString(bytes); + + auto ret_value = EnsureValidOfflineFrame(offline_frame); + + EXPECT_EQ(ret_value.value, Exception::kIllegalCharacters); +} + +TEST(OfflineFramesValidatorTest, ValidatesAsFailedTypeFileWithNullInFilePath) { + PayloadTransferFrame::PayloadHeader header; + PayloadTransferFrame::PayloadChunk chunk; + header.set_id(12345); + header.set_type(PayloadTransferFrame::PayloadHeader::FILE); + header.set_total_size(100); + header.set_file_name(std::string("hello\0world", 11)); + header.set_parent_folder(std::string()); + chunk.set_body("payload data"); + chunk.set_offset(0); + chunk.set_flags(1); + + OfflineFrame offline_frame; + std::string bytes = ForDataPayloadTransfer(header, chunk); + offline_frame.ParseFromString(bytes); + + auto ret_value = EnsureValidOfflineFrame(offline_frame); + + EXPECT_EQ(ret_value.value, Exception::kIllegalCharacters); +} + +TEST(OfflineFramesValidatorTest, + ValidatesAsFailedTypeFileWithNullInParentFolder) { + PayloadTransferFrame::PayloadHeader header; + PayloadTransferFrame::PayloadChunk chunk; + header.set_id(12345); + header.set_type(PayloadTransferFrame::PayloadHeader::FILE); + header.set_total_size(100); + header.set_file_name(std::string("valid.txt")); + header.set_parent_folder(std::string("folder\0name", 11)); + chunk.set_body("payload data"); + chunk.set_offset(0); + chunk.set_flags(1); + + OfflineFrame offline_frame; + std::string bytes = ForDataPayloadTransfer(header, chunk); + offline_frame.ParseFromString(bytes); + + auto ret_value = EnsureValidOfflineFrame(offline_frame); + + EXPECT_EQ(ret_value.value, Exception::kIllegalCharacters); +} + TEST(OfflineFramesValidatorTest, ValidatesAsFailWithNullPayloadTransferFrame) { PayloadTransferFrame::PayloadHeader header; PayloadTransferFrame::PayloadChunk chunk; @@ -998,8 +1083,7 @@ TEST(OfflineFramesValidatorTest, EXPECT_FALSE(ret_value.Ok()); - std::string wifi_direct_ssid_64_length = - "DIRECT-A0-" + std::string(54, 'A'); + std::string wifi_direct_ssid_64_length = "DIRECT-A0-" + std::string(54, 'A'); bytes = ForBwuWifiDirectPathAvailable( wifi_direct_ssid_64_length, std::string(kWifiDirectPassword), kPort, kWifiDirectFrequency, kSupportsDisablingEncryption, std::string(kGateway), diff --git a/internal/platform/implementation/apple/platform.mm b/internal/platform/implementation/apple/platform.mm index e11b524b..62b5c4ef 100644 --- a/internal/platform/implementation/apple/platform.mm +++ b/internal/platform/implementation/apple/platform.mm @@ -51,8 +51,12 @@ namespace api { std::string ImplementationPlatform::GetCustomSavePath(const std::string& parent_folder, const std::string& file_name) { // Collapse any path escaping characters. - NSString* parentFolder = [@(parent_folder.c_str()) stringByReplacingOccurrencesOfString:@"../" - withString:@""]; + NSString* parentFolderRaw = @(parent_folder.c_str()); + if (parentFolderRaw == nil) { + return std::string(); + } + NSString* parentFolder = [parentFolderRaw stringByReplacingOccurrencesOfString:@"../" + withString:@""]; NSURL* parentFolderURL = [NSURL fileURLWithPath:parentFolder]; // The only reserved character in a file name on macOS is the forward-slash. It's unclear if iOS @@ -66,8 +70,12 @@ std::string ImplementationPlatform::GetCustomSavePath(const std::string& parent_ // """ // // See: https://en.wikipedia.org/wiki/Filename - NSString* fileName = [@(file_name.c_str()) stringByReplacingOccurrencesOfString:@"/" - withString:@":"]; + NSString* fileNameRaw = @(file_name.c_str()); + if (fileNameRaw == nil) { + return std::string(); + } + NSString* fileName = [fileNameRaw stringByReplacingOccurrencesOfString:@"/" + withString:@":"]; NSString* baseName = [fileName stringByDeletingPathExtension]; NSString* extension = [fileName pathExtension]; @@ -86,8 +94,12 @@ std::string ImplementationPlatform::GetCustomSavePath(const std::string& parent_ std::string ImplementationPlatform::GetDownloadPath(const std::string& parent_folder, const std::string& file_name) { + NSString* parentFolderRaw = @(parent_folder.c_str()); + if (parentFolderRaw == nil) { + return std::string(); + } NSString* customSavePath = - [NSTemporaryDirectory() stringByAppendingPathComponent:@(parent_folder.c_str())]; + [NSTemporaryDirectory() stringByAppendingPathComponent:parentFolderRaw]; return GetCustomSavePath(customSavePath.UTF8String, file_name); } diff --git a/internal/platform/implementation/apple/scheduled_executor.mm b/internal/platform/implementation/apple/scheduled_executor.mm index ce5f89fe..2f80b10a 100644 --- a/internal/platform/implementation/apple/scheduled_executor.mm +++ b/internal/platform/implementation/apple/scheduled_executor.mm @@ -21,6 +21,7 @@ #include #include "absl/time/time.h" +#import "internal/platform/implementation/apple/Log/GNCLogger.h" #include "internal/platform/runnable.h" // Defines the state of a scheduled task. This enum is at global scope @@ -93,7 +94,23 @@ class ExecutorCancelable : public nearby::api::Cancelable { namespace nearby { namespace apple { +namespace { +void ExecuteRunnable(Runnable &runnable) { + @try { + try { + runnable(); + } catch (const std::exception &e) { + GNCLoggerError(@"Runnable threw C++ exception: %s", e.what()); + } catch (...) { + GNCLoggerError(@"Runnable threw unknown C++ exception"); + } + } @catch (NSException *e) { + GNCLoggerError(@"Runnable threw ObjC exception: %@: %@", e.name, e.reason); + } +} + +} // namespace ScheduledExecutor::ScheduledExecutor() { impl_ = [GNCOperationQueueImpl implWithMaxConcurrency:1]; } @@ -106,7 +123,6 @@ ScheduledExecutor::~ScheduledExecutor() { impl_ = nil; } - std::shared_ptr ScheduledExecutor::Schedule(Runnable &&runnable, absl::Duration duration) { if (impl_.shuttingDown) return std::shared_ptr(nullptr); @@ -133,7 +149,7 @@ std::shared_ptr ScheduledExecutor::Schedule(Runnable &&runnable } GNCScheduledTaskState expected = GNCScheduledTaskState::kScheduled; if (task->_state.compare_exchange_strong(expected, GNCScheduledTaskState::kRunning)) { - task->_runnable(); + ExecuteRunnable(task->_runnable); task->_state.store(GNCScheduledTaskState::kDone); } }]; @@ -152,7 +168,7 @@ bool ScheduledExecutor::DoSubmit(Runnable &&runnable) { // Submit the runnable to the queue. __block Runnable local_runnable = std::move(runnable); [impl_.queue addOperationWithBlock:^{ - local_runnable(); + ExecuteRunnable(local_runnable); }]; return true; } diff --git a/sharing/internal/base/utf_string_conversions.h b/sharing/internal/base/utf_string_conversions.h index 545953d8..93a8d065 100644 --- a/sharing/internal/base/utf_string_conversions.h +++ b/sharing/internal/base/utf_string_conversions.h @@ -19,9 +19,9 @@ // Stub out string conversion functions for github builds. namespace nearby::utils { -bool IsStringUtf8(std::string_view str) { return true; } +inline bool IsStringUtf8(std::string_view str) { return true; } -void TruncateUtf8ToByteSize(const std::string& input, size_t byte_size, +inline void TruncateUtf8ToByteSize(const std::string& input, size_t byte_size, std::string* output) {} } // namespace nearby::utils @@ -29,11 +29,11 @@ void TruncateUtf8ToByteSize(const std::string& input, size_t byte_size, // Forward to chromium implementations. #include "base/strings/string_util.h" namespace nearby::utils { -bool IsStringUtf8(std::string_view str) { +inline bool IsStringUtf8(std::string_view str) { return base::IsStringUTF8(str); } -void TruncateUtf8ToByteSize(const std::string& input, size_t byte_size, +inline void TruncateUtf8ToByteSize(const std::string& input, size_t byte_size, std::string* output) { base::TruncateUTF8ToByteSize(input, byte_size, output); }