From 97ef555f60ea5f7491f1e5e28bacd2f4c99c6e58 Mon Sep 17 00:00:00 2001 From: Francis Tsui Date: Wed, 13 May 2026 10:10:32 -0700 Subject: [PATCH] Ensure that incoming connections are authenticated unless visibility is Everyone. PiperOrigin-RevId: 914926223 --- sharing/BUILD | 2 + sharing/nearby_sharing_service_impl.cc | 7 +- sharing/paired_key_verification_runner.cc | 105 +++++++++--------- sharing/paired_key_verification_runner.h | 26 ++--- .../paired_key_verification_runner_test.cc | 89 +++++++++++---- sharing/share_session.cc | 6 - sharing/share_session_test.cc | 14 --- 7 files changed, 139 insertions(+), 110 deletions(-) diff --git a/sharing/BUILD b/sharing/BUILD index ee2baf42..eb7cee28 100644 --- a/sharing/BUILD +++ b/sharing/BUILD @@ -200,6 +200,7 @@ cc_library( "//sharing/proto:enums_cc_proto", "//sharing/proto:share_cc_proto", "//sharing/proto:wire_format_cc_proto", + "@com_google_absl//absl/base:nullability", "@com_google_absl//absl/functional:any_invocable", "@com_google_absl//absl/time", ], @@ -551,6 +552,7 @@ cc_test( "//sharing/proto:wire_format_cc_proto", "@com_github_protobuf_matchers//protobuf-matchers", "@com_google_absl//absl/functional:any_invocable", + "@com_google_absl//absl/strings", "@com_google_absl//absl/time", "@com_google_googletest//:gtest_main", ], diff --git a/sharing/nearby_sharing_service_impl.cc b/sharing/nearby_sharing_service_impl.cc index a999d629..f125391c 100644 --- a/sharing/nearby_sharing_service_impl.cc +++ b/sharing/nearby_sharing_service_impl.cc @@ -2216,9 +2216,10 @@ void NearbySharingServiceImpl::OnOutgoingConnection( session->RunPairedKeyVerification( ToProtoOsType(device_info_.GetOsType()), { - .visibility = settings_->GetVisibility(), - .last_visibility = settings_->GetLastVisibility(), - .last_visibility_time = settings_->GetLastVisibilityTimestamp(), + // Sender always uses ALL_CONTACTS cert to sign and verify signature. + .visibility = DeviceVisibility::DEVICE_VISIBILITY_ALL_CONTACTS, + .last_visibility = DeviceVisibility::DEVICE_VISIBILITY_ALL_CONTACTS, + .last_visibility_time = absl::UnixEpoch(), }, GetCertificateManager(), absl::bind_front( diff --git a/sharing/paired_key_verification_runner.cc b/sharing/paired_key_verification_runner.cc index e8cd74f3..213d6799 100644 --- a/sharing/paired_key_verification_runner.cc +++ b/sharing/paired_key_verification_runner.cc @@ -26,6 +26,7 @@ #include #include +#include "absl/base/nullability.h" #include "absl/functional/any_invocable.h" #include "absl/time/time.h" #include "internal/platform/clock.h" @@ -56,20 +57,18 @@ namespace { // if a valid signature cannot be generated. This size is consistent with the // GmsCore implementation. const size_t kNearbyShareNumBytesRandomSignature = 72; -constexpr absl::Duration kRelaxAfterSetVisibilityTimeout = absl::Minutes(15); +constexpr absl::Duration kRelaxAfterSetVisibilityTimeout = absl::Minutes(1); PairedKeyVerificationRunner::PairedKeyVerificationResult Convert( nearby::sharing::service::proto::PairedKeyResultFrame::Status status) { switch (status) { - case PairedKeyResultFrame::UNKNOWN: - return PairedKeyVerificationRunner::PairedKeyVerificationResult::kUnknown; - case PairedKeyResultFrame::SUCCESS: return PairedKeyVerificationRunner::PairedKeyVerificationResult::kSuccess; case PairedKeyResultFrame::FAIL: return PairedKeyVerificationRunner::PairedKeyVerificationResult::kFail; + case PairedKeyResultFrame::UNKNOWN: case PairedKeyResultFrame::UNABLE: return PairedKeyVerificationRunner::PairedKeyVerificationResult::kUnable; } @@ -91,40 +90,24 @@ std::ostream& operator<<( } PairedKeyVerificationRunner::PairedKeyVerificationRunner( - Clock* clock, OSType os_type, bool share_target_is_incoming, + Clock* absl_nonnull clock, OSType os_type, bool share_target_is_incoming, const VisibilityHistory& visibility_history, const std::vector& token, absl::AnyInvocable frame_writer, const std::optional& certificate, - NearbyShareCertificateManager* certificate_manager, - IncomingFramesReader* frames_reader, absl::Duration read_frame_timeout) - : clock_(clock), + NearbyShareCertificateManager* absl_nonnull certificate_manager, + IncomingFramesReader* absl_nonnull frames_reader, + absl::Duration read_frame_timeout) + : clock_(*clock), + certificate_manager_(*certificate_manager), + frames_reader_(*frames_reader), + share_target_is_incoming_(share_target_is_incoming), os_type_(os_type), - raw_token_(token), - frame_writer_(std::move(frame_writer)), + visibility_history_(visibility_history), certificate_(certificate), - certificate_manager_(certificate_manager), - frames_reader_(frames_reader), - read_frame_timeout_(read_frame_timeout) { - DCHECK(clock_); - DCHECK(certificate_manager); - DCHECK(frames_reader); - - if (share_target_is_incoming) { - local_prefix_ = kNearbyShareReceiverVerificationPrefix; - remote_prefix_ = kNearbyShareSenderVerificationPrefix; - visibility_history_ = visibility_history; - } else { - remote_prefix_ = kNearbyShareReceiverVerificationPrefix; - local_prefix_ = kNearbyShareSenderVerificationPrefix; - // Sender always uses ALL_CONTACTS cert to sign and verify signature. - visibility_history_ = { - .visibility = DeviceVisibility::DEVICE_VISIBILITY_ALL_CONTACTS, - .last_visibility = DeviceVisibility::DEVICE_VISIBILITY_ALL_CONTACTS, - .last_visibility_time = absl::UnixEpoch(), - }; - } -} + read_frame_timeout_(read_frame_timeout), + raw_token_(token), + frame_writer_(std::move(frame_writer)) {} PairedKeyVerificationRunner::~PairedKeyVerificationRunner() = default; @@ -135,7 +118,7 @@ void PairedKeyVerificationRunner::Run( verification_result_ = PairedKeyVerificationResult::kSuccess; SendPairedKeyEncryptionFrame(); - frames_reader_->ReadFrame( + frames_reader_.ReadFrame( V1Frame::PAIRED_KEY_ENCRYPTION, [&, runner = GetWeakPtr()](bool is_timeout, std::optional frame) { @@ -169,6 +152,17 @@ void PairedKeyVerificationRunner::OnReadPairedKeyEncryptionFrame( } } + if (auth_token_hash_result == PairedKeyVerificationResult::kUnable) { + if (share_target_is_incoming_ && + visibility_history_.visibility != + DeviceVisibility::DEVICE_VISIBILITY_EVERYONE) { + VLOG(1) << __func__ << ": Incoming connection with non-everyone " + "visibility cannot verify public certificate. " + "Treating as kFail."; + auth_token_hash_result = PairedKeyVerificationResult::kFail; + } + } + ApplyResult(auth_token_hash_result); VLOG(1) << __func__ << ": Remote public certificate verification result " << auth_token_hash_result; @@ -178,10 +172,18 @@ void PairedKeyVerificationRunner::OnReadPairedKeyEncryptionFrame( ApplyResult(local_result); VLOG(1) << __func__ << ": Paired key encryption verification result " << local_result; + if (share_target_is_incoming_ && visibility_history_.visibility == + DeviceVisibility::DEVICE_VISIBILITY_HIDDEN) { + VLOG(1) << __func__ + << ": device is hidden, reject all incoming connections."; + local_result = PairedKeyVerificationResult::kFail; + ApplyResult(PairedKeyVerificationResult::kFail); + } + SendPairedKeyResultFrame(local_result); - frames_reader_->ReadFrame( + frames_reader_.ReadFrame( V1Frame::PAIRED_KEY_RESULT, [this, runner = GetWeakPtr()](bool is_timeout, std::optional frame) { @@ -240,10 +242,6 @@ void PairedKeyVerificationRunner::SendPairedKeyResultFrame( case PairedKeyVerificationResult::kFail: result_frame->set_status(PairedKeyResultFrame::FAIL); break; - - case PairedKeyVerificationResult::kUnknown: - result_frame->set_status(PairedKeyResultFrame::UNKNOWN); - break; } // Set OS type to allow remote device knowns the paring device OS type. @@ -253,9 +251,13 @@ void PairedKeyVerificationRunner::SendPairedKeyResultFrame( } void PairedKeyVerificationRunner::SendPairedKeyEncryptionFrame() { + std::vector padded_token = PadPrefix( + share_target_is_incoming_ ? kNearbyShareReceiverVerificationPrefix + : kNearbyShareSenderVerificationPrefix, + raw_token_); std::optional> signature = - certificate_manager_->SignWithPrivateCertificate( - visibility_history_.visibility, PadPrefix(local_prefix_, raw_token_)); + certificate_manager_.SignWithPrivateCertificate( + visibility_history_.visibility, padded_token); if (!signature.has_value() || signature->empty()) { signature = GenerateRandomBytes(kNearbyShareNumBytesRandomSignature); } @@ -280,9 +282,8 @@ void PairedKeyVerificationRunner::SendPairedKeyEncryptionFrame() { LOG(INFO) << "Attempts to sign authentication token with a previous private key."; std::optional> optional_signature = - certificate_manager_->SignWithPrivateCertificate( - visibility_history_.last_visibility, - PadPrefix(local_prefix_, raw_token_)); + certificate_manager_.SignWithPrivateCertificate( + visibility_history_.last_visibility, padded_token); if (optional_signature.has_value()) { encryption_frame->set_optional_signed_data(optional_signature->data(), @@ -300,7 +301,7 @@ PairedKeyVerificationRunner::VerifyAuthTokenHashWithPrivateCertificate( DeviceVisibility visibility, const nearby::sharing::service::proto::V1Frame& frame) { std::optional> hash = - certificate_manager_->HashAuthenticationTokenWithPrivateCertificate( + certificate_manager_.HashAuthenticationTokenWithPrivateCertificate( visibility, raw_token_); const std::string& frame_hash = @@ -328,8 +329,11 @@ PairedKeyVerificationRunner::VerifyPairedKeyEncryptionFrame( auto signed_data = frame.paired_key_encryption().signed_data(); std::vector data(signed_data.begin(), signed_data.end()); - if (!certificate_->VerifySignature(PadPrefix(remote_prefix_, raw_token_), - data)) { + std::vector padded_token = PadPrefix( + share_target_is_incoming_ ? kNearbyShareSenderVerificationPrefix + : kNearbyShareReceiverVerificationPrefix, + raw_token_); + if (!certificate_->VerifySignature(padded_token, data)) { if (!frame.paired_key_encryption().has_optional_signed_data()) { LOG(WARNING) << __func__ << ": Unable to verify remote paired key encryption frame. " @@ -341,8 +345,7 @@ PairedKeyVerificationRunner::VerifyPairedKeyEncryptionFrame( frame.paired_key_encryption().optional_signed_data(); std::vector optional_data(optional_signed_data.begin(), optional_signed_data.end()); - if (certificate_->VerifySignature(PadPrefix(remote_prefix_, raw_token_), - optional_data)) { + if (certificate_->VerifySignature(padded_token, optional_data)) { LOG(INFO) << "Successfully verified remote paired key encryption " "frame with the optional signed data."; } else { @@ -374,17 +377,13 @@ void PairedKeyVerificationRunner::ApplyResult( case PairedKeyVerificationResult::kUnable: verification_result_ = PairedKeyVerificationResult::kUnable; break; - case PairedKeyVerificationResult::kUnknown: - default: - verification_result_ = PairedKeyVerificationResult::kUnable; - break; } } bool PairedKeyVerificationRunner::IsVisibilityRecentlyUpdated() const { return visibility_history_.visibility != visibility_history_.last_visibility && - (clock_->Now() - visibility_history_.last_visibility_time < + (clock_.Now() - visibility_history_.last_visibility_time < kRelaxAfterSetVisibilityTimeout); } diff --git a/sharing/paired_key_verification_runner.h b/sharing/paired_key_verification_runner.h index 6bfc8026..372e656c 100644 --- a/sharing/paired_key_verification_runner.h +++ b/sharing/paired_key_verification_runner.h @@ -22,6 +22,7 @@ #include #include +#include "absl/base/nullability.h" #include "absl/functional/any_invocable.h" #include "absl/time/time.h" #include "internal/platform/clock.h" @@ -38,8 +39,6 @@ class PairedKeyVerificationRunner : public std::enable_shared_from_this { public: enum class PairedKeyVerificationResult { - // Default value for verification result. - kUnknown, // Succeeded with verification. kSuccess, // Failed to verify. @@ -55,7 +54,8 @@ class PairedKeyVerificationRunner }; PairedKeyVerificationRunner( - Clock* clock, location::nearby::proto::sharing::OSType os_type, + Clock* absl_nonnull clock, + location::nearby::proto::sharing::OSType os_type, bool share_target_is_incoming, const VisibilityHistory& visibility_history, const std::vector& token, @@ -63,8 +63,9 @@ class PairedKeyVerificationRunner void(const nearby::sharing::service::proto::Frame& frame)> frame_writer, const std::optional& certificate, - NearbyShareCertificateManager* certificate_manager, - IncomingFramesReader* frames_reader, absl::Duration read_frame_timeout); + NearbyShareCertificateManager* absl_nonnull certificate_manager, + IncomingFramesReader* absl_nonnull frames_reader, + absl::Duration read_frame_timeout); ~PairedKeyVerificationRunner(); @@ -95,22 +96,21 @@ class PairedKeyVerificationRunner // True if visibility has changed recently. bool IsVisibilityRecentlyUpdated() const; - nearby::Clock* const clock_; + nearby::Clock& clock_; + NearbyShareCertificateManager& certificate_manager_; + IncomingFramesReader& frames_reader_; + const bool share_target_is_incoming_; const location::nearby::proto::sharing::OSType os_type_; - VisibilityHistory visibility_history_; + const VisibilityHistory visibility_history_; + const std::optional certificate_; + const absl::Duration read_frame_timeout_; std::vector raw_token_; absl::AnyInvocable frame_writer_; - std::optional certificate_; - NearbyShareCertificateManager* certificate_manager_; - IncomingFramesReader* frames_reader_; - const absl::Duration read_frame_timeout_; std::function callback_; PairedKeyVerificationResult verification_result_; - char local_prefix_; - char remote_prefix_; }; } // namespace nearby::sharing diff --git a/sharing/paired_key_verification_runner_test.cc b/sharing/paired_key_verification_runner_test.cc index 32e0e72c..6b02462d 100644 --- a/sharing/paired_key_verification_runner_test.cc +++ b/sharing/paired_key_verification_runner_test.cc @@ -124,7 +124,6 @@ GenerateVisibilityHistory() { DeviceVisibility::DEVICE_VISIBILITY_ALL_CONTACTS, DeviceVisibility::DEVICE_VISIBILITY_SELF_SHARE, DeviceVisibility::DEVICE_VISIBILITY_EVERYONE, - DeviceVisibility::DEVICE_VISIBILITY_HIDDEN, }; std::list result; for (DeviceVisibility visibility : kValidVisibilities) { @@ -355,13 +354,32 @@ class PairedKeyVerificationRunnerTest : public testing::Test { }; TEST_F(PairedKeyVerificationRunnerTest, - NullCertificate_InvalidPairedKeyEncryptionFrame) { + Incoming_NullCertificate_InvalidPairedKeyEncryptionFrame) { // Empty key encryption frame fails the certificate verification. SetUpPairedKeyEncryptionFrame(ReturnFrameType::kEmpty); SetUpPairedKeyResultFrame(ReturnFrameType::kValid); RunVerification( - true, + /*is_incoming=*/true, + /*use_valid_public_certificate=*/false, + {.visibility = DeviceVisibility::DEVICE_VISIBILITY_ALL_CONTACTS, + .last_visibility = DeviceVisibility::DEVICE_VISIBILITY_ALL_CONTACTS, + .last_visibility_time = GetFakeClock()->Now()}, + /*expected_result=*/ + PairedKeyVerificationResult::kFail); + + ExpectPairedKeyEncryptionFrameSent(); + ExpectPairedKeyResultFrameSent(PairedKeyResultFrame::UNABLE); +} + +TEST_F(PairedKeyVerificationRunnerTest, + Outgoing_NullCertificate_InvalidPairedKeyEncryptionFrame) { + // Empty key encryption frame fails the certificate verification. + SetUpPairedKeyEncryptionFrame(ReturnFrameType::kEmpty); + SetUpPairedKeyResultFrame(ReturnFrameType::kValid); + + RunVerification( + /*is_incoming=*/false, /*use_valid_public_certificate=*/false, {.visibility = DeviceVisibility::DEVICE_VISIBILITY_ALL_CONTACTS, .last_visibility = DeviceVisibility::DEVICE_VISIBILITY_ALL_CONTACTS, @@ -373,6 +391,25 @@ TEST_F(PairedKeyVerificationRunnerTest, ExpectPairedKeyResultFrameSent(PairedKeyResultFrame::UNABLE); } +TEST_F(PairedKeyVerificationRunnerTest, + Incoming_HiddenDevice_FailsConnection) { + // Empty key encryption frame fails the certificate verification. + SetUpPairedKeyEncryptionFrame(ReturnFrameType::kEmpty); + SetUpPairedKeyResultFrame(ReturnFrameType::kValid); + + RunVerification( + /*is_incoming=*/true, + /*use_valid_public_certificate=*/false, + {.visibility = DeviceVisibility::DEVICE_VISIBILITY_HIDDEN, + .last_visibility = DeviceVisibility::DEVICE_VISIBILITY_ALL_CONTACTS, + .last_visibility_time = GetFakeClock()->Now()}, + /*expected_result=*/ + PairedKeyVerificationResult::kFail); + + ExpectPairedKeyEncryptionFrameSent(); + ExpectPairedKeyResultFrameSent(PairedKeyResultFrame::FAIL); +} + TEST_F(PairedKeyVerificationRunnerTest, ValidPairedKeyEncryptionFrame_ResultFrameTimedOut) { SetUpPairedKeyEncryptionFrame(ReturnFrameType::kValid); @@ -437,14 +474,40 @@ TEST_P(ParameterisedPairedKeyVerificationRunnerTest, PairedKeyResultFrame result_frame = std::get<1>(GetParam()); PairedKeyVerificationRunner::VisibilityHistory visibility_history = std::get<2>(GetParam()); + PairedKeyVerificationRunner::PairedKeyVerificationResult result = + params.result; + // If our visibility has no certificates, then downgrade expected result to + // kUnable if it is not expected to fail. + if ((visibility_history.visibility == + DeviceVisibility::DEVICE_VISIBILITY_EVERYONE) && + !(visibility_history.last_visibility != + DeviceVisibility::DEVICE_VISIBILITY_EVERYONE && + (params.encryption_frame_type == + PairedKeyVerificationRunnerTest::ReturnFrameType::kOptionalValid || + params.encryption_frame_type == + PairedKeyVerificationRunnerTest::ReturnFrameType::kValid))) { + if (result == + PairedKeyVerificationRunner::PairedKeyVerificationResult::kSuccess) { + result = + PairedKeyVerificationRunner::PairedKeyVerificationResult::kUnable; + } + } + if (params.is_incoming && + params.encryption_frame_type == + PairedKeyVerificationRunnerTest::ReturnFrameType::kEmpty && + visibility_history.visibility != + DeviceVisibility::DEVICE_VISIBILITY_EVERYONE) { + result = + PairedKeyVerificationRunner::PairedKeyVerificationResult::kFail; + } PairedKeyVerificationRunner::PairedKeyVerificationResult expected_result = - Merge(params.result, result_frame.status()); + Merge(result, result_frame.status()); LOG(ERROR) << "ValidEncryptionFrame_ValidResultFrame: " << "is_incoming=" << params.is_incoming << ", has_valid_cert=" << params.has_valid_certificate << ", encryption_frame_type=" << (int)params.encryption_frame_type - << ", result=" << (int)params.result + << ", result=" << (int)result << ", expected_result=" << (int)expected_result << ", result_frame=" << (int)result_frame.status() << ", visibility=" << (int)visibility_history.visibility @@ -463,22 +526,6 @@ TEST_P(ParameterisedPairedKeyVerificationRunnerTest, : OSType::UNKNOWN_OS_TYPE); } - // If our visibility has no certificates, then downgrade expected result to - // kUnable if it is not expected to fail. - if ((visibility_history.visibility == - DeviceVisibility::DEVICE_VISIBILITY_EVERYONE || - visibility_history.visibility == - DeviceVisibility::DEVICE_VISIBILITY_HIDDEN) && - (visibility_history.last_visibility == - DeviceVisibility::DEVICE_VISIBILITY_EVERYONE || - visibility_history.last_visibility == - DeviceVisibility::DEVICE_VISIBILITY_HIDDEN)) { - if (expected_result == - PairedKeyVerificationRunner::PairedKeyVerificationResult::kSuccess) { - expected_result = - PairedKeyVerificationRunner::PairedKeyVerificationResult::kUnable; - } - } visibility_history.last_visibility_time = GetFakeClock()->Now(); RunVerification( /*is_incoming=*/params.is_incoming, diff --git a/sharing/share_session.cc b/sharing/share_session.cc index 004b3779..3c7d1676 100644 --- a/sharing/share_session.cc +++ b/sharing/share_session.cc @@ -235,12 +235,6 @@ bool ShareSession::ProcessKeyVerificationResult( // share flag. self_share_ = false; break; - - case PairedKeyVerificationRunner::PairedKeyVerificationResult::kUnknown: - LOG(WARNING) << __func__ - << ": Unknown PairedKeyVerificationResult for target " - << share_target().id << ". Disconnecting."; - return false; } return true; } diff --git a/sharing/share_session_test.cc b/sharing/share_session_test.cc index fc6d9211..ddb3d3b5 100644 --- a/sharing/share_session_test.cc +++ b/sharing/share_session_test.cc @@ -389,20 +389,6 @@ TEST(ShareSessionTest, ProcessKeyVerificationResultNotSelfShareUnable) { EXPECT_FALSE(session.token().empty()); } -TEST(ShareSessionTest, ProcessKeyVerificationResultUnknown) { - ShareTarget share_target; - TestShareSession session(std::string(kEndpointId), share_target); - NearbyConnectionImpl connection(session.device_info()); - session.SetNearbyConnection(&connection); - session.SetTokenForTests("9876"); - - EXPECT_FALSE(session.ProcessKeyVerificationResult( - PairedKeyVerificationRunner::PairedKeyVerificationResult::kUnknown, - OSType::WINDOWS)); - EXPECT_EQ(session.os_type(), OSType::WINDOWS); - EXPECT_FALSE(session.token().empty()); -} - TEST(ShareSessionTest, AbortNotConnected) { ShareTarget share_target; TestShareSession session(std::string(kEndpointId), share_target);