From 38081a6ddf876345a15b92dc7d5e655b1f28976f Mon Sep 17 00:00:00 2001

From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= <zbyszek@in.waw.pl>

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