From a89b032b2bad17eafeb4e1c2c0b2d793ac64f7bf Mon Sep 17 00:00:00 2001
From: hamda mare <[email protected]>
Date: Wed, 29 Apr 2026 12:45:34 -0700
Subject: [PATCH] Fix potential UAF in PlistSettingsClient on macOS

This CL refactors ParsePlist to merge loops and add safety checks using
ObjCCast. It also replaces unsafe Key-Value Coding (KVC) method calls
with explicit dictionary lookups using objectForKey:. This prevents
untrusted input from being executed as Objective-C selectors, addressing
a potential Use-After-Free vulnerability.

Fixed: 501360342
Change-Id: I8058e32b8e1748888dd0fafca344bd77b6d1eea6
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/7796282
Reviewed-by: Sebastien Lalancette <[email protected]>
Commit-Queue: Hamda Mare <[email protected]>
Cr-Commit-Position: refs/heads/main@{#1622657}
---

diff --git a/components/device_signals/core/browser/mac/plist_settings_client.mm b/components/device_signals/core/browser/mac/plist_settings_client.mm
index e93ae0f..4eea4624 100644
--- a/components/device_signals/core/browser/mac/plist_settings_client.mm
+++ b/components/device_signals/core/browser/mac/plist_settings_client.mm
@@ -65,7 +65,7 @@
     }
 
     NSUInteger index = base::checked_cast<NSUInteger>(index_str.integerValue);
-    if (index > data_array.count) {
+    if (index >= data_array.count) {
       return nil;
     }
 
@@ -78,30 +78,39 @@
 // Parses the loaded plist `dict` for the setting item at `key_path`. Returns
 // the setting object if it is found or nil otherwise.
 id ParsePlist(NSDictionary* dict, NSString* key_path) {
-  // Check if an array exists in the path, If not, the plist can be parsed
-  // directly.
-  NSRange test_range = [key_path rangeOfString:@"["];
-  if (test_range.location == NSNotFound)
-    return [dict valueForKeyPath:key_path];
-
-  NSDictionary* current_obj = dict;
+  id current_obj = dict;
+  bool has_brackets = false;
   for (NSString* sub_path in [key_path componentsSeparatedByString:@"."]) {
+    NSDictionary* current_dict =
+        base::apple::ObjCCast<NSDictionary>(current_obj);
+    if (!current_dict) {
+      return nil;
+    }
+
     NSRange range = [sub_path rangeOfString:@"["];
     if (range.location == NSNotFound) {
-      current_obj = [current_obj valueForKey:sub_path];
+      current_obj = [current_dict objectForKey:sub_path];
     } else {
-      current_obj =
-          [current_obj valueForKey:[sub_path substringToIndex:range.location]];
+      has_brackets = true;
+      current_obj = [current_dict
+          objectForKey:[sub_path substringToIndex:range.location]];
       current_obj = ParseArrays(current_obj,
                                 [sub_path substringFromIndex:range.location]);
     }
+
+    if (!current_obj) {
+      return nil;
+    }
   }
 
   // This will occur if the key path is incorrect and does not actually point to
   // a setting item. At the end of a parse, the only remaining object should be
   // the single setting item.
-  if ([current_obj isKindOfClass:[NSArray class]] && current_obj.count != 1) {
-    return nil;
+  if (has_brackets) {
+    NSArray* final_array = base::apple::ObjCCast<NSArray>(current_obj);
+    if (final_array && final_array.count != 1) {
+      return nil;
+    }
   }
   return current_obj;
 }
diff --git a/components/device_signals/core/browser/mac/plist_settings_client_unittest.mm b/components/device_signals/core/browser/mac/plist_settings_client_unittest.mm
index 5172a51d..581ef96 100644
--- a/components/device_signals/core/browser/mac/plist_settings_client_unittest.mm
+++ b/components/device_signals/core/browser/mac/plist_settings_client_unittest.mm
@@ -299,4 +299,40 @@
   EXPECT_EQ(items, future_.Get());
 }
 
+// Tests a request to GetSettings with an array index that is exactly equal to
+// the array size. This should be handled as "Not Found".
+TEST_F(PlistSettingsClientTest,
+       GetSettings_Plist_MixOfArrayDictItems_ArrayOutOfBounds) {
+  test_file_path_ = test::GetMixArrayDictionaryPlistPath();
+
+  std::string key_path = "Key1.Array[2]";
+
+  std::vector<GetSettingsOptions> options;
+  options.push_back(CreateOption(key_path, true));
+
+  std::vector<SettingsItem> items;
+  items.push_back(CreateSettingItem(key_path, PresenceValue::kNotFound, ""));
+
+  client_.GetSettings(options, future_.GetCallback());
+  EXPECT_EQ(items, future_.Get());
+}
+
+// Tests that a key path containing a KVC operator is not executed and is
+// instead treated as a literal key (which should not be found).
+TEST_F(PlistSettingsClientTest,
+       GetSettings_Plist_OnlyDictionaryItems_KVCVulnerability) {
+  test_file_path_ = test::GetOnlyDictionaryPlistPath();
+
+  std::string key_path = "@count";
+
+  std::vector<GetSettingsOptions> options;
+  options.push_back(CreateOption(key_path, true));
+
+  std::vector<SettingsItem> items;
+  items.push_back(CreateSettingItem(key_path, PresenceValue::kNotFound, ""));
+
+  client_.GetSettings(options, future_.GetCallback());
+  EXPECT_EQ(items, future_.Get());
+}
+
 }  // namespace device_signals

diff --git a/components/device_signals/core/browser/mac/plist_settings_client_unittest.mm b/components/device_signals/core/browser/mac/plist_settings_client_unittest.mm
index 5172a51d..581ef96 100644
--- a/components/device_signals/core/browser/mac/plist_settings_client_unittest.mm
+++ b/components/device_signals/core/browser/mac/plist_settings_client_unittest.mm
@@ -299,4 +299,40 @@
   EXPECT_EQ(items, future_.Get());
 }
 
+// Tests a request to GetSettings with an array index that is exactly equal to
+// the array size. This should be handled as "Not Found".
+TEST_F(PlistSettingsClientTest,
+       GetSettings_Plist_MixOfArrayDictItems_ArrayOutOfBounds) {
+  test_file_path_ = test::GetMixArrayDictionaryPlistPath();
+
+  std::string key_path = "Key1.Array[2]";
+
+  std::vector<GetSettingsOptions> options;
+  options.push_back(CreateOption(key_path, true));
+
+  std::vector<SettingsItem> items;
+  items.push_back(CreateSettingItem(key_path, PresenceValue::kNotFound, ""));
+
+  client_.GetSettings(options, future_.GetCallback());
+  EXPECT_EQ(items, future_.Get());
+}
+
+// Tests that a key path containing a KVC operator is not executed and is
+// instead treated as a literal key (which should not be found).
+TEST_F(PlistSettingsClientTest,
+       GetSettings_Plist_OnlyDictionaryItems_KVCVulnerability) {
+  test_file_path_ = test::GetOnlyDictionaryPlistPath();
+
+  std::string key_path = "@count";
+
+  std::vector<GetSettingsOptions> options;
+  options.push_back(CreateOption(key_path, true));
+
+  std::vector<SettingsItem> items;
+  items.push_back(CreateSettingItem(key_path, PresenceValue::kNotFound, ""));
+
+  client_.GetSettings(options, future_.GetCallback());
+  EXPECT_EQ(items, future_.Get());
+}
+
 }  // namespace device_signals