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