From c8ab547f17910391d0cd66a9c6f1917c295480db Mon Sep 17 00:00:00 2001

From: Jakub Filak <jfilak@redhat.com>

Date: Wed, 29 Apr 2015 13:57:39 +0200

Subject: [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 <jfilak@redhat.com>

---

 po/POTFILES.in                                     |   1 +

 .../abrt-action-install-debuginfo-to-abrt-cache.c  | 137 ++++++++++++++++-----

 2 files changed, 109 insertions(+), 29 deletions(-)

diff --git a/po/POTFILES.in b/po/POTFILES.in

index 15b879f..bd76b87 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..4fa1783 100644

--- a/src/plugins/abrt-action-install-debuginfo-to-abrt-cache.c

+++ b/src/plugins/abrt-action-install-debuginfo-to-abrt-cache.c

@@ -29,41 +29,120 @@

  */

 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);

+

+    const gid_t egid = getegid();

+    const gid_t rgid = getgid();

+    const uid_t euid = geteuid();

+    const gid_t ruid = getuid();

+

+    /* 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)

+    {

+        if (setregid(egid, rgid) < 0)

+            perror_msg_and_die("setregid(egid, rgid)");

+

+        if (setreuid(euid, ruid) < 0)

+            perror_msg_and_die("setreuid(euid, ruid)");

+

+        const int build_ids_fd = open(build_ids, O_RDONLY);

+

+        if (setregid(rgid, egid) < 0)

+            perror_msg_and_die("setregid(rgid, egid)");

+

+        if (setreuid(ruid, euid) < 0 )

+            perror_msg_and_die("setreuid(ruid, euid)");

+

+        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 : "-";

+        if (g_verbose > 0)

+            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.

      * Otherwise yum library gets confused - gets EPERM (why??).

      */

-    gid_t g = getegid();

     /* do setregid only if we have to, to not upset selinux needlessly */

-    if (g != getgid())

-        IGNORE_RESULT(setregid(g, g));

-    uid_t u = geteuid();

-    if (u != getuid())

+    if (egid != rgid)

+        IGNORE_RESULT(setregid(egid, egid));

+    if (euid != ruid)

     {

-        IGNORE_RESULT(setreuid(u, u));

+        IGNORE_RESULT(setreuid(euid, euid));

         /* We are suid'ed! */

         /* Prevent malicious user from messing up with suid'ed process: */

 #if 1

@@ -117,11 +196,11 @@ int main(int argc, char **argv)

         // abrt-action-install-debuginfo doesn't fail when spawning

         // abrt-action-trim-files

         char path_env[] = "PATH=/usr/sbin:/sbin:/usr/bin:/bin:"BIN_DIR":"SBIN_DIR;

-        if (u != 0)

+        if (euid != 0)

             strcpy(path_env, "PATH=/usr/bin:/bin:"BIN_DIR);

         putenv(path_env);

     }

-    execvp(EXECUTABLE, argv);

+    execvp(EXECUTABLE, (char **)args);

     error_msg_and_die("Can't execute %s", EXECUTABLE);

 }

-- 

2.1.0