From 6f959ff2929484fbd6ce8224db4d8b52f904811d Mon Sep 17 00:00:00 2001 From: Michael Kirk Date: Wed, 31 Jan 2018 17:13:42 -0800 Subject: [PATCH] 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;