From 517e1f17cfec2143d4d10a64b1496a550acf3ea2 Mon Sep 17 00:00:00 2001 From: Tom Cherry Date: Thu, 4 May 2017 17:40:33 -0700 Subject: [PATCH] init: Check DecodeUid() result and use error string Check the result of DecodeUid() and return failure when uids/gids are unable to be decoded. Also, use an error string instead of logging directly such that more context can be added when decoding fails. Bug: 38038887 Test: Boot bullhead Test: Init unit tests Change-Id: I84c11aa5a8041bf5d2f754ee9af748344b789b37 --- init/builtins.cpp | 39 +++++++++++++++++++-------- init/init_parser_test.cpp | 18 ++++++++++--- init/service.cpp | 57 +++++++++++++++++++++++++++++++++------ init/util.cpp | 48 +++++++++++++++------------------ init/util.h | 2 +- init/util_test.cpp | 19 ++++++++++--- 6 files changed, 128 insertions(+), 55 deletions(-) diff --git a/init/builtins.cpp b/init/builtins.cpp index 3dadfd7dc..848dfdbf9 100644 --- a/init/builtins.cpp +++ b/init/builtins.cpp @@ -215,11 +215,19 @@ static int do_mkdir(const std::vector& args) { } if (args.size() >= 4) { - uid_t uid = decode_uid(args[3].c_str()); + uid_t uid; + std::string decode_uid_err; + if (!DecodeUid(args[3], &uid, &decode_uid_err)) { + LOG(ERROR) << "Unable to find UID for '" << args[3] << "': " << decode_uid_err; + return -1; + } gid_t gid = -1; if (args.size() == 5) { - gid = decode_uid(args[4].c_str()); + if (!DecodeUid(args[4], &gid, &decode_uid_err)) { + LOG(ERROR) << "Unable to find GID for '" << args[3] << "': " << decode_uid_err; + return -1; + } } if (lchown(args[1].c_str(), uid, gid) == -1) { @@ -655,17 +663,26 @@ static int do_copy(const std::vector& args) { } static int do_chown(const std::vector& args) { - /* GID is optional. */ - if (args.size() == 3) { - if (lchown(args[2].c_str(), decode_uid(args[1].c_str()), -1) == -1) - return -errno; - } else if (args.size() == 4) { - if (lchown(args[3].c_str(), decode_uid(args[1].c_str()), - decode_uid(args[2].c_str())) == -1) - return -errno; - } else { + uid_t uid; + std::string decode_uid_err; + if (!DecodeUid(args[1], &uid, &decode_uid_err)) { + LOG(ERROR) << "Unable to find UID for '" << args[1] << "': " << decode_uid_err; return -1; } + + // GID is optional and pushes the index of path out by one if specified. + const std::string& path = (args.size() == 4) ? args[3] : args[2]; + gid_t gid = -1; + + if (args.size() == 4) { + if (!DecodeUid(args[2], &gid, &decode_uid_err)) { + LOG(ERROR) << "Unable to find GID for '" << args[2] << "': " << decode_uid_err; + return -1; + } + } + + if (lchown(path.c_str(), uid, gid) == -1) return -errno; + return 0; } diff --git a/init/init_parser_test.cpp b/init/init_parser_test.cpp index d8fd2bab9..86d60d024 100644 --- a/init/init_parser_test.cpp +++ b/init/init_parser_test.cpp @@ -86,19 +86,29 @@ static void Test_make_exec_oneshot_service(bool dash_dash, bool seclabel, bool u ASSERT_EQ("", svc->seclabel()); } if (uid) { - ASSERT_EQ(decode_uid("log"), svc->uid()); + uid_t decoded_uid; + std::string err; + ASSERT_TRUE(DecodeUid("log", &decoded_uid, &err)); + ASSERT_EQ(decoded_uid, svc->uid()); } else { ASSERT_EQ(0U, svc->uid()); } if (gid) { - ASSERT_EQ(decode_uid("shell"), svc->gid()); + uid_t decoded_uid; + std::string err; + ASSERT_TRUE(DecodeUid("shell", &decoded_uid, &err)); + ASSERT_EQ(decoded_uid, svc->gid()); } else { ASSERT_EQ(0U, svc->gid()); } if (supplementary_gids) { ASSERT_EQ(2U, svc->supp_gids().size()); - ASSERT_EQ(decode_uid("system"), svc->supp_gids()[0]); - ASSERT_EQ(decode_uid("adb"), svc->supp_gids()[1]); + uid_t decoded_uid; + std::string err; + ASSERT_TRUE(DecodeUid("system", &decoded_uid, &err)); + ASSERT_EQ(decoded_uid, svc->supp_gids()[0]); + ASSERT_TRUE(DecodeUid("adb", &decoded_uid, &err)); + ASSERT_EQ(decoded_uid, svc->supp_gids()[1]); } else { ASSERT_EQ(0U, svc->supp_gids().size()); } diff --git a/init/service.cpp b/init/service.cpp index 3a9f62228..4d9edc445 100644 --- a/init/service.cpp +++ b/init/service.cpp @@ -380,9 +380,18 @@ bool Service::ParseDisabled(const std::vector& args, std::string* e } bool Service::ParseGroup(const std::vector& args, std::string* err) { - gid_ = decode_uid(args[1].c_str()); + std::string decode_uid_err; + if (!DecodeUid(args[1], &gid_, &decode_uid_err)) { + *err = "Unable to find GID for '" + args[1] + "': " + decode_uid_err; + return false; + } for (std::size_t n = 2; n < args.size(); n++) { - supp_gids_.emplace_back(decode_uid(args[n].c_str())); + gid_t gid; + if (!DecodeUid(args[n], &gid, &decode_uid_err)) { + *err = "Unable to find GID for '" + args[n] + "': " + decode_uid_err; + return false; + } + supp_gids_.emplace_back(gid); } return true; } @@ -480,10 +489,25 @@ bool Service::ParseSetenv(const std::vector& args, std::string* err template bool Service::AddDescriptor(const std::vector& args, std::string* err) { int perm = args.size() > 3 ? std::strtoul(args[3].c_str(), 0, 8) : -1; - uid_t uid = args.size() > 4 ? decode_uid(args[4].c_str()) : 0; - gid_t gid = args.size() > 5 ? decode_uid(args[5].c_str()) : 0; + uid_t uid = 0; + gid_t gid = 0; std::string context = args.size() > 6 ? args[6] : ""; + std::string decode_uid_err; + if (args.size() > 4) { + if (!DecodeUid(args[4], &uid, &decode_uid_err)) { + *err = "Unable to find UID for '" + args[4] + "': " + decode_uid_err; + return false; + } + } + + if (args.size() > 5) { + if (!DecodeUid(args[5], &gid, &decode_uid_err)) { + *err = "Unable to find GID for '" + args[5] + "': " + decode_uid_err; + return false; + } + } + auto descriptor = std::make_unique(args[1], args[2], uid, gid, perm, context); auto old = @@ -522,7 +546,11 @@ bool Service::ParseFile(const std::vector& args, std::string* err) } bool Service::ParseUser(const std::vector& args, std::string* err) { - uid_ = decode_uid(args[1].c_str()); + std::string decode_uid_err; + if (!DecodeUid(args[1], &uid_, &decode_uid_err)) { + *err = "Unable to find UID for '" + args[1] + "': " + decode_uid_err; + return false; + } return true; } @@ -936,15 +964,28 @@ Service* ServiceManager::MakeExecOneshotService(const std::vector& } uid_t uid = 0; if (command_arg > 3) { - uid = decode_uid(args[2].c_str()); + std::string decode_uid_err; + if (!DecodeUid(args[2], &uid, &decode_uid_err)) { + LOG(ERROR) << "Unable to find UID for '" << args[2] << "': " << decode_uid_err; + return nullptr; + } } gid_t gid = 0; std::vector supp_gids; if (command_arg > 4) { - gid = decode_uid(args[3].c_str()); + std::string decode_uid_err; + if (!DecodeUid(args[3], &gid, &decode_uid_err)) { + LOG(ERROR) << "Unable to find GID for '" << args[3] << "': " << decode_uid_err; + return nullptr; + } std::size_t nr_supp_gids = command_arg - 1 /* -- */ - 4 /* exec SECLABEL UID GID */; for (size_t i = 0; i < nr_supp_gids; ++i) { - supp_gids.push_back(decode_uid(args[4 + i].c_str())); + gid_t supp_gid; + if (!DecodeUid(args[4 + i], &supp_gid, &decode_uid_err)) { + LOG(ERROR) << "Unable to find UID for '" << args[4 + i] << "': " << decode_uid_err; + return nullptr; + } + supp_gids.push_back(supp_gid); } } diff --git a/init/util.cpp b/init/util.cpp index 87b8d9d81..4df0c25c9 100644 --- a/init/util.cpp +++ b/init/util.cpp @@ -48,39 +48,33 @@ #endif using android::base::boot_clock; +using namespace std::literals::string_literals; -static unsigned int do_decode_uid(const char *s) -{ - unsigned int v; +// DecodeUid() - decodes and returns the given string, which can be either the +// numeric or name representation, into the integer uid or gid. Returns +// UINT_MAX on error. +bool DecodeUid(const std::string& name, uid_t* uid, std::string* err) { + *uid = UINT_MAX; + *err = ""; - if (!s || *s == '\0') - return UINT_MAX; - - if (isalpha(s[0])) { - struct passwd* pwd = getpwnam(s); - if (!pwd) - return UINT_MAX; - return pwd->pw_uid; + if (isalpha(name[0])) { + passwd* pwd = getpwnam(name.c_str()); + if (!pwd) { + *err = "getpwnam failed: "s + strerror(errno); + return false; + } + *uid = pwd->pw_uid; + return true; } errno = 0; - v = (unsigned int) strtoul(s, 0, 0); - if (errno) - return UINT_MAX; - return v; -} - -/* - * decode_uid - decodes and returns the given string, which can be either the - * numeric or name representation, into the integer uid or gid. Returns - * UINT_MAX on error. - */ -unsigned int decode_uid(const char *s) { - unsigned int v = do_decode_uid(s); - if (v == UINT_MAX) { - LOG(ERROR) << "decode_uid: Unable to find UID for '" << s << "'; returning UINT_MAX"; + uid_t result = static_cast(strtoul(name.c_str(), 0, 0)); + if (errno) { + *err = "strtoul failed: "s + strerror(errno); + return false; } - return v; + *uid = result; + return true; } /* diff --git a/init/util.h b/init/util.h index 55ebded57..4957f1550 100644 --- a/init/util.h +++ b/init/util.h @@ -61,7 +61,7 @@ class Timer { std::ostream& operator<<(std::ostream& os, const Timer& t); -unsigned int decode_uid(const char *s); +bool DecodeUid(const std::string& name, uid_t* uid, std::string* err); int mkdir_recursive(const std::string& pathname, mode_t mode, selabel_handle* sehandle); int wait_for_file(const char *filename, std::chrono::nanoseconds timeout); diff --git a/init/util_test.cpp b/init/util_test.cpp index ac23a326e..a24185b00 100644 --- a/init/util_test.cpp +++ b/init/util_test.cpp @@ -115,10 +115,21 @@ TEST(util, write_file_exist) { EXPECT_STREQ("2ll2", s2.c_str()); } -TEST(util, decode_uid) { - EXPECT_EQ(0U, decode_uid("root")); - EXPECT_EQ(UINT_MAX, decode_uid("toot")); - EXPECT_EQ(123U, decode_uid("123")); +TEST(util, DecodeUid) { + uid_t decoded_uid; + std::string err; + + EXPECT_TRUE(DecodeUid("root", &decoded_uid, &err)); + EXPECT_EQ("", err); + EXPECT_EQ(0U, decoded_uid); + + EXPECT_FALSE(DecodeUid("toot", &decoded_uid, &err)); + EXPECT_EQ("getpwnam failed: No such file or directory", err); + EXPECT_EQ(UINT_MAX, decoded_uid); + + EXPECT_TRUE(DecodeUid("123", &decoded_uid, &err)); + EXPECT_EQ("", err); + EXPECT_EQ(123U, decoded_uid); } TEST(util, is_dir) {