lockspool getting killed by pledge on OpenBSD 6.7

classic Classic list List threaded Threaded
9 messages Options
Reply | Threaded
Open this post in threaded view
|

lockspool getting killed by pledge on OpenBSD 6.7

Dawid Czeluśniak
Hi,

I noticed that when I try to run /usr/libexec/lockspool directly as root
I'm getting Abort trap on my machine:

$ /usr/libexec/lockspool
Abort trap
$ echo $?
134

And in dmesg I can see plenty of pledge logs:
lockspool[73511]: pledge "id", syscall 183
lockspool[94755]: pledge "id", syscall 183
lockspool[38910]: pledge "id", syscall 183

1. Is this reproducible on your end?
2. Is "id" pledge request missing here?
https://cvsweb.openbsd.org/cgi-bin/cvsweb/src/libexec/lockspool/lockspool.c?rev=1.21

Latest kdump:
   919 lockspool RET   stat 0
   919 lockspool CALL  kbind(0x7f7ffffcb278,24,0xc70598ce16a08728)
   919 lockspool RET   kbind 0
   919 lockspool CALL  seteuid(0<"root">)
   919 lockspool PLDG  seteuid, "id", errno 1 Operation not permitted
   919 lockspool PSIG  SIGABRT SIG_DFL code <74513776>

Seems like seteuid(2) is called...

Dawid

Reply | Threaded
Open this post in threaded view
|

Re: lockspool getting killed by pledge on OpenBSD 6.7

Dawid Czeluśniak
After changing permissions of /var/mail directory to 755:

$ chmod 755 /var/mail

everything is fine and seteuid(2) is not called:
 92121 lockspool NAMI  "/var/mail/root.lock"
 92121 lockspool RET   unlink 0
 92121 lockspool CALL  kbind(0x7f7ffffc7f58,24,0xefbb72852ff02523)
 92121 lockspool RET   kbind 0
 92121 lockspool CALL  exit(0)

Killing lockspool(1) by pledge(2) happens when permissions of /var/mail
are greater than 755. Maybe it would be useful to give user an indication
that it is the permission issue instead of killing the process by pledge?

What do you think?

Reply | Threaded
Open this post in threaded view
|

Re: lockspool getting killed by pledge on OpenBSD 6.7

Bob Beck-2
On Mon, May 25, 2020 at 11:07:12PM +0200, Dawid Czelu??niak wrote:

> After changing permissions of /var/mail directory to 755:
>
> $ chmod 755 /var/mail
>
> everything is fine and seteuid(2) is not called:
>  92121 lockspool NAMI  "/var/mail/root.lock"
>  92121 lockspool RET   unlink 0
>  92121 lockspool CALL  kbind(0x7f7ffffc7f58,24,0xefbb72852ff02523)
>  92121 lockspool RET   kbind 0
>  92121 lockspool CALL  exit(0)
>
> Killing lockspool(1) by pledge(2) happens when permissions of /var/mail
> are greater than 755. Maybe it would be useful to give user an indication
> that it is the permission issue instead of killing the process by pledge?
>
> What do you think?

You obviously have a non-default config for this, nevertheless, this is supposedly
a supported config according to getlock() which is actually living in mail.local's
sources.

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:

ok?

Index: lockspool.c
===================================================================
RCS file: /cvs/src/libexec/lockspool/lockspool.c,v
retrieving revision 1.21
diff -u -p -u -p -r1.21 lockspool.c
--- lockspool.c 9 Feb 2020 14:59:20 -0000 1.21
+++ lockspool.c 25 May 2020 22:01:13 -0000
@@ -55,7 +55,7 @@ main(int argc, char *argv[])
 
  if (unveil(_PATH_MAILDIR, "rwc") == -1)
  err(1, "unveil");
- if (pledge("stdio rpath wpath getpw cpath fattr", NULL) == -1)
+ if (pledge("id flock stdio rpath wpath getpw cpath fattr", NULL) == -1)
  err(1, "pledge");
 
  openlog(__progname, LOG_PERROR, LOG_MAIL);

Reply | Threaded
Open this post in threaded view
|

Re: lockspool getting killed by pledge on OpenBSD 6.7

Theo de Raadt-2
> +       if (pledge("id flock stdio rpath wpath getpw cpath fattr", NULL) == -1)

Please wait for other pledge people to respond.

But re-order.  Add the new things to the end.

Reply | Threaded
Open this post in threaded view
|

Re: lockspool getting killed by pledge on OpenBSD 6.7

Bob Beck-2
On Mon, May 25, 2020 at 04:05:47PM -0600, Theo de Raadt wrote:
> > +       if (pledge("id flock stdio rpath wpath getpw cpath fattr", NULL) == -1)
>
> Please wait for other pledge people to respond.
>
> But re-order.  Add the new things to the end.
>
Ack

Reply | Threaded
Open this post in threaded view
|

Re: lockspool getting killed by pledge on OpenBSD 6.7

Todd C. Miller-3
In reply to this post by Bob Beck-2
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.

 - 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

Reply | Threaded
Open this post in threaded view
|

Re: lockspool getting killed by pledge on OpenBSD 6.7

Bob Beck-2
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

Reply | Threaded
Open this post in threaded view
|

Re: lockspool getting killed by pledge on OpenBSD 6.7

Dawid Czeluśniak
On Mon, May 25, 2020 at 04:15:24PM -0600, Todd C. Miller wrote:
> 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.

It's definitely a good option and would simplify
/cvs/src/libexec/mail.local/locking.c
file significantly as well as gives a clear warning to the user.

Reply | Threaded
Open this post in threaded view
|

Re: lockspool getting killed by pledge on OpenBSD 6.7

Bob Beck-2
ok beck@ for todd’s diff to remove support

> On May 26, 2020, at 03:20, Dawid Czeluśniak <[hidden email]> wrote:
>
> On Mon, May 25, 2020 at 04:15:24PM -0600, Todd C. Miller wrote:
>> 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.
>
> It's definitely a good option and would simplify
> /cvs/src/libexec/mail.local/locking.c
> file significantly as well as gives a clear warning to the user.