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

Commit 7dd39723 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: I5fb99aa797c488406083178a6b05355d98710d3b
parent 61d4a093

cmds/stagefright/stagefright.cpp

+1 −1
Original line number Diff line number Diff line
@@ -989,7 +989,7 @@ int main(int argc, char **argv) { 
                failed = false;

                printf("getFrameAtTime(%s) => OK\n", filename);



                VideoFrame *frame = (VideoFrame *)mem->pointer();

                VideoFrame *frame = (VideoFrame *)mem->unsecurePointer();



                CHECK_EQ(writeJpegFile("/sdcard/out.jpg",

                            frame->getFlattenedData(),

cmds/stagefright/stream.cpp

+2 −2
Original line number Diff line number Diff line
@@ -116,7 +116,7 @@ void MyStreamSource::onBufferAvailable(size_t index) { 


    sp<IMemory> mem = mBuffers.itemAt(index);



    ssize_t n = read(mFd, mem->pointer(), mem->size());

    ssize_t n = read(mFd, mem->unsecurePointer(), mem->size());

    if (n <= 0) {

        mListener->issueCommand(IStreamListener::EOS, false /* synchronous */);

    } else {
@@ -238,7 +238,7 @@ ssize_t MyConvertingStreamSource::writeData(const void *data, size_t size) { 
            copy = mem->size() - mCurrentBufferOffset;

        }



        memcpy((uint8_t *)mem->pointer() + mCurrentBufferOffset, data, copy);

        memcpy((uint8_t *)mem->unsecurePointer() + mCurrentBufferOffset, data, copy);

        mCurrentBufferOffset += copy;



        if (mCurrentBufferOffset == mem->size()) {

drm/libmediadrm/CryptoHal.cpp

+5 −1
Original line number Diff line number Diff line
@@ -321,7 +321,11 @@ status_t CryptoHal::toSharedBuffer(const sp<IMemory>& memory, int32_t seqNum, :: 
     }



    // memory must be within the address space of the heap

    if (memory->pointer() != static_cast<uint8_t *>(heap->getBase()) + memory->offset()  ||

    // 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 (memory->unsecurePointer() != static_cast<uint8_t *>(heap->getBase()) + memory->offset()  ||

            heap->getSize() < memory->offset() + memory->size() ||

            SIZE_MAX - memory->offset() < memory->size()) {

        android_errorWriteLog(0x534e4554, "76221123");

media/codec2/sfplugin/Codec2Buffer.cpp

+6 −2
Original line number Diff line number Diff line
@@ -764,7 +764,11 @@ EncryptedLinearBlockBuffer::EncryptedLinearBlockBuffer( 
        const std::shared_ptr<C2LinearBlock> &block,

        const sp<IMemory> &memory,

        int32_t heapSeqNum)

    : Codec2Buffer(format, new ABuffer(memory->pointer(), memory->size())),

    // 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).

    : Codec2Buffer(format, new ABuffer(memory->unsecurePointer(), memory->size())),

      mBlock(block),

      mMemory(memory),

      mHeapSeqNum(heapSeqNum) {
@@ -800,7 +804,7 @@ bool EncryptedLinearBlockBuffer::copyDecryptedContent( 
    if (view.size() < length) {

        return false;

    }

    memcpy(view.data(), decrypted->pointer(), length);

    memcpy(view.data(), decrypted->unsecurePointer(), length);

    return true;

}

media/libaudioclient/AudioEffect.cpp

+5 −1
Original line number Diff line number Diff line
@@ -159,7 +159,11 @@ status_t AudioEffect::set(const effect_uuid_t *type, 


    mIEffect = iEffect;

    mCblkMemory = cblk;

    mCblk = static_cast<effect_param_cblk_t*>(cblk->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).

    mCblk = static_cast<effect_param_cblk_t*>(cblk->unsecurePointer());

    int bufOffset = ((sizeof(effect_param_cblk_t) - 1) / sizeof(int) + 1) * sizeof(int);

    mCblk->buffer = (uint8_t *)mCblk + bufOffset;