From 10c6374e81c070faf0ed17b79b6659c0f6f63082 Mon Sep 17 00:00:00 2001 From: Suchang Woo Date: Thu, 13 May 2021 18:56:31 +0900 Subject: [PATCH] ueventd: Run external handler as non-root group The external firmware handler always has root group privileges because it is forked/executed without setgid() by ueventd which has root privileges. This patch calls setgid() with group ID specified in ueventd.rc before execv(). Test: atest CtsInitTestCases Signed-off-by: Suchang Woo Change-Id: Id1430e783b0e409d55ac80fe213e81ba099729e2 --- init/README.ueventd.md | 5 ++++- init/firmware_handler.cpp | 22 +++++++++++++++++----- init/firmware_handler.h | 5 ++++- init/ueventd_parser.cpp | 18 +++++++++++++++--- init/ueventd_parser_test.cpp | 24 +++++++++++++++++++++++- 5 files changed, 63 insertions(+), 11 deletions(-) diff --git a/init/README.ueventd.md b/init/README.ueventd.md index d22f68fb0..76f51939e 100644 --- a/init/README.ueventd.md +++ b/init/README.ueventd.md @@ -123,7 +123,10 @@ not present. The exact firmware file to be served can be customized by running an external program by a `external_firmware_handler` line in a ueventd.rc file. This line takes the format of - external_firmware_handler + external_firmware_handler + +The handler will be run as the given user, or if a group is provided, as the given user and group. + For example external_firmware_handler /devices/leds/red/firmware/coeffs.bin system /vendor/bin/led_coeffs.bin diff --git a/init/firmware_handler.cpp b/init/firmware_handler.cpp index bdc292275..30e808d9f 100644 --- a/init/firmware_handler.cpp +++ b/init/firmware_handler.cpp @@ -19,6 +19,7 @@ #include #include #include +#include #include #include #include @@ -81,9 +82,9 @@ static bool IsBooting() { return access("/dev/.booting", F_OK) == 0; } -ExternalFirmwareHandler::ExternalFirmwareHandler(std::string devpath, uid_t uid, +ExternalFirmwareHandler::ExternalFirmwareHandler(std::string devpath, uid_t uid, gid_t gid, std::string handler_path) - : devpath(std::move(devpath)), uid(uid), handler_path(std::move(handler_path)) { + : devpath(std::move(devpath)), uid(uid), gid(gid), handler_path(std::move(handler_path)) { auto wildcard_position = this->devpath.find('*'); if (wildcard_position != std::string::npos) { if (wildcard_position == this->devpath.length() - 1) { @@ -97,13 +98,17 @@ ExternalFirmwareHandler::ExternalFirmwareHandler(std::string devpath, uid_t uid, } } +ExternalFirmwareHandler::ExternalFirmwareHandler(std::string devpath, uid_t uid, + std::string handler_path) + : ExternalFirmwareHandler(devpath, uid, 0, handler_path) {} + FirmwareHandler::FirmwareHandler(std::vector firmware_directories, std::vector external_firmware_handlers) : firmware_directories_(std::move(firmware_directories)), external_firmware_handlers_(std::move(external_firmware_handlers)) {} Result FirmwareHandler::RunExternalHandler(const std::string& handler, uid_t uid, - const Uevent& uevent) const { + gid_t gid, const Uevent& uevent) const { unique_fd child_stdout; unique_fd parent_stdout; if (!Socketpair(AF_UNIX, SOCK_STREAM | SOCK_CLOEXEC, 0, &child_stdout, &parent_stdout)) { @@ -140,6 +145,13 @@ Result FirmwareHandler::RunExternalHandler(const std::string& handl } c_args.emplace_back(nullptr); + if (gid != 0) { + if (setgid(gid) != 0) { + fprintf(stderr, "setgid() failed: %s", strerror(errno)); + _exit(EXIT_FAILURE); + } + } + if (setuid(uid) != 0) { fprintf(stderr, "setuid() failed: %s", strerror(errno)); _exit(EXIT_FAILURE); @@ -196,8 +208,8 @@ std::string FirmwareHandler::GetFirmwarePath(const Uevent& uevent) const { << "' for devpath: '" << uevent.path << "' firmware: '" << uevent.firmware << "'"; - auto result = - RunExternalHandler(external_handler.handler_path, external_handler.uid, uevent); + auto result = RunExternalHandler(external_handler.handler_path, external_handler.uid, + external_handler.gid, uevent); if (!result.ok()) { LOG(ERROR) << "Using default firmware; External firmware handler failed: " << result.error(); diff --git a/init/firmware_handler.h b/init/firmware_handler.h index 3c35b1f16..d2f7347ce 100644 --- a/init/firmware_handler.h +++ b/init/firmware_handler.h @@ -16,6 +16,7 @@ #pragma once +#include #include #include @@ -31,9 +32,11 @@ namespace init { struct ExternalFirmwareHandler { ExternalFirmwareHandler(std::string devpath, uid_t uid, std::string handler_path); + ExternalFirmwareHandler(std::string devpath, uid_t uid, gid_t gid, std::string handler_path); std::string devpath; uid_t uid; + gid_t gid; std::string handler_path; std::function match; @@ -51,7 +54,7 @@ class FirmwareHandler : public UeventHandler { friend void FirmwareTestWithExternalHandler(const std::string& test_name, bool expect_new_firmware); - Result RunExternalHandler(const std::string& handler, uid_t uid, + Result RunExternalHandler(const std::string& handler, uid_t uid, gid_t gid, const Uevent& uevent) const; std::string GetFirmwarePath(const Uevent& uevent) const; void ProcessFirmwareEvent(const std::string& root, const std::string& firmware) const; diff --git a/init/ueventd_parser.cpp b/init/ueventd_parser.cpp index 2221228d1..9a14406bc 100644 --- a/init/ueventd_parser.cpp +++ b/init/ueventd_parser.cpp @@ -101,8 +101,8 @@ Result ParseFirmwareDirectoriesLine(std::vector&& args, Result ParseExternalFirmwareHandlerLine( std::vector&& args, std::vector* external_firmware_handlers) { - if (args.size() != 4) { - return Error() << "external_firmware_handler lines must have exactly 3 parameters"; + if (args.size() != 4 && args.size() != 5) { + return Error() << "external_firmware_handler lines must have 3 or 4 parameters"; } if (std::find_if(external_firmware_handlers->begin(), external_firmware_handlers->end(), @@ -117,7 +117,19 @@ Result ParseExternalFirmwareHandlerLine( return ErrnoError() << "invalid handler uid'" << args[2] << "'"; } - ExternalFirmwareHandler handler(std::move(args[1]), pwd->pw_uid, std::move(args[3])); + gid_t gid = 0; + int handler_index = 3; + if (args.size() == 5) { + struct group* grp = getgrnam(args[3].c_str()); + if (!grp) { + return ErrnoError() << "invalid handler gid '" << args[3] << "'"; + } + gid = grp->gr_gid; + handler_index = 4; + } + + ExternalFirmwareHandler handler(std::move(args[1]), pwd->pw_uid, gid, + std::move(args[handler_index])); external_firmware_handlers->emplace_back(std::move(handler)); return {}; diff --git a/init/ueventd_parser_test.cpp b/init/ueventd_parser_test.cpp index c5aa9e3b5..d77cb03b8 100644 --- a/init/ueventd_parser_test.cpp +++ b/init/ueventd_parser_test.cpp @@ -49,6 +49,7 @@ void TestExternalFirmwareHandler(const ExternalFirmwareHandler& expected, const ExternalFirmwareHandler& test) { EXPECT_EQ(expected.devpath, test.devpath) << expected.devpath; EXPECT_EQ(expected.uid, test.uid) << expected.uid; + EXPECT_EQ(expected.gid, test.gid) << expected.gid; EXPECT_EQ(expected.handler_path, test.handler_path) << expected.handler_path; } @@ -157,39 +158,59 @@ external_firmware_handler /devices/path/firmware/something002.bin radio "/vendor external_firmware_handler /devices/path/firmware/* root "/vendor/bin/firmware_handler.sh" external_firmware_handler /devices/path/firmware/something* system "/vendor/bin/firmware_handler.sh" external_firmware_handler /devices/path/*/firmware/something*.bin radio "/vendor/bin/firmware_handler.sh" +external_firmware_handler /devices/path/firmware/something003.bin system system /vendor/bin/firmware_handler.sh +external_firmware_handler /devices/path/firmware/something004.bin radio radio "/vendor/bin/firmware_handler.sh --has --arguments" )"; auto external_firmware_handlers = std::vector{ { "devpath", AID_ROOT, + AID_ROOT, "handler_path", }, { "/devices/path/firmware/something001.bin", AID_SYSTEM, + AID_ROOT, "/vendor/bin/firmware_handler.sh", }, { "/devices/path/firmware/something002.bin", AID_RADIO, + AID_ROOT, "/vendor/bin/firmware_handler.sh --has --arguments", }, { "/devices/path/firmware/", AID_ROOT, + AID_ROOT, "/vendor/bin/firmware_handler.sh", }, { "/devices/path/firmware/something", AID_SYSTEM, + AID_ROOT, "/vendor/bin/firmware_handler.sh", }, { "/devices/path/*/firmware/something*.bin", AID_RADIO, + AID_ROOT, "/vendor/bin/firmware_handler.sh", }, + { + "/devices/path/firmware/something003.bin", + AID_SYSTEM, + AID_SYSTEM, + "/vendor/bin/firmware_handler.sh", + }, + { + "/devices/path/firmware/something004.bin", + AID_RADIO, + AID_RADIO, + "/vendor/bin/firmware_handler.sh --has --arguments", + }, }; TestUeventdFile(ueventd_file, {{}, {}, {}, {}, external_firmware_handlers}); @@ -205,6 +226,7 @@ external_firmware_handler devpath root handler_path2 { "devpath", AID_ROOT, + AID_ROOT, "handler_path", }, }; @@ -305,7 +327,7 @@ parallel_restorecon enabled }; auto external_firmware_handlers = std::vector{ - {"/devices/path/firmware/firmware001.bin", AID_ROOT, "/vendor/bin/touch.sh"}, + {"/devices/path/firmware/firmware001.bin", AID_ROOT, AID_ROOT, "/vendor/bin/touch.sh"}, }; size_t uevent_socket_rcvbuf_size = 6 * 1024 * 1024;