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

Commit 6ec2bbaa authored by Devin Moore's avatar Devin Moore
Browse files

Protect objects in Parcel::appendFrom 

* only aquire objects within the range supplied to appendFrom
* don't append over existing objects
* unset the mObjectsSorted flag a couple more cases
* keep mObjectPositions sorted

Flag: EXEMPT bug fix
Ignore-AOSP-First: security fix
Test: binder_parcel_fuzzer
Bug: 402319736
Merged-In: I63715fdd81781aaf04f5fc0cb8bdb74c09d5d807
Change-Id: I63715fdd81781aaf04f5fc0cb8bdb74c09d5d807
parent f325d393

libs/binder/Parcel.cpp

+21 −11
Original line number Diff line number Diff line
@@ -424,7 +424,6 @@ status_t Parcel::appendFrom(const Parcel *parcel, size_t offset, size_t len) 
    const binder_size_t *objects = parcel->mObjects;

    size_t size = parcel->mObjectsSize;

    int startPos = mDataPos;

    int firstIndex = -1, lastIndex = -2;



    if (len == 0) {

        return NO_ERROR;
@@ -444,16 +443,18 @@ status_t Parcel::appendFrom(const Parcel *parcel, size_t offset, size_t len) 
    }



    // Count objects in range

    int numObjects = 0;

    for (int i = 0; i < (int)size; i++) {

        size_t off = objects[i];

        if ((off >= offset) && (off + sizeof(flat_binder_object) <= offset + len)) {

            if (firstIndex == -1) {

                firstIndex = i;

        size_t pos = objects[i];

        if ((pos >= offset) && (pos + sizeof(flat_binder_object) <= offset + len)) {

            numObjects++;

        }

            lastIndex = i;

    }



    // Make sure we aren't appending over objects.

    if (status_t status = validateReadData(mDataPos + len); status != OK) {

        return status;

    }

    int numObjects = lastIndex - firstIndex + 1;



    if ((mDataPos + len) > mDataCapacity) {

        // grow data
@@ -489,8 +490,12 @@ status_t Parcel::appendFrom(const Parcel *parcel, size_t offset, size_t len) 


        // append and acquire objects

        int idx = mObjectsSize;

        for (int i = firstIndex; i <= lastIndex; i++) {

            size_t off = objects[i] - offset + startPos;

        for (int i = 0; i < (int)size; i++) {

            size_t pos = objects[i];

            if (!(pos >= offset) || !(pos + sizeof(flat_binder_object) <= offset + len)) {

                continue;

            }

            size_t off = pos - offset + startPos;

            mObjects[idx++] = off;

            mObjectsSize++;
@@ -510,6 +515,9 @@ status_t Parcel::appendFrom(const Parcel *parcel, size_t offset, size_t len) 
                }

            }

        }

        // Always clear sorted flag. It is tricky to infer if the append

        // result maintains the sort or not.

        mObjectsSorted = false;

    }



    return err;
@@ -1429,6 +1437,8 @@ restart_write: 
            mObjects[mObjectsSize] = mDataPos;

            acquire_object(ProcessState::self(), val, this);

            mObjectsSize++;

            // Clear sorted flag if we aren't appending to the end.

            mObjectsSorted &= mDataPos == mDataSize;

        }



        return finishWrite(sizeof(flat_binder_object));

libs/binder/tests/binderParcelUnitTest.cpp

+11 −0
Original line number Diff line number Diff line
@@ -25,6 +25,7 @@ using android::IBinder; 
using android::IPCThreadState;

using android::OK;

using android::Parcel;

using android::PERMISSION_DENIED;

using android::sp;

using android::status_t;

using android::String16;
@@ -123,6 +124,16 @@ TEST(Parcel, AppendWithBadDataPos) { 
    EXPECT_EQ(android::BAD_VALUE, p2.appendFrom(&p1, 0, 8));

}



TEST(Parcel, AppendOverObject) {

    Parcel p1;

    p1.writeDupFileDescriptor(0);

    Parcel p2;

    p2.writeInt32(2);



    p1.setDataPosition(8);

    ASSERT_EQ(PERMISSION_DENIED, p1.appendFrom(&p2, 0, p2.dataSize()));

}



// Tests a second operation results in a parcel at the same location as it

// started.

void parcelOpSameLength(const std::function<void(Parcel*)>& a, const std::function<void(Parcel*)>& b) {