Improve visibility of IMemory security risks
This change renames the IMemory raw pointer accessors to
unsecure*() to make it apparent to coders and code reviewers
that the returned buffer may potentially be shared with
untrusted processes, who may, after the fact, attempt to
read and/or modify the contents. This may lead to hard to
find security bugs and hopefully the rename makes it harder
to forget.
The change also attempts to fix all the callsites to make
everything build correctly, but in the processes, wherever the
callsite code was not obviously secure, I added a TODO requesting
the owners to either document why it's secure or to change the
code. Apologies in advance to the owners if there are some false
positives here - I don't have enough context to reason about all
the different callsites.
Test: Completely syntactic change. Made sure code still builds.
Change-Id: I4c555ef8c8c47cf28b42b17ad8b4021a783548cd
diff --git a/core/jni/android_hardware_SoundTrigger.cpp b/core/jni/android_hardware_SoundTrigger.cpp
index 03057dc..0002f8b 100644
--- a/core/jni/android_hardware_SoundTrigger.cpp
+++ b/core/jni/android_hardware_SoundTrigger.cpp
@@ -606,12 +606,12 @@
goto exit;
}
memory = memoryDealer->allocate(offset + size);
- if (memory == 0 || memory->pointer() == NULL) {
+ if (memory == 0 || memory->unsecurePointer() == NULL) {
status = SOUNDTRIGGER_STATUS_ERROR;
goto exit;
}
- nSoundModel = (struct sound_trigger_sound_model *)memory->pointer();
+ nSoundModel = (struct sound_trigger_sound_model *)memory->unsecurePointer();
nSoundModel->type = type;
nSoundModel->uuid = nUuid;
@@ -737,18 +737,18 @@
return SOUNDTRIGGER_STATUS_ERROR;
}
sp<IMemory> memory = memoryDealer->allocate(totalSize);
- if (memory == 0 || memory->pointer() == NULL) {
+ if (memory == 0 || memory->unsecurePointer() == NULL) {
return SOUNDTRIGGER_STATUS_ERROR;
}
if (dataSize != 0) {
- memcpy((char *)memory->pointer() + sizeof(struct sound_trigger_recognition_config),
+ memcpy((char *)memory->unsecurePointer() + sizeof(struct sound_trigger_recognition_config),
nData,
dataSize);
env->ReleaseByteArrayElements(jData, nData, 0);
}
env->DeleteLocalRef(jData);
struct sound_trigger_recognition_config *config =
- (struct sound_trigger_recognition_config *)memory->pointer();
+ (struct sound_trigger_recognition_config *)memory->unsecurePointer();
config->data_size = dataSize;
config->data_offset = sizeof(struct sound_trigger_recognition_config);
config->capture_requested = env->GetBooleanField(jConfig,
diff --git a/core/jni/android_media_AudioTrack.cpp b/core/jni/android_media_AudioTrack.cpp
index daa6347..c5049ec 100644
--- a/core/jni/android_media_AudioTrack.cpp
+++ b/core/jni/android_media_AudioTrack.cpp
@@ -649,7 +649,7 @@
if ((size_t)sizeInBytes > track->sharedBuffer()->size()) {
sizeInBytes = track->sharedBuffer()->size();
}
- memcpy(track->sharedBuffer()->pointer(), data + offsetInSamples, sizeInBytes);
+ memcpy(track->sharedBuffer()->unsecurePointer(), data + offsetInSamples, sizeInBytes);
written = sizeInBytes;
}
if (written >= 0) {
diff --git a/media/jni/android_media_MediaDataSource.cpp b/media/jni/android_media_MediaDataSource.cpp
index 8c38d88..9705b91 100644
--- a/media/jni/android_media_MediaDataSource.cpp
+++ b/media/jni/android_media_MediaDataSource.cpp
@@ -106,7 +106,8 @@
}
ALOGV("readAt %lld / %zu => %d.", (long long)offset, size, numread);
- env->GetByteArrayRegion(mByteArrayObj, 0, numread, (jbyte*)mMemory->pointer());
+ env->GetByteArrayRegion(mByteArrayObj, 0, numread,
+ (jbyte*)mMemory->unsecurePointer());
return numread;
}
diff --git a/media/jni/android_media_MediaDescrambler.cpp b/media/jni/android_media_MediaDescrambler.cpp
index aa79ce0..c61365a 100644
--- a/media/jni/android_media_MediaDescrambler.cpp
+++ b/media/jni/android_media_MediaDescrambler.cpp
@@ -220,7 +220,7 @@
return NO_MEMORY;
}
- memcpy(mMem->pointer(),
+ memcpy(mMem->unsecurePointer(),
(const void*)((const uint8_t*)srcPtr + srcOffset), totalLength);
DestinationBuffer dstBuffer;
@@ -248,7 +248,8 @@
if (*status == Status::OK) {
if (*bytesWritten > 0 && (ssize_t) *bytesWritten <= totalLength) {
- memcpy((void*)((uint8_t*)dstPtr + dstOffset), mMem->pointer(), *bytesWritten);
+ memcpy((void*)((uint8_t*)dstPtr + dstOffset), mMem->unsecurePointer(),
+ *bytesWritten);
} else {
// status seems OK but bytesWritten is invalid, we really
// have no idea what is wrong.
diff --git a/media/jni/android_media_MediaHTTPConnection.cpp b/media/jni/android_media_MediaHTTPConnection.cpp
index 365e045..53adff3 100644
--- a/media/jni/android_media_MediaHTTPConnection.cpp
+++ b/media/jni/android_media_MediaHTTPConnection.cpp
@@ -148,7 +148,7 @@
byteArrayObj,
0,
n,
- (jbyte *)conn->getIMemory()->pointer());
+ (jbyte *)conn->getIMemory()->unsecurePointer());
}
return n;
diff --git a/media/jni/android_media_MediaMetadataRetriever.cpp b/media/jni/android_media_MediaMetadataRetriever.cpp
index 3809bc4..18fd1a0 100644
--- a/media/jni/android_media_MediaMetadataRetriever.cpp
+++ b/media/jni/android_media_MediaMetadataRetriever.cpp
@@ -396,8 +396,12 @@
// Call native method to retrieve a video frame
VideoFrame *videoFrame = NULL;
sp<IMemory> frameMemory = retriever->getFrameAtTime(timeUs, option);
+ // TODO: Using unsecurePointer() has some associated security pitfalls
+ // (see declaration for details).
+ // Either document why it is safe in this case or address the
+ // issue (e.g. by copying).
if (frameMemory != 0) { // cast the shared structure to a VideoFrame object
- videoFrame = static_cast<VideoFrame *>(frameMemory->pointer());
+ videoFrame = static_cast<VideoFrame *>(frameMemory->unsecurePointer());
}
if (videoFrame == NULL) {
ALOGE("getFrameAtTime: videoFrame is a NULL pointer");
@@ -423,7 +427,11 @@
VideoFrame *videoFrame = NULL;
sp<IMemory> frameMemory = retriever->getImageAtIndex(index, colorFormat);
if (frameMemory != 0) { // cast the shared structure to a VideoFrame object
- videoFrame = static_cast<VideoFrame *>(frameMemory->pointer());
+ // TODO: Using unsecurePointer() has some associated security pitfalls
+ // (see declaration for details).
+ // Either document why it is safe in this case or address the
+ // issue (e.g. by copying).
+ videoFrame = static_cast<VideoFrame *>(frameMemory->unsecurePointer());
}
if (videoFrame == NULL) {
ALOGE("getImageAtIndex: videoFrame is a NULL pointer");
@@ -454,7 +462,11 @@
sp<IMemory> frameMemory = retriever->getImageAtIndex(
index, colorFormat, true /*metaOnly*/, true /*thumbnail*/);
if (frameMemory != 0) {
- videoFrame = static_cast<VideoFrame *>(frameMemory->pointer());
+ // TODO: Using unsecurePointer() has some associated security pitfalls
+ // (see declaration for details).
+ // Either document why it is safe in this case or address the
+ // issue (e.g. by copying).
+ videoFrame = static_cast<VideoFrame *>(frameMemory->unsecurePointer());
int32_t thumbWidth = videoFrame->mWidth;
int32_t thumbHeight = videoFrame->mHeight;
videoFrame = NULL;
@@ -467,7 +479,11 @@
|| thumbPixels * 6 >= maxPixels) {
frameMemory = retriever->getImageAtIndex(
index, colorFormat, false /*metaOnly*/, true /*thumbnail*/);
- videoFrame = static_cast<VideoFrame *>(frameMemory->pointer());
+ // TODO: Using unsecurePointer() has some associated security pitfalls
+ // (see declaration for details).
+ // Either document why it is safe in this case or address the
+ // issue (e.g. by copying).
+ videoFrame = static_cast<VideoFrame *>(frameMemory->unsecurePointer());
if (thumbPixels > maxPixels) {
int downscale = ceil(sqrt(thumbPixels / (float)maxPixels));
@@ -514,11 +530,15 @@
size_t i = 0;
for (; i < numFrames; i++) {
sp<IMemory> frame = retriever->getFrameAtIndex(frameIndex + i, colorFormat);
- if (frame == NULL || frame->pointer() == NULL) {
+ if (frame == NULL || frame->unsecurePointer() == NULL) {
ALOGE("video frame at index %zu is a NULL pointer", frameIndex + i);
break;
}
- VideoFrame *videoFrame = static_cast<VideoFrame *>(frame->pointer());
+ // TODO: Using unsecurePointer() has some associated security pitfalls
+ // (see declaration for details).
+ // Either document why it is safe in this case or address the
+ // issue (e.g. by copying).
+ VideoFrame *videoFrame = static_cast<VideoFrame *>(frame->unsecurePointer());
jobject bitmapObj = getBitmapFromVideoFrame(env, videoFrame, -1, -1, outColorType);
env->CallBooleanMethod(arrayList, fields.arrayListAdd, bitmapObj);
env->DeleteLocalRef(bitmapObj);
@@ -551,7 +571,11 @@
// the method name to getEmbeddedPicture().
sp<IMemory> albumArtMemory = retriever->extractAlbumArt();
if (albumArtMemory != 0) { // cast the shared structure to a MediaAlbumArt object
- mediaAlbumArt = static_cast<MediaAlbumArt *>(albumArtMemory->pointer());
+ // TODO: Using unsecurePointer() has some associated security pitfalls
+ // (see declaration for details).
+ // Either document why it is safe in this case or address the
+ // issue (e.g. by copying).
+ mediaAlbumArt = static_cast<MediaAlbumArt *>(albumArtMemory->unsecurePointer());
}
if (mediaAlbumArt == NULL) {
ALOGE("getEmbeddedPicture: Call to getEmbeddedPicture failed.");
diff --git a/media/jni/soundpool/SoundPool.h b/media/jni/soundpool/SoundPool.h
index 9d74103..01e4faa 100644
--- a/media/jni/soundpool/SoundPool.h
+++ b/media/jni/soundpool/SoundPool.h
@@ -61,7 +61,7 @@
audio_channel_mask_t channelMask() { return mChannelMask; }
size_t size() { return mSize; }
int state() { return mState; }
- uint8_t* data() { return static_cast<uint8_t*>(mData->pointer()); }
+ uint8_t* data() { return static_cast<uint8_t*>(mData->unsecurePointer()); }
status_t doLoad();
void startLoad() { mState = LOADING; }
sp<IMemory> getIMemory() { return mData; }
diff --git a/media/tests/audiotests/shared_mem_test.cpp b/media/tests/audiotests/shared_mem_test.cpp
index 2f57499..d586b6a 100644
--- a/media/tests/audiotests/shared_mem_test.cpp
+++ b/media/tests/audiotests/shared_mem_test.cpp
@@ -92,7 +92,7 @@
iMem = heap->allocate(BUF_SZ*sizeof(short));
- p = static_cast<uint8_t*>(iMem->pointer());
+ p = static_cast<uint8_t*>(iMem->unsecurePointer());
memcpy(p, smpBuf, BUF_SZ*sizeof(short));
sp<AudioTrack> track = new AudioTrack(AUDIO_STREAM_MUSIC,// stream type