Fix vulnerability in MemoryIntArray
MemoryIntArray was using the size of the undelying
ashmem region to mmap the data but the ashmem size
can be changed until the former is memory mapped.
Since we use the ashmem region size for boundary
checking and memory unmapping if it does not match
the size used while mapping an attacker can force
the system to unmap memory or to access undefined
memory and crash.
Also we were passing the memory address where the
ashmem region is mapped in the owner process to
support cases where the client can pass back the
MemoryIntArray instance. This allows an attacker
to put invalid address and cause arbitrary memory
to be freed.
Now we no longer support passing back the instance
to the owner process (the passed back instance is
read only), so no need to pass the memory adress
of the owner's mapping, thus not allowing freeing
arbitrary memory.
Further, we now check the memory mapped size against
the size of the underlying ashmem region after we do
the memory mapping (to fix the ahsmem size) and if
an attacker changed the size under us we throw.
Tests: Updated the tests and they pass.
bug:33039926
bug:33042690
Change-Id: Ib8e50afcdb5475123968572ac9696e8ed4031631
diff --git a/core/jni/android_util_MemoryIntArray.cpp b/core/jni/android_util_MemoryIntArray.cpp
index 9513c8b..2dfbe3e 100644
--- a/core/jni/android_util_MemoryIntArray.cpp
+++ b/core/jni/android_util_MemoryIntArray.cpp
@@ -54,7 +54,7 @@
}
static jlong android_util_MemoryIntArray_open(JNIEnv* env, jobject clazz, jint fd,
- jboolean owner, jboolean writable)
+ jboolean owner)
{
if (fd < 0) {
jniThrowException(env, "java/io/IOException", "bad file descriptor");
@@ -72,19 +72,35 @@
return -1;
}
- int protMode = (owner || writable) ? (PROT_READ | PROT_WRITE) : PROT_READ;
+ // IMPORTANT: Ashmem allows the caller to change its size until
+ // it is memory mapped for the first time which lazily creates
+ // the underlying VFS file. So the size we get above may not
+ // reflect the size of the underlying shared memory region. Therefore,
+ // we first memory map to set the size in stone an verify if
+ // the underlying ashmem region has the same size as the one we
+ // memory mapped. This is critical as we use the underlying
+ // ashmem size for boundary checks and memory unmapping.
+ int protMode = owner ? (PROT_READ | PROT_WRITE) : PROT_READ;
void* ashmemAddr = mmap(NULL, ashmemSize, protMode, MAP_SHARED, fd, 0);
if (ashmemAddr == MAP_FAILED) {
jniThrowException(env, "java/io/IOException", "cannot mmap ashmem");
return -1;
}
+ // Check if the mapped size is the same as the ashmem region.
+ int mmapedSize = ashmem_get_size_region(fd);
+ if (mmapedSize != ashmemSize) {
+ munmap(reinterpret_cast<void *>(ashmemAddr), ashmemSize);
+ jniThrowException(env, "java/io/IOException", "bad file descriptor");
+ return -1;
+ }
+
if (owner) {
int size = ashmemSize / sizeof(std::atomic_int);
new (ashmemAddr) std::atomic_int[size];
}
- if (owner && !writable) {
+ if (owner) {
int setProtResult = ashmem_set_prot_region(fd, PROT_READ);
if (setProtResult < 0) {
jniThrowException(env, "java/io/IOException", "cannot set ashmem prot mode");
@@ -131,7 +147,7 @@
}
static jint android_util_MemoryIntArray_get(JNIEnv* env, jobject clazz,
- jint fd, jlong address, jint index, jboolean owner)
+ jint fd, jlong address, jint index)
{
if (fd < 0) {
jniThrowException(env, "java/io/IOException", "bad file descriptor");
@@ -153,7 +169,7 @@
}
static void android_util_MemoryIntArray_set(JNIEnv* env, jobject clazz,
- jint fd, jlong address, jint index, jint newValue, jboolean owner)
+ jint fd, jlong address, jint index, jint newValue)
{
if (fd < 0) {
jniThrowException(env, "java/io/IOException", "bad file descriptor");
@@ -195,10 +211,10 @@
static const JNINativeMethod methods[] = {
{"nativeCreate", "(Ljava/lang/String;I)I", (void*)android_util_MemoryIntArray_create},
- {"nativeOpen", "(IZZ)J", (void*)android_util_MemoryIntArray_open},
+ {"nativeOpen", "(IZ)J", (void*)android_util_MemoryIntArray_open},
{"nativeClose", "(IJZ)V", (void*)android_util_MemoryIntArray_close},
- {"nativeGet", "(IJIZ)I", (void*)android_util_MemoryIntArray_get},
- {"nativeSet", "(IJIIZ)V", (void*) android_util_MemoryIntArray_set},
+ {"nativeGet", "(IJI)I", (void*)android_util_MemoryIntArray_get},
+ {"nativeSet", "(IJII)V", (void*) android_util_MemoryIntArray_set},
{"nativeSize", "(I)I", (void*) android_util_MemoryIntArray_size},
};