From 92b0827a90559a266bd00662d842b643ac8bdc81 Mon Sep 17 00:00:00 2001 From: Jeremy Harris Date: Thu, 22 Sep 2016 22:55:49 +0100 Subject: [PATCH] Defend against symlink attack by another process running as exim Reported-by: http://www.halfdog.net/Security/2016/DebianEximSpoolLocalRoot/ --- doc/doc-txt/ChangeLog | 5 ++ src/src/deliver.c | 166 ++++++++++++++++++++++++++---------------- src/src/exim.c | 4 +- src/src/log.c | 28 ++++++- src/src/spool_in.c | 15 +++- src/src/transport.c | 11 ++- 6 files changed, 152 insertions(+), 77 deletions(-) diff --git a/doc/doc-txt/ChangeLog b/doc/doc-txt/ChangeLog index b920d92cc..28007d01f 100644 --- a/doc/doc-txt/ChangeLog +++ b/doc/doc-txt/ChangeLog @@ -99,6 +99,11 @@ JH/26 Fix problem with one_time used on a redirect router which returned the delivered, so not attempt the (identical) child. As a result mail would be lost. +JH/27 Fix a possible security hole, wherein a process operating with the Exim + UID can gain a root shell. Credit to http://www.halfdog.net/ for + discovery and writeup. Ubuntu bug 1580454; no bug raised against Exim + itself :( + Exim version 4.87 ----------------- diff --git a/src/src/deliver.c b/src/src/deliver.c index 72405da39..357c60702 100644 --- a/src/src/deliver.c +++ b/src/src/deliver.c @@ -268,6 +268,8 @@ msglog directory that are used to catch output from pipes. Try to create the directory if it does not exist. From release 4.21, normal message logs should be created when the message is received. +Called from deliver_message(), can be operating as root. + Argument: filename the file name mode the mode required @@ -279,37 +281,49 @@ Returns: a file descriptor, or -1 (with errno set) static int open_msglog_file(uschar *filename, int mode, uschar **error) { -int fd = Uopen(filename, O_WRONLY|O_APPEND|O_CREAT, mode); +int fd, i; -if (fd < 0 && errno == ENOENT) +for (i = 2; i > 0; i--) { + fd = Uopen(filename, +#ifdef O_CLOEXEC + O_CLOEXEC | +#endif +#ifdef O_NOFOLLOW + O_NOFOLLOW | +#endif + O_WRONLY|O_APPEND|O_CREAT, mode); + if (fd >= 0) + { + /* Set the close-on-exec flag and change the owner to the exim uid/gid (this + function is called as root). Double check the mode, because the group setting + doesn't always get set automatically. */ + +#ifndef O_CLOEXEC + (void)fcntl(fd, F_SETFD, fcntl(fd, F_GETFD) | FD_CLOEXEC); +#endif + if (fchown(fd, exim_uid, exim_gid) < 0) + { + *error = US"chown"; + return -1; + } + if (fchmod(fd, mode) < 0) + { + *error = US"chmod"; + return -1; + } + return fd; + } + if (errno != ENOENT) + break; + (void)directory_make(spool_directory, spool_sname(US"msglog", message_subdir), MSGLOG_DIRECTORY_MODE, TRUE); - fd = Uopen(filename, O_WRONLY|O_APPEND|O_CREAT, mode); - } - -/* Set the close-on-exec flag and change the owner to the exim uid/gid (this -function is called as root). Double check the mode, because the group setting -doesn't always get set automatically. */ - -if (fd >= 0) - { - (void)fcntl(fd, F_SETFD, fcntl(fd, F_GETFD) | FD_CLOEXEC); - if (fchown(fd, exim_uid, exim_gid) < 0) - { - *error = US"chown"; - return -1; - } - if (fchmod(fd, mode) < 0) - { - *error = US"chmod"; - return -1; - } } -else *error = US"create"; -return fd; +*error = US"create"; +return -1; } @@ -4597,15 +4611,20 @@ for (delivery_count = 0; addr_remote; delivery_count++) { uschar * fname = spool_fname(US"input", message_subdir, message_id, US"-D"); - if ((deliver_datafile = Uopen(fname, O_RDWR | O_APPEND, 0)) < 0) + if ((deliver_datafile = Uopen(fname, +#ifdef O_CLOEXEC + O_CLOEXEC | +#endif + O_RDWR | O_APPEND, 0)) < 0) log_write(0, LOG_MAIN|LOG_PANIC_DIE, "Failed to reopen %s for remote " "parallel delivery: %s", fname, strerror(errno)); } /* Set the close-on-exec flag */ - +#ifndef O_CLOEXEC (void)fcntl(deliver_datafile, F_SETFD, fcntl(deliver_datafile, F_GETFD) | FD_CLOEXEC); +#endif /* Set the uid/gid of this process; bombs out on failure. */ @@ -5348,6 +5367,8 @@ A delivery operation has a process all to itself; we never deliver more than one message in the same process. Therefore we needn't worry too much about store leakage. +Liable to be called as root. + Arguments: id the id of the message to be delivered forced TRUE if delivery was forced by an administrator; this overrides @@ -5372,7 +5393,6 @@ int final_yield = DELIVER_ATTEMPTED_NORMAL; time_t now = time(NULL); address_item *addr_last = NULL; uschar *filter_message = NULL; -FILE *jread; int process_recipients = RECIP_ACCEPT; open_db dbblock; open_db *dbm_file; @@ -5512,8 +5532,19 @@ Otherwise it might be needed again. */ { uschar * fname = spool_fname(US"input", message_subdir, id, US"-J"); + FILE * jread; - if ((jread = Ufopen(fname, "rb"))) + if ( (journal_fd = Uopen(fname, O_RDWR|O_APPEND +#ifdef O_CLOEXEC + | O_CLOEXEC +#endif +#ifdef O_NOFOLLOW + | O_NOFOLLOW +#endif + , SPOOL_MODE)) >= 0 + && lseek(journal_fd, 0, SEEK_SET) == 0 + && (jread = fdopen(journal_fd, "rb")) + ) { while (Ufgets(big_buffer, big_buffer_size, jread)) { @@ -5523,7 +5554,8 @@ Otherwise it might be needed again. */ DEBUG(D_deliver) debug_printf("Previously delivered address %s taken from " "journal file\n", big_buffer); } - (void)fclose(jread); + rewind(jread); + journal_fd = fileno(jread); /* Panic-dies on error */ (void)spool_write_header(message_id, SW_DELIVERING, NULL); } @@ -5911,22 +5943,18 @@ else if (system_filter && process_recipients != RECIP_FAIL_TIMEOUT) tpname = tmp; } else - { p->message = string_sprintf("system_filter_%s_transport is unset", type); - } if (tpname) { transport_instance *tp; for (tp = transports; tp; tp = tp->next) - { if (Ustrcmp(tp->name, tpname) == 0) { p->transport = tp; break; } - } if (!tp) p->message = string_sprintf("failed to find \"%s\" transport " "for system filter delivery", tpname); @@ -6839,10 +6867,8 @@ there is only address to be delivered - if it succeeds the spool write need not happen. */ if ( header_rewritten - && ( ( addr_local - && (addr_local->next || addr_remote) - ) - || (addr_remote && addr_remote->next) + && ( addr_local && (addr_local->next || addr_remote) + || addr_remote && addr_remote->next ) ) { /* Panic-dies on error */ @@ -6851,10 +6877,10 @@ if ( header_rewritten } -/* If there are any deliveries to be done, open the journal file. This is used -to record successful deliveries as soon as possible after each delivery is -known to be complete. A file opened with O_APPEND is used so that several -processes can run simultaneously. +/* If there are any deliveries to be and we do not already have the journal +file, create it. This is used to record successful deliveries as soon as +possible after each delivery is known to be complete. A file opened with +O_APPEND is used so that several processes can run simultaneously. The journal is just insurance against crashes. When the spool file is ultimately updated at the end of processing, the journal is deleted. If a @@ -6863,33 +6889,47 @@ therein are added to the non-recipients. */ if (addr_local || addr_remote) { - uschar * fname = spool_fname(US"input", message_subdir, id, US"-J"); - - if ((journal_fd = Uopen(fname, O_WRONLY|O_APPEND|O_CREAT, SPOOL_MODE)) <0) + if (journal_fd < 0) { - log_write(0, LOG_MAIN|LOG_PANIC, "Couldn't open journal file %s: %s", - fname, strerror(errno)); - return DELIVER_NOT_ATTEMPTED; - } + uschar * fname = spool_fname(US"input", message_subdir, id, US"-J"); + + if ((journal_fd = Uopen(fname, +#ifdef O_CLOEXEC + O_CLOEXEC | +#endif + O_WRONLY|O_APPEND|O_CREAT|O_EXCL, SPOOL_MODE)) < 0) + { + log_write(0, LOG_MAIN|LOG_PANIC, "Couldn't open journal file %s: %s", + fname, strerror(errno)); + return DELIVER_NOT_ATTEMPTED; + } - /* Set the close-on-exec flag, make the file owned by Exim, and ensure - that the mode is correct - the group setting doesn't always seem to get - set automatically. */ + /* Set the close-on-exec flag, make the file owned by Exim, and ensure + that the mode is correct - the group setting doesn't always seem to get + set automatically. */ - if( fcntl(journal_fd, F_SETFD, fcntl(journal_fd, F_GETFD) | FD_CLOEXEC) - || fchown(journal_fd, exim_uid, exim_gid) - || fchmod(journal_fd, SPOOL_MODE) - ) - { - int ret = Uunlink(fname); - log_write(0, LOG_MAIN|LOG_PANIC, "Couldn't set perms on journal file %s: %s", - fname, strerror(errno)); - if(ret && errno != ENOENT) - log_write(0, LOG_MAIN|LOG_PANIC_DIE, "failed to unlink %s: %s", - fname, strerror(errno)); - return DELIVER_NOT_ATTEMPTED; + if( fchown(journal_fd, exim_uid, exim_gid) + || fchmod(journal_fd, SPOOL_MODE) +#ifndef O_CLOEXEC + || fcntl(journal_fd, F_SETFD, fcntl(journal_fd, F_GETFD) | FD_CLOEXEC) +#endif + ) + { + int ret = Uunlink(fname); + log_write(0, LOG_MAIN|LOG_PANIC, "Couldn't set perms on journal file %s: %s", + fname, strerror(errno)); + if(ret && errno != ENOENT) + log_write(0, LOG_MAIN|LOG_PANIC_DIE, "failed to unlink %s: %s", + fname, strerror(errno)); + return DELIVER_NOT_ATTEMPTED; + } } } +else if (journal_fd >= 0) + { + close(journal_fd); + journal_fd = -1; + } diff --git a/src/src/exim.c b/src/src/exim.c index f2d0e9e65..91636fb30 100644 --- a/src/src/exim.c +++ b/src/src/exim.c @@ -174,10 +174,8 @@ Returns: nothing void set_process_info(const char *format, ...) { -int len; +int len = sprintf(CS process_info, "%5d ", (int)getpid()); va_list ap; -sprintf(CS process_info, "%5d ", (int)getpid()); -len = Ustrlen(process_info); va_start(ap, format); if (!string_vformat(process_info + len, PROCESS_INFO_SIZE - len - 2, format, ap)) Ustrcpy(process_info + len, "**** string overflowed buffer ****"); diff --git a/src/src/log.c b/src/src/log.c index 032eb876f..f9b0722d8 100644 --- a/src/src/log.c +++ b/src/src/log.c @@ -252,7 +252,11 @@ Returns: a file descriptor, or < 0 on failure (errno set) int log_create(uschar *name) { -int fd = Uopen(name, O_CREAT|O_APPEND|O_WRONLY, LOG_MODE); +int fd = Uopen(name, +#ifdef O_CLOEXEC + O_CLOEXEC | +#endif + O_CREAT|O_APPEND|O_WRONLY, LOG_MODE); /* If creation failed, attempt to build a log directory in case that is the problem. */ @@ -266,7 +270,11 @@ if (fd < 0 && errno == ENOENT) DEBUG(D_any) debug_printf("%s log directory %s\n", created? "created" : "failed to create", name); *lastslash = '/'; - if (created) fd = Uopen(name, O_CREAT|O_APPEND|O_WRONLY, LOG_MODE); + if (created) fd = Uopen(name, +#ifdef O_CLOEXEC + O_CLOEXEC | +#endif + O_CREAT|O_APPEND|O_WRONLY, LOG_MODE); } return fd; @@ -316,7 +324,11 @@ if (pid == 0) /* If we created a subprocess, wait for it. If it succeeded, try the open. */ while (pid > 0 && waitpid(pid, &status, 0) != pid); -if (status == 0) fd = Uopen(name, O_APPEND|O_WRONLY, LOG_MODE); +if (status == 0) fd = Uopen(name, +#ifdef O_CLOEXEC + O_CLOEXEC | +#endif + O_APPEND|O_WRONLY, LOG_MODE); /* If we failed to create a subprocess, we are in a bad way. We return with fd still < 0, and errno set, letting the caller handle the error. */ @@ -438,11 +450,17 @@ if (!ok) /* We now have the file name. Try to open an existing file. After a successful open, arrange for automatic closure on exec(), and then return. */ -*fd = Uopen(buffer, O_APPEND|O_WRONLY, LOG_MODE); +*fd = Uopen(buffer, +#ifdef O_CLOEXEC + O_CLOEXEC | +#endif + O_APPEND|O_WRONLY, LOG_MODE); if (*fd >= 0) { +#ifndef O_CLOEXEC (void)fcntl(*fd, F_SETFD, fcntl(*fd, F_GETFD) | FD_CLOEXEC); +#endif return; } @@ -469,7 +487,9 @@ else if (euid == root_uid) *fd = log_create_as_exim(buffer); if (*fd >= 0) { +#ifndef O_CLOEXEC (void)fcntl(*fd, F_SETFD, fcntl(*fd, F_GETFD) | FD_CLOEXEC); +#endif return; } diff --git a/src/src/spool_in.c b/src/src/spool_in.c index 1b7cee6b0..3592fa7b6 100644 --- a/src/src/spool_in.c +++ b/src/src/spool_in.c @@ -25,6 +25,8 @@ fact it won't be written to. Just in case there's a major disaster (e.g. overwriting some other file descriptor with the value of this one), open it with append. +As called by deliver_message() (at least) we are operating as root. + Argument: the id of the message Returns: fd if file successfully opened and locked, else -1 @@ -55,7 +57,11 @@ for (i = 0; i < 2; i++) fname = spool_fname(US"input", message_subdir, id, US"-D"); DEBUG(D_deliver) debug_printf("Trying spool file %s\n", fname); - if ((fd = Uopen(fname, O_RDWR | O_APPEND, 0)) >= 0) + if ((fd = Uopen(fname, +#ifdef O_CLOEXEC + O_CLOEXEC | +#endif + O_RDWR | O_APPEND, 0)) >= 0) break; save_errno = errno; if (errno == ENOENT) @@ -81,8 +87,9 @@ an open file descriptor (at least, I think that's the Cygwin story). On real Unix systems it doesn't make any difference as long as Exim is consistent in what it locks. */ -(void)fcntl(fd, F_SETFD, fcntl(fd, F_GETFD) | - FD_CLOEXEC); +#ifndef O_CLOEXEC +(void)fcntl(fd, F_SETFD, fcntl(fd, F_GETFD) | FD_CLOEXEC); +#endif lock_data.l_type = F_WRLCK; lock_data.l_whence = SEEK_SET; @@ -215,6 +222,8 @@ have been the cause of that incident, but in any case, this code must be robust against such an event, and if such a file is encountered, it must be treated as malformed. +As called from deliver_message() (at least) we are running as root. + Arguments: name name of the header file, including the -H read_headers TRUE if in-store header structures are to be built diff --git a/src/src/transport.c b/src/src/transport.c index 33f9a580a..6ec5f3720 100644 --- a/src/src/transport.c +++ b/src/src/transport.c @@ -1281,10 +1281,13 @@ save_errno = 0; yield = FALSE; write_pid = (pid_t)(-1); -(void)fcntl(fd, F_SETFD, fcntl(fd, F_GETFD) | FD_CLOEXEC); -filter_pid = child_open(USS transport_filter_argv, NULL, 077, - &fd_write, &fd_read, FALSE); -(void)fcntl(fd, F_SETFD, fcntl(fd, F_GETFD) & ~FD_CLOEXEC); + { + int bits = fcntl(fd, F_GETFD); + (void)fcntl(fd, F_SETFD, bits | FD_CLOEXEC); + filter_pid = child_open(USS transport_filter_argv, NULL, 077, + &fd_write, &fd_read, FALSE); + (void)fcntl(fd, F_SETFD, bits & ~FD_CLOEXEC); + } if (filter_pid < 0) goto TIDY_UP; /* errno set */ DEBUG(D_transport) -- 2.25.1