Merge pull request #193 from square/dfed--fix-shared-access-group-cat…

…alina Fix keychains that rely on kSecAttrAccessGroup on macOS Catalina
square · Jan 14, 2020 · 7447d16 · 7447d16
2 parents 50e134e + ad3ab18
commit 7447d16
Show file tree

Hide file tree

Showing 10 changed files with 120 additions and 11 deletions.
diff --git a/Package.swift b/Package.swift
@@ -23,4 +23,4 @@ let package = Package(
     ],
     swiftLanguageVersions: [.v4, .v4_2, .v5]
 )
-let version = Version(3, 2, 7)
+let version = Version(3, 2, 8)
diff --git a/README.md b/README.md
@@ -171,6 +171,10 @@ However, because the Keychain is effectively disk storage, there is no guarantee
 
 Already using the Keychain and no longer want to maintain your own Keychain code? We feel you. That’s why we wrote `migrateObjects(matching query: [String : AnyHashable], removeOnCompletion: Bool)`. This method allows you to migrate all your existing Keychain entries to a Valet instance in one line. Just pass in a Dictionary with the `kSecClass`, `kSecAttrService`, and any other `kSecAttr*` attributes you use – we’ll migrate the data for you.
 
+### Supporting macOS Catalina (10.15) and later
+
+If your macOS application supports macOS 10.14 or prior, you must run `myValet.migrateObjectsFromPreCatalina()` before reading values from a Valet. macOS Catalina introduced a breaking change to the macOS keychain, requiring that macOS keychain items that utilize `kSecAttrAccessible` or `kSecAttrAccessGroup` set `kSecUseDataProtectionKeychain` to `true` [when writing or accessing](https://developer.apple.com/documentation/security/ksecusedataprotectionkeychain) these items. Valet’s `migrateObjectsFromPreCatalina()` upgrades items entered into the keychain on older macOS devices or other operating systems to include the key:value pair `kSecUseDataProtectionKeychain:true`. Note that Valets that share keychain items between devices with iCloud are exempt from this requirement. Similarly, `SecureEnclaveValet` and `SinglePromptSecureEnclaveValet` are exempt from this requirement.
+
 ### Debugging
 
 Valet guarantees it will never fail to write to or read from the keychain unless `canAccessKeychain()` returns `false`. There are only a few cases that can lead to the keychain being inaccessible:

diff --git a/Sources/Valet/Internal/SecItem.swift b/Sources/Valet/Internal/SecItem.swift
@@ -70,23 +70,32 @@ internal final class SecItem {
 
     /// Programatically grab the required prefix for the shared access group (i.e. Bundle Seed ID). The value for the kSecAttrAccessGroup key in queries for data that is shared between apps must be of the format bundleSeedID.sharedAccessGroup. For more information on the Bundle Seed ID, see https://developer.apple.com/library/ios/qa/qa1713/_index.html
     internal static var sharedAccessGroupPrefix: String {
-        let query = [
+        var query: [CFString : Any] = [
             kSecClass : kSecClassGenericPassword,
             kSecAttrAccount : "SharedAccessGroupAlwaysAccessiblePrefixPlaceholder",
             kSecReturnAttributes : true,
-            kSecAttrAccessible : Accessibility.alwaysThisDeviceOnly.secAccessibilityAttribute
-            ] as CFDictionary
-
+            kSecAttrAccessible : Accessibility.alwaysThisDeviceOnly.secAccessibilityAttribute,
+        ]
+
+        if #available(iOS 13.0, tvOS 13.0, watchOS 6.0, macOS 10.15, *) {
+            // Add kSecUseDataProtectionKeychain to the query to ensure we can retrieve the shared access group prefix.
+            #if swift(>=5.1)
+            query[kSecUseDataProtectionKeychain] = true
+            #else
+            query["nleg" as CFString] = true // kSecUseDataProtectionKeychain for Xcode 9 and Xcode 10 compatibility.
+            #endif
+        }
+
         secItemLock.lock()
         defer {
             secItemLock.unlock()
         }
 
         var result: AnyObject? = nil
-        var status = SecItemCopyMatching(query, &result)
+        var status = SecItemCopyMatching(query as CFDictionary, &result)
 
         if status == errSecItemNotFound {
-            status = SecItemAdd(query, &result)
+            status = SecItemAdd(query as CFDictionary, &result)
         }
 
         guard status == errSecSuccess, let queryResult = result as? [CFString : AnyHashable], let accessGroup = queryResult[kSecAttrAccessGroup] as? String else {

diff --git a/Sources/Valet/Internal/Service.swift b/Sources/Valet/Internal/Service.swift
@@ -42,9 +42,17 @@ internal enum Service: CustomStringConvertible, Equatable {
     internal func generateBaseQuery() -> [String : AnyHashable] {
         var baseQuery: [String : AnyHashable] = [
             kSecClass as String : kSecClassGenericPassword as String,
-            kSecAttrService as String : secService
+            kSecAttrService as String : secService,
         ]
-
+
+        if #available(iOS 13.0, tvOS 13.0, watchOS 6.0, macOS 10.15, *) {
+            #if swift(>=5.1)
+            baseQuery[kSecUseDataProtectionKeychain as String] = true
+            #else
+            baseQuery["nleg"] = true // kSecUseDataProtectionKeychain for Xcode 9 and Xcode 10 compatibility.
+            #endif
+        }
+
         let configuration: Configuration
         switch self {
         case let .standard(_, desiredConfiguration):

diff --git a/Sources/Valet/Valet.swift b/Sources/Valet/Valet.swift
@@ -242,6 +242,25 @@ public final class Valet: NSObject, KeychainQueryConvertible {
         return migrateObjects(matching: keychain.keychainQuery, removeOnCompletion: removeOnCompletion)
     }
 
+    #if os(macOS)
+    /// Migrates objects that were written to this Valet prior to macOS 10.15 to a format that can be read on macOS 10.15 and later. The new format is backwards compatible, allowing these values to be read on older operating systems.
+    /// - returns: Whether the migration succeeded or failed.
+    /// - note: The keychain is not modified if a failure occurs. This method can only be called from macOS 10.15 or later.
+    @available(macOS 10.15, *)
+    @objc(migrateObjectsFromPreCatalina)
+    public func migrateObjectsFromPreCatalina() -> MigrationResult {
+        var baseQuery = keychainQuery
+        #if swift(>=5.1)
+        baseQuery[kSecUseDataProtectionKeychain as String] = false
+        #else
+        baseQuery["nleg"] = false // kSecUseDataProtectionKeychain for Xcode 9 and Xcode 10 compatibility.
+        #endif
+
+        // We do not need to remove these items on completion, since we are updating the kSecUseDataProtectionKeychain attribute in-place.
+        return migrateObjects(matching: baseQuery, removeOnCompletion: false)
+    }
+    #endif
+
     // MARK: Internal Properties
 
     internal let configuration: Configuration

diff --git a/.../ValetIntegrationTests/BackwardsCompatibilityTests/ValetBackwardsCompatibilityTests.swift b/.../ValetIntegrationTests/BackwardsCompatibilityTests/ValetBackwardsCompatibilityTests.swift
@@ -93,6 +93,11 @@ class ValetBackwardsCompatibilityIntegrationTests: ValetIntegrationTests {
             legacyValet.setString(passcode, forKey: key)
 
             XCTAssertNotNil(legacyValet.string(forKey: key))
+            if #available(OSX 10.15, *) {
+                #if os(macOS)
+                _ = permutation.migrateObjectsFromPreCatalina()
+                #endif
+            }
             XCTAssertEqual(legacyValet.string(forKey: key), permutation.string(forKey: key), "\(permutation) was not able to read from legacy counterpart: \(legacyValet)")
         }
     }
@@ -102,6 +107,11 @@ class ValetBackwardsCompatibilityIntegrationTests: ValetIntegrationTests {
             legacyValet.setString(passcode, forKey: key)
 
             XCTAssertNotNil(legacyValet.string(forKey: key))
+            if #available(OSX 10.15, *) {
+                #if os(macOS)
+                _ = permutation.migrateObjectsFromPreCatalina()
+                #endif
+            }
             XCTAssertEqual(legacyValet.string(forKey: key), permutation.string(forKey: key), "\(permutation) was not able to read from legacy counterpart: \(legacyValet)")
         }
     }

diff --git a/Tests/ValetIntegrationTests/MacTests.swift b/Tests/ValetIntegrationTests/MacTests.swift
@@ -91,5 +91,38 @@ class ValetMacTests: XCTestCase
         // If you add a breakpoint here then manually inspect the keychain via Keychain.app (search for "MacOSVulnTest"), "xctest" should be the only member of the Access Control list.
         // This is not be the case upon setting a breakpoint and inspecting before the valet.setString(, forKey:) call above.
     }
