Date: Fri, 2 Aug 2002 23:33:15 +0200 (CEST) From: Pawel Jakub Dawidek <nick@garage.freebsd.pl> To: FreeBSD-gnats-submit@FreeBSD.org Subject: bin/41271: Non-suid-crontab. Message-ID: <20020802213315.CA1943ABD6C@milla.ask33.net>
next in thread | raw e-mail | index | archive | help
>Number: 41271
>Category: bin
>Synopsis: Non-suid-crontab.
>Confidential: no
>Severity: non-critical
>Priority: medium
>Responsible: freebsd-bugs
>State: open
>Quarter:
>Keywords:
>Date-Required:
>Class: change-request
>Submitter-Id: current-users
>Arrival-Date: Fri Aug 02 14:40:01 PDT 2002
>Closed-Date:
>Last-Modified:
>Originator: Pawel Jakub Dawidek
>Release: FreeBSD 4.6-STABLE i386
>Organization:
>Environment:
System: FreeBSD milla.ask33.net 4.6-STABLE FreeBSD 4.6-STABLE #9: Wed Jul 31 21:18:46 CEST 2002 root@milla.ask33.net:/usr/obj/usr/src/sys/MILLA i386
>Description:
Now crontab(1) is a set-uid-root binary. This is not needed, crontab
could be only set-gid on unprivileged group, for example ,,crontab''.
I'm sending a patch that is a port of crontab from OpenBSD, but with
serious hole fixed. This patch fix some little bugs in FreeBSD
crontab version too. I've added also path size checks.
>How-To-Repeat:
>Fix:
diff -ru /usr/src/usr.sbin/cron/cron/compat.h cron/cron/compat.h
--- /usr/src/usr.sbin/cron/cron/compat.h Sat Aug 28 03:15:49 1999
+++ cron/cron/compat.h Thu Aug 1 20:32:19 2002
@@ -109,7 +109,7 @@
#ifdef POSIX
#include <unistd.h>
#ifdef _POSIX_SAVED_IDS
-# define HAVE_SAVED_UIDS
+# define HAVE_SAVED_GIDS
#endif
#endif
@@ -133,8 +133,4 @@
#if !defined(UNICOS) && !defined(UNIXPC)
# define HAS_FCHOWN
-#endif
-
-#if !defined(UNICOS) && !defined(UNIXPC)
-# define HAS_FCHMOD
#endif
diff -ru /usr/src/usr.sbin/cron/cron/cron.h cron/cron/cron.h
--- /usr/src/usr.sbin/cron/cron/cron.h Tue May 29 01:37:26 2001
+++ cron/cron/cron.h Fri Aug 2 03:12:50 2002
@@ -74,6 +74,8 @@
#define MAX_TEMPSTR 100 /* obvious */
#define MAX_UNAME 20 /* max length of username, should be overkill */
#define ROOT_UID 0 /* don't change this, it really must be root */
+#define WHEEL_GID 0 /* don't change this, it really must be wheel */
+#define CRONTAB_GID 88 /* gid of "crontab" group */
#define ROOT_USER "root" /* ditto */
/* NOTE: these correspond to DebugFlagNames,
@@ -219,7 +221,7 @@
set_debug_flags __P((char *)),
get_char __P((FILE *)),
get_string __P((char *, int, FILE *, char *)),
- swap_uids __P((void)),
+ swap_gids __P((void)),
load_env __P((char *, FILE *)),
cron_pclose __P((FILE *)),
strcmp_until __P((char *, char *, int)),
diff -ru /usr/src/usr.sbin/cron/cron/database.c cron/cron/database.c
--- /usr/src/usr.sbin/cron/cron/database.c Sat Aug 28 03:15:50 1999
+++ cron/cron/database.c Fri Aug 2 03:38:49 2002
@@ -87,7 +87,7 @@
new_db.head = new_db.tail = NULL;
if (syscron_stat.st_mtime) {
- process_crontab("root", "*system*",
+ process_crontab(ROOT_USER, "*system*",
SYSCRONTAB, &syscron_stat,
&new_db, old_db);
}
@@ -113,9 +113,13 @@
if (dp->d_name[0] == '.')
continue;
- (void) strncpy(fname, dp->d_name, sizeof(fname));
- fname[sizeof(fname)-1] = '\0';
+ (void) strncpy(fname, dp->d_name, sizeof fname - 1);
+ fname[sizeof fname - 1] = '\0';
+ if (strlen(fname) >= sizeof fname - 1)
+ continue; /* XXX log? */
(void) snprintf(tabname, sizeof tabname, CRON_TAB(fname));
+ if (strlen(tabname) >= sizeof tabname - 1)
+ continue; /* XXX log? */
process_crontab(fname, fname, tabname,
&statbuf, &new_db, old_db);
@@ -182,7 +186,6 @@
cron_db *db;
char *name;
{
- char *env_get();
user *u;
for (u = db->head; u != NULL; u = u->next)
@@ -204,6 +207,8 @@
struct passwd *pw = NULL;
int crontab_fd = OK - 1;
user *u;
+ uid_t pwuid;
+ gid_t pwgid;
if (strcmp(fname, "*system*") && !(pw = getpwnam(uname))) {
/* file doesn't have a user in passwd file.
@@ -211,6 +216,14 @@
log_it(fname, getpid(), "ORPHAN", "no passwd entry");
goto next_crontab;
}
+ /* this have to be done, because of fake "*system*" files */
+ if (pw) {
+ pwuid = pw->pw_uid;
+ pwgid = pw->pw_gid;
+ } else {
+ pwuid = ROOT_UID;
+ pwgid = WHEEL_GID;
+ }
if ((crontab_fd = open(tabname, O_RDONLY, 0)) < OK) {
/* crontab not accessible?
@@ -221,6 +234,26 @@
if (fstat(crontab_fd, statbuf) < OK) {
log_it(fname, getpid(), "FSTAT FAILED", tabname);
+ goto next_crontab;
+ }
+ if (!S_ISREG(statbuf->st_mode)) {
+ log_it(fname, getpid(), "NOT REGULAR", tabname);
+ goto next_crontab;
+ }
+ if ((statbuf->st_mode & 07777) != 0600) {
+ log_it(fname, getpid(), "BAD FILE MODE", tabname);
+ goto next_crontab;
+ }
+ if (statbuf->st_uid != pwuid) {
+ log_it(fname, getpid(), "WRONG FILE USER OWNER", tabname);
+ goto next_crontab;
+ }
+ if (statbuf->st_gid != pwgid) {
+ log_it(fname, getpid(), "WRONG FILE GROUP OWNER", tabname);
+ goto next_crontab;
+ }
+ if (statbuf->st_nlink != 1) {
+ log_it(fname, getpid(), "BAD LINK COUNT", tabname);
goto next_crontab;
}
diff -ru /usr/src/usr.sbin/cron/crontab/Makefile cron/crontab/Makefile
--- /usr/src/usr.sbin/cron/crontab/Makefile Wed Apr 25 14:09:24 2001
+++ cron/crontab/Makefile Fri Aug 2 03:47:20 2002
@@ -8,7 +8,8 @@
BINDIR= /usr/bin
BINOWN= root
-BINMODE=4555
+BINGRP= crontab
+BINMODE=2555
INSTALLFLAGS=-fschg
.include <bsd.prog.mk>
diff -ru /usr/src/usr.sbin/cron/crontab/crontab.c cron/crontab/crontab.c
--- /usr/src/usr.sbin/cron/crontab/crontab.c Sat Jun 16 05:18:37 2001
+++ cron/crontab/crontab.c Fri Aug 2 09:56:36 2002
@@ -57,7 +57,7 @@
static PID_T Pid;
static char User[MAX_UNAME], RealUser[MAX_UNAME];
-static char Filename[MAX_FNAME];
+static char Filename[MAX_FNAME + 1], TempFilename[MAX_FNAME + 1];
static FILE *NewCrontab;
static int CheckErrorCount;
static enum opt_t Option;
@@ -69,6 +69,7 @@
check_error __P((char *)),
parse_args __P((int c, char *v[]));
static int replace_cmd __P((void));
+static void clean_turds(int);
static void
@@ -101,7 +102,6 @@
setlinebuf(stderr);
#endif
parse_args(argc, argv); /* sets many globals, opens a file */
- set_cron_uid();
set_cron_cwd();
if (!allowed(User)) {
warnx("you (%s) are not allowed to use this program", User);
@@ -200,20 +200,12 @@
if (!strcmp(Filename, "-")) {
NewCrontab = stdin;
} else {
- /* relinquish the setuid status of the binary during
- * the open, lest nonroot users read files they should
- * not be able to read. we can't use access() here
- * since there's a race condition. thanks go out to
- * Arnt Gulbrandsen <agulbra@pvv.unit.no> for spotting
- * the race.
- */
-
- if (swap_uids() < OK)
- err(ERROR_EXIT, "swapping uids");
+ if (swap_gids() < OK)
+ err(ERROR_EXIT, "swapping gids");
if (!(NewCrontab = fopen(Filename, "r")))
err(ERROR_EXIT, "%s", Filename);
- if (swap_uids() < OK)
- err(ERROR_EXIT, "swapping uids back");
+ if (swap_gids() < OK)
+ err(ERROR_EXIT, "swapping gids back");
}
}
@@ -224,12 +216,14 @@
static void
list_cmd() {
- char n[MAX_FNAME];
+ char n[MAX_FNAME + 1];
FILE *f;
int ch, x;
log_it(RealUser, Pid, "LIST", User);
- (void) sprintf(n, CRON_TAB(User));
+ (void) snprintf(n, sizeof n, CRON_TAB(User));
+ if (strlen(n) >= sizeof n - 1)
+ err(ERROR_EXIT, "path too long");
if (!(f = fopen(n, "r"))) {
if (errno == ENOENT)
errx(ERROR_EXIT, "no crontab for %s", User);
@@ -266,7 +260,7 @@
static void
delete_cmd() {
- char n[MAX_FNAME];
+ char n[MAX_FNAME + 1];
int ch, first;
if (isatty(STDIN_FILENO)) {
@@ -279,7 +273,9 @@
}
log_it(RealUser, Pid, "DELETE", User);
- (void) sprintf(n, CRON_TAB(User));
+ (void) snprintf(n, sizeof n, CRON_TAB(User));
+ if (strlen(n) >= sizeof n - 1)
+ err(ERROR_EXIT, "path too long");
if (unlink(n)) {
if (errno == ENOENT)
errx(ERROR_EXIT, "no crontab for %s", User);
@@ -301,17 +297,18 @@
static void
edit_cmd() {
- char n[MAX_FNAME], q[MAX_TEMPSTR], *editor;
+ char n[MAX_FNAME + 1], q[MAX_TEMPSTR], *editor;
FILE *f;
int ch, t, x;
struct stat statbuf, fsbuf;
time_t mtime;
WAIT_T waiter;
PID_T pid, xpid;
- mode_t um;
log_it(RealUser, Pid, "BEGIN EDIT", User);
- (void) sprintf(n, CRON_TAB(User));
+ (void) snprintf(n, sizeof n, CRON_TAB(User));
+ if (strlen(n) >= sizeof n - 1)
+ err(ERROR_EXIT, "path too long");
if (!(f = fopen(n, "r"))) {
if (errno != ENOENT)
err(ERROR_EXIT, "%s", n);
@@ -320,20 +317,19 @@
err(ERROR_EXIT, _PATH_DEVNULL);
}
- um = umask(077);
- (void) sprintf(Filename, "/tmp/crontab.XXXXXXXXXX");
- if ((t = mkstemp(Filename)) == -1) {
- warn("%s", Filename);
- (void) umask(um);
- goto fatal;
- }
- (void) umask(um);
+ strncpy(Filename, "/tmp/crontab.XXXXXXXXXX", sizeof Filename - 1);
+ Filename[sizeof Filename - 1] = '\0';
+ if (strlen(Filename) >= sizeof Filename - 1)
+ err(ERROR_EXIT, "path too long");
+ if ((t = mkstemp(Filename)) == -1)
+ err(ERROR_EXIT, "%s", Filename);
#ifdef HAS_FCHOWN
if (fchown(t, getuid(), getgid()) < 0) {
+ warn("fchown");
#else
if (chown(Filename, getuid(), getgid()) < 0) {
+ warn("chown");
#endif
- warn("fchown");
goto fatal;
}
if (!(NewCrontab = fdopen(t, "r+"))) {
@@ -402,8 +398,8 @@
goto fatal;
case 0:
/* child */
- if (setuid(getuid()) < 0)
- err(ERROR_EXIT, "setuid(getuid())");
+ if (setgid(getgid()) < 0)
+ err(ERROR_EXIT, "setgid(getgid())");
if (chdir("/tmp") < 0)
err(ERROR_EXIT, "chdir(/tmp)");
if (strlen(editor) + strlen(Filename) + 2 >= MAX_TEMPSTR)
@@ -491,9 +487,10 @@
*/
static int
replace_cmd() {
- char n[MAX_FNAME], envstr[MAX_ENVSTR], tn[MAX_FNAME];
+ char n[MAX_FNAME + 1], envstr[MAX_ENVSTR];
FILE *tmp;
- int ch, eof;
+ int ch, eof, fd;
+ int error = 0;
entry *e;
time_t now = time(NULL);
char **envp = env_init();
@@ -503,13 +500,28 @@
return (-2);
}
- (void) sprintf(n, "tmp.%d", Pid);
- (void) sprintf(tn, CRON_TAB(n));
- if (!(tmp = fopen(tn, "w+"))) {
- warn("%s", tn);
+ (void) snprintf(TempFilename, sizeof TempFilename,
+ CRON_TAB("tmp.XXXXXXXXX"));
+ if (strlen(TempFilename) >= sizeof TempFilename - 1) {
+ TempFilename[0] = '\0';
+ warn("path too long");
+ return (-2);
+ }
+
+ if ((fd = mkstemp(TempFilename)) == -1 || !(tmp = fdopen(fd, "w+"))) {
+ warn("%s", TempFilename);
+ if (fd != -1) {
+ close(fd);
+ unlink(TempFilename);
+ }
+ TempFilename[0] = '\0';
return (-2);
}
+ (void) signal(SIGHUP, clean_turds);
+ (void) signal(SIGINT, clean_turds);
+ (void) signal(SIGQUIT, clean_turds);
+
/* write a signature at the top of the file.
*
* VERY IMPORTANT: make sure NHEADER_LINES agrees with this code.
@@ -528,9 +540,10 @@
fflush(tmp); rewind(tmp);
if (ferror(tmp)) {
- warnx("error while writing new crontab to %s", tn);
- fclose(tmp); unlink(tn);
- return (-2);
+ warnx("error while writing new crontab to %s", TempFilename);
+ fclose(tmp);
+ error = -2;
+ goto done;
}
/* check the syntax of the file being installed.
@@ -559,49 +572,54 @@
if (CheckErrorCount != 0) {
warnx("errors in crontab file, can't install");
- fclose(tmp); unlink(tn);
- return (-1);
+ fclose(tmp);
+ error = -1;
+ goto done;
}
#ifdef HAS_FCHOWN
- if (fchown(fileno(tmp), ROOT_UID, -1) < OK)
+ if (fchown(fileno(tmp), getuid(), getgid()) < OK) {
+ warn("fchown");
#else
- if (chown(tn, ROOT_UID, -1) < OK)
-#endif
- {
+ if (chown(TempFilename, getuid(), getgid()) < OK) {
warn("chown");
- fclose(tmp); unlink(tn);
- return (-2);
- }
-
-#ifdef HAS_FCHMOD
- if (fchmod(fileno(tmp), 0600) < OK)
-#else
- if (chmod(tn, 0600) < OK)
#endif
- {
- warn("chown");
- fclose(tmp); unlink(tn);
- return (-2);
+ fclose(tmp);
+ error = -2;
+ goto done;
}
if (fclose(tmp) == EOF) {
warn("fclose");
- unlink(tn);
- return (-2);
+ error = -2;
+ goto done;
}
- (void) sprintf(n, CRON_TAB(User));
- if (rename(tn, n)) {
- warn("error renaming %s to %s", tn, n);
- unlink(tn);
- return (-2);
+ (void) snprintf(n, sizeof n, CRON_TAB(User));
+ if (strlen(n) >= sizeof n - 1) {
+ warn("path too long");
+ error = -2;
+ goto done;
}
+ if (rename(TempFilename, n)) {
+ warn("error renaming %s to %s", TempFilename, n);
+ error = -2;
+ goto done;
+ }
+ TempFilename[0] = '\0';
log_it(RealUser, Pid, "REPLACE", User);
poke_daemon();
- return (0);
+done:
+ (void) signal(SIGHUP, SIG_DFL);
+ (void) signal(SIGINT, SIG_DFL);
+ (void) signal(SIGQUIT, SIG_DFL);
+ if (TempFilename[0]) {
+ (void) unlink(TempFilename);
+ TempFilename[0] = '\0';
+ }
+ return (error);
}
@@ -623,4 +641,20 @@
return;
}
#endif /*USE_UTIMES*/
+}
+
+
+static void
+clean_turds(signo)
+ int signo;
+{
+ int save_errno = errno;
+
+ if (TempFilename[0])
+ (void) unlink(TempFilename);
+ if (signo) {
+ (void) signal(signo, SIG_DFL);
+ (void) raise(signo);
+ }
+ errno = save_errno;
}
diff -ru /usr/src/usr.sbin/cron/lib/misc.c cron/lib/misc.c
--- /usr/src/usr.sbin/cron/lib/misc.c Mon Apr 29 00:45:53 2002
+++ cron/lib/misc.c Fri Aug 2 03:32:07 2002
@@ -210,7 +210,11 @@
*/
if (stat(SPOOL_DIR, &sb) < OK && errno == ENOENT) {
warn("%s", SPOOL_DIR);
- if (OK == mkdir(SPOOL_DIR, 0700)) {
+ if (OK == mkdir(SPOOL_DIR, 0730)) {
+ if (OK != chown(SPOOL_DIR, ROOT_UID, CRONTAB_GID)) {
+ rmdir(SPOOL_DIR);
+ err(ERROR_EXIT, "%s: chown", SPOOL_DIR);
+ }
warnx("%s: created", SPOOL_DIR);
stat(SPOOL_DIR, &sb);
} else {
@@ -219,6 +223,20 @@
}
if (!(sb.st_mode & S_IFDIR))
err(ERROR_EXIT, "'%s' is not a directory, bailing out", SPOOL_DIR);
+ if (sb.st_uid != ROOT_UID || sb.st_gid != CRONTAB_GID) {
+ if (OK != chown(SPOOL_DIR, ROOT_UID, CRONTAB_GID)) {
+ rmdir(SPOOL_DIR);
+ err(ERROR_EXIT, "%s: chown", SPOOL_DIR);
+ }
+ warnx("%s: set owner user and group", SPOOL_DIR);
+ }
+ if ((sb.st_mode & 07777) != 0730) {
+ if (OK != chmod(SPOOL_DIR, 0730)) {
+ rmdir(SPOOL_DIR);
+ err(ERROR_EXIT, "%s: chmod", SPOOL_DIR);
+ }
+ warnx("%s: set permissions", SPOOL_DIR);
+ }
}
@@ -244,15 +262,19 @@
}
if (!fp) {
- char pidfile[MAX_FNAME];
+ char pidfile[MAX_FNAME + 1];
char buf[MAX_TEMPSTR];
int fd, otherpid;
- (void) sprintf(pidfile, PIDFILE, PIDDIR);
+ (void) snprintf(pidfile, sizeof pidfile, PIDFILE, PIDDIR);
+ if (strlen(pidfile) >= sizeof pidfile - 1) {
+ log_it("CRON", getpid(), "DEATH", "path too long");
+ errx(ERROR_EXIT, "path too long");
+ }
if ((-1 == (fd = open(pidfile, O_RDWR|O_CREAT, 0644)))
|| (NULL == (fp = fdopen(fd, "r+")))
) {
- sprintf(buf, "can't open or create %s: %s",
+ snprintf(buf, sizeof buf, "can't open or create %s: %s",
pidfile, strerror(errno));
log_it("CRON", getpid(), "DEATH", buf);
errx(ERROR_EXIT, "%s", buf);
@@ -262,7 +284,8 @@
int save_errno = errno;
fscanf(fp, "%d", &otherpid);
- sprintf(buf, "can't lock %s, otherpid may be %d: %s",
+ snprintf(buf, sizeof buf,
+ "can't lock %s, otherpid may be %d: %s",
pidfile, otherpid, strerror(save_errno));
log_it("CRON", getpid(), "DEATH", buf);
errx(ERROR_EXIT, "%s", buf);
@@ -390,7 +413,7 @@
char line[MAX_TEMPSTR];
rewind(file);
- while (fgets(line, MAX_TEMPSTR, file)) {
+ while (fgets(line, sizeof line, file)) {
if (line[0] != '\0')
if (line[strlen(line)-1] == '\n')
line[strlen(line)-1] = '\0';
@@ -410,20 +433,13 @@
allowed(username)
char *username;
{
- static int init = FALSE;
- static FILE *allow, *deny;
+ static FILE *allow = NULL, *deny = NULL;
- if (!init) {
- init = TRUE;
#if defined(ALLOW_FILE) && defined(DENY_FILE)
- allow = fopen(ALLOW_FILE, "r");
- deny = fopen(DENY_FILE, "r");
- Debug(DMISC, ("allow/deny enabled, %d/%d\n", !!allow, !!deny))
-#else
- allow = NULL;
- deny = NULL;
+ allow = fopen(ALLOW_FILE, "r");
+ deny = fopen(DENY_FILE, "r");
+ Debug(DMISC, ("allow/deny enabled, %d/%d\n", !!allow, !!deny))
#endif
- }
if (allow)
return (in_file(username, allow));
@@ -591,7 +607,7 @@
*dst++ = '^';
*dst++ = '?';
} else { /* parity character */
- sprintf(dst, "\\%03o", ch);
+ snprintf(dst, 5, "\\%03o", ch);
dst += 4;
}
}
@@ -645,11 +661,9 @@
#endif /*MAIL_DATE*/
-#ifdef HAVE_SAVED_UIDS
-static int save_euid;
-int swap_uids() { save_euid = geteuid(); return seteuid(getuid()); }
-int swap_uids_back() { return seteuid(save_euid); }
-#else /*HAVE_SAVED_UIDS*/
-int swap_uids() { return setreuid(geteuid(), getuid()); }
-int swap_uids_back() { return swap_uids(); }
-#endif /*HAVE_SAVED_UIDS*/
+#ifdef HAVE_SAVED_GIDS
+static int save_egid;
+int swap_gids() { save_egid = getegid(); return setegid(getgid()); }
+#else /*HAVE_SAVED_GIDS*/
+int swap_gids() { return setregid(getegid(), getgid()); }
+#endif /*HAVE_SAVED_GIDS*/
>Release-Note:
>Audit-Trail:
>Unformatted:
To Unsubscribe: send mail to majordomo@FreeBSD.org
with "unsubscribe freebsd-bugs" in the body of the message
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20020802213315.CA1943ABD6C>