From 9943a77bca37a0829ccd3784d1dfab37f8c24e7b Mon Sep 17 00:00:00 2001 From: Jakub Filak Date: Wed, 29 Apr 2015 13:57:39 +0200 Subject: [ABRT PATCH] a-a-i-d-t-a-cache: sanitize arguments Parse command lines arguments and use them to create new arguments for exec(). No black listing algorithm would be safe enough. The only allowed arguments are the following: * v - verbose * y - noninteractive * repo - enable only repositories whose names match the pattern * exact - download packages for the specified files * ids - passed as magic proc fd path to the wrapped executable The wrapper opens the list of needed build ids passes /proc/self/fd/[fd] to the wrapped process. This allows us to open the file with caller's UID/GID in order to avoid information disclosures. Forbidden arguments: * cache - allows regular users to create a user writable dump directory * tmpdir - the same as above * size_mb - no need to allow users to fiddle with the cache size Related: #1216962 Signed-off-by: Jakub Filak --- po/POTFILES.in | 1 + .../abrt-action-install-debuginfo-to-abrt-cache.c | 106 ++++++++++++++++----- 2 files changed, 85 insertions(+), 22 deletions(-) diff --git a/po/POTFILES.in b/po/POTFILES.in index cbe89fa..1248c46 100644 --- a/po/POTFILES.in +++ b/po/POTFILES.in @@ -31,6 +31,7 @@ src/plugins/abrt-action-check-oops-for-hw-error.in src/plugins/abrt-action-generate-backtrace.c src/plugins/abrt-action-generate-core-backtrace.c src/plugins/abrt-action-install-debuginfo.in +src/plugins/abrt-action-install-debuginfo-to-abrt-cache.c src/plugins/abrt-action-perform-ccpp-analysis.in src/plugins/abrt-action-trim-files.c src/plugins/abrt-action-ureport diff --git a/src/plugins/abrt-action-install-debuginfo-to-abrt-cache.c b/src/plugins/abrt-action-install-debuginfo-to-abrt-cache.c index e0eccc0..eb2f7c5 100644 --- a/src/plugins/abrt-action-install-debuginfo-to-abrt-cache.c +++ b/src/plugins/abrt-action-install-debuginfo-to-abrt-cache.c @@ -29,28 +29,90 @@ */ int main(int argc, char **argv) { - /* - * We disallow passing of arguments which point to writable dirs - * and other files possibly not accessible to calling user. - * This way, the script will always use default values for these arguments. - */ - char **pp = argv; - char *arg; - while ((arg = *++pp) != NULL) + /* I18n */ + setlocale(LC_ALL, ""); +#if ENABLE_NLS + bindtextdomain(PACKAGE, LOCALEDIR); + textdomain(PACKAGE); +#endif + + abrt_init(argv); + + /* Can't keep these strings/structs static: _() doesn't support that */ + const char *program_usage_string = _( + "& [-y] [-i BUILD_IDS_FILE|-i -] [-e PATH[:PATH]...]\n" + "\t[-r REPO]\n" + "\n" + "Installs debuginfo packages for all build-ids listed in BUILD_IDS_FILE to\n" + "ABRT system cache." + ); + + enum { + OPT_v = 1 << 0, + OPT_y = 1 << 1, + OPT_i = 1 << 2, + OPT_e = 1 << 3, + OPT_r = 1 << 4, + OPT_s = 1 << 5, + }; + + const char *build_ids = "build_ids"; + const char *exact = NULL; + const char *repo = NULL; + const char *size_mb = NULL; + + struct options program_options[] = { + OPT__VERBOSE(&g_verbose), + OPT_BOOL ('y', "yes", NULL, _("Noninteractive, assume 'Yes' to all questions")), + OPT_STRING('i', "ids", &build_ids, "BUILD_IDS_FILE", _("- means STDIN, default: build_ids")), + OPT_STRING('e', "exact", &exact, "EXACT", _("Download only specified files")), + OPT_STRING('r', "repo", &repo, "REPO", _("Pattern to use when searching for repos, default: *debug*")), + OPT_STRING('s', "size_mb", &size_mb, "SIZE_MB", _("Ignored option")), + OPT_END() + }; + const unsigned opts = parse_opts(argc, argv, program_options, program_usage_string); + + /* We need to open the build ids file under the caller's UID/GID to avoid + * information disclosures when reading files with changed UID. + * Unfortunately, we cannot replace STDIN with the new fd because ABRT uses + * STDIN to communicate with the caller. So, the following code opens a + * dummy file descriptor to the build ids file and passes the new fd's proc + * path to the wrapped program in the ids argument. + * The new fd remains opened, the OS will close it for us. */ + char *build_ids_self_fd = NULL; + if (strcmp("-", build_ids) != 0) + { + const int build_ids_fd = open(build_ids, O_RDONLY); + if (build_ids_fd < 0) + perror_msg_and_die("Failed to open file '%s'", build_ids); + + /* We are not going to free this memory. There is no place to do so. */ + build_ids_self_fd = xasprintf("/proc/self/fd/%d", build_ids_fd); + } + + /* name, -v, --ids, -, -y, -e, EXACT, -r, REPO, --, NULL */ + const char *args[11]; { - /* Allow taking ids from stdin */ - if (strcmp(arg, "--ids=-") == 0) - continue; - - if (strncmp(arg, "--exact", 7) == 0) - continue; - - if (strncmp(arg, "--cache", 7) == 0) - error_msg_and_die("bad option %s", arg); - if (strncmp(arg, "--tmpdir", 8) == 0) - error_msg_and_die("bad option %s", arg); - if (strncmp(arg, "--ids", 5) == 0) - error_msg_and_die("bad option %s", arg); + const char *verbs[] = { "", "-v", "-vv", "-vvv" }; + unsigned i = 0; + args[i++] = EXECUTABLE; + args[i++] = "--ids"; + args[i++] = (build_ids_self_fd != NULL) ? build_ids_self_fd : "-"; + args[i++] = verbs[g_verbose <= 3 ? g_verbose : 3]; + if ((opts & OPT_y)) + args[i++] = "-y"; + if ((opts & OPT_e)) + { + args[i++] = "--exact"; + args[i++] = exact; + } + if ((opts & OPT_r)) + { + args[i++] = "--repo"; + args[i++] = repo; + } + args[i++] = "--"; + args[i] = NULL; } /* Switch real user/group to effective ones. @@ -122,6 +184,6 @@ int main(int argc, char **argv) putenv(path_env); } - execvp(EXECUTABLE, argv); + execvp(EXECUTABLE, (char **)args); error_msg_and_die("Can't execute %s", EXECUTABLE); } -- 1.8.3.1