Fix the potential vulnerability in CheckMembership function The std:equal used to compare two blobs of data had a risk of out-of-bounds. The fix is adding the condition about the size. Bug: b:524176462 Change-Id: Ib72ec0e43e32a5ac5664560928a0925234d30d59 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/8010516 Commit-Queue: Amr Aboelkher <amraboelkher@chromium.org> Commit-Queue: Igor <igorcov@chromium.org> Reviewed-by: Amr Aboelkher <amraboelkher@chromium.org> Cr-Commit-Position: refs/heads/main@{#1654039} NOKEYCHECK=True GitOrigin-RevId: a22ff9d11f228bd1c2e377f55d103671bb6e9e59
diff --git a/src/private_membership_rlwe_client.cc b/src/private_membership_rlwe_client.cc index 39bc87a..3a53174 100644 --- a/src/private_membership_rlwe_client.cc +++ b/src/private_membership_rlwe_client.cc
@@ -351,7 +351,8 @@ const auto& encrypted_id = encrypted_id_value_pair.encrypted_id(); // Check encrypted_id is a prefix of to_match_hash. If it is, then the id // is a member. - if (std::equal(encrypted_id.begin(), encrypted_id.end(), + if (encrypted_id.size() <= to_match_hash.size() && + std::equal(encrypted_id.begin(), encrypted_id.end(), to_match_hash.begin())) { membership_response.set_is_member(true); if (!encrypted_id_value_pair.encrypted_value().empty()) {
diff --git a/src/private_membership_rlwe_client.h b/src/private_membership_rlwe_client.h index d04b808..de25b56 100644 --- a/src/private_membership_rlwe_client.h +++ b/src/private_membership_rlwe_client.h
@@ -230,6 +230,8 @@ query_response); private: + friend class PrivateMembershipRlweClientTestPeer; + static ::rlwe::StatusOr<std::unique_ptr<PrivateMembershipRlweClient>> CreateInternal( private_membership::rlwe::RlweUseCase use_case,
diff --git a/src/private_membership_rlwe_client_regression_test.cc b/src/private_membership_rlwe_client_regression_test.cc index a102c47..2a086de 100644 --- a/src/private_membership_rlwe_client_regression_test.cc +++ b/src/private_membership_rlwe_client_regression_test.cc
@@ -7,6 +7,8 @@ #include <gmock/gmock.h> #include <gtest/gtest.h> #include "absl/status/status.h" +#include "third_party/private-join-and-compute/src/crypto/context.h" +#include "third_party/private_membership/src/internal/rlwe_id_utils.h" #include "third_party/shell-encryption/src/testing/status_testing.h" namespace private_membership { @@ -84,5 +86,111 @@ } } // namespace + +class PrivateMembershipRlweClientTestPeer { + public: + static ::rlwe::StatusOr<private_membership::MembershipResponse> CheckMembership( + PrivateMembershipRlweClient* client, + absl::string_view server_encrypted_id, + const private_membership::rlwe::EncryptedBucket& encrypted_bucket) { + return client->CheckMembership(server_encrypted_id, encrypted_bucket); + } + + static void SetEncryptedBucketParams( + PrivateMembershipRlweClient* client, + const private_membership::rlwe::EncryptedBucketsParameters& params) { + client->encrypted_bucket_params_ = params; + } + + static ::private_join_and_compute::Context* GetContext( + PrivateMembershipRlweClient* client) { + return &client->context_; + } +}; + +namespace { + +TEST(PrivateMembershipRlweClientTest, CheckMembershipOversizedEncryptedId) { + RlwePlaintextId plaintext_id; + plaintext_id.set_non_sensitive_id("nsid"); + plaintext_id.set_sensitive_id("sid"); + ASSERT_OK_AND_ASSIGN( + auto client, + PrivateMembershipRlweClient::Create( + private_membership::rlwe::TEST_USE_CASE, + {plaintext_id})); + + private_membership::rlwe::EncryptedBucketsParameters params; + params.set_encrypted_bucket_id_length(8); + params.set_sensitive_id_hash_type(private_membership::SHA256_NON_SENSITIVE_AND_SENSITIVE_ID); + PrivateMembershipRlweClientTestPeer::SetEncryptedBucketParams(client.get(), params); + + std::string server_encrypted_id = "some_server_encrypted_id"; + ASSERT_OK_AND_ASSIGN( + std::string to_match_hash, + ComputeBucketStoredEncryptedId(server_encrypted_id, params, + PrivateMembershipRlweClientTestPeer::GetContext(client.get()))); + + // Craft an oversized encrypted_id starting with `to_match_hash`. + std::string oversized_encrypted_id = to_match_hash + "extra_bytes_that_make_it_longer"; + + // Create an EncryptedBucket containing this oversized encrypted_id. + private_membership::rlwe::EncryptedBucket encrypted_bucket; + auto* pair = encrypted_bucket.add_encrypted_id_value_pairs(); + pair->set_encrypted_id(oversized_encrypted_id); + pair->set_encrypted_value("some_encrypted_value"); + + // Call CheckMembership. + ASSERT_OK_AND_ASSIGN( + auto response, + PrivateMembershipRlweClientTestPeer::CheckMembership( + client.get(), server_encrypted_id, encrypted_bucket)); + + // The oversized encrypted_id should not be considered a match, because its size is greater + // than `to_match_hash.size()`. + EXPECT_FALSE(response.is_member()); +} + +TEST(PrivateMembershipRlweClientTest, CheckMembershipExactMatch) { + RlwePlaintextId plaintext_id; + plaintext_id.set_non_sensitive_id("nsid"); + plaintext_id.set_sensitive_id("sid"); + ASSERT_OK_AND_ASSIGN( + auto client, + PrivateMembershipRlweClient::Create( + private_membership::rlwe::TEST_USE_CASE, + {plaintext_id})); + + private_membership::rlwe::EncryptedBucketsParameters params; + params.set_encrypted_bucket_id_length(8); + params.set_sensitive_id_hash_type(private_membership::SHA256_NON_SENSITIVE_AND_SENSITIVE_ID); + PrivateMembershipRlweClientTestPeer::SetEncryptedBucketParams(client.get(), params); + + std::string server_encrypted_id = "some_server_encrypted_id"; + ASSERT_OK_AND_ASSIGN( + std::string to_match_hash, + ComputeBucketStoredEncryptedId(server_encrypted_id, params, + PrivateMembershipRlweClientTestPeer::GetContext(client.get()))); + + // Exact matching encrypted_id. + std::string matching_encrypted_id = to_match_hash; + + // Create an EncryptedBucket containing this matching encrypted_id. + private_membership::rlwe::EncryptedBucket encrypted_bucket; + auto* pair = encrypted_bucket.add_encrypted_id_value_pairs(); + pair->set_encrypted_id(matching_encrypted_id); + + // Call CheckMembership. + ASSERT_OK_AND_ASSIGN( + auto response, + PrivateMembershipRlweClientTestPeer::CheckMembership( + client.get(), server_encrypted_id, encrypted_bucket)); + + // It should be a member. + EXPECT_TRUE(response.is_member()); +} + +} // namespace } // namespace rlwe } // namespace private_membership +