CR: be more conservative about deriving key spec, clear old passphrase

after deriving key spec.

// FREEBIE

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:

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

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

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;
 

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;