In cron(8), require that crontab and at files in the spool be owned

by group crontab.  The at(1) command now creates files owned by
group crontab, the crontab(1) command already does this.

Files in the crontab spool with parse errors are now ignored;
crontab(1) will not install a crontab file with parse errors.
The system crontab file (/etc/crontab) is not affected by this.

The required permissions on crontab files have been tightened.
Files in the cron spool must be mode 0600 (as created by crontab(1)).
The system crontab file may be readable/writable by the owner,
readable by group and readable by other.  The system crontab must
be readable by the owner.

diff --git a/usr.bin/at/at.c b/usr.bin/at/at.c
index 7a9c63d..16a96a7 100644 (file)
--- a/usr.bin/at/at.c
+++ b/usr.bin/at/at.c
@@ -1,4 +1,4 @@
-/*     $OpenBSD: at.c,v 1.79 2017/03/18 02:58:54 deraadt Exp $ */
+/*     $OpenBSD: at.c,v 1.80 2017/06/07 23:36:43 millert Exp $ */
 
 /*
  *  at.c : Put file into atrun queue
@@ -217,9 +217,6 @@ writefile(const char *cwd, time_t runtimer, char queue)
        if ((fd = newjob(runtimer, queue)) == -1)
                fatal("unable to create atjob file");
 
-       if (fchown(fd, -1, user_gid) != 0)
-               fatal("fchown");
-
        /*
         * We've successfully created the file; let's set the flag so it
         * gets removed in case of an interrupt or error.
@@ -256,7 +253,7 @@ writefile(const char *cwd, time_t runtimer, char queue)
        }
 
        (void)fprintf(fp, "#!/bin/sh\n# atrun uid=%lu gid=%lu\n# mail %*s %d\n",
-           (unsigned long)user_uid, (unsigned long)user_gid,
+           (unsigned long)user_uid, (unsigned long)spool_gid,
            MAX_UNAME, mailname, send_mail);
 
        /* Write out the umask at the time of invocation */

