Skip to content

Commit f2163b8

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). Flag: EXEMPT bugfix Ignore-AOSP-First: security fix Bug: 239222407, 359179312 Test: atest binderLibTest Change-Id: Iadf7e2e98e3eb97c56ec2fed2b49d1e6492af9a3
1 parent 886c2c0 commit f2163b8

3 files changed

Lines changed: 212 additions & 13 deletions

File tree

‎libs/binder/Parcel.cpp‎

Lines changed: 55 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -2656,14 +2656,14 @@ const flat_binder_object* Parcel::readObject(bool nullMetaData) const
26562656
}
26572657
#endif // BINDER_WITH_KERNEL_IPC
26582658

2659-
void Parcel::closeFileDescriptors() {
2659+
void Parcel::closeFileDescriptors(size_t newObjectsSize) {
26602660
if (auto* kernelFields = maybeKernelFields()) {
26612661
#ifdef BINDER_WITH_KERNEL_IPC
26622662
size_t i = kernelFields->mObjectsSize;
26632663
if (i > 0) {
26642664
// ALOGI("Closing file descriptors for %zu objects...", i);
26652665
}
2666-
while (i > 0) {
2666+
while (i > newObjectsSize) {
26672667
i--;
26682668
const flat_binder_object* flat =
26692669
reinterpret_cast<flat_binder_object*>(mData + kernelFields->mObjects[i]);
@@ -2674,6 +2674,7 @@ void Parcel::closeFileDescriptors() {
26742674
}
26752675
}
26762676
#else // BINDER_WITH_KERNEL_IPC
2677+
(void)newObjectsSize;
26772678
LOG_ALWAYS_FATAL("Binder kernel driver disabled at build time");
26782679
#endif // BINDER_WITH_KERNEL_IPC
26792680
} else if (auto* rpcFields = maybeRpcFields()) {
@@ -2898,7 +2899,7 @@ void Parcel::freeDataNoInit()
28982899
//ALOGI("Freeing data ref of %p (pid=%d)", this, getpid());
28992900
auto* kernelFields = maybeKernelFields();
29002901
// Close FDs before freeing, otherwise they will leak for kernel binder.
2901-
closeFileDescriptors();
2902+
closeFileDescriptors(/*newObjectsSize=*/0);
29022903
mOwner(mData, mDataSize, kernelFields ? kernelFields->mObjects : nullptr,
29032904
kernelFields ? kernelFields->mObjectsSize : 0);
29042905
} else {
@@ -3035,13 +3036,38 @@ status_t Parcel::continueWrite(size_t desired)
30353036
objectsSize = 0;
30363037
} else {
30373038
if (kernelFields) {
3039+
#ifdef BINDER_WITH_KERNEL_IPC
3040+
validateReadData(mDataSize); // hack to sort the objects
30383041
while (objectsSize > 0) {
3039-
if (kernelFields->mObjects[objectsSize - 1] < desired) break;
3042+
if (kernelFields->mObjects[objectsSize - 1] + sizeof(flat_binder_object) <=
3043+
desired)
3044+
break;
30403045
objectsSize--;
30413046
}
3047+
#endif // BINDER_WITH_KERNEL_IPC
30423048
} else {
30433049
while (objectsSize > 0) {
3044-
if (rpcFields->mObjectPositions[objectsSize - 1] < desired) break;
3050+
// Object size varies by type.
3051+
uint32_t pos = rpcFields->mObjectPositions[objectsSize - 1];
3052+
size_t size = sizeof(RpcFields::ObjectType);
3053+
uint32_t minObjectEnd;
3054+
if (__builtin_add_overflow(pos, sizeof(RpcFields::ObjectType), &minObjectEnd) ||
3055+
minObjectEnd > mDataSize) {
3056+
return BAD_VALUE;
3057+
}
3058+
const auto type = *reinterpret_cast<const RpcFields::ObjectType*>(mData + pos);
3059+
switch (type) {
3060+
case RpcFields::TYPE_BINDER_NULL:
3061+
break;
3062+
case RpcFields::TYPE_BINDER:
3063+
size += sizeof(uint64_t); // address
3064+
break;
3065+
case RpcFields::TYPE_NATIVE_FILE_DESCRIPTOR:
3066+
size += sizeof(int32_t); // fd index
3067+
break;
3068+
}
3069+
3070+
if (pos + size <= desired) break;
30453071
objectsSize--;
30463072
}
30473073
}
@@ -3090,15 +3116,24 @@ status_t Parcel::continueWrite(size_t desired)
30903116
if (mData) {
30913117
memcpy(data, mData, mDataSize < desired ? mDataSize : desired);
30923118
}
3119+
#ifdef BINDER_WITH_KERNEL_IPC
30933120
if (objects && kernelFields && kernelFields->mObjects) {
30943121
memcpy(objects, kernelFields->mObjects, objectsSize * sizeof(binder_size_t));
3122+
// All FDs are owned when `mOwner`, even when `cookie == 0`. When
3123+
// we switch to `!mOwner`, we need to explicitly mark the FDs as
3124+
// owned.
3125+
for (size_t i = 0; i < objectsSize; i++) {
3126+
flat_binder_object* flat = reinterpret_cast<flat_binder_object*>(data + objects[i]);
3127+
if (flat->hdr.type == BINDER_TYPE_FD) {
3128+
flat->cookie = 1;
3129+
}
3130+
}
30953131
}
30963132
// ALOGI("Freeing data ref of %p (pid=%d)", this, getpid());
30973133
if (kernelFields) {
3098-
// TODO(b/239222407): This seems wrong. We should only free FDs when
3099-
// they are in a truncated section of the parcel.
3100-
closeFileDescriptors();
3134+
closeFileDescriptors(objectsSize);
31013135
}
3136+
#endif // BINDER_WITH_KERNEL_IPC
31023137
mOwner(mData, mDataSize, kernelFields ? kernelFields->mObjects : nullptr,
31033138
kernelFields ? kernelFields->mObjectsSize : 0);
31043139
mOwner = nullptr;
@@ -3225,11 +3260,19 @@ status_t Parcel::truncateRpcObjects(size_t newObjectsSize) {
32253260
}
32263261
while (rpcFields->mObjectPositions.size() > newObjectsSize) {
32273262
uint32_t pos = rpcFields->mObjectPositions.back();
3228-
rpcFields->mObjectPositions.pop_back();
3263+
uint32_t minObjectEnd;
3264+
if (__builtin_add_overflow(pos, sizeof(RpcFields::ObjectType), &minObjectEnd) ||
3265+
minObjectEnd > mDataSize) {
3266+
return BAD_VALUE;
3267+
}
32293268
const auto type = *reinterpret_cast<const RpcFields::ObjectType*>(mData + pos);
32303269
if (type == RpcFields::TYPE_NATIVE_FILE_DESCRIPTOR) {
3231-
const auto fdIndex =
3232-
*reinterpret_cast<const int32_t*>(mData + pos + sizeof(RpcFields::ObjectType));
3270+
uint32_t objectEnd;
3271+
if (__builtin_add_overflow(minObjectEnd, sizeof(int32_t), &objectEnd) ||
3272+
objectEnd > mDataSize) {
3273+
return BAD_VALUE;
3274+
}
3275+
const auto fdIndex = *reinterpret_cast<const int32_t*>(mData + minObjectEnd);
32333276
if (rpcFields->mFds == nullptr || fdIndex < 0 ||
32343277
static_cast<size_t>(fdIndex) >= rpcFields->mFds->size()) {
32353278
ALOGE("RPC Parcel contains invalid file descriptor index. index=%d fd_count=%zu",
@@ -3239,6 +3282,7 @@ status_t Parcel::truncateRpcObjects(size_t newObjectsSize) {
32393282
// In practice, this always removes the last element.
32403283
rpcFields->mFds->erase(rpcFields->mFds->begin() + fdIndex);
32413284
}
3285+
rpcFields->mObjectPositions.pop_back();
32423286
}
32433287
return OK;
32443288
}

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

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -648,8 +648,8 @@ class Parcel {
648648
LIBBINDER_EXPORTED void print(std::ostream& to, uint32_t flags = 0) const;
649649

650650
private:
651-
// Explicitly close all file descriptors in the parcel.
652-
void closeFileDescriptors();
651+
// Close all file descriptors in the parcel at object positions >= newObjectsSize.
652+
void closeFileDescriptors(size_t newObjectsSize);
653653

654654
// `objects` and `objectsSize` always 0 for RPC Parcels.
655655
typedef void (*release_func)(const uint8_t* data, size_t dataSize, const binder_size_t* objects,

‎libs/binder/tests/binderLibTest.cpp‎

Lines changed: 155 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -46,6 +46,7 @@
4646

4747
#include <linux/sched.h>
4848
#include <sys/epoll.h>
49+
#include <sys/mman.h>
4950
#include <sys/prctl.h>
5051
#include <sys/socket.h>
5152
#include <sys/un.h>
@@ -110,6 +111,8 @@ enum BinderLibTestTranscationCode {
110111
BINDER_LIB_TEST_LINK_DEATH_TRANSACTION,
111112
BINDER_LIB_TEST_WRITE_FILE_TRANSACTION,
112113
BINDER_LIB_TEST_WRITE_PARCEL_FILE_DESCRIPTOR_TRANSACTION,
114+
BINDER_LIB_TEST_GET_FILE_DESCRIPTORS_OWNED_TRANSACTION,
115+
BINDER_LIB_TEST_GET_FILE_DESCRIPTORS_UNOWNED_TRANSACTION,
113116
BINDER_LIB_TEST_EXIT_TRANSACTION,
114117
BINDER_LIB_TEST_DELAYED_EXIT_TRANSACTION,
115118
BINDER_LIB_TEST_GET_PTR_SIZE_TRANSACTION,
@@ -536,6 +539,30 @@ class TestDeathRecipient : public IBinder::DeathRecipient, public BinderLibTestE
536539
};
537540
};
538541

542+
ssize_t countFds() {
543+
return std::distance(std::filesystem::directory_iterator("/proc/self/fd"),
544+
std::filesystem::directory_iterator{});
545+
}
546+
547+
struct FdLeakDetector {
548+
int startCount;
549+
550+
FdLeakDetector() {
551+
// This log statement is load bearing. We have to log something before
552+
// counting FDs to make sure the logging system is initialized, otherwise
553+
// the sockets it opens will look like a leak.
554+
ALOGW("FdLeakDetector counting FDs.");
555+
startCount = countFds();
556+
}
557+
~FdLeakDetector() {
558+
int endCount = countFds();
559+
if (startCount != endCount) {
560+
ADD_FAILURE() << "fd count changed (" << startCount << " -> " << endCount
561+
<< ") fd leak?";
562+
}
563+
}
564+
};
565+
539566
TEST_F(BinderLibTest, CannotUseBinderAfterFork) {
540567
// EXPECT_DEATH works by forking the process
541568
EXPECT_DEATH({ ProcessState::self(); }, "libbinder ProcessState can not be used after fork");
@@ -1175,6 +1202,100 @@ TEST_F(BinderLibTest, PassParcelFileDescriptor) {
11751202
EXPECT_EQ(0, read(read_end.get(), readbuf.data(), datasize));
11761203
}
11771204

1205+
TEST_F(BinderLibTest, RecvOwnedFileDescriptors) {
1206+
FdLeakDetector fd_leak_detector;
1207+
1208+
Parcel data;
1209+
Parcel reply;
1210+
EXPECT_EQ(NO_ERROR,
1211+
m_server->transact(BINDER_LIB_TEST_GET_FILE_DESCRIPTORS_OWNED_TRANSACTION, data,
1212+
&reply));
1213+
unique_fd a, b;
1214+
EXPECT_EQ(OK, reply.readUniqueFileDescriptor(&a));
1215+
EXPECT_EQ(OK, reply.readUniqueFileDescriptor(&b));
1216+
}
1217+
1218+
// Used to trigger fdsan error (b/239222407).
1219+
TEST_F(BinderLibTest, RecvOwnedFileDescriptorsAndWriteInt) {
1220+
GTEST_SKIP() << "triggers fdsan false positive: b/370824489";
1221+
1222+
FdLeakDetector fd_leak_detector;
1223+
1224+
Parcel data;
1225+
Parcel reply;
1226+
EXPECT_EQ(NO_ERROR,
1227+
m_server->transact(BINDER_LIB_TEST_GET_FILE_DESCRIPTORS_OWNED_TRANSACTION, data,
1228+
&reply));
1229+
reply.setDataPosition(reply.dataSize());
1230+
reply.writeInt32(0);
1231+
reply.setDataPosition(0);
1232+
unique_fd a, b;
1233+
EXPECT_EQ(OK, reply.readUniqueFileDescriptor(&a));
1234+
EXPECT_EQ(OK, reply.readUniqueFileDescriptor(&b));
1235+
}
1236+
1237+
// Used to trigger fdsan error (b/239222407).
1238+
TEST_F(BinderLibTest, RecvOwnedFileDescriptorsAndTruncate) {
1239+
GTEST_SKIP() << "triggers fdsan false positive: b/370824489";
1240+
1241+
FdLeakDetector fd_leak_detector;
1242+
1243+
Parcel data;
1244+
Parcel reply;
1245+
EXPECT_EQ(NO_ERROR,
1246+
m_server->transact(BINDER_LIB_TEST_GET_FILE_DESCRIPTORS_OWNED_TRANSACTION, data,
1247+
&reply));
1248+
reply.setDataSize(reply.dataSize() - sizeof(flat_binder_object));
1249+
unique_fd a, b;
1250+
EXPECT_EQ(OK, reply.readUniqueFileDescriptor(&a));
1251+
EXPECT_EQ(BAD_TYPE, reply.readUniqueFileDescriptor(&b));
1252+
}
1253+
1254+
TEST_F(BinderLibTest, RecvUnownedFileDescriptors) {
1255+
FdLeakDetector fd_leak_detector;
1256+
1257+
Parcel data;
1258+
Parcel reply;
1259+
EXPECT_EQ(NO_ERROR,
1260+
m_server->transact(BINDER_LIB_TEST_GET_FILE_DESCRIPTORS_UNOWNED_TRANSACTION, data,
1261+
&reply));
1262+
unique_fd a, b;
1263+
EXPECT_EQ(OK, reply.readUniqueFileDescriptor(&a));
1264+
EXPECT_EQ(OK, reply.readUniqueFileDescriptor(&b));
1265+
}
1266+
1267+
// Used to trigger fdsan error (b/239222407).
1268+
TEST_F(BinderLibTest, RecvUnownedFileDescriptorsAndWriteInt) {
1269+
FdLeakDetector fd_leak_detector;
1270+
1271+
Parcel data;
1272+
Parcel reply;
1273+
EXPECT_EQ(NO_ERROR,
1274+
m_server->transact(BINDER_LIB_TEST_GET_FILE_DESCRIPTORS_UNOWNED_TRANSACTION, data,
1275+
&reply));
1276+
reply.setDataPosition(reply.dataSize());
1277+
reply.writeInt32(0);
1278+
reply.setDataPosition(0);
1279+
unique_fd a, b;
1280+
EXPECT_EQ(OK, reply.readUniqueFileDescriptor(&a));
1281+
EXPECT_EQ(OK, reply.readUniqueFileDescriptor(&b));
1282+
}
1283+
1284+
// Used to trigger fdsan error (b/239222407).
1285+
TEST_F(BinderLibTest, RecvUnownedFileDescriptorsAndTruncate) {
1286+
FdLeakDetector fd_leak_detector;
1287+
1288+
Parcel data;
1289+
Parcel reply;
1290+
EXPECT_EQ(NO_ERROR,
1291+
m_server->transact(BINDER_LIB_TEST_GET_FILE_DESCRIPTORS_UNOWNED_TRANSACTION, data,
1292+
&reply));
1293+
reply.setDataSize(reply.dataSize() - sizeof(flat_binder_object));
1294+
unique_fd a, b;
1295+
EXPECT_EQ(OK, reply.readUniqueFileDescriptor(&a));
1296+
EXPECT_EQ(BAD_TYPE, reply.readUniqueFileDescriptor(&b));
1297+
}
1298+
11781299
TEST_F(BinderLibTest, PromoteLocal) {
11791300
sp<IBinder> strong = new BBinder();
11801301
wp<IBinder> weak = strong;
@@ -2224,6 +2345,40 @@ class BinderLibTestService : public BBinder {
22242345
if (ret != size) return UNKNOWN_ERROR;
22252346
return NO_ERROR;
22262347
}
2348+
case BINDER_LIB_TEST_GET_FILE_DESCRIPTORS_OWNED_TRANSACTION: {
2349+
unique_fd fd1(memfd_create("memfd1", MFD_CLOEXEC));
2350+
if (!fd1.ok()) {
2351+
PLOGE("memfd_create failed");
2352+
return UNKNOWN_ERROR;
2353+
}
2354+
unique_fd fd2(memfd_create("memfd2", MFD_CLOEXEC));
2355+
if (!fd2.ok()) {
2356+
PLOGE("memfd_create failed");
2357+
return UNKNOWN_ERROR;
2358+
}
2359+
status_t ret;
2360+
ret = reply->writeFileDescriptor(fd1.release(), true);
2361+
if (ret != NO_ERROR) {
2362+
return ret;
2363+
}
2364+
ret = reply->writeFileDescriptor(fd2.release(), true);
2365+
if (ret != NO_ERROR) {
2366+
return ret;
2367+
}
2368+
return NO_ERROR;
2369+
}
2370+
case BINDER_LIB_TEST_GET_FILE_DESCRIPTORS_UNOWNED_TRANSACTION: {
2371+
status_t ret;
2372+
ret = reply->writeFileDescriptor(STDOUT_FILENO, false);
2373+
if (ret != NO_ERROR) {
2374+
return ret;
2375+
}
2376+
ret = reply->writeFileDescriptor(STDERR_FILENO, false);
2377+
if (ret != NO_ERROR) {
2378+
return ret;
2379+
}
2380+
return NO_ERROR;
2381+
}
22272382
case BINDER_LIB_TEST_DELAYED_EXIT_TRANSACTION:
22282383
alarm(10);
22292384
return NO_ERROR;

0 commit comments

Comments
 (0)