Blame SOURCES/0110-a-a-i-d-t-a-cache-sanitize-arguments.patch

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