Skip to content

Commit 8010cbb

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. Large diffs in this branch because it didn't have the disruptive RPC FD support, main diff is that the closeFileDescriptors call is move out of the mOwner implementation. 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 001bb2f commit 8010cbb

5 files changed

Lines changed: 189 additions & 4 deletions

File tree

‎libs/binder/IPCThreadState.cpp‎

Lines changed: 1 addition & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1419,7 +1419,7 @@ status_t IPCThreadState::freeze(pid_t pid, bool enable, uint32_t timeout_ms) {
14191419
return ret;
14201420
}
14211421

1422-
void IPCThreadState::freeBuffer(Parcel* parcel, const uint8_t* data,
1422+
void IPCThreadState::freeBuffer(Parcel* /*parcel*/, const uint8_t* data,
14231423
size_t /*dataSize*/,
14241424
const binder_size_t* /*objects*/,
14251425
size_t /*objectsSize*/)
@@ -1429,7 +1429,6 @@ void IPCThreadState::freeBuffer(Parcel* parcel, const uint8_t* data,
14291429
alog << "Writing BC_FREE_BUFFER for " << data << endl;
14301430
}
14311431
ALOG_ASSERT(data != NULL, "Called with NULL data");
1432-
if (parcel != nullptr) parcel->closeFileDescriptors();
14331432
IPCThreadState* state = self();
14341433
state->mOut.writeInt32(BC_FREE_BUFFER);
14351434
state->mOut.writePointer((uintptr_t)data);

‎libs/binder/Parcel.cpp‎

Lines changed: 18 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -2076,11 +2076,15 @@ const flat_binder_object* Parcel::readObject(bool nullMetaData) const
20762076

20772077
void Parcel::closeFileDescriptors()
20782078
{
2079+
truncateFileDescriptors(0);
2080+
}
2081+
2082+
void Parcel::truncateFileDescriptors(size_t newObjectsSize) {
20792083
size_t i = mObjectsSize;
20802084
if (i > 0) {
20812085
//ALOGI("Closing file descriptors for %zu objects...", i);
20822086
}
2083-
while (i > 0) {
2087+
while (i > newObjectsSize) {
20842088
i--;
20852089
const flat_binder_object* flat
20862090
= reinterpret_cast<flat_binder_object*>(mData+mObjects[i]);
@@ -2228,6 +2232,7 @@ void Parcel::freeDataNoInit()
22282232
if (mOwner) {
22292233
LOG_ALLOC("Parcel %p: freeing other owner data", this);
22302234
//ALOGI("Freeing data ref of %p (pid=%d)", this, getpid());
2235+
closeFileDescriptors();
22312236
mOwner(this, mData, mDataSize, mObjects, mObjectsSize);
22322237
} else {
22332238
LOG_ALLOC("Parcel %p: freeing allocated data", this);
@@ -2343,8 +2348,9 @@ status_t Parcel::continueWrite(size_t desired)
23432348
if (desired == 0) {
23442349
objectsSize = 0;
23452350
} else {
2351+
validateReadData(mDataSize); // hack to sort the objects
23462352
while (objectsSize > 0) {
2347-
if (mObjects[objectsSize-1] < desired)
2353+
if (mObjects[objectsSize-1] + sizeof(flat_binder_object) <= desired)
23482354
break;
23492355
objectsSize--;
23502356
}
@@ -2389,8 +2395,18 @@ status_t Parcel::continueWrite(size_t desired)
23892395
}
23902396
if (objects && mObjects) {
23912397
memcpy(objects, mObjects, objectsSize*sizeof(binder_size_t));
2398+
// All FDs are owned when `mOwner`, even when `cookie == 0`. When
2399+
// we switch to `!mOwner`, we need to explicitly mark the FDs as
2400+
// owned.
2401+
for (size_t i = 0; i < objectsSize; i++) {
2402+
flat_binder_object* flat = reinterpret_cast<flat_binder_object*>(data + objects[i]);
2403+
if (flat->hdr.type == BINDER_TYPE_FD) {
2404+
flat->cookie = 1;
2405+
}
2406+
}
23922407
}
23932408
//ALOGI("Freeing data ref of %p (pid=%d)", this, getpid());
2409+
truncateFileDescriptors(objectsSize);
23942410
mOwner(this, mData, mDataSize, mObjects, mObjectsSize);
23952411
mOwner = nullptr;
23962412

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

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -528,6 +528,9 @@ class Parcel {
528528
void print(TextOutput& to, uint32_t flags = 0) const;
529529

530530
private:
531+
// Close all file descriptors in the parcel at object positions >= newObjectsSize.
532+
__attribute__((__visibility__("hidden"))) void truncateFileDescriptors(size_t newObjectsSize);
533+
531534
typedef void (*release_func)(Parcel* parcel,
532535
const uint8_t* data, size_t dataSize,
533536
const binder_size_t* objects, size_t objectsSize);

‎libs/binder/tests/Android.bp‎

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -60,7 +60,9 @@ cc_test {
6060
defaults: ["binder_test_defaults"],
6161
srcs: ["binderLibTest.cpp"],
6262
shared_libs: [
63+
"libbase",
6364
"libbinder",
65+
"liblog",
6466
"libutils",
6567
],
6668
static_libs: [
@@ -101,7 +103,9 @@ cc_test {
101103

102104
srcs: ["binderLibTest.cpp"],
103105
shared_libs: [
106+
"libbase",
104107
"libbinder",
108+
"liblog",
105109
"libutils",
106110
],
107111
static_libs: [

‎libs/binder/tests/binderLibTest.cpp‎

Lines changed: 163 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -26,13 +26,16 @@
2626
#include <gmock/gmock.h>
2727
#include <gtest/gtest.h>
2828

29+
#include <android-base/logging.h>
30+
#include <android-base/unique_fd.h>
2931
#include <binder/Binder.h>
3032
#include <binder/IBinder.h>
3133
#include <binder/IPCThreadState.h>
3234
#include <binder/IServiceManager.h>
3335

3436
#include <linux/sched.h>
3537
#include <sys/epoll.h>
38+
#include <sys/mman.h>
3639
#include <sys/prctl.h>
3740

3841
#include "../binder_module.h"
@@ -41,6 +44,7 @@
4144
#define ARRAY_SIZE(array) (sizeof array / sizeof array[0])
4245

4346
using namespace android;
47+
using android::base::unique_fd;
4448
using testing::Not;
4549

4650
// e.g. EXPECT_THAT(expr, StatusEq(OK)) << "additional message";
@@ -85,6 +89,8 @@ enum BinderLibTestTranscationCode {
8589
BINDER_LIB_TEST_LINK_DEATH_TRANSACTION,
8690
BINDER_LIB_TEST_WRITE_FILE_TRANSACTION,
8791
BINDER_LIB_TEST_WRITE_PARCEL_FILE_DESCRIPTOR_TRANSACTION,
92+
BINDER_LIB_TEST_GET_FILE_DESCRIPTORS_OWNED_TRANSACTION,
93+
BINDER_LIB_TEST_GET_FILE_DESCRIPTORS_UNOWNED_TRANSACTION,
8894
BINDER_LIB_TEST_EXIT_TRANSACTION,
8995
BINDER_LIB_TEST_DELAYED_EXIT_TRANSACTION,
9096
BINDER_LIB_TEST_GET_PTR_SIZE_TRANSACTION,
@@ -404,6 +410,35 @@ class TestDeathRecipient : public IBinder::DeathRecipient, public BinderLibTestE
404410
};
405411
};
406412

413+
ssize_t countFds() {
414+
DIR* dir = opendir("/proc/self/fd/");
415+
if (dir == nullptr) return -1;
416+
ssize_t ret = 0;
417+
dirent* ent;
418+
while ((ent = readdir(dir)) != nullptr) ret++;
419+
closedir(dir);
420+
return ret;
421+
}
422+
423+
struct FdLeakDetector {
424+
int startCount;
425+
426+
FdLeakDetector() {
427+
// This log statement is load bearing. We have to log something before
428+
// counting FDs to make sure the logging system is initialized, otherwise
429+
// the sockets it opens will look like a leak.
430+
ALOGW("FdLeakDetector counting FDs.");
431+
startCount = countFds();
432+
}
433+
~FdLeakDetector() {
434+
int endCount = countFds();
435+
if (startCount != endCount) {
436+
ADD_FAILURE() << "fd count changed (" << startCount << " -> " << endCount
437+
<< ") fd leak?";
438+
}
439+
}
440+
};
441+
407442
TEST_F(BinderLibTest, NopTransaction) {
408443
Parcel data, reply;
409444
EXPECT_THAT(m_server->transact(BINDER_LIB_TEST_NOP_TRANSACTION, data, &reply),
@@ -805,6 +840,100 @@ TEST_F(BinderLibTest, PassParcelFileDescriptor) {
805840
EXPECT_EQ(0, read(read_end.get(), readbuf.data(), datasize));
806841
}
807842

843+
TEST_F(BinderLibTest, RecvOwnedFileDescriptors) {
844+
FdLeakDetector fd_leak_detector;
845+
846+
Parcel data;
847+
Parcel reply;
848+
EXPECT_EQ(NO_ERROR,
849+
m_server->transact(BINDER_LIB_TEST_GET_FILE_DESCRIPTORS_OWNED_TRANSACTION, data,
850+
&reply));
851+
unique_fd a, b;
852+
EXPECT_EQ(OK, reply.readUniqueFileDescriptor(&a));
853+
EXPECT_EQ(OK, reply.readUniqueFileDescriptor(&b));
854+
}
855+
856+
// Used to trigger fdsan error (b/239222407).
857+
TEST_F(BinderLibTest, RecvOwnedFileDescriptorsAndWriteInt) {
858+
GTEST_SKIP() << "triggers fdsan false positive: b/370824489";
859+
860+
FdLeakDetector fd_leak_detector;
861+
862+
Parcel data;
863+
Parcel reply;
864+
EXPECT_EQ(NO_ERROR,
865+
m_server->transact(BINDER_LIB_TEST_GET_FILE_DESCRIPTORS_OWNED_TRANSACTION, data,
866+
&reply));
867+
reply.setDataPosition(reply.dataSize());
868+
reply.writeInt32(0);
869+
reply.setDataPosition(0);
870+
unique_fd a, b;
871+
EXPECT_EQ(OK, reply.readUniqueFileDescriptor(&a));
872+
EXPECT_EQ(OK, reply.readUniqueFileDescriptor(&b));
873+
}
874+
875+
// Used to trigger fdsan error (b/239222407).
876+
TEST_F(BinderLibTest, RecvOwnedFileDescriptorsAndTruncate) {
877+
GTEST_SKIP() << "triggers fdsan false positive: b/370824489";
878+
879+
FdLeakDetector fd_leak_detector;
880+
881+
Parcel data;
882+
Parcel reply;
883+
EXPECT_EQ(NO_ERROR,
884+
m_server->transact(BINDER_LIB_TEST_GET_FILE_DESCRIPTORS_OWNED_TRANSACTION, data,
885+
&reply));
886+
reply.setDataSize(reply.dataSize() - sizeof(flat_binder_object));
887+
unique_fd a, b;
888+
EXPECT_EQ(OK, reply.readUniqueFileDescriptor(&a));
889+
EXPECT_EQ(BAD_TYPE, reply.readUniqueFileDescriptor(&b));
890+
}
891+
892+
TEST_F(BinderLibTest, RecvUnownedFileDescriptors) {
893+
FdLeakDetector fd_leak_detector;
894+
895+
Parcel data;
896+
Parcel reply;
897+
EXPECT_EQ(NO_ERROR,
898+
m_server->transact(BINDER_LIB_TEST_GET_FILE_DESCRIPTORS_UNOWNED_TRANSACTION, data,
899+
&reply));
900+
unique_fd a, b;
901+
EXPECT_EQ(OK, reply.readUniqueFileDescriptor(&a));
902+
EXPECT_EQ(OK, reply.readUniqueFileDescriptor(&b));
903+
}
904+
905+
// Used to trigger fdsan error (b/239222407).
906+
TEST_F(BinderLibTest, RecvUnownedFileDescriptorsAndWriteInt) {
907+
FdLeakDetector fd_leak_detector;
908+
909+
Parcel data;
910+
Parcel reply;
911+
EXPECT_EQ(NO_ERROR,
912+
m_server->transact(BINDER_LIB_TEST_GET_FILE_DESCRIPTORS_UNOWNED_TRANSACTION, data,
913+
&reply));
914+
reply.setDataPosition(reply.dataSize());
915+
reply.writeInt32(0);
916+
reply.setDataPosition(0);
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, RecvUnownedFileDescriptorsAndTruncate) {
924+
FdLeakDetector fd_leak_detector;
925+
926+
Parcel data;
927+
Parcel reply;
928+
EXPECT_EQ(NO_ERROR,
929+
m_server->transact(BINDER_LIB_TEST_GET_FILE_DESCRIPTORS_UNOWNED_TRANSACTION, data,
930+
&reply));
931+
reply.setDataSize(reply.dataSize() - sizeof(flat_binder_object));
932+
unique_fd a, b;
933+
EXPECT_EQ(OK, reply.readUniqueFileDescriptor(&a));
934+
EXPECT_EQ(BAD_TYPE, reply.readUniqueFileDescriptor(&b));
935+
}
936+
808937
TEST_F(BinderLibTest, PromoteLocal) {
809938
sp<IBinder> strong = new BBinder();
810939
wp<IBinder> weak = strong;
@@ -1438,6 +1567,40 @@ class BinderLibTestService : public BBinder
14381567
if (ret != size) return UNKNOWN_ERROR;
14391568
return NO_ERROR;
14401569
}
1570+
case BINDER_LIB_TEST_GET_FILE_DESCRIPTORS_OWNED_TRANSACTION: {
1571+
unique_fd fd1(memfd_create("memfd1", MFD_CLOEXEC));
1572+
if (!fd1.ok()) {
1573+
PLOG(ERROR) << "memfd_create failed";
1574+
return UNKNOWN_ERROR;
1575+
}
1576+
unique_fd fd2(memfd_create("memfd2", MFD_CLOEXEC));
1577+
if (!fd2.ok()) {
1578+
PLOG(ERROR) << "memfd_create failed";
1579+
return UNKNOWN_ERROR;
1580+
}
1581+
status_t ret;
1582+
ret = reply->writeFileDescriptor(fd1.release(), true);
1583+
if (ret != NO_ERROR) {
1584+
return ret;
1585+
}
1586+
ret = reply->writeFileDescriptor(fd2.release(), true);
1587+
if (ret != NO_ERROR) {
1588+
return ret;
1589+
}
1590+
return NO_ERROR;
1591+
}
1592+
case BINDER_LIB_TEST_GET_FILE_DESCRIPTORS_UNOWNED_TRANSACTION: {
1593+
status_t ret;
1594+
ret = reply->writeFileDescriptor(STDOUT_FILENO, false);
1595+
if (ret != NO_ERROR) {
1596+
return ret;
1597+
}
1598+
ret = reply->writeFileDescriptor(STDERR_FILENO, false);
1599+
if (ret != NO_ERROR) {
1600+
return ret;
1601+
}
1602+
return NO_ERROR;
1603+
}
14411604
case BINDER_LIB_TEST_DELAYED_EXIT_TRANSACTION:
14421605
alarm(10);
14431606
return NO_ERROR;

0 commit comments

Comments
 (0)