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.

2 files changed, 51 insertions, 9 deletions

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);
     }