From ee110cfef1dd36063b02b4a760fd0af41c19a98e Mon Sep 17 00:00:00 2001 From: Hansong Zhang Date: Thu, 1 Oct 2020 12:09:57 -0700 Subject: [PATCH] GD: Enforce link encryption in L2cap Tag: #gd-refactor Bug: 141555841 Test: cert/run --host Change-Id: I2cbeeab04ba23c6575f9042fd2785c2c427ce200 --- gd/l2cap/classic/internal/link.cc | 11 +++++++++++ gd/l2cap/classic/internal/link.h | 7 +++++++ gd/l2cap/classic/internal/link_test.cc | 2 +- gd/l2cap/classic/internal/signalling_manager.cc | 16 ++++++++++++++-- gd/l2cap/classic/internal/signalling_manager.h | 7 ++++++- gd/security/channel/security_manager_channel.cc | 2 -- gd/security/channel/security_manager_channel.h | 1 - gd/security/channel/security_manager_channel_unittest.cc | 2 -- gd/security/internal/security_manager_impl.cc | 12 ++---------- gd/security/internal/security_manager_impl.h | 6 ------ gd/security/pairing/classic_pairing_handler_unittest.cc | 2 -- gd/security/record/security_record.h | 9 --------- gd/security/record/security_record_storage.cc | 2 -- 13 files changed, 41 insertions(+), 38 deletions(-) diff --git a/gd/l2cap/classic/internal/link.cc b/gd/l2cap/classic/internal/link.cc index 9a4a6cc3e..b66ea6d68 100644 --- a/gd/l2cap/classic/internal/link.cc +++ b/gd/l2cap/classic/internal/link.cc @@ -307,6 +307,13 @@ void Link::OnAuthenticationComplete() { void Link::OnEncryptionChange(hci::EncryptionEnabled enabled) { encryption_enabled_ = enabled; link_manager_->OnEncryptionChange(GetDevice().GetAddress(), enabled); + for (auto& listener : encryption_change_listener_) { + signalling_manager_.on_security_result_for_outgoing( + ClassicSignallingManager::SecurityEnforcementType::ENCRYPTION, + listener.psm, + listener.cid, + enabled != hci::EncryptionEnabled::OFF); + } } void Link::OnChangeConnectionLinkKeyComplete() { @@ -398,6 +405,10 @@ void Link::OnReadRemoteVersionInformationComplete( sub_version); } +void Link::AddEncryptionChangeListener(EncryptionChangeListener listener) { + encryption_change_listener_.push_back(listener); +} + } // namespace internal } // namespace classic } // namespace l2cap diff --git a/gd/l2cap/classic/internal/link.h b/gd/l2cap/classic/internal/link.h index 9312bbe86..ca5df99dc 100644 --- a/gd/l2cap/classic/internal/link.h +++ b/gd/l2cap/classic/internal/link.h @@ -176,6 +176,12 @@ class Link : public l2cap::internal::ILink, public hci::acl_manager::ConnectionM void OnDisconnection(hci::ErrorCode reason) override; void OnReadRemoteVersionInformationComplete(uint8_t lmp_version, uint16_t manufacturer_name, uint16_t sub_version); + struct EncryptionChangeListener { + Cid cid; + Psm psm; + }; + void AddEncryptionChangeListener(EncryptionChangeListener); + private: friend class DumpsysHelper; void connect_to_pending_dynamic_channels(); @@ -203,6 +209,7 @@ class Link : public l2cap::internal::ILink, public hci::acl_manager::ConnectionM std::list pending_outgoing_configuration_request_list_; bool used_by_security_module_ = false; bool has_requested_authentication_ = false; + std::list encryption_change_listener_; DISALLOW_COPY_AND_ASSIGN(Link); }; diff --git a/gd/l2cap/classic/internal/link_test.cc b/gd/l2cap/classic/internal/link_test.cc index 6e09151fa..89d6dc75d 100644 --- a/gd/l2cap/classic/internal/link_test.cc +++ b/gd/l2cap/classic/internal/link_test.cc @@ -130,7 +130,7 @@ TEST_F(L2capClassicLinkTest, pending_channels_get_notified_on_acl_disconnect) { EXPECT_CALL(mock_classic_dynamic_channel_service_manager_, GetSecurityEnforcementInterface()) .WillOnce(::testing::Return(&security_module_impl_)); EXPECT_CALL(mock_classic_dynamic_channel_service_manager_, GetService(::testing::_)) - .WillOnce(::testing::Return(&service)); + .WillRepeatedly(::testing::Return(&service)); link_->SendConnectionRequest(kPsm, kCid, std::move(pending_dynamic_channel_connection)); link_->OnAclDisconnected(hci::ErrorCode::UNKNOWN_HCI_COMMAND); diff --git a/gd/l2cap/classic/internal/signalling_manager.cc b/gd/l2cap/classic/internal/signalling_manager.cc index 386c84e4d..cc7d375f5 100644 --- a/gd/l2cap/classic/internal/signalling_manager.cc +++ b/gd/l2cap/classic/internal/signalling_manager.cc @@ -92,10 +92,16 @@ void ClassicSignallingManager::SendConnectionRequest(Psm psm, Cid local_cid) { dynamic_service_manager_->GetSecurityEnforcementInterface()->Enforce( link_->GetDevice(), dynamic_service_manager_->GetService(psm)->GetSecurityPolicy(), - handler_->BindOnceOn(this, &ClassicSignallingManager::on_security_result_for_outgoing, psm, local_cid)); + handler_->BindOnceOn( + this, + &ClassicSignallingManager::on_security_result_for_outgoing, + SecurityEnforcementType::LINK_KEY, + psm, + local_cid)); } -void ClassicSignallingManager::on_security_result_for_outgoing(Psm psm, Cid local_cid, bool result) { +void ClassicSignallingManager::on_security_result_for_outgoing( + SecurityEnforcementType type, Psm psm, Cid local_cid, bool result) { if (enqueue_buffer_.get() == nullptr) { LOG_ERROR("Got security result callback after deletion"); return; @@ -110,6 +116,12 @@ void ClassicSignallingManager::on_security_result_for_outgoing(Psm psm, Cid loca link_->OnOutgoingConnectionRequestFail(local_cid, connection_result); return; } + if (type == SecurityEnforcementType::LINK_KEY && !link_->IsAuthenticated() && + dynamic_service_manager_->GetService(psm)->GetSecurityPolicy() != + SecurityPolicy::_SDP_ONLY_NO_SECURITY_WHATSOEVER_PLAINTEXT_TRANSPORT_OK) { + link_->Encrypt(); + return; + } PendingCommand pending_command = {next_signal_id_, CommandCode::CONNECTION_REQUEST, psm, local_cid, {}, {}, {}}; next_signal_id_++; diff --git a/gd/l2cap/classic/internal/signalling_manager.h b/gd/l2cap/classic/internal/signalling_manager.h index e12f89d85..76637ae56 100644 --- a/gd/l2cap/classic/internal/signalling_manager.h +++ b/gd/l2cap/classic/internal/signalling_manager.h @@ -99,6 +99,12 @@ class ClassicSignallingManager { void OnInformationResponse(SignalId signal_id, const InformationResponseView& response); + enum class SecurityEnforcementType { + LINK_KEY, + ENCRYPTION, + }; + void on_security_result_for_outgoing(SecurityEnforcementType type, Psm psm, Cid local_cid, bool result); + private: void on_incoming_packet(); void handle_one_command(ControlView control_view); @@ -112,7 +118,6 @@ class ClassicSignallingManager { void send_configuration_request(Cid remote_cid, std::vector> config); void on_security_result_for_incoming(Psm psm, Cid remote_cid, SignalId signal_id, bool result); - void on_security_result_for_outgoing(Psm psm, Cid local_cid, bool result); os::Handler* handler_; Link* link_; diff --git a/gd/security/channel/security_manager_channel.cc b/gd/security/channel/security_manager_channel.cc index 48580ab0e..babf18263 100644 --- a/gd/security/channel/security_manager_channel.cc +++ b/gd/security/channel/security_manager_channel.cc @@ -119,8 +119,6 @@ void SecurityManagerChannel::OnAuthenticationComplete(hci::Address remote) { } void SecurityManagerChannel::OnEncryptionChange(hci::Address remote, bool encrypted) { - ASSERT_LOG(listener_ != nullptr, "No listener set!"); - listener_->OnEncryptionChange(remote, encrypted); } } // namespace channel diff --git a/gd/security/channel/security_manager_channel.h b/gd/security/channel/security_manager_channel.h index 24241dfd7..0bd2b76e6 100644 --- a/gd/security/channel/security_manager_channel.h +++ b/gd/security/channel/security_manager_channel.h @@ -40,7 +40,6 @@ class ISecurityManagerChannelListener { virtual ~ISecurityManagerChannelListener() = default; virtual void OnHciEventReceived(hci::EventPacketView packet) = 0; virtual void OnConnectionClosed(hci::Address) = 0; - virtual void OnEncryptionChange(hci::Address, bool encrypted) = 0; }; /** diff --git a/gd/security/channel/security_manager_channel_unittest.cc b/gd/security/channel/security_manager_channel_unittest.cc index cebee87c6..ba5452031 100644 --- a/gd/security/channel/security_manager_channel_unittest.cc +++ b/gd/security/channel/security_manager_channel_unittest.cc @@ -212,8 +212,6 @@ class SecurityManagerChannelCallback : public ISecurityManagerChannelListener { void OnConnectionClosed(hci::Address address) override { LOG_INFO("Called"); } - - void OnEncryptionChange(hci::Address address, bool encrypted) override {} }; class SecurityManagerChannelTest : public ::testing::Test { diff --git a/gd/security/internal/security_manager_impl.cc b/gd/security/internal/security_manager_impl.cc index 05f91237d..3ed93bcf2 100644 --- a/gd/security/internal/security_manager_impl.cc +++ b/gd/security/internal/security_manager_impl.cc @@ -331,13 +331,6 @@ void SecurityManagerImpl::OnConnectionClosed(hci::Address address) { } } -void SecurityManagerImpl::OnEncryptionChange(hci::Address address, bool encrypted) { - auto remote = hci::AddressWithType(address, hci::AddressType::PUBLIC_DEVICE_ADDRESS); - auto record = security_database_.FindOrCreate(remote); - record->SetIsEncrypted(encrypted); - UpdateLinkSecurityCondition(remote); -} - void SecurityManagerImpl::OnHciLeEvent(hci::LeMetaEventView event) { hci::SubeventCode code = event.GetSubeventCode(); @@ -829,10 +822,9 @@ bool SecurityManagerImpl::IsSecurityRequirementSatisfied( switch (policy) { case l2cap::classic::SecurityPolicy::BEST: case l2cap::classic::SecurityPolicy::AUTHENTICATED_ENCRYPTED_TRANSPORT: - // TODO(b/165671060): Use IO cap to check whether we can waive authenticated LK requirement - return (!record->RequiresMitmProtection() || record->IsAuthenticated()) && record->IsEncrypted(); + return (record->IsPaired() && record->IsAuthenticated()); case l2cap::classic::SecurityPolicy::ENCRYPTED_TRANSPORT: - return record->IsEncrypted(); + return record->IsPaired(); case l2cap::classic::SecurityPolicy::_SDP_ONLY_NO_SECURITY_WHATSOEVER_PLAINTEXT_TRANSPORT_OK: return true; default: diff --git a/gd/security/internal/security_manager_impl.h b/gd/security/internal/security_manager_impl.h index 97ee93478..9351ca3c4 100644 --- a/gd/security/internal/security_manager_impl.h +++ b/gd/security/internal/security_manager_impl.h @@ -156,12 +156,6 @@ class SecurityManagerImpl : public channel::ISecurityManagerChannelListener, pub void OnConnectionClosed(hci::Address address) override; /** - * When link encryption status change, we need to update the device record (temporary). - * @param encrypted - */ - void OnEncryptionChange(hci::Address remote, bool encrypted) override; - - /** * Pairing handler has finished or cancelled * * @param address address for pairing handler diff --git a/gd/security/pairing/classic_pairing_handler_unittest.cc b/gd/security/pairing/classic_pairing_handler_unittest.cc index fcfb94d36..9fab86a16 100644 --- a/gd/security/pairing/classic_pairing_handler_unittest.cc +++ b/gd/security/pairing/classic_pairing_handler_unittest.cc @@ -125,8 +125,6 @@ class SecurityManagerChannelCallback : public ISecurityManagerChannelListener { LOG_INFO("Called"); } - void OnEncryptionChange(hci::Address address, bool encrypted) override {} - private: pairing::ClassicPairingHandler* pairing_handler_ = nullptr; }; diff --git a/gd/security/record/security_record.h b/gd/security/record/security_record.h index 0f476d0ff..3d302c1a0 100644 --- a/gd/security/record/security_record.h +++ b/gd/security/record/security_record.h @@ -96,14 +96,6 @@ class SecurityRecord { return this->is_encryption_required_; } - void SetIsEncrypted(bool is_encrypted) { - this->is_encrypted_ = is_encrypted; - } - - bool IsEncrypted() { - return this->is_encrypted_; - } - bool IsClassicLinkKeyValid() const { return !std::all_of(link_key_.begin(), link_key_.end(), [](uint8_t b) { return b == 0; }); } @@ -126,7 +118,6 @@ class SecurityRecord { bool is_authenticated_ = false; bool requires_mitm_protection_ = false; bool is_encryption_required_ = false; - bool is_encrypted_ = false; public: /* First address we have ever seen this device with, that we used to create bond */ diff --git a/gd/security/record/security_record_storage.cc b/gd/security/record/security_record_storage.cc index 238b6a1e5..d9bb5c128 100644 --- a/gd/security/record/security_record_storage.cc +++ b/gd/security/record/security_record_storage.cc @@ -176,8 +176,6 @@ void SecurityRecordStorage::LoadSecurityRecords(std::setsecurity_level = peer_signature_resolving_keys->data()[20]; } } - - record->SetIsEncrypted(false); record->SetIsEncryptionRequired(device.GetIsEncryptionRequired() == 1 ? true : false); record->SetAuthenticated(device.GetIsAuthenticated() == 1 ? true : false); record->SetRequiresMitmProtection(device.GetRequiresMitmProtection() == 1 ? true : false); -- 2.11.0