Only use uids and gids for which we've allocated AIDs
Currently, getpwnam, getpwent, etc return successfully for any uid
from AID_APP_START (10000) to AID_USER_OFFSET (100000) for each user.
This is not correct however, as only specific ranges above
AID_APP_START are reserved as valid ranges. This change corrects this.
This is particularly important as the newly added AID_OVERFLOWUID is
65534, which is above AID_APP_START but not in any reserved range,
collided with the faulty returned values.
Bug: 69119022
Bug: 69128408
Test: pwd/grp bionic unit tests
Change-Id: I3dae97a90597915fa30a88fe27cda88b107e9c35
diff --git a/libc/bionic/grp_pwd.cpp b/libc/bionic/grp_pwd.cpp
index 50fcbce..43a5032 100644
--- a/libc/bionic/grp_pwd.cpp
+++ b/libc/bionic/grp_pwd.cpp
@@ -195,6 +195,89 @@
return NULL;
}
+// These are a list of the reserved app ranges, and should never contain anything below
+// AID_APP_START. They exist per user, so a given uid/gid modulo AID_USER_OFFSET will map
+// to these ranges.
+static constexpr struct {
+ uid_t start;
+ uid_t end;
+} user_ranges[] = {
+ { AID_APP_START, AID_APP_END },
+ { AID_CACHE_GID_START, AID_CACHE_GID_END },
+ { AID_EXT_GID_START, AID_EXT_GID_END },
+ { AID_EXT_CACHE_GID_START, AID_EXT_CACHE_GID_END },
+ { AID_SHARED_GID_START, AID_SHARED_GID_END },
+ { AID_ISOLATED_START, AID_ISOLATED_END },
+};
+
+static constexpr bool verify_user_ranges_ascending() {
+ auto array_size = arraysize(user_ranges);
+ if (array_size < 2) return false;
+
+ if (user_ranges[0].start > user_ranges[0].end) return false;
+
+ for (size_t i = 1; i < array_size; ++i) {
+ if (user_ranges[i].start > user_ranges[i].end) return false;
+ if (user_ranges[i - 1].end > user_ranges[i].start) return false;
+ }
+ return true;
+}
+
+static_assert(verify_user_ranges_ascending(), "user_ranges must have ascending ranges");
+
+static bool is_valid_app_id(id_t id) {
+ id_t appid = id % AID_USER_OFFSET;
+
+ // AID_OVERFLOWUID is never a valid app id, so we explicitly return false to ensure this.
+ // This is true across all users, as there is no reason to ever map this id into any user range.
+ if (appid == AID_OVERFLOWUID) {
+ return false;
+ }
+
+ // If we've resolved to something before app_start, we have already checked against
+ // android_ids, so no need to check again.
+ if (appid < user_ranges[0].start) {
+ return true;
+ }
+
+ // Otherwise check that the appid is in one of the reserved ranges.
+ for (size_t i = 0; i < arraysize(user_ranges); ++i) {
+ if (appid >= user_ranges[i].start && appid <= user_ranges[i].end) {
+ return true;
+ }
+ }
+
+ return false;
+}
+
+// This provides an iterater for app_ids within the first user's app id's.
+static uid_t get_next_app_id(uid_t current_id) {
+ // If current_id is below the first of the user_ranges, then we're uninitialized, and return the
+ // first valid id.
+ if (current_id < user_ranges[0].start) {
+ return user_ranges[0].start;
+ }
+
+ uid_t incremented_id = current_id + 1;
+
+ // Check to see if our incremented_id is between two ranges, and if so, return the beginning of
+ // the next valid range.
+ for (size_t i = 1; i < arraysize(user_ranges); ++i) {
+ if (incremented_id > user_ranges[i - 1].end && incremented_id < user_ranges[i].start) {
+ return user_ranges[i].start;
+ }
+ }
+
+ // Check to see if our incremented_id is above final range, and return -1 to indicate that we've
+ // completed if so.
+ if (incremented_id > user_ranges[arraysize(user_ranges) - 1].end) {
+ return -1;
+ }
+
+ // Otherwise the incremented_id is valid, so return it.
+ return incremented_id;
+}
+
// Translate a user/group name to the corresponding user/group id.
// all_a1234 -> 0 * AID_USER_OFFSET + AID_SHARED_GID_START + 1234 (group name only)
// u0_a1234_cache -> 0 * AID_USER_OFFSET + AID_CACHE_GID_START + 1234 (group name only)
@@ -377,7 +460,7 @@
// AID_USER_OFFSET+ -> u1_radio, u1_a1234, u2_i1234, etc.
// returns a passwd structure (sets errno to ENOENT on failure).
static passwd* app_id_to_passwd(uid_t uid, passwd_state_t* state) {
- if (uid < AID_APP_START) {
+ if (uid < AID_APP_START || !is_valid_app_id(uid)) {
errno = ENOENT;
return NULL;
}
@@ -405,7 +488,7 @@
// Translate a gid into the corresponding app_<gid>
// group structure (sets errno to ENOENT on failure).
static group* app_id_to_group(gid_t gid, group_state_t* state) {
- if (gid < AID_APP_START) {
+ if (gid < AID_APP_START || !is_valid_app_id(gid)) {
errno = ENOENT;
return NULL;
}
@@ -521,15 +604,13 @@
state->getpwent_idx++ - start + AID_OEM_RESERVED_2_START, state);
}
- start = end;
- end += AID_USER_OFFSET - AID_APP_START; // Do not expose higher users
+ state->getpwent_idx = get_next_app_id(state->getpwent_idx);
- if (state->getpwent_idx < end) {
- return app_id_to_passwd(state->getpwent_idx++ - start + AID_APP_START, state);
+ if (state->getpwent_idx != -1) {
+ return app_id_to_passwd(state->getpwent_idx, state);
}
// We are not reporting u1_a* and higher or we will be here forever
- state->getpwent_idx = -1;
return NULL;
}
@@ -652,12 +733,12 @@
start = end;
end += AID_USER_OFFSET - AID_APP_START; // Do not expose higher groups
- if (state->getgrent_idx < end) {
- init_group_state(state);
- return app_id_to_group(state->getgrent_idx++ - start + AID_APP_START, state);
+ state->getgrent_idx = get_next_app_id(state->getgrent_idx);
+
+ if (state->getgrent_idx != -1) {
+ return app_id_to_group(state->getgrent_idx, state);
}
// We are not reporting u1_a* and higher or we will be here forever
- state->getgrent_idx = -1;
return NULL;
}
diff --git a/libc/private/bionic_macros.h b/libc/private/bionic_macros.h
index a3a3ece..979a704 100644
--- a/libc/private/bionic_macros.h
+++ b/libc/private/bionic_macros.h
@@ -78,4 +78,20 @@
#define BIONIC_STOP_UNWIND asm volatile(".cfi_undefined $ra")
#endif
+// The arraysize(arr) macro returns the # of elements in an array arr.
+// The expression is a compile-time constant, and therefore can be
+// used in defining new arrays, for example. If you use arraysize on
+// a pointer by mistake, you will get a compile-time error.
+//
+// One caveat is that arraysize() doesn't accept any array of an
+// anonymous type or a type defined inside a function.
+//
+// This template function declaration is used in defining arraysize.
+// Note that the function doesn't need an implementation, as we only
+// use its type.
+template <typename T, size_t N>
+char (&ArraySizeHelper(T (&array)[N]))[N]; // NOLINT(readability/casting)
+
+#define arraysize(array) (sizeof(ArraySizeHelper(array)))
+
#endif // _BIONIC_MACROS_H_
diff --git a/tests/grp_pwd_test.cpp b/tests/grp_pwd_test.cpp
index 38afee2..61748c8 100644
--- a/tests/grp_pwd_test.cpp
+++ b/tests/grp_pwd_test.cpp
@@ -26,13 +26,17 @@
#include <sys/types.h>
#include <unistd.h>
-#include <bitset>
+#include <set>
+#include <vector>
+#include <android-base/strings.h>
#include <private/android_filesystem_config.h>
// Generated android_ids array
#include "generated_android_ids.h"
+using android::base::Join;
+
enum uid_type_t {
TYPE_SYSTEM,
TYPE_APP
@@ -186,51 +190,86 @@
check_get_passwd("u1_i0", 199000, TYPE_APP);
}
+template <typename T>
+static void expect_ids(const T& ids) {
+ std::set<typename T::key_type> expected_ids;
+ // Ensure that all android_ids are iterated through.
+ for (size_t n = 0; n < android_id_count; ++n) {
+ EXPECT_EQ(1U, ids.count(android_ids[n].aid)) << "android_ids[n].aid: " << android_ids[n].aid;
+ expected_ids.emplace(android_ids[n].aid);
+ }
+
+ auto expect_range = [&ids, &expected_ids](uid_t start, uid_t end) {
+ for (size_t n = start; n <= end; ++n) {
+ EXPECT_EQ(1U, ids.count(n)) << "n: " << n;
+ expected_ids.emplace(n);
+ }
+ };
+
+ // Ensure that all reserved ranges are iterated through.
+ expect_range(AID_OEM_RESERVED_START, AID_OEM_RESERVED_END);
+ expect_range(AID_OEM_RESERVED_2_START, AID_OEM_RESERVED_2_END);
+ expect_range(AID_APP_START, AID_APP_END);
+ expect_range(AID_CACHE_GID_START, AID_CACHE_GID_END);
+ expect_range(AID_EXT_GID_START, AID_EXT_GID_END);
+ expect_range(AID_EXT_CACHE_GID_START, AID_EXT_CACHE_GID_END);
+ expect_range(AID_SHARED_GID_START, AID_SHARED_GID_END);
+ expect_range(AID_ISOLATED_START, AID_ISOLATED_END);
+
+ // Ensure that no other ids were returned.
+ auto return_differences = [&ids, &expected_ids] {
+ std::vector<typename T::key_type> missing_from_ids;
+ std::set_difference(expected_ids.begin(), expected_ids.end(), ids.begin(), ids.end(),
+ std::inserter(missing_from_ids, missing_from_ids.begin()));
+ std::vector<typename T::key_type> extra_in_ids;
+ std::set_difference(ids.begin(), ids.end(), expected_ids.begin(), expected_ids.end(),
+ std::inserter(extra_in_ids, extra_in_ids.begin()));
+ std::string result;
+ if (!missing_from_ids.empty()) {
+ result += "Missing ids from results: " + Join(missing_from_ids, " ");
+ }
+ if (!extra_in_ids.empty()) {
+ if (!result.empty()) result += ", ";
+ result += "Extra ids in results: " + Join(extra_in_ids, " ");
+ }
+ return result;
+ };
+ EXPECT_EQ(expected_ids, ids) << return_differences();
+}
+
TEST(pwd, getpwent_iterate) {
passwd* pwd;
- std::bitset<10000> exist;
- bool application = false;
-
- exist.reset();
+ std::set<uid_t> uids;
setpwent();
while ((pwd = getpwent()) != NULL) {
ASSERT_TRUE(NULL != pwd->pw_name);
+
EXPECT_EQ(pwd->pw_gid, pwd->pw_uid) << "pwd->pw_uid: " << pwd->pw_uid;
EXPECT_EQ(NULL, pwd->pw_passwd) << "pwd->pw_uid: " << pwd->pw_uid;
#ifdef __LP64__
EXPECT_TRUE(NULL == pwd->pw_gecos) << "pwd->pw_uid: " << pwd->pw_uid;
#endif
EXPECT_TRUE(NULL != pwd->pw_shell);
- if (pwd->pw_uid >= exist.size()) {
- EXPECT_STREQ("/data", pwd->pw_dir) << "pwd->pw_uid: " << pwd->pw_uid;
- application = true;
- } else {
+ if (pwd->pw_uid < AID_APP_START || pwd->pw_uid == AID_OVERFLOWUID) {
EXPECT_STREQ("/", pwd->pw_dir) << "pwd->pw_uid: " << pwd->pw_uid;
- // TODO(b/27999086): fix this check with the OEM range
- // If OEMs add their own AIDs to private/android_filesystem_config.h, this check will fail.
- // Long term we want to create a better solution for OEMs adding AIDs, but we're not there
- // yet, so therefore we do not check for uid's in the OEM range.
- if (!(pwd->pw_uid >= 2900 && pwd->pw_uid <= 2999) &&
- !(pwd->pw_uid >= 5000 && pwd->pw_uid <= 5999)) {
- EXPECT_FALSE(exist[pwd->pw_uid]) << "pwd->pw_uid: " << pwd->pw_uid;
- }
- exist[pwd->pw_uid] = true;
+ } else {
+ EXPECT_STREQ("/data", pwd->pw_dir) << "pwd->pw_uid: " << pwd->pw_uid;
}
+
+ // TODO(b/27999086): fix this check with the OEM range
+ // If OEMs add their own AIDs to private/android_filesystem_config.h, this check will fail.
+ // Long term we want to create a better solution for OEMs adding AIDs, but we're not there
+ // yet, so therefore we do not check for uid's in the OEM range.
+ if (!(pwd->pw_uid >= 2900 && pwd->pw_uid <= 2999) &&
+ !(pwd->pw_uid >= 5000 && pwd->pw_uid <= 5999)) {
+ EXPECT_EQ(0U, uids.count(pwd->pw_uid)) << "pwd->pw_uid: " << pwd->pw_uid;
+ }
+ uids.emplace(pwd->pw_uid);
}
endpwent();
- // Required content
- for (size_t n = 0; n < android_id_count; ++n) {
- EXPECT_TRUE(exist[android_ids[n].aid]) << "android_ids[n].aid: " << android_ids[n].aid;
- }
- for (size_t n = 2900; n < 2999; ++n) {
- EXPECT_TRUE(exist[n]) << "n: " << n;
- }
- for (size_t n = 5000; n < 5999; ++n) {
- EXPECT_TRUE(exist[n]) << "n: " << n;
- }
- EXPECT_TRUE(application);
+ expect_ids(uids);
}
static void check_group(const group* grp, const char* group_name, gid_t gid) {
@@ -446,10 +485,7 @@
TEST(grp, getgrent_iterate) {
group* grp;
- std::bitset<10000> exist;
- bool application = false;
-
- exist.reset();
+ std::set<gid_t> gids;
setgrent();
while ((grp = getgrent()) != NULL) {
@@ -457,31 +493,18 @@
ASSERT_TRUE(grp->gr_mem != NULL) << "grp->gr_gid: " << grp->gr_gid;
EXPECT_STREQ(grp->gr_name, grp->gr_mem[0]) << "grp->gr_gid: " << grp->gr_gid;
EXPECT_TRUE(grp->gr_mem[1] == NULL) << "grp->gr_gid: " << grp->gr_gid;
- if (grp->gr_gid >= exist.size()) {
- application = true;
- } else {
- // TODO(b/27999086): fix this check with the OEM range
- // If OEMs add their own AIDs to private/android_filesystem_config.h, this check will fail.
- // Long term we want to create a better solution for OEMs adding AIDs, but we're not there
- // yet, so therefore we do not check for gid's in the OEM range.
- if (!(grp->gr_gid >= 2900 && grp->gr_gid <= 2999) &&
- !(grp->gr_gid >= 5000 && grp->gr_gid <= 5999)) {
- EXPECT_FALSE(exist[grp->gr_gid]) << "grp->gr_gid: " << grp->gr_gid;
- }
- exist[grp->gr_gid] = true;
+
+ // TODO(b/27999086): fix this check with the OEM range
+ // If OEMs add their own AIDs to private/android_filesystem_config.h, this check will fail.
+ // Long term we want to create a better solution for OEMs adding AIDs, but we're not there
+ // yet, so therefore we do not check for gid's in the OEM range.
+ if (!(grp->gr_gid >= 2900 && grp->gr_gid <= 2999) &&
+ !(grp->gr_gid >= 5000 && grp->gr_gid <= 5999)) {
+ EXPECT_EQ(0U, gids.count(grp->gr_gid)) << "grp->gr_gid: " << grp->gr_gid;
}
+ gids.emplace(grp->gr_gid);
}
endgrent();
- // Required content
- for (size_t n = 0; n < android_id_count; ++n) {
- EXPECT_TRUE(exist[android_ids[n].aid]) << "android_ids[n].aid: " << android_ids[n].aid;
- }
- for (size_t n = 2900; n < 2999; ++n) {
- EXPECT_TRUE(exist[n]) << "n: " << n;
- }
- for (size_t n = 5000; n < 5999; ++n) {
- EXPECT_TRUE(exist[n]) << "n: " << n;
- }
- EXPECT_TRUE(application);
+ expect_ids(gids);
}