Fix for this issue appeared as CVE-2024-49746: bulletin, patch

"This seems wrong"

Above title is the comment from Parcel::continueWrite method, which is actually responsible for resizing Parcel objects, either when explicitly request by user (for example through setDataSize()) or when calling one of write methods when current data capacity is too small

status_t Parcel::continueWrite(size_t desired)
{
    // SNIP: Validate desired size
    // SNIP: Assign kernelFields & rpcFields from variant member of this class
    // SNIP: Count number of objects (Binder handles and File Descriptors)
    //       that will be present after resize and assign to objectsSize

    if (mOwner) {
        // If the size is going to zero, just release the owner's data.
        if (desired == 0) {
            freeData();
            return NO_ERROR;
        }

        // If there is a different owner, we need to take
        // posession.
        uint8_t* data = (uint8_t*)malloc(desired);
        // SNIP: Check if malloc succeeded
        binder_size_t* objects = nullptr;

        if (kernelFields && objectsSize) {
            objects = (binder_size_t*)calloc(objectsSize, sizeof(binder_size_t));
            // SNIP: Check if calloc succeeded

            // Little hack to only acquire references on objects
            // we will be keeping.
            size_t oldObjectsSize = kernelFields->mObjectsSize;
            kernelFields->mObjectsSize = objectsSize;
            acquireObjects();
            kernelFields->mObjectsSize = oldObjectsSize;
        }
        // SNIP: rpcFields handling for non-/dev/binder Parcels

        if (mData) {
            memcpy(data, mData, mDataSize < desired ? mDataSize : desired);
        }
        if (objects && kernelFields && kernelFields->mObjects) {
            memcpy(objects, kernelFields->mObjects, objectsSize * sizeof(binder_size_t));
        }
        // ALOGI("Freeing data ref of %p (pid=%d)", this, getpid());
        if (kernelFields) {
            // TODO(b/239222407): This seems wrong. We should only free FDs when
            // they are in a truncated section of the parcel.
            closeFileDescriptors();
        }
        mOwner(mData, mDataSize, kernelFields ? kernelFields->mObjects : nullptr,
               kernelFields ? kernelFields->mObjectsSize : 0);
        mOwner = nullptr;

        // SNIP: Allocation count tracking
        // SNIP: Assign data and objects to this object
    } else if (mData) {
        // SNIP: Resize data owned by this instance of Parcel
    } else {
        // SNIP: Allocate initial data for currently empty Parcel
    }

    return NO_ERROR;
}

When that comment was introduced, closeFileDescriptors() call was moved from IPCThreadState::freeBuffer() (which is called in above code through mOwner() function pointer) to continueWrite() method, however logic was same as before. After all, Parcel is core part of Android IPC and if core IPC was closing File Descriptors it shouldn't it'd be obvious problem

Which brings us to important part: when above code is being used? It is used when Parcel class moves ownership of data received from Binder driver (which at that point reside in /dev/binder mmap and cannot be written to (any attempts to write that memory would lead to SIGSEGV)), that is Parcel being either incoming transaction data (data argument passed to onTransact()) or incoming reply (that is, Parcel object that was passed to transact() call as reply argument, transact() sets reference within that Parcel object)

In practice, only case we'd enter if (mOwner) block is when system calls setDataSize(0) to release transaction data, but in that case we'd also enter if (desired == 0) which does early return. During legitimate system usage there's no case where we'd enter "If there is a different owner, we need to take possession" path though

Triggering the "take possession" path

In one of my previous exploits I've shown case where createFromParcel() can actually call writeInt(0) on Parcel it should be reading from. While fix there prevented execution of any non-Intent createFromParcel() methods within AccountManagerService, the createFromParcel() to writeInt(0) path was kept intact

To recap, inside PackageParser we have following code:

final Class<T> cls = (Class<T>) Class.forName(componentName);
final Constructor<T> cons = cls.getConstructor(Parcel.class);

intentsList = new ArrayList<>(N);
for (int i = 0; i < N; ++i) {
    intentsList.add(cons.newInstance(in));
}

Therefore, we can have Parcel object which was passed to createFromParcel passed to any available in system public constructor that accepts single Parcel argument

And elsewhere we have following code:

public PooledStringWriter(Parcel out) {
    mOut = out;
    mPool = new HashMap<>();
    mStart = out.dataPosition();
    out.writeInt(0); // reserve space for final pool size.
}

Therefore, in order to trigger "take possession" path, we need to have arbitrary readParcelable call on Parcel that was passed to onTransact() as data, in this exploit I'm using for that same path I previously used in different one. I have readParcelable call PackageParser$Activity.CREATOR.createFromParcel(), which in turn reads PooledStringWriter name and calls its constructor and after that Parcel data ends, so writeInt() needs to reallocate Parcel, entering our "take possession" path

Of note here, if there wasn't end of Parcel data at that point, writeInt() would attempt overwriting data in place, which in case of data backed by /dev/binder mmap would lead to SIGSEGV

File Descriptor Sanitizer

My initial idea was to have "take possession" path close File Descriptors, after which at end of transaction same Descriptors would be closed again, but between those things happen I'd put other File Descriptor within system_server in another transaction and at later point get my File Descriptor back, as that FD refers at that point to different file