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.

6 files changed, 86 insertions, 46 deletions

diff --git a/usr.sbin/cron/atrun.c b/usr.sbin/cron/atrun.c
index d1414d42592..4f230c30b19 100644
--- 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 2a76ae249c4..f76d95a1de2 100644
--- 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 6fe1a0d2ee3..950c9713d91 100644
--- 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 f8ce702ae08..a26277da4e1 100644
--- 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 342f8e217ac..811ec7b5562 100644
--- 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 0f19b0bdc2c..69a2640ed03 100644
--- 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) {