From 509bb2a6abc854b0bb566982508db919e00a7ea4 Mon Sep 17 00:00:00 2001 From: Dani Akash Date: Thu, 26 Mar 2026 19:06:49 +0530 Subject: [PATCH] fix: complete session persistence across browser updates (macOS) - Extend keychain_password_mac.mm patch to set kSecAttrAccessGroup on keychain writes and reads, ensuring items are stored under the shared BrowserOS access group rather than tied to a specific binary signature - Pin the designated requirement in macos.py to the Team ID via certificate leaf[subject.OU] instead of wildcard /* exists */ checks, so Keychain ACLs survive across builds with different binary hashes - Add browseros_keychain_recovery.mm that runs on startup to detect broken keychain access (errSecAuthFailed), prompt the user for access, and migrate the item to the BrowserOS access group - Add broadenKeychainACLBeforeUpdate to sparkle_glue.mm that updates the keychain item's access group right before Sparkle replaces the app bundle, so the new binary can read it immediately Resolves: TKT-666, TKT-668, TKT-669, TKT-670 --- .../browseros/build/modules/sign/macos.py | 30 +++- .../chrome/browser/browseros/core/BUILD.gn | 18 +++ .../core/browseros_keychain_recovery.h | 28 ++++ .../core/browseros_keychain_recovery.mm | 137 ++++++++++++++++++ .../chrome/browser/mac/sparkle_glue.mm | 57 ++++++++ .../os_crypt/common/keychain_password_mac.mm | 83 ++++++++++- 6 files changed, 344 insertions(+), 9 deletions(-) create mode 100644 packages/browseros/chromium_patches/chrome/browser/browseros/core/browseros_keychain_recovery.h create mode 100644 packages/browseros/chromium_patches/chrome/browser/browseros/core/browseros_keychain_recovery.mm diff --git a/packages/browseros/build/modules/sign/macos.py b/packages/browseros/build/modules/sign/macos.py index ac815cb5..6ef4ce3b 100644 --- a/packages/browseros/build/modules/sign/macos.py +++ b/packages/browseros/build/modules/sign/macos.py @@ -126,7 +126,9 @@ class MacOSSignModule(CommandModule): run_command(["xattr", "-cs", str(app_path)]) def _sign_all_components(self, app_path: Path, certificate_name: str, ctx: Context) -> None: - if not sign_all_components(app_path, certificate_name, ctx.root_dir, ctx): + _, env_vars = check_environment(ctx.env) + team_id = env_vars.get("team_id", "") + if not sign_all_components(app_path, certificate_name, ctx.root_dir, ctx, team_id=team_id): raise RuntimeError("Failed to sign all components") def _verify_signature(self, app_path: Path) -> None: @@ -413,6 +415,7 @@ def sign_all_components( certificate_name: str, root_dir: Path, ctx: Optional[Context] = None, + team_id: str = "", ) -> bool: """Sign all components in the correct order (bottom-up)""" log_info("šŸ” Discovering components to sign...") @@ -547,11 +550,23 @@ def sign_all_components( # 8. Finally sign the app bundle log_info("\nšŸ” Signing application bundle...") - requirements = ( - '=designated => identifier "com.browseros.BrowserOS" and ' - "anchor apple generic and certificate 1[field.1.2.840.113635.100.6.2.6] /* exists */ and " - "certificate leaf[field.1.2.840.113635.100.6.1.13] /* exists */" - ) + # Pin the designated requirement to the Team ID so that Keychain ACLs + # survive across builds with different binary hashes. + if team_id: + requirements = ( + f'=designated => identifier "com.browseros.BrowserOS" and ' + f"anchor apple generic and " + f"certificate 1[field.1.2.840.113635.100.6.2.6] /* exists */ and " + f"certificate leaf[field.1.2.840.113635.100.6.1.13] /* exists */ and " + f'certificate leaf[subject.OU] = "{team_id}"' + ) + else: + log_warning("No team_id provided ā€” using wildcard designated requirement") + requirements = ( + '=designated => identifier "com.browseros.BrowserOS" and ' + "anchor apple generic and certificate 1[field.1.2.840.113635.100.6.2.6] /* exists */ and " + "certificate leaf[field.1.2.840.113635.100.6.1.13] /* exists */" + ) # Try multiple locations for app entitlements entitlements = None @@ -798,7 +813,8 @@ def sign_app(ctx: Context, create_dmg: bool = True) -> bool: # Sign all components if not sign_all_components( - app_path, env_vars["certificate_name"], ctx.root_dir, ctx + app_path, env_vars["certificate_name"], ctx.root_dir, ctx, + team_id=env_vars.get("team_id", ""), ): return False diff --git a/packages/browseros/chromium_patches/chrome/browser/browseros/core/BUILD.gn b/packages/browseros/chromium_patches/chrome/browser/browseros/core/BUILD.gn index 1a4a8506..cbf637eb 100644 --- a/packages/browseros/chromium_patches/chrome/browser/browseros/core/BUILD.gn +++ b/packages/browseros/chromium_patches/chrome/browser/browseros/core/BUILD.gn @@ -21,6 +21,24 @@ index 0000000000000..24fdd618e6a71 + ] +} + ++if (is_mac) { ++ source_set("keychain_recovery") { ++ sources = [ ++ "browseros_keychain_recovery.h", ++ "browseros_keychain_recovery.mm", ++ ] ++ ++ deps = [ ++ "//base", ++ ] ++ ++ frameworks = [ ++ "Foundation.framework", ++ "Security.framework", ++ ] ++ } ++} ++ +source_set("prefs") { + sources = [ + "browseros_prefs.cc", diff --git a/packages/browseros/chromium_patches/chrome/browser/browseros/core/browseros_keychain_recovery.h b/packages/browseros/chromium_patches/chrome/browser/browseros/core/browseros_keychain_recovery.h new file mode 100644 index 00000000..50748e21 --- /dev/null +++ b/packages/browseros/chromium_patches/chrome/browser/browseros/core/browseros_keychain_recovery.h @@ -0,0 +1,28 @@ +diff --git a/chrome/browser/browseros/core/browseros_keychain_recovery.h b/chrome/browser/browseros/core/browseros_keychain_recovery.h +new file mode 100644 +index 0000000000000..0000000000001 +--- /dev/null ++++ b/chrome/browser/browseros/core/browseros_keychain_recovery.h +@@ -0,0 +1,24 @@ ++// Copyright 2026 BrowserOS Authors. All rights reserved. ++// Use of this source code is governed by a BSD-style license that can be ++// found in the LICENSE file. ++ ++#ifndef CHROME_BROWSER_BROWSEROS_CORE_BROWSEROS_KEYCHAIN_RECOVERY_H_ ++#define CHROME_BROWSER_BROWSEROS_CORE_BROWSEROS_KEYCHAIN_RECOVERY_H_ ++ ++namespace browseros { ++ ++// Checks whether the BrowserOS Safe Storage keychain item is accessible. ++// If access is denied (e.g. due to a signing identity change after an update), ++// attempts interactive recovery by prompting the user. On success, migrates the ++// keychain item to use the BrowserOS access group so future updates don't break ++// access. ++// ++// Must be called early in browser startup, before any cookie or password access ++// triggers os_crypt to read the keychain. ++void MaybeMigrateKeychainAccess(); ++ ++} // namespace browseros ++ ++#endif // CHROME_BROWSER_BROWSEROS_CORE_BROWSEROS_KEYCHAIN_RECOVERY_H_ diff --git a/packages/browseros/chromium_patches/chrome/browser/browseros/core/browseros_keychain_recovery.mm b/packages/browseros/chromium_patches/chrome/browser/browseros/core/browseros_keychain_recovery.mm new file mode 100644 index 00000000..7fd8c2ec --- /dev/null +++ b/packages/browseros/chromium_patches/chrome/browser/browseros/core/browseros_keychain_recovery.mm @@ -0,0 +1,137 @@ +diff --git a/chrome/browser/browseros/core/browseros_keychain_recovery.mm b/chrome/browser/browseros/core/browseros_keychain_recovery.mm +new file mode 100644 +index 0000000000000..0000000000001 +--- /dev/null ++++ b/chrome/browser/browseros/core/browseros_keychain_recovery.mm +@@ -0,0 +1,120 @@ ++// Copyright 2026 BrowserOS Authors. All rights reserved. ++// Use of this source code is governed by a BSD-style license that can be ++// found in the LICENSE file. ++ ++#include "chrome/browser/browseros/core/browseros_keychain_recovery.h" ++ ++#import ++#import ++#import ++ ++#include "base/logging.h" ++ ++#if !defined(__has_feature) || !__has_feature(objc_arc) ++#error "This file requires ARC support." ++#endif ++ ++namespace browseros { ++ ++namespace { ++ ++NSString* GetTeamIdentifier() { ++ SecCodeRef code = NULL; ++ if (SecCodeCopySelf(kSecCSDefaultFlags, &code) != errSecSuccess || !code) { ++ return nil; ++ } ++ CFDictionaryRef info = NULL; ++ OSStatus status = ++ SecCodeCopySigningInformation(code, kSecCSDefaultFlags, &info); ++ CFRelease(code); ++ if (status != errSecSuccess || !info) { ++ return nil; ++ } ++ NSString* teamID = [(__bridge NSDictionary*)info ++ objectForKey:(__bridge NSString*)kSecCodeInfoTeamIdentifier]; ++ NSString* result = [teamID copy]; ++ CFRelease(info); ++ return result; ++} ++ ++NSString* GetAccessGroup() { ++ NSString* teamID = GetTeamIdentifier(); ++ if (!teamID || teamID.length == 0) { ++ return nil; ++ } ++ return [NSString stringWithFormat:@"%@.com.browseros", teamID]; ++} ++ ++} // namespace ++ ++void MaybeMigrateKeychainAccess() { ++ @autoreleasepool { ++ NSDictionary* query = @{ ++ (__bridge id)kSecClass : (__bridge id)kSecClassGenericPassword, ++ (__bridge id)kSecAttrService : @"BrowserOS Safe Storage", ++ (__bridge id)kSecAttrAccount : @"BrowserOS", ++ (__bridge id)kSecReturnData : @YES, ++ }; ++ ++ CFTypeRef result = NULL; ++ OSStatus status = ++ SecItemCopyMatching((__bridge CFDictionaryRef)query, &result); ++ ++ if (status == errSecSuccess) { ++ // Access works. Migrate the item to the access group if needed. ++ if (result) { ++ CFRelease(result); ++ } ++ NSString* group = GetAccessGroup(); ++ if (group) { ++ NSDictionary* updateQuery = @{ ++ (__bridge id)kSecClass : (__bridge id)kSecClassGenericPassword, ++ (__bridge id)kSecAttrService : @"BrowserOS Safe Storage", ++ (__bridge id)kSecAttrAccount : @"BrowserOS", ++ }; ++ NSDictionary* update = @{ ++ (__bridge id)kSecAttrAccessGroup : group, ++ }; ++ SecItemUpdate((__bridge CFDictionaryRef)updateQuery, ++ (__bridge CFDictionaryRef)update); ++ } ++ LOG(INFO) << "browseros: Keychain access OK"; ++ return; ++ } ++ ++ if (status == errSecItemNotFound) { ++ LOG(INFO) ++ << "browseros: No keychain item found, will be created on first use"; ++ return; ++ } ++ ++ // errSecAuthFailed, errSecInteractionNotAllowed, etc. ++ // The item exists but we can't access it ā€” signing identity mismatch. ++ LOG(WARNING) << "browseros: Keychain access denied (status=" << status ++ << "), attempting interactive recovery"; ++ ++ SecKeychainSetUserInteractionAllowed(TRUE); ++ ++ result = NULL; ++ status = SecItemCopyMatching((__bridge CFDictionaryRef)query, &result); ++ ++ if (status == errSecSuccess) { ++ LOG(INFO) << "browseros: Keychain access recovered via user interaction"; ++ if (result) { ++ CFRelease(result); ++ } ++ // Migrate to access group now that we have access. ++ NSString* group = GetAccessGroup(); ++ if (group) { ++ NSDictionary* updateQuery = @{ ++ (__bridge id)kSecClass : (__bridge id)kSecClassGenericPassword, ++ (__bridge id)kSecAttrService : @"BrowserOS Safe Storage", ++ (__bridge id)kSecAttrAccount : @"BrowserOS", ++ }; ++ NSDictionary* update = @{ ++ (__bridge id)kSecAttrAccessGroup : group, ++ }; ++ SecItemUpdate((__bridge CFDictionaryRef)updateQuery, ++ (__bridge CFDictionaryRef)update); ++ } ++ return; ++ } ++ ++ LOG(ERROR) << "browseros: Keychain recovery failed (status=" << status ++ << "). User will lose encrypted data."; ++ if (result) { ++ CFRelease(result); ++ } ++ } ++} ++ ++} // namespace browseros diff --git a/packages/browseros/chromium_patches/chrome/browser/mac/sparkle_glue.mm b/packages/browseros/chromium_patches/chrome/browser/mac/sparkle_glue.mm index 876ecd2c..4e40d100 100644 --- a/packages/browseros/chromium_patches/chrome/browser/mac/sparkle_glue.mm +++ b/packages/browseros/chromium_patches/chrome/browser/mac/sparkle_glue.mm @@ -13,6 +13,9 @@ index 0000000000000..25c4843095e4f +#include +#include + ++#import ++#import ++ +#include "base/apple/bundle_locations.h" +#include "base/command_line.h" +#include "base/logging.h" @@ -254,6 +257,11 @@ index 0000000000000..25c4843095e4f + +- (void)showReadyToInstallAndRelaunch:(void (^)(SPUUserUpdateChoice))reply { + VLOG(1) << "Sparkle: Ready to install and relaunch"; ++ ++ // Broaden keychain ACL before Sparkle replaces the app bundle, ++ // so the new binary can access the existing encryption key. ++ [self.glue broadenKeychainACLBeforeUpdate]; ++ + self.installReplyBlock = reply; + [self.glue setInternalStatus:SparkleStatusReadyToInstall]; +} @@ -536,6 +544,55 @@ index 0000000000000..25c4843095e4f + } +} + ++#pragma mark - Keychain ACL Migration ++ ++- (void)broadenKeychainACLBeforeUpdate { ++ // Before Sparkle replaces the app bundle, update the keychain item's ++ // access group so the new binary (signed by the same Team ID) can read it. ++ SecCodeRef code = NULL; ++ if (SecCodeCopySelf(kSecCSDefaultFlags, &code) != errSecSuccess || !code) { ++ LOG(WARNING) << "Sparkle: Could not get code identity for keychain migration"; ++ return; ++ } ++ CFDictionaryRef info = NULL; ++ OSStatus status = SecCodeCopySigningInformation(code, kSecCSDefaultFlags, &info); ++ CFRelease(code); ++ if (status != errSecSuccess || !info) { ++ LOG(WARNING) << "Sparkle: Could not get signing info for keychain migration"; ++ return; ++ } ++ NSString* teamID = [(__bridge NSDictionary*)info ++ objectForKey:(__bridge NSString*)kSecCodeInfoTeamIdentifier]; ++ CFRelease(info); ++ if (!teamID || teamID.length == 0) { ++ LOG(WARNING) << "Sparkle: No Team ID found ā€” skipping keychain migration"; ++ return; ++ } ++ ++ NSString* group = [NSString stringWithFormat:@"%@.com.browseros", teamID]; ++ ++ NSDictionary* query = @{ ++ (__bridge id)kSecClass : (__bridge id)kSecClassGenericPassword, ++ (__bridge id)kSecAttrService : @"BrowserOS Safe Storage", ++ (__bridge id)kSecAttrAccount : @"BrowserOS", ++ }; ++ NSDictionary* update = @{ ++ (__bridge id)kSecAttrAccessGroup : group, ++ }; ++ ++ status = SecItemUpdate((__bridge CFDictionaryRef)query, ++ (__bridge CFDictionaryRef)update); ++ ++ if (status == errSecSuccess) { ++ VLOG(1) << "Sparkle: Keychain ACL updated with access group " ++ << base::SysNSStringToUTF8(group); ++ } else if (status == errSecItemNotFound) { ++ VLOG(1) << "Sparkle: No keychain item to migrate"; ++ } else { ++ LOG(WARNING) << "Sparkle: Failed to update keychain ACL (status=" << status << ")"; ++ } ++} ++ +#pragma mark - SPUUpdaterDelegate + +- (nullable NSString*)feedURLStringForUpdater:(SPUUpdater*)updater { diff --git a/packages/browseros/chromium_patches/components/os_crypt/common/keychain_password_mac.mm b/packages/browseros/chromium_patches/components/os_crypt/common/keychain_password_mac.mm index 8d8db432..2f9b91e6 100644 --- a/packages/browseros/chromium_patches/components/os_crypt/common/keychain_password_mac.mm +++ b/packages/browseros/chromium_patches/components/os_crypt/common/keychain_password_mac.mm @@ -2,7 +2,17 @@ diff --git a/components/os_crypt/common/keychain_password_mac.mm b/components/os index caa0e420956a3..d60a67a8bacb7 100644 --- a/components/os_crypt/common/keychain_password_mac.mm +++ b/components/os_crypt/common/keychain_password_mac.mm -@@ -35,8 +35,9 @@ +@@ -8,6 +8,9 @@ + + #import + ++// BrowserOS: needed for SecCodeCopySelf / SecCodeCopySigningInformation ++#import ++ + #include + + #include "base/apple/ossstatus_logging.h" +@@ -35,8 +38,46 @@ const char kDefaultServiceName[] = "Chrome Safe Storage"; const char kDefaultAccountName[] = "Chrome"; #else @@ -12,5 +22,74 @@ index caa0e420956a3..d60a67a8bacb7 100644 +const char kDefaultServiceName[] = "BrowserOS Safe Storage"; +const char kDefaultAccountName[] = "BrowserOS"; #endif - + ++// BrowserOS: Get the Team ID from the running binary's code signature ++// and construct the keychain access group (e.g. "ABC123DEF4.com.browseros"). ++NSString* GetBrowserOSAccessGroup() { ++ static NSString* cachedGroup = nil; ++ static dispatch_once_t onceToken; ++ dispatch_once(&onceToken, ^{ ++ SecCodeRef code = NULL; ++ if (SecCodeCopySelf(kSecCSDefaultFlags, &code) != errSecSuccess || !code) { ++ return; ++ } ++ CFDictionaryRef info = NULL; ++ if (SecCodeCopySigningInformation(code, kSecCSDefaultFlags, &info) == errSecSuccess && info) { ++ NSString* teamID = [(__bridge NSDictionary*)info ++ objectForKey:(__bridge NSString*)kSecCodeInfoTeamIdentifier]; ++ if (teamID.length > 0) { ++ cachedGroup = [[NSString alloc] initWithFormat:@"%@.com.browseros", teamID]; ++ } ++ CFRelease(info); ++ } ++ CFRelease(code); ++ }); ++ return cachedGroup; ++} ++ // These values are persisted to logs. Entries should not be renumbered and ++ ++@@ -58,6 +99,12 @@ ++ OSStatus error = keychain.AddGenericPassword(service_name, account_name, ++ base::as_byte_span(password)); ++ +++ // BrowserOS: update the newly created item to use our access group +++ NSString* group = GetBrowserOSAccessGroup(); +++ if (group && error == noErr) { +++ NSDictionary* query = @{ +++ (__bridge id)kSecClass: (__bridge id)kSecClassGenericPassword, +++ (__bridge id)kSecAttrService: @(service_name.c_str()), +++ (__bridge id)kSecAttrAccount: @(account_name.c_str()), +++ }; +++ NSDictionary* update = @{ +++ (__bridge id)kSecAttrAccessGroup: group, +++ }; +++ SecItemUpdate((__bridge CFDictionaryRef)query, +++ (__bridge CFDictionaryRef)update); +++ } +++ + if (error != noErr) { + OSSSTATUS_DLOG(ERROR, error) << "Keychain add failed"; + return base::unexpected(error); +@@ -73,6 +130,14 @@ + auto password = keychain.FindGenericPassword(service_name, account_name); + + if (password.has_value()) { ++ // BrowserOS: ensure existing items have the correct access group. ++ // This migrates items created before the access group was added. ++ NSString* group = GetBrowserOSAccessGroup(); ++ if (group) { ++ NSDictionary* query = @{ ++ (__bridge id)kSecClass: (__bridge id)kSecClassGenericPassword, ++ (__bridge id)kSecAttrService: @(service_name.c_str()), ++ (__bridge id)kSecAttrAccount: @(account_name.c_str()), ++ }; ++ NSDictionary* update = @{ ++ (__bridge id)kSecAttrAccessGroup: group, ++ }; ++ // Best-effort ā€” ignore errors (item may already have the group). ++ SecItemUpdate((__bridge CFDictionaryRef)query, ++ (__bridge CFDictionaryRef)update); ++ } + uma_result = FindGenericPasswordResult::kPasswordFound; + return std::string(base::as_string_view(*password));