Refactor bufferhub token to be more secure
Now BufferHubService will use HMAC hashing from openssl library to
enhance the security of the token.
The token handle will use its first int to store token id, then the HMAC
hash of the token id, so that BufferHubService could store token in
mTokenMap using token id as the key to improve looking up performance.
Test: VtsHalBufferHubV1_0TargetTest
Change-Id: Ica9639bc1504a477c6a1b1f703399bb07b3a2d2c
Fix: 118180214
diff --git a/services/bufferhub/Android.bp b/services/bufferhub/Android.bp
index 72d210c..a4b81fe 100644
--- a/services/bufferhub/Android.bp
+++ b/services/bufferhub/Android.bp
@@ -34,6 +34,7 @@
],
shared_libs: [
"android.frameworks.bufferhub@1.0",
+ "libcrypto",
"libcutils",
"libhidlbase",
"libhidltransport",
@@ -60,6 +61,7 @@
shared_libs: [
"android.frameworks.bufferhub@1.0",
"libbufferhubservice",
+ "libcrypto",
"libcutils",
"libhidltransport",
"libhwbinder",
diff --git a/services/bufferhub/BufferHubService.cpp b/services/bufferhub/BufferHubService.cpp
index 54796a2..43235f2 100644
--- a/services/bufferhub/BufferHubService.cpp
+++ b/services/bufferhub/BufferHubService.cpp
@@ -14,13 +14,16 @@
* limitations under the License.
*/
+#include <array>
#include <iomanip>
+#include <random>
#include <sstream>
#include <android/hardware_buffer.h>
#include <bufferhub/BufferHubService.h>
#include <cutils/native_handle.h>
#include <log/log.h>
+#include <openssl/hmac.h>
#include <system/graphics-base.h>
using ::android::BufferHubDefs::MetadataHeader;
@@ -32,6 +35,13 @@
namespace V1_0 {
namespace implementation {
+BufferHubService::BufferHubService() {
+ std::mt19937_64 randomEngine;
+ randomEngine.seed(time(nullptr));
+
+ mKey = randomEngine();
+}
+
Return<void> BufferHubService::allocateBuffer(const HardwareBufferDescription& description,
const uint32_t userMetadataSize,
allocateBuffer_cb _hidl_cb) {
@@ -66,27 +76,49 @@
Return<void> BufferHubService::importBuffer(const hidl_handle& tokenHandle,
importBuffer_cb _hidl_cb) {
- if (!tokenHandle.getNativeHandle() || tokenHandle->numFds != 0 || tokenHandle->numInts != 1) {
+ if (!tokenHandle.getNativeHandle() || tokenHandle->numFds != 0 || tokenHandle->numInts <= 1) {
// nullptr handle or wrong format
_hidl_cb(/*status=*/BufferHubStatus::INVALID_TOKEN, /*bufferClient=*/nullptr,
/*bufferTraits=*/{});
return Void();
}
- uint32_t token = tokenHandle->data[0];
+ int tokenId = tokenHandle->data[0];
wp<BufferClient> originClientWp;
{
- std::lock_guard<std::mutex> lock(mTokenMapMutex);
- auto iter = mTokenMap.find(token);
+ std::lock_guard<std::mutex> lock(mTokenMutex);
+ auto iter = mTokenMap.find(tokenId);
if (iter == mTokenMap.end()) {
- // Invalid token
+ // Token Id not exist
+ ALOGD("%s: token #%d not found.", __FUNCTION__, tokenId);
_hidl_cb(/*status=*/BufferHubStatus::INVALID_TOKEN, /*bufferClient=*/nullptr,
/*bufferTraits=*/{});
return Void();
}
- originClientWp = iter->second;
+ const std::vector<uint8_t>& tokenHMAC = iter->second.first;
+
+ int numIntsForHMAC = (int)ceil(tokenHMAC.size() * sizeof(uint8_t) / (double)sizeof(int));
+ if (tokenHandle->numInts - 1 != numIntsForHMAC) {
+ // HMAC size not match
+ ALOGD("%s: token #%d HMAC size not match. Expected: %d Actual: %d", __FUNCTION__,
+ tokenId, numIntsForHMAC, tokenHandle->numInts - 1);
+ _hidl_cb(/*status=*/BufferHubStatus::INVALID_TOKEN, /*bufferClient=*/nullptr,
+ /*bufferTraits=*/{});
+ return Void();
+ }
+
+ size_t hmacSize = tokenHMAC.size() * sizeof(uint8_t);
+ if (memcmp(tokenHMAC.data(), &tokenHandle->data[1], hmacSize) != 0) {
+ // HMAC not match
+ ALOGD("%s: token #%d HMAC not match.", __FUNCTION__, tokenId);
+ _hidl_cb(/*status=*/BufferHubStatus::INVALID_TOKEN, /*bufferClient=*/nullptr,
+ /*bufferTraits=*/{});
+ return Void();
+ }
+
+ originClientWp = iter->second.second;
mTokenMap.erase(iter);
}
@@ -225,9 +257,9 @@
// Map from bufferId to tokenCount
std::map<int, uint32_t> tokenCount;
{
- std::lock_guard<std::mutex> lock(mTokenMapMutex);
+ std::lock_guard<std::mutex> lock(mTokenMutex);
for (auto iter = mTokenMap.begin(); iter != mTokenMap.end(); ++iter) {
- sp<BufferClient> client = iter->second.promote();
+ sp<BufferClient> client = iter->second.second.promote();
if (client != nullptr) {
const std::shared_ptr<BufferNode> node = client->getBufferNode();
auto mapIter = tokenCount.find(node->id());
@@ -262,21 +294,35 @@
}
hidl_handle BufferHubService::registerToken(const wp<BufferClient>& client) {
- uint32_t token;
- std::lock_guard<std::mutex> lock(mTokenMapMutex);
+ // Find next available token id
+ std::lock_guard<std::mutex> lock(mTokenMutex);
do {
- token = mTokenEngine();
- } while (mTokenMap.find(token) != mTokenMap.end());
+ ++mLastTokenId;
+ } while (mTokenMap.find(mLastTokenId) != mTokenMap.end());
- // native_handle_t use int[], so here need one slots to fit in uint32_t
- native_handle_t* handle = native_handle_create(/*numFds=*/0, /*numInts=*/1);
- handle->data[0] = token;
+ std::array<uint8_t, EVP_MAX_MD_SIZE> hmac;
+ uint32_t hmacSize = 0U;
+
+ HMAC(/*evp_md=*/EVP_sha256(), /*key=*/&mKey, /*key_len=*/kKeyLen,
+ /*data=*/(uint8_t*)&mLastTokenId, /*data_len=*/mTokenIdSize,
+ /*out=*/hmac.data(), /*out_len=*/&hmacSize);
+
+ int numIntsForHMAC = (int)ceil(hmacSize / (double)sizeof(int));
+ native_handle_t* handle = native_handle_create(/*numFds=*/0, /*numInts=*/1 + numIntsForHMAC);
+ handle->data[0] = mLastTokenId;
+ // Set all the the bits of last int to 0 since it might not be fully overwritten
+ handle->data[numIntsForHMAC] = 0;
+ memcpy(&handle->data[1], hmac.data(), hmacSize);
// returnToken owns the native_handle_t* thus doing lifecycle management
hidl_handle returnToken;
returnToken.setTo(handle, /*shoudOwn=*/true);
- mTokenMap.emplace(token, client);
+ std::vector<uint8_t> hmacVec;
+ hmacVec.resize(hmacSize);
+ memcpy(hmacVec.data(), hmac.data(), hmacSize);
+ mTokenMap.emplace(mLastTokenId, std::pair(hmacVec, client));
+
return returnToken;
}
@@ -291,10 +337,10 @@
}
void BufferHubService::removeTokenByClient(const BufferClient* client) {
- std::lock_guard<std::mutex> lock(mTokenMapMutex);
+ std::lock_guard<std::mutex> lock(mTokenMutex);
auto iter = mTokenMap.begin();
while (iter != mTokenMap.end()) {
- if (iter->second == client) {
+ if (iter->second.second == client) {
auto oldIter = iter;
++iter;
mTokenMap.erase(oldIter);
diff --git a/services/bufferhub/include/bufferhub/BufferHubService.h b/services/bufferhub/include/bufferhub/BufferHubService.h
index 255f329..0195bcf 100644
--- a/services/bufferhub/include/bufferhub/BufferHubService.h
+++ b/services/bufferhub/include/bufferhub/BufferHubService.h
@@ -17,8 +17,10 @@
#ifndef ANDROID_FRAMEWORKS_BUFFERHUB_V1_0_BUFFER_HUB_SERVICE_H
#define ANDROID_FRAMEWORKS_BUFFERHUB_V1_0_BUFFER_HUB_SERVICE_H
+#include <map>
#include <mutex>
-#include <random>
+#include <set>
+#include <vector>
#include <android/frameworks/bufferhub/1.0/IBufferHub.h>
#include <bufferhub/BufferClient.h>
@@ -39,6 +41,8 @@
class BufferHubService : public IBufferHub {
public:
+ BufferHubService();
+
Return<void> allocateBuffer(const HardwareBufferDescription& description,
const uint32_t userMetadataSize,
allocateBuffer_cb _hidl_cb) override;
@@ -60,11 +64,19 @@
std::mutex mClientSetMutex;
std::set<wp<BufferClient>> mClientSet GUARDED_BY(mClientSetMutex);
- // TODO(b/118180214): use a more secure implementation
- std::mt19937 mTokenEngine;
- // The mapping from token to the client creates it.
- std::mutex mTokenMapMutex;
- std::map<uint32_t, const wp<BufferClient>> mTokenMap GUARDED_BY(mTokenMapMutex);
+ // Token generation related
+ // A random number used as private key for HMAC
+ uint64_t mKey;
+ static constexpr size_t kKeyLen = sizeof(uint64_t);
+
+ std::mutex mTokenMutex;
+ // The first TokenId will be 1. TokenId could be negative.
+ int mLastTokenId GUARDED_BY(mTokenMutex) = 0;
+ static constexpr size_t mTokenIdSize = sizeof(int);
+ // A map from token id to the token-buffer_client pair. Using token id as the key to reduce
+ // looking up time
+ std::map<int, std::pair<std::vector<uint8_t>, const wp<BufferClient>>> mTokenMap
+ GUARDED_BY(mTokenMutex);
};
} // namespace implementation