Skip to content

Commit

Permalink
[Sync] Implement encryption of outgoing password sharing invitations
Browse files Browse the repository at this point in the history
The ecnryption is happening in the model type worker because both data
types are not encrypted like other data types and use their own
encryption. ModelTypeWorker was chosen because it contains cryptographer
and will implement a corresponding decryption code.

Note that the encryption does not actually work yet (see integration
tests), they will be fixed in follow-up CLs.

Bug: 1468523
Change-Id: Iae8ef4097f1f6bdcdb0b26850382464b8b3c54b0
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4798514
Commit-Queue: Rushan Suleymanov <rushans@google.com>
Reviewed-by: Marc Treib <treib@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1187286}
  • Loading branch information
Rushan Suleymanov authored and Chromium LUCI CQ committed Aug 23, 2023
1 parent 9391e3e commit 155ebd8
Show file tree
Hide file tree
Showing 12 changed files with 242 additions and 14 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -2,26 +2,137 @@
// Use of this source code is governed by a BSD-style license that can be
// found in the LICENSE file.

#include <array>

#include "base/test/scoped_feature_list.h"
#include "chrome/browser/password_manager/password_sender_service_factory.h"
#include "chrome/browser/sync/test/integration/fake_server_match_status_checker.h"
#include "chrome/browser/sync/test/integration/sync_test.h"
#include "components/password_manager/core/browser/features/password_features.h"
#include "components/password_manager/core/browser/password_form.h"
#include "components/password_manager/core/browser/sharing/password_sender_service.h"
#include "components/password_manager/core/browser/sharing/recipient_info.h"
#include "components/sync/base/features.h"
#include "components/sync/engine/nigori/cross_user_sharing_public_private_key_pair.h"
#include "components/sync/protocol/nigori_specifics.pb.h"
#include "content/public/test/browser_test.h"
#include "third_party/boringssl/src/include/openssl/curve25519.h"

using password_manager::PasswordForm;
using password_manager::PasswordRecipient;
using password_manager::PasswordSenderService;
using password_manager::PublicKey;

namespace {

constexpr char kRecipientUserId[] = "recipient_user_id";
constexpr char kPasswordValue[] = "password";
constexpr char kSignonRealm[] = "signon_realm";
constexpr char kOrigin[] = "http://abc.com/";
constexpr char kUsernameElement[] = "username_element";
constexpr char kUsernameValue[] = "username";
constexpr char kPasswordElement[] = "password_element";
constexpr char kPasswordDisplayName[] = "password_display_name";
constexpr char kPasswordAvatarUrl[] = "http://avatar.url/";

constexpr uint32_t kRecipientPublicKeyVersion = 1;

PasswordForm MakePasswordForm() {
PasswordForm password_form;
password_form.password_value = base::UTF8ToUTF16(std::string(kPasswordValue));
password_form.signon_realm = kSignonRealm;
password_form.url = GURL(kOrigin);
password_form.username_element =
base::UTF8ToUTF16(std::string(kUsernameElement));
password_form.username_value = base::UTF8ToUTF16(std::string(kUsernameValue));
password_form.password_element =
base::UTF8ToUTF16(std::string(kPasswordElement));
password_form.display_name =
base::UTF8ToUTF16(std::string(kPasswordDisplayName));
password_form.icon_url = GURL(kPasswordAvatarUrl);
return password_form;
}

class InvitationCommittedChecker
: public fake_server::FakeServerMatchStatusChecker {
public:
explicit InvitationCommittedChecker(size_t expected_entities_count)
: expected_entities_count_(expected_entities_count) {}

bool IsExitConditionSatisfied(std::ostream* os) override {
*os << "Waiting for outgoing password sharing invitation to be committed.";

std::vector<sync_pb::SyncEntity> entities =
fake_server()->GetSyncEntitiesByModelType(
syncer::OUTGOING_PASSWORD_SHARING_INVITATION);
*os << " Actual entities: " << entities.size()
<< ", expected: " << expected_entities_count_;
return entities.size() == expected_entities_count_;
}

private:
const size_t expected_entities_count_;
};

class SingleClientOutgoingPasswordSharingInvitationTest : public SyncTest {
public:
SingleClientOutgoingPasswordSharingInvitationTest()
: SyncTest(SINGLE_CLIENT) {}
: SyncTest(SINGLE_CLIENT) {
override_features_.InitWithFeatures(
/*enabled_features=*/
{password_manager::features::kPasswordManagerEnableSenderService,
syncer::kSharingOfferKeyPairBootstrap},
/*disabled_features=*/{});
}

PasswordSenderService* GetPasswordSenderService() {
return PasswordSenderServiceFactory::GetForProfile(GetProfile(0));
}

PublicKey GenerateRecipientPublicKey() {
sync_pb::CrossUserSharingPublicKey proto_public_key;
proto_public_key.set_version(kRecipientPublicKeyVersion);
std::array<uint8_t, X25519_PUBLIC_VALUE_LEN> public_key =
syncer::CrossUserSharingPublicPrivateKeyPair::GenerateNewKeyPair()
.GetRawPublicKey();
proto_public_key.set_x25519_public_key(public_key.data(),
public_key.size());
return PublicKey::FromProto(proto_public_key);
}

private:
base::test::ScopedFeatureList override_features_{
password_manager::features::kPasswordManagerEnableSenderService};
base::test::ScopedFeatureList override_features_;
};

IN_PROC_BROWSER_TEST_F(SingleClientOutgoingPasswordSharingInvitationTest,
SanityCheck) {
ASSERT_TRUE(SetupSync());
}

IN_PROC_BROWSER_TEST_F(SingleClientOutgoingPasswordSharingInvitationTest,
ShouldCommitSentPassword) {
ASSERT_TRUE(SetupSync());

PasswordRecipient recipient = {.user_id = kRecipientUserId,
.public_key = GenerateRecipientPublicKey()};
GetPasswordSenderService()->SendPasswords({MakePasswordForm()}, recipient);

ASSERT_TRUE(InvitationCommittedChecker(/*expected_entities_count=*/1).Wait());
std::vector<sync_pb::SyncEntity> entities =
GetFakeServer()->GetSyncEntitiesByModelType(
syncer::OUTGOING_PASSWORD_SHARING_INVITATION);
ASSERT_EQ(1u, entities.size());

const sync_pb::OutgoingPasswordSharingInvitationSpecifics& specifics =
entities.front().specifics().outgoing_password_sharing_invitation();
EXPECT_EQ(specifics.recipient_user_id(), kRecipientUserId);
EXPECT_FALSE(specifics.guid().empty());
EXPECT_FALSE(specifics.has_client_only_unencrypted_data());
// TODO(crbug.com/1468523): uncomment the following lines once encryption is
// fixed.
// EXPECT_FALSE(specifics.encrypted_password_sharing_invitation_data()
// .empty());
// EXPECT_EQ(specifics.recipient_key_version(), kRecipientPublicKeyVersion);
}

} // namespace
3 changes: 1 addition & 2 deletions components/sync/engine/commit_contribution_impl.cc
Original file line number Diff line number Diff line change
Expand Up @@ -4,13 +4,12 @@

