Donate to e Foundation | Murena handsets with /e/OS | Own a part of Murena! Learn more

Commit 2c9d8f6d authored by Ytai Ben-Tsvi's avatar Ytai Ben-Tsvi
Browse files

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
parent 32d22286

core/jni/android_hardware_SoundTrigger.cpp

+5 −5
Original line number Diff line number Diff line
@@ -606,12 +606,12 @@ android_hardware_SoundTrigger_loadSoundModel(JNIEnv *env, jobject thiz, 
        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 @@ android_hardware_SoundTrigger_startRecognition(JNIEnv *env, jobject thiz, 
        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,

core/jni/android_media_AudioTrack.cpp

+1 −1
Original line number Diff line number Diff line
@@ -649,7 +649,7 @@ static jint writeToTrack(const sp<AudioTrack>& track, jint audioFormat, const T 
        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) {

media/jni/android_media_MediaDataSource.cpp

+2 −1
Original line number Diff line number Diff line
@@ -106,7 +106,8 @@ ssize_t JMediaDataSource::readAt(off64_t offset, size_t size) { 
    }



    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;

}

media/jni/android_media_MediaDescrambler.cpp

+3 −2
Original line number Diff line number Diff line
@@ -220,7 +220,7 @@ status_t JDescrambler::descramble( 
        return NO_MEMORY;

    }



    memcpy(mMem->pointer(),

    memcpy(mMem->unsecurePointer(),

            (const void*)((const uint8_t*)srcPtr + srcOffset), totalLength);



    DestinationBuffer dstBuffer;
@@ -248,7 +248,8 @@ status_t JDescrambler::descramble( 


    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.

media/jni/android_media_MediaHTTPConnection.cpp

+1 −1
Original line number Diff line number Diff line
@@ -148,7 +148,7 @@ static jint android_media_MediaHTTPConnection_native_readAt( 
                byteArrayObj,

                0,

                n,

                (jbyte *)conn->getIMemory()->pointer());

                (jbyte *)conn->getIMemory()->unsecurePointer());

    }



    return n;