File CVE-2010-2024.diff of Package exim

Overview Repositories Revisions Requests Users Attributes Meta

File CVE-2010-2024.diff of Package exim

From bf83d8d38bc9a0f3b3574eb6641b619e68d7a796 Mon Sep 17 00:00:00 2001
From: Phil Pennock <pdp@exim.org>
Date: Sat, 29 May 2010 12:11:48 +0000
Subject: [PATCH] Protect against symlink attacks on MBX lockfile in /tmp as best we can:
  * if system supports O_NOFOLLOW, use it, protection complete
  * else detect the attack "too late" and abort, where at worst an empty file
    has been created as the attacked user
 Our hands are tied by not changing the locking algorithm.

fixes: bug #989
---
 src/src/exim_lock.c             |   35 ++++++++++++++-
 src/src/transports/appendfile.c |   87 ++++++++++++++++++++++++++++++++++++++-
 2 files changed, 117 insertions(+), 5 deletions(-)

--- src/exim_lock.c
+++ src/exim_lock.c
@@ -310,7 +310,8 @@ if (use_lockfile)
 for (j = 0; j < lock_retries; j++)
   {
   int sleep_before_retry = TRUE;
-  struct stat statbuf, ostatbuf;
+  struct stat statbuf, ostatbuf, lstatbuf, statbuf2;
+  int mbx_tmp_oflags;
 
   /* Try to build a lock file if so configured */
 
@@ -431,7 +432,11 @@ for (j = 0; j < lock_retries; j++)
         }
       }
 