#include "components/sync/engine/commit_contribution_impl.h"

#include <algorithm>
#include <memory>
#include <string>
#include <utility>

#include "base/logging.h"
#include "base/uuid.h"
#include "base/values.h"
#include "components/sync/base/data_type_histogram.h"
#include "components/sync/base/features.h"
#include "components/sync/base/passphrase_enums.h"
Expand Down
45 changes: 42 additions & 3 deletions components/sync/engine/model_type_worker.cc
Original file line number Diff line number Diff line change
Expand Up @@ -776,14 +776,19 @@ std::unique_ptr<CommitContribution> ModelTypeWorker::GetContribution(
has_local_changes_state_ = kAllNudgedLocalChangesInFlight;
}

if (type_ == OUTGOING_PASSWORD_SHARING_INVITATION) {
// Password sharing invitation types have different encryption scheme and
// are handled separately.
EncryptOutgoingPasswordSharingInvitations(&response);
} else if (type_ == PASSWORDS) {
EncryptPasswordSpecificsData(&response);
}

DCHECK(!AlwaysEncryptedUserTypes().Has(type_) || encryption_enabled_);
DCHECK(!encryption_enabled_ ||
(model_type_state_.encryption_key_name() ==
cryptographer_->GetDefaultEncryptionKeyName()));

