From: Phil Pennock <pdp@exim.org>
Date: Fri, 21 Jan 2011 08:56:02 +0000 (-0500)
Subject: Check return values of setgid/setuid.
X-Git-Tag: exim-4_74_RC1~5
X-Git-Url: https://git.exim.org/exim.git/commitdiff_plain/1670ef10063d7708eb736a482d1ad25b9c59521d

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 ff375d398..a1bd4e7fc 100644
--- 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 8c8aeaa50..3a3ad5de5 100644
--- 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 dd3b5f9e7..67fbc5cf7 100644
--- 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 1d2c8f5e0..67a3d8543 100644
--- 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);
     }