From 38081a6ddf876345a15b92dc7d5e655b1f28976f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Thu, 3 Jan 2019 11:35:22 +0100 Subject: [PATCH] journald: do not store the iovec entry for process commandline on stack This fixes a crash where we would read the commandline, whose length is under control of the sending program, and then crash when trying to create a stack allocation for it. CVE-2018-16864 https://bugzilla.redhat.com/show_bug.cgi?id=1653855 The message actually doesn't get written to disk, because journal_file_append_entry() returns -E2BIG. Resolves: #1657788 --- src/journal/coredump.c | 189 +++++++++++++--------------------- src/journal/journald-server.c | 13 +-- src/shared/util.c | 17 +++ src/shared/util.h | 7 ++ 4 files changed, 98 insertions(+), 128 deletions(-) diff --git a/src/journal/coredump.c b/src/journal/coredump.c index 59ccd46bb..40de86f05 100644 --- a/src/journal/coredump.c +++ b/src/journal/coredump.c @@ -526,14 +526,6 @@ static int compose_open_fds(pid_t pid, char **open_fds) { } int main(int argc, char* argv[]) { - - /* The small core field we allocate on the stack, to keep things simple */ - char - *core_pid = NULL, *core_uid = NULL, *core_gid = NULL, *core_signal = NULL, - *core_session = NULL, *core_exe = NULL, *core_comm = NULL, *core_cmdline = NULL, - *core_cgroup = NULL, *core_cwd = NULL, *core_root = NULL, *core_unit = NULL, - *core_slice = NULL; - /* The larger ones we allocate on the heap */ _cleanup_free_ char *core_timestamp = NULL, *core_message = NULL, *coredump_data = NULL, *core_owner_uid = NULL, @@ -547,7 +539,8 @@ int main(int argc, char* argv[]) { struct iovec iovec[26]; off_t coredump_size; - int r, j = 0; + int r; + unsigned int n_iovec = 0; uid_t uid, owner_uid; gid_t gid; pid_t pid; @@ -634,151 +627,107 @@ int main(int argc, char* argv[]) { goto finish; } - core_unit = strjoina("COREDUMP_UNIT=", t); - free(t); - - } else if (cg_pid_get_user_unit(pid, &t) >= 0) { - core_unit = strjoina("COREDUMP_USER_UNIT=", t); - free(t); + if (!set_iovec_field_free(iovec, &n_iovec, "COREDUMP_UNIT=", t)) { + r = log_oom(); + goto finish; + } } - if (core_unit) - IOVEC_SET_STRING(iovec[j++], core_unit); + if (cg_pid_get_user_unit(pid, &t) >= 0) { + if (!set_iovec_field_free(iovec, &n_iovec, "COREDUMP_USER_UNIT=", t)) { + r = log_oom(); + goto finish; + } + } /* OK, now we know it's not the journal, hence we can make use * of it now. */ log_set_target(LOG_TARGET_JOURNAL_OR_KMSG); log_open(); - core_pid = strjoina("COREDUMP_PID=", info[INFO_PID]); - IOVEC_SET_STRING(iovec[j++], core_pid); - - core_uid = strjoina("COREDUMP_UID=", info[INFO_UID]); - IOVEC_SET_STRING(iovec[j++], core_uid); - - core_gid = strjoina("COREDUMP_GID=", info[INFO_GID]); - IOVEC_SET_STRING(iovec[j++], core_gid); - - core_signal = strjoina("COREDUMP_SIGNAL=", info[INFO_SIGNAL]); - IOVEC_SET_STRING(iovec[j++], core_signal); - - if (sd_pid_get_session(pid, &t) >= 0) { - core_session = strjoina("COREDUMP_SESSION=", t); - free(t); - - IOVEC_SET_STRING(iovec[j++], core_session); + if (!set_iovec_string_field(iovec, &n_iovec, "COREDUMP_PID=", info[INFO_PID])) { + r = log_oom(); + goto finish; } - if (sd_pid_get_owner_uid(pid, &owner_uid) >= 0) { - r = asprintf(&core_owner_uid, - "COREDUMP_OWNER_UID=" UID_FMT, owner_uid); - if (r > 0) - IOVEC_SET_STRING(iovec[j++], core_owner_uid); + if (!set_iovec_string_field(iovec, &n_iovec, "COREDUMP_UID=", info[INFO_UID])) { + r = log_oom(); + goto finish; } - if (sd_pid_get_slice(pid, &t) >= 0) { - core_slice = strjoina("COREDUMP_SLICE=", t); - free(t); + if (!set_iovec_string_field(iovec, &n_iovec, "COREDUMP_GID=", info[INFO_GID])) { + r = log_oom(); + goto finish; + } - IOVEC_SET_STRING(iovec[j++], core_slice); + if (!set_iovec_string_field(iovec, &n_iovec, "COREDUMP_SIGNAL=", info[INFO_SIGNAL])) { + r = log_oom(); + goto finish; } - if (comm) { - core_comm = strjoina("COREDUMP_COMM=", comm); - IOVEC_SET_STRING(iovec[j++], core_comm); + if (comm && !set_iovec_string_field(iovec, &n_iovec, "COREDUMP_COMM=", comm)) { + r = log_oom(); + goto finish; } - if (exe) { - core_exe = strjoina("COREDUMP_EXE=", exe); - IOVEC_SET_STRING(iovec[j++], core_exe); + if (exe && !set_iovec_string_field(iovec, &n_iovec, "COREDUMP_EXE=", exe)) { + r = log_oom(); + goto finish; } - if (get_process_cmdline(pid, 0, false, &t) >= 0) { - core_cmdline = strjoina("COREDUMP_CMDLINE=", t); - free(t); + if (sd_pid_get_session(pid, &t) >= 0) + set_iovec_field_free(iovec, &n_iovec, "COREDUMP_SESSION=", t); - IOVEC_SET_STRING(iovec[j++], core_cmdline); + if (sd_pid_get_owner_uid(pid, &owner_uid) >= 0) { + r = asprintf(&core_owner_uid, + "COREDUMP_OWNER_UID=" UID_FMT, owner_uid); + if (r > 0) + IOVEC_SET_STRING(iovec[n_iovec++], core_owner_uid); } - if (cg_pid_get_path_shifted(pid, NULL, &t) >= 0) { - core_cgroup = strjoina("COREDUMP_CGROUP=", t); - free(t); + if (sd_pid_get_slice(pid, &t) >= 0) + set_iovec_field_free(iovec, &n_iovec, "COREDUMP_SLICE=", t); - IOVEC_SET_STRING(iovec[j++], core_cgroup); - } + if (get_process_cmdline(pid, 0, false, &t) >= 0) + set_iovec_field_free(iovec, &n_iovec, "COREDUMP_CMDLINE=", t); - if (compose_open_fds(pid, &t) >= 0) { - core_open_fds = strappend("COREDUMP_OPEN_FDS=", t); - free(t); + if (cg_pid_get_path_shifted(pid, NULL, &t) >= 0) + set_iovec_field_free(iovec, &n_iovec, "COREDUMP_CGROUP=", t); - if (core_open_fds) - IOVEC_SET_STRING(iovec[j++], core_open_fds); - } + if (compose_open_fds(pid, &t) >= 0) + set_iovec_field_free(iovec, &n_iovec, "COREDUMP_OPEN_FDS=", t); p = procfs_file_alloca(pid, "status"); - if (read_full_file(p, &t, NULL) >= 0) { - core_proc_status = strappend("COREDUMP_PROC_STATUS=", t); - free(t); - - if (core_proc_status) - IOVEC_SET_STRING(iovec[j++], core_proc_status); - } + if (read_full_file(p, &t, NULL) >= 0) + set_iovec_field_free(iovec, &n_iovec, "COREDUMP_PROC_STATUS=", t); p = procfs_file_alloca(pid, "maps"); - if (read_full_file(p, &t, NULL) >= 0) { - core_proc_maps = strappend("COREDUMP_PROC_MAPS=", t); - free(t); - - if (core_proc_maps) - IOVEC_SET_STRING(iovec[j++], core_proc_maps); - } + if (read_full_file(p, &t, NULL) >= 0) + set_iovec_field_free(iovec, &n_iovec, "COREDUMP_PROC_MAPS=", t); p = procfs_file_alloca(pid, "limits"); - if (read_full_file(p, &t, NULL) >= 0) { - core_proc_limits = strappend("COREDUMP_PROC_LIMITS=", t); - free(t); - - if (core_proc_limits) - IOVEC_SET_STRING(iovec[j++], core_proc_limits); - } + if (read_full_file(p, &t, NULL) >= 0) + set_iovec_field_free(iovec, &n_iovec, "COREDUMP_PROC_LIMITS=", t); p = procfs_file_alloca(pid, "cgroup"); - if (read_full_file(p, &t, NULL) >=0) { - core_proc_cgroup = strappend("COREDUMP_PROC_CGROUP=", t); - free(t); + if (read_full_file(p, &t, NULL) >=0) + set_iovec_field_free(iovec, &n_iovec, "COREDUMP_PROC_CGROUP=", t); - if (core_proc_cgroup) - IOVEC_SET_STRING(iovec[j++], core_proc_cgroup); - } - - if (get_process_cwd(pid, &t) >= 0) { - core_cwd = strjoina("COREDUMP_CWD=", t); - free(t); + if (get_process_cwd(pid, &t) >= 0) + set_iovec_field_free(iovec, &n_iovec, "COREDUMP_CWD=", t); - IOVEC_SET_STRING(iovec[j++], core_cwd); - } + if (get_process_root(pid, &t) >= 0) + set_iovec_field_free(iovec, &n_iovec, "COREDUMP_ROOT=", t); - if (get_process_root(pid, &t) >= 0) { - core_root = strjoina("COREDUMP_ROOT=", t); - free(t); - - IOVEC_SET_STRING(iovec[j++], core_root); - } - - if (get_process_environ(pid, &t) >= 0) { - core_environ = strappend("COREDUMP_ENVIRON=", t); - free(t); - - if (core_environ) - IOVEC_SET_STRING(iovec[j++], core_environ); - } + if (get_process_environ(pid, &t) >= 0) + set_iovec_field_free(iovec, &n_iovec, "COREDUMP_ENVIRON=", t); core_timestamp = strjoin("COREDUMP_TIMESTAMP=", info[INFO_TIMESTAMP], "000000", NULL); if (core_timestamp) - IOVEC_SET_STRING(iovec[j++], core_timestamp); + IOVEC_SET_STRING(iovec[n_iovec++], core_timestamp); - IOVEC_SET_STRING(iovec[j++], "MESSAGE_ID=fc2e22bc6ee647b6b90729ab34a250b1"); - IOVEC_SET_STRING(iovec[j++], "PRIORITY=2"); + IOVEC_SET_STRING(iovec[n_iovec++], "MESSAGE_ID=fc2e22bc6ee647b6b90729ab34a250b1"); + IOVEC_SET_STRING(iovec[n_iovec++], "PRIORITY=2"); /* Vacuum before we write anything again */ coredump_vacuum(-1, arg_keep_free, arg_max_use); @@ -800,7 +749,7 @@ int main(int argc, char* argv[]) { const char *coredump_filename; coredump_filename = strjoina("COREDUMP_FILENAME=", filename); - IOVEC_SET_STRING(iovec[j++], coredump_filename); + IOVEC_SET_STRING(iovec[n_iovec++], coredump_filename); } /* Vacuum again, but exclude the coredump we just created */ @@ -838,7 +787,7 @@ int main(int argc, char* argv[]) { log: core_message = strjoin("MESSAGE=Process ", info[INFO_PID], " (", comm, ") of user ", info[INFO_UID], " dumped core.", NULL); if (core_message) - IOVEC_SET_STRING(iovec[j++], core_message); + IOVEC_SET_STRING(iovec[n_iovec++], core_message); /* Optionally store the entire coredump in the journal */ if (IN_SET(arg_storage, COREDUMP_STORAGE_JOURNAL, COREDUMP_STORAGE_BOTH) && @@ -849,13 +798,13 @@ log: r = allocate_journal_field(coredump_fd, (size_t) coredump_size, &coredump_data, &sz); if (r >= 0) { - iovec[j].iov_base = coredump_data; - iovec[j].iov_len = sz; - j++; + iovec[n_iovec].iov_base = coredump_data; + iovec[n_iovec].iov_len = sz; + n_iovec++; } } - r = sd_journal_sendv(iovec, j); + r = sd_journal_sendv(iovec, n_iovec); if (r < 0) log_error_errno(r, "Failed to log coredump: %m"); diff --git a/src/journal/journald-server.c b/src/journal/journald-server.c index 7e67e055e..c35858247 100644 --- a/src/journal/journald-server.c +++ b/src/journal/journald-server.c @@ -788,9 +788,9 @@ static void dispatch_message_real( r = get_process_cmdline(ucred->pid, 0, false, &t); if (r >= 0) { - x = strjoina("_CMDLINE=", t); - free(t); - IOVEC_SET_STRING(iovec[n++], x); + /* At most _SC_ARG_MAX (2MB usually), which is too much to put on stack. + * Let's use a heap allocation for this one. */ + set_iovec_field_free(iovec, &n, "_CMDLINE=", t); } r = get_process_capeff(ucred->pid, &t); @@ -915,11 +915,8 @@ static void dispatch_message_real( } r = get_process_cmdline(object_pid, 0, false, &t); - if (r >= 0) { - x = strjoina("OBJECT_CMDLINE=", t); - free(t); - IOVEC_SET_STRING(iovec[n++], x); - } + if (r >= 0) + set_iovec_field_free(iovec, &n, "OBJECT_CMDLINE=", t); #ifdef HAVE_AUDIT r = audit_session_from_pid(object_pid, &audit); diff --git a/src/shared/util.c b/src/shared/util.c index 78967103a..c71e021cd 100644 --- a/src/shared/util.c +++ b/src/shared/util.c @@ -2275,6 +2275,23 @@ int flush_fd(int fd) { } } +char* set_iovec_string_field(struct iovec *iovec, unsigned int *n_iovec, const char *field, const char *value) { + char *x; + + x = strappend(field, value); + if (x) + iovec[(*n_iovec)++] = IOVEC_MAKE_STRING(x); + return x; +} + +char* set_iovec_field_free(struct iovec *iovec, unsigned int *n_iovec, const char *field, char *value) { + char *x; + + x = set_iovec_string_field(iovec, n_iovec, field, value); + free(value); + return x; +} + int acquire_terminal( const char *name, bool fail, diff --git a/src/shared/util.h b/src/shared/util.h index cf096aa07..8fc237495 100644 --- a/src/shared/util.h +++ b/src/shared/util.h @@ -1140,3 +1140,10 @@ static inline void block_signals_reset(sigset_t *ss) { _t; \ }) +#define IOVEC_INIT(base, len) { .iov_base = (base), .iov_len = (len) } +#define IOVEC_MAKE(base, len) (struct iovec) IOVEC_INIT(base, len) +#define IOVEC_INIT_STRING(string) IOVEC_INIT((char*) string, strlen(string)) +#define IOVEC_MAKE_STRING(string) (struct iovec) IOVEC_INIT_STRING(string) + +char* set_iovec_string_field(struct iovec *iovec, unsigned int *n_iovec, const char *field, const char *value); +char* set_iovec_field_free(struct iovec *iovec, unsigned int *n_iovec, const char *field, char *value);