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
(cherry picked from https://googleplex-android-review.googlesource.com/q/commit:517f23af4b4acd44d17394e16a6a42709ccb453c)
(cherry picked from commit 0abce000697a6e9dda217766338c7f1149179b98)
(cherry picked from https://googleplex-android-review.googlesource.com/q/commit:e870fdf57b48c0558d905522053185979a7cc12b)
Merged-In: I63715fdd81781aaf04f5fc0cb8bdb74c09d5d807
Change-Id: I63715fdd81781aaf04f5fc0cb8bdb74c09d5d807
diff --git a/libs/binder/Parcel.cpp b/libs/binder/Parcel.cpp
index acf262a..2bc18d0 100644
--- a/libs/binder/Parcel.cpp
+++ b/libs/binder/Parcel.cpp
@@ -424,7 +424,6 @@
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 @@
}
// Count objects in range
- 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;
+ int numObjects = 0;
+ for (int i = 0; i < (int)size; i++) {
+ size_t pos = objects[i];
+ if ((pos >= offset) && (pos + sizeof(flat_binder_object) <= offset + len)) {
+ numObjects++;
}
}
- int numObjects = lastIndex - firstIndex + 1;
+
+ // Make sure we aren't appending over objects.
+ if (status_t status = validateReadData(mDataPos + len); status != OK) {
+ return status;
+ }
if ((mDataSize+len) > mDataCapacity) {
// grow data
@@ -489,8 +490,12 @@
// 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 @@
}
}
}
+ // 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 @@
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));
diff --git a/libs/binder/tests/binderParcelUnitTest.cpp b/libs/binder/tests/binderParcelUnitTest.cpp
index 359c783..6368997 100644
--- a/libs/binder/tests/binderParcelUnitTest.cpp
+++ b/libs/binder/tests/binderParcelUnitTest.cpp
@@ -25,6 +25,7 @@
using android::IPCThreadState;
using android::OK;
using android::Parcel;
+using android::PERMISSION_DENIED;
using android::sp;
using android::status_t;
using android::String16;
@@ -112,6 +113,16 @@
EXPECT_EQ(ret[1], STDIN_FILENO);
}
+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) {
