From 44bbaeef5ada22e90d1c1d74c983160a8befaa01 Mon Sep 17 00:00:00 2001 From: Michael Kirk Date: Wed, 31 Jan 2018 09:46:56 -0800 Subject: [PATCH 1/6] fixup test --- Signal/test/util/OWSDatabaseConverterTest.m | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/Signal/test/util/OWSDatabaseConverterTest.m b/Signal/test/util/OWSDatabaseConverterTest.m index 45e4fd27e..3ef874042 100644 --- a/Signal/test/util/OWSDatabaseConverterTest.m +++ b/Signal/test/util/OWSDatabaseConverterTest.m @@ -382,7 +382,7 @@ NS_ASSUME_NONNULL_BEGIN const int kItemCount = 50 * 1000; - // Create an populate the unconverted database. + // Create and populate an unconverted database. [self openYapDatabase:databaseFilePath databasePassword:databasePassword databaseSalt:nil @@ -391,7 +391,8 @@ NS_ASSUME_NONNULL_BEGIN YapDatabaseConnection *dbConnection = database.newConnection; [dbConnection readWriteWithBlock:^(YapDatabaseReadWriteTransaction *_Nonnull transaction) { for (int i = 0; i < kItemCount; i++) { - [transaction setObject:@(i) forKey:@"test_key_name" inCollection:@"test_collection_name"]; + NSString *key = [NSString stringWithFormat:@"key-%d", i]; + [transaction setObject:@"test-object" forKey:key inCollection:@"test_collection_name"]; } }]; }]; From 938b9c85b828008d62f4412a81baba38fe4a43fc Mon Sep 17 00:00:00 2001 From: Michael Kirk Date: Tue, 30 Jan 2018 08:54:50 -0800 Subject: [PATCH 2/6] Don't crash on clean install Otherwise we'll error when retrieving non-existent password. // FREEBIE --- Podfile.lock | 10 +++------- Signal/src/AppDelegate.m | 9 ++++++--- Signal/test/util/OWSDatabaseConverterTest.m | 20 ++++++++------------ SignalServiceKit/src/Storage/OWSStorage.m | 10 ++++++---- 4 files changed, 23 insertions(+), 26 deletions(-) diff --git a/Podfile.lock b/Podfile.lock index e2c1142f2..e09dd8b5e 100644 --- a/Podfile.lock +++ b/Podfile.lock @@ -141,7 +141,7 @@ DEPENDENCIES: - SocketRocket (from `https://github.com/facebook/SocketRocket.git`) - SQLCipher (from `https://github.com/sqlcipher/sqlcipher.git`, commit `d5c2bec`) - SSZipArchive - - YapDatabase/SQLCipher (from `https://github.com/WhisperSystems/YapDatabase.git`, branch `release/unencryptedHeaders`) + - YapDatabase/SQLCipher (from `../YapDatabase`) - YYImage EXTERNAL SOURCES: @@ -167,8 +167,7 @@ EXTERNAL SOURCES: :commit: d5c2bec :git: https://github.com/sqlcipher/sqlcipher.git YapDatabase: - :branch: release/unencryptedHeaders - :git: https://github.com/WhisperSystems/YapDatabase.git + :path: ../YapDatabase CHECKOUT OPTIONS: AxolotlKit: @@ -192,9 +191,6 @@ CHECKOUT OPTIONS: SQLCipher: :commit: d5c2bec :git: https://github.com/sqlcipher/sqlcipher.git - YapDatabase: - :commit: eaff655ebc774105e83f835ead71f8b7a02e4ac1 - :git: https://github.com/WhisperSystems/YapDatabase.git SPEC CHECKSUMS: AFNetworking: 5e0e199f73d8626b11e79750991f5d173d1f8b67 @@ -221,6 +217,6 @@ SPEC CHECKSUMS: YapDatabase: 299a32de9d350d37a9ac5b0532609d87d5d2a5de YYImage: 1e1b62a9997399593e4b9c4ecfbbabbf1d3f3b54 -PODFILE CHECKSUM: 0d804514eb2db34b9874b653e543255d8c2f5751 +PODFILE CHECKSUM: bb32efdc239e2a93d6304d25de33a25dc4cdbab2 COCOAPODS: 1.3.1 diff --git a/Signal/src/AppDelegate.m b/Signal/src/AppDelegate.m index 0f8d406d6..20adce933 100644 --- a/Signal/src/AppDelegate.m +++ b/Signal/src/AppDelegate.m @@ -251,6 +251,10 @@ static NSString *const kURLHostVerifyPrefix = @"verify"; - (nullable NSError *)convertDatabaseIfNecessary { NSString *databaseFilePath = [TSStorageManager legacyDatabaseFilePath]; + if (![[NSFileManager defaultManager] fileExistsAtPath:databaseFilePath]) { + DDLogVerbose(@"%@ no legacy database file found", self.logTag); + return nil; + } NSError *error; NSData *_Nullable databasePassword = [OWSStorage tryToLoadDatabasePassword:&error]; @@ -260,7 +264,7 @@ static NSString *const kURLHostVerifyPrefix = @"verify"; OWSErrorCodeDatabaseConversionFatalError, @"Failed to load database password")); } - YapDatabaseSaltBlock saltBlock = ^(NSData *saltData) { + YapDatabaseSaltBlock recordSaltBlock = ^(NSData *saltData) { DDLogVerbose(@"%@ saltData: %@", self.logTag, saltData.hexadecimalString); [OWSStorage storeDatabaseSalt:saltData]; }; @@ -271,8 +275,7 @@ static NSString *const kURLHostVerifyPrefix = @"verify"; return [YapDatabaseCryptoUtils convertDatabaseIfNecessary:databaseFilePath databasePassword:databasePassword - saltBlock:saltBlock - keySpecBlock:keySpecBlock]; + recordSaltBlock:recordSaltBlock]; } - (void)startupLogging diff --git a/Signal/test/util/OWSDatabaseConverterTest.m b/Signal/test/util/OWSDatabaseConverterTest.m index 3ef874042..b9a24bfc3 100644 --- a/Signal/test/util/OWSDatabaseConverterTest.m +++ b/Signal/test/util/OWSDatabaseConverterTest.m @@ -298,7 +298,7 @@ NS_ASSUME_NONNULL_BEGIN XCTAssertTrue([YapDatabaseCryptoUtils doesDatabaseNeedToBeConverted:databaseFilePath]); __block NSData *_Nullable databaseSalt = nil; - YapDatabaseSaltBlock saltBlock = ^(NSData *saltData) { + YapDatabaseSaltBlock recordSaltBlock = ^(NSData *saltData) { OWSAssert(!databaseSalt); OWSAssert(saltData); @@ -313,8 +313,7 @@ NS_ASSUME_NONNULL_BEGIN }; NSError *_Nullable error = [YapDatabaseCryptoUtils convertDatabaseIfNecessary:databaseFilePath databasePassword:databasePassword - saltBlock:saltBlock - keySpecBlock:keySpecBlock]; + recordSaltBlock:recordSaltBlock]; if (error) { DDLogError(@"%s error: %@", __PRETTY_FUNCTION__, error); } @@ -355,8 +354,7 @@ NS_ASSUME_NONNULL_BEGIN }; NSError *_Nullable error = [YapDatabaseCryptoUtils convertDatabaseIfNecessary:databaseFilePath databasePassword:databasePassword - saltBlock:saltBlock - keySpecBlock:keySpecBlock]; + recordSaltBlock:recordSaltBlock]; if (error) { DDLogError(@"%s error: %@", __PRETTY_FUNCTION__, error); } @@ -400,7 +398,7 @@ NS_ASSUME_NONNULL_BEGIN XCTAssertTrue([YapDatabaseCryptoUtils doesDatabaseNeedToBeConverted:databaseFilePath]); __block NSData *_Nullable databaseSalt = nil; - YapDatabaseSaltBlock saltBlock = ^(NSData *saltData) { + YapDatabaseSaltBlock recordSaltBlock = ^(NSData *saltData) { OWSAssert(!databaseSalt); OWSAssert(saltData); @@ -450,7 +448,7 @@ NS_ASSUME_NONNULL_BEGIN [self createDatabase:databasePassword databaseSalt:databaseSalt databaseKeySpec:databaseKeySpec]; XCTAssertFalse([YapDatabaseCryptoUtils doesDatabaseNeedToBeConverted:databaseFilePath]); - YapDatabaseSaltBlock saltBlock = ^(NSData *saltData) { + YapDatabaseSaltBlock recordSaltBlock = ^(NSData *saltData) { OWSAssert(saltData); XCTFail(@"%s No conversion should be necessary", __PRETTY_FUNCTION__); @@ -463,8 +461,7 @@ NS_ASSUME_NONNULL_BEGIN NSError *_Nullable error = [YapDatabaseCryptoUtils convertDatabaseIfNecessary:databaseFilePath databasePassword:databasePassword - saltBlock:saltBlock - keySpecBlock:keySpecBlock]; + recordSaltBlock:recordSaltBlock]; if (error) { DDLogError(@"%s error: %@", __PRETTY_FUNCTION__, error); } @@ -488,7 +485,7 @@ NS_ASSUME_NONNULL_BEGIN [self createDatabase:databasePassword databaseSalt:databaseSalt databaseKeySpec:databaseKeySpec]; XCTAssertFalse([YapDatabaseCryptoUtils doesDatabaseNeedToBeConverted:databaseFilePath]); - YapDatabaseSaltBlock saltBlock = ^(NSData *saltData) { + YapDatabaseSaltBlock recordSaltBlock = ^(NSData *saltData) { OWSAssert(saltData); XCTFail(@"%s No conversion should be necessary", __PRETTY_FUNCTION__); @@ -501,8 +498,7 @@ NS_ASSUME_NONNULL_BEGIN NSError *_Nullable error = [YapDatabaseCryptoUtils convertDatabaseIfNecessary:databaseFilePath databasePassword:databasePassword - saltBlock:saltBlock - keySpecBlock:keySpecBlock]; + recordSaltBlock:recordSaltBlock]; if (error) { DDLogError(@"%s error: %@", __PRETTY_FUNCTION__, error); } diff --git a/SignalServiceKit/src/Storage/OWSStorage.m b/SignalServiceKit/src/Storage/OWSStorage.m index f99e40c06..918a589c4 100644 --- a/SignalServiceKit/src/Storage/OWSStorage.m +++ b/SignalServiceKit/src/Storage/OWSStorage.m @@ -388,10 +388,10 @@ typedef NSData *_Nullable (^CreateDatabaseMetadataBlock)(void); // Although we don't use databasePassword or databaseSalt in this method, // we use their accessors to ensure that all three exist in the keychain // and can be loaded or that we reset the database & keychain. - NSData *databasePassword = [self databasePassword]; - OWSAssert(databasePassword.length > 0); - NSData *databaseSalt = [self databaseSalt]; - OWSAssert(databaseSalt.length > 0); + // NSData *databasePassword = [self databasePassword]; + // OWSAssert(databasePassword.length > 0); + // NSData *databaseSalt = [self databaseSalt]; + // OWSAssert(databaseSalt.length > 0); NSData *databaseKeySpec = [self databaseKeySpec]; OWSAssert(databaseKeySpec.length == kSQLCipherKeySpecLength); @@ -608,6 +608,8 @@ typedef NSData *_Nullable (^CreateDatabaseMetadataBlock)(void); - (NSData *)databaseKeySpec { + // Get or generate salt and cipherKeyData + return [self loadMetadataOrClearDatabase:^(NSError **_Nullable errorHandle) { return [OWSStorage tryToLoadDatabaseKeySpec:errorHandle]; } From 426c9baa1638a3a7955e62222a118bb391075879 Mon Sep 17 00:00:00 2001 From: Michael Kirk Date: Wed, 31 Jan 2018 09:15:16 -0800 Subject: [PATCH 3/6] Key material changes - For new installs, generate raw key-spec rather than derive it - Adapt to separated concerns of the key derivation migration from the unencrypted header migration - Reduce number of places where we delete/generate keying information - Only store relevant keying material // FREEBIE --- Podfile.lock | 10 +- Signal/src/AppDelegate.m | 12 +- Signal/test/util/OWSDatabaseConverterTest.m | 49 ++--- SignalServiceKit/src/Storage/OWSStorage.h | 7 +- SignalServiceKit/src/Storage/OWSStorage.m | 201 ++++---------------- 5 files changed, 66 insertions(+), 213 deletions(-) diff --git a/Podfile.lock b/Podfile.lock index e09dd8b5e..a3c1a7d90 100644 --- a/Podfile.lock +++ b/Podfile.lock @@ -141,7 +141,7 @@ DEPENDENCIES: - SocketRocket (from `https://github.com/facebook/SocketRocket.git`) - SQLCipher (from `https://github.com/sqlcipher/sqlcipher.git`, commit `d5c2bec`) - SSZipArchive - - YapDatabase/SQLCipher (from `../YapDatabase`) + - YapDatabase/SQLCipher (from `https://github.com/WhisperSystems/YapDatabase.git`, branch `release/unencryptedHeaders`) - YYImage EXTERNAL SOURCES: @@ -167,7 +167,8 @@ EXTERNAL SOURCES: :commit: d5c2bec :git: https://github.com/sqlcipher/sqlcipher.git YapDatabase: - :path: ../YapDatabase + :branch: release/unencryptedHeaders + :git: https://github.com/WhisperSystems/YapDatabase.git CHECKOUT OPTIONS: AxolotlKit: @@ -191,6 +192,9 @@ CHECKOUT OPTIONS: SQLCipher: :commit: d5c2bec :git: https://github.com/sqlcipher/sqlcipher.git + YapDatabase: + :commit: a88958a8db03a050650a495394e1817e48d99f4b + :git: https://github.com/WhisperSystems/YapDatabase.git SPEC CHECKSUMS: AFNetworking: 5e0e199f73d8626b11e79750991f5d173d1f8b67 @@ -217,6 +221,6 @@ SPEC CHECKSUMS: YapDatabase: 299a32de9d350d37a9ac5b0532609d87d5d2a5de YYImage: 1e1b62a9997399593e4b9c4ecfbbabbf1d3f3b54 -PODFILE CHECKSUM: bb32efdc239e2a93d6304d25de33a25dc4cdbab2 +PODFILE CHECKSUM: 0d804514eb2db34b9874b653e543255d8c2f5751 COCOAPODS: 1.3.1 diff --git a/Signal/src/AppDelegate.m b/Signal/src/AppDelegate.m index 20adce933..fa62472d4 100644 --- a/Signal/src/AppDelegate.m +++ b/Signal/src/AppDelegate.m @@ -257,7 +257,7 @@ static NSString *const kURLHostVerifyPrefix = @"verify"; } NSError *error; - NSData *_Nullable databasePassword = [OWSStorage tryToLoadDatabasePassword:&error]; + NSData *_Nullable databasePassword = [OWSStorage tryToLoadDatabaseLegacyPassphrase:&error]; if (!databasePassword || error) { return (error ?: OWSErrorWithCodeDescription( @@ -266,11 +266,11 @@ static NSString *const kURLHostVerifyPrefix = @"verify"; YapDatabaseSaltBlock recordSaltBlock = ^(NSData *saltData) { DDLogVerbose(@"%@ saltData: %@", self.logTag, saltData.hexadecimalString); - [OWSStorage storeDatabaseSalt:saltData]; - }; - YapDatabaseKeySpecBlock keySpecBlock = ^(NSData *keySpecData) { - DDLogVerbose(@"%@ keySpecData: %@", self.logTag, keySpecData.hexadecimalString); - [OWSStorage storeDatabaseKeySpec:keySpecData]; + + // Derive and store the raw cipher key spec, to avoid the ongoing tax of future KDF + NSData *keySpecData = + [YapDatabaseCryptoUtils deriveDatabaseKeySpecForPassword:databasePassword saltData:saltData]; + [OWSStorage storeDatabaseCipherKeySpec:keySpecData]; }; return [YapDatabaseCryptoUtils convertDatabaseIfNecessary:databaseFilePath diff --git a/Signal/test/util/OWSDatabaseConverterTest.m b/Signal/test/util/OWSDatabaseConverterTest.m index b9a24bfc3..a7ffce43f 100644 --- a/Signal/test/util/OWSDatabaseConverterTest.m +++ b/Signal/test/util/OWSDatabaseConverterTest.m @@ -298,19 +298,15 @@ NS_ASSUME_NONNULL_BEGIN XCTAssertTrue([YapDatabaseCryptoUtils doesDatabaseNeedToBeConverted:databaseFilePath]); __block NSData *_Nullable databaseSalt = nil; + __block NSData *_Nullable databaseKeySpec = nil; YapDatabaseSaltBlock recordSaltBlock = ^(NSData *saltData) { OWSAssert(!databaseSalt); OWSAssert(saltData); databaseSalt = saltData; + databaseKeySpec = [YapDatabaseCryptoUtils deriveDatabaseKeySpecForPassword:databasePassword saltData:saltData]; }; - __block NSData *_Nullable databaseKeySpec = nil; - YapDatabaseKeySpecBlock keySpecBlock = ^(NSData *keySpecData) { - OWSAssert(!databaseKeySpec); - OWSAssert(keySpecData); - databaseKeySpec = keySpecData; - }; NSError *_Nullable error = [YapDatabaseCryptoUtils convertDatabaseIfNecessary:databaseFilePath databasePassword:databasePassword recordSaltBlock:recordSaltBlock]; @@ -339,19 +335,16 @@ NS_ASSUME_NONNULL_BEGIN XCTAssertTrue([YapDatabaseCryptoUtils doesDatabaseNeedToBeConverted:databaseFilePath]); __block NSData *_Nullable databaseSalt = nil; - YapDatabaseSaltBlock saltBlock = ^(NSData *saltData) { + + __block NSData *_Nullable databaseKeySpec = nil; + YapDatabaseSaltBlock recordSaltBlock = ^(NSData *saltData) { OWSAssert(!databaseSalt); OWSAssert(saltData); databaseSalt = saltData; + databaseKeySpec = [YapDatabaseCryptoUtils deriveDatabaseKeySpecForPassword:databasePassword saltData:saltData]; }; - __block NSData *_Nullable databaseKeySpec = nil; - YapDatabaseKeySpecBlock keySpecBlock = ^(NSData *keySpecData) { - OWSAssert(!databaseKeySpec); - OWSAssert(keySpecData); - - databaseKeySpec = keySpecData; - }; + NSError *_Nullable error = [YapDatabaseCryptoUtils convertDatabaseIfNecessary:databaseFilePath databasePassword:databasePassword recordSaltBlock:recordSaltBlock]; @@ -398,23 +391,19 @@ NS_ASSUME_NONNULL_BEGIN XCTAssertTrue([YapDatabaseCryptoUtils doesDatabaseNeedToBeConverted:databaseFilePath]); __block NSData *_Nullable databaseSalt = nil; + __block NSData *_Nullable databaseKeySpec = nil; YapDatabaseSaltBlock recordSaltBlock = ^(NSData *saltData) { OWSAssert(!databaseSalt); OWSAssert(saltData); databaseSalt = saltData; + databaseKeySpec = [YapDatabaseCryptoUtils deriveDatabaseKeySpecForPassword:databasePassword saltData:saltData]; }; - __block NSData *_Nullable databaseKeySpec = nil; - YapDatabaseKeySpecBlock keySpecBlock = ^(NSData *keySpecData) { - OWSAssert(!databaseKeySpec); - OWSAssert(keySpecData); - - databaseKeySpec = keySpecData; - }; + + NSError *_Nullable error = [YapDatabaseCryptoUtils convertDatabaseIfNecessary:databaseFilePath databasePassword:databasePassword - saltBlock:saltBlock - keySpecBlock:keySpecBlock]; + recordSaltBlock:recordSaltBlock]; if (error) { DDLogError(@"%s error: %@", __PRETTY_FUNCTION__, error); } @@ -428,9 +417,9 @@ NS_ASSUME_NONNULL_BEGIN // Verify the contents of the unconverted database. __block BOOL isValid = NO; [self openYapDatabase:databaseFilePath - databasePassword:databasePassword + databasePassword:nil databaseSalt:nil - databaseKeySpec:nil + databaseKeySpec:databaseKeySpec databaseBlock:^(YapDatabase *database) { YapDatabaseConnection *dbConnection = database.newConnection; isValid = [dbConnection numberOfKeysInCollection:@"test_collection_name"] == kItemCount; @@ -453,11 +442,6 @@ NS_ASSUME_NONNULL_BEGIN XCTFail(@"%s No conversion should be necessary", __PRETTY_FUNCTION__); }; - YapDatabaseKeySpecBlock keySpecBlock = ^(NSData *keySpecData) { - OWSAssert(keySpecData); - - XCTFail(@"%s No conversion should be necessary", __PRETTY_FUNCTION__); - }; NSError *_Nullable error = [YapDatabaseCryptoUtils convertDatabaseIfNecessary:databaseFilePath databasePassword:databasePassword @@ -490,11 +474,6 @@ NS_ASSUME_NONNULL_BEGIN XCTFail(@"%s No conversion should be necessary", __PRETTY_FUNCTION__); }; - YapDatabaseKeySpecBlock keySpecBlock = ^(NSData *keySpecData) { - OWSAssert(keySpecData); - - XCTFail(@"%s No conversion should be necessary", __PRETTY_FUNCTION__); - }; NSError *_Nullable error = [YapDatabaseCryptoUtils convertDatabaseIfNecessary:databaseFilePath databasePassword:databasePassword diff --git a/SignalServiceKit/src/Storage/OWSStorage.h b/SignalServiceKit/src/Storage/OWSStorage.h index 9101cc3cc..d9ef80d83 100644 --- a/SignalServiceKit/src/Storage/OWSStorage.h +++ b/SignalServiceKit/src/Storage/OWSStorage.h @@ -75,13 +75,10 @@ extern NSString *const StorageIsReadyNotification; */ + (BOOL)isDatabasePasswordAccessible; -+ (nullable NSData *)tryToLoadDatabasePassword:(NSError **)errorHandle; ++ (nullable NSData *)tryToLoadDatabaseLegacyPassphrase:(NSError **)errorHandle; -+ (nullable NSData *)tryToLoadDatabaseSalt:(NSError **)errorHandle; -+ (void)storeDatabaseSalt:(NSData *)saltData; ++ (void)storeDatabaseCipherKeySpec:(NSData *)cipherKeySpecData; -+ (nullable NSData *)tryToLoadDatabaseKeySpec:(NSError **)errorHandle; -+ (void)storeDatabaseKeySpec:(NSData *)keySpecData; @end diff --git a/SignalServiceKit/src/Storage/OWSStorage.m b/SignalServiceKit/src/Storage/OWSStorage.m index 918a589c4..c444ffe6a 100644 --- a/SignalServiceKit/src/Storage/OWSStorage.m +++ b/SignalServiceKit/src/Storage/OWSStorage.m @@ -27,9 +27,8 @@ NSString *const OWSStorageExceptionName_NoDatabase = @"OWSStorageExceptionName_N NSString *const OWSResetStorageNotification = @"OWSResetStorageNotification"; static NSString *keychainService = @"TSKeyChainService"; -static NSString *keychainDBPassAccount = @"TSDatabasePass"; -static NSString *keychainDBSalt = @"OWSDatabaseSalt"; -static NSString *keychainDBKeySpec = @"OWSDatabaseKeySpec"; +static NSString *keychainDBLegacyPassphrase = @"TSDatabasePass"; +static NSString *keychainDBCipherKeySpec = @"OWSDatabaseCipherKeySpec"; const NSUInteger kDatabasePasswordLength = 30; @@ -381,17 +380,9 @@ typedef NSData *_Nullable (^CreateDatabaseMetadataBlock)(void); - (BOOL)tryToLoadDatabase { - // We determine the database password, salt and key spec first, since a side effect of + // We determine the database key spec first, since a side effect of // this can be deleting any existing database file (if we're recovering // from a corrupt keychain). - // - // Although we don't use databasePassword or databaseSalt in this method, - // we use their accessors to ensure that all three exist in the keychain - // and can be loaded or that we reset the database & keychain. - // NSData *databasePassword = [self databasePassword]; - // OWSAssert(databasePassword.length > 0); - // NSData *databaseSalt = [self databaseSalt]; - // OWSAssert(databaseSalt.length > 0); NSData *databaseKeySpec = [self databaseKeySpec]; OWSAssert(databaseKeySpec.length == kSQLCipherKeySpecLength); @@ -401,6 +392,11 @@ typedef NSData *_Nullable (^CreateDatabaseMetadataBlock)(void); options.cipherKeySpecBlock = ^{ return databaseKeySpec; }; + + // We leave a portion of the header decrypted so that iOS will recognize the file + // as a SQLite database. Otherwise, because the database lives in a shared data container, + // and our usage of sqlite's write-ahead logging retains a lock on the database, the OS + // would kill the app/share extension as soon as it is backgrounded. options.cipherUnencryptedHeaderLength = kSqliteHeaderLength; // If any of these asserts fails, we need to verify and update @@ -506,7 +502,7 @@ typedef NSData *_Nullable (^CreateDatabaseMetadataBlock)(void); // This might be redundant but in the spirit of thoroughness... [self deleteDatabaseFiles]; - [self deletePasswordFromKeychain]; + [self deleteDBKeys]; if (CurrentAppContext().isMainApp) { [TSAttachmentStream deleteAttachments]; @@ -528,116 +524,41 @@ typedef NSData *_Nullable (^CreateDatabaseMetadataBlock)(void); + (BOOL)isDatabasePasswordAccessible { - [SAMKeychain setAccessibilityType:kSecAttrAccessibleAfterFirstUnlockThisDeviceOnly]; NSError *error; - NSString *dbPassword = [SAMKeychain passwordForService:keychainService account:keychainDBPassAccount error:&error]; + NSData *cipherKeySpec = [self tryToLoadDatabaseCipherKeySpec:&error]; - if (dbPassword && !error) { + if (cipherKeySpec && !error) { return YES; } if (error) { - DDLogWarn(@"Database password couldn't be accessed: %@", error.localizedDescription); + DDLogWarn(@"Database key couldn't be accessed: %@", error.localizedDescription); } return NO; } -+ (nullable NSData *)tryToLoadKeyChainValue:(NSString *)keychainKey errorHandle:(NSError **)errorHandle -{ - OWSAssert(keychainKey.length > 0); - OWSAssert(errorHandle); - - [SAMKeychain setAccessibilityType:kSecAttrAccessibleAfterFirstUnlockThisDeviceOnly]; - - return [SAMKeychain passwordDataForService:keychainService account:keychainKey error:errorHandle]; -} - -+ (nullable NSData *)tryToLoadDatabasePassword:(NSError **)errorHandle -{ - return [self tryToLoadKeyChainValue:keychainDBPassAccount errorHandle:errorHandle]; -} - -+ (nullable NSData *)tryToLoadDatabaseSalt:(NSError **)errorHandle -{ - return [self tryToLoadKeyChainValue:keychainDBSalt errorHandle:errorHandle]; -} - -+ (nullable NSData *)tryToLoadDatabaseKeySpec:(NSError **)errorHandle ++ (nullable NSData *)tryToLoadDatabaseLegacyPassphrase:(NSError **)errorHandle { - return [self tryToLoadKeyChainValue:keychainDBKeySpec errorHandle:errorHandle]; + return [self tryToLoadKeyChainValue:keychainDBLegacyPassphrase errorHandle:errorHandle]; } -- (NSData *)databasePassword ++ (nullable NSData *)tryToLoadDatabaseCipherKeySpec:(NSError **)errorHandle { - return [self loadMetadataOrClearDatabase:^(NSError **_Nullable errorHandle) { - return [OWSStorage tryToLoadDatabasePassword:errorHandle]; - } - createDataBlock:^{ - NSData *passwordData = [self createAndSetNewDatabasePassword]; - NSData *saltData = [self createAndSetNewDatabaseSalt]; - NSData *keySpecData = [self createAndSetNewDatabaseKeySpec]; - - OWSAssert(passwordData.length > 0); - OWSAssert(saltData.length == kSQLCipherSaltLength); - OWSAssert(keySpecData.length == kSQLCipherKeySpecLength); - - return passwordData; - } - label:@"Database password"]; + return [self tryToLoadKeyChainValue:keychainDBCipherKeySpec errorHandle:errorHandle]; } -- (NSData *)databaseSalt ++ (void)storeDatabaseCipherKeySpec:(NSData *)cipherKeySpecData { - return [self loadMetadataOrClearDatabase:^(NSError **_Nullable errorHandle) { - return [OWSStorage tryToLoadDatabaseSalt:errorHandle]; - } - createDataBlock:^{ - NSData *passwordData = [self createAndSetNewDatabasePassword]; - NSData *saltData = [self createAndSetNewDatabaseSalt]; - NSData *keySpecData = [self createAndSetNewDatabaseKeySpec]; + OWSAssert(cipherKeySpecData.length == kSQLCipherKeySpecLength); - OWSAssert(passwordData.length > 0); - OWSAssert(saltData.length == kSQLCipherSaltLength); - OWSAssert(keySpecData.length == kSQLCipherKeySpecLength); - - return saltData; - } - label:@"Database salt"]; + [self storeKeyChainValue:cipherKeySpecData keychainKey:keychainDBCipherKeySpec]; } - (NSData *)databaseKeySpec { - // Get or generate salt and cipherKeyData - - return [self loadMetadataOrClearDatabase:^(NSError **_Nullable errorHandle) { - return [OWSStorage tryToLoadDatabaseKeySpec:errorHandle]; - } - createDataBlock:^{ - OWSFail(@"%@ It should never be necessary to generate a random key spec.", self.logTag); - - NSData *passwordData = [self createAndSetNewDatabasePassword]; - NSData *saltData = [self createAndSetNewDatabaseSalt]; - NSData *keySpecData = [self createAndSetNewDatabaseKeySpec]; - - OWSAssert(passwordData.length > 0); - OWSAssert(saltData.length == kSQLCipherSaltLength); - OWSAssert(keySpecData.length == kSQLCipherKeySpecLength); - - return keySpecData; - } - label:@"Database key spec"]; -} - -- (NSData *)loadMetadataOrClearDatabase:(LoadDatabaseMetadataBlock)loadDataBlock - createDataBlock:(CreateDatabaseMetadataBlock)createDataBlock - label:(NSString *)label -{ - OWSAssert(loadDataBlock); - OWSAssert(createDataBlock); - NSError *error; - NSData *_Nullable data = loadDataBlock(&error); + NSData *_Nullable data = [[self class] tryToLoadDatabaseCipherKeySpec:&error]; if (error) { // Because we use kSecAttrAccessibleAfterFirstUnlockThisDeviceOnly, @@ -647,7 +568,7 @@ typedef NSData *_Nullable (^CreateDatabaseMetadataBlock)(void); // process that notification, so we should just terminate by throwing // an uncaught exception. NSString *errorDescription = - [NSString stringWithFormat:@"%@ inaccessible. No unlock since device restart? Error: %@", label, error]; + [NSString stringWithFormat:@"CipherKeySpec inaccessible. No unlock since device restart? Error: %@", error]; if (CurrentAppContext().isMainApp) { UIApplicationState applicationState = CurrentAppContext().mainApplicationState; errorDescription = @@ -661,11 +582,10 @@ typedef NSData *_Nullable (^CreateDatabaseMetadataBlock)(void); // TODO: Rather than crash here, we should detect the situation earlier // and exit gracefully - (in the app delegate?). See the ` // This is a last ditch effort to avoid blowing away the user's database. - [self backgroundedAppDatabasePasswordInaccessibleWithErrorDescription:errorDescription]; + [self raiseKeySpecInaccessibleExceptionWithErrorDescription:errorDescription]; } } else { - [self backgroundedAppDatabasePasswordInaccessibleWithErrorDescription: - [NSString stringWithFormat:@"%@ inaccessible; not main app.", label]]; + [self raiseKeySpecInaccessibleExceptionWithErrorDescription:@"CipherKeySpec inaccessible; not main app."]; } // At this point, either this is a new install so there's no existing password to retrieve @@ -681,47 +601,14 @@ typedef NSData *_Nullable (^CreateDatabaseMetadataBlock)(void); // Try to reset app by deleting database. [OWSStorage resetAllStorage]; - data = createDataBlock(); + data = [Randomness generateRandomBytes:(int)kSQLCipherKeySpecLength]; + [[self class] storeDatabaseCipherKeySpec:data]; } return data; } -- (NSData *)createAndSetNewDatabasePassword -{ - NSData *password = [[[Randomness generateRandomBytes:kDatabasePasswordLength] base64EncodedString] - dataUsingEncoding:NSUTF8StringEncoding]; - - [OWSStorage storeDatabasePassword:password]; - - return password; -} - -- (NSData *)createAndSetNewDatabaseSalt -{ - NSData *saltData = [Randomness generateRandomBytes:(int)kSQLCipherSaltLength]; - - [OWSStorage storeDatabaseSalt:saltData]; - - return saltData; -} - -- (NSData *)createAndSetNewDatabaseKeySpec -{ - NSData *databasePassword = [self databasePassword]; - OWSAssert(databasePassword.length > 0); - NSData *databaseSalt = [self databaseSalt]; - OWSAssert(databaseSalt.length == kSQLCipherSaltLength); - - NSData *keySpecData = [YapDatabaseCryptoUtils databaseKeySpecForPassword:databasePassword saltData:databaseSalt]; - OWSAssert(keySpecData.length == kSQLCipherKeySpecLength); - - [OWSStorage storeDatabaseKeySpec:keySpecData]; - - return keySpecData; -} - -- (void)backgroundedAppDatabasePasswordInaccessibleWithErrorDescription:(NSString *)errorDescription +- (void)raiseKeySpecInaccessibleExceptionWithErrorDescription:(NSString *)errorDescription { OWSAssert(CurrentAppContext().isMainApp && CurrentAppContext().isInBackground); @@ -734,11 +621,10 @@ typedef NSData *_Nullable (^CreateDatabaseMetadataBlock)(void); OWSRaiseException(OWSStorageExceptionName_DatabasePasswordInaccessibleWhileBackgrounded, @"%@", errorDescription); } -+ (void)deletePasswordFromKeychain ++ (void)deleteDBKeys { - [SAMKeychain deletePasswordForService:keychainService account:keychainDBPassAccount]; - [SAMKeychain deletePasswordForService:keychainService account:keychainDBSalt]; - [SAMKeychain deletePasswordForService:keychainService account:keychainDBKeySpec]; + [SAMKeychain deletePasswordForService:keychainService account:keychainDBLegacyPassphrase]; + [SAMKeychain deletePasswordForService:keychainService account:keychainDBCipherKeySpec]; } - (unsigned long long)databaseFileSize @@ -746,6 +632,14 @@ typedef NSData *_Nullable (^CreateDatabaseMetadataBlock)(void); return [OWSFileSystem fileSizeOfPath:self.databaseFilePath].unsignedLongLongValue; } ++ (nullable NSData *)tryToLoadKeyChainValue:(NSString *)keychainKey errorHandle:(NSError **)errorHandle +{ + OWSAssert(keychainKey.length > 0); + OWSAssert(errorHandle); + + return [SAMKeychain passwordDataForService:keychainService account:keychainKey error:errorHandle]; +} + + (void)storeKeyChainValue:(NSData *)data keychainKey:(NSString *)keychainKey { OWSAssert(keychainKey.length > 0); @@ -758,8 +652,6 @@ typedef NSData *_Nullable (^CreateDatabaseMetadataBlock)(void); OWSFail(@"%@ Could not store database metadata", self.logTag); OWSProdCritical([OWSAnalyticsEvents storageErrorCouldNotStoreKeychainValue]); - [OWSStorage deletePasswordFromKeychain]; - // Sleep to give analytics events time to be delivered. [NSThread sleepForTimeInterval:15.0f]; @@ -770,25 +662,6 @@ typedef NSData *_Nullable (^CreateDatabaseMetadataBlock)(void); } } -+ (void)storeDatabasePassword:(NSData *)passwordData -{ - [self storeKeyChainValue:passwordData keychainKey:keychainDBPassAccount]; -} - -+ (void)storeDatabaseSalt:(NSData *)saltData -{ - OWSAssert(saltData.length == kSQLCipherSaltLength); - - [self storeKeyChainValue:saltData keychainKey:keychainDBSalt]; -} - -+ (void)storeDatabaseKeySpec:(NSData *)keySpecData -{ - OWSAssert(keySpecData.length == kSQLCipherKeySpecLength); - - [self storeKeyChainValue:keySpecData keychainKey:keychainDBKeySpec]; -} - @end NS_ASSUME_NONNULL_END From d22fc664f2502a2e8009325c4dbf2d95b9b5822a Mon Sep 17 00:00:00 2001 From: Michael Kirk Date: Wed, 31 Jan 2018 13:24:58 -0800 Subject: [PATCH 4/6] more granular key access // FREEBIE --- ...8A553EB1-B9DF-4DDE-8F93-10474ECF05C2.plist | 52 +++++ .../Info.plist | 40 ++++ Signal/test/util/OWSDatabaseConverterTest.m | 195 ++++++++++++++++++ SignalServiceKit/src/Storage/OWSStorage.m | 11 +- 4 files changed, 292 insertions(+), 6 deletions(-) create mode 100644 Signal.xcodeproj/xcshareddata/xcbaselines/D221A0A9169C9E5F00537ABF.xcbaseline/8A553EB1-B9DF-4DDE-8F93-10474ECF05C2.plist create mode 100644 Signal.xcodeproj/xcshareddata/xcbaselines/D221A0A9169C9E5F00537ABF.xcbaseline/Info.plist diff --git a/Signal.xcodeproj/xcshareddata/xcbaselines/D221A0A9169C9E5F00537ABF.xcbaseline/8A553EB1-B9DF-4DDE-8F93-10474ECF05C2.plist b/Signal.xcodeproj/xcshareddata/xcbaselines/D221A0A9169C9E5F00537ABF.xcbaseline/8A553EB1-B9DF-4DDE-8F93-10474ECF05C2.plist new file mode 100644 index 000000000..39650aa08 --- /dev/null +++ b/Signal.xcodeproj/xcshareddata/xcbaselines/D221A0A9169C9E5F00537ABF.xcbaseline/8A553EB1-B9DF-4DDE-8F93-10474ECF05C2.plist @@ -0,0 +1,52 @@ + + + + + classNames + + OWSDatabaseConverterTest + + testGranularKeySpecFetchingStrategy + + com.apple.XCTPerformanceMetric_WallClockTime + + baselineAverage + 0.039171 + baselineIntegrationDisplayName + Local Baseline + + + testGranularPassphraseFetchingStrategy + + com.apple.XCTPerformanceMetric_WallClockTime + + baselineAverage + 0.22846 + baselineIntegrationDisplayName + Local Baseline + + + testWideKeyFetchingStrategy + + com.apple.XCTPerformanceMetric_WallClockTime + + baselineAverage + 0.039649 + baselineIntegrationDisplayName + Local Baseline + + + testWidePassphraseFetchingStrategy + + com.apple.XCTPerformanceMetric_WallClockTime + + baselineAverage + 0.21819 + baselineIntegrationDisplayName + Local Baseline + + + + + + diff --git a/Signal.xcodeproj/xcshareddata/xcbaselines/D221A0A9169C9E5F00537ABF.xcbaseline/Info.plist b/Signal.xcodeproj/xcshareddata/xcbaselines/D221A0A9169C9E5F00537ABF.xcbaseline/Info.plist new file mode 100644 index 000000000..78d6282c7 --- /dev/null +++ b/Signal.xcodeproj/xcshareddata/xcbaselines/D221A0A9169C9E5F00537ABF.xcbaseline/Info.plist @@ -0,0 +1,40 @@ + + + + + runDestinationsByUUID + + 8A553EB1-B9DF-4DDE-8F93-10474ECF05C2 + + localComputer + + busSpeedInMHz + 100 + cpuCount + 1 + cpuKind + Intel Core i7 + cpuSpeedInMHz + 2900 + logicalCPUCoresPerPackage + 8 + modelCode + MacBookPro13,3 + physicalCPUCoresPerPackage + 4 + platformIdentifier + com.apple.platform.macosx + + targetArchitecture + x86_64 + targetDevice + + modelCode + iPhone8,1 + platformIdentifier + com.apple.platform.iphonesimulator + + + + + diff --git a/Signal/test/util/OWSDatabaseConverterTest.m b/Signal/test/util/OWSDatabaseConverterTest.m index a7ffce43f..0a6abf744 100644 --- a/Signal/test/util/OWSDatabaseConverterTest.m +++ b/Signal/test/util/OWSDatabaseConverterTest.m @@ -17,6 +17,8 @@ NS_ASSUME_NONNULL_BEGIN @interface OWSStorage (OWSDatabaseConverterTest) + (YapDatabaseDeserializer)logOnFailureDeserializer; ++ (void)storeKeyChainValue:(NSData *)data keychainKey:(NSString *)keychainKey; ++ (nullable NSData *)tryToLoadKeyChainValue:(NSString *)keychainKey errorHandle:(NSError **)errorHandle; @end @@ -905,6 +907,199 @@ NS_ASSUME_NONNULL_BEGIN DDLogInfo(@"%@: %@", label, output); } + +#pragma mark - keychain strategy benchmarks + + +- (void)verifyTestDatabase:(NSString *)databaseFilePath + databaseKeySpecBlock:(NSData *_Nullable (^_Nullable)(void))databaseKeySpecBlock + databasePasswordBlock:(NSData *_Nullable (^_Nullable)(void))databasePasswordBlock + databaseSaltBlock:(NSData *_Nullable (^_Nullable)(void))databaseSaltBlock +{ + NSData *_Nullable databaseKeySpec = databaseKeySpecBlock ? databaseKeySpecBlock() : nil; + NSData *_Nullable databasePassword = databasePasswordBlock ? databasePasswordBlock() : nil; + NSData *_Nullable databaseSalt = databaseSaltBlock ? databaseSaltBlock() : nil; + + [self verifyTestDatabase:databaseFilePath + databasePassword:databasePassword + databaseSalt:databaseSalt + databaseKeySpec:databaseKeySpec]; +} + +- (void)createTestDatabase:(NSString *)databaseFilePath + databaseKeySpecBlock:(NSData *_Nullable (^_Nullable)(void))databaseKeySpecBlock + databasePasswordBlock:(NSData *_Nullable (^_Nullable)(void))databasePasswordBlock + databaseSaltBlock:(NSData *_Nullable (^_Nullable)(void))databaseSaltBlock +{ + NSData *_Nullable databaseKeySpec = databaseKeySpecBlock ? databaseKeySpecBlock() : nil; + NSData *_Nullable databasePassword = databasePasswordBlock ? databasePasswordBlock() : nil; + NSData *_Nullable databaseSalt = databaseSaltBlock ? databaseSaltBlock() : nil; + + [self createTestDatabase:databaseFilePath + databasePassword:databasePassword + databaseSalt:databaseSalt + databaseKeySpec:databaseKeySpec]; +} + +- (void)storeTestPasswordInKeychain:(NSData *)password +{ + // legacy password length + OWSAssert(password.length == 30); + [OWSStorage storeKeyChainValue:password keychainKey:@"_OWSTestingPassword"]; +} + +- (nullable NSData *)fetchTestPasswordFromKeychain +{ + NSError *error; + NSData *password = [OWSStorage tryToLoadKeyChainValue:@"_OWSTestingPassword" errorHandle:&error]; + OWSAssert(password); + OWSAssert(!error); + // legacy password length + OWSAssert(password.length == 30); + + return password; +} + +- (void)storeTestSaltInKeychain:(NSData *)salt +{ + OWSAssert(salt.length == kSQLCipherSaltLength); + [OWSStorage storeKeyChainValue:salt keychainKey:@"_OWSTestingSalt"]; +} + +- (nullable NSData *)fetchTestSaltFromKeychain +{ + NSError *error; + NSData *salt = [OWSStorage tryToLoadKeyChainValue:@"_OWSTestingSalt" errorHandle:&error]; + OWSAssert(salt); + OWSAssert(!error); + OWSAssert(salt.length == kSQLCipherSaltLength); + return salt; +} + +- (void)storeTestKeySpecInKeychain:(NSData *)keySpec +{ + OWSAssert(keySpec.length == kSQLCipherKeySpecLength); + [OWSStorage storeKeyChainValue:keySpec keychainKey:@"_OWSTestingKeySpec"]; +} + +- (nullable NSData *)fetchTestKeySpecFromKeychain +{ + NSError *error; + NSData *keySpec = [OWSStorage tryToLoadKeyChainValue:@"_OWSTestingKeySpec" errorHandle:&error]; + OWSAssert(keySpec); + OWSAssert(!error); + OWSAssert(keySpec.length == kSQLCipherKeySpecLength); + + return keySpec; +} + +- (void)testWidePassphraseFetchingStrategy +{ + NSData *password = [self randomDatabasePassword]; + NSData *salt = [self randomDatabaseSalt]; + + [self measureBlock:^{ + NSString *databaseFilePath = [self createTempDatabaseFilePath]; + + [self createTestDatabase:databaseFilePath + databaseKeySpecBlock:nil + databasePasswordBlock:^() { + return password; + } + databaseSaltBlock:^() { + return salt; + }]; + + [self verifyTestDatabase:databaseFilePath + databaseKeySpecBlock:nil + databasePasswordBlock:^() { + return password; + } + databaseSaltBlock:^() { + return salt; + }]; + }]; +} + +- (void)testGranularPassphraseFetchingStrategy +{ + NSData *password = [self randomDatabasePassword]; + NSData *salt = [self randomDatabaseSalt]; + [self storeTestPasswordInKeychain:password]; + [self storeTestSaltInKeychain:salt]; + + [self measureBlock:^{ + + NSString *databaseFilePath = [self createTempDatabaseFilePath]; + + + [self createTestDatabase:databaseFilePath + databaseKeySpecBlock:nil + databasePasswordBlock:^() { + return [self fetchTestPasswordFromKeychain]; + } + databaseSaltBlock:^() { + return [self fetchTestSaltFromKeychain]; + }]; + + [self verifyTestDatabase:databaseFilePath + databaseKeySpecBlock:nil + databasePasswordBlock:^() { + return [self fetchTestPasswordFromKeychain]; + } + databaseSaltBlock:^() { + return [self fetchTestSaltFromKeychain]; + }]; + }]; +} + +- (void)testGranularKeySpecFetchingStrategy +{ + NSData *keySpec = [self randomDatabaseKeySpec]; + [self storeTestKeySpecInKeychain:keySpec]; + + [self measureBlock:^{ + NSString *databaseFilePath = [self createTempDatabaseFilePath]; + + [self createTestDatabase:databaseFilePath + databaseKeySpecBlock:^() { + return [self fetchTestKeySpecFromKeychain]; + } + databasePasswordBlock:nil + databaseSaltBlock:nil]; + + [self verifyTestDatabase:databaseFilePath + databaseKeySpecBlock:^() { + return [self fetchTestKeySpecFromKeychain]; + } + databasePasswordBlock:nil + databaseSaltBlock:nil]; + }]; +} + +- (void)testWideKeyFetchingStrategy +{ + NSData *keySpec = [self randomDatabaseKeySpec]; + + [self measureBlock:^{ + NSString *databaseFilePath = [self createTempDatabaseFilePath]; + + [self createTestDatabase:databaseFilePath + databaseKeySpecBlock:^() { + return keySpec; + } + databasePasswordBlock:nil + databaseSaltBlock:nil]; + + [self verifyTestDatabase:databaseFilePath + databaseKeySpecBlock:^() { + return keySpec; + } + databasePasswordBlock:nil + databaseSaltBlock:nil]; + }]; +} + @end NS_ASSUME_NONNULL_END diff --git a/SignalServiceKit/src/Storage/OWSStorage.m b/SignalServiceKit/src/Storage/OWSStorage.m index c444ffe6a..ce8b0ba43 100644 --- a/SignalServiceKit/src/Storage/OWSStorage.m +++ b/SignalServiceKit/src/Storage/OWSStorage.m @@ -380,16 +380,15 @@ typedef NSData *_Nullable (^CreateDatabaseMetadataBlock)(void); - (BOOL)tryToLoadDatabase { - // We determine the database key spec first, since a side effect of - // this can be deleting any existing database file (if we're recovering - // from a corrupt keychain). - NSData *databaseKeySpec = [self databaseKeySpec]; - OWSAssert(databaseKeySpec.length == kSQLCipherKeySpecLength); - YapDatabaseOptions *options = [[YapDatabaseOptions alloc] init]; options.corruptAction = YapDatabaseCorruptAction_Fail; options.enableMultiProcessSupport = YES; options.cipherKeySpecBlock = ^{ + // Rather than compute this once and capture the value of the key + // in the closure, we prefer to fetch the key from the keychain multiple times + // in order to keep the key out of application memory. + NSData *databaseKeySpec = [self databaseKeySpec]; + OWSAssert(databaseKeySpec.length == kSQLCipherKeySpecLength); return databaseKeySpec; }; From 6f959ff2929484fbd6ce8224db4d8b52f904811d Mon Sep 17 00:00:00 2001 From: Michael Kirk Date: Wed, 31 Jan 2018 17:13:42 -0800 Subject: [PATCH 5/6] CR: be more conservative about deriving key spec, clear old passphrase after deriving key spec. // FREEBIE --- Podfile.lock | 2 +- Signal/src/AppDelegate.m | 13 ++++- Signal/test/util/OWSDatabaseConverterTest.m | 58 ++++++++++++++++++--- SignalServiceKit/src/Storage/OWSStorage.h | 1 + SignalServiceKit/src/Storage/OWSStorage.m | 12 ++++- 5 files changed, 75 insertions(+), 11 deletions(-) diff --git a/Podfile.lock b/Podfile.lock index a3c1a7d90..c2906f6ef 100644 --- a/Podfile.lock +++ b/Podfile.lock @@ -193,7 +193,7 @@ CHECKOUT OPTIONS: :commit: d5c2bec :git: https://github.com/sqlcipher/sqlcipher.git YapDatabase: - :commit: a88958a8db03a050650a495394e1817e48d99f4b + :commit: bdd7409de45f9e38b9144adba3b38d74ca48ea77 :git: https://github.com/WhisperSystems/YapDatabase.git SPEC CHECKSUMS: diff --git a/Signal/src/AppDelegate.m b/Signal/src/AppDelegate.m index fa62472d4..4bbd3ca9f 100644 --- a/Signal/src/AppDelegate.m +++ b/Signal/src/AppDelegate.m @@ -264,13 +264,22 @@ static NSString *const kURLHostVerifyPrefix = @"verify"; OWSErrorCodeDatabaseConversionFatalError, @"Failed to load database password")); } - YapDatabaseSaltBlock recordSaltBlock = ^(NSData *saltData) { + YapRecordDatabaseSaltBlock recordSaltBlock = ^(NSData *saltData) { DDLogVerbose(@"%@ saltData: %@", self.logTag, saltData.hexadecimalString); // Derive and store the raw cipher key spec, to avoid the ongoing tax of future KDF - NSData *keySpecData = + NSData *_Nullable keySpecData = [YapDatabaseCryptoUtils deriveDatabaseKeySpecForPassword:databasePassword saltData:saltData]; + + if (!keySpecData) { + DDLogError(@"%@ Failed to derive key spec.", self.logTag); + return NO; + } + [OWSStorage storeDatabaseCipherKeySpec:keySpecData]; + [OWSStorage removeLegacyPassphrase]; + + return YES; }; return [YapDatabaseCryptoUtils convertDatabaseIfNecessary:databaseFilePath diff --git a/Signal/test/util/OWSDatabaseConverterTest.m b/Signal/test/util/OWSDatabaseConverterTest.m index 0a6abf744..2f9778e0f 100644 --- a/Signal/test/util/OWSDatabaseConverterTest.m +++ b/Signal/test/util/OWSDatabaseConverterTest.m @@ -301,12 +301,15 @@ NS_ASSUME_NONNULL_BEGIN __block NSData *_Nullable databaseSalt = nil; __block NSData *_Nullable databaseKeySpec = nil; - YapDatabaseSaltBlock recordSaltBlock = ^(NSData *saltData) { + YapRecordDatabaseSaltBlock recordSaltBlock = ^(NSData *saltData) { OWSAssert(!databaseSalt); OWSAssert(saltData); databaseSalt = saltData; databaseKeySpec = [YapDatabaseCryptoUtils deriveDatabaseKeySpecForPassword:databasePassword saltData:saltData]; + XCTAssert(databaseKeySpec.length == kSQLCipherKeySpecLength); + + return YES; }; NSError *_Nullable error = [YapDatabaseCryptoUtils convertDatabaseIfNecessary:databaseFilePath @@ -339,12 +342,15 @@ NS_ASSUME_NONNULL_BEGIN __block NSData *_Nullable databaseSalt = nil; __block NSData *_Nullable databaseKeySpec = nil; - YapDatabaseSaltBlock recordSaltBlock = ^(NSData *saltData) { + YapRecordDatabaseSaltBlock recordSaltBlock = ^(NSData *saltData) { OWSAssert(!databaseSalt); OWSAssert(saltData); databaseSalt = saltData; databaseKeySpec = [YapDatabaseCryptoUtils deriveDatabaseKeySpecForPassword:databasePassword saltData:saltData]; + XCTAssert(databaseKeySpec.length == kSQLCipherKeySpecLength); + + return YES; }; NSError *_Nullable error = [YapDatabaseCryptoUtils convertDatabaseIfNecessary:databaseFilePath @@ -367,6 +373,40 @@ NS_ASSUME_NONNULL_BEGIN XCTAssertTrue(isValid); } +// If we fail to record the salt for some reason, we'll be unable to re-open the database +// halt the conversion in hopes that either the failure is intermittent or we can push out +// a patch to fix the problem without having lost the user's DB. +- (void)testDatabaseConversionDoesNotProceedWhenRecordingSaltFails +{ + NSData *databasePassword = [self randomDatabasePassword]; + NSString *_Nullable databaseFilePath = [self createUnconvertedDatabase:databasePassword]; + XCTAssertTrue([YapDatabaseCryptoUtils doesDatabaseNeedToBeConverted:databaseFilePath]); + + __block NSData *_Nullable databaseSalt = nil; + + __block NSData *_Nullable databaseKeySpec = nil; + YapRecordDatabaseSaltBlock recordSaltBlock = ^(NSData *saltData) { + OWSAssert(!databaseSalt); + OWSAssert(saltData); + + // Simulate a failure to record the new salt, e.g. if KDF returns nil + return NO; + }; + + NSError *_Nullable error = [YapDatabaseCryptoUtils convertDatabaseIfNecessary:databaseFilePath + databasePassword:databasePassword + recordSaltBlock:recordSaltBlock]; + + XCTAssertNotNil(error); + XCTAssertTrue([YapDatabaseCryptoUtils doesDatabaseNeedToBeConverted:databaseFilePath]); + + BOOL isValid = [self verifyTestDatabase:databaseFilePath + databasePassword:databasePassword + databaseSalt:nil + databaseKeySpec:databaseKeySpec]; + XCTAssertTrue(isValid); +} + // Verifies that legacy users with non-converted databases can convert. - (void)testDatabaseConversionPerformance_WithKeyspec { @@ -394,15 +434,17 @@ NS_ASSUME_NONNULL_BEGIN __block NSData *_Nullable databaseSalt = nil; __block NSData *_Nullable databaseKeySpec = nil; - YapDatabaseSaltBlock recordSaltBlock = ^(NSData *saltData) { + YapRecordDatabaseSaltBlock recordSaltBlock = ^(NSData *saltData) { OWSAssert(!databaseSalt); OWSAssert(saltData); databaseSalt = saltData; databaseKeySpec = [YapDatabaseCryptoUtils deriveDatabaseKeySpecForPassword:databasePassword saltData:saltData]; + XCTAssert(databaseKeySpec.length == kSQLCipherKeySpecLength); + + return YES; }; - - + NSError *_Nullable error = [YapDatabaseCryptoUtils convertDatabaseIfNecessary:databaseFilePath databasePassword:databasePassword recordSaltBlock:recordSaltBlock]; @@ -439,10 +481,11 @@ NS_ASSUME_NONNULL_BEGIN [self createDatabase:databasePassword databaseSalt:databaseSalt databaseKeySpec:databaseKeySpec]; XCTAssertFalse([YapDatabaseCryptoUtils doesDatabaseNeedToBeConverted:databaseFilePath]); - YapDatabaseSaltBlock recordSaltBlock = ^(NSData *saltData) { + YapRecordDatabaseSaltBlock recordSaltBlock = ^(NSData *saltData) { OWSAssert(saltData); XCTFail(@"%s No conversion should be necessary", __PRETTY_FUNCTION__); + return NO; }; NSError *_Nullable error = [YapDatabaseCryptoUtils convertDatabaseIfNecessary:databaseFilePath @@ -471,10 +514,11 @@ NS_ASSUME_NONNULL_BEGIN [self createDatabase:databasePassword databaseSalt:databaseSalt databaseKeySpec:databaseKeySpec]; XCTAssertFalse([YapDatabaseCryptoUtils doesDatabaseNeedToBeConverted:databaseFilePath]); - YapDatabaseSaltBlock recordSaltBlock = ^(NSData *saltData) { + YapRecordDatabaseSaltBlock recordSaltBlock = ^(NSData *saltData) { OWSAssert(saltData); XCTFail(@"%s No conversion should be necessary", __PRETTY_FUNCTION__); + return NO; }; NSError *_Nullable error = [YapDatabaseCryptoUtils convertDatabaseIfNecessary:databaseFilePath diff --git a/SignalServiceKit/src/Storage/OWSStorage.h b/SignalServiceKit/src/Storage/OWSStorage.h index d9ef80d83..83db4afb1 100644 --- a/SignalServiceKit/src/Storage/OWSStorage.h +++ b/SignalServiceKit/src/Storage/OWSStorage.h @@ -76,6 +76,7 @@ extern NSString *const StorageIsReadyNotification; + (BOOL)isDatabasePasswordAccessible; + (nullable NSData *)tryToLoadDatabaseLegacyPassphrase:(NSError **)errorHandle; ++ (void)removeLegacyPassphrase; + (void)storeDatabaseCipherKeySpec:(NSData *)cipherKeySpecData; diff --git a/SignalServiceKit/src/Storage/OWSStorage.m b/SignalServiceKit/src/Storage/OWSStorage.m index ce8b0ba43..555a0da46 100644 --- a/SignalServiceKit/src/Storage/OWSStorage.m +++ b/SignalServiceKit/src/Storage/OWSStorage.m @@ -544,7 +544,10 @@ typedef NSData *_Nullable (^CreateDatabaseMetadataBlock)(void); + (nullable NSData *)tryToLoadDatabaseCipherKeySpec:(NSError **)errorHandle { - return [self tryToLoadKeyChainValue:keychainDBCipherKeySpec errorHandle:errorHandle]; + NSData *_Nullable data = [self tryToLoadKeyChainValue:keychainDBCipherKeySpec errorHandle:errorHandle]; + OWSAssert(!data || data.length == kSQLCipherKeySpecLength); + + return data; } + (void)storeDatabaseCipherKeySpec:(NSData *)cipherKeySpecData @@ -554,6 +557,13 @@ typedef NSData *_Nullable (^CreateDatabaseMetadataBlock)(void); [self storeKeyChainValue:cipherKeySpecData keychainKey:keychainDBCipherKeySpec]; } ++ (void)removeLegacyPassphrase +{ + DDLogInfo(@"%@ removing legacy passphrase", self.logTag); + + [SAMKeychain deletePasswordForService:keychainService account:keychainDBLegacyPassphrase]; +} + - (NSData *)databaseKeySpec { NSError *error; From 4f8db63fb3c841d0dbfa61d4492835fbbbd61cb7 Mon Sep 17 00:00:00 2001 From: Michael Kirk Date: Wed, 31 Jan 2018 17:54:58 -0800 Subject: [PATCH 6/6] Ensure keyspec is generated before DB is created // FREEBIE --- .../src/Messages/OWSMessageSender.m | 2 +- SignalServiceKit/src/Storage/OWSStorage.m | 45 ++++++++++++++----- 2 files changed, 34 insertions(+), 13 deletions(-) diff --git a/SignalServiceKit/src/Messages/OWSMessageSender.m b/SignalServiceKit/src/Messages/OWSMessageSender.m index 8657cf54d..73b1af86c 100644 --- a/SignalServiceKit/src/Messages/OWSMessageSender.m +++ b/SignalServiceKit/src/Messages/OWSMessageSender.m @@ -1215,7 +1215,7 @@ NSString *const OWSMessageSenderRateLimitedException = @"RateLimitedException"; thread:message.thread attempts:OWSMessageSenderRetryAttempts success:^{ - DDLogInfo(@"Succesfully sent sync transcript."); + DDLogInfo(@"Successfully sent sync transcript."); } failure:^(NSError *error) { // FIXME: We don't yet honor the isRetryable flag here, since sendSyncTranscriptForMessage diff --git a/SignalServiceKit/src/Storage/OWSStorage.m b/SignalServiceKit/src/Storage/OWSStorage.m index 555a0da46..756ef7a7e 100644 --- a/SignalServiceKit/src/Storage/OWSStorage.m +++ b/SignalServiceKit/src/Storage/OWSStorage.m @@ -407,6 +407,11 @@ typedef NSData *_Nullable (^CreateDatabaseMetadataBlock)(void); OWSAssert(options.pragmaJournalSizeLimit == 0); OWSAssert(options.pragmaMMapSize == 0); + // Sanity checking elsewhere asserts we should only regenerate key specs when + // there is no existing database, so rather than lazily generate in the cipherKeySpecBlock + // we must ensure the keyspec exists before we create the database. + [self ensureDatabaseKeySpecExists]; + OWSDatabase *database = [[OWSDatabase alloc] initWithPath:[self databaseFilePath] serializer:nil deserializer:[[self class] logOnFailureDeserializer] @@ -564,20 +569,21 @@ typedef NSData *_Nullable (^CreateDatabaseMetadataBlock)(void); [SAMKeychain deletePasswordForService:keychainService account:keychainDBLegacyPassphrase]; } -- (NSData *)databaseKeySpec +- (void)ensureDatabaseKeySpecExists { NSError *error; - NSData *_Nullable data = [[self class] tryToLoadDatabaseCipherKeySpec:&error]; + NSData *_Nullable keySpec = [[self class] tryToLoadDatabaseCipherKeySpec:&error]; - if (error) { + if (error || (keySpec.length != kSQLCipherKeySpecLength)) { // Because we use kSecAttrAccessibleAfterFirstUnlockThisDeviceOnly, // the keychain will be inaccessible after device restart until // device is unlocked for the first time. If the app receives // a push notification, we won't be able to access the keychain to // process that notification, so we should just terminate by throwing // an uncaught exception. - NSString *errorDescription = - [NSString stringWithFormat:@"CipherKeySpec inaccessible. No unlock since device restart? Error: %@", error]; + NSString *errorDescription = [NSString + stringWithFormat:@"CipherKeySpec inaccessible. New install or no unlock since device restart? Error: %@", + error]; if (CurrentAppContext().isMainApp) { UIApplicationState applicationState = CurrentAppContext().mainApplicationState; errorDescription = @@ -600,9 +606,8 @@ typedef NSData *_Nullable (^CreateDatabaseMetadataBlock)(void); // At this point, either this is a new install so there's no existing password to retrieve // or the keychain has become corrupt. Either way, we want to get back to a // "known good state" and behave like a new install. - - BOOL shouldHaveDatabaseMetadata = [NSFileManager.defaultManager fileExistsAtPath:[self databaseFilePath]]; - if (shouldHaveDatabaseMetadata) { + BOOL doesDBExist = [NSFileManager.defaultManager fileExistsAtPath:[self databaseFilePath]]; + if (doesDBExist) { OWSFail(@"%@ Could not load database metadata", self.logTag); OWSProdCritical([OWSAnalyticsEvents storageErrorCouldNotLoadDatabaseSecondAttempt]); } @@ -610,11 +615,27 @@ typedef NSData *_Nullable (^CreateDatabaseMetadataBlock)(void); // Try to reset app by deleting database. [OWSStorage resetAllStorage]; - data = [Randomness generateRandomBytes:(int)kSQLCipherKeySpecLength]; - [[self class] storeDatabaseCipherKeySpec:data]; + keySpec = [Randomness generateRandomBytes:(int)kSQLCipherKeySpecLength]; + [[self class] storeDatabaseCipherKeySpec:keySpec]; } +} - return data; +- (NSData *)databaseKeySpec +{ + NSError *error; + NSData *_Nullable keySpec = [[self class] tryToLoadDatabaseCipherKeySpec:&error]; + + if (error) { + DDLogError(@"%@ failed to fetch databaseKeySpec with error: %@", self.logTag, error); + [self raiseKeySpecInaccessibleExceptionWithErrorDescription:@"CipherKeySpec inaccessible"]; + } + + if (keySpec.length != kSQLCipherKeySpecLength) { + DDLogError(@"%@ keyspec had length: %lu", self.logTag, (unsigned long)keySpec.length); + [self raiseKeySpecInaccessibleExceptionWithErrorDescription:@"CipherKeySpec invalid"]; + } + + return keySpec; } - (void)raiseKeySpecInaccessibleExceptionWithErrorDescription:(NSString *)errorDescription @@ -667,7 +688,7 @@ typedef NSData *_Nullable (^CreateDatabaseMetadataBlock)(void); OWSRaiseException( OWSStorageExceptionName_DatabasePasswordUnwritable, @"Setting keychain value failed with error: %@", error); } else { - DDLogWarn(@"Succesfully set new keychain value."); + DDLogWarn(@"Successfully set new keychain value."); } }