Validate file/folder names for UTF-8 and null characters in Nearby Connections.

PiperOrigin-RevId: 948685221

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(

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",
     ],

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",

connections/implementation/offline_frames_validator.cc

@@ -19,6 +19,8 @@
 #include <regex>  //NOLINT
 #include <string>
 
+#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])) !=
+    if (absl::StrContains(toBeValidated, illegalPatterns[index])) {
-        std::string::npos) {
       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(),
+      const std::string& file_name = frame.payload_header().file_name();
-                                    kIllegalFileNamePatterns,
+      if (CheckForIllegalCharacters(file_name, kIllegalFileNamePatterns,
                                     kIllegalFileNamePatternsSize)) {
-        LOG(ERROR) << "File name " << frame.payload_header().file_name()
+        LOG(ERROR) << "File name (hex) " << absl::BytesToHexString(file_name)
-                   << " has illegal characters";
+                   << " has illegal characters or invalid UTF-8";
         return {Exception::kIllegalCharacters};
       }
     }
     if (frame.payload_header().has_parent_folder()) {
-      if (CheckForIllegalCharacters(frame.payload_header().parent_folder(),
+      const std::string& parent_folder = frame.payload_header().parent_folder();
-                                    kIllegalParentFolderPatterns,
+      if (CheckForIllegalCharacters(parent_folder, kIllegalParentFolderPatterns,
                                     kIllegalParentFolderPatternsSize)) {
-        LOG(ERROR) << "Parent folder " << frame.payload_header().parent_folder()
+        LOG(ERROR) << "Parent folder (hex) "
-                   << " has illegal characters";
+                   << absl::BytesToHexString(parent_folder)
+                   << " has illegal characters or invalid UTF-8";
         return {Exception::kIllegalCharacters};
       }
     }

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,
+  std::string bytes =
-                                          "device_name");
+      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 =
+  std::string bytes = ForConnectionResponse(-1, os_info, "device_name");
-      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 =
+  std::string wifi_direct_ssid_64_length = "DIRECT-A0-" + std::string(54, 'A');
-      "DIRECT-A0-" + std::string(54, 'A');
   bytes = ForBwuWifiDirectPathAvailable(
       wifi_direct_ssid_64_length, std::string(kWifiDirectPassword), kPort,
       kWifiDirectFrequency, kSupportsDisablingEncryption, std::string(kGateway),

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:@"../"
+  NSString* parentFolderRaw = @(parent_folder.c_str());
-                                                                               withString:@""];
+  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:@"/"
+  NSString* fileNameRaw = @(file_name.c_str());
-                                                                       withString:@":"];
+  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);
 }
 

internal/platform/implementation/apple/scheduled_executor.mm

@@ -21,6 +21,7 @@
 #include <utility>
 
 #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<api::Cancelable> ScheduledExecutor::Schedule(Runnable &&runnable,
                                                              absl::Duration duration) {
   if (impl_.shuttingDown) return std::shared_ptr<api::Cancelable>(nullptr);
@@ -133,7 +149,7 @@ std::shared_ptr<api::Cancelable> 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;
 }

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);
 }