Perform class check on kSecAttrAccount keychain value during migration #114

kgleong · 2017-08-12T01:06:12Z

Valet error when kSecAttAccount is not a String during keychain migration #113
(migrateObjectsMatchingQuery: removeOnCompletion:) assumes that data in kSecAttrAccount is a NSString.
This can cause a crash if the value in kSecAttrAccount is an NSData object, for example.

kgleong · 2017-08-12T01:07:52Z

Valet.xcodeproj/project.pbxproj

@@ -974,7 +974,7 @@
 				GCC_WARN_UNINITIALIZED_AUTOS = YES_AGGRESSIVE;
 				GCC_WARN_UNUSED_FUNCTION = YES;
 				GCC_WARN_UNUSED_VARIABLE = YES;
-				IPHONEOS_DEPLOYMENT_TARGET = 6.0;
+				IPHONEOS_DEPLOYMENT_TARGET = 9.0;


Xcode wouldn't build unless this was > 8.0

This would be a breaking change, requiring a major version bump. Let's revert this part of the change since it'll affect people using us in submodules?

dfed

We need to revert the breaking change and bump a bugfix version in the podspec before this can be merged. Good fix & test though! Appreciate the change :)

dfed · 2017-08-12T01:10:21Z

Valet.xcodeproj/project.pbxproj

@@ -974,7 +974,7 @@
 				GCC_WARN_UNINITIALIZED_AUTOS = YES_AGGRESSIVE;
 				GCC_WARN_UNUSED_FUNCTION = YES;
 				GCC_WARN_UNUSED_VARIABLE = YES;
-				IPHONEOS_DEPLOYMENT_TARGET = 6.0;
+				IPHONEOS_DEPLOYMENT_TARGET = 9.0;


This would be a breaking change, requiring a major version bump. Let's revert this part of the change since it'll affect people using us in submodules?

kgleong · 2017-08-12T02:42:12Z

@dfed that makes sense. I've removed the deployment target change

dfed · 2017-08-12T03:28:05Z

Looks like CI is failing

/Users/travis/build/square/Valet/ValetTests/ValetTests.m:684: error: -[KeychainTests test_migrateObjectsMatchingQueryRemoveOnCompletion_withExistingAccountNameNSDataKeychainEntry_doesNotRaiseException] : ((error) == nil) failed: "Error Domain=VALMigrationErrorDomain Code=4 "(null)""

Possible that CI is running against a different SDK that didn't have this issue?

kgleong · 2017-08-12T10:29:49Z

ah, looks like the mac test failed. apparently adding an NSData entry for kSecAttrAccount is discarded by OSX but not iOS. Seems like an Apple inconsistency that they enforce it on OSX but not iOS, though perhaps it's due to backwards compat issues.

when inspecting the keychain during migration in OSX:

(lldb) po keychainEntry
{
    cdat = "2017-08-12 11:37:39 +0000";
    class = genp;
    labl = "Keychain_With_Account_Name_As_NSData";
    mdat = "2017-08-12 11:37:39 +0000";
    svce = "Keychain_With_Account_Name_As_NSData";
    "v_Data" = <666f6f>;
    "v_PersistentRef" = <...>;
}

in iOS:

(lldb) po keychainEntry
{
    acct = <666f6f>;
    agrp = "9XUJ7M53NG.com.squareup.Valet-iOS-Test-Host-App";
    cdat = "2017-08-12 11:35:45 +0000";
    mdat = "2017-08-12 11:35:45 +0000";
    musr = <>;
    pdmn = ak;
    svce = "Keychain_With_Account_Name_As_NSData";
    sync = 0;
    tomb = 0;
    "v_Data" = <666f6f>;
    "v_PersistentRef" = <67656e70 00000000 00000325>;
}

kgleong · 2017-08-12T11:57:52Z

ValetTests/ValetTests.m

    for (VALValet *const additionalValet in self.additionalValets) {
        [additionalValet removeAllObjects];
    }
