Don't call into system_server for permissions check
With the recently added permission information in traffic controller,
netd can check if the calling process has permission UPDATE_DEVICE_STATS
without calling into system_server. Update the code path and add some
test cases for it.
Bug: 111560570
Bug: 111560739
Test: netd_unit_test, netd_integration_test
Change-Id: I79eee1321f32154e91466f023f7952db23df8494
diff --git a/server/FwmarkServer.cpp b/server/FwmarkServer.cpp
index dc05093..51d5398 100644
--- a/server/FwmarkServer.cpp
+++ b/server/FwmarkServer.cpp
@@ -37,7 +37,6 @@
namespace android {
namespace net {
-constexpr const char *UPDATE_DEVICE_STATS = "android.permission.UPDATE_DEVICE_STATS";
constexpr const char *SYSTEM_SERVER_CONTEXT = "u:r:system_server:s0";
bool isSystemServer(SocketClient* client) {
@@ -58,14 +57,6 @@
return ret;
}
-bool hasUpdateDeviceStatsPermission(SocketClient* client) {
- // If the caller is the system server, allow without any further checks.
- // Otherwise, if the system server's binder thread pool is full, and all the threads are
- // blocked on a thread that's waiting for us to complete, we deadlock. http://b/69389492
- return isSystemServer(client) ||
- checkPermission(String16(UPDATE_DEVICE_STATS), client->getPid(), client->getUid());
-}
-
FwmarkServer::FwmarkServer(NetworkController* networkController, EventReporter* eventReporter,
TrafficController* trafficCtrl)
: SocketListener(SOCKET_NAME, true),
@@ -133,17 +124,11 @@
}
if (command.cmdId == FwmarkCommand::SET_COUNTERSET) {
- if (!hasUpdateDeviceStatsPermission(client)) {
- return -EPERM;
- }
- return mTrafficCtrl->setCounterSet(command.trafficCtrlInfo, command.uid);
+ return mTrafficCtrl->setCounterSet(command.trafficCtrlInfo, command.uid, client->getUid());
}
if (command.cmdId == FwmarkCommand::DELETE_TAGDATA) {
- if (!hasUpdateDeviceStatsPermission(client)) {
- return -EPERM;
- }
- return mTrafficCtrl->deleteTagData(command.trafficCtrlInfo, command.uid);
+ return mTrafficCtrl->deleteTagData(command.trafficCtrlInfo, command.uid, client->getUid());
}
cmsghdr* const cmsgh = CMSG_FIRSTHDR(&message);
@@ -307,10 +292,8 @@
if (static_cast<int>(command.uid) == -1) {
command.uid = client->getUid();
}
- if (command.uid != client->getUid() && !hasUpdateDeviceStatsPermission(client)) {
- return -EPERM;
- }
- return mTrafficCtrl->tagSocket(*socketFd, command.trafficCtrlInfo, command.uid);
+ return mTrafficCtrl->tagSocket(*socketFd, command.trafficCtrlInfo, command.uid,
+ client->getUid());
}
case FwmarkCommand::UNTAG_SOCKET: {
diff --git a/server/TrafficController.cpp b/server/TrafficController.cpp
index 9aaa9fa..a30f9dd 100644
--- a/server/TrafficController.cpp
+++ b/server/TrafficController.cpp
@@ -92,6 +92,10 @@
return matchType;
}
+bool TrafficController::hasUpdateDeviceStatsPermission(uid_t uid) {
+ return mPrivilegedUser.find(uid) != mPrivilegedUser.end();
+}
+
const std::string UidPermissionTypeToString(uint8_t permission) {
std::string permissionType;
FLAG_MSG_TRANS(permissionType, ALLOW_SOCK_CREATE, permission);
@@ -299,7 +303,11 @@
return netdutils::status::ok;
}
-int TrafficController::tagSocket(int sockFd, uint32_t tag, uid_t uid) {
+int TrafficController::tagSocket(int sockFd, uint32_t tag, uid_t uid, uid_t callingUid) {
+ if (uid != callingUid && !hasUpdateDeviceStatsPermission(callingUid)) {
+ return -EPERM;
+ }
+
if (!ebpfSupported) {
if (legacy_tagSocket(sockFd, tag, uid)) return -errno;
return 0;
@@ -337,9 +345,11 @@
return -res.code();
}
-int TrafficController::setCounterSet(int counterSetNum, uid_t uid) {
+int TrafficController::setCounterSet(int counterSetNum, uid_t uid, uid_t callingUid) {
if (counterSetNum < 0 || counterSetNum >= OVERFLOW_COUNTERSET) return -EINVAL;
- Status res;
+
+ if (!hasUpdateDeviceStatsPermission(callingUid)) return -EPERM;
+
if (!ebpfSupported) {
if (legacy_setCounterSet(counterSetNum, uid)) return -errno;
return 0;
@@ -357,7 +367,7 @@
}
}
uint8_t tmpCounterSetNum = (uint8_t)counterSetNum;
- res = mUidCounterSetMap.writeValue(uid, tmpCounterSetNum, BPF_ANY);
+ Status res = mUidCounterSetMap.writeValue(uid, tmpCounterSetNum, BPF_ANY);
if (!isOk(res)) {
ALOGE("Failed to set the counterSet: %s, fd: %d", strerror(res.code()),
mUidCounterSetMap.getMap().get());
@@ -369,7 +379,9 @@
// This method only get called by system_server when an app get uinstalled, it
// is called inside removeUidsLocked() while holding mStatsLock. So it is safe
// to iterate and modify the stats maps.
-int TrafficController::deleteTagData(uint32_t tag, uid_t uid) {
+int TrafficController::deleteTagData(uint32_t tag, uid_t uid, uid_t callingUid) {
+ if (!hasUpdateDeviceStatsPermission(callingUid)) return -EPERM;
+
if (!ebpfSupported) {
if (legacy_deleteTagData(tag, uid)) return -errno;
return 0;
diff --git a/server/TrafficController.h b/server/TrafficController.h
index 263377f..35bea4a 100644
--- a/server/TrafficController.h
+++ b/server/TrafficController.h
@@ -58,7 +58,7 @@
* the spinlock initialized with the map. So the behavior of two modules
* should be the same. No additional lock needed.
*/
- int tagSocket(int sockFd, uint32_t tag, uid_t uid);
+ int tagSocket(int sockFd, uint32_t tag, uid_t uid, uid_t callingUid);
/*
* The untag process is similiar to tag socket and both old qtaguid module and
@@ -70,7 +70,7 @@
/*
* Similiar as above, no external lock required.
*/
- int setCounterSet(int counterSetNum, uid_t uid);
+ int setCounterSet(int counterSetNum, uid_t uid, uid_t callingUid);
/*
* When deleting a tag data, the qtaguid module will grab the spinlock of each
@@ -80,7 +80,7 @@
* each map one by one. And deleting processes are also protected by the
* spinlock of the map. So no additional lock is required.
*/
- int deleteTagData(uint32_t tag, uid_t uid);
+ int deleteTagData(uint32_t tag, uid_t uid, uid_t callingUid);
/*
* Check if the current device have the bpf traffic stats accounting service
@@ -218,6 +218,8 @@
std::set<uid_t> mPrivilegedUser;
UidOwnerMatchType jumpOpToMatch(BandwidthController::IptJumpOp jumpHandling);
+
+ bool hasUpdateDeviceStatsPermission(uid_t uid);
// For testing
friend class TrafficControllerTest;
};
diff --git a/server/TrafficControllerTest.cpp b/server/TrafficControllerTest.cpp
index 7f32988..541738b 100644
--- a/server/TrafficControllerTest.cpp
+++ b/server/TrafficControllerTest.cpp
@@ -124,14 +124,16 @@
mTc.mConfigurationMap.reset(mFakeConfigurationMap.getMap());
mTc.mUidOwnerMap.reset(mFakeUidOwnerMap.getMap());
mTc.mUidPermissionMap.reset(mFakeUidPermissionMap.getMap());
+ mTc.mPrivilegedUser.clear();
}
- int setUpSocketAndTag(int protocol, uint64_t* cookie, uint32_t tag, uid_t uid) {
+ int setUpSocketAndTag(int protocol, uint64_t* cookie, uint32_t tag, uid_t uid,
+ uid_t callingUid) {
int sock = socket(protocol, SOCK_STREAM | SOCK_CLOEXEC, 0);
EXPECT_LE(0, sock);
*cookie = getSocketCookie(sock);
EXPECT_NE(NONEXISTENT_COOKIE, *cookie);
- EXPECT_EQ(0, mTc.tagSocket(sock, tag, uid));
+ EXPECT_EQ(0, mTc.tagSocket(sock, tag, uid, callingUid));
return sock;
}
@@ -257,13 +259,47 @@
}
void expectPrivilegedUserSetEmpty() { EXPECT_TRUE(mTc.mPrivilegedUser.empty()); }
+
+ void addPrivilegedUid(uid_t uid) {
+ std::vector privilegedUid = {uid};
+ mTc.setPermissionForUids(INetd::PERMISSION_UPDATE_DEVICE_STATS, privilegedUid);
+ }
+
+ void removePrivilegedUid(uid_t uid) {
+ std::vector privilegedUid = {uid};
+ mTc.setPermissionForUids(INetd::NO_PERMISSIONS, privilegedUid);
+ }
+
+ void expectFakeStatsUnchanged(uint64_t cookie, uint32_t tag, uint32_t uid,
+ StatsKey tagStatsMapKey) {
+ StatusOr<UidTag> cookieMapResult = mFakeCookieTagMap.readValue(cookie);
+ EXPECT_TRUE(isOk(cookieMapResult));
+ EXPECT_EQ(uid, cookieMapResult.value().uid);
+ EXPECT_EQ(tag, cookieMapResult.value().tag);
+ StatusOr<uint8_t> counterSetResult = mFakeUidCounterSetMap.readValue(uid);
+ EXPECT_TRUE(isOk(counterSetResult));
+ EXPECT_EQ(TEST_COUNTERSET, counterSetResult.value());
+ StatusOr<StatsValue> statsMapResult = mFakeTagStatsMap.readValue(tagStatsMapKey);
+ EXPECT_TRUE(isOk(statsMapResult));
+ EXPECT_EQ((uint64_t)1, statsMapResult.value().rxPackets);
+ EXPECT_EQ((uint64_t)100, statsMapResult.value().rxBytes);
+ tagStatsMapKey.tag = 0;
+ statsMapResult = mFakeUidStatsMap.readValue(tagStatsMapKey);
+ EXPECT_TRUE(isOk(statsMapResult));
+ EXPECT_EQ((uint64_t)1, statsMapResult.value().rxPackets);
+ EXPECT_EQ((uint64_t)100, statsMapResult.value().rxBytes);
+ auto appStatsResult = mFakeAppUidStatsMap.readValue(uid);
+ EXPECT_TRUE(isOk(appStatsResult));
+ EXPECT_EQ((uint64_t)1, appStatsResult.value().rxPackets);
+ EXPECT_EQ((uint64_t)100, appStatsResult.value().rxBytes);
+ }
};
TEST_F(TrafficControllerTest, TestTagSocketV4) {
SKIP_IF_BPF_NOT_SUPPORTED;
uint64_t sockCookie;
- int v4socket = setUpSocketAndTag(AF_INET, &sockCookie, TEST_TAG, TEST_UID);
+ int v4socket = setUpSocketAndTag(AF_INET, &sockCookie, TEST_TAG, TEST_UID, TEST_UID);
expectUidTag(sockCookie, TEST_UID, TEST_TAG);
ASSERT_EQ(0, mTc.untagSocket(v4socket));
expectNoTag(sockCookie);
@@ -274,9 +310,9 @@
SKIP_IF_BPF_NOT_SUPPORTED;
uint64_t sockCookie;
- int v4socket = setUpSocketAndTag(AF_INET, &sockCookie, TEST_TAG, TEST_UID);
+ int v4socket = setUpSocketAndTag(AF_INET, &sockCookie, TEST_TAG, TEST_UID, TEST_UID);
expectUidTag(sockCookie, TEST_UID, TEST_TAG);
- ASSERT_EQ(0, mTc.tagSocket(v4socket, TEST_TAG + 1, TEST_UID + 1));
+ ASSERT_EQ(0, mTc.tagSocket(v4socket, TEST_TAG + 1, TEST_UID + 1, TEST_UID + 1));
expectUidTag(sockCookie, TEST_UID + 1, TEST_TAG + 1);
}
@@ -285,8 +321,8 @@
uint64_t sockCookie1;
uint64_t sockCookie2;
- int v4socket1 = setUpSocketAndTag(AF_INET, &sockCookie1, TEST_TAG, TEST_UID);
- setUpSocketAndTag(AF_INET, &sockCookie2, TEST_TAG, TEST_UID);
+ int v4socket1 = setUpSocketAndTag(AF_INET, &sockCookie1, TEST_TAG, TEST_UID, TEST_UID);
+ setUpSocketAndTag(AF_INET, &sockCookie2, TEST_TAG, TEST_UID, TEST_UID);
expectUidTag(sockCookie1, TEST_UID, TEST_TAG);
expectUidTag(sockCookie2, TEST_UID, TEST_TAG);
ASSERT_EQ(0, mTc.untagSocket(v4socket1));
@@ -299,7 +335,7 @@
SKIP_IF_BPF_NOT_SUPPORTED;
uint64_t sockCookie;
- int v6socket = setUpSocketAndTag(AF_INET6, &sockCookie, TEST_TAG, TEST_UID);
+ int v6socket = setUpSocketAndTag(AF_INET6, &sockCookie, TEST_TAG, TEST_UID, TEST_UID);
expectUidTag(sockCookie, TEST_UID, TEST_TAG);
ASSERT_EQ(0, mTc.untagSocket(v6socket));
expectNoTag(sockCookie);
@@ -310,10 +346,39 @@
SKIP_IF_BPF_NOT_SUPPORTED;
int invalidSocket = -1;
- ASSERT_GT(0, mTc.tagSocket(invalidSocket, TEST_TAG, TEST_UID));
+ ASSERT_GT(0, mTc.tagSocket(invalidSocket, TEST_TAG, TEST_UID, TEST_UID));
expectMapEmpty(mFakeCookieTagMap);
}
+TEST_F(TrafficControllerTest, TestTagSocketWithoutPermission) {
+ SKIP_IF_BPF_NOT_SUPPORTED;
+
+ int sock = socket(AF_INET6, SOCK_STREAM | SOCK_CLOEXEC, 0);
+ ASSERT_NE(-1, sock);
+ ASSERT_EQ(-EPERM, mTc.tagSocket(sock, TEST_TAG, TEST_UID, TEST_UID2));
+ expectMapEmpty(mFakeCookieTagMap);
+}
+
+TEST_F(TrafficControllerTest, TestTagSocketWithPermission) {
+ SKIP_IF_BPF_NOT_SUPPORTED;
+
+ // Grant permission to calling uid.
+ std::vector<uid_t> callingUid = {TEST_UID2};
+ mTc.setPermissionForUids(INetd::PERMISSION_UPDATE_DEVICE_STATS, callingUid);
+
+ // Tag a socket to a different uid other then callingUid.
+ uint64_t sockCookie;
+ int v6socket = setUpSocketAndTag(AF_INET6, &sockCookie, TEST_TAG, TEST_UID, TEST_UID2);
+ expectUidTag(sockCookie, TEST_UID, TEST_TAG);
+ EXPECT_EQ(0, mTc.untagSocket(v6socket));
+ expectNoTag(sockCookie);
+ expectMapEmpty(mFakeCookieTagMap);
+
+ // Clean up the permission
+ mTc.setPermissionForUids(INetd::NO_PERMISSIONS, callingUid);
+ expectPrivilegedUserSetEmpty();
+}
+
TEST_F(TrafficControllerTest, TestUntagInvalidSocket) {
SKIP_IF_BPF_NOT_SUPPORTED;
@@ -327,12 +392,23 @@
TEST_F(TrafficControllerTest, TestSetCounterSet) {
SKIP_IF_BPF_NOT_SUPPORTED;
- ASSERT_EQ(0, mTc.setCounterSet(TEST_COUNTERSET, TEST_UID));
+ uid_t callingUid = TEST_UID2;
+ addPrivilegedUid(callingUid);
+ ASSERT_EQ(0, mTc.setCounterSet(TEST_COUNTERSET, TEST_UID, callingUid));
uid_t uid = TEST_UID;
StatusOr<uint8_t> counterSetResult = mFakeUidCounterSetMap.readValue(uid);
ASSERT_TRUE(isOk(counterSetResult));
ASSERT_EQ(TEST_COUNTERSET, counterSetResult.value());
- ASSERT_EQ(0, mTc.setCounterSet(DEFAULT_COUNTERSET, TEST_UID));
+ ASSERT_EQ(0, mTc.setCounterSet(DEFAULT_COUNTERSET, TEST_UID, callingUid));
+ ASSERT_FALSE(isOk(mFakeUidCounterSetMap.readValue(uid)));
+ expectMapEmpty(mFakeUidCounterSetMap);
+}
+
+TEST_F(TrafficControllerTest, TestSetCounterSetWithoutPermission) {
+ SKIP_IF_BPF_NOT_SUPPORTED;
+
+ ASSERT_EQ(-EPERM, mTc.setCounterSet(TEST_COUNTERSET, TEST_UID, TEST_UID2));
+ uid_t uid = TEST_UID;
ASSERT_FALSE(isOk(mFakeUidCounterSetMap.readValue(uid)));
expectMapEmpty(mFakeUidCounterSetMap);
}
@@ -340,13 +416,15 @@
TEST_F(TrafficControllerTest, TestSetInvalidCounterSet) {
SKIP_IF_BPF_NOT_SUPPORTED;
- ASSERT_GT(0, mTc.setCounterSet(OVERFLOW_COUNTERSET, TEST_UID));
+ uid_t callingUid = TEST_UID2;
+ addPrivilegedUid(callingUid);
+ ASSERT_GT(0, mTc.setCounterSet(OVERFLOW_COUNTERSET, TEST_UID, callingUid));
uid_t uid = TEST_UID;
ASSERT_FALSE(isOk(mFakeUidCounterSetMap.readValue(uid)));
expectMapEmpty(mFakeUidCounterSetMap);
}
-TEST_F(TrafficControllerTest, TestDeleteTagData) {
+TEST_F(TrafficControllerTest, TestDeleteTagDataWithoutPermission) {
SKIP_IF_BPF_NOT_SUPPORTED;
uint64_t cookie = 1;
@@ -354,7 +432,22 @@
uint32_t tag = TEST_TAG;
StatsKey tagStatsMapKey;
populateFakeStats(cookie, uid, tag, &tagStatsMapKey);
- ASSERT_EQ(0, mTc.deleteTagData(TEST_TAG, TEST_UID));
+ ASSERT_EQ(-EPERM, mTc.deleteTagData(0, TEST_UID, TEST_UID2));
+
+ expectFakeStatsUnchanged(cookie, tag, uid, tagStatsMapKey);
+}
+
+TEST_F(TrafficControllerTest, TestDeleteTagData) {
+ SKIP_IF_BPF_NOT_SUPPORTED;
+
+ uid_t callingUid = TEST_UID2;
+ addPrivilegedUid(callingUid);
+ uint64_t cookie = 1;
+ uid_t uid = TEST_UID;
+ uint32_t tag = TEST_TAG;
+ StatsKey tagStatsMapKey;
+ populateFakeStats(cookie, uid, tag, &tagStatsMapKey);
+ ASSERT_EQ(0, mTc.deleteTagData(TEST_TAG, TEST_UID, callingUid));
ASSERT_FALSE(isOk(mFakeCookieTagMap.readValue(cookie)));
StatusOr<uint8_t> counterSetResult = mFakeUidCounterSetMap.readValue(uid);
ASSERT_TRUE(isOk(counterSetResult));
@@ -374,12 +467,14 @@
TEST_F(TrafficControllerTest, TestDeleteAllUidData) {
SKIP_IF_BPF_NOT_SUPPORTED;
+ uid_t callingUid = TEST_UID2;
+ addPrivilegedUid(callingUid);
uint64_t cookie = 1;
uid_t uid = TEST_UID;
uint32_t tag = TEST_TAG;
StatsKey tagStatsMapKey;
populateFakeStats(cookie, uid, tag, &tagStatsMapKey);
- ASSERT_EQ(0, mTc.deleteTagData(0, TEST_UID));
+ ASSERT_EQ(0, mTc.deleteTagData(0, TEST_UID, callingUid));
ASSERT_FALSE(isOk(mFakeCookieTagMap.readValue(cookie)));
ASSERT_FALSE(isOk(mFakeUidCounterSetMap.readValue(uid)));
ASSERT_FALSE(isOk(mFakeTagStatsMap.readValue(tagStatsMapKey)));
@@ -391,6 +486,8 @@
TEST_F(TrafficControllerTest, TestDeleteDataWithTwoTags) {
SKIP_IF_BPF_NOT_SUPPORTED;
+ uid_t callingUid = TEST_UID2;
+ addPrivilegedUid(callingUid);
uint64_t cookie1 = 1;
uint64_t cookie2 = 2;
uid_t uid = TEST_UID;
@@ -400,7 +497,7 @@
StatsKey tagStatsMapKey2;
populateFakeStats(cookie1, uid, tag1, &tagStatsMapKey1);
populateFakeStats(cookie2, uid, tag2, &tagStatsMapKey2);
- ASSERT_EQ(0, mTc.deleteTagData(TEST_TAG, TEST_UID));
+ ASSERT_EQ(0, mTc.deleteTagData(TEST_TAG, TEST_UID, callingUid));
ASSERT_FALSE(isOk(mFakeCookieTagMap.readValue(cookie1)));
StatusOr<UidTag> cookieMapResult = mFakeCookieTagMap.readValue(cookie2);
ASSERT_TRUE(isOk(cookieMapResult));
@@ -419,6 +516,8 @@
TEST_F(TrafficControllerTest, TestDeleteDataWithTwoUids) {
SKIP_IF_BPF_NOT_SUPPORTED;
+ uid_t callingUid = TEST_UID2;
+ addPrivilegedUid(callingUid);
uint64_t cookie1 = 1;
uint64_t cookie2 = 2;
uid_t uid1 = TEST_UID;
@@ -431,7 +530,7 @@
// Delete the stats of one of the uid. Check if it is properly collected by
// removedStats.
- ASSERT_EQ(0, mTc.deleteTagData(0, uid2));
+ ASSERT_EQ(0, mTc.deleteTagData(0, uid2, callingUid));
ASSERT_FALSE(isOk(mFakeCookieTagMap.readValue(cookie2)));
StatusOr<uint8_t> counterSetResult = mFakeUidCounterSetMap.readValue(uid1);
ASSERT_TRUE(isOk(counterSetResult));
@@ -452,7 +551,7 @@
ASSERT_EQ((uint64_t)100, appStatsResult.value().rxBytes);
// Delete the stats of the other uid.
- ASSERT_EQ(0, mTc.deleteTagData(0, uid1));
+ ASSERT_EQ(0, mTc.deleteTagData(0, uid1, callingUid));
ASSERT_FALSE(isOk(mFakeUidStatsMap.readValue(tagStatsMapKey1)));
ASSERT_FALSE(isOk(mFakeAppUidStatsMap.readValue(uid1)));
}