From: Christian Kastner <ckk@kvr.at>
Date: Fri, 25 Dec 2015 13:07:25 +0100
Subject: Allow editors with tmpfiles

Certain editors such as vi use temporary files for editing. In the past, this
has caused problems with crontab's operation mode.

Fix provided by Steve Greenland <stevegr@debian.org>.

Bug-Debian: https://bugs.debian.org/149908
Bug-Debian: https://bugs.debian.org/413962
Forwarded: no
Last-Update: 2015-12-25
---
 crontab.c | 223 ++++++++++++++++++++++++++++++++++++++++++++++++++++----------
 1 file changed, 189 insertions(+), 34 deletions(-)

diff --git a/crontab.c b/crontab.c
index 24de55f..015da98 100644
--- a/crontab.c
+++ b/crontab.c
@@ -58,6 +58,7 @@ static char	*Options[] = { "???", "list", "delete", "edit", "replace" };
 static	PID_T		Pid;
 static	char		*User, *RealUser;
 static	char		Filename[MAX_FNAME];
+static	char		Directory[MAX_FNAME];
 static	FILE		*NewCrontab = NULL;
 static	int		CheckErrorCount;
 static	enum opt_t	Option;
@@ -70,6 +71,10 @@ static	void		list_cmd __P((void)),
 			parse_args __P((int c, char *v[]));
 static	int		replace_cmd __P((void));
 
+/* Support edit command */
+static	int		create_tmp_crontab __P((void));
+static	int		open_tmp_crontab __P((struct stat *fsbuf));
+static	void		cleanup_tmp_crontab __P((void));
 
 static void
 usage(msg)
@@ -318,13 +323,153 @@ check_error(msg)
 }
 
 
