Fix keychains that rely on kSecAttrAccessGroup on macOS Catalina #193

dfed · 2019-12-22T21:52:31Z

It looks like Catalina broke our logic for finding a sharedAccessGroupPrefix. There's new documentation for kSecAttrAccessGroup in macOS Catalina:

This PR adds kSecUseDataProtectionKeychain to all keychain queries on macOS. This follows Apple's recommendation:

NickEntin

Is this going to affect accessing values set using older versions of Valet?

Use it unless you specifically need access to items previously stored in a legacy keychain in macOS.

Not sure what a "legacy" keychain is in this context. If it includes anything before this attribute was set, that might be problematic.

Sources/Valet/Internal/SecItem.swift

…ying a value

dfed · 2019-12-22T22:52:45Z

Is this going to affect accessing values set using older versions of Valet

Good catch! I've added a commit that adds this field when setting a value, but doesn't require it when reading values out of the keychain.

Note that I've run all macOS tests locally in a signed environment, and they are passing with this latest update. These tests include our tests that Legacy Valet instances can write values that current Valet instances can read. Given that those are passing, I think we're good to go.

Sources/Valet/Internal/Keychain.swift

dfed · 2020-01-04T16:34:22Z

Would love to get this fix into a release so that Mac apps can recover from this issue. Can I get another review?

NickEntin · 2020-01-04T23:13:59Z

Based on testing, it appears that SecItemAdd is the only place where the kSecUseDataProtectionKeychain property validation is currently occurring. There is some potential for the future appearance of data loss here - specifically if Apple begins to enforce that kSecUseDataProtectionKeychain be included when utilizing SecItemDelete and SecItemCopyMatching as described in the documentation. In this case, items could be written to the Valet, but not read or deleted from it. The fix for this is to specify kSecUseDataProtectionKeychain when reading and deleting.

The trade-off here is that if we kSecUseDataProtectionKeychain on read, we will cause data loss when data is written to a Valet on macOS 10.14 or earlier and the system is subsequently updated to macOS 10.15. Leaving kSecUseDataProtectionKeychain out on read means that we could potentially read in data from a Valet with the same identifier but different accessibility level or access group, since kSecAttrAccessible and kSecAttrAccessGroup only apply when kSecUseDataProtectionKeychain is true.

…lity

…n-Mac targets

dfed · 2020-01-05T00:14:22Z

I've updated this PR to always use kSecUseDataProtectionKeychain, and I've added func migrateObjectsFromPreCatalina() to Valet. I need to add a README blurb on how to use that method still.

dfed · 2020-01-06T03:08:07Z

Leaving kSecUseDataProtectionKeychain out on read means that we could potentially read in data from a Valet with the same identifier but different accessibility level or access group, since kSecAttrAccessible and kSecAttrAccessGroup only apply when kSecUseDataProtectionKeychain is true.

Upon further investigation, this isn't entirely true. We encode the accessibility in the kSecAttrService, so we can't read data from another Valet with the same identifier but a different accessibility level.

Additionally, since kSecAttrAccessGroup is derived from the identifier, it is impossible to have a Valet with the same identifier but different kSecAttrAccessGroup value.

That said, I still think protecting against a future where Apple enforces the presence of kSecUseDataProtectionKeychain on read or delete.

… migration of objects from other Valets

…prevent data loss on iOS, tvOS, and watchOS

dfed · 2020-01-06T03:52:48Z

Sources/Valet/Internal/SecItem.swift

+            kSecAttrAccessible : Accessibility.alwaysThisDeviceOnly.secAccessibilityAttribute,
+        ]
+
+        if #available(iOS 13.0, tvOS 13.0, watchOS 6.0, macOS 10.15, *) {


There's no harm in using kSecUseDataProtectionKeychain when looking up the sharedAccessGroupPrefix on all operating systems.

dfed · 2020-01-06T03:53:59Z

Sources/Valet/Internal/SecItem.swift

