From 989975ec5a2a9284d13b6b5e76d34bc7c214673e Mon Sep 17 00:00:00 2001 From: Edwin Wong Date: Mon, 8 Mar 2021 18:46:42 -0800 Subject: [PATCH] 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 | 15 ++++++++------- drm/1.0/default/CryptoPlugin.cpp | 10 ++++++++-- drm/1.0/default/CryptoPlugin.h | 21 +++++++++++++-------- 3 files changed, 29 insertions(+), 17 deletions(-) diff --git a/drm/1.0/default/Android.bp b/drm/1.0/default/Android.bp index af1c076e0d..cbdab4ffe0 100644 --- a/drm/1.0/default/Android.bp +++ b/drm/1.0/default/Android.bp @@ -32,6 +32,7 @@ cc_library_static { "-Werror", "-Wextra", "-Wall", + "-Wthread-safety", ], shared_libs: [ "liblog", @@ -42,7 +43,7 @@ cc_library_static { export_header_lib_headers: [ "libutils_headers", ], - export_include_dirs : ["include"] + export_include_dirs: ["include"], } soong_config_module_type { @@ -59,8 +60,8 @@ android_hardware_drm_1_0_multilib { soong_config_variables: { TARGET_ENABLE_MEDIADRM_64: { compile_multilib: "both", - } - } + }, + }, } android_hardware_drm_1_0_multilib { @@ -69,8 +70,8 @@ android_hardware_drm_1_0_multilib { soong_config_variables: { TARGET_ENABLE_MEDIADRM_64: { compile_multilib: "first", - } - } + }, + }, } cc_defaults { @@ -98,7 +99,7 @@ cc_binary { name: "android.hardware.drm@1.0-service", defaults: [ "android.hardware.drm@1.0-multilib-exe", - "android.hardware.drm@1.0-service-defaults" + "android.hardware.drm@1.0-service-defaults", ], init_rc: ["android.hardware.drm@1.0-service.rc"], srcs: ["service.cpp"], @@ -110,7 +111,7 @@ cc_binary { name: "android.hardware.drm@1.0-service-lazy", defaults: [ "android.hardware.drm@1.0-multilib-exe", - "android.hardware.drm@1.0-service-defaults" + "android.hardware.drm@1.0-service-defaults", ], overrides: ["android.hardware.drm@1.0-service"], init_rc: ["android.hardware.drm@1.0-service-lazy.rc"], 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