mail.local.c patch

[J.S.Peatfield () amtp cam ac uk (Jon Peatfield)]

Well for the past few days (weeks) I've been reading the mail.local.c code 
which CERT recommended for use on BSD derived systems with mode 1777 mail 
spools.  I think I've found a small problem with it which allows a race which 
can cause problems.

However by adding an extra lstat() after we open the file I *think* that the 
problem can be detected.

Here is my first attempt at a patch for this, note that I've not actually 
tried running this code since I'm not confident enough that I've not made it 
worse.  Some of you people can probably see any hole better than I can.

The version of mail.local.c which I got had had all the indentation removed so 
I re-indented it before patching it.  Therefore it may not apply cleanly.

--cut-here--mail.local.c.diff--
*** mail.local.c.orig   Sat Feb 18 13:53:12 1995
--- mail.local.c        Sat Feb 18 13:56:36 1995
***************
*** 312,317 ****
--- 312,347 ----
     * XXX
     * open(2) should support flock'ing the file.
     */
+   
+   /* 
+      The original version of this has two possible races which can cause
+      problems.
+      
+      The first is when the file we are about to open doesn't exist, in which
+      case there is a race between the lstat() and the open() in which someone
+      can create a symlink to anywhere.  The file pointed at must not exist
+      but this still allows the creation of .rhosts etc.
+      
+      The second is harder as it must fool the check on the inode number etc.
+      I'm not sure if there really is a race here but I get the feeling that
+      someone *might* be able to almost fill a file-system, such that they get
+      the same inode number after the race.
+      
+      The proposed fix it to perform a (possibly second) lstat() *after*
+      opening the file.  We then check that the inode, dev etc are the same.
+      Since we hold the file open at this point we know that noone can have a
+      file of the same dev,inode existing at the same time, so if they match
+      then we are ok.
+ 
+      The only thing I can see left is that you can create zero length files
+      owned by the recipient using the first race as we still create the file
+      before the extra check.  Since these do get logged at least the sysadmin
+      can try to fix the mess.
+      
+      J.S.Peatfield () damtp cam ac uk 1995-02-18
+ 
+    */
+   
   tryagain:
    lockmbox(path);
    if (lstat(path, &sb)) {
***************
*** 320,330 ****
      if (mbfd == -1) {
        if (errno == EEXIST)
        goto tryagain;
!     } else if (fchown(mbfd, pw->pw_uid, pw->pw_gid)) {
!       e_to_sys(errno);
!       warn("chown %u.%u: %s", pw->pw_uid, pw->pw_gid, name);
!       unlockmbox();
!       return;
      }
    } else if (sb.st_nlink != 1 || !S_ISREG(sb.st_mode)) {
      e_to_sys(errno);
--- 350,373 ----
      if (mbfd == -1) {
        if (errno == EEXIST)
        goto tryagain;
!     } else {
!       if (fchown(mbfd, pw->pw_uid, pw->pw_gid)) {
!       e_to_sys(errno);
!       warn("chown %u.%u: %s", pw->pw_uid, pw->pw_gid, name);
!       unlockmbox();
!       return;
!       }
! 
!       /* Perform an fstat() on the file we opened for later use.  Might as
!        well do a trivial check on it now.  JSP */
!       if (fstat(mbfd, &fsb) ||
!         fsb.st_nlink != 1 ||
!         !S_ISREG(fsb.st_mode)) {
!       warn("%s: irregular file", path);
!       (void)close(mbfd);
!       unlockmbox();
!       return;
!       }
      }
    } else if (sb.st_nlink != 1 || !S_ISREG(sb.st_mode)) {
      e_to_sys(errno);
***************
*** 351,356 ****
--- 394,424 ----
    if (mbfd == -1) {
      e_to_sys(errno);
      warn("%s: %s", path, strerror(errno));
+     unlockmbox();
+     return;
+   }
+   
+   /* Now we have the file open, so lstat() it again and check the info
+      against what the fstat() above told us.  JSP */
+   if ((lstat(path, &sb) || sb.st_nlink != 1 ||
+        !S_ISREG(sb.st_mode) || sb.st_dev != fsb.st_dev ||
+        sb.st_ino != fsb.st_ino || sb.st_uid != fsb.st_uid)) {
+     warn("%s: file changed after open (lstat check)", path);
+ 
+     /* Check if lstat() showed a symlink */
+     if (S_ISLNK(sb.st_mode)) {  /* Caught them at it ! */
+       char slink[BUFSIZ] ;
+       int slen ;
+ 
+       if ((slen = readlink(path, slink, BUFSIZ)) != -1) { /* Read it */
+       slink[slen] = 0 ;
+       warn("%s: is symlink apparently pointing to %s", path, slink) ;
+       }
+     } else if (sb.st_nlink != 1) { /* Multiple links! */
+       warn("%s: has %d links", path, sb.st_nlink) ;
+     }
+ 
+     (void)close(mbfd);
      unlockmbox();
      return;
    }
--cut-here--mail.local.c.diff--

Does anyone have any comments?  Was this all discussed when CERT released 
this?  If so I missed it all.

  -- Jon Peatfield

Jon Peatfield, Computer Officer, the DAMTP, University of Cambridge
Telephone: +44 1223  337852     Mail: J.S.Peatfield () damtp cam ac uk