Skip to content

Commit 2b403a6

Browse files
maksymmalyhinmaksymmalyhin
authored
FIRInstanceIDKeyPairStore crash fix (#2953)
* FIRInstanceIDKeyPairStore fix SecKeyRef memory management issue * [WIP] FIRInstanceIDKeyPairStore - perform operations in the order they were scheduled * Revert "[WIP] FIRInstanceIDKeyPairStore - perform operations in the order they were scheduled" This reverts commit 7ff7b51. * FIRInstanceIDKeyPairStore - schedule updating public and private keys at once (not from a callback) to avoid modifications of keychain in between. Update `self.keyPair` before updating keychain for all cases. * Chengelog * Don't use dispatch_group, rely on `FIRInstanceIDKeychain` serialized implementation instead
1 parent b8f1631 commit 2b403a6

File tree

3 files changed

+111
-61
lines changed

3 files changed

+111
-61
lines changed

‎Example/InstanceID/Tests/FIRInstanceIDKeyPairMigrationTest.m

Lines changed: 50 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -30,7 +30,7 @@
3030
@interface FIRInstanceIDKeyPairStore (ExposedForTest)
3131

3232
@property(nonatomic, readwrite, strong) FIRInstanceIDBackupExcludedPlist *plist;
33-
@property(nonatomic, readwrite, strong) FIRInstanceIDKeyPair *keyPair;
33+
@property(atomic, readwrite, strong) FIRInstanceIDKeyPair *keyPair;
3434
BOOL FIRInstanceIDHasMigratedKeyPair(NSString *legacyPublicKeyTag, NSString *newPublicKeyTag);
3535
NSString *FIRInstanceIDLegacyPublicTagWithSubtype(NSString *subtype);
3636
NSString *FIRInstanceIDLegacyPrivateTagWithSubtype(NSString *subtype);
@@ -44,6 +44,10 @@ + (void)deleteKeyPairWithPrivateTag:(NSString *)privateTag
4444
handler:(void (^)(NSError *))handler;
4545
- (void)migrateKeyPairCacheIfNeededWithHandler:(void (^)(NSError *error))handler;
4646
+ (NSString *)keyStoreFileName;
47+
48+
- (void)updateKeyRef:(SecKeyRef)keyRef
49+
withTag:(NSString *)tag
50+
handler:(void (^)(NSError *error))handler;
4751
@end
4852

4953
// Need to separate the tests from FIRInstanceIDKeyPairStoreTest for separate keychain operations
@@ -68,7 +72,7 @@ - (void)tearDown {
6872
[self.keyPairStore removeKeyPairCreationTimePlistWithError:&error];
6973
}
7074

71-
- (void)testMigrationDataIfLegtacyKeyPairsNotExist {
75+
- (void)testMigrationDataIfLegacyKeyPairsNotExist {
7276
NSString *legacyPublicKeyTag =
7377
FIRInstanceIDLegacyPublicTagWithSubtype(kFIRInstanceIDKeyPairSubType);
7478

@@ -132,6 +136,49 @@ - (void)testMigrationIfLegacyKeyPairsExist {
132136
}];
133137
}];
134138

135-
[self waitForExpectationsWithTimeout:1000 handler:nil];
139+
[self waitForExpectationsWithTimeout:1 handler:nil];
136140
}
141+
142+
- (void)testUpdateKeyRefWithTagRetainsAndReleasesKeyRef {
143+
SecKeyRef publicKeyRef;
144+
145+
@autoreleasepool {
146+
NSString *legacyPublicKeyTag =
147+
FIRInstanceIDLegacyPublicTagWithSubtype(kFIRInstanceIDKeyPairSubType);
148+
NSString *legacyPrivateKeyTag =
149+
FIRInstanceIDLegacyPrivateTagWithSubtype(kFIRInstanceIDKeyPairSubType);
150+
FIRInstanceIDKeyPair *keyPair =
151+
[[FIRInstanceIDKeychain sharedInstance] generateKeyPairWithPrivateTag:legacyPrivateKeyTag
152+
publicTag:legacyPublicKeyTag];
153+
XCTAssertTrue([keyPair isValid]);
154+
155+
publicKeyRef = keyPair.publicKey;
156+
157+
// Retain to keep publicKeyRef alive to verify its reatin count
158+
CFRetain(publicKeyRef);
159+
160+
// 2 = 1 from keyPair + 1 from CFRetain()
161+
XCTAssertEqual(CFGetRetainCount(publicKeyRef), 2);
162+
163+
XCTestExpectation *completionExpectaion =
164+
[self expectationWithDescription:@"completionExpectaion"];
165+
[self.keyPairStore updateKeyRef:keyPair.publicKey
166+
withTag:@"test"
167+
handler:^(NSError *error) {
168+
[completionExpectaion fulfill];
169+
}];
170+
171+
// 3 = from keyPair + 1 from CFRetain() + 1 retained by `updateKeyRef`
172+
XCTAssertEqual(CFGetRetainCount(publicKeyRef), 3);
173+
}
174+
175+
// 2 = 1 from CFRetain() + 1 retained by `updateKeyRef`
176+
XCTAssertEqual(CFGetRetainCount(publicKeyRef), 2);
177+
178+
[self waitForExpectationsWithTimeout:0.5 handler:NULL];
179+
180+
// No one else owns publicKeyRef except the test
181+
XCTAssertEqual(CFGetRetainCount(publicKeyRef), 1);
182+
}
183+
137184
@end

