Defend against symlink attack by another process running as exim

Reported-by:
http://www.halfdog.net/Security/2016/DebianEximSpoolLocalRoot/

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)