+
+    // MARK: Migration - PreCatalina
+
+    func test_migrateObjectsFromPreCatalina_migratesDataWrittenPreCatalina() {
+        guard #available(macOS 10.15, *) else {
+            return
+        }
+        guard testEnvironmentIsSigned() else {
+            return
+        }
+
+        let valet = Valet.valet(with: Identifier(nonEmpty: "PreCatalinaTest")!, accessibility: .afterFirstUnlock)
+        var preCatalinaWriteQuery = valet.keychainQuery
+        #if swift(>=5.1)
+        preCatalinaWriteQuery[kSecUseDataProtectionKeychain as String] = nil
+        #else
+        preCatalinaWriteQuery["nleg"] = nil // kSecUseDataProtectionKeychain for Xcode 9 and Xcode 10 compatibility.
+        #endif
+
+        let key = "PreCatalinaKey"
+        let object = Data("PreCatalinaValue".utf8)
+        preCatalinaWriteQuery[kSecAttrAccount as String] = key
+        preCatalinaWriteQuery[kSecValueData as String] = object
+
+        // Make sure the item is not in the keychain before we start this test
+        SecItemDelete(preCatalinaWriteQuery as CFDictionary)
+
+        XCTAssertEqual(SecItemAdd(preCatalinaWriteQuery as CFDictionary, nil), errSecSuccess)
+        XCTAssertNil(valet.object(forKey: key))
+        XCTAssertEqual(valet.migrateObjectsFromPreCatalina(), .success)
+        XCTAssertEqual(valet.object(forKey: key), object)
+    }
+
 }
 #endif