-    md = open(tempname, O_RDWR | O_CREAT, 0600);
+    mbx_tmp_oflags = O_RDWR | O_CREAT;
+#ifdef O_NOFOLLOW
+    mbx_tmp_oflags |= O_NOFOLLOW;
+#endif
+    md = open(tempname, mbx_tmp_oflags, 0600);
     if (md < 0)
       {
       printf("exim_lock: failed to create mbx lock file %s: %s\n",
@@ -439,6 +444,30 @@ for (j = 0; j < lock_retries; j++)
       goto CLEAN_UP;
       }
 
+    /* security fixes from 2010-05 */
+    if (lstat(tempname, &lstatbuf) < 0)
+      {
+      printf("exim_lock: failed to lstat(%s) after opening it: %s\n",
+          tempname, strerror(errno));
+      goto CLEAN_UP;
+      }
+    if (fstat(md, &statbuf2) < 0)
+      {
+      printf("exim_lock: failed to fstat() open fd of \"%s\": %s\n",
+          tempname, strerror(errno));
+      goto CLEAN_UP;
+      }
+    if ((statbuf2.st_nlink > 1) ||
+        (lstatbuf.st_nlink > 1) ||
+        (!S_ISREG(lstatbuf.st_mode)) ||
+        (lstatbuf.st_dev != statbuf2.st_dev) ||
+        (lstatbuf.st_ino != statbuf2.st_ino))
+      {
+      printf("exim_lock: race condition exploited against us when "
+          "locking \"%s\"\n", tempname);
+      goto CLEAN_UP;
+      }
+
     (void)chmod(tempname, 0600);
 
     if (apply_lock(md, F_WRLCK, use_fcntl, lock_fcntl_timeout, use_flock,
--- src/transports/appendfile.c
+++ src/transports/appendfile.c
@@ -2010,6 +2010,8 @@ if (!isdirectory)
     #ifdef SUPPORT_MBX
     else if (ob->use_mbx_lock)
       {
+      int mbx_tmp_oflags;
+      struct stat lstatbuf, statbuf2;
       if (apply_lock(fd, F_RDLCK, ob->use_fcntl, ob->lock_fcntl_timeout,
            ob->use_flock, ob->lock_flock_timeout) >= 0 &&
            fstat(fd, &statbuf) >= 0)
@@ -2017,6 +2019,21 @@ if (!isdirectory)
         sprintf(CS mbx_lockname, "/tmp/.%lx.%lx", (long)statbuf.st_dev,
           (long)statbuf.st_ino);
 
+        /*
+         * 2010-05-29: SECURITY
+         * Dan Rosenberg reported the presence of a race-condition in the
+         * original code here.  Beware that many systems still allow symlinks
+         * to be followed in /tmp so an attacker can create a symlink pointing
+         * elsewhere between a stat and an open, which we should avoid
+         * following.
+         *
+         * It's unfortunate that we can't just use all the heavily debugged
+         * locking from above.
+         *
+         * Also: remember to mirror changes into exim_lock.c */
+
+        /* first leave the old pre-check in place, it provides better
+         * diagnostics for common cases */
         if (Ulstat(mbx_lockname, &statbuf) >= 0)
           {
           if ((statbuf.st_mode & S_IFMT) == S_IFLNK)
@@ -2035,7 +2052,19 @@ if (!isdirectory)
             }
           }
 
-        mbx_lockfd = Uopen(mbx_lockname, O_RDWR | O_CREAT, ob->lockfile_mode);
+        /* If we could just declare "we must be the ones who create this
+         * file" then a hitching post in a subdir would work, since a
+         * subdir directly in /tmp/ which we create wouldn't follow links
+         * but this isn't our locking logic, so we can't safely change the
+         * file existence rules. */
+
+        /* On systems which support O_NOFOLLOW, it's the easiest and most
+         * obviously correct security fix */
+        mbx_tmp_oflags = O_RDWR | O_CREAT;
+#ifdef O_NOFOLLOW
+        mbx_tmp_oflags |= O_NOFOLLOW;
+#endif
+        mbx_lockfd = Uopen(mbx_lockname, mbx_tmp_oflags, ob->lockfile_mode);
         if (mbx_lockfd < 0)
           {
           addr->basic_errno = ERRNO_LOCKFAILED;
@@ -2044,6 +2073,60 @@ if (!isdirectory)
           goto RETURN;
           }
 
+        if (lstat(mbx_lockname, &lstatbuf) < 0)
+          {
+          addr->basic_errno = ERRNO_LOCKFAILED;
+          addr->message = string_sprintf("attempting to lstat open MBX "
+             "lock file %s: %s", mbx_lockname, strerror(errno));
+          goto RETURN;
+          }
+        if (fstat(mbx_lockfd, &statbuf2) < 0)
+          {
+          addr->basic_errno = ERRNO_LOCKFAILED;
+          addr->message = string_sprintf("attempting to stat fd of open MBX "
+              "lock file %s: %s", mbx_lockname, strerror(errno));
+          goto RETURN;
+          }
+
+        /*
+         * At this point:
+         *  statbuf: if exists, is file which existed prior to opening the
+         *           lockfile, might have been replaced since then
+         *  statbuf2: result of stat'ing the open fd, is what was actually
+         *            opened
+         *  lstatbuf: result of lstat'ing the filename immediately after
+         *            the open but there's a race condition again between
+         *            those two steps: before open, symlink to foo, after
+         *            open but before lstat have one of:
+         *             * was no symlink, so is the opened file
+         *               (we created it, no messing possible after that point)
+         *             * hardlink to foo
+         *             * symlink elsewhere
+         *             * hardlink elsewhere
+         *             * new file/other
+         * Don't want to compare to device of /tmp because some modern systems
+         * have regressed to having /tmp be the safe actual filesystem as
+         * valuable data, so is mostly worthless, unless we assume that *only*
+         * Linux systems do this and that all Linux has O_NOFOLLOW.  Something
+         * for further consideration.
+         * No point in doing a readlink on the lockfile as that will always be
+         * at a different point in time from when we open it, so tells us
+         * nothing; attempts to clean up and delete after ourselves would risk
+         * deleting a *third* filename.
+         */
+        if ((statbuf2.st_nlink > 1) ||
+            (lstatbuf.st_nlink > 1) ||
+            (!S_ISREG(lstatbuf.st_mode)) ||
+            (lstatbuf.st_dev != statbuf2.st_dev) ||
+            (lstatbuf.st_ino != statbuf2.st_ino))
+          {
+          addr->basic_errno = ERRNO_LOCKFAILED;
+          addr->message = string_sprintf("RACE CONDITION detected: "
+              "mismatch post-initial-checks between \"%s\" and opened "
+              "fd lead us to abort!", mbx_lockname);
+          goto RETURN;
+          }
+
         (void)Uchmod(mbx_lockname, ob->lockfile_mode);
 
         if (apply_lock(mbx_lockfd, F_WRLCK, ob->use_fcntl,
-- 
1.6.3.2

Places

File CVE-2010-2024.diff of Package exim

Places