From 9c1e6eeb198de2e525c5759b619ca65dec13be4c Mon Sep 17 00:00:00 2001 From: Devin Moore Date: Mon, 17 Mar 2025 23:09:26 +0000 Subject: [SP 2025-09-01] 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 Change-Id: I63715fdd81781aaf04f5fc0cb8bdb74c09d5d807 (cherry picked from commit 28e7af08b92e7b97f46d8ecd88ebd3f27a065e08) --- libs/binder/Parcel.cpp | 40 +++++++++++++++++++++--------- libs/binder/tests/binderParcelUnitTest.cpp | 11 ++++++++ 2 files changed, 39 insertions(+), 12 deletions(-) diff --git a/libs/binder/Parcel.cpp b/libs/binder/Parcel.cpp index 2c37624304..c7273ad246 100644 --- a/libs/binder/Parcel.cpp +++ b/libs/binder/Parcel.cpp @@ -542,6 +542,11 @@ status_t Parcel::appendFrom(const Parcel* parcel, size_t offset, size_t len) { return BAD_VALUE; } + // Make sure we aren't appending over objects. + if (status_t status = validateReadData(mDataPos + len); status != OK) { + return status; + } + if ((mDataPos + len) > mDataCapacity) { // grow data err = growData(len); @@ -565,17 +570,13 @@ status_t Parcel::appendFrom(const Parcel* parcel, size_t offset, size_t len) { const binder_size_t* objects = otherKernelFields->mObjects; size_t size = otherKernelFields->mObjectsSize; // Count objects in range - int firstIndex = -1, lastIndex = -2; + 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; - } - lastIndex = i; + size_t pos = objects[i]; + if ((pos >= offset) && (pos + sizeof(flat_binder_object) <= offset + len)) { + numObjects++; } } - int numObjects = lastIndex - firstIndex + 1; if (numObjects > 0) { const sp proc(ProcessState::self()); // grow objects @@ -597,8 +598,12 @@ status_t Parcel::appendFrom(const Parcel* parcel, size_t offset, size_t len) { // append and acquire objects int idx = kernelFields->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; kernelFields->mObjects[idx++] = off; kernelFields->mObjectsSize++; @@ -618,6 +623,9 @@ status_t Parcel::appendFrom(const Parcel* parcel, size_t offset, size_t len) { acquire_object(proc, *flat, this, true /*tagFds*/); } + // Always clear sorted flag. It is tricky to infer if the append + // result maintains the sort or not. + kernelFields->mObjectsSorted = false; } #else LOG_ALWAYS_FATAL("Binder kernel driver disabled at build time"); @@ -649,7 +657,10 @@ status_t Parcel::appendFrom(const Parcel* parcel, size_t offset, size_t len) { const binder_size_t objPos = otherRpcFields->mObjectPositions[i]; if (offset <= objPos && objPos < offset + len) { size_t newDataPos = objPos - offset + startPos; - rpcFields->mObjectPositions.push_back(newDataPos); + rpcFields->mObjectPositions + .insert(std::upper_bound(rpcFields->mObjectPositions.begin(), + rpcFields->mObjectPositions.end(), newDataPos), + newDataPos); mDataPos = newDataPos; int32_t objectType; @@ -1594,7 +1605,10 @@ status_t Parcel::writeFileDescriptor(int fd, bool takeOwnership) { if (status_t err = writeInt32(rpcFields->mFds->size()); err != OK) { return err; } - rpcFields->mObjectPositions.push_back(dataPos); + rpcFields->mObjectPositions + .insert(std::upper_bound(rpcFields->mObjectPositions.begin(), + rpcFields->mObjectPositions.end(), dataPos), + dataPos); rpcFields->mFds->push_back(std::move(fdVariant)); return OK; } @@ -1802,6 +1816,8 @@ restart_write: kernelFields->mObjects[kernelFields->mObjectsSize] = mDataPos; acquire_object(ProcessState::self(), val, this, true /*tagFds*/); kernelFields->mObjectsSize++; + // Clear sorted flag if we aren't appending to the end. + kernelFields->mObjectsSorted &= mDataPos == mDataSize; } return finishWrite(sizeof(flat_binder_object)); diff --git a/libs/binder/tests/binderParcelUnitTest.cpp b/libs/binder/tests/binderParcelUnitTest.cpp index a71da3f384..c4b3299428 100644 --- a/libs/binder/tests/binderParcelUnitTest.cpp +++ b/libs/binder/tests/binderParcelUnitTest.cpp @@ -26,6 +26,7 @@ using android::IPCThreadState; using android::NO_ERROR; using android::OK; using android::Parcel; +using android::PERMISSION_DENIED; using android::sp; using android::status_t; using android::String16; @@ -356,6 +357,16 @@ TEST(Parcel, AppendWithFdPartial) { ASSERT_NE(-1, p1.readFileDescriptor()); } +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& a, const std::function& b) { -- cgit v1.2.3-59-g8ed1b