diff options
| -rw-r--r-- | chrome/browser/sync/internal_api/sync_manager.cc | 68 | ||||
| -rw-r--r-- | sync/engine/conflict_resolver.cc | 31 | ||||
| -rw-r--r-- | sync/engine/syncer_unittest.cc | 92 | ||||
| -rw-r--r-- | sync/util/cryptographer.cc | 30 | ||||
| -rw-r--r-- | sync/util/cryptographer.h | 26 |
5 files changed, 197 insertions, 50 deletions
diff --git a/chrome/browser/sync/internal_api/sync_manager.cc b/chrome/browser/sync/internal_api/sync_manager.cc index 481061c..a02c965 100644 --- a/chrome/browser/sync/internal_api/sync_manager.cc +++ b/chrome/browser/sync/internal_api/sync_manager.cc @@ -232,6 +232,11 @@ class SyncManager::SyncInternal const std::string& chrome_version, const base::Closure& done_callback); + // Stores the current set of encryption keys (if the cryptographer is ready) + // and encrypted types into the nigori node. + void UpdateNigoriEncryptionState(Cryptographer* cryptographer, + WriteNode* nigori_node) const; + // Updates the nigori node with any new encrypted types and then // encrypts the nodes for those new data types as well as other // nodes that should be encrypted but aren't. Triggers @@ -1000,6 +1005,27 @@ void SyncManager::SyncInternal::UpdateCryptographerAndNigori( done_callback)); } +void SyncManager::SyncInternal::UpdateNigoriEncryptionState( + Cryptographer* cryptographer, + WriteNode* nigori_node) const { + DCHECK(nigori_node); + sync_pb::NigoriSpecifics nigori = nigori_node->GetNigoriSpecifics(); + + if (cryptographer->is_ready()) { + // Does not modify the encrypted blob if the unencrypted data already + // matches what is about to be written. + if (!cryptographer->GetKeys(nigori.mutable_encrypted())) + NOTREACHED(); + // Note: we don't try to set using_explicit_passphrase here since if that + // is lost the user can always set it again. The main point is to preserve + // the encryption keys so all data remains decryptable. + } + cryptographer->UpdateNigoriFromEncryptedTypes(&nigori); + + // If nothing has changed, this is a no-op. + nigori_node->SetNigoriSpecifics(nigori); +} + void SyncManager::SyncInternal::UpdateCryptographerAndNigoriCallback( const std::string& chrome_version, const base::Closure& done_callback, @@ -1027,14 +1053,6 @@ void SyncManager::SyncInternal::UpdateCryptographerAndNigoriCallback( pending_keys)); } - // Due to http://crbug.com/102526, we must check if the encryption keys - // are present in the nigori node. If they're not, we write the current - // set of keys. - if (!nigori.has_encrypted() && cryptographer->is_ready()) { - if (!cryptographer->GetKeys(nigori.mutable_encrypted())) { - NOTREACHED(); - } - } // Add or update device information. bool contains_this_device = false; @@ -1069,13 +1087,11 @@ void SyncManager::SyncInternal::UpdateCryptographerAndNigoriCallback( device_information->set_name(session_name); device_information->set_chrome_version(chrome_version); } - - // Ensure the nigori node reflects the most recent set of sensitive - // types and properly sets encrypt_everything. This is a no-op if - // nothing changes. - cryptographer->UpdateNigoriFromEncryptedTypes(&nigori); node.SetNigoriSpecifics(nigori); + // Make sure the nigori node has the up to date encryption info. + UpdateNigoriEncryptionState(cryptographer, &node); + allstatus_.SetCryptographerReady(cryptographer->is_ready()); allstatus_.SetCryptoHasPendingKeys(cryptographer->has_pending_keys()); allstatus_.SetEncryptedTypes(cryptographer->GetEncryptedTypes()); @@ -1462,6 +1478,10 @@ void SyncManager::SyncInternal::FinishSetPassphrase( bool is_explicit, WriteTransaction* trans, WriteNode* nigori_node) { + Cryptographer* cryptographer = trans->GetCryptographer(); + allstatus_.SetCryptographerReady(cryptographer->is_ready()); + allstatus_.SetCryptoHasPendingKeys(cryptographer->has_pending_keys()); + // It's possible we need to change the bootstrap token even if we failed to // set the passphrase (for example if we need to preserve the new GAIA // passphrase). @@ -1472,7 +1492,6 @@ void SyncManager::SyncInternal::FinishSetPassphrase( } if (!success) { - Cryptographer* cryptographer = trans->GetCryptographer(); if (cryptographer->is_ready()) { LOG(ERROR) << "Attempt to change passphrase failed while cryptographer " << "was ready."; @@ -1490,7 +1509,6 @@ void SyncManager::SyncInternal::FinishSetPassphrase( FOR_EACH_OBSERVER(SyncManager::Observer, observers_, OnPassphraseAccepted()); - Cryptographer* cryptographer = trans->GetCryptographer(); DCHECK(cryptographer->is_ready()); // TODO(tim): Bug 58231. It would be nice if setting a passphrase didn't @@ -1551,13 +1569,8 @@ void SyncManager::SyncInternal::RefreshEncryption() { return; } - // Update the Nigori node's set of encrypted datatypes. - // Note, we merge the current encrypted types with those requested. Once a - // datatypes is marked as needing encryption, it is never unmarked. - sync_pb::NigoriSpecifics nigori; - nigori.CopyFrom(node.GetNigoriSpecifics()); - cryptographer->UpdateNigoriFromEncryptedTypes(&nigori); - node.SetNigoriSpecifics(nigori); + UpdateNigoriEncryptionState(cryptographer, &node); + allstatus_.SetEncryptedTypes(cryptographer->GetEncryptedTypes()); // We reencrypt everything regardless of whether the set of encrypted @@ -1994,7 +2007,7 @@ void SyncManager::SyncInternal::OnSyncEngineEvent( { // Check to see if we need to notify the frontend that we have newly // encrypted types or that we require a passphrase. - sync_api::ReadTransaction trans(FROM_HERE, GetUserShare()); + ReadTransaction trans(FROM_HERE, GetUserShare()); Cryptographer* cryptographer = trans.GetCryptographer(); // If we've completed a sync cycle and the cryptographer isn't ready // yet, prompt the user for a passphrase. @@ -2025,6 +2038,15 @@ void SyncManager::SyncInternal::OnSyncEngineEvent( } if (!event.snapshot->has_more_to_sync) { + // To account for a nigori node arriving with stale/bad data, we ensure + // that the nigori node is up to date at the end of each cycle. + WriteTransaction trans(FROM_HERE, GetUserShare()); + WriteNode nigori_node(&trans); + if (nigori_node.InitByTagLookup(kNigoriTag)) { + Cryptographer* cryptographer = trans.GetCryptographer(); + UpdateNigoriEncryptionState(cryptographer, &nigori_node); + } + DVLOG(1) << "Sending OnSyncCycleCompleted"; FOR_EACH_OBSERVER(SyncManager::Observer, observers_, OnSyncCycleCompleted(event.snapshot)); diff --git a/sync/engine/conflict_resolver.cc b/sync/engine/conflict_resolver.cc index 17f76a4..a365e1c 100644 --- a/sync/engine/conflict_resolver.cc +++ b/sync/engine/conflict_resolver.cc @@ -231,13 +231,34 @@ ConflictResolver::ProcessSimpleConflict(WriteTransaction* trans, // Store the merged set of encrypted types (cryptographer->Update(..) will // have merged the local types already). cryptographer->UpdateNigoriFromEncryptedTypes(server_nigori); - // The local set of keys is already merged with the server's set within - // the cryptographer. If we don't have pending keys we can store the - // merged set back immediately. Else we preserve the server keys and will - // update the nigori when the user provides the pending passphrase via - // SetDecryptionPassphrase(..). + // The cryptographer has the both the local and remote encryption keys + // (added at cryptographer->Update(..) time). + // If the cryptographer is ready, then it already merged both sets of keys + // and we can store them back in. In that case, the remote key was already + // part of the local keybag, so we preserve the local key as the default + // (including whether it's an explicit key). + // If the cryptographer is not ready, then the user will have to provide + // the passphrase to decrypt the pending keys. When they do so, the + // SetDecryptionPassphrase code will act based on whether the server + // update has an explicit passphrase or not. + // - If the server had an explicit passphrase, that explicit passphrase + // will be preserved as the default encryption key. + // - If the server did not have an explicit passphrase, we assume the + // local passphrase is the most up to date and preserve the local + // default encryption key marked as an implicit passphrase. + // This works fine except for the case where we had locally set an + // explicit passphrase. In that case the nigori node will have the default + // key based on the local explicit passphassphrase, but will not have it + // marked as explicit. To fix this we'd have to track whether we have a + // explicit passphrase or not separate from the nigori, which would + // introduce even more complexity, so we leave it up to the user to + // reset that passphrase as an explicit one via settings. The goal here + // is to ensure both sets of encryption keys are preserved. if (cryptographer->is_ready()) { cryptographer->GetKeys(server_nigori->mutable_encrypted()); + server_nigori->set_using_explicit_passphrase( + entry.Get(syncable::SPECIFICS).nigori(). + using_explicit_passphrase()); } // TODO(zea): Find a better way of doing this. As it stands, we have to // update this code whenever we add a new non-cryptographer related field diff --git a/sync/engine/syncer_unittest.cc b/sync/engine/syncer_unittest.cc index 9a66d98..56908df 100644 --- a/sync/engine/syncer_unittest.cc +++ b/sync/engine/syncer_unittest.cc @@ -943,6 +943,94 @@ TEST_F(SyncerTest, EncryptionAwareConflicts) { #undef VERIFY_ENTRY +// Receive an old nigori with old encryption keys and encrypted types. We should +// not revert our default key or encrypted types. +TEST_F(SyncerTest, ReceiveOldNigori) { + KeyParams old_key = {"localhost", "dummy", "old"}; + KeyParams current_key = {"localhost", "dummy", "cur"}; + + // Data for testing encryption/decryption. + browser_sync::Cryptographer other_cryptographer(&encryptor_); + other_cryptographer.AddKey(old_key); + sync_pb::EntitySpecifics other_encrypted_specifics; + other_encrypted_specifics.mutable_bookmark()->set_title("title"); + other_cryptographer.Encrypt( + other_encrypted_specifics, + other_encrypted_specifics.mutable_encrypted()); + sync_pb::EntitySpecifics our_encrypted_specifics; + our_encrypted_specifics.mutable_bookmark()->set_title("title2"); + syncable::ModelTypeSet encrypted_types = syncable::ModelTypeSet::All(); + + + // Receive the initial nigori node. + sync_pb::EntitySpecifics initial_nigori_specifics; + initial_nigori_specifics.mutable_nigori(); + mock_server_->SetNigori(1, 10, 10, initial_nigori_specifics); + SyncShareAsDelegate(); + + { + // Set up the current nigori node (containing both keys and encrypt + // everything). + WriteTransaction wtrans(FROM_HERE, UNITTEST, directory()); + sync_pb::EntitySpecifics specifics; + sync_pb::NigoriSpecifics* nigori = specifics.mutable_nigori(); + cryptographer(&wtrans)->AddKey(old_key); + cryptographer(&wtrans)->AddKey(current_key); + cryptographer(&wtrans)->Encrypt( + our_encrypted_specifics, + our_encrypted_specifics.mutable_encrypted()); + cryptographer(&wtrans)->GetKeys( + nigori->mutable_encrypted()); + cryptographer(&wtrans)->set_encrypt_everything(); + cryptographer(&wtrans)->UpdateNigoriFromEncryptedTypes(nigori); + MutableEntry nigori_entry(&wtrans, GET_BY_SERVER_TAG, + syncable::ModelTypeToRootTag(syncable::NIGORI)); + ASSERT_TRUE(nigori_entry.good()); + nigori_entry.Put(SPECIFICS, specifics); + nigori_entry.Put(IS_UNSYNCED, true); + EXPECT_FALSE(cryptographer(&wtrans)->has_pending_keys()); + EXPECT_TRUE(encrypted_types.Equals( + cryptographer(&wtrans)->GetEncryptedTypes())); + } + + SyncShareAsDelegate(); // Commit it. + + // Now set up the old nigori node and add it as a server update. + sync_pb::EntitySpecifics old_nigori_specifics; + sync_pb::NigoriSpecifics *old_nigori = old_nigori_specifics.mutable_nigori(); + other_cryptographer.GetKeys(old_nigori->mutable_encrypted()); + other_cryptographer.UpdateNigoriFromEncryptedTypes(old_nigori); + mock_server_->SetNigori(1, 30, 30, old_nigori_specifics); + + SyncShareAsDelegate(); // Download the old nigori and apply it. + + { + // Ensure everything is committed and stable now. The cryptographer + // should be able to decrypt both sets of keys and still be encrypting with + // the newest, and the encrypted types should be the most recent + ReadTransaction trans(FROM_HERE, directory()); + Entry nigori_entry(&trans, GET_BY_SERVER_TAG, + syncable::ModelTypeToRootTag(syncable::NIGORI)); + ASSERT_TRUE(nigori_entry.good()); + EXPECT_FALSE(nigori_entry.Get(IS_UNAPPLIED_UPDATE)); + EXPECT_FALSE(nigori_entry.Get(IS_UNSYNCED)); + const sync_pb::NigoriSpecifics& nigori = + nigori_entry.Get(SPECIFICS).nigori(); + EXPECT_TRUE(cryptographer(&trans)->CanDecryptUsingDefaultKey( + our_encrypted_specifics.encrypted())); + EXPECT_TRUE(cryptographer(&trans)->CanDecrypt( + other_encrypted_specifics.encrypted())); + EXPECT_TRUE(cryptographer(&trans)->CanDecrypt( + nigori.encrypted())); + EXPECT_TRUE(cryptographer(&trans)->encrypt_everything()); + } +} + +// Resolve a confict between two nigori's with different encrypted types, +// sync_tabs bits, and encryption keys (remote is explicit). Afterwards, the +// encrypted types should be unioned, the sync_tab bit should be true, and the +// cryptographer should have both keys and be encrypting with the remote +// encryption key by default. TEST_F(SyncerTest, NigoriConflicts) { KeyParams local_key_params = {"localhost", "dummy", "blargle"}; KeyParams other_key_params = {"localhost", "dummy", "foobar"}; @@ -995,6 +1083,7 @@ TEST_F(SyncerTest, NigoriConflicts) { nigori->set_encrypt_bookmarks(true); nigori->set_encrypt_preferences(true); nigori->set_encrypt_everything(false); + nigori->set_using_explicit_passphrase(true); mock_server_->SetNigori(1, 20, 20, specifics); } @@ -1019,6 +1108,7 @@ TEST_F(SyncerTest, NigoriConflicts) { cryptographer(&wtrans)->GetEncryptedTypes())); EXPECT_TRUE(cryptographer(&wtrans)->encrypt_everything()); EXPECT_TRUE(specifics.nigori().sync_tabs()); + EXPECT_TRUE(specifics.nigori().using_explicit_passphrase()); // Supply the pending keys. Afterwards, we should be able to decrypt both // our own encrypted data and data encrypted by the other cryptographer, // but the key provided by the other cryptographer should be the default. @@ -1053,6 +1143,8 @@ TEST_F(SyncerTest, NigoriConflicts) { EXPECT_TRUE(cryptographer(&wtrans)-> CanDecryptUsingDefaultKey(other_encrypted_specifics.encrypted())); EXPECT_TRUE(nigori_entry.Get(SPECIFICS).nigori().sync_tabs()); + EXPECT_TRUE(nigori_entry.Get(SPECIFICS).nigori(). + using_explicit_passphrase()); } } diff --git a/sync/util/cryptographer.cc b/sync/util/cryptographer.cc index b3d1833..e2ab5e2 100644 --- a/sync/util/cryptographer.cc +++ b/sync/util/cryptographer.cc @@ -168,15 +168,13 @@ bool Cryptographer::AddKeyImpl(Nigori* initialized_nigori) { return true; } -bool Cryptographer::SetKeys(const sync_pb::EncryptedData& encrypted) { +void Cryptographer::InstallKeys(const sync_pb::EncryptedData& encrypted) { DCHECK(CanDecrypt(encrypted)); sync_pb::NigoriKeyBag bag; - if (!Decrypt(encrypted, &bag)) { - return false; - } - InstallKeys(encrypted.key_name(), bag); - return true; + if (!Decrypt(encrypted, &bag)) + return; + InstallKeyBag(bag); } void Cryptographer::SetPendingKeys(const sync_pb::EncryptedData& encrypted) { @@ -207,7 +205,10 @@ bool Cryptographer::DecryptPendingKeys(const KeyParams& params) { NOTREACHED(); return false; } - InstallKeys(pending_keys_->key_name(), bag); + InstallKeyBag(bag); + const std::string& new_default_key_name = pending_keys_->key_name(); + DCHECK(nigoris_.end() != nigoris_.find(new_default_key_name)); + default_nigori_ = &*nigoris_.find(new_default_key_name); pending_keys_.reset(); return true; } @@ -289,7 +290,15 @@ Cryptographer::UpdateResult Cryptographer::Update( UpdateEncryptedTypesFromNigori(nigori); if (!nigori.encrypted().blob().empty()) { if (CanDecrypt(nigori.encrypted())) { - SetKeys(nigori.encrypted()); + InstallKeys(nigori.encrypted()); + // We only update the default passphrase if this was a new explicit + // passphrase. Else, since it was decryptable, it must not have been a new + // key. + if (nigori.using_explicit_passphrase()) { + std::string new_default_key_name = nigori.encrypted().key_name(); + DCHECK(nigoris_.end() != nigoris_.find(new_default_key_name)); + default_nigori_ = &*nigoris_.find(new_default_key_name); + } return Cryptographer::SUCCESS; } else { SetPendingKeys(nigori.encrypted()); @@ -423,8 +432,7 @@ void Cryptographer::EmitEncryptedTypesChangedNotification() { OnEncryptedTypesChanged(encrypted_types_, encrypt_everything_)); } -void Cryptographer::InstallKeys(const std::string& default_key_name, - const sync_pb::NigoriKeyBag& bag) { +void Cryptographer::InstallKeyBag(const sync_pb::NigoriKeyBag& bag) { int key_size = bag.key_size(); for (int i = 0; i < key_size; ++i) { const sync_pb::NigoriKey key = bag.key(i); @@ -440,8 +448,6 @@ void Cryptographer::InstallKeys(const std::string& default_key_name, nigoris_[key.name()] = make_linked_ptr(new_nigori.release()); } } - DCHECK(nigoris_.end() != nigoris_.find(default_key_name)); - default_nigori_ = &*nigoris_.find(default_key_name); } } // namespace browser_sync diff --git a/sync/util/cryptographer.h b/sync/util/cryptographer.h index af7b272..12f3412 100644 --- a/sync/util/cryptographer.h +++ b/sync/util/cryptographer.h @@ -139,11 +139,6 @@ class Cryptographer { // with a cryptographer that has already been initialized. bool AddKeyFromBootstrapToken(const std::string restored_bootstrap_token); - // Decrypts |encrypted| and uses its contents to initialize Nigori instances. - // Returns true unless decryption of |encrypted| fails. The caller is - // responsible for checking that CanDecrypt(encrypted) == true. - bool SetKeys(const sync_pb::EncryptedData& encrypted); - // Makes a local copy of |encrypted| to later be decrypted by // DecryptPendingKeys. This should only be used if CanDecrypt(encrypted) == // false. @@ -157,7 +152,8 @@ class Cryptographer { // Attempts to decrypt the set of keys that was copied in the previous call to // SetPendingKeys using |params|. Returns true if the pending keys were - // successfully decrypted and installed. + // successfully decrypted and installed. If successful, the default key + // is updated. bool DecryptPendingKeys(const KeyParams& params); bool is_initialized() const { return !nigoris_.empty() && default_nigori_; } @@ -178,6 +174,10 @@ class Cryptographer { // This updates both the encryption keys and the set of encrypted types. // Returns NEEDS_PASSPHRASE if was unable to decrypt the pending keys, // SUCCESS otherwise. + // Note: will not change the default key. If the nigori's keybag + // is decryptable, all keys are added to the local keybag and the current + // default is preserved. If the nigori's keybag is not decryptable, it is + // stored in the |pending_keys_|. UpdateResult Update(const sync_pb::NigoriSpecifics& nigori); // The set of types that are always encrypted. @@ -214,12 +214,18 @@ class Cryptographer { void EmitEncryptedTypesChangedNotification(); + // Decrypts |encrypted| and uses its contents to initialize Nigori instances. + // Returns true unless decryption of |encrypted| fails. The caller is + // responsible for checking that CanDecrypt(encrypted) == true. + // Does not update the default nigori. + void InstallKeys(const sync_pb::EncryptedData& encrypted); + // Helper method to instantiate Nigori instances for each set of key - // parameters in |bag| and setting the default encryption key to - // |default_key_name|. - void InstallKeys(const std::string& default_key_name, - const sync_pb::NigoriKeyBag& bag); + // parameters in |bag|. + // Does not update the default nigori. + void InstallKeyBag(const sync_pb::NigoriKeyBag& bag); + // Helper method to add a nigori as the new default nigori. bool AddKeyImpl(Nigori* nigori); // Functions to serialize + encrypt a Nigori object in an opaque format for |