[Sync] Don't crash for encryption errors

Encryption errors can arise due to OS issues in some cases or simply directory
corruption. Rather than crashing trigger an unrecoverable error.

BUG=123223

Review URL: https://codereview.chromium.org/1161463005

Cr-Commit-Position: refs/heads/master@{#333644}

6 files changed, 121 insertions, 21 deletions

diff --git a/sync/internal_api/base_node.cc b/sync/internal_api/base_node.cc
index 4d6d56f..1121faa 100644
--- a/sync/internal_api/base_node.cc
+++ b/sync/internal_api/base_node.cc
@@ -21,6 +21,7 @@
 #include "sync/protocol/typed_url_specifics.pb.h"
 #include "sync/syncable/directory.h"
 #include "sync/syncable/entry.h"
+#include "sync/syncable/syncable_base_transaction.h"
 #include "sync/syncable/syncable_id.h"
 #include "sync/util/time.h"
 
@@ -56,7 +57,9 @@ bool BaseNode::DecryptIfNecessary() {
     scoped_ptr<sync_pb::PasswordSpecificsData> data(DecryptPasswordSpecifics(
         specifics, GetTransaction()->GetCryptographer()));
     if (!data) {
-      LOG(ERROR) << "Failed to decrypt password specifics.";
+      GetTransaction()->GetWrappedTrans()->OnUnrecoverableError(
+          FROM_HERE, std::string("Failed to decrypt encrypted node of type ") +
+                         ModelTypeToString(GetModelType()));
       return false;
     }
     password_data_.swap(data);
@@ -89,17 +92,14 @@ bool BaseNode::DecryptIfNecessary() {
   std::string plaintext_data = GetTransaction()->GetCryptographer()->
       DecryptToString(encrypted);
   if (plaintext_data.length() == 0) {
-    LOG(ERROR) << "Failed to decrypt encrypted node of type "
-               << ModelTypeToString(GetModelType()) << ".";
-    // Debugging for crbug.com/123223. We failed to decrypt the data, which
-    // means we applied an update without having the key or lost the key at a
-    // later point.
-    CHECK(false);
+    GetTransaction()->GetWrappedTrans()->OnUnrecoverableError(
+        FROM_HERE, std::string("Failed to decrypt encrypted node of type ") +
+                       ModelTypeToString(GetModelType()));
     return false;
   } else if (!unencrypted_data_.ParseFromString(plaintext_data)) {
-    // Debugging for crbug.com/123223. We should never succeed in decrypting
-    // but fail to parse into a protobuf.
-    CHECK(false);
+    GetTransaction()->GetWrappedTrans()->OnUnrecoverableError(
+        FROM_HERE, std::string("Failed to parse encrypted node of type ") +
+                       ModelTypeToString(GetModelType()));
     return false;
   }
   DVLOG(2) << "Decrypted specifics of type "
diff --git a/sync/internal_api/sync_encryption_handler_impl.cc b/sync/internal_api/sync_encryption_handler_impl.cc
index 4bf559f..1e73b4a 100644
--- a/sync/internal_api/sync_encryption_handler_impl.cc
+++ b/sync/internal_api/sync_encryption_handler_impl.cc
@@ -833,10 +833,8 @@ void SyncEncryptionHandlerImpl::ReEncryptEverything(
     int64 child_id = passwords_root.GetFirstChildId();
     while (child_id != kInvalidId) {
       WriteNode child(trans);
-      if (child.InitByIdLookup(child_id) != BaseNode::INIT_OK) {
-        NOTREACHED();
-        return;
-      }
+      if (child.InitByIdLookup(child_id) != BaseNode::INIT_OK)
+        break;  // Possible if we failed to decrypt the data for some reason.
       child.SetPasswordSpecifics(child.GetPasswordSpecifics());
       child_id = child.GetSuccessorId();
     }
diff --git a/sync/internal_api/sync_manager_impl_unittest.cc b/sync/internal_api/sync_manager_impl_unittest.cc
index 41b2e2df4..8bab47a 100644
--- a/sync/internal_api/sync_manager_impl_unittest.cc
+++ b/sync/internal_api/sync_manager_impl_unittest.cc
@@ -69,7 +69,7 @@
 #include "sync/test/fake_encryptor.h"
 #include "sync/util/cryptographer.h"
 #include "sync/util/extensions_activity.h"
-#include "sync/util/test_unrecoverable_error_handler.h"
+#include "sync/util/mock_unrecoverable_error_handler.h"
 #include "sync/util/time.h"
 #include "testing/gmock/include/gmock/gmock.h"
 #include "testing/gtest/include/gtest/gtest.h"
@@ -836,7 +836,8 @@ class SyncManagerTest : public testing::Test,
   };
 
   SyncManagerTest()
-      : sync_manager_("Test sync manager") {
+      : sync_manager_("Test sync manager"),
+        mock_unrecoverable_error_handler_(NULL) {
     switches_.encryption_method =
         InternalComponentsFactory::ENCRYPTION_KEYSTORE;
   }
@@ -886,7 +887,8 @@ class SyncManagerTest : public testing::Test,
     args.invalidator_client_id = "fake_invalidator_client_id";
     args.internal_components_factory.reset(GetFactory());
     args.encryptor = &encryptor_;
-    args.unrecoverable_error_handler.reset(new TestUnrecoverableErrorHandler);
+    mock_unrecoverable_error_handler_ = new MockUnrecoverableErrorHandler();
+    args.unrecoverable_error_handler.reset(mock_unrecoverable_error_handler_);
     args.cancelation_signal = &cancelation_signal_;
     sync_manager_.Init(&args);
 
@@ -1080,6 +1082,12 @@ class SyncManagerTest : public testing::Test,
               sync_manager_.GetEncryptionHandler()->GetPassphraseType());
   }
 
+  bool HasUnrecoverableError() {
+    if (mock_unrecoverable_error_handler_)
+      return mock_unrecoverable_error_handler_->invocation_count() > 0;
+    return false;
+  }
+
  private:
   // Needed by |sync_manager_|.
   base::MessageLoop message_loop_;
@@ -1099,6 +1107,9 @@ class SyncManagerTest : public testing::Test,
   StrictMock<SyncEncryptionHandlerObserverMock> encryption_observer_;
   InternalComponentsFactory::Switches switches_;
   InternalComponentsFactory::StorageOption storage_used_;
+
+  // Not owned (ownership is passed to the SyncManager).
+  MockUnrecoverableErrorHandler* mock_unrecoverable_error_handler_;
 };
 
 TEST_F(SyncManagerTest, GetAllNodesForTypeTest) {
@@ -2058,6 +2069,93 @@ TEST_F(SyncManagerTest, UpdatePasswordReencryptEverything) {
   EXPECT_FALSE(ResetUnsyncedEntry(PASSWORDS, client_tag));
 }
 
+// Test that attempting to start up with corrupted password data triggers
+// an unrecoverable error (rather than crashing).
+TEST_F(SyncManagerTest, ReencryptEverythingWithUnrecoverableErrorPasswords) {
+  const char kClientTag[] = "client_tag";
+
+  EXPECT_TRUE(SetUpEncryption(WRITE_TO_NIGORI, DEFAULT_ENCRYPTION));
+  sync_pb::EntitySpecifics entity_specifics;
+  {
+    // Create a synced bookmark with undecryptable data.
+    ReadTransaction trans(FROM_HERE, sync_manager_.GetUserShare());
+
+    Cryptographer other_cryptographer(&encryptor_);
+    KeyParams fake_params = {"localhost", "dummy", "fake_key"};
+    other_cryptographer.AddKey(fake_params);
+    sync_pb::PasswordSpecificsData data;
+    data.set_password_value("secret");
+    other_cryptographer.Encrypt(
+        data,
+        entity_specifics.mutable_password()->mutable_encrypted());
+
+    // Set up the real cryptographer with a different key.
+    KeyParams real_params = {"localhost", "username", "real_key"};
+    trans.GetCryptographer()->AddKey(real_params);
+  }
+  MakeServerNode(sync_manager_.GetUserShare(), PASSWORDS, kClientTag,
+                 syncable::GenerateSyncableHash(PASSWORDS,
+                                                kClientTag),
+                 entity_specifics);
+  EXPECT_FALSE(ResetUnsyncedEntry(PASSWORDS, kClientTag));
+
+  // Force a re-encrypt everything. Should trigger an unrecoverable error due
+  // to being unable to decrypt the data that was previously applied.
+  testing::Mock::VerifyAndClearExpectations(&encryption_observer_);
+  EXPECT_CALL(encryption_observer_, OnEncryptionComplete());
+  EXPECT_CALL(encryption_observer_, OnCryptographerStateChanged(_));
+  EXPECT_CALL(encryption_observer_, OnEncryptedTypesChanged(_, false));
+  EXPECT_FALSE(HasUnrecoverableError());
+  sync_manager_.GetEncryptionHandler()->Init();
+  PumpLoop();
+  EXPECT_TRUE(HasUnrecoverableError());
+}
+
+// Test that attempting to start up with corrupted bookmark data triggers
+// an unrecoverable error (rather than crashing).
+TEST_F(SyncManagerTest, ReencryptEverythingWithUnrecoverableErrorBookmarks) {
+  const char kClientTag[] = "client_tag";
+  EXPECT_CALL(encryption_observer_,
+              OnEncryptedTypesChanged(
+                  HasModelTypes(EncryptableUserTypes()), true));
+  EXPECT_TRUE(SetUpEncryption(WRITE_TO_NIGORI, FULL_ENCRYPTION));
+  sync_pb::EntitySpecifics entity_specifics;
+  {
+    // Create a synced bookmark with undecryptable data.
+    ReadTransaction trans(FROM_HERE, sync_manager_.GetUserShare());
+
+    Cryptographer other_cryptographer(&encryptor_);
+    KeyParams fake_params = {"localhost", "dummy", "fake_key"};
+    other_cryptographer.AddKey(fake_params);
+    sync_pb::EntitySpecifics bm_specifics;
+    bm_specifics.mutable_bookmark()->set_title("title");
+    bm_specifics.mutable_bookmark()->set_url("url");
+    sync_pb::EncryptedData encrypted;
+    other_cryptographer.Encrypt(bm_specifics, &encrypted);
+    entity_specifics.mutable_encrypted()->CopyFrom(encrypted);
+
+    // Set up the real cryptographer with a different key.
+    KeyParams real_params = {"localhost", "username", "real_key"};
+    trans.GetCryptographer()->AddKey(real_params);
+  }
+  MakeServerNode(sync_manager_.GetUserShare(), BOOKMARKS, kClientTag,
+                 syncable::GenerateSyncableHash(BOOKMARKS,
+                                                kClientTag),
+                 entity_specifics);
+  EXPECT_FALSE(ResetUnsyncedEntry(BOOKMARKS, kClientTag));
+
+  // Force a re-encrypt everything. Should trigger an unrecoverable error due
+  // to being unable to decrypt the data that was previously applied.
+  testing::Mock::VerifyAndClearExpectations(&encryption_observer_);
+  EXPECT_CALL(encryption_observer_, OnEncryptionComplete());
+  EXPECT_CALL(encryption_observer_, OnCryptographerStateChanged(_));
+  EXPECT_CALL(encryption_observer_, OnEncryptedTypesChanged(_, true));
+  EXPECT_FALSE(HasUnrecoverableError());
+  sync_manager_.GetEncryptionHandler()->Init();
+  PumpLoop();
+  EXPECT_TRUE(HasUnrecoverableError());
+}
+
 // Verify SetTitle(..) doesn't unnecessarily set IS_UNSYNCED for bookmarks
 // when we write the same data, but does set it when we write new data.
 TEST_F(SyncManagerTest, SetBookmarkTitle) {
diff --git a/sync/internal_api/syncapi_internal.cc b/sync/internal_api/syncapi_internal.cc
index 91a6d79..d3ac523 100644
--- a/sync/internal_api/syncapi_internal.cc
+++ b/sync/internal_api/syncapi_internal.cc
@@ -30,6 +30,8 @@ sync_pb::PasswordSpecificsData* DecryptPasswordSpecifics(
   const sync_pb::EncryptedData& encrypted = password_specifics.encrypted();
   scoped_ptr<sync_pb::PasswordSpecificsData> data(
       new sync_pb::PasswordSpecificsData);
+  if (!crypto->CanDecrypt(encrypted))
+    return NULL;
   if (!crypto->Decrypt(encrypted, data.get()))
     return NULL;
   return data.release();
diff --git a/sync/internal_api/write_node.cc b/sync/internal_api/write_node.cc
index cefe854..4f49dfd 100644
--- a/sync/internal_api/write_node.cc
+++ b/sync/internal_api/write_node.cc
@@ -168,8 +168,8 @@ void WriteNode::SetPasswordSpecifics(
   // This will only update password_specifics if the underlying unencrypted blob
   // was different from |data| or was not encrypted with the proper passphrase.
   if (!cryptographer->Encrypt(data, password_specifics->mutable_encrypted())) {
-    NOTREACHED() << "Failed to encrypt password, possibly due to sync node "
-                 << "corruption";
+    LOG(ERROR) << "Failed to encrypt password, possibly due to sync node "
+               << "corruption";
     return;
   }
   SetEntitySpecifics(entity_specifics);
diff --git a/sync/util/cryptographer.cc b/sync/util/cryptographer.cc
index c22f0a3..bccf9a2 100644
--- a/sync/util/cryptographer.cc
+++ b/sync/util/cryptographer.cc
@@ -127,8 +127,10 @@ std::string Cryptographer::DecryptToString(
     const sync_pb::EncryptedData& encrypted) const {
   NigoriMap::const_iterator it = nigoris_.find(encrypted.key_name());
   if (nigoris_.end() == it) {
-    NOTREACHED() << "Cannot decrypt message";
-    return std::string();  // Caller should have called CanDecrypt(encrypt).
+    // The key used to encrypt the blob is not part of the set of installed
+    // nigoris.
+    LOG(ERROR) << "Cannot decrypt message";
+    return std::string();
   }
 
   std::string plaintext;