CVE-2020-28007: Link attack in Exim's log directory

We patch this vulnerability by opening (instead of just creating) the
log file in an unprivileged (exim) child process, and by passing this
file descriptor back to the privileged (root) parent process. The two
functions log_send_fd() and log_recv_fd() are inspired by OpenSSH's
functions mm_send_fd() and mm_receive_fd(); thanks!

This patch also fixes:

- a NULL-pointer dereference in usr1_handler() (this signal handler is
  installed before process_log_path is initialized);

- a file-descriptor leak in dmarc_write_history_file() (two return paths
  did not close history_file_fd).

Note: the use of log_open_as_exim() in dmarc_write_history_file() should
be fine because the documentation explicitly states "Make sure the
directory of this file is writable by the user exim runs as."

(cherry picked from commit 2502cc41d1d92c1413eca6a4ba035c21162662bd)

diff --git a/src/src/dmarc.c b/src/src/dmarc.c
index 2e43f846d39155e8116a7f067bc20a30a4292e2e..ef2b3197e42d5eb1bdbf19d3df3647cee8a473ab 100644 (file)
--- a/src/src/dmarc.c
+++ b/src/src/dmarc.c
@@ -223,6 +223,97 @@ return NULL;
 }
 
 
 }
 
 

+static int
+dmarc_write_history_file()
+{
+int tmp_ans;
+u_char **rua; /* aggregate report addressees */
+uschar *history_buffer = NULL;
+
+if (!dmarc_history_file)
+  {
+  DEBUG(D_receive) debug_printf("DMARC history file not set\n");
+  return DMARC_HIST_DISABLED;
+  }
+
+/* Generate the contents of the history file */
+history_buffer = string_sprintf(
+  "job %s\nreporter %s\nreceived %ld\nipaddr %s\nfrom %s\nmfrom %s\n",
+  message_id, primary_hostname, time(NULL), sender_host_address,
+  header_from_sender, expand_string(US"$sender_address_domain"));
+
+if (spf_response)
+  history_buffer = string_sprintf("%sspf %d\n", history_buffer, dmarc_spf_ares_result);
+  /* history_buffer = string_sprintf("%sspf -1\n", history_buffer); */
+
+history_buffer = string_sprintf(
+  "%s%spdomain %s\npolicy %d\n",
+  history_buffer, dkim_history_buffer, dmarc_used_domain, dmarc_policy);
+
+if ((rua = opendmarc_policy_fetch_rua(dmarc_pctx, NULL, 0, 1)))
+  for (tmp_ans = 0; rua[tmp_ans]; tmp_ans++)
+    history_buffer = string_sprintf("%srua %s\n", history_buffer, rua[tmp_ans]);
+else
+  history_buffer = string_sprintf("%srua -\n", history_buffer);
+
+opendmarc_policy_fetch_pct(dmarc_pctx, &tmp_ans);
+history_buffer = string_sprintf("%spct %d\n", history_buffer, tmp_ans);
+
+opendmarc_policy_fetch_adkim(dmarc_pctx, &tmp_ans);
+history_buffer = string_sprintf("%sadkim %d\n", history_buffer, tmp_ans);
+
+opendmarc_policy_fetch_aspf(dmarc_pctx, &tmp_ans);
+history_buffer = string_sprintf("%saspf %d\n", history_buffer, tmp_ans);
+
+opendmarc_policy_fetch_p(dmarc_pctx, &tmp_ans);
+history_buffer = string_sprintf("%sp %d\n", history_buffer, tmp_ans);
+
+opendmarc_policy_fetch_sp(dmarc_pctx, &tmp_ans);
+history_buffer = string_sprintf("%ssp %d\n", history_buffer, tmp_ans);
+
+history_buffer = string_sprintf(
+  "%salign_dkim %d\nalign_spf %d\naction %d\n",
+  history_buffer, da, sa, action);
+
+/* Write the contents to the history file */
+DEBUG(D_receive)
+  debug_printf("DMARC logging history data for opendmarc reporting%s\n",
+            (host_checking || f.running_in_test_harness) ? " (not really)" : "");
+if (host_checking || f.running_in_test_harness)
+  {
+  DEBUG(D_receive)
+    debug_printf("DMARC history data for debugging:\n%s", history_buffer);
+  }
+else
+  {
+  ssize_t written_len;
+  const int history_file_fd = log_open_as_exim(dmarc_history_file);
+
+  if (history_file_fd < 0)
+    {
+    log_write(0, LOG_MAIN|LOG_PANIC, "failure to create DMARC history file: %s",
+                          dmarc_history_file);
+    return DMARC_HIST_FILE_ERR;
+    }
+
+  written_len = write_to_fd_buf(history_file_fd,
+                               history_buffer,
+                               Ustrlen(history_buffer));
+
+  (void)close(history_file_fd);
+
+  if (written_len <= 0)
+    {
+    log_write(0, LOG_MAIN|LOG_PANIC, "failure to write to DMARC history file: %s",
+                          dmarc_history_file);
+    return DMARC_HIST_WRITE_ERR;
+    }
+  }
+return DMARC_HIST_OK;
+}
+
+
+>>>>>>> a378ff99d... CVE-2020-28007: Link attack in Exim's log directory

 /* dmarc_process adds the envelope sender address to the existing
 context (if any), retrieves the result, sets up expansion
 strings and evaluates the condition outcome. */
 /* dmarc_process adds the envelope sender address to the existing
 context (if any), retrieves the result, sets up expansion
 strings and evaluates the condition outcome. */


