On Mon, May 25, 2020 at 04:15:24PM -0600, Todd C. Miller wrote:
> On Mon, 25 May 2020 16:04:25 -0600, Bob Beck wrote:
>
> > getlock()'s behaviour changes in the case of a writeable mail spool. if we
> > want to keep supporting this, I we can modify the pledge as follows:
>
> I thought we decided not to adjust the pledge when I brought it up
> last time. Here's the diff I had in my tree to remove support for
> world-writable spool dirs.
Indeed, not supporting a world writable spool is the other option ;)
>
> - todd
>
> Index: libexec/mail.local/locking.c
> ===================================================================
> RCS file: /cvs/src/libexec/mail.local/locking.c,v
> retrieving revision 1.14
> diff -u -p -u -r1.14 locking.c
> --- libexec/mail.local/locking.c 9 Feb 2020 14:59:20 -0000 1.14
> +++ libexec/mail.local/locking.c 9 Feb 2020 15:07:22 -0000
> @@ -33,7 +33,6 @@
> #include <fcntl.h>
> #include <pwd.h>
> #include <syslog.h>
> -#include <time.h>
> #include <unistd.h>
> #include <limits.h>
> #include <errno.h>
> @@ -57,9 +56,8 @@ rellock(void)
> int
> getlock(const char *name, struct passwd *pw)
> {
> - struct stat sb, fsb;
> + struct stat sb;
> int lfd=-1;
> - char buf[8*1024];
> int tries = 0;
>
> (void)snprintf(lpath, sizeof lpath, "%s/%s.lock",
> @@ -67,58 +65,8 @@ getlock(const char *name, struct passwd
>
> if (stat(_PATH_MAILDIR, &sb) != -1 &&
> (sb.st_mode & S_IWOTH) == S_IWOTH) {
> - /*
> - * We have a writeable spool, deal with it as
> - * securely as possible.
> - */
> - time_t ctim = -1;
> -
> - seteuid(pw->pw_uid);
> - if (lstat(lpath, &sb) != -1)
> - ctim = sb.st_ctime;
> - while (1) {
> - /*
> - * Deal with existing user.lock files
> - * or directories or symbolic links that
> - * should not be here.
> - */
> - if (readlink(lpath, buf, sizeof buf-1) != -1) {
> - if (lstat(lpath, &sb) != -1 &&
> - S_ISLNK(sb.st_mode)) {
> - seteuid(sb.st_uid);
> - unlink(lpath);
> - seteuid(pw->pw_uid);
> - }
> - goto again;
> - }
> - if ((lfd = open(lpath, O_CREAT|O_WRONLY|O_EXCL|O_EXLOCK,
> - S_IRUSR|S_IWUSR)) != -1)
> - break;
> -again:
> - if (tries > 10) {
> - mwarn("%s: %s", lpath, strerror(errno));
> - seteuid(0);
> - return(-1);
> - }
> - if (tries > 9 &&
> - (lfd = open(lpath, O_WRONLY|O_EXLOCK, 0)) != -1) {
> - if (fstat(lfd, &fsb) != -1 &&
> - lstat(lpath, &sb) != -1) {
> - if (fsb.st_dev == sb.st_dev &&
> - fsb.st_ino == sb.st_ino &&
> - ctim == fsb.st_ctime ) {
> - seteuid(fsb.st_uid);
> - baditem(lpath);
> - seteuid(pw->pw_uid);
> - }
> - }
> - close(lfd);
> - }
> - sleep(1U << tries);
> - tries++;
> - continue;
> - }
> - seteuid(0);
> + mwarn("%s: will not deliver to world-writable spool",
> + _PATH_MAILDIR);
> } else {
> /*
> * Only root can write the spool directory.
> @@ -136,25 +84,6 @@ again:
> }
> }
> return(lfd);
> -}
> -
> -void
> -baditem(char *path)
> -{
> - char npath[PATH_MAX];
> - int fd;
> -
> - if (unlink(path) == 0)
> - return;
> - snprintf(npath, sizeof npath, "%s/mailXXXXXXXXXX", _PATH_MAILDIR);
> - if ((fd = mkstemp(npath)) == -1)
> - return;
> - close(fd);
> - if (rename(path, npath) == -1)
> - unlink(npath);
> - else
> - mwarn("nasty spool item %s renamed to %s", path, npath);
> - /* XXX if we fail to rename, another attempt will happen later */
> }
>
> void
> Index: libexec/mail.local/mail.local.8
> ===================================================================
> RCS file: /cvs/src/libexec/mail.local/mail.local.8,v
> retrieving revision 1.31
> diff -u -p -u -r1.31 mail.local.8
> --- libexec/mail.local/mail.local.8 16 Sep 2014 21:28:51 -0000 1.31
> +++ libexec/mail.local/mail.local.8 9 Feb 2020 15:18:48 -0000
> @@ -77,19 +77,18 @@ is prepended to any line in the message
> .Dq "From\&\ "
> delimiter line.
> .Pp
> -Significant efforts have been made to ensure that
> +Significant effort has been made to ensure that
> .Nm
> -acts as securely as possible if the spool directory is mode 1777 or 755.
> -The default of mode 755 is more secure, but it prevents mail clients from
> using
> -.Pa username.lock
> -style locking.
> -The use of 1777 is more flexible in an NFS shared-spool environment,
> -so many sites use it.
> -However, it does carry some risks, such as attackers filling the spool disk.
> -Some of these problems may be alleviated
> -by making the spool a separate filesystem, and placing quotas on it.
> -The use of any mode other than 1777 and 755 for the spool directory is
> -recommended against but may work properly.
> +acts as securely as possible.
> +It will only deliver to a mail spool directory that is not world-writable.
> +The default mode of
> +.Pa /var/mail
> +on
> +.Ox
> +is 755, which prevents non-root processes from creating mail spool files.
> +The MTA is expected to either create the mail spool file itself, or call
> +.Nm
> +as root.
> .Pp
> The mailbox is always locked using
> .Xr flock 2
