From 9fcd4886a3e1ccbc18acfadd84906400c9882eda Mon Sep 17 00:00:00 2001 From: Edwin Wong Date: Sat, 23 Jan 2021 17:25:14 -0800 Subject: [PATCH 1/2] [RESTRICT AUTOMERGE] Fix CryptoPlugin use after free vulnerability. The shared memory buffer used by srcPtr can be freed by another thread because it is not protected by a mutex. Subsequently, a use after free AIGABRT can occur in a race condition. SafetyNet logging is not added to avoid log spamming. The mutex lock is called to setup for decryption, which is called frequently. The crash was reproduced on the device before the fix. Verified the test passes after the fix. Test: sts sts-tradefed run sts-engbuild-no-spl-lock -m StsHostTestCases --test android.security.sts.Bug_176495665#testPocBug_176495665 Test: push to device with target_hwasan-userdebug build adb shell /data/local/tmp/Bug-176495665_sts64 Bug: 176495665 Bug: 176444161 Change-Id: I3ec33cd444183f40ee76bec4c88dec0dac859cd3 --- drm/1.0/default/Android.bp | 3 ++- drm/1.0/default/CryptoPlugin.cpp | 9 +++++++-- drm/1.0/default/CryptoPlugin.h | 21 +++++++++++++-------- 3 files changed, 22 insertions(+), 11 deletions(-) diff --git a/drm/1.0/default/Android.bp b/drm/1.0/default/Android.bp index ed6bcdeee6..1122e46a05 100644 --- a/drm/1.0/default/Android.bp +++ b/drm/1.0/default/Android.bp @@ -9,6 +9,7 @@ cc_library_static { "-Werror", "-Wextra", "-Wall", + "-Wthread-safety", ], shared_libs: [ "liblog", @@ -19,5 +20,5 @@ cc_library_static { export_header_lib_headers: [ "libutils_headers", ], - export_include_dirs : ["include"] + export_include_dirs: ["include"], } diff --git a/drm/1.0/default/CryptoPlugin.cpp b/drm/1.0/default/CryptoPlugin.cpp index 2db360765b..dd519e6b3a 100644 --- a/drm/1.0/default/CryptoPlugin.cpp +++ b/drm/1.0/default/CryptoPlugin.cpp @@ -53,6 +53,8 @@ namespace implementation { uint32_t bufferId) { sp hidlMemory = mapMemory(base); + std::unique_lock lock(mSharedBufferLock); + // allow mapMemory to return nullptr mSharedBufferMap[bufferId] = hidlMemory; return Void(); @@ -65,7 +67,7 @@ namespace implementation { const SharedBuffer& source, uint64_t offset, const DestinationBuffer& destination, decrypt_cb _hidl_cb) { - + std::unique_lock lock(mSharedBufferLock); if (mSharedBufferMap.find(source.bufferId) == mSharedBufferMap.end()) { _hidl_cb(Status::ERROR_DRM_CANNOT_HANDLE, 0, "source decrypt buffer base not set"); return Void(); @@ -79,7 +81,7 @@ namespace implementation { } } - android::CryptoPlugin::Mode legacyMode; + android::CryptoPlugin::Mode legacyMode = android::CryptoPlugin::kMode_Unencrypted; switch(mode) { case Mode::UNENCRYPTED: legacyMode = android::CryptoPlugin::kMode_Unencrypted; @@ -166,6 +168,9 @@ namespace implementation { _hidl_cb(Status::BAD_VALUE, 0, "invalid destination type"); return Void(); } + + // release mSharedBufferLock + lock.unlock(); ssize_t result = mLegacyPlugin->decrypt(secure, keyId.data(), iv.data(), legacyMode, legacyPattern, srcPtr, legacySubSamples.get(), subSamples.size(), destPtr, &detailMessage); diff --git a/drm/1.0/default/CryptoPlugin.h b/drm/1.0/default/CryptoPlugin.h index 11cc2aae47..0d091fae65 100644 --- a/drm/1.0/default/CryptoPlugin.h +++ b/drm/1.0/default/CryptoPlugin.h @@ -17,11 +17,14 @@ #ifndef ANDROID_HARDWARE_DRM_V1_0__CRYPTOPLUGIN_H #define ANDROID_HARDWARE_DRM_V1_0__CRYPTOPLUGIN_H -#include +#include #include +#include #include #include +#include + namespace android { namespace hardware { namespace drm { @@ -60,19 +63,21 @@ struct CryptoPlugin : public ICryptoPlugin { Return setSharedBufferBase(const ::android::hardware::hidl_memory& base, uint32_t bufferId) override; - Return decrypt(bool secure, const hidl_array& keyId, - const hidl_array& iv, Mode mode, const Pattern& pattern, - const hidl_vec& subSamples, const SharedBuffer& source, - uint64_t offset, const DestinationBuffer& destination, - decrypt_cb _hidl_cb) override; + Return decrypt( + bool secure, const hidl_array& keyId, const hidl_array& iv, + Mode mode, const Pattern& pattern, const hidl_vec& subSamples, + const SharedBuffer& source, uint64_t offset, const DestinationBuffer& destination, + decrypt_cb _hidl_cb) override NO_THREAD_SAFETY_ANALYSIS; // use unique_lock -private: + private: android::CryptoPlugin *mLegacyPlugin; - std::map > mSharedBufferMap; + std::map> mSharedBufferMap GUARDED_BY(mSharedBufferLock); CryptoPlugin() = delete; CryptoPlugin(const CryptoPlugin &) = delete; void operator=(const CryptoPlugin &) = delete; + + std::mutex mSharedBufferLock; }; } // namespace implementation From a4e76aab230a565dd0cef11e2e6e2d782b685327 Mon Sep 17 00:00:00 2001 From: Edwin Wong Date: Mon, 8 Mar 2021 18:46:42 -0800 Subject: [PATCH 2/2] [RESTRICT AUTOMERGE] Fix CryptoPlugin use after free vulnerability. The shared memory buffer used by srcPtr can be freed by another thread because it is not protected by a mutex. Subsequently, a use after free AIGABRT can occur in a race condition. SafetyNet logging is not added to avoid log spamming. The mutex lock is called to setup for decryption, which is called frequently. The crash was reproduced on the device before the fix. Verified the test passes after the fix. Test: sts sts-tradefed run sts-engbuild-no-spl-lock -m StsHostTestCases --test android.security.sts.Bug_176495665#testPocBug_176495665 Test: push to device with target_hwasan-userdebug build adb shell /data/local/tmp/Bug-176495665_sts64 Bug: 176495665 Bug: 176444161 Change-Id: I4c83c44873eef960b654f387a3574fcad49c41a9 --- drm/1.0/default/Android.bp | 3 ++- drm/1.0/default/CryptoPlugin.cpp | 10 ++++++++-- drm/1.0/default/CryptoPlugin.h | 21 +++++++++++++-------- 3 files changed, 23 insertions(+), 11 deletions(-) diff --git a/drm/1.0/default/Android.bp b/drm/1.0/default/Android.bp index ed6bcdeee6..1122e46a05 100644 --- a/drm/1.0/default/Android.bp +++ b/drm/1.0/default/Android.bp @@ -9,6 +9,7 @@ cc_library_static { "-Werror", "-Wextra", "-Wall", + "-Wthread-safety", ], shared_libs: [ "liblog", @@ -19,5 +20,5 @@ cc_library_static { export_header_lib_headers: [ "libutils_headers", ], - export_include_dirs : ["include"] + export_include_dirs: ["include"], } diff --git a/drm/1.0/default/CryptoPlugin.cpp b/drm/1.0/default/CryptoPlugin.cpp index e6d4e8447b..de8bbd599c 100644 --- a/drm/1.0/default/CryptoPlugin.cpp +++ b/drm/1.0/default/CryptoPlugin.cpp @@ -53,6 +53,8 @@ namespace implementation { uint32_t bufferId) { sp hidlMemory = mapMemory(base); + std::lock_guard shared_buffer_lock(mSharedBufferLock); + // allow mapMemory to return nullptr mSharedBufferMap[bufferId] = hidlMemory; return Void(); @@ -65,7 +67,7 @@ namespace implementation { const SharedBuffer& source, uint64_t offset, const DestinationBuffer& destination, decrypt_cb _hidl_cb) { - + std::unique_lock shared_buffer_lock(mSharedBufferLock); if (mSharedBufferMap.find(source.bufferId) == mSharedBufferMap.end()) { _hidl_cb(Status::ERROR_DRM_CANNOT_HANDLE, 0, "source decrypt buffer base not set"); return Void(); @@ -79,7 +81,7 @@ namespace implementation { } } - android::CryptoPlugin::Mode legacyMode; + android::CryptoPlugin::Mode legacyMode = android::CryptoPlugin::kMode_Unencrypted; switch(mode) { case Mode::UNENCRYPTED: legacyMode = android::CryptoPlugin::kMode_Unencrypted; @@ -170,6 +172,10 @@ namespace implementation { _hidl_cb(Status::BAD_VALUE, 0, "invalid destination type"); return Void(); } + + // release mSharedBufferLock + shared_buffer_lock.unlock(); + ssize_t result = mLegacyPlugin->decrypt(secure, keyId.data(), iv.data(), legacyMode, legacyPattern, srcPtr, legacySubSamples.get(), subSamples.size(), destPtr, &detailMessage); diff --git a/drm/1.0/default/CryptoPlugin.h b/drm/1.0/default/CryptoPlugin.h index 11cc2aae47..0d091fae65 100644 --- a/drm/1.0/default/CryptoPlugin.h +++ b/drm/1.0/default/CryptoPlugin.h @@ -17,11 +17,14 @@ #ifndef ANDROID_HARDWARE_DRM_V1_0__CRYPTOPLUGIN_H #define ANDROID_HARDWARE_DRM_V1_0__CRYPTOPLUGIN_H -#include +#include #include +#include #include #include +#include + namespace android { namespace hardware { namespace drm { @@ -60,19 +63,21 @@ struct CryptoPlugin : public ICryptoPlugin { Return setSharedBufferBase(const ::android::hardware::hidl_memory& base, uint32_t bufferId) override; - Return decrypt(bool secure, const hidl_array& keyId, - const hidl_array& iv, Mode mode, const Pattern& pattern, - const hidl_vec& subSamples, const SharedBuffer& source, - uint64_t offset, const DestinationBuffer& destination, - decrypt_cb _hidl_cb) override; + Return decrypt( + bool secure, const hidl_array& keyId, const hidl_array& iv, + Mode mode, const Pattern& pattern, const hidl_vec& subSamples, + const SharedBuffer& source, uint64_t offset, const DestinationBuffer& destination, + decrypt_cb _hidl_cb) override NO_THREAD_SAFETY_ANALYSIS; // use unique_lock -private: + private: android::CryptoPlugin *mLegacyPlugin; - std::map > mSharedBufferMap; + std::map> mSharedBufferMap GUARDED_BY(mSharedBufferLock); CryptoPlugin() = delete; CryptoPlugin(const CryptoPlugin &) = delete; void operator=(const CryptoPlugin &) = delete; + + std::mutex mSharedBufferLock; }; } // namespace implementation