@@ -514,94 +605,6 @@ if (!f.dmarc_disable_verify)
 return OK;
 }
 
 return OK;
 }
 

-static int
-dmarc_write_history_file()
-{
-int history_file_fd;
-ssize_t written_len;
-int tmp_ans;
-u_char **rua; /* aggregate report addressees */
-uschar *history_buffer = NULL;
-
-if (!dmarc_history_file)
-  {
-  DEBUG(D_receive) debug_printf("DMARC history file not set\n");
-  return DMARC_HIST_DISABLED;
-  }
-history_file_fd = log_create(dmarc_history_file);
-
-if (history_file_fd < 0)
-  {
-  log_write(0, LOG_MAIN|LOG_PANIC, "failure to create DMARC history file: %s",
-                          dmarc_history_file);
-  return DMARC_HIST_FILE_ERR;
-  }
-
-/* Generate the contents of the history file */
-history_buffer = string_sprintf(
-  "job %s\nreporter %s\nreceived %ld\nipaddr %s\nfrom %s\nmfrom %s\n",
-  message_id, primary_hostname, time(NULL), sender_host_address,
-  header_from_sender, expand_string(US"$sender_address_domain"));
-
-if (spf_response)
-  history_buffer = string_sprintf("%sspf %d\n", history_buffer, dmarc_spf_ares_result);
-  /* history_buffer = string_sprintf("%sspf -1\n", history_buffer); */
-
-history_buffer = string_sprintf(
-  "%s%spdomain %s\npolicy %d\n",
-  history_buffer, dkim_history_buffer, dmarc_used_domain, dmarc_policy);
-
-if ((rua = opendmarc_policy_fetch_rua(dmarc_pctx, NULL, 0, 1)))
-  for (tmp_ans = 0; rua[tmp_ans]; tmp_ans++)
-    history_buffer = string_sprintf("%srua %s\n", history_buffer, rua[tmp_ans]);
-else
-  history_buffer = string_sprintf("%srua -\n", history_buffer);
-
-opendmarc_policy_fetch_pct(dmarc_pctx, &tmp_ans);
-history_buffer = string_sprintf("%spct %d\n", history_buffer, tmp_ans);
-
-opendmarc_policy_fetch_adkim(dmarc_pctx, &tmp_ans);
-history_buffer = string_sprintf("%sadkim %d\n", history_buffer, tmp_ans);
-
-opendmarc_policy_fetch_aspf(dmarc_pctx, &tmp_ans);
-history_buffer = string_sprintf("%saspf %d\n", history_buffer, tmp_ans);
-
-opendmarc_policy_fetch_p(dmarc_pctx, &tmp_ans);
-history_buffer = string_sprintf("%sp %d\n", history_buffer, tmp_ans);
-
-opendmarc_policy_fetch_sp(dmarc_pctx, &tmp_ans);
-history_buffer = string_sprintf("%ssp %d\n", history_buffer, tmp_ans);
-
-history_buffer = string_sprintf(
-  "%salign_dkim %d\nalign_spf %d\naction %d\n",
-  history_buffer, da, sa, action);
-
-/* Write the contents to the history file */
-DEBUG(D_receive)
-  debug_printf("DMARC logging history data for opendmarc reporting%s\n",
-            (host_checking || f.running_in_test_harness) ? " (not really)" : "");
-if (host_checking || f.running_in_test_harness)
-  {
-  DEBUG(D_receive)
-    debug_printf("DMARC history data for debugging:\n%s", history_buffer);
-  }
-else
-  {
-  written_len = write_to_fd_buf(history_file_fd,
-                               history_buffer,
-                               Ustrlen(history_buffer));
-  if (written_len == 0)
-    {
-    log_write(0, LOG_MAIN|LOG_PANIC, "failure to write to DMARC history file: %s",
-                          dmarc_history_file);
-    return DMARC_HIST_WRITE_ERR;
-    }
-  (void)close(history_file_fd);
-  }
-return DMARC_HIST_OK;
-}
-
-

 uschar *
 dmarc_exim_expand_query(int what)
 {
 uschar *
 dmarc_exim_expand_query(int what)
 {

diff --git a/src/src/exim.c b/src/src/exim.c
index 975b39a5828d596f4ad6f911631143dde67ad3fd..29e457fe276775360ec50acd24c8cb4da5362a85 100644 (file)
--- a/src/src/exim.c
+++ b/src/src/exim.c
@@ -233,18 +233,8 @@ int fd;
 
 os_restarting_signal(sig, usr1_handler);
 
 
 os_restarting_signal(sig, usr1_handler);
 

-if ((fd = Uopen(process_log_path, O_APPEND|O_WRONLY, LOG_MODE)) < 0)
-  {
-  /* If we are already running as the Exim user, try to create it in the
-  current process (assuming spool_directory exists). Otherwise, if we are
-  root, do the creation in an exim:exim subprocess. */
-
-  int euid = geteuid();
-  if (euid == exim_uid)
-    fd = Uopen(process_log_path, O_CREAT|O_APPEND|O_WRONLY, LOG_MODE);
-  else if (euid == root_uid)
-    fd = log_create_as_exim(process_log_path);
-  }
+if (!process_log_path) return;
+fd = log_open_as_exim(process_log_path);

 
 /* If we are neither exim nor root, or if we failed to create the log file,
 give up. There is not much useful we can do with errors, since we don't want
 
 /* If we are neither exim nor root, or if we failed to create the log file,
 give up. There is not much useful we can do with errors, since we don't want

diff --git a/src/src/functions.h b/src/src/functions.h
index fc4e0444d4247b2dac9ded5f74172b436d3ed45c..e22fd4f9954a39a4f328239a7b43a338bb332a14 100644 (file)
--- a/src/src/functions.h
+++ b/src/src/functions.h
@@ -308,8 +308,7 @@ extern int     ip_streamsocket(const uschar *, uschar **, int, host_item *);
 extern int     ipv6_nmtoa(int *, uschar *);
 
 extern uschar *local_part_quote(uschar *);
 extern int     ipv6_nmtoa(int *, uschar *);
 
 extern uschar *local_part_quote(uschar *);

-extern int     log_create(uschar *);
-extern int     log_create_as_exim(uschar *);
+extern int     log_open_as_exim(uschar *);

 extern void    log_close_all(void);
 
 extern macro_item * macro_create(const uschar *, const uschar *, BOOL);
 extern void    log_close_all(void);
 
 extern macro_item * macro_create(const uschar *, const uschar *, BOOL);

diff --git a/src/src/log.c b/src/src/log.c
index 07bf2ce7f601b6def317c0793df2cb119015879c..5d36b498391aececef408921927ced34286d0010 100644 (file)
--- a/src/src/log.c
+++ b/src/src/log.c
@@ -264,14 +264,19 @@ overwrite it temporarily if it is necessary to create the directory.
 Returns:       a file descriptor, or < 0 on failure (errno set)
 */
 
 Returns:       a file descriptor, or < 0 on failure (errno set)
 */
 

-int
-log_create(uschar *name)
+static int
+log_open_already_exim(uschar * const name)

 {
 {

-int fd = Uopen(name,
-#ifdef O_CLOEXEC
-       O_CLOEXEC |
-#endif
-       O_CREAT|O_APPEND|O_WRONLY, LOG_MODE);
+int fd = -1;
+const int flags = O_WRONLY | O_APPEND | O_CREAT | O_NONBLOCK;
+
+if (geteuid() != exim_uid)
+  {
+  errno = EACCES;
+  return -1;
+  }
+
+fd = Uopen(name, flags, LOG_MODE);

 
 /* If creation failed, attempt to build a log directory in case that is the
 problem. */
 
 /* If creation failed, attempt to build a log directory in case that is the
 problem. */


@@ -285,11 +290,7 @@ if (fd < 0 && errno == ENOENT)
   DEBUG(D_any) debug_printf("%s log directory %s\n",
     created ? "created" : "failed to create", name);
   *lastslash = '/';
   DEBUG(D_any) debug_printf("%s log directory %s\n",
     created ? "created" : "failed to create", name);
   *lastslash = '/';

-  if (created) fd = Uopen(name,
-#ifdef O_CLOEXEC
-                       O_CLOEXEC |
-#endif
-                       O_CREAT|O_APPEND|O_WRONLY, LOG_MODE);
+  if (created) fd = Uopen(name, flags, LOG_MODE);

   }
 
 return fd;
   }
 
 return fd;


@@ -297,6 +298,81 @@ return fd;
 
 
 
 
 
 

+/* Inspired by OpenSSH's mm_send_fd(). Thanks! */
+
+static int
+log_send_fd(const int sock, const int fd)
+{
+struct msghdr msg;
+union {
+  struct cmsghdr hdr;
+  char buf[CMSG_SPACE(sizeof(int))];
+} cmsgbuf;
+struct cmsghdr *cmsg;
+struct iovec vec;
+char ch = 'A';
+ssize_t n;
+
+memset(&msg, 0, sizeof(msg));
+memset(&cmsgbuf, 0, sizeof(cmsgbuf));
+msg.msg_control = &cmsgbuf.buf;
+msg.msg_controllen = sizeof(cmsgbuf.buf);
+
+cmsg = CMSG_FIRSTHDR(&msg);
+cmsg->cmsg_len = CMSG_LEN(sizeof(int));
+cmsg->cmsg_level = SOL_SOCKET;
+cmsg->cmsg_type = SCM_RIGHTS;
+*(int *)CMSG_DATA(cmsg) = fd;
+
+vec.iov_base = &ch;
+vec.iov_len = 1;
+msg.msg_iov = &vec;
+msg.msg_iovlen = 1;
+
+while ((n = sendmsg(sock, &msg, 0)) == -1 && errno == EINTR);
+if (n != 1) return -1;
+return 0;
+}
+
+/* Inspired by OpenSSH's mm_receive_fd(). Thanks! */
+
+static int
+log_recv_fd(const int sock)
+{
+struct msghdr msg;
+union {
+  struct cmsghdr hdr;
+  char buf[CMSG_SPACE(sizeof(int))];
+} cmsgbuf;
+struct cmsghdr *cmsg;
+struct iovec vec;
+ssize_t n;
+char ch = '\0';
+int fd = -1;
+
+memset(&msg, 0, sizeof(msg));
+vec.iov_base = &ch;
+vec.iov_len = 1;
+msg.msg_iov = &vec;
+msg.msg_iovlen = 1;
+
+memset(&cmsgbuf, 0, sizeof(cmsgbuf));
+msg.msg_control = &cmsgbuf.buf;
+msg.msg_controllen = sizeof(cmsgbuf.buf);
+
+while ((n = recvmsg(sock, &msg, 0)) == -1 && errno == EINTR);
+if (n != 1 || ch != 'A') return -1;
+
+cmsg = CMSG_FIRSTHDR(&msg);
+if (cmsg == NULL) return -1;
+if (cmsg->cmsg_type != SCM_RIGHTS) return -1;
+fd = *(const int *)CMSG_DATA(cmsg);
+if (fd < 0) return -1;
+return fd;
+}
+
+
+

 /*************************************************
 *     Create a log file as the exim user         *
 *************************************************/
 /*************************************************
 *     Create a log file as the exim user         *
 *************************************************/


@@ -312,41 +388,60 @@ Returns:       a file descriptor, or < 0 on failure (errno set)
 */
 
 int
 */
 
 int

-log_create_as_exim(uschar *name)
+log_open_as_exim(uschar * const name)

 {
 {

-pid_t pid = exim_fork(US"logfile-create");
-int status = 1;

 int fd = -1;
 int fd = -1;

+const uid_t euid = geteuid();

 
 

-/* In the subprocess, change uid/gid and do the creation. Return 0 from the
-subprocess on success. If we don't check for setuid failures, then the file
-can be created as root, so vulnerabilities which cause setuid to fail mean
-that the Exim user can use symlinks to cause a file to be opened/created as
-root. We always open for append, so can't nuke existing content but it would
-still be Rather Bad. */
-
-if (pid == 0)
+if (euid == exim_uid)

   {
   {

-  if (setgid(exim_gid) < 0)
-    die(US"exim: setgid for log-file creation failed, aborting",
-      US"Unexpected log failure, please try later");
-  if (setuid(exim_uid) < 0)
-    die(US"exim: setuid for log-file creation failed, aborting",
-      US"Unexpected log failure, please try later");
-  _exit((log_create(name) < 0)? 1 : 0);
+  fd = log_open_already_exim(name);

   }
   }

+else if (euid == root_uid)
+  {
+  int sock[2];
+  if (socketpair(AF_UNIX, SOCK_STREAM, 0, sock) == 0)
+    {
+    const pid_t pid = exim_fork(US"logfile-open");
+    if (pid == 0)
+      {
+      (void)close(sock[0]);
+      if (setgroups(1, &exim_gid) != 0) _exit(EXIT_FAILURE);
+      if (setgid(exim_gid) != 0) _exit(EXIT_FAILURE);
+      if (setuid(exim_uid) != 0) _exit(EXIT_FAILURE);
+
+      if (getuid() != exim_uid || geteuid() != exim_uid) _exit(EXIT_FAILURE);
+      if (getgid() != exim_gid || getegid() != exim_gid) _exit(EXIT_FAILURE);
+
+      fd = log_open_already_exim(name);
+      if (fd < 0) _exit(EXIT_FAILURE);
+      if (log_send_fd(sock[1], fd) != 0) _exit(EXIT_FAILURE);
+      (void)close(sock[1]);
+      _exit(EXIT_SUCCESS);
+      }

 
 

-/* 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,
-#ifdef O_CLOEXEC
-                       O_CLOEXEC |
-#endif
-                       O_APPEND|O_WRONLY, LOG_MODE);
+    (void)close(sock[1]);
+    if (pid > 0)
+      {
+      fd = log_recv_fd(sock[0]);
+      while (waitpid(pid, NULL, 0) == -1 && errno == EINTR);
+      }
+    (void)close(sock[0]);
+    }
+  }

 
 

-/* 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. */
+if (fd >= 0)
+  {
+  int flags;
+  flags = fcntl(fd, F_GETFD);
+  if (flags != -1) (void)fcntl(fd, F_SETFD, flags | FD_CLOEXEC);
+  flags = fcntl(fd, F_GETFL);
+  if (flags != -1) (void)fcntl(fd, F_SETFL, flags & ~O_NONBLOCK);
+  }
+else
+  {
+  errno = EACCES;
+  }

 
 return fd;
 }
 
 return fd;
 }


@@ -459,52 +554,17 @@ if (!ok)
   die(US"exim: log file path too long: aborting",
       US"Logging failure; please try later");
 
   die(US"exim: log file path too long: aborting",
       US"Logging failure; please try later");
 

-/* 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. */
+/* We now have the file name. After a successful open, return. */

 
 

-*fd = Uopen(buffer,
-#ifdef O_CLOEXEC
-               O_CLOEXEC |
-#endif
-               O_APPEND|O_WRONLY, LOG_MODE);
+*fd = log_open_as_exim(buffer);

 
 if (*fd >= 0)
   {
 
 if (*fd >= 0)
   {

-#ifndef O_CLOEXEC
-  (void)fcntl(*fd, F_SETFD, fcntl(*fd, F_GETFD) | FD_CLOEXEC);
-#endif

   return;
   }
 
   return;
   }
 

-/* Open was not successful: try creating the file. If this is a root process,
-we must do the creating in a subprocess set to exim:exim in order to ensure
-that the file is created with the right ownership. Otherwise, there can be a
-race if another Exim process is trying to write to the log at the same time.
-The use of SIGUSR1 by the exiwhat utility can provoke a lot of simultaneous
-writing. */
-

 euid = geteuid();
 
 euid = geteuid();
 

-/* If we are already running as the Exim user (even if that user is root),
-we can go ahead and create in the current process. */
-
-if (euid == exim_uid) *fd = log_create(buffer);
-
-/* Otherwise, if we are root, do the creation in an exim:exim subprocess. If we
-are neither exim nor root, creation is not attempted. */
-
-else if (euid == root_uid) *fd = log_create_as_exim(buffer);
-
-/* If we now have an open file, set the close-on-exec flag and return. */
-
-if (*fd >= 0)
-  {
-#ifndef O_CLOEXEC
-  (void)fcntl(*fd, F_SETFD, fcntl(*fd, F_GETFD) | FD_CLOEXEC);
-#endif
-  return;
-  }
-

 /* Creation failed. There are some circumstances in which we get here when
 the effective uid is not root or exim, which is the problem. (For example, a
 non-setuid binary with log_arguments set, called in certain ways.) Rather than
 /* Creation failed. There are some circumstances in which we get here when
 the effective uid is not root or exim, which is the problem. (For example, a
 non-setuid binary with log_arguments set, called in certain ways.) Rather than

diff --git a/test/stderr/0397 b/test/stderr/0397
index 82f14378783efc1ef1a8d110e6cc46c246676b64..4d9dcaa475d57fbda378b32225a54c95ce9b84cd 100644 (file)
--- a/test/stderr/0397
+++ b/test/stderr/0397
@@ -1,7 +1,7 @@
-1999-03-02 09:44:33 Cannot open main log file "/non/existent/path/to/force/failure/main": No such file or directory: euid=uuuu egid=EXIM_GID
+1999-03-02 09:44:33 Cannot open main log file "/non/existent/path/to/force/failure/main": Permission denied: euid=uuuu egid=EXIM_GID

 1999-03-02 09:44:33 Start queue run: pid=pppp
 1999-03-02 09:44:33 Start queue run: pid=pppp

-1999-03-02 09:44:33 Cannot open main log file "/non/existent/path/to/force/failure/main": No such file or directory: euid=uuuu egid=EXIM_GID
+1999-03-02 09:44:33 Cannot open main log file "/non/existent/path/to/force/failure/main": Permission denied: euid=uuuu egid=EXIM_GID

 SYSLOG: '2017-07-30 18:51:05 Start queue run: pid=pppp'
 SYSLOG: '2017-07-30 18:51:05 Start queue run: pid=pppp'

-SYSLOG: '2017-07-30 18:51:05 Cannot open main log file "/non/existent/path/to/force/failure/main": No such file or directory: euid=uuuu egid=EXIM_GID'
+SYSLOG: '2017-07-30 18:51:05 Cannot open main log file "/non/existent/path/to/force/failure/main": Permission denied: euid=uuuu egid=EXIM_GID'

 SYSLOG: 'exim: could not open panic log - aborting: see message(s) above'
 exim: could not open panic log - aborting: see message(s) above
 SYSLOG: 'exim: could not open panic log - aborting: see message(s) above'
 exim: could not open panic log - aborting: see message(s) above