+
+        if #available(iOS 13.0, tvOS 13.0, watchOS 6.0, macOS 10.15, *) {
+            // Update the account to be distinct, since we're changing the query.
+            query[kSecAttrAccount] = "SharedAccessGroupAlwaysAccessiblePrefixPlaceholderWithDataProtection"


If we don't do this, a SharedAccessGroupAlwaysAccessiblePrefixPlaceholder that was added without kSecUseDataProtectionKeychain wouldn't be found on SecItemCopy, but trying to add that account/data to the keychain would return a errSecDuplicateItem.

Nevermind. Looks like errSecDuplicateItem is only returned when a kSecAttrService or kSecAttrAccount overlaps. Removing this line.

dfed · 2020-01-06T03:56:37Z

Sources/Valet/Internal/Service.swift

-
+
+        #if os(macOS)
+        if #available(macOS 10.15, *) {


I only add kSecUseDataProtectionKeychain to our base query on macOS, since macOS is the only OS that needs this value.

Adding this value to the base query is a breaking change, since ~~anything~~ some items written to the keychain without kSecUseDataProtectionKeychain set will not be read when kSecUseDataProtectionKeychain is in the base query. I wrote migrateObjectsFromPreCatalina to solve this issue on macOS, but decided to allow non-Mac customers to continue using the keychain without migrating objects to include kSecUseDataProtectionKeychain.

After sleeping on it: I realize we can use kSecUseDataProtectionKeychain on all platforms. I had written a test to prove this.

From Apple's documentation:

It’s highly recommended that you set the value of this key to true for all keychain operations. This key helps to improve the portability of your code across platforms. Use it unless you specifically need access to items previously stored in a legacy keychain in macOS.

Fixed in 0c9799c

dfed · 2020-01-06T03:57:23Z

@NickEntin this PR is now ready for review again.

…atalina unless test environment is signed

…ion requirement

…l only meaningfully update objects written from macOS

…dance

Tests/ValetIntegrationTests/ValetIntegrationTests.swift

dfed added 3 commits December 22, 2019 13:47

Fix keychains that rely on kSecAttrAccessGroup on macOS Catalina

f11cff9

Bump version

fa4e65c

whitespace

a6f4053

NickEntin reviewed Dec 22, 2019

View reviewed changes

Sources/Valet/Internal/SecItem.swift Outdated Show resolved Hide resolved

Sources/Valet/Internal/SecItem.swift Show resolved Hide resolved

Add kSecUseDataProtectionKeychain when setting a value, not when quer…

cebed52

…ying a value

dfed and others added 3 commits December 22, 2019 16:00

Get CI passing

f8b7bfe

Merge branch 'master' into dfed--fix-shared-access-group-catalina

40bae79

Support Xcode 9 and 10

e1baeba

dfed force-pushed the dfed--fix-shared-access-group-catalina branch from 8e35ea5 to e1baeba Compare December 23, 2019 06:46

NickEntin reviewed Dec 23, 2019

View reviewed changes

Sources/Valet/Internal/Keychain.swift Outdated Show resolved Hide resolved

dfed and others added 2 commits December 23, 2019 12:33

Merge branch 'master' into dfed--fix-shared-access-group-catalina

25902ae

Use kSecUseDataProtectionKeychain constant where available

63c3397

dfed force-pushed the dfed--fix-shared-access-group-catalina branch from 2ccc52a to 63c3397 Compare December 29, 2019 21:11

dfed requested review from NickEntin, EricMuller22 and gfontenot January 4, 2020 16:34

dfed added 2 commits January 4, 2020 15:52

Use kSecUseDataProtectionKeychain on read as well. Add upgrade capabi…

83d1ef8

…lity

Add test to prove that kSecUseDataProtectionKeychain hasn't broken no…

263a0d3

…n-Mac targets

Merge branch 'master' into dfed--fix-shared-access-group-catalina

d432b5a

dfed added 5 commits January 5, 2020 19:36

Update README

bf050cc

