From 607202c69d513a26e5ddf2bc2e9d2cf31986f577 Mon Sep 17 00:00:00 2001 From: Sally Qi Date: Wed, 5 Oct 2022 11:42:30 -0700 Subject: [PATCH 1/4] Mitigate the security vulnerability by sanitizing the transaction flags. - This is part of fix of commit Id9d9012d4ede9c8330f0ce1096bcb78e51b7c5df for backporting. Bug: 248031255 Test: test using displaytoken app manually on the phone, test shell screenrecord during using displaytoken; atest android.hardware.camera2.cts.FastBasicsTest Change-Id: Id9d9012d4ede9c8330f0ce1096bcb78e51b7c5df Merged-In: Id9d9012d4ede9c8330f0ce1096bcb78e51b7c5df (cherry picked from commit 3ea58dbc1d7a248160403f089b9998bf6694aae1) Merged-In: Id9d9012d4ede9c8330f0ce1096bcb78e51b7c5df --- libs/gui/LayerState.cpp | 21 +++++++++++++++++++++ libs/gui/include/gui/LayerState.h | 1 + services/surfaceflinger/SurfaceFlinger.cpp | 9 +++++---- services/surfaceflinger/SurfaceFlinger.h | 2 +- 4 files changed, 28 insertions(+), 5 deletions(-) diff --git a/libs/gui/LayerState.cpp b/libs/gui/LayerState.cpp index bf275a5900..9654a60310 100644 --- a/libs/gui/LayerState.cpp +++ b/libs/gui/LayerState.cpp @@ -391,6 +391,27 @@ void DisplayState::merge(const DisplayState& other) { } } +void DisplayState::sanitize(int32_t permissions) { + if (what & DisplayState::eLayerStackChanged) { + if (!(permissions & layer_state_t::Permission::ACCESS_SURFACE_FLINGER)) { + what &= ~DisplayState::eLayerStackChanged; + ALOGE("Stripped attempt to set eLayerStackChanged in sanitize"); + } + } + if (what & DisplayState::eDisplayProjectionChanged) { + if (!(permissions & layer_state_t::Permission::ACCESS_SURFACE_FLINGER)) { + what &= ~DisplayState::eDisplayProjectionChanged; + ALOGE("Stripped attempt to set eDisplayProjectionChanged in sanitize"); + } + } + if (what & DisplayState::eSurfaceChanged) { + if (!(permissions & layer_state_t::Permission::ACCESS_SURFACE_FLINGER)) { + what &= ~DisplayState::eSurfaceChanged; + ALOGE("Stripped attempt to set eSurfaceChanged in sanitize"); + } + } +} + void layer_state_t::sanitize(int32_t permissions) { // TODO: b/109894387 // diff --git a/libs/gui/include/gui/LayerState.h b/libs/gui/include/gui/LayerState.h index 2a8d30d2da..e5a029b1ff 100644 --- a/libs/gui/include/gui/LayerState.h +++ b/libs/gui/include/gui/LayerState.h @@ -278,6 +278,7 @@ struct DisplayState { DisplayState(); void merge(const DisplayState& other); + void sanitize(int32_t permissions); uint32_t what; sp token; diff --git a/services/surfaceflinger/SurfaceFlinger.cpp b/services/surfaceflinger/SurfaceFlinger.cpp index 1c5e0b043c..d1d6a4b793 100644 --- a/services/surfaceflinger/SurfaceFlinger.cpp +++ b/services/surfaceflinger/SurfaceFlinger.cpp @@ -3502,7 +3502,7 @@ void SurfaceFlinger::flushTransactionQueues() { // to prevent onHandleDestroyed from being called while the lock is held, // we must keep a copy of the transactions (specifically the composer // states) around outside the scope of the lock - std::vector transactions; + std::vector transactions; // Layer handles that have transactions with buffers that are ready to be applied. std::unordered_set, ISurfaceComposer::SpHash> bufferLayersReadyToPresent; { @@ -3566,7 +3566,7 @@ void SurfaceFlinger::flushTransactionQueues() { } // Now apply all transactions. - for (const auto& transaction : transactions) { + for (auto& transaction : transactions) { applyTransactionState(transaction.frameTimelineInfo, transaction.states, transaction.displays, transaction.flags, transaction.inputWindowCommands, transaction.desiredPresentTime, @@ -3786,7 +3786,7 @@ status_t SurfaceFlinger::setTransactionState( void SurfaceFlinger::applyTransactionState(const FrameTimelineInfo& frameTimelineInfo, const Vector& states, - const Vector& displays, uint32_t flags, + Vector& displays, uint32_t flags, const InputWindowCommands& inputWindowCommands, const int64_t desiredPresentTime, bool isAutoTimestamp, const client_cache_t& uncacheBuffer, @@ -3795,7 +3795,8 @@ void SurfaceFlinger::applyTransactionState(const FrameTimelineInfo& frameTimelin const std::vector& listenerCallbacks, int originPid, int originUid, uint64_t transactionId) { uint32_t transactionFlags = 0; - for (const DisplayState& display : displays) { + for (DisplayState& display : displays) { + display.sanitize(permissions); transactionFlags |= setDisplayStateLocked(display); } diff --git a/services/surfaceflinger/SurfaceFlinger.h b/services/surfaceflinger/SurfaceFlinger.h index bca194a51f..3d744601d3 100644 --- a/services/surfaceflinger/SurfaceFlinger.h +++ b/services/surfaceflinger/SurfaceFlinger.h @@ -832,7 +832,7 @@ class SurfaceFlinger : public BnSurfaceComposer, * Transactions */ void applyTransactionState(const FrameTimelineInfo& info, const Vector& state, - const Vector& displays, uint32_t flags, + Vector& displays, uint32_t flags, const InputWindowCommands& inputWindowCommands, const int64_t desiredPresentTime, bool isAutoTimestamp, const client_cache_t& uncacheBuffer, const int64_t postTime, From f6841b7b2c5efe01a4606f3aa089853bc41e4499 Mon Sep 17 00:00:00 2001 From: Devin Moore Date: Fri, 17 Feb 2023 17:12:46 +0000 Subject: [PATCH 2/4] Check for malformed Sensor Flattenable Test: libsensorserviceaidl_fuzzer with testcase from bug Bug: 269014004 Merged-In: I0e255c64243c38876fb657cbf942fc1613363216 Change-Id: I0e255c64243c38876fb657cbf942fc1613363216 (cherry picked from commit aeec1802f7befc8fbb18313ad3ac0969c3811870) Merged-In: I0e255c64243c38876fb657cbf942fc1613363216 (cherry picked from commit on googleplex-android-review.googlesource.com host: 9170e60bf6306e65f936e9ec723ccfb054c181dd) Merged-In: I0e255c64243c38876fb657cbf942fc1613363216 --- libs/sensor/Sensor.cpp | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/libs/sensor/Sensor.cpp b/libs/sensor/Sensor.cpp index 0a49008584..efb18c0e31 100644 --- a/libs/sensor/Sensor.cpp +++ b/libs/sensor/Sensor.cpp @@ -612,7 +612,13 @@ bool Sensor::unflattenString8(void const*& buffer, size_t& size, String8& output return false; } outputString8.setTo(static_cast(buffer), len); + + if (size < FlattenableUtils::align<4>(len)) { + ALOGE("Malformed Sensor String8 field. Should be in a 4-byte aligned buffer but is not."); + return false; + } FlattenableUtils::advance(buffer, size, FlattenableUtils::align<4>(len)); + return true; } From 590423b323c84d22ca7dac4ef8b5957216ace417 Mon Sep 17 00:00:00 2001 From: Devin Moore Date: Fri, 17 Feb 2023 19:35:25 +0000 Subject: [PATCH 3/4] Remove some new memory leaks from SensorManager After catching an error in Sensor::unflatten, there are memory leaks caught by the fuzzer in the same test case. Test: libsensorserviceaidl_fuzzer with testcase from bug Bug: 269014004 Merged-In: I509cceb41f56ca117d9475f6f6674244560fe582 Change-Id: I509cceb41f56ca117d9475f6f6674244560fe582 (cherry picked from commit c95fa0f0e7c7b73746ff850b85a79fc5f92b784e) Merged-In: I509cceb41f56ca117d9475f6f6674244560fe582 (cherry picked from commit on googleplex-android-review.googlesource.com host: e54698d02ce9a0058478c838545aa6ba52ca96c1) Merged-In: I509cceb41f56ca117d9475f6f6674244560fe582 --- libs/sensor/ISensorServer.cpp | 12 ++++++++++-- libs/sensor/SensorManager.cpp | 5 +++++ 2 files changed, 15 insertions(+), 2 deletions(-) diff --git a/libs/sensor/ISensorServer.cpp b/libs/sensor/ISensorServer.cpp index a6cacad374..93c95b98c5 100644 --- a/libs/sensor/ISensorServer.cpp +++ b/libs/sensor/ISensorServer.cpp @@ -66,7 +66,11 @@ class BpSensorServer : public BpInterface v.setCapacity(n); while (n) { n--; - reply.read(s); + if(reply.read(s) != OK) { + ALOGE("Failed to read reply from getSensorList"); + v.clear(); + break; + } v.add(s); } return v; @@ -84,7 +88,11 @@ class BpSensorServer : public BpInterface v.setCapacity(n); while (n) { n--; - reply.read(s); + if(reply.read(s) != OK) { + ALOGE("Failed to read reply from getDynamicSensorList"); + v.clear(); + break; + } v.add(s); } return v; diff --git a/libs/sensor/SensorManager.cpp b/libs/sensor/SensorManager.cpp index 62f4b4e3e2..625612c0c9 100644 --- a/libs/sensor/SensorManager.cpp +++ b/libs/sensor/SensorManager.cpp @@ -162,6 +162,11 @@ status_t SensorManager::assertStateLocked() { mSensors = mSensorServer->getSensorList(mOpPackageName); size_t count = mSensors.size(); + if (count == 0) { + ALOGE("Failed to get Sensor list"); + mSensorServer.clear(); + return UNKNOWN_ERROR; + } mSensorList = static_cast(malloc(count * sizeof(Sensor*))); LOG_ALWAYS_FATAL_IF(mSensorList == nullptr, "mSensorList NULL"); From 13006f99201c607bb1676453f37ca3fc6d236753 Mon Sep 17 00:00:00 2001 From: Anthony Stange Date: Tue, 21 Feb 2023 17:57:38 +0000 Subject: [PATCH 4/4] Add removeInstanceForPackageMethod to SensorManager In order to ensure that clients don't leak their sensor manager instance that we currently store in a static map, they need to be able to remove their instance. Otherwise, this instance is never removed from the list and will hang around until our SensorManage instance is destroyed. Bug: 269014004 Test: Run ./libsensorserviceaidl_fuzzer Change-Id: I52185f74ae8d28b379440235ca6f03c5089081f5 (cherry picked from commit 9532f7c682fdd4b1e6e553cd6f61fc0cf2555902) Merged-In: I52185f74ae8d28b379440235ca6f03c5089081f5 (cherry picked from commit on googleplex-android-review.googlesource.com host: 972675ed8101c0f0ed98688c9ffb071d9c0bb872) Merged-In: I52185f74ae8d28b379440235ca6f03c5089081f5 --- libs/sensor/SensorManager.cpp | 10 ++++++++++ libs/sensor/include/sensor/SensorManager.h | 1 + services/sensorservice/hidl/SensorManager.cpp | 3 +++ 3 files changed, 14 insertions(+) diff --git a/libs/sensor/SensorManager.cpp b/libs/sensor/SensorManager.cpp index 625612c0c9..cc5bcd8422 100644 --- a/libs/sensor/SensorManager.cpp +++ b/libs/sensor/SensorManager.cpp @@ -92,6 +92,16 @@ SensorManager& SensorManager::getInstanceForPackage(const String16& packageName) return *sensorManager; } +void SensorManager::removeInstanceForPackage(const String16& packageName) { + Mutex::Autolock _l(sLock); + auto iterator = sPackageInstances.find(packageName); + if (iterator != sPackageInstances.end()) { + SensorManager* sensorManager = iterator->second; + delete sensorManager; + sPackageInstances.erase(iterator); + } +} + SensorManager::SensorManager(const String16& opPackageName) : mSensorList(nullptr), mOpPackageName(opPackageName), mDirectConnectionHandle(1) { Mutex::Autolock _l(mLock); diff --git a/libs/sensor/include/sensor/SensorManager.h b/libs/sensor/include/sensor/SensorManager.h index 09ac7edf27..b2a9e1eb1b 100644 --- a/libs/sensor/include/sensor/SensorManager.h +++ b/libs/sensor/include/sensor/SensorManager.h @@ -54,6 +54,7 @@ class SensorManager : public ASensorManager { public: static SensorManager& getInstanceForPackage(const String16& packageName); + static void removeInstanceForPackage(const String16& packageName); ~SensorManager(); ssize_t getSensorList(Sensor const* const** list); diff --git a/services/sensorservice/hidl/SensorManager.cpp b/services/sensorservice/hidl/SensorManager.cpp index 938060063f..0a4e68412d 100644 --- a/services/sensorservice/hidl/SensorManager.cpp +++ b/services/sensorservice/hidl/SensorManager.cpp @@ -60,6 +60,9 @@ SensorManager::~SensorManager() { if (mPollThread.joinable()) { mPollThread.join(); } + + ::android::SensorManager::removeInstanceForPackage( + String16(ISensorManager::descriptor)); } // Methods from ::android::frameworks::sensorservice::V1_0::ISensorManager follow.