diff --git a/usr.sbin/cron/atrun.c b/usr.sbin/cron/atrun.c
index d1414d4..4f230c3 100644 (file)
--- a/usr.sbin/cron/atrun.c
+++ b/usr.sbin/cron/atrun.c
@@ -1,4 +1,4 @@
-/*     $OpenBSD: atrun.c,v 1.44 2017/06/07 17:59:36 millert Exp $      */
+/*     $OpenBSD: atrun.c,v 1.45 2017/06/07 23:36:43 millert Exp $      */
 
 /*
  * Copyright (c) 2002-2003 Todd C. Miller <Todd.Miller@courtesan.com>
@@ -317,6 +317,11 @@ run_job(const atjob *job, int dfd, const char *atfile)
                    atfile);
                _exit(EXIT_FAILURE);
        }
+       if (sb.st_gid != cron_gid) {
+               syslog(LOG_WARNING, "(%s) WRONG FILE GROUP (%s)", pw->pw_name,
+                   atfile);
+               _exit(EXIT_FAILURE);
+       }
        if (sb.st_nlink > 1) {
                syslog(LOG_WARNING, "(%s) BAD LINK COUNT (%s)", pw->pw_name,
                    atfile);

diff --git a/usr.sbin/cron/cron.c b/usr.sbin/cron/cron.c
index 2a76ae2..f76d95a 100644 (file)
--- a/usr.sbin/cron/cron.c
+++ b/usr.sbin/cron/cron.c
@@ -1,4 +1,4 @@
-/*     $OpenBSD: cron.c,v 1.75 2017/06/05 01:42:45 millert Exp $       */
+/*     $OpenBSD: cron.c,v 1.76 2017/06/07 23:36:43 millert Exp $       */
 
 /* Copyright 1988,1990,1993,1994 by Paul Vixie
  * Copyright (c) 2004 by Internet Systems Consortium, Inc. ("ISC")
@@ -67,6 +67,7 @@ static        at_db                   *at_database;
 static double                  batch_maxload = BATCH_MAXLOAD;
 static int                     NoFork;
 static time_t                  StartTime;
+       gid_t                   cron_gid;
 
 static void
 usage(void)
@@ -81,6 +82,7 @@ main(int argc, char *argv[])
 {
        struct sigaction sact;
        sigset_t blocked, omask;
+       struct group *grp;
 
        setlocale(LC_ALL, "");
 
@@ -107,6 +109,13 @@ main(int argc, char *argv[])
                exit(EXIT_FAILURE);
        }
 
+       if ((grp = getgrnam(CRON_GROUP)) == NULL) {
+               warnx("can't find cron group %s", CRON_GROUP);
+               syslog(LOG_ERR, "(CRON) DEATH (can't find cron group)");
+               exit(EXIT_FAILURE);
+       }
+       cron_gid = grp->gr_gid;
+
        cronSock = open_socket();
 
        if (putenv("PATH="_PATH_DEFPATH) < 0) {
@@ -420,12 +429,8 @@ open_socket(void)
 {
        int                sock, rc;
        mode_t             omask;
-       struct group *grp;
        struct sockaddr_un s_un;
 
-       if ((grp = getgrnam(CRON_GROUP)) == NULL)
-               syslog(LOG_WARNING, "(CRON) STARTUP (can't find cron group)");
-
        sock = socket(AF_UNIX, SOCK_STREAM|SOCK_CLOEXEC|SOCK_NONBLOCK, 0);
        if (sock == -1) {
                warn("socket");
@@ -462,12 +467,11 @@ open_socket(void)
                syslog(LOG_ERR, "(CRON) DEATH (can't listen on socket)");
                exit(EXIT_FAILURE);
        }
-       if (grp != NULL) {
-               /* pledge won't let us change files to a foreign group. */
-               if (setegid(grp->gr_gid) == 0) {
-                       chown(s_un.sun_path, -1, grp->gr_gid);
-                       (void)setegid(getgid());
-               }
+
+       /* pledge won't let us change files to a foreign group. */
+       if (setegid(cron_gid) == 0) {
+               chown(s_un.sun_path, -1, cron_gid);
+               (void)setegid(getgid());
        }
        chmod(s_un.sun_path, 0660);
 