Remove incorrect warning. The specificity of kSecAttrService prevents…

8d9fcbe

… migration of objects from other Valets

Better comment

4d33ebc

Add kSecUseDataProtectionKeychain to the base query only on macOS to …

069e195

…prevent data loss on iOS, tvOS, and watchOS

Update the account when retrieving the shared access group prefix

5e93066

dfed commented Jan 6, 2020

View reviewed changes

dfed added 5 commits January 5, 2020 20:25

Do not run test_migrateObjectsFromPreCatalina_migratesDataWrittenPreC…

49374b6

…atalina unless test environment is signed

Migrate objects from pre-catalina in tests

e7f0737

Update README to reflect which Valet types are exempt from the migrat…

b9cde67

…ion requirement

Do not use removeOnCompletion parameter, which is misleading

e49b779

Update code comment to reflect that migrateObjectsFromPreCatalina wil…

34cfbe3

…l only meaningfully update objects written from macOS

dfed force-pushed the dfed--fix-shared-access-group-catalina branch from 471507f to f4de2d4 Compare January 6, 2020 05:30

README typo

f60f87d

dfed force-pushed the dfed--fix-shared-access-group-catalina branch from f4de2d4 to f60f87d Compare January 6, 2020 05:30

dfed added 3 commits January 5, 2020 21:50

Test sharedAccessGroupPrefix on macOS in a signed environment

0bb571c

Do not unnecessarily update kSecAttrAccount

564a298

Bring kSecUseDataProtectionKeychain to all platforms, per Apple's gui…

0c9799c

…dance

dfed requested a review from fdiaz January 13, 2020 19:57

NickEntin approved these changes Jan 14, 2020

View reviewed changes

Tests/ValetIntegrationTests/ValetIntegrationTests.swift Outdated Show resolved Hide resolved

Fix tests on Xcode 11

ad3ab18

dfed merged commit 7447d16 into master Jan 14, 2020

dfed deleted the dfed--fix-shared-access-group-catalina branch January 14, 2020 23:13

dfed mentioned this pull request Jan 25, 2020

Migrating Objects from Pre Catalina unexpected results #213

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix keychains that rely on kSecAttrAccessGroup on macOS Catalina #193

Fix keychains that rely on kSecAttrAccessGroup on macOS Catalina #193

dfed commented Dec 22, 2019

NickEntin left a comment

dfed commented Dec 22, 2019 •

edited

Loading

dfed commented Jan 4, 2020

NickEntin commented Jan 4, 2020

dfed commented Jan 5, 2020

dfed commented Jan 6, 2020 •

edited

Loading

dfed Jan 6, 2020

dfed Jan 6, 2020

dfed Jan 6, 2020

dfed Jan 6, 2020 •

edited

Loading

dfed Jan 6, 2020

dfed Jan 6, 2020

dfed commented Jan 6, 2020

Fix keychains that rely on kSecAttrAccessGroup on macOS Catalina #193

Fix keychains that rely on kSecAttrAccessGroup on macOS Catalina #193

Conversation

dfed commented Dec 22, 2019

NickEntin left a comment

Choose a reason for hiding this comment

dfed commented Dec 22, 2019 • edited Loading

dfed commented Jan 4, 2020

NickEntin commented Jan 4, 2020

dfed commented Jan 5, 2020

dfed commented Jan 6, 2020 • edited Loading

dfed Jan 6, 2020

Choose a reason for hiding this comment

dfed Jan 6, 2020

Choose a reason for hiding this comment

dfed Jan 6, 2020

Choose a reason for hiding this comment

dfed Jan 6, 2020 • edited Loading

Choose a reason for hiding this comment

dfed Jan 6, 2020

Choose a reason for hiding this comment

dfed Jan 6, 2020

Choose a reason for hiding this comment

dfed commented Jan 6, 2020

dfed commented Dec 22, 2019 •

edited

Loading

dfed commented Jan 6, 2020 •

edited

Loading

dfed Jan 6, 2020 •

edited

Loading