+/* The next several function implement 'crontab -e' */
+
+/* Returns -1 on error, or fd to tempfile. */
+static int
+create_tmp_crontab()
+{
+	const char *template = "/crontab.XXXXXX";
+	int nfd;
+	char *tmp;
+
+	/* Create the temp directory. Note that since crontab is
+	setuid(root), TMPDIR only work for root. */
+	if ((tmp=getenv("TMPDIR")) && strlen(tmp) < MAX_FNAME) {
+		strcpy(Directory, tmp);
+	} else {
+		strcpy(Directory,"/tmp");
+	}
+
+	if (strlen(Directory) + strlen(template) < MAX_FNAME) {
+		strcat(Directory, template);
+	} else {
+		fprintf(stderr, "TMPDIR value is to long -- exiting\n");
+		Directory[0] = '\0';
+		return -1;
+	}
+
+	if (!mkdtemp(Directory)) {
+		perror(Directory);
+		Directory[0] = '\0';
+		return -1;
+	}
+
+	if (chown(Directory, getuid(), getgid()) < 0) {
+		perror(Directory);
+		Directory[0] = '\0';
+		return -1;
+	}
+
+	/* Now create the actual temporary crontab file */
+	if (snprintf(Filename, MAX_FNAME, "%s/crontab", Directory) >= MAX_FNAME) {
+		fprintf(stderr, "Temporary filename too long - aborting\n");
+		Filename[0] = '\0';
+		return -1;
+	}
+	if ((nfd=open(Filename, O_CREAT|O_EXCL|O_WRONLY, 0600)) == -1) {
+		perror(Filename);
+		Filename[0] = '\0';
+		return -1;
+	}
+	return nfd;
+}
+
+/* Re-open the new (temporary) crontab, and check to make sure that
+   no-one is playing games. Return 0 on success, -1 on error. (Why not
+   just fopen() and stat()? Because there's no guarantee that you
+   fopen()ed the file you stat()ed.) */
+static int
+open_tmp_crontab(fsbuf)
+	struct stat *fsbuf;
+{
+	int t;
+	struct stat statbuf;
+
+	if ((t=open(Filename, O_RDONLY)) < 0) {
+		perror("Can't open tempfile after edit");
+		return -1;
+	}
+
+	if (fstat(t, &statbuf) < 0) {
+		perror("fstat");
+		return -1;
+	}
+	if (statbuf.st_uid != getuid() || statbuf.st_gid != getgid()) {
+		fprintf(stderr, "Temporary crontab no longer owned by you.\n");
+		return -1;;
+	}
+
+	if (!S_ISREG(statbuf.st_mode)) {
+		fprintf(stderr, "The temporary crontab must remain a regular file");
+		return -1;
+	}
+
+	if (statbuf.st_mtime == fsbuf->st_mtime) {
+		return 1; /* No change to file */
+	}
+
+	NewCrontab = fdopen(t, "r");
+	if (!NewCrontab) {
+		perror("fdopen(): after edit");
+		return -1;
+	}
+	return 0;
+}
+
+/* We can't just delete Filename, because the editor might have
+   created other temporary files in there. If there's an error, we
+   just bail, and let the user/admin deal with it.*/
+
+static void
+cleanup_tmp_crontab(void)
+{
+	DIR *dp;
+	struct dirent *ep;
+	char fname[MAX_FNAME];
+
+	if (Directory[0] == '\0') {
+		return;
+	}
+
+	/* Delete contents */
+	dp = opendir (Directory);
+	if (dp == NULL) {
+		perror(Directory);
+		return;
+	}
+
+	while ((ep = readdir (dp))) {
+		if (!strcmp(ep->d_name, ".") ||
+			!strcmp(ep->d_name, "..")) {
+			continue;
+		}
+		if (snprintf(fname, MAX_FNAME, "%s/%s",
+				Directory, ep->d_name) >= MAX_FNAME) {
+			fprintf(stderr, "filename too long to delete: %s/%s",
+				Directory, ep->d_name);
+			return;
+		}
+		if (unlink(fname)) {
+			perror(ep->d_name);
+			return;
+		}
+	}
+	(void) closedir (dp);
+
+	if (rmdir(Directory)) {
+		perror(Directory);
+		return;
+	}
+	return;
+}
+
 static void
 edit_cmd() {
 	char		n[MAX_FNAME], q[MAX_TEMPSTR], *editor;
 	FILE		*f;
 	int		ch, t, x;
-	struct stat	statbuf;
-	time_t		mtime;
+	struct stat	fsbuf;
 	WAIT_T		waiter;
 	PID_T		pid, xpid;
 	mode_t		um;
@@ -345,23 +490,15 @@ edit_cmd() {
 	}
 
 	um = umask(077);
-	(void) snprintf(Filename, sizeof(Filename)-1, "/tmp/crontab.XXXXXX");
-	Filename[sizeof(Filename)-1] = '\0';
-	if (-1 == (t = mkstemp(Filename))) {
-		perror(Filename);
-		goto fatal;
-	}
-#ifdef HAS_FCHOWN
-	if (fchown(t, getuid(), getgid()) < 0) {
-#else
-	if (chown(Filename, getuid(), getgid()) < 0) {
-#endif
-		perror("fchown");
+
+        if ((t=create_tmp_crontab()) < 0) {
+                fprintf(stderr, "Creation of temporary crontab file failed - aborting\n");
+                (void) umask(um);
 		goto fatal;
 	}
 
 	(void) umask(um);
-	if (!(NewCrontab = fdopen(t, "r+"))) {
+	if (!(NewCrontab = fdopen(t, "w"))) {
 		perror("fdopen");
 		goto fatal;
 	}
@@ -391,23 +528,14 @@ edit_cmd() {
 		while (EOF != (ch = get_char(f)))
 			putc(ch, NewCrontab);
 	fclose(f);
-	if (fflush(NewCrontab) < OK) {
-		perror(Filename);
-		exit(ERROR_EXIT);
-	}
- again:
-	rewind(NewCrontab);
 	if (ferror(NewCrontab)) {
 		fprintf(stderr, "%s: error while writing new crontab to %s\n",
 			ProgramName, Filename);
- fatal:		unlink(Filename);
-		exit(ERROR_EXIT);
 	}
-	if (fstat(t, &statbuf) < 0) {
-		perror("fstat");
+	if (fstat(t, &fsbuf) < 0) {
+		perror("unable to stat temp file");
 		goto fatal;
 	}
-	mtime = statbuf.st_mtime;
 
 	if ((!((editor = getenv("VISUAL")) && strlen(editor)))
 	 && (!((editor = getenv("EDITOR")) && strlen(editor)))
@@ -415,13 +543,15 @@ edit_cmd() {
 		editor = EDITOR;
 	}
 
-	/* we still have the file open.  editors will generally rewrite the
-	 * original file rather than renaming/unlinking it and starting a
-	 * new one; even backup files are supposed to be made by copying
-	 * rather than by renaming.  if some editor does not support this,
-	 * then don't use it.  the security problems are more severe if we
-	 * close and reopen the file around the edit.
-	 */
+
+        /*  Close before cleanup_tmp_crontab is called or otherwise
+         *  (on NFS mounted /) will get renamed on unlink */
+	if (fclose(NewCrontab) != 0) {
+		perror(Filename);
+                goto fatal;
+	}
+
+again: /* Loop point for retrying edit after error */
 
 	/* Turn off signals. */
 	(void)signal(SIGHUP, SIG_IGN);
@@ -487,6 +617,21 @@ edit_cmd() {
 	(void)signal(SIGQUIT, SIG_DFL);
 	(void)signal(SIGTSTP, SIG_DFL);
 
+	switch (open_tmp_crontab(&fsbuf)) {
+	case -1:
+		fprintf(stderr, "Error while editing crontab\n");
+		goto fatal;
+	case 1:
+		fprintf(stderr, "No modification made\n");
+		goto remove;
+	case 0:
+		break;
+	default:
+		fprintf(stderr,
+			"cron@packages.debian.org fscked up. Send him a nasty note\n");
+		break;
+	}
+
 	fprintf(stderr, "%s: installing new crontab\n", ProgramName);
 	switch (replace_cmd()) {
 	case 0:
@@ -517,10 +662,20 @@ edit_cmd() {
 			ProgramName);
 		goto fatal;
 	}
+
+	if (fclose(NewCrontab) != 0) {
+		perror(Filename);
+	}
+
  remove:
-	unlink(Filename);
+	cleanup_tmp_crontab();
  done:
 	log_it(RealUser, Pid, "END EDIT", User);
+        return;
+ fatal:
+	cleanup_tmp_crontab();
+	unlink(Filename);
+	exit(ERROR_EXIT);
 }
 
 static char tn[MAX_FNAME];