if (type_ == PASSWORDS) {
EncryptPasswordSpecificsData(&response);
}
return std::make_unique<CommitContributionImpl>(
type_, model_type_state_.type_context(), std::move(response),
base::BindOnce(&ModelTypeWorker::OnCommitResponse,
Expand Down Expand Up @@ -1305,6 +1310,40 @@ void ModelTypeWorker::EncryptPasswordSpecificsData(
}
}

void ModelTypeWorker::EncryptOutgoingPasswordSharingInvitations(
CommitRequestDataList* request_data_list) {
CHECK(cryptographer_);

for (std::unique_ptr<CommitRequestData>& request_data : *request_data_list) {
EntityData* entity_data = request_data->entity.get();
sync_pb::OutgoingPasswordSharingInvitationSpecifics* specifics =
entity_data->specifics.mutable_outgoing_password_sharing_invitation();

CHECK(specifics->has_client_only_unencrypted_data());
std::string serialized_password_data;
bool success = specifics->client_only_unencrypted_data().SerializeToString(
&serialized_password_data);
specifics->clear_client_only_unencrypted_data();
CHECK(success);

absl::optional<std::vector<uint8_t>> encrypted_data =
cryptographer_->AuthEncryptForCrossUserSharing(
base::as_bytes(base::make_span(serialized_password_data)),
base::as_bytes(base::make_span(
entity_data->recipient_public_key.x25519_public_key())));
// There should not be encryption failure but DCHECK is not used because
// it's not guaranteed. In the worst case, the entity will be committed with
// empty specifics (no unencrypted data will be committed to the server).
// TODO(crbug.com/1468523): add a metric to record encryption failures.
if (encrypted_data) {
specifics->set_encrypted_password_sharing_invitation_data(
encrypted_data->data(), encrypted_data->size());
} else {
DLOG(ERROR) << "Failed to encrypt outgoing password sharing invitation";
}
}
}

GetLocalChangesRequest::GetLocalChangesRequest(
CancelationSignal* cancelation_signal)
: cancelation_signal_(cancelation_signal),
Expand Down
4 changes: 4 additions & 0 deletions components/sync/engine/model_type_worker.h
Original file line number Diff line number Diff line change
Expand Up @@ -306,6 +306,10 @@ class ModelTypeWorker : public UpdateHandler,
// Encrypt the specifics and hide the title if necessary.
void EncryptPasswordSpecificsData(CommitRequestDataList* request_data_list);

// Encrypts password sharing invitation using cross user sharing encryption.
void EncryptOutgoingPasswordSharingInvitations(
CommitRequestDataList* request_data_list);

// The (up to kMaxPayloads) most recent invalidations received since the last
// successful sync cycle.
std::vector<PendingInvalidation> pending_invalidations_;
Expand Down
27 changes: 25 additions & 2 deletions components/sync/engine/model_type_worker_unittest.cc
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,6 @@
#include "components/sync/engine/commit_contribution.h"
#include "components/sync/engine/cycle/entity_change_metric_recording.h"
#include "components/sync/engine/cycle/status_controller.h"
#include "components/sync/engine/model_type_processor.h"
#include "components/sync/protocol/autofill_specifics.pb.h"
#include "components/sync/protocol/entity_specifics.pb.h"
#include "components/sync/protocol/model_type_state.pb.h"
Expand Down Expand Up @@ -209,7 +208,7 @@ class ModelTypeWorkerTest : public ::testing::Test {
initial_state.set_initial_sync_state(
sync_pb::ModelTypeState_InitialSyncState_INITIAL_SYNC_DONE);

InitializeWithState(USER_EVENTS, initial_state);
InitializeWithState(model_type, initial_state);
}

// Initialize with a custom initial ModelTypeState and pending updates.
Expand Down Expand Up @@ -3006,6 +3005,30 @@ TEST_F(ModelTypeWorkerTest, DropHintsAtServer_WithOtherInvalidations) {
}
}

TEST_F(ModelTypeWorkerTest, ShouldEncryptOutgoingPasswordSharingInvitation) {
InitializeCommitOnly(OUTGOING_PASSWORD_SHARING_INVITATION);

EntitySpecifics specifics;
specifics.mutable_outgoing_password_sharing_invitation()
->mutable_client_only_unencrypted_data()
->mutable_password_data()
->set_password_value("password");
processor()->SetCommitRequest(GenerateCommitRequest(kHash1, specifics));
DoSuccessfulCommit();

ASSERT_EQ(1U, server()->GetNumCommitMessages());
ASSERT_EQ(1, server()->GetNthCommitMessage(0).commit().entries_size());
const SyncEntity& entity =
server()->GetNthCommitMessage(0).commit().entries(0);

EXPECT_TRUE(entity.specifics()
.outgoing_password_sharing_invitation()
.has_encrypted_password_sharing_invitation_data());
EXPECT_FALSE(entity.specifics()
.outgoing_password_sharing_invitation()
.has_client_only_unencrypted_data());
}

