From faa556c46b5082fc998638714b545c5e371b721f Mon Sep 17 00:00:00 2001 From: 18392170496 Date: Fri, 1 Sep 2023 10:33:33 +0800 Subject: [PATCH] fixed a security issue in the use of RPC mutex. Signed-off-by: 18392170496 --- .../src/mock/include/dbinder_base_invoker.h | 53 ++++++++++++------- .../mock/source/dbinder_databus_invoker.cpp | 1 + .../src/socket/dbinder_remote_listener.cpp | 32 ++++------- 3 files changed, 45 insertions(+), 41 deletions(-) diff --git a/ipc/native/src/mock/include/dbinder_base_invoker.h b/ipc/native/src/mock/include/dbinder_base_invoker.h index 9b872388..8995dafc 100644 --- a/ipc/native/src/mock/include/dbinder_base_invoker.h +++ b/ipc/native/src/mock/include/dbinder_base_invoker.h @@ -96,6 +96,7 @@ public: virtual int CheckAndSetCallerInfo(uint32_t listenFd, uint64_t stubIndex) = 0; virtual int OnSendRawData(std::shared_ptr session, const void *data, size_t size) = 0; bool CheckTransactionData(const dbinder_transaction_data *tr) const; + std::mutex &GetObjectMutex(); private: uint32_t TranslateBinderType(flat_binder_object *binderObject, char *sessionOffset, std::shared_ptr session); @@ -125,6 +126,9 @@ private: std::shared_ptr MakeThreadProcessInfo(uint32_t handle, const char *buffer, uint32_t size); std::shared_ptr MakeThreadMessageInfo(uint32_t handle); uint32_t MakeRemoteHandle(std::shared_ptr session); + +private: + std::mutex objectMutex_; }; template @@ -901,27 +905,32 @@ template void DBinderBaseInvoker::ProcessTransaction(dbinder_transa const uint32_t flags = tr->flags; uint64_t senderSeqNumber = tr->seqNumber; - auto *stub = current->QueryStubByIndex(tr->cookie); - if (stub == nullptr) { - ZLOGE(LOG_LABEL, "stubIndex is invalid"); - return; - } - if (!IRemoteObjectTranslateWhenRcv(reinterpret_cast(tr->buffer), tr->buffer_size, data, - listenFd, nullptr)) { - ZLOGE(LOG_LABEL, "translate object failed"); - return; + int error = ERR_NONE; + { + std::lock_guard lockGuard(objectMutex_); + auto *stub = current->QueryStubByIndex(tr->cookie); + if (stub == nullptr) { + ZLOGE(LOG_LABEL, "stubIndex is invalid"); + return; + } + if (!IRemoteObjectTranslateWhenRcv(reinterpret_cast(tr->buffer), tr->buffer_size, data, + listenFd, nullptr)) { + ZLOGE(LOG_LABEL, "translate object failed"); + return; + } + + auto *stubObject = reinterpret_cast(stub); + MessageOption option; + option.SetFlags(flags); + // cannot use stub any more after SendRequest because this cmd may be + // dbinder dec ref and thus stub will be destroyed + int error = stubObject->SendRequest(tr->code, data, reply, option); + if (error != ERR_NONE) { + ZLOGE(LOG_LABEL, "stub sendrequest failed, cmd: %{public}u, error: %{public}d", tr->code, error); + // can not return; + } } - auto *stubObject = reinterpret_cast(stub); - MessageOption option; - option.SetFlags(flags); - // cannot use stub any more after SendRequest because this cmd may be - // dbinder dec ref and thus stub will be destroyed - int error = stubObject->SendRequest(tr->code, data, reply, option); - if (error != ERR_NONE) { - ZLOGE(LOG_LABEL, "stub sendrequest failed, cmd: %{public}u, error: %{public}d", tr->code, error); - // can not return; - } if (data.GetRawData() != nullptr) { ZLOGE(LOG_LABEL, "delete raw data in process skeleton, fd: %{public}u", listenFd); current->DetachRawData(listenFd); @@ -1058,5 +1067,11 @@ template bool DBinderBaseInvoker::CheckTransactionData(const dbinde return true; } + +template std::mutex &DBinderBaseInvoker::GetObjectMutex() +{ + return objectMutex_; +} + } // namespace OHOS #endif // OHOS_IPC_DBINDER_BASE_INVOKER_H diff --git a/ipc/native/src/mock/source/dbinder_databus_invoker.cpp b/ipc/native/src/mock/source/dbinder_databus_invoker.cpp index 3eaeeb1d..f4e84a49 100644 --- a/ipc/native/src/mock/source/dbinder_databus_invoker.cpp +++ b/ipc/native/src/mock/source/dbinder_databus_invoker.cpp @@ -547,6 +547,7 @@ bool DBinderDatabusInvoker::OnDatabusSessionServerSideClosed(std::shared_ptr stubIndexs = current->DetachAppInfoToStubIndex(session->GetPeerPid(), session->GetPeerUid(), tokenId, session->GetPeerDeviceId(), IPCProcessSkeleton::ConvertChannelID2Int(channelId)); + std::lock_guard lockGuard(GetObjectMutex()); for (auto it = stubIndexs.begin(); it != stubIndexs.end(); it++) { // note that we canont remove mapping from stub to index here because other session may still be used IRemoteObject *stub = current->QueryStubByIndex(*it); diff --git a/services/dbinder/dbinder_service/src/socket/dbinder_remote_listener.cpp b/services/dbinder/dbinder_service/src/socket/dbinder_remote_listener.cpp index c2616f99..1501352c 100644 --- a/services/dbinder/dbinder_service/src/socket/dbinder_remote_listener.cpp +++ b/services/dbinder/dbinder_service/src/socket/dbinder_remote_listener.cpp @@ -59,7 +59,6 @@ bool DBinderRemoteListener::StartListener(std::shared_ptr bool DBinderRemoteListener::StopListener() { - ClearDeviceLock(); std::lock_guard lockGuard(busManagerMutex_); if (softbusManager_ == nullptr) { DBINDER_LOGE(LOG_LABEL, "softbus manager is null"); @@ -154,7 +153,6 @@ bool DBinderRemoteListener::SendDataReply(const std::string &deviceId, const str bool DBinderRemoteListener::CloseDatabusSession(const std::string &deviceId) { - EraseDeviceLock(deviceId); std::lock_guard lockGuard(busManagerMutex_); if (softbusManager_ == nullptr) { DBINDER_LOGE(LOG_LABEL, "softbus manager is null"); @@ -175,23 +173,16 @@ bool DBinderRemoteListener::CloseDatabusSession(const std::string &deviceId) std::shared_ptr DBinderRemoteListener::OpenSoftbusSession(const std::string &peerDeviceId) { - { - std::lock_guard lockGuard(busManagerMutex_); - if (softbusManager_ == nullptr) { - DBINDER_LOGE(LOG_LABEL, "softbus manager is null"); - return nullptr; - } - auto it = clientSessionMap_.find(peerDeviceId); - if (it != clientSessionMap_.end()) { - return it->second; - } - } - std::shared_ptr lockInfo = QueryOrNewDeviceLock(peerDeviceId); - if (lockInfo == nullptr) { + std::lock_guard lockGuard(busManagerMutex_); + if (softbusManager_ == nullptr) { + DBINDER_LOGE(LOG_LABEL, "softbus manager is null"); return nullptr; } - // OpenSession is not thread-safe - std::lock_guard lock_unique(lockInfo->mutex); + auto it = clientSessionMap_.find(peerDeviceId); + if (it != clientSessionMap_.end()) { + return it->second; + } + std::shared_ptr session = softbusManager_->OpenSession(OWN_SESSION_NAME, PEER_SESSION_NAME, peerDeviceId, std::string(""), Session::TYPE_BYTES); if (session == nullptr) { @@ -199,10 +190,8 @@ std::shared_ptr DBinderRemoteListener::OpenSoftbusSession(const std::st DBinderService::ConvertToSecureDeviceID(peerDeviceId).c_str()); return nullptr; } - { - std::lock_guard lockGuard(busManagerMutex_); - clientSessionMap_.insert(std::pair>(peerDeviceId, session)); - } + + clientSessionMap_.insert(std::pair>(peerDeviceId, session)); return session; } @@ -249,7 +238,6 @@ void DBinderRemoteListener::OnSessionClosed(std::shared_ptr session) } } } else { - EraseDeviceLock(session->GetPeerDeviceId()); std::lock_guard lockGuard(busManagerMutex_); for (auto it = clientSessionMap_.begin(); it != clientSessionMap_.end(); it++) { if (it->second->GetChannelId() == session->GetChannelId()) {