Protect against symlink attacks on MBX lockfile in /tmp as best we can:

[exim.git] / src / src / transports / appendfile.c

diff --git a/src/src/transports/appendfile.c b/src/src/transports/appendfile.c
index d7f2705448b17501046bbd45f135c5b12e5e4082..780e4b245ff3e4c97ef4bc4689f3785a8116d26f 100644 (file)
--- a/src/src/transports/appendfile.c
+++ b/src/src/transports/appendfile.c
@@ -1,10 +1,10 @@
-/* $Cambridge: exim/src/src/transports/appendfile.c,v 1.17 2006/04/25 14:02:30 ph10 Exp $ */
+/* $Cambridge: exim/src/src/transports/appendfile.c,v 1.26 2010/05/29 12:11:48 pdp Exp $ */

 
 /*************************************************
 *     Exim - an Internet mail transport agent    *
 *************************************************/
 
 
 /*************************************************
 *     Exim - an Internet mail transport agent    *
 *************************************************/
 

-/* Copyright (c) University of Cambridge 1995 - 2006 */
+/* Copyright (c) University of Cambridge 1995 - 2009 */

 /* See the file NOTICE for conditions of use and distribution. */
 
 
 /* See the file NOTICE for conditions of use and distribution. */
 
 


@@ -107,6 +107,8 @@ optionlist appendfile_transport_options[] = {
       (void *)offsetof(appendfile_transport_options_block, maildir_tag) },
   { "maildir_use_size_file", opt_bool,
       (void *)offsetof(appendfile_transport_options_block, maildir_use_size_file ) } ,
       (void *)offsetof(appendfile_transport_options_block, maildir_tag) },
   { "maildir_use_size_file", opt_bool,
       (void *)offsetof(appendfile_transport_options_block, maildir_use_size_file ) } ,

+  { "maildirfolder_create_regex", opt_stringptr,
+      (void *)offsetof(appendfile_transport_options_block, maildirfolder_create_regex ) },

 #endif  /* SUPPORT_MAILDIR */
 #ifdef SUPPORT_MAILSTORE
   { "mailstore_format",  opt_bool,
 #endif  /* SUPPORT_MAILDIR */
 #ifdef SUPPORT_MAILSTORE
   { "mailstore_format",  opt_bool,


@@ -184,6 +186,7 @@ appendfile_transport_options_block appendfile_transport_option_defaults = {
   NULL,           /* mailbox_filecount_string */
   US"^(?:cur|new|\\..*)$",  /* maildir_dir_regex */
   NULL,           /* maildir_tag */
   NULL,           /* mailbox_filecount_string */
   US"^(?:cur|new|\\..*)$",  /* maildir_dir_regex */
   NULL,           /* maildir_tag */

+  NULL,           /* maildirfolder_create_regex */

   NULL,           /* mailstore_prefix */
   NULL,           /* mailstore_suffix */
   NULL,           /* check_string (default changed for non-bsmtp file)*/
   NULL,           /* mailstore_prefix */
   NULL,           /* mailstore_suffix */
   NULL,           /* check_string (default changed for non-bsmtp file)*/


@@ -596,10 +599,10 @@ host.next = NULL;
 until one succeeds. However, it appears that at least on some systems, comsat
 doesn't listen on the ::1 address. So for the moment, just force the address to
 be 127.0.0.1. At some future stage, when IPv6 really is superseding IPv4, this
 until one succeeds. However, it appears that at least on some systems, comsat
 doesn't listen on the ::1 address. So for the moment, just force the address to
 be 127.0.0.1. At some future stage, when IPv6 really is superseding IPv4, this

-can be changed. */
+can be changed. (But actually, comsat is probably dying out anyway.) */

 
 /******
 
 /******

-if (host_find_byname(&host, NULL, NULL, FALSE) == HOST_FIND_FAILED)
+if (host_find_byname(&host, NULL, 0, NULL, FALSE) == HOST_FIND_FAILED)

   {
   DEBUG(D_transport) debug_printf("\"localhost\" unknown\n");
   return;
   {
   DEBUG(D_transport) debug_printf("\"localhost\" unknown\n");
   return;


@@ -1803,6 +1806,18 @@ if (!isdirectory)
         goto RETURN;
         }
 
         goto RETURN;
         }
 

+      /* Just in case this is a sticky-bit mail directory, we don't want
+      users to be able to create hard links to other users' files. */
+
+      if (statbuf.st_nlink != 1)
+        {
+        addr->basic_errno = ERRNO_NOTREGULAR;
+        addr->message = string_sprintf("mailbox %s%s has too many links (%d)",
+          filename, islink? " (symlink)" : "", statbuf.st_nlink);
+        goto RETURN;
+
+        }
+

       /* If symlinks are permitted (not recommended), the lstat() above will
       have found the symlink. Its ownership has just been checked; go round
       the loop again, using stat() instead of lstat(). That will never yield a
       /* If symlinks are permitted (not recommended), the lstat() above will
       have found the symlink. Its ownership has just been checked; go round
       the loop again, using stat() instead of lstat(). That will never yield a


@@ -1995,6 +2010,8 @@ if (!isdirectory)
     #ifdef SUPPORT_MBX
     else if (ob->use_mbx_lock)
       {
     #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)
       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)


@@ -2002,6 +2019,21 @@ if (!isdirectory)
         sprintf(CS mbx_lockname, "/tmp/.%lx.%lx", (long)statbuf.st_dev,
           (long)statbuf.st_ino);
 
         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)
         if (Ulstat(mbx_lockname, &statbuf) >= 0)
           {
           if ((statbuf.st_mode & S_IFMT) == S_IFLNK)


@@ -2020,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;
         if (mbx_lockfd < 0)
           {
           addr->basic_errno = ERRNO_LOCKFAILED;


@@ -2029,6 +2073,60 @@ if (!isdirectory)
           goto RETURN;
           }
 
           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,
         (void)Uchmod(mbx_lockname, ob->lockfile_mode);
 
         if (apply_lock(mbx_lockfd, F_WRLCK, ob->use_fcntl,


@@ -2152,10 +2250,11 @@ else
     }
 
   #ifdef SUPPORT_MAILDIR
     }
 
   #ifdef SUPPORT_MAILDIR

-  /* For a maildir delivery, ensure that all the relevant directories exist */
+  /* For a maildir delivery, ensure that all the relevant directories exist,
+  and a maildirfolder file if necessary. */

 
   if (mbformat == mbf_maildir && !maildir_ensure_directories(path, addr,
 
   if (mbformat == mbf_maildir && !maildir_ensure_directories(path, addr,

-    ob->create_directory, ob->dirmode))
+    ob->create_directory, ob->dirmode, ob->maildirfolder_create_regex))

       return FALSE;
   #endif  /* SUPPORT_MAILDIR */
 
       return FALSE;
   #endif  /* SUPPORT_MAILDIR */
 


@@ -2236,6 +2335,8 @@ else
             {
             *slash = 0;
             check_path = new_check_path;
             {
             *slash = 0;
             check_path = new_check_path;

+            DEBUG(D_transport) debug_printf("maildirfolder file exists: "
+              "quota check directory changed to %s\n", check_path);

             }
           }
         }
             }
           }
         }