class ModelTypeWorkerAckTrackingTest : public ModelTypeWorkerTest {
public:
ModelTypeWorkerAckTrackingTest() = default;
Expand Down
19 changes: 19 additions & 0 deletions components/sync/engine/nigori/cryptographer.h
Original file line number Diff line number Diff line change
Expand Up @@ -7,9 +7,12 @@

#include <memory>
#include <string>
#include <vector>

#include "base/containers/span.h"
#include "components/sync/engine/nigori/cross_user_sharing_public_private_key_pair.h"
#include "components/sync/protocol/encryption.pb.h"
#include "third_party/abseil-cpp/absl/types/optional.h"

namespace syncer {

Expand Down Expand Up @@ -48,6 +51,22 @@ class Cryptographer {
virtual bool DecryptToString(const sync_pb::EncryptedData& encrypted,
std::string* decrypted) const = 0;

// Encrypts |plaintext| using Auth HPKE using |recipient_public_key|.
// Authentication is added with the current sender's authentication key.
// Empty optional is returned upon failure.
virtual absl::optional<std::vector<uint8_t>> AuthEncryptForCrossUserSharing(
base::span<const uint8_t> plaintext,
base::span<const uint8_t> recipient_public_key) const = 0;

// Decrypts |encrypted_data| using Auth HPKE using the keys corresponding
// to |recipient_key_version| and authenticates that the sender actually used
// |sender_public_key| upon auth encryption.
// Empty optional is returned upon failure.
virtual absl::optional<std::vector<uint8_t>> AuthDecryptForCrossUserSharing(
base::span<const uint8_t> encrypted_data,
base::span<const uint8_t> sender_public_key,
const uint32_t recipient_key_version) const = 0;

// For testing purposes only: returns the Public-private key-pair associated
// with |version|.
virtual const CrossUserSharingPublicPrivateKeyPair&
Expand Down
2 changes: 2 additions & 0 deletions components/sync/nigori/cryptographer_impl.cc
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@
#include <vector>

#include "base/check.h"
#include "base/logging.h"
#include "base/memory/ptr_util.h"
#include "components/sync/nigori/cross_user_sharing_keys.h"

Expand Down Expand Up @@ -184,6 +185,7 @@ CryptographerImpl::AuthEncryptForCrossUserSharing(
absl::optional encryption_key_version =
cross_user_sharing_keys_.GetEncryptionKeyPairVersion();
if (!encryption_key_version.has_value()) {
DVLOG(1) << "Encryption key pair is not available";
return absl::nullopt;
}
const CrossUserSharingPublicPrivateKeyPair& encryption_key_pair =
Expand Down
4 changes: 2 additions & 2 deletions components/sync/nigori/cryptographer_impl.h
Original file line number Diff line number Diff line change
Expand Up @@ -112,7 +112,7 @@ class CryptographerImpl : public Cryptographer {
// Empty optional is returned upon failure.
absl::optional<std::vector<uint8_t>> AuthEncryptForCrossUserSharing(
base::span<const uint8_t> plaintext,
base::span<const uint8_t> recipient_public_key) const;
base::span<const uint8_t> recipient_public_key) const override;

// Decrypts |encrypted_data| using Auth HPKE using the keys corresponding
// to |recipient_key_version| and authenticates that the sender actually used
Expand All @@ -121,7 +121,7 @@ class CryptographerImpl : public Cryptographer {
absl::optional<std::vector<uint8_t>> AuthDecryptForCrossUserSharing(
base::span<const uint8_t> encrypted_data,
base::span<const uint8_t> sender_public_key,
const uint32_t recipient_key_version) const;
const uint32_t recipient_key_version) const override;

// Cryptographer overrides.
bool CanEncrypt() const override;
Expand Down
22 changes: 22 additions & 0 deletions components/sync/test/fake_cryptographer.cc
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,8 @@

#include "components/sync/test/fake_cryptographer.h"

#include <iterator>

#include "base/containers/contains.h"
#include "base/ranges/algorithm.h"
#include "base/strings/strcat.h"
Expand Down Expand Up @@ -94,4 +96,24 @@ FakeCryptographer::GetCrossUserSharingKeyPairForTesting(
return cross_user_sharing_key_pair_;
}

absl::optional<std::vector<uint8_t>>
FakeCryptographer::AuthEncryptForCrossUserSharing(
base::span<const uint8_t> plaintext,
base::span<const uint8_t> recipient_public_key) const {
std::vector<uint8_t> result;
result.reserve(plaintext.size() + recipient_public_key.size());
base::ranges::copy(recipient_public_key, std::back_inserter(result));
base::ranges::copy(plaintext, std::back_inserter(result));
return result;
}

absl::optional<std::vector<uint8_t>>
FakeCryptographer::AuthDecryptForCrossUserSharing(
base::span<const uint8_t> encrypted_data,
base::span<const uint8_t> sender_public_key,
const uint32_t recipient_key_version) const {
NOTIMPLEMENTED();
return absl::nullopt;
}

} // namespace syncer

0 comments on commit 155ebd8

Please sign in to comment.