commit f743b8d2d541e581eff86b106b67b409bed462df
author Jeff Tinker <jtinker@google.com> Thu Apr 19 16:23:21 2018 -0700
committer android-build-team Robot <android-build-team-robot@google.com> Fri Jul 20 00:17:05 2018 +0000
tree 38147235e1cc6649b767028992f9d3a181da7d9f
parent 08d392085c095e227c029f64644bc08ef5a544de

Fix security vulnerability in CryptoHal

CryptoHal was not checking that the memory heap set by setHeap
was the same one that was actually used for the decrypt call, allowing
the caller to spoof the decrypt call into accessing arbitrary memory.

bug:76221123
test: mediadrmserverpoc included in the bug & GTS media tests
Change-Id: I35214a1a6d0a4b864123e147d1a1adc2377bfbc5
Merged-in: I4ae6d1080be406bf53e3617c59c75206cc5066c6
(cherry picked from commit 9a9c3ab4d76f03f3abb3756bca9cdfe55c74326a)

drm/libmediadrm/CryptoHal.cpp

diff --git a/drm/libmediadrm/CryptoHal.cpp b/drm/libmediadrm/CryptoHal.cpp
index 1fdc6e1..5dd2563 100644
--- a/drm/libmediadrm/CryptoHal.cpp
+++ b/drm/libmediadrm/CryptoHal.cpp
@@ -240,11 +240,12 @@
     Mutex::Autolock autoLock(mLock);
 
     int32_t seqNum = mHeapSeqNum++;
+
     int fd = heap->getHeapID();
     nativeHandle->data[0] = fd;
     auto hidlHandle = hidl_handle(nativeHandle);
     auto hidlMemory = hidl_memory("ashmem", hidlHandle, heap->getSize());
-    mHeapBases.add(seqNum, mNextBufferId);
+    mHeapBases.add(seqNum, HeapBase(mNextBufferId, heap->getSize()));
     Return<void> hResult = mPlugin->setSharedBufferBase(hidlMemory, mNextBufferId++);
     ALOGE_IF(!hResult.isOk(), "setSharedBufferBase(): remote call failed");
     return seqNum;
@@ -269,10 +270,26 @@
         return UNEXPECTED_NULL;
     }
 
-    // memory must be in the declared heap
-    CHECK(mHeapBases.indexOfKey(seqNum) >= 0);
+    // memory must be in one of the heaps that have been set
+    if (mHeapBases.indexOfKey(seqNum) < 0) {
+        return UNKNOWN_ERROR;
+    }
 
-    buffer->bufferId = mHeapBases.valueFor(seqNum);
+    // heap must be the same size as the one that was set in setHeapBase
+    if (mHeapBases.valueFor(seqNum).getSize() != heap->getSize()) {
+        android_errorWriteLog(0x534e4554, "76221123");
+        return UNKNOWN_ERROR;
+     }
+
+    // memory must be within the address space of the heap
+    if (memory->pointer() != static_cast<uint8_t *>(heap->getBase()) + memory->offset()  ||
+            heap->getSize() < memory->offset() + memory->size() ||
+            SIZE_MAX - memory->offset() < memory->size()) {
+        android_errorWriteLog(0x534e4554, "76221123");
+        return UNKNOWN_ERROR;
+    }
+
+    buffer->bufferId = mHeapBases.valueFor(seqNum).getBufferId();
     buffer->offset = offset >= 0 ? offset : 0;
     buffer->size = size;
     return OK;

media/libmedia/include/media/CryptoHal.h

diff --git a/media/libmedia/include/media/CryptoHal.h b/media/libmedia/include/media/CryptoHal.h
index a5d8b43..80e181e 100644
--- a/media/libmedia/include/media/CryptoHal.h
+++ b/media/libmedia/include/media/CryptoHal.h
@@ -79,7 +79,20 @@
      */
     status_t mInitCheck;
 
-    KeyedVector<int32_t, uint32_t> mHeapBases;
+    struct HeapBase {
+        HeapBase() : mBufferId(0), mSize(0) {}
+        HeapBase(uint32_t bufferId, size_t size) :
+            mBufferId(bufferId), mSize(size) {}
+
+        uint32_t getBufferId() const {return mBufferId;}
+        size_t getSize() const {return mSize;}
+
+    private:
+        uint32_t mBufferId;
+        size_t mSize;
+    };
+
+    KeyedVector<int32_t, HeapBase> mHeapBases;
     uint32_t mNextBufferId;
     int32_t mHeapSeqNum;