@@ -2445,6 +2546,8 @@ else
         addr->message = string_sprintf ("failed to open %s (%d tr%s)",
           filename, i, (i == 1)? "y" : "ies");
         addr->basic_errno = errno;
         addr->message = string_sprintf ("failed to open %s (%d tr%s)",
           filename, i, (i == 1)? "y" : "ies");
         addr->basic_errno = errno;

+        if (errno == errno_quota || errno == ENOSPC)
+          addr->user_message = US"mailbox is full";

         return FALSE;
         }
 
         return FALSE;
         }
 


@@ -2785,7 +2888,7 @@ if (temp_file != NULL && ob->mbx_format)
 /* Force out the remaining data to check for any errors; some OS don't allow
 fsync() to be called for a FIFO. */
 
 /* Force out the remaining data to check for any errors; some OS don't allow
 fsync() to be called for a FIFO. */
 

-if (yield == OK && !isfifo && fsync(fd) < 0) yield = DEFER;
+if (yield == OK && !isfifo && EXIMfsync(fd) < 0) yield = DEFER;

 
 /* Update message_size to the accurate count of bytes written, including
 added headers. */
 
 /* Update message_size to the accurate count of bytes written, including
 added headers. */


@@ -2897,6 +3000,7 @@ if (yield != OK)
     #else
     addr->message = string_sprintf("mailbox is full");
     #endif  /* EDQUOT */
     #else
     addr->message = string_sprintf("mailbox is full");
     #endif  /* EDQUOT */

+    addr->user_message = US"mailbox is full";

     DEBUG(D_transport) debug_printf("System quota exceeded for %s%s%s\n",
       dataname,
       isdirectory? US"" : US": time since file read = ",
     DEBUG(D_transport) debug_printf("System quota exceeded for %s%s%s\n",
       dataname,
       isdirectory? US"" : US": time since file read = ",