Fix a rare crash in TSKSPKIHashCache

TrustKit/Pinning/TSKSPKIHashCache.m

@@ -148,8 +148,22 @@ - (SPKICacheDictionnary *)loadSPKICacheFromFileSystem;
 
 @implementation TSKSPKIHashCache
 
+/// Blocks to call out to the main thread if necessary
 static BOOL isProtectedDataAvailable(void)
 {
+    if (NSThread.isMainThread) {
+        return _isProtectedDataAvailable();
+    } else {
+        __block BOOL ret = NO;
+        dispatch_sync(dispatch_get_main_queue(), ^{
+            ret = _isProtectedDataAvailable();
+        });
+
+        return ret;
+    }
+}
+
+static BOOL _isProtectedDataAvailable(void) {
     Class uiApplicationClass = objc_getClass("UIApplication");
     if (uiApplicationClass) {
         SEL sharedApplicationSelector = sel_registerName("sharedApplication");
@@ -168,7 +182,7 @@ - (instancetype)initWithIdentifier:(NSString *)uniqueIdentifier
     self = [super init];
     if (self) {
         // Initialize our locks
-        _lockQueue = dispatch_queue_create("TSKSPKIHashLock", DISPATCH_QUEUE_CONCURRENT);
+        _lockQueue = dispatch_queue_create("TSKSPKIHashLock", DISPATCH_QUEUE_SERIAL);
 
         // Ensure a non-nil identifier was provided
         NSAssert(uniqueIdentifier, @"TSKSPKIHashCache initializer must be passed a unique identifier");
@@ -185,16 +199,21 @@ - (instancetype)initWithIdentifier:(NSString *)uniqueIdentifier
     return self;
 }
 
-- (NSData *)hashSubjectPublicKeyInfoFromCertificate:(SecCertificateRef)certificate
-{
-    __block NSData *cachedSubjectPublicKeyInfo;
+- (NSData *)hashSubjectPublicKeyInfoFromCertificate:(SecCertificateRef)certificate {
+    __block NSData *hash = nil;
 
+    dispatch_sync(self.lockQueue, ^{
+        hash = [self _hashSubjectPublicKeyInfoFromCertificate:certificate];
+    });
+
+    return hash;
+}
+
+- (NSData *)_hashSubjectPublicKeyInfoFromCertificate:(SecCertificateRef)certificate
+{
     // Have we seen this certificate before? Look for the SPKI in the cache
     NSData *certificateData = (__bridge_transfer NSData *)(SecCertificateCopyData(certificate));
-
-    dispatch_sync(_lockQueue, ^{
-        cachedSubjectPublicKeyInfo = self->_spkiCache[certificateData];
-    });
+    NSData *cachedSubjectPublicKeyInfo = self->_spkiCache[certificateData];
 
     if (cachedSubjectPublicKeyInfo)
     {
@@ -255,35 +274,22 @@ - (NSData *)hashSubjectPublicKeyInfoFromCertificate:(SecCertificateRef)certifica
 
 
     // Store the hash in our memory cache
-    dispatch_barrier_sync(_lockQueue, ^{
-        self->_spkiCache[certificateData] = subjectPublicKeyInfoHash;
-    });
+    self->_spkiCache[certificateData] = subjectPublicKeyInfoHash;
 
     // Update the cache on the filesystem
-    if (self.spkiCacheFilename.length > 0) {
-
-        __weak typeof(self) weakSelf = self;
-        void (^updateCacheBlock)(void) = ^{
-
-            if (isProtectedDataAvailable()) {
-                NSData *serializedSpkiCache = [NSKeyedArchiver archivedDataWithRootObject:weakSelf.spkiCache requiringSecureCoding:YES error:nil];
-                if ([serializedSpkiCache writeToURL:[weakSelf SPKICachePath] atomically:YES] == NO) {
-                    NSAssert(false, @"Failed to write cache");
-                    TSKLog(@"Could not persist SPKI cache to the filesystem");
-                }
-            }
-            else {
-                TSKLog(@"Protected data not available, skipping SPKI cache persistence");
-            }
-        };
-
-        if ([NSThread isMainThread]) {
-            updateCacheBlock();
-        }
-        else {
-            dispatch_async(dispatch_get_main_queue(), updateCacheBlock);
+    if (self.spkiCacheFilename.length && isProtectedDataAvailable()) {
+        NSData *serializedSpkiCache = [NSKeyedArchiver archivedDataWithRootObject:self.spkiCache
+                                                            requiringSecureCoding:YES
+                                                                            error:nil];
+        NSURL *cachePath = [self SPKICachePath];
+        if (!cachePath || ![serializedSpkiCache writeToURL:cachePath atomically:YES]) {
+            NSAssert(false, @"Failed to write cache");
+            TSKLog(@"Could not persist SPKI cache to the filesystem");
         }
     }
+    else if (self.spkiCacheFilename.length) {
+        TSKLog(@"Protected data not available, skipping SPKI cache persistence");
+    }
 
     return subjectPublicKeyInfoHash;
 }

TrustKitTests/TSKPublicKeyAlgorithmTests.m

@@ -156,4 +156,46 @@ - (void)testSPKICacheThreadSafetyAndProtectedData
     [self waitForExpectationsWithTimeout:5.0 handler:nil];
 }
 
+// This test hardly manages to reproduce the crash, but it does reproduce it sometimes. 
+- (void)testSPKICacheThreadSafetyAndProtectedDataDoesntCrash
+{
+    XCTestExpectation *expectation = [self expectationWithDescription:@"Cache operations completed"];
+
+    id mockApplication = OCMClassMock([UIApplication class]);
+    OCMStub([mockApplication sharedApplication]).andReturn(mockApplication);
+    OCMStub([mockApplication isProtectedDataAvailable]).andReturn(YES);
+
+    // Perform multiple cache operations in parallel on a background queue
+    SecCertificateRef certificate = [TSKCertificateUtils createCertificateFromDer:@"www.globalsign.com"];
+    dispatch_group_t group = dispatch_group_create();
+    for (int i = 0; i < 100; i++) {
+        dispatch_group_async(group, dispatch_get_global_queue(DISPATCH_QUEUE_PRIORITY_DEFAULT, 0), ^{
+            [self->spkiCache hashSubjectPublicKeyInfoFromCertificate:certificate];
+        });
+    }
+
+    dispatch_group_notify(group, dispatch_get_main_queue(), ^{
+
+        NSDictionary *cache = [self->spkiCache getSubjectPublicKeyInfoHashesCache];
+        XCTAssertEqual(cache.count, 1, @"Cache should contain one entry");
+
+        BOOL yes = YES;
+        OCMStub([mockApplication isProtectedDataAvailable]).andReturn(YES).andDo(^(NSInvocation *invocation) {
+            self->spkiCache = nil;
+            [invocation setReturnValue:(void *)&yes];
+        });
+
+        // Perform one more cache operation to trigger filesystem write
+        [self->spkiCache hashSubjectPublicKeyInfoFromCertificate:certificate];
+
+        dispatch_after(dispatch_time(DISPATCH_TIME_NOW, (int64_t)(1 * NSEC_PER_SEC)), dispatch_get_main_queue(), ^{
+            CFRelease(certificate);
+            [mockApplication stopMocking];
+            [expectation fulfill];
+        });
+    });
+
+    [self waitForExpectationsWithTimeout:5.0 handler:nil];
+}
+
 @end