‎Firebase/InstanceID/CHANGELOG.md

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,3 +1,6 @@
1+
# Unreleased
2+
- A keychain migration crash fix (#2731)
3+
14
# 2019-05-07 -- 4.0.0
25
- Remove deprecated `token` method. Use `instanceIDWithHandler:` instead. (#2741)
36
- Send `firebaseUserAgent` with a register request (#2679)

‎Firebase/InstanceID/FIRInstanceIDKeyPairStore.m

Lines changed: 58 additions & 58 deletions
Original file line numberDiff line numberDiff line change
@@ -119,7 +119,7 @@ BOOL FIRInstanceIDHasMigratedKeyPair(NSString *legacyPublicKeyTag, NSString *new
119119
@interface FIRInstanceIDKeyPairStore ()
120120

121121
@property(nonatomic, readwrite, strong) FIRInstanceIDBackupExcludedPlist *plist;
122-
@property(nonatomic, readwrite, strong) FIRInstanceIDKeyPair *keyPair;
122+
@property(atomic, readwrite, strong) FIRInstanceIDKeyPair *keyPair;
123123
@property(nonatomic, readwrite, assign) NSInteger keychainEntitlementsErrorCount;
124124

125125
@end
@@ -365,30 +365,31 @@ - (void)migrateKeyPairCacheIfNeededWithHandler:(void (^)(NSError *error))handler
365365
self.keyPair = keyPair;
366366

367367
// Either new key pair doesn't exist or it's different than legacy key pair, start the migration.
368+
__block NSError *updateKeyRefError;
369+
368370
NSString *privateKeyTag = FIRInstanceIDPrivateTagWithSubtype(kFIRInstanceIDKeyPairSubType);
369371
[self updateKeyRef:keyPair.publicKey
370372
withTag:publicKeyTag
371373
handler:^(NSError *error) {
372374
if (error) {
373375
FIRInstanceIDLoggerError(kFIRInstanceIDMessageCodeKeyPairMigrationError,
374376
@"Unable to migrate key pair from legacy ones.");
377+
updateKeyRefError = error;
378+
}
379+
}];
380+
381+
[self updateKeyRef:keyPair.privateKey
382+
withTag:privateKeyTag
383+
handler:^(NSError *error) {
384+
if (error) {
385+
FIRInstanceIDLoggerError(kFIRInstanceIDMessageCodeKeyPairMigrationError,
386+
@"Unable to migrate key pair from legacy ones.");
387+
updateKeyRefError = error;
388+
}
389+
390+
if (handler) {
391+
handler(updateKeyRefError);
375392
}
376-
[self updateKeyRef:keyPair.privateKey
377-
withTag:privateKeyTag
378-
handler:^(NSError *error) {
379-
if (error) {
380-
FIRInstanceIDLoggerError(
381-
kFIRInstanceIDMessageCodeKeyPairMigrationError,
382-
@"Unable to migrate key pair from legacy ones.");
383-
return;
384-
}
385-
FIRInstanceIDLoggerDebug(
386-
kFIRInstanceIDMessageCodeKeyPairMigrationSuccess,
387-
@"Successfully migrated the key pair from legacy ones.");
388-
if (handler) {
389-
handler(error);
390-
}
391-
}];
392393
}];
393394
}
394395

@@ -400,37 +401,38 @@ - (void)updateKeyRef:(SecKeyRef)keyRef
400401
handler:(void (^)(NSError *error))handler {
401402
NSData *updatedTagData = [tag dataUsingEncoding:NSUTF8StringEncoding];
402403

404+
__block NSError *keychainError;
405+
403406
// Always delete the old keychain before adding a new one to avoid conflicts.
404407
NSDictionary *deleteQuery = @{
405408
(__bridge id)kSecAttrApplicationTag : updatedTagData,
406409
(__bridge id)kSecClass : (__bridge id)kSecClassKey,
407410
(__bridge id)kSecAttrKeyType : (__bridge id)kSecAttrKeyTypeRSA,
408411
(__bridge id)kSecReturnRef : @(YES),
409412
};
413+
[[FIRInstanceIDKeychain sharedInstance] removeItemWithQuery:deleteQuery
414+
handler:^(NSError *error) {
415+
if (error) {
416+
keychainError = error;
417+
}
418+
}];
410419

411-
[[FIRInstanceIDKeychain sharedInstance]
412-
removeItemWithQuery:deleteQuery
413-
handler:^(NSError *error) {
414-
if (error) {
415-
if (handler) {
416-
handler(error);
417-
}
418-
return;
419-
}
420-
NSDictionary *addQuery = @{
421-
(__bridge id)kSecAttrApplicationTag : updatedTagData,
422-
(__bridge id)kSecClass : (__bridge id)kSecClassKey,
423-
(__bridge id)kSecValueRef : (__bridge id)keyRef,
424-
(__bridge id)
425-
kSecAttrAccessible : (__bridge id)kSecAttrAccessibleAlwaysThisDeviceOnly,
426-
};
427-
[[FIRInstanceIDKeychain sharedInstance] addItemWithQuery:addQuery
428-
handler:^(NSError *addError) {
429-
if (handler) {
430-
handler(addError);
431-
}
432-
}];
433-
}];
420+
NSDictionary *addQuery = @{
421+
(__bridge id)kSecAttrApplicationTag : updatedTagData,
422+
(__bridge id)kSecClass : (__bridge id)kSecClassKey,
423+
(__bridge id)kSecValueRef : (__bridge id)keyRef,
424+
(__bridge id)kSecAttrAccessible : (__bridge id)kSecAttrAccessibleAlwaysThisDeviceOnly,
425+
};
426+
[[FIRInstanceIDKeychain sharedInstance] addItemWithQuery:addQuery
427+
handler:^(NSError *addError) {
428+
if (addError) {
429+
keychainError = addError;
430+
}
431+
432+
if (handler) {
433+
handler(keychainError);
434+
}
435+
}];
434436
}
435437

436438
- (void)deleteSavedKeyPairWithSubtype:(NSString *)subtype
@@ -453,6 +455,8 @@ - (void)deleteSavedKeyPairWithSubtype:(NSString *)subtype
453455
}
454456
}
455457

458+
self.keyPair = nil;
459+
456460
[FIRInstanceIDKeyPairStore
457461
deleteKeyPairWithPrivateTag:privateKeyTag
458462
publicTag:publicKeyTag
@@ -475,7 +479,6 @@ - (void)deleteSavedKeyPairWithSubtype:(NSString *)subtype
475479
handler(error);
476480
}
477481
} else {
478-
self.keyPair = nil;
479482
if (handler) {
480483
handler(nil);
481484
}
@@ -489,28 +492,25 @@ + (void)deleteKeyPairWithPrivateTag:(NSString *)privateTag
489492
NSDictionary *queryPublicKey = FIRInstanceIDKeyPairQuery(publicTag, NO, NO);
490493
NSDictionary *queryPrivateKey = FIRInstanceIDKeyPairQuery(privateTag, NO, NO);
491494

495+
__block NSError *keychainError;
496+
492497
// Always remove public key first because it is the key we generate IID.
493498
[[FIRInstanceIDKeychain sharedInstance] removeItemWithQuery:queryPublicKey
494499
handler:^(NSError *error) {
495500
if (error) {
496-
if (handler) {
497-
handler(error);
498-
}
499-
return;
501+
keychainError = error;
502+
}
503+
}];
504+
505+
[[FIRInstanceIDKeychain sharedInstance] removeItemWithQuery:queryPrivateKey
506+
handler:^(NSError *error) {
507+
if (error) {
508+
keychainError = error;
509+
}
510+
511+
if (handler) {
512+
handler(keychainError);
500513
}
501-
[[FIRInstanceIDKeychain sharedInstance]
502-
removeItemWithQuery:queryPrivateKey
503-
handler:^(NSError *error) {
504-
if (error) {
505-
if (handler) {
506-
handler(error);
507-
}
508-
return;
509-
}
510-
if (handler) {
511-
handler(nil);
512-
}
513-
}];
514514
}];
515515
}
516516

0 commit comments

Comments
 (0)