diff --git a/Tests/ValetIntegrationTests/SecItemTests.swift b/Tests/ValetIntegrationTests/SecItemTests.swift
@@ -34,7 +34,10 @@ class SecItemTests: XCTestCase {
                 XCTAssertEqual(SecItem.sharedAccessGroupPrefix, "9XUJ7M53NG")
             #endif
         #elseif os(macOS)
-            // Do nothing.
+            guard testEnvironmentIsSigned() else {
+                return
+            }
+            XCTAssertEqual(SecItem.sharedAccessGroupPrefix, "9XUJ7M53NG")
         #else
             // Currently unsupported build configuration. This next line will compile-time error.
             doNotCommentOutThisLine()

diff --git a/Tests/ValetIntegrationTests/ValetIntegrationTests.swift b/Tests/ValetIntegrationTests/ValetIntegrationTests.swift
@@ -284,6 +284,29 @@ class ValetIntegrationTests: XCTestCase
 
         XCTAssertNil(anotherFlavor.string(forKey: "cookie"))
     }
+
+    #if !os(macOS)
+    func test_objectForKey_canReadItemsWithout_kSecUseDataProtectionKeychain_when_kSecUseDataProtectionKeychain_isSetToTrueInKeychainQuery() {
+        let valet = Valet.valet(with: Identifier(nonEmpty: "DataProtectionTest")!, accessibility: .afterFirstUnlock)
+        var dataProtectionWriteQuery = valet.keychainQuery
+        #if swift(>=5.1)
+        dataProtectionWriteQuery[kSecUseDataProtectionKeychain as String] = nil
+        #else
+        dataProtectionWriteQuery["nleg"] = nil // kSecUseDataProtectionKeychain for Xcode 9 and Xcode 10 compatibility.
+        #endif
+
+        let key = "DataProtectionKey"
+        let object = Data("DataProtectionValue".utf8)
+        dataProtectionWriteQuery[kSecAttrAccount as String] = key
+        dataProtectionWriteQuery[kSecValueData as String] = object
+
+        // Make sure the item is not in the keychain before we start this test
+        SecItemDelete(dataProtectionWriteQuery as CFDictionary)
+
+        XCTAssertEqual(SecItemAdd(dataProtectionWriteQuery as CFDictionary, nil), errSecSuccess)
+        XCTAssertEqual(valet.object(forKey: key), object) // If this breaks, it means Apple has changed behavior of SecItemCopy. It means that we need to remove `kSecUseDataProtectionKeychain` from our query on non-Mac platforms.
+    }
+    #endif
 
     // MARK: set(string:forKey:)
 

diff --git a/Valet.podspec b/Valet.podspec
@@ -1,6 +1,6 @@
 Pod::Spec.new do |s|
   s.name     = 'Valet'
-  s.version  = '3.2.6'
+  s.version  = '3.2.8'
   s.license  = 'Apache License, Version 2.0'
   s.summary  = 'Securely store data on iOS, tvOS, watchOS, or macOS without knowing a thing about how the Keychain works. It\'s easy. We promise.'
   s.homepage = 'https://github.com/square/Valet'