[Date Prev][Date Next] [Thread Prev][Thread Next] [Date Index] [Thread Index]

Re: crontab and editors (was Re: Editor Priorities)



On 09-May-02, 10:10 (CDT), Wichert Akkerman <wichert@wiggy.net> wrote: 
> Previously Steve Greenland wrote:
> > Submit an audited patch. 
> 
> Attached. The new code creates a temporary directory securely and
> puts a temporary file in there. This is a guaranteed safe procedure
> and allows an editor freedom to create backup files as well.

Unfortunately, it doesn't work. The problem is this:

> +		Directory=tmpnam(NULL);
> +		if (mkdir(Directory, 0700)==0)
> +			break;

because the directory is owned by root.<users_group>, and thus the
editor can't open the file. We can't simply make it mode 0770, because
not all systems run usergroups. So I changed it to this:


		Directory=tmpnam(NULL);
		if (mkdir(Directory, 0700)==0) {
	        if (chown(Directory, getuid(), getgid()) < 0) {
                perror(Directory);
                goto fatal;
            }
			break;
        }

That seems correct, and works as expected, but I wanted to check and
make sure I've not missed something unsafe.

I've also added few Directory=NULL and Filename=NULL before jumps to
fatal: to avoid trying to delete a directory/file we didn't create.

I've attached the new version of the patch, I'd appreciate any comments
w.r.t. the security of it (but you can skip the "geez, the crontab code
is hideous", as we know that).

Steve

-- 
Steve Greenland
--- crontab.c.HEAD	Sun May 12 14:59:31 2002
+++ crontab.c	Sun May 12 15:01:51 2002
@@ -32,6 +32,7 @@
 #include <errno.h>
 #include <fcntl.h>
 #include <signal.h>
+#include <stdio.h>
 #include <sys/file.h>
 #include <sys/stat.h>
 #ifdef USE_UTIMES
@@ -359,13 +360,14 @@
 edit_cmd() {
 	char		n[MAX_FNAME], q[MAX_TEMPSTR], *editor;
 	FILE		*f;
-	int		ch, t, x;
+	int		ch, t, x, r;
 	struct stat	statbuf, fsbuf;
 	time_t		mtime;
 	WAIT_T		waiter;
 	PID_T		pid, xpid;
-	mode_t		um;
+	char*		Directory = NULL;
 
+	Filename[0]='\0';
 	log_it(RealUser, Pid, "BEGIN EDIT", User);
 	(void) snprintf(n, MAX_FNAME, CRON_TAB(User));
 	if (!(f = fopen(n, "r"))) {
@@ -381,26 +383,41 @@
 		}
 	}
 
-	um = umask(077);
-#if 0
-	/* The support for TMPDIR is temporarily removed, because of
-	   interactions with emacs */
-	if (getenv("TMPDIR")) {
-	  strcpy(Filename, getenv("TMPDIR"));
-	} else {
-	  strcpy(Filename,"/tmp");
+	/* Try 5 times to create a temporary directory. This is safe, even
+	 * over NFS.
+	 */
+	for (t=0; t<5; t++) {
+		Directory=tmpnam(NULL);
+		if (mkdir(Directory, 0700)==0) {
+	        if (chown(Directory, getuid(), getgid()) < 0) {
+                perror(Directory);
+                goto fatal;
+            }
+			break;
 	}
-#else
-	  strcpy(Filename,"/tmp");
-#endif
 	 
-	(void) sprintf(Filename+strlen(Filename), "/crontab.XXXXXXXXXX");
-	if ((t = mkstemp(Filename)) == -1) {
+		if (errno==EEXIST)
+			continue;
+		perror(Directory);
+		Directory = NULL; /* To keep fatal: from trying to rmdir() it */
+		goto fatal;
+	}
+	/* If we hit an existing filename 5 times assume someone is trying
+	 * a symlink attack, so abort.
+	 */
+	if (t==5) {
+		fprintf(stderr, "Somebody is trying to attack us - aborting\n");
+		Directory=NULL; /* To keep fatal: from trying to rmdir() it */
+		goto fatal;
+	}
+
+	sprintf(Filename, "%s/crontab", Directory);
+	if ((t=open(Filename,O_CREAT|O_EXCL|O_WRONLY, 0600))==-1) {
+	 
 		perror(Filename);
-		(void) umask(um);
+        Filename=NULL;
 		goto fatal;
 	}
-	(void) umask(um);
 #ifdef HAS_FCHOWN
 	if (fchown(t, getuid(), getgid()) < 0) {
 #else
@@ -409,7 +426,7 @@
 		perror("fchown");
 		goto fatal;
 	}
-	if (!(NewCrontab = fdopen(t, "r+"))) {
+	if (!(NewCrontab = fdopen(t, "w"))) {
 		perror("fdopen");
 		goto fatal;
 	}
@@ -447,24 +464,17 @@
 		perror("unable to fstat temp file");
 		goto fatal;
 	}
- 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");
 		goto fatal;
 	}
-	if (statbuf.st_dev != fsbuf.st_dev || statbuf.st_ino != fsbuf.st_ino) {
-		fprintf(stderr, "temp file must be edited in place\n");
-		exit(ERROR_EXIT);
-	}
+
+	fclose(NewCrontab);
 
 	mtime = statbuf.st_mtime;
+ again:
+
 
 	if ((!(editor = getenv("VISUAL")))
 	 && (!(editor = getenv("EDITOR")))
@@ -472,14 +482,6 @@
 		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.
-	 */
-
 	/* Turn off signals. */
 	(void)signal(SIGHUP, SIG_IGN);
 	(void)signal(SIGINT, SIG_IGN);
@@ -542,21 +544,26 @@
 	(void)signal(SIGINT, SIG_DFL);
 	(void)signal(SIGQUIT, SIG_DFL);
 	(void)signal(SIGTSTP, SIG_DFL);
-	if (statbuf.st_dev != fsbuf.st_dev || statbuf.st_ino != fsbuf.st_ino) {
-		fprintf(stderr, "temp file must be edited in place\n");
-		exit(ERROR_EXIT);
+
+	if ((NewCrontab=fopen(Filename, "r"))==NULL) {
+		perror("fopen");
+		goto fatal;
 	}
+	t=fileno(NewCrontab);
 	if (fstat(t, &statbuf) < 0) {
 		perror("fstat");
 		goto fatal;
 	}
+
 	if (mtime == statbuf.st_mtime) {
 		fprintf(stderr, "%s: no changes made to crontab\n",
 			ProgramName);
 		goto remove;
 	}
 	fprintf(stderr, "%s: installing new crontab\n", ProgramName);
-	switch (replace_cmd()) {
+	r=replace_cmd();
+	fclose(NewCrontab);
+	switch (r) {
 	case 0:
 		break;
 	case -1:
@@ -587,8 +594,17 @@
 	}
  remove:
 	unlink(Filename);
+	rmdir(Directory);
  done:
 	log_it(RealUser, Pid, "END EDIT", User);
+	return;
+
+ fatal:
+	if (Filename[0])
+		unlink(Filename);
+	if (Directory)
+		rmdir(Directory);
+	exit(ERROR_EXIT);
 }
 
 static char tn[MAX_FNAME];

Reply to: