Skip to content

Commit e32c1ab

Browse files
fkm3fkm3
committed
binder: fix FD handling in continueWrite
Only close FDs within the truncated part of the parcel. This change also fixes a bug where a parcel truncated into the middle of an object would not properly free that object. That could have resulted in an OOB access in `Parcel::truncateRpcObjects`, so more bounds checking is added. The new tests show how to reproduce the bug by appending to or partially truncating Parcels owned by the kernel. Two cases are disabled because of a bug in the Parcel fdsan code (b/370824489). Cherry-pick notes: Add truncateFileDescriptors method instead of modifying closeFileDescriptors to avoid API change errors. Tweaked the test to support older C++ and android-base libs. Flag: EXEMPT bugfix Ignore-AOSP-First: security fix Bug: 239222407, 359179312 Test: atest binderLibTest Merged-In: Iadf7e2e98e3eb97c56ec2fed2b49d1e6492af9a3 Change-Id: Iadf7e2e98e3eb97c56ec2fed2b49d1e6492af9a3
1 parent 8e2ca26 commit e32c1ab

3 files changed

Lines changed: 222 additions & 9 deletions

File tree

‎libs/binder/Parcel.cpp‎

Lines changed: 57 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -2505,13 +2505,17 @@ const flat_binder_object* Parcel::readObject(bool nullMetaData) const
25052505
#endif // BINDER_WITH_KERNEL_IPC
25062506

25072507
void Parcel::closeFileDescriptors() {
2508+
truncateFileDescriptors(0);
2509+
}
2510+
2511+
void Parcel::truncateFileDescriptors(size_t newObjectsSize) {
25082512
if (auto* kernelFields = maybeKernelFields()) {
25092513
#ifdef BINDER_WITH_KERNEL_IPC
25102514
size_t i = kernelFields->mObjectsSize;
25112515
if (i > 0) {
25122516
// ALOGI("Closing file descriptors for %zu objects...", i);
25132517
}
2514-
while (i > 0) {
2518+
while (i > newObjectsSize) {
25152519
i--;
25162520
const flat_binder_object* flat =
25172521
reinterpret_cast<flat_binder_object*>(mData + kernelFields->mObjects[i]);
@@ -2521,6 +2525,7 @@ void Parcel::closeFileDescriptors() {
25212525
}
25222526
}
25232527
#else // BINDER_WITH_KERNEL_IPC
2528+
(void)newObjectsSize;
25242529
LOG_ALWAYS_FATAL("Binder kernel driver disabled at build time");
25252530
#endif // BINDER_WITH_KERNEL_IPC
25262531
} else if (auto* rpcFields = maybeRpcFields()) {
@@ -2870,13 +2875,38 @@ status_t Parcel::continueWrite(size_t desired)
28702875
objectsSize = 0;
28712876
} else {
28722877
if (kernelFields) {
2878+
#ifdef BINDER_WITH_KERNEL_IPC
2879+
validateReadData(mDataSize); // hack to sort the objects
28732880
while (objectsSize > 0) {
2874-
if (kernelFields->mObjects[objectsSize - 1] < desired) break;
2881+
if (kernelFields->mObjects[objectsSize - 1] + sizeof(flat_binder_object) <=
2882+
desired)
2883+
break;
28752884
objectsSize--;
28762885
}
2886+
#endif // BINDER_WITH_KERNEL_IPC
28772887
} else {
28782888
while (objectsSize > 0) {
2879-
if (rpcFields->mObjectPositions[objectsSize - 1] < desired) break;
2889+
// Object size varies by type.
2890+
uint32_t pos = rpcFields->mObjectPositions[objectsSize - 1];
2891+
size_t size = sizeof(RpcFields::ObjectType);
2892+
uint32_t minObjectEnd;
2893+
if (__builtin_add_overflow(pos, sizeof(RpcFields::ObjectType), &minObjectEnd) ||
2894+
minObjectEnd > mDataSize) {
2895+
return BAD_VALUE;
2896+
}
2897+
const auto type = *reinterpret_cast<const RpcFields::ObjectType*>(mData + pos);
2898+
switch (type) {
2899+
case RpcFields::TYPE_BINDER_NULL:
2900+
break;
2901+
case RpcFields::TYPE_BINDER:
2902+
size += sizeof(uint64_t); // address
2903+
break;
2904+
case RpcFields::TYPE_NATIVE_FILE_DESCRIPTOR:
2905+
size += sizeof(int32_t); // fd index
2906+
break;
2907+
}
2908+
2909+
if (pos + size <= desired) break;
28802910
objectsSize--;
28812911
}
28822912
}
@@ -2925,15 +2955,24 @@ status_t Parcel::continueWrite(size_t desired)
29252955
if (mData) {
29262956
memcpy(data, mData, mDataSize < desired ? mDataSize : desired);
29272957
}
2958+
#ifdef BINDER_WITH_KERNEL_IPC
29282959
if (objects && kernelFields && kernelFields->mObjects) {
29292960
memcpy(objects, kernelFields->mObjects, objectsSize * sizeof(binder_size_t));
2961+
// All FDs are owned when `mOwner`, even when `cookie == 0`. When
2962+
// we switch to `!mOwner`, we need to explicitly mark the FDs as
2963+
// owned.
2964+
for (size_t i = 0; i < objectsSize; i++) {
2965+
flat_binder_object* flat = reinterpret_cast<flat_binder_object*>(data + objects[i]);
2966+
if (flat->hdr.type == BINDER_TYPE_FD) {
2967+
flat->cookie = 1;
2968+
}
2969+
}
29302970
}
29312971
// ALOGI("Freeing data ref of %p (pid=%d)", this, getpid());
29322972
if (kernelFields) {
2933-
// TODO(b/239222407): This seems wrong. We should only free FDs when
2934-
// they are in a truncated section of the parcel.
2935-
closeFileDescriptors();
2973+
truncateFileDescriptors(objectsSize);
29362974
}
2975+
#endif // BINDER_WITH_KERNEL_IPC
29372976
mOwner(mData, mDataSize, kernelFields ? kernelFields->mObjects : nullptr,
29382977
kernelFields ? kernelFields->mObjectsSize : 0);
29392978
mOwner = nullptr;
@@ -3060,11 +3099,19 @@ status_t Parcel::truncateRpcObjects(size_t newObjectsSize) {
30603099
}
30613100
while (rpcFields->mObjectPositions.size() > newObjectsSize) {
30623101
uint32_t pos = rpcFields->mObjectPositions.back();
3063-
rpcFields->mObjectPositions.pop_back();
3102+
uint32_t minObjectEnd;
3103+
if (__builtin_add_overflow(pos, sizeof(RpcFields::ObjectType), &minObjectEnd) ||
3104+
minObjectEnd > mDataSize) {
3105+
return BAD_VALUE;
3106+
}
30643107
const auto type = *reinterpret_cast<const RpcFields::ObjectType*>(mData + pos);
30653108
if (type == RpcFields::TYPE_NATIVE_FILE_DESCRIPTOR) {
3066-
const auto fdIndex =
3067-
*reinterpret_cast<const int32_t*>(mData + pos + sizeof(RpcFields::ObjectType));
3109+
uint32_t objectEnd;
3110+
if (__builtin_add_overflow(minObjectEnd, sizeof(int32_t), &objectEnd) ||
3111+
objectEnd > mDataSize) {
3112+
return BAD_VALUE;
3113+
}
3114+
const auto fdIndex = *reinterpret_cast<const int32_t*>(mData + minObjectEnd);
30683115
if (rpcFields->mFds == nullptr || fdIndex < 0 ||
30693116
static_cast<size_t>(fdIndex) >= rpcFields->mFds->size()) {
30703117
ALOGE("RPC Parcel contains invalid file descriptor index. index=%d fd_count=%zu",
@@ -3074,6 +3121,7 @@ status_t Parcel::truncateRpcObjects(size_t newObjectsSize) {
30743121
// In practice, this always removes the last element.
30753122
rpcFields->mFds->erase(rpcFields->mFds->begin() + fdIndex);
30763123
}
3124+
rpcFields->mObjectPositions.pop_back();
30773125
}
30783126
return OK;
30793127
}

‎libs/binder/include/binder/Parcel.h‎

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -602,6 +602,9 @@ class Parcel {
602602
void print(std::ostream& to, uint32_t flags = 0) const;
603603

604604
private:
605+
// Close all file descriptors in the parcel at object positions >= newObjectsSize.
606+
__attribute__((__visibility__("hidden"))) void truncateFileDescriptors(size_t newObjectsSize);
607+
605608
// `objects` and `objectsSize` always 0 for RPC Parcels.
606609
typedef void (*release_func)(const uint8_t* data, size_t dataSize, const binder_size_t* objects,
607610
size_t objectsSize);

‎libs/binder/tests/binderLibTest.cpp‎

Lines changed: 162 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -27,6 +27,7 @@
2727
#include <gmock/gmock.h>
2828
#include <gtest/gtest.h>
2929

30+
#include <android-base/logging.h>
3031
#include <android-base/properties.h>
3132
#include <android-base/result-gmock.h>
3233
#include <android-base/result.h>
@@ -43,6 +44,7 @@
4344

4445
#include <linux/sched.h>
4546
#include <sys/epoll.h>
47+
#include <sys/mman.h>
4648
#include <sys/prctl.h>
4749
#include <sys/socket.h>
4850
#include <sys/un.h>
@@ -57,6 +59,7 @@ using namespace std::string_literals;
5759
using namespace std::chrono_literals;
5860
using android::base::testing::HasValue;
5961
using android::base::testing::Ok;
62+
using android::base::unique_fd;
6063
using testing::ExplainMatchResult;
6164
using testing::Matcher;
6265
using testing::Not;
@@ -106,6 +109,8 @@ enum BinderLibTestTranscationCode {
106109
BINDER_LIB_TEST_LINK_DEATH_TRANSACTION,
107110
BINDER_LIB_TEST_WRITE_FILE_TRANSACTION,
108111
BINDER_LIB_TEST_WRITE_PARCEL_FILE_DESCRIPTOR_TRANSACTION,
112+
BINDER_LIB_TEST_GET_FILE_DESCRIPTORS_OWNED_TRANSACTION,
113+
BINDER_LIB_TEST_GET_FILE_DESCRIPTORS_UNOWNED_TRANSACTION,
109114
BINDER_LIB_TEST_EXIT_TRANSACTION,
110115
BINDER_LIB_TEST_DELAYED_EXIT_TRANSACTION,
111116
BINDER_LIB_TEST_GET_PTR_SIZE_TRANSACTION,
@@ -445,6 +450,35 @@ class TestDeathRecipient : public IBinder::DeathRecipient, public BinderLibTestE
445450
};
446451
};
447452

453+
ssize_t countFds() {
454+
DIR* dir = opendir("/proc/self/fd/");
455+
if (dir == nullptr) return -1;
456+
ssize_t ret = 0;
457+
dirent* ent;
458+
while ((ent = readdir(dir)) != nullptr) ret++;
459+
closedir(dir);
460+
return ret;
461+
}
462+
463+
struct FdLeakDetector {
464+
int startCount;
465+
466+
FdLeakDetector() {
467+
// This log statement is load bearing. We have to log something before
468+
// counting FDs to make sure the logging system is initialized, otherwise
469+
// the sockets it opens will look like a leak.
470+
ALOGW("FdLeakDetector counting FDs.");
471+
startCount = countFds();
472+
}
473+
~FdLeakDetector() {
474+
int endCount = countFds();
475+
if (startCount != endCount) {
476+
ADD_FAILURE() << "fd count changed (" << startCount << " -> " << endCount
477+
<< ") fd leak?";
478+
}
479+
}
480+
};
481+
448482
TEST_F(BinderLibTest, CannotUseBinderAfterFork) {
449483
// EXPECT_DEATH works by forking the process
450484
EXPECT_DEATH({ ProcessState::self(); }, "libbinder ProcessState can not be used after fork");
@@ -872,6 +906,100 @@ TEST_F(BinderLibTest, PassParcelFileDescriptor) {
872906
EXPECT_EQ(0, read(read_end.get(), readbuf.data(), datasize));
873907
}
874908

909+
TEST_F(BinderLibTest, RecvOwnedFileDescriptors) {
910+
FdLeakDetector fd_leak_detector;
911+
912+
Parcel data;
913+
Parcel reply;
914+
EXPECT_EQ(NO_ERROR,
915+
m_server->transact(BINDER_LIB_TEST_GET_FILE_DESCRIPTORS_OWNED_TRANSACTION, data,
916+
&reply));
917+
unique_fd a, b;
918+
EXPECT_EQ(OK, reply.readUniqueFileDescriptor(&a));
919+
EXPECT_EQ(OK, reply.readUniqueFileDescriptor(&b));
920+
}
921+
922+
// Used to trigger fdsan error (b/239222407).
923+
TEST_F(BinderLibTest, RecvOwnedFileDescriptorsAndWriteInt) {
924+
GTEST_SKIP() << "triggers fdsan false positive: b/370824489";
925+
926+
FdLeakDetector fd_leak_detector;
927+
928+
Parcel data;
929+
Parcel reply;
930+
EXPECT_EQ(NO_ERROR,
931+
m_server->transact(BINDER_LIB_TEST_GET_FILE_DESCRIPTORS_OWNED_TRANSACTION, data,
932+
&reply));
933+
reply.setDataPosition(reply.dataSize());
934+
reply.writeInt32(0);
935+
reply.setDataPosition(0);
936+
unique_fd a, b;
937+
EXPECT_EQ(OK, reply.readUniqueFileDescriptor(&a));
938+
EXPECT_EQ(OK, reply.readUniqueFileDescriptor(&b));
939+
}
940+
941+
// Used to trigger fdsan error (b/239222407).
942+
TEST_F(BinderLibTest, RecvOwnedFileDescriptorsAndTruncate) {
943+
GTEST_SKIP() << "triggers fdsan false positive: b/370824489";
944+
945+
FdLeakDetector fd_leak_detector;
946+
947+
Parcel data;
948+
Parcel reply;
949+
EXPECT_EQ(NO_ERROR,
950+
m_server->transact(BINDER_LIB_TEST_GET_FILE_DESCRIPTORS_OWNED_TRANSACTION, data,
951+
&reply));
952+
reply.setDataSize(reply.dataSize() - sizeof(flat_binder_object));
953+
unique_fd a, b;
954+
EXPECT_EQ(OK, reply.readUniqueFileDescriptor(&a));
955+
EXPECT_EQ(BAD_TYPE, reply.readUniqueFileDescriptor(&b));
956+
}
957+
958+
TEST_F(BinderLibTest, RecvUnownedFileDescriptors) {
959+
FdLeakDetector fd_leak_detector;
960+
961+
Parcel data;
962+
Parcel reply;
963+
EXPECT_EQ(NO_ERROR,
964+
m_server->transact(BINDER_LIB_TEST_GET_FILE_DESCRIPTORS_UNOWNED_TRANSACTION, data,
965+
&reply));
966+
unique_fd a, b;
967+
EXPECT_EQ(OK, reply.readUniqueFileDescriptor(&a));
968+
EXPECT_EQ(OK, reply.readUniqueFileDescriptor(&b));
969+
}
970+
971+
// Used to trigger fdsan error (b/239222407).
972+
TEST_F(BinderLibTest, RecvUnownedFileDescriptorsAndWriteInt) {
973+
FdLeakDetector fd_leak_detector;
974+
975+
Parcel data;
976+
Parcel reply;
977+
EXPECT_EQ(NO_ERROR,
978+
m_server->transact(BINDER_LIB_TEST_GET_FILE_DESCRIPTORS_UNOWNED_TRANSACTION, data,
979+
&reply));
980+
reply.setDataPosition(reply.dataSize());
981+
reply.writeInt32(0);
982+
reply.setDataPosition(0);
983+
unique_fd a, b;
984+
EXPECT_EQ(OK, reply.readUniqueFileDescriptor(&a));
985+
EXPECT_EQ(OK, reply.readUniqueFileDescriptor(&b));
986+
}
987+
988+
// Used to trigger fdsan error (b/239222407).
989+
TEST_F(BinderLibTest, RecvUnownedFileDescriptorsAndTruncate) {
990+
FdLeakDetector fd_leak_detector;
991+
992+
Parcel data;
993+
Parcel reply;
994+
EXPECT_EQ(NO_ERROR,
995+
m_server->transact(BINDER_LIB_TEST_GET_FILE_DESCRIPTORS_UNOWNED_TRANSACTION, data,
996+
&reply));
997+
reply.setDataSize(reply.dataSize() - sizeof(flat_binder_object));
998+
unique_fd a, b;
999+
EXPECT_EQ(OK, reply.readUniqueFileDescriptor(&a));
1000+
EXPECT_EQ(BAD_TYPE, reply.readUniqueFileDescriptor(&b));
1001+
}
1002+
8751003
TEST_F(BinderLibTest, PromoteLocal) {
8761004
sp<IBinder> strong = new BBinder();
8771005
wp<IBinder> weak = strong;
@@ -1800,6 +1928,40 @@ class BinderLibTestService : public BBinder {
18001928
if (ret != size) return UNKNOWN_ERROR;
18011929
return NO_ERROR;
18021930
}
1931+
case BINDER_LIB_TEST_GET_FILE_DESCRIPTORS_OWNED_TRANSACTION: {
1932+
unique_fd fd1(memfd_create("memfd1", MFD_CLOEXEC));
1933+
if (!fd1.ok()) {
1934+
PLOG(ERROR) << "memfd_create failed";
1935+
return UNKNOWN_ERROR;
1936+
}
1937+
unique_fd fd2(memfd_create("memfd2", MFD_CLOEXEC));
1938+
if (!fd2.ok()) {
1939+
PLOG(ERROR) << "memfd_create failed";
1940+
return UNKNOWN_ERROR;
1941+
}
1942+
status_t ret;
1943+
ret = reply->writeFileDescriptor(fd1.release(), true);
1944+
if (ret != NO_ERROR) {
1945+
return ret;
1946+
}
1947+
ret = reply->writeFileDescriptor(fd2.release(), true);
1948+
if (ret != NO_ERROR) {
1949+
return ret;
1950+
}
1951+
return NO_ERROR;
1952+
}
1953+
case BINDER_LIB_TEST_GET_FILE_DESCRIPTORS_UNOWNED_TRANSACTION: {
1954+
status_t ret;
1955+
ret = reply->writeFileDescriptor(STDOUT_FILENO, false);
1956+
if (ret != NO_ERROR) {
1957+
return ret;
1958+
}
1959+
ret = reply->writeFileDescriptor(STDERR_FILENO, false);
1960+
if (ret != NO_ERROR) {
1961+
return ret;
1962+
}
1963+
return NO_ERROR;
1964+
}
18031965
case BINDER_LIB_TEST_DELAYED_EXIT_TRANSACTION:
18041966
alarm(10);
18051967
return NO_ERROR;

0 commit comments

Comments
 (0)