Check return values of setgid/setuid.

CVE-2011-0017

One assertion of the unimportance of checking the return value was wrong,
in the event of a compromised exim run-time user.

diff --git a/doc/doc-txt/ChangeLog b/doc/doc-txt/ChangeLog
index ff375d39898aeaa36c454b9ca0010fe0d7e41898..a1bd4e7fca8f6057257240b217eb7a15f8051de9 100644 (file)
--- a/doc/doc-txt/ChangeLog
+++ b/doc/doc-txt/ChangeLog
@@ -32,6 +32,11 @@ PP/03 Report version information for many libraries, including
       version.h, now support a version extension string for distributors
       who patch heavily. Dynamic module ABI change.
 
+PP/04 CVE-2011-0017 - check return value of setuid/setgid. This is a
+      privilege escalation vulnerability whereby the Exim run-time user
+      can cause root to append content of the attacker's choosing to
+      arbitrary files.
+
 
 Exim version 4.73
 -----------------

diff --git a/doc/doc-txt/NewStuff b/doc/doc-txt/NewStuff
index 8c8aeaa506185a80697816caed033ab226231a43..3a3ad5de5fc326fb3464aed332c570007b9539e2 100644 (file)
--- a/doc/doc-txt/NewStuff
+++ b/doc/doc-txt/NewStuff
@@ -12,7 +12,12 @@ the documentation is updated, this file is reduced to a short list.
 Version 4.74
 ------------
 
- 1. Exim now supports loading some lookup types at run-time, using your
+ 1. SECURITY FIX: privilege escalation flaw fixed. On Linux (and only Linux)
+    the flaw permitted the Exim run-time user to cause root to append to
+    arbitrary files of the attacker's choosing, with the content based
+    on content supplied by the attacker.
+
+ 2. Exim now supports loading some lookup types at run-time, using your
     platform's dlopen() functionality.  This has limited platform support
     and the intention is not to support every variant, it's limited to
     dlopen().  This permits the main Exim binary to not be linked against

diff --git a/src/src/exim.c b/src/src/exim.c
index dd3b5f9e7ecc38ed1932ab81804d5e29886cf295..67fbc5cf7a9a558ae3b9d422eb71fdf2f9d05f69 100644 (file)
--- a/src/src/exim.c
+++ b/src/src/exim.c
@@ -1300,7 +1300,7 @@ int  arg_error_handling = error_handling;
 int  filter_sfd = -1;
 int  filter_ufd = -1;
 int  group_count;
-int  i;
+int  i, rv;
 int  list_queue_option = 0;
 int  msg_action = 0;
 int  msg_action_arg = -1;
@@ -1639,8 +1639,20 @@ real_gid = getgid();
 
 if (real_uid == root_uid)
   {
-  setgid(real_gid);
-  setuid(real_uid);
+  rv = setgid(real_gid);
+  if (rv)
+    {
+    fprintf(stderr, "exim: setgid(%ld) failed: %s\n",
+        (long int)real_gid, strerror(errno));
+    exit(EXIT_FAILURE);
+    }
+  rv = setuid(real_uid);
+  if (rv)
+    {
+    fprintf(stderr, "exim: setuid(%ld) failed: %s\n",
+        (long int)real_uid, strerror(errno));
+    exit(EXIT_FAILURE);
+    }
   }
 
 /* If neither the original real uid nor the original euid was root, Exim is
@@ -3862,7 +3874,28 @@ if (!unprivileged &&                      /* originally had root AND */
 
 /* When we are retaining a privileged uid, we still change to the exim gid. */
 
-else setgid(exim_gid);
+else
+  {
+  int rv;
+  rv = setgid(exim_gid);
+  /* Impact of failure is that some stuff might end up with an incorrect group.
+  We track this for failures from root, since any attempt to change privilege
+  by root should succeed and failures should be examined.  For non-root,
+  there's no security risk.  For me, it's { exim -bV } on a just-built binary,
+  no need to complain then. */
+  if (rv == -1)
+    {
+    if (!unprivileged)
+      {
+      fprintf(stderr,
+          "exim: changing group failed: %s\n", strerror(errno));
+      exit(EXIT_FAILURE);
+      }
+    else
+      debug_printf("changing group to %ld failed: %s\n",
+          (long int)exim_gid, strerror(errno));
+    }
+  }
 
 /* Handle a request to scan a file for malware */
 if (malware_test_file)

diff --git a/src/src/log.c b/src/src/log.c
index 1d2c8f5e0a7a16833d21b46fbceb015c94a9d3cf..67a3d8543335598b675024742a6bc8844b57794c 100644 (file)
--- a/src/src/log.c
+++ b/src/src/log.c
@@ -361,17 +361,26 @@ are neither exim nor root, creation is not attempted. */
 
 else if (euid == root_uid)
   {
-  int status;
+  int status, rv;
   pid_t pid = fork();
 
   /* In the subprocess, change uid/gid and do the creation. Return 0 from the
-  subprocess on success. There doesn't seem much point in testing for setgid
-  and setuid errors. */
+  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)
     {
-    (void)setgid(exim_gid);
-    (void)setuid(exim_uid);
+    rv = setgid(exim_gid);
+    if (rv)
+      die(US"exim: setgid for log-file creation failed, aborting",
+         US"Unexpected log failure, please try later");
+    rv = setuid(exim_uid);
+    if (rv)
+      die(US"exim: setuid for log-file creation failed, aborting",
+         US"Unexpected log failure, please try later");
     _exit((create_log(buffer) < 0)? 1 : 0);
     }