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. What someone should really do is compltely rewrite crontab, that code is complete horror, especially for something that is suid. Wichert. -- _________________________________________________________________ /wichert@wiggy.net This space intentionally left occupied \ | wichert@deephackmode.org http://www.liacs.nl/~wichert/ | | 1024D/2FA3BC2D 576E 100B 518D 2F16 36B0 2805 3CB8 9250 2FA3 BC2D |
diff -wur org/cron-3.0pl1/crontab.c cron-3.0pl1/crontab.c
--- org/cron-3.0pl1/crontab.c Thu May 9 17:07:18 2002
+++ cron-3.0pl1/crontab.c Thu May 9 17:06:47 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,33 @@
}
}
- 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)
+ break;
+
+ if (errno==EEXIST)
+ continue;
+ perror(Directory);
+ goto fatal;
}
-#else
- strcpy(Filename,"/tmp");
-#endif
+ /* 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");
+ goto fatal;
+ }
+
+ sprintf(Filename, "%s/crontab", Directory);
+ if ((t=open(Filename,O_CREAT|O_EXCL|O_WRONLY, 0600))==-1) {
- (void) sprintf(Filename+strlen(Filename), "/crontab.XXXXXXXXXX");
- if ((t = mkstemp(Filename)) == -1) {
perror(Filename);
- (void) umask(um);
goto fatal;
}
- (void) umask(um);
#ifdef HAS_FCHOWN
if (fchown(t, getuid(), getgid()) < 0) {
#else
@@ -409,7 +418,7 @@
perror("fchown");
goto fatal;
}
- if (!(NewCrontab = fdopen(t, "r+"))) {
+ if (!(NewCrontab = fdopen(t, "w"))) {
perror("fdopen");
goto fatal;
}
@@ -447,24 +456,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 +474,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 +536,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 +586,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];
Attachment:
pgptkKL1geM2c.pgp
Description: PGP signature