+
+    for (NSDictionary *keychainEntry in self.manualKeychainEntries) {


seems much simpler to do this than some of the other workarounds to reset the keychain:

https://medium.com/@ryuichi/setup-host-app-for-keychain-tests-in-xcode-8-97222611917e

https://stackoverflow.com/questions/34907873

Seems like a reasonable solution! Since this cleanup is needed for only one test may be best to keep the cleanup localized to that test. I'm a big fan of putting the cleanup at the top of the test, so if tearDown is aborted the next run of the test still succeeds.

ah, what about a finally block? I didn't know this existed and it does the same thing i wanted to get out of tearDown(): something that is ensured to run regardless of whether an exception is thrown and execution stops.

NSDictionary *keychainEntry = <..> @try { .. all the code } @finally { SecItemDelete(keychainEntry); }

I'm throwing straw men out there now (but I have been bit by these in the past): finally won't prevent against

Hitting cmd+. in the middle of the test

Another test polluting the keychain

Normally I don't protect against these kinds of issues in my tests. But since keychain is both persistent and a black box I like to control for as many factors as I can in each test.

ah gotcha. thanks for all the tips and the open source search terms

dfed · 2017-08-12T17:06:26Z

Really good find re Mac/iOS not having the same implementation here. Not surprising at all – there are lovely subtle differences between the two implementations... keychain on iOS and Mac have entirely different implementations under then hood (the code is open source). Thank you again for the fix! Last thing before we get an approval is making the test more resilient to a rerun after failing halfway through the prior run. Almost there!

(Keychain is always more complex than it looks. Always. 😅)

kgleong · 2017-08-13T00:30:45Z

where does one find the open source code? i remember @EricMuller22 entering some magical google search term...

dfed · 2017-08-13T00:45:00Z

I tend to google "Apple open source secitem" 🙂

Here's some of it: https://opensource.apple.com/source/Security/Security-55471/sec/Security/SecItem.c

…ncheck. * #113 * `(migrateObjectsMatchingQuery: removeOnCompletion:)` assumes that data in `kSecAttrAccount` is a NSString. * This can cause a crash if the value in kSecAttrAccount is an NSData object, for example.

kgleong · 2017-08-13T11:36:24Z

@dfed I placed the SecItemDelete back in its previous place (as early as possible in the test)

dfed · 2017-08-13T15:24:28Z

@EricMuller22 want to merge/release? I'll get to it tomorrow if not.

EricMuller22 · 2017-08-14T17:13:36Z

LGTM, I'll let you handle merge/release though @dfed, going to be in meetings all day 😞

kgleong · 2017-12-02T17:17:59Z

Posted this SO question https://stackoverflow.com/questions/47602876 to see if anyone knows why OSX and iOS handle NSData objects in kSecAttrAccount differently. fyi @EricMuller22 @dfed

kgleong commented Aug 12, 2017

View reviewed changes

dfed requested changes Aug 12, 2017

View reviewed changes

kgleong commented Aug 12, 2017

View reviewed changes

dfed approved these changes Aug 13, 2017

View reviewed changes

dfed merged commit abdb53e into square:master Aug 14, 2017

dfed mentioned this pull request Sep 16, 2017

Valet error when kSecAttAccount is not a String during keychain migration #113

Closed

kgleong deleted the kgleong/class-check branch December 2, 2017 17:18

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Perform class check on kSecAttrAccount keychain value during migration #114

Perform class check on kSecAttrAccount keychain value during migration #114

kgleong commented Aug 12, 2017

kgleong Aug 12, 2017

dfed Aug 12, 2017

dfed left a comment

dfed Aug 12, 2017

kgleong commented Aug 12, 2017

dfed commented Aug 12, 2017

kgleong commented Aug 12, 2017 •

edited

kgleong Aug 12, 2017

dfed Aug 12, 2017

kgleong Aug 13, 2017

dfed Aug 13, 2017

kgleong Aug 13, 2017

dfed commented Aug 12, 2017

kgleong commented Aug 13, 2017

dfed commented Aug 13, 2017

kgleong commented Aug 13, 2017

dfed commented Aug 13, 2017

EricMuller22 commented Aug 14, 2017

kgleong commented Dec 2, 2017

Perform class check on kSecAttrAccount keychain value during migration #114

Perform class check on kSecAttrAccount keychain value during migration #114

Conversation

kgleong commented Aug 12, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dfed left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kgleong commented Aug 12, 2017

dfed commented Aug 12, 2017

kgleong commented Aug 12, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dfed commented Aug 12, 2017

kgleong commented Aug 13, 2017

dfed commented Aug 13, 2017

kgleong commented Aug 13, 2017

dfed commented Aug 13, 2017

EricMuller22 commented Aug 14, 2017

kgleong commented Dec 2, 2017

kgleong commented Aug 12, 2017 •

edited