From 6e774bae19bc8d92e1cb714237df43287eaba00d Mon Sep 17 00:00:00 2001 From: Elliott Hughes Date: Fri, 26 Apr 2024 15:30:30 +0000 Subject: [PATCH] parse_line: allow -1 for apexes and sdk libraries. Since we're in much worse trouble if `/data/system/packages.list` is attacker-controlled, there doesn't seem like much benefit to having the little bit of [incomplete] range checking we had on the uid field (by using a wider type than `uid_t` actually is), and apparently we're now abusing `-1` to mean "apex or sdk library", despite `uid_t` being an unsigned type. Bug: http://b/336659478 Change-Id: I7a270eea937d21fc1d7fcda8654054210cf631fe --- .../packagelistparser/packagelistparser.h | 5 ++++- libpackagelistparser/packagelistparser.cpp | 16 ++++------------ 2 files changed, 8 insertions(+), 13 deletions(-) diff --git a/libpackagelistparser/include/packagelistparser/packagelistparser.h b/libpackagelistparser/include/packagelistparser/packagelistparser.h index e89cb5400..9bd212af3 100644 --- a/libpackagelistparser/include/packagelistparser/packagelistparser.h +++ b/libpackagelistparser/include/packagelistparser/packagelistparser.h @@ -33,7 +33,10 @@ typedef struct pkg_info { /** Package name like "com.android.blah". */ char* name; - /** Package uid like 10014. */ + /** + * Package uid like 10014. + * Note that apexes and SDK libraries may have a bogus 0xffffffff value. + */ uid_t uid; /** Package's AndroidManifest.xml debuggable flag. */ diff --git a/libpackagelistparser/packagelistparser.cpp b/libpackagelistparser/packagelistparser.cpp index 59c3a7491..638cc43fe 100644 --- a/libpackagelistparser/packagelistparser.cpp +++ b/libpackagelistparser/packagelistparser.cpp @@ -63,14 +63,13 @@ static bool parse_gids(const char* path, size_t line_number, const char* gids, p } static bool parse_line(const char* path, size_t line_number, const char* line, pkg_info* info) { - unsigned long uid; int debuggable; char* gid_list; int profileable_from_shell = 0; - int fields = - sscanf(line, "%ms %lu %d %ms %ms %ms %d %ld", &info->name, &uid, &debuggable, &info->data_dir, - &info->seinfo, &gid_list, &profileable_from_shell, &info->version_code); + sscanf(line, "%ms %u %d %ms %ms %ms %d %ld", &info->name, &info->uid, + &debuggable, &info->data_dir, &info->seinfo, &gid_list, + &profileable_from_shell, &info->version_code); // Handle the more complicated gids field and free the temporary string. bool gids_okay = parse_gids(path, line_number, gid_list, info); @@ -84,14 +83,7 @@ static bool parse_line(const char* path, size_t line_number, const char* line, p return false; } - // Extra validation. - if (uid > UID_MAX) { - ALOGE("%s:%zu: uid %lu > UID_MAX", path, line_number, uid); - return false; - } - info->uid = uid; - - // Integer to bool conversions. + // Convert integers to bools. info->debuggable = debuggable; info->profileable_from_shell = profileable_from_shell;