diff --git a/usr.sbin/cron/database.c b/usr.sbin/cron/database.c
index 6fe1a0d..950c971 100644 (file)
--- a/usr.sbin/cron/database.c
+++ b/usr.sbin/cron/database.c
@@ -1,4 +1,4 @@
-/*     $OpenBSD: database.c,v 1.34 2016/01/11 14:23:50 millert Exp $   */
+/*     $OpenBSD: database.c,v 1.35 2017/06/07 23:36:43 millert Exp $   */
 
 /* Copyright 1988,1990,1993,1994 by Paul Vixie
  * Copyright (c) 2004 by Internet Systems Consortium, Inc. ("ISC")
@@ -34,6 +34,7 @@
 #include <unistd.h>
 
 #include "pathnames.h"
+#include "globals.h"
 #include "macros.h"
 #include "structs.h"
 #include "funcs.h"
@@ -170,7 +171,8 @@ process_crontab(int dfd, const char *uname, const char *fname,
 {
        struct passwd *pw = NULL;
        int crontab_fd = -1;
-       user *u;
+       user *u, *new_u;
+       mode_t tabmask, tabperm;
 
        /* Note: pw must remain NULL for system crontab (see below). */
        if (fname[0] != '/' && (pw = getpwnam(uname)) == NULL) {
@@ -196,19 +198,22 @@ process_crontab(int dfd, const char *uname, const char *fname,
                syslog(LOG_WARNING, "(%s) NOT REGULAR (%s)", uname, fname);
                goto next_crontab;
        }
-       if (pw != NULL) {
-               /* Looser permissions on system crontab. */
-               if ((statbuf->st_mode & 077) != 0) {
-                       syslog(LOG_WARNING, "(%s) BAD FILE MODE (%s)",
-                           uname, fname);
-                       goto next_crontab;
-               }
+       /* Looser permissions on system crontab. */
+       tabmask = pw ? ALLPERMS : (ALLPERMS & ~(S_IWUSR|S_IRGRP|S_IROTH));
+       tabperm = pw ? (S_IRUSR|S_IWUSR) : S_IRUSR;
+       if ((statbuf->st_mode & tabmask) != tabperm) {
+               syslog(LOG_WARNING, "(%s) BAD FILE MODE (%s)", uname, fname);
+               goto next_crontab;
        }
        if (statbuf->st_uid != 0 && (pw == NULL ||
            statbuf->st_uid != pw->pw_uid || strcmp(uname, pw->pw_name) != 0)) {
                syslog(LOG_WARNING, "(%s) WRONG FILE OWNER (%s)", uname, fname);
                goto next_crontab;
        }
+       if (pw != NULL && statbuf->st_gid != cron_gid) {
+               syslog(LOG_WARNING, "(%s) WRONG FILE GROUP (%s)", uname, fname);
+               goto next_crontab;
+       }
        if (pw != NULL && statbuf->st_nlink != 1) {
                syslog(LOG_WARNING, "(%s) BAD LINK COUNT (%s)", uname, fname);
                goto next_crontab;
@@ -224,21 +229,21 @@ process_crontab(int dfd, const char *uname, const char *fname,
                        TAILQ_INSERT_TAIL(&new_db->users, u, entries);
                        goto next_crontab;
                }
-
-               /* before we fall through to the code that will reload
-                * the user, let's deallocate and unlink the user in
-                * the old database.  This is more a point of memory
-                * efficiency than anything else, since all leftover
-                * users will be deleted from the old database when
-                * we finish with the crontab...
-                */
-               TAILQ_REMOVE(&old_db->users, u, entries);
-               free_user(u);
                syslog(LOG_INFO, "(%s) RELOAD (%s)", uname, fname);
        }
-       u = load_user(crontab_fd, pw, fname);
-       if (u != NULL) {
-               u->mtime = statbuf->st_mtim;
+
+       new_u = load_user(crontab_fd, pw, fname);
+       if (new_u != NULL) {
+               /* Insert user into the new database and remove from old. */
+               new_u->mtime = statbuf->st_mtim;
+               TAILQ_INSERT_TAIL(&new_db->users, new_u, entries);
+               if (u != NULL) {
+                       TAILQ_REMOVE(&old_db->users, u, entries);
+                       free_user(u);
+               }
+       } else if (u != NULL) {
+               /* New user crontab failed to load, preserve the old one. */
+               TAILQ_REMOVE(&old_db->users, u, entries);
                TAILQ_INSERT_TAIL(&new_db->users, u, entries);
        }
 

diff --git a/usr.sbin/cron/env.c b/usr.sbin/cron/env.c
index f8ce702..a26277d 100644 (file)
--- a/usr.sbin/cron/env.c
+++ b/usr.sbin/cron/env.c
@@ -1,4 +1,4 @@
-/*     $OpenBSD: env.c,v 1.32 2015/11/04 20:28:17 millert Exp $        */
+/*     $OpenBSD: env.c,v 1.33 2017/06/07 23:36:43 millert Exp $        */
 
 /* Copyright 1988,1990,1993,1994 by Paul Vixie
  * Copyright (c) 2004 by Internet Systems Consortium, Inc. ("ISC")
@@ -232,11 +232,8 @@ load_env(char *envstr, FILE *f)
                        break;
                }
        }
-       if (state != FINI && !(state == VALUE && !quotechar)) {
-               fseek(f, filepos, SEEK_SET);
-               Set_LineNum(fileline);
-               return (FALSE);
-       }
+       if (state != FINI && !(state == VALUE && !quotechar))
+               goto not_env;
        if (state == VALUE) {
                /* End of unquoted value: trim trailing whitespace */
                c = val + strlen(val);
@@ -251,6 +248,10 @@ load_env(char *envstr, FILE *f)
         * name and val fields.  Still, it doesn't hurt to be careful...
         */
        if (snprintf(envstr, MAX_ENVSTR, "%s=%s", name, val) >= MAX_ENVSTR)
-               return (FALSE);
+               goto not_env;
        return (TRUE);
+ not_env:
+       fseek(f, filepos, SEEK_SET);
+       Set_LineNum(fileline);
+       return (FALSE);
 }

diff --git a/usr.sbin/cron/globals.h b/usr.sbin/cron/globals.h
index 342f8e2..811ec7b 100644 (file)
--- a/usr.sbin/cron/globals.h
+++ b/usr.sbin/cron/globals.h
@@ -1,4 +1,4 @@
-/*     $OpenBSD: globals.h,v 1.13 2015/11/06 23:47:42 millert Exp $    */
+/*     $OpenBSD: globals.h,v 1.14 2017/06/07 23:36:43 millert Exp $    */
 
 /*
  * Copyright (c) 2004 by Internet Systems Consortium, Inc. ("ISC")
@@ -17,5 +17,6 @@
  * OF OR IN CONNECTION WITH THE USE OR PERFORMANCE OF THIS SOFTWARE.
  */
 
+extern gid_t   cron_gid;
 extern int     LineNumber;
 extern char    *__progname;

diff --git a/usr.sbin/cron/user.c b/usr.sbin/cron/user.c
index 0f19b0b..69a2640 100644 (file)
--- a/usr.sbin/cron/user.c
+++ b/usr.sbin/cron/user.c
@@ -1,4 +1,4 @@
-/*     $OpenBSD: user.c,v 1.19 2016/08/30 14:08:16 millert Exp $       */
+/*     $OpenBSD: user.c,v 1.20 2017/06/07 23:36:43 millert Exp $       */
 
 /* Copyright 1988,1990,1993,1994 by Paul Vixie
  * Copyright (c) 2004 by Internet Systems Consortium, Inc. ("ISC")
@@ -32,6 +32,7 @@
 #include "macros.h"
 #include "structs.h"
 #include "funcs.h"
+#include "globals.h"
 
 void
 free_user(user *u)
@@ -46,6 +47,16 @@ free_user(user *u)
        free(u);
 }
 
+static int ParseErrorCount;
+static const char *CrontabFilename;
+
+static void
+parse_error(const char *msg)
+{
+       ParseErrorCount++;
+       syslog(LOG_ERR, "(CRON) %s:%d (%s)", CrontabFilename, LineNumber, msg);
+}
+
 user *
 load_user(int crontab_fd, struct passwd        *pw, const char *name)
 {
@@ -57,9 +68,11 @@ load_user(int crontab_fd, struct passwd      *pw, const char *name)
        char **envp = NULL, **tenvp;
 
        if (!(file = fdopen(crontab_fd, "r"))) {
-               syslog(LOG_ERR, "(%s) FDOPEN (%m)", pw->pw_name);
+               syslog(LOG_ERR, "(%s) FDOPEN (%m)", name);
                return (NULL);
        }
+       CrontabFilename = name;
+       LineNumber = 0;
 
        /* file is open.  build user entry, then read the crontab file.
         */
@@ -86,13 +99,24 @@ load_user(int crontab_fd, struct passwd     *pw, const char *name)
 
        /* load the crontab
         */
+       ParseErrorCount = 0;
        while ((status = load_env(envstr, file)) >= 0) {
                switch (status) {
                case FALSE:
                        /* Not an env variable, parse as crontab entry. */
-                       e = load_entry(file, NULL, pw, envp);
-                       if (e)
+                       e = load_entry(file, parse_error, pw, envp);
+                       if (e == NULL) {
+                               /* Parse error, ignore for non-root entries */
+                               if (pw != NULL) {
+                                       save_errno = errno;
+                                       free_user(u);
+                                       u = NULL;
+                                       errno = save_errno;
+                                       goto done;
+                               }
+                       } else {
                                SLIST_INSERT_HEAD(&u->crontab, e, entries);
+                       }
                        break;
                case TRUE:
                        if ((tenvp = env_set(envp, envstr)) == NULL) {