From travis@borneo.evtech.com Tue Aug 27 02:27:07 1996 Received: from midway.evtech.com (midway.evtech.com [204.96.163.2]) by suburbia.net (8.7.4/Proff-950810) with ESMTP id CAA04177 for ; Tue, 27 Aug 1996 02:26:12 +1000 Received: from tahiti.evtech.com (tahiti.evtech.com [192.35.179.19]) by midway.evtech.com (8.7.3/8.6.9) with ESMTP id LAA08224; Mon, 26 Aug 1996 11:25:28 -0500 (CDT) Received: from borneo.evtech.com (borneo.evtech.com [192.35.179.29]) by tahiti.evtech.com (8.6.12/8.6.12) with ESMTP id LAA08577; Mon, 26 Aug 1996 11:25:22 -0500 Message-Id: <199608261625.LAA08577@tahiti.evtech.com> To: Piete Brooks cc: Daniel Bromberg , leitner@math.fu-berlin.de (Felix von Leitner), best-of-security@suburbia.net, travis@EvTech.com Subject: BSD mail.local & Re: BoS: cfingerd possible security hole In-reply-to: Your message of "Mon, 26 Aug 1996 09:07:17 BST." Date: Mon, 26 Aug 1996 11:25:20 -0500 From: Travis Hassloch x231 In message you write: > Not that I know of, but (I think) you can reduce the general problem to just > allowing someone to change the last modified time on a file interesting, hadn't thought of the mod times > (5f) Stat the file. lstat, save stat block structure for later > (6f) If the file already exists: > If it's not a regular file, complain to local administrator and defer > delivery with a local error code that causes subsequent delivery to > be prevented until manually enabled. interesting Also check that it's not linked: if (sb.st_nlink != 1 || S_ISLNK(sb.st_mode)) { > Check owner, group and permissions. If not, complain and defer. Some people like to allow some strange things here. Keith Bostic, of the BSD group, mentioned some sites having mail spool files for non-users. I think this is strange, and agree with checking ownership and permissions. > Open with O_WRONLY + O_APPEND, thus failing if the file has vanished. > If open fails because the file does not exist, go to (7); on any other > failure except EWOULDBLOCK, complain & defer. For EWOULDBLOCK (NFS > failure), just defer. I just give up if the file has vanished here. Users shouldn't be playing peekaboo with their mail spool files. > Check the inode number hasn't changed - I realize this isn't perfect (an > inode can be reused) but it's cheap and will catch some of the races. Make sure to use fstat with this; you don't want another namei traversal. > Check it's still a regular file. > Check that the owner and permissions haven't changed. Depending on your level of paranoia, you can re-check a number of things; the above, plus linkhood status, or nothing. Any discussion should probably be continued on bugtraq. Here's a patch I made for the BSD mail.local.c used in the various free *BSDs, which fixes the problem with world-writeable mail spools (which, some assert, are a Bad Thing anyway). I'm writing a little doc on what kinds of restrictions a local delivery agent should make. --- mail.local.c~ Tue Jul 16 19:00:07 1996 +++ mail.local.c Tue Jul 16 21:02:15 1996 @@ -168,9 +168,9 @@ char *name; int lockfile; { - struct stat sb; + struct stat sb, fsb; struct passwd *pw; - int created, mbfd, nr, nw, off, rval=0, lfd=-1; + int mbfd=-1, nr, nw, off, rval=1, lfd=-1; char biffmsg[100], buf[8*1024], path[MAXPATHLEN], lpath[MAXPATHLEN]; off_t curoff; @@ -194,59 +194,94 @@ return(1); } } - - if (!(created = lstat(path, &sb)) && - (sb.st_nlink != 1 || S_ISLNK(sb.st_mode))) { - err(NOTFATAL, "%s: linked file", path); - return(1); - } - if((mbfd = open(path, O_APPEND|O_WRONLY|O_EXLOCK, - S_IRUSR|S_IWUSR)) < 0) { + /* after this point, always exit via bad to remove lockfile */ +retry: + if (lstat(path, &sb)) { + if (errno != ENOENT) { + err(NOTFATAL, "%s: %s", path, strerror(errno)); + goto bad; + } if ((mbfd = open(path, O_APPEND|O_CREAT|O_WRONLY|O_EXLOCK, - S_IRUSR|S_IWUSR)) < 0) { - err(NOTFATAL, "%s: %s", path, strerror(errno)); - return(1); + S_IRUSR|S_IWUSR)) < 0) { + if (errno == EEXIST) { + /* file appeared since lstat */ + goto retry; + } + else { + err(NOTFATAL, "%s: %s", path, strerror(errno)); + goto bad; + } + } + /* + * Set the owner and group. Historically, binmail repeated this at + * each mail delivery. We no longer do this, assuming that if the + * ownership or permissions were changed there was a reason for doing + * so. + */ + if (fchown(mbfd, pw->pw_uid, pw->pw_gid) < 0) { + err(NOTFATAL, "chown %u:%u: %s", + pw->pw_uid, pw->pw_gid, name); + goto bad; + } } + else { + if (sb.st_nlink != 1 || S_ISLNK(sb.st_mode)) { + err(NOTFATAL, "%s: linked file", path); + goto bad; + } + if ((mbfd = open(path, O_APPEND|O_WRONLY|O_EXLOCK, + S_IRUSR|S_IWUSR)) < 0) { + err(NOTFATAL, "%s: %s", path, strerror(errno)); + goto bad; + } + if (fstat(mbfd, &fsb)) { + /* relating error to path may be bad style */ + err(NOTFATAL, "%s: %s", path, strerror(errno)); + goto bad; + } + if (sb.st_dev != fsb.st_dev || sb.st_ino != fsb.st_ino) { + err(NOTFATAL, "%s: changed after open", path); + goto bad; + } + /* paranoia? */ + if (fsb.st_nlink != 1 || S_ISLNK(fsb.st_mode)) { + err(NOTFATAL, "%s: linked file", path); + goto bad; + } } curoff = lseek(mbfd, 0, SEEK_END); (void)sprintf(biffmsg, "%s@%qd\n", name, curoff); if (lseek(fd, 0, SEEK_SET) == (off_t)-1) { err(FATAL, "temporary file: %s", strerror(errno)); - rval = 1; goto bad; } + /* This section (to trunc) is ugly */ while ((nr = read(fd, buf, sizeof(buf))) > 0) for (off = 0; off < nr; off += nw) if ((nw = write(mbfd, buf + off, nr - off)) < 0) { err(NOTFATAL, "%s: %s", path, strerror(errno)); goto trunc; } - if (nr < 0) { + if (!nr) { + rval = 0; + } + else { err(FATAL, "temporary file: %s", strerror(errno)); trunc: (void)ftruncate(mbfd, curoff); - rval = 1; } - /* - * Set the owner and group. Historically, binmail repeated this at - * each mail delivery. We no longer do this, assuming that if the - * ownership or permissions were changed there was a reason for doing - * so. - */ bad: - if(lockfile) { - if(lfd >= 0) { - unlink(lpath); - close(lfd); - } + if(lfd >= 0) { + unlink(lpath); + close(lfd); } - if (created) - (void)fchown(mbfd, pw->pw_uid, pw->pw_gid); - (void)fsync(mbfd); /* Don't wait for update. */ - (void)close(mbfd); /* Implicit unlock. */ + if (mbfd >= 0) { + (void)fsync(mbfd); /* Don't wait for update. */ + (void)close(mbfd); /* Implicit unlock. */ + } if (!rval) notifybiff(biffmsg);