Looks good to me, ok nicm
On Mon, Nov 02, 2015 at 02:26:15PM -0700, Todd C. Miller wrote:
> at(1) tries to run as little code as possible with privileges. This
> creates a false sense of security since if there is an overflow an
> attacker can easily change the effective gid anyway.
>
> The only place we really need to drop the setgid crontab is when
> reading a file with the -f flag.
>
> - todd
>
> Index: usr.bin/at/at.c
> ===================================================================
> RCS file: /cvs/src/usr.bin/at/at.c,v
> retrieving revision 1.66
> diff -u -p -u -r1.66 at.c
> --- usr.bin/at/at.c 28 Oct 2015 20:17:31 -0000 1.66
> +++ usr.bin/at/at.c 2 Nov 2015 21:16:09 -0000
> @@ -35,7 +35,6 @@
>
> #include "cron.h"
> #include "at.h"
> -#include "privs.h"
> #include <limits.h>
>
> #define ALARMC 10 /* Number of seconds to wait for timeout */
> @@ -56,6 +55,9 @@ char vflag = 0; /* show completed but
> char force = 0; /* suppress errors (atrm) */
> char interactive = 0; /* interactive mode (atrm) */
> static int send_mail = 0; /* whether we are sending mail */
> +uid_t user_uid; /* user's real uid */
> +gid_t user_gid; /* user's real gid */
> +gid_t spool_gid; /* gid for writing to at spool */
>
> static void sigc(int);
> static void alarmc(int);
> @@ -77,11 +79,8 @@ static __dead void
> panic(const char *a)
> {
> (void)fprintf(stderr, "%s: %s\n", ProgramName, a);
> - if (fcreated) {
> - PRIV_START;
> + if (fcreated)
> unlink(atfile);
> - PRIV_END;
> - }
>
> exit(EXIT_FAILURE);
> }
> @@ -93,11 +92,8 @@ static __dead void
> panic2(const char *a, const char *b)
> {
> (void)fprintf(stderr, "%s: %s%s\n", ProgramName, a, b);
> - if (fcreated) {
> - PRIV_START;
> + if (fcreated)
> unlink(atfile);
> - PRIV_END;
> - }
>
> exit(EXIT_FAILURE);
> }
> @@ -110,11 +106,8 @@ perr(const char *a)
> {
> if (!force)
> perror(a);
> - if (fcreated) {
> - PRIV_START;
> + if (fcreated)
> unlink(atfile);
> - PRIV_END;
> - }
>
> exit(EXIT_FAILURE);
> }
> @@ -135,11 +128,8 @@ static void
> sigc(int signo)
> {
> /* If the user presses ^C, remove the spool file and exit. */
> - if (fcreated) {
> - PRIV_START;
> + if (fcreated)
> (void)unlink(atfile);
> - PRIV_END;
> - }
>
> _exit(EXIT_FAILURE);
> }
> @@ -204,8 +194,6 @@ writefile(const char *cwd, time_t runtim
> act.sa_flags = 0;
> sigaction(SIGINT, &act, NULL);
>
> - PRIV_START;
> -
> if ((lockdes = open(AT_DIR, O_RDONLY, 0)) < 0)
> perr("Cannot open jobs dir");
>
> @@ -241,11 +229,9 @@ writefile(const char *cwd, time_t runtim
> if ((fd2 = dup(fdes)) < 0)
> perr("Error in dup() of job file");
>
> - if (fchown(fd2, real_uid, real_gid) != 0)
> + if (fchown(fd2, user_uid, user_gid) != 0)
> perr("Cannot give away file");
>
> - PRIV_END;
> -
> /*
> * We've successfully created the file; let's set the flag so it
> * gets removed in case of an interrupt or error.
> @@ -268,7 +254,7 @@ writefile(const char *cwd, time_t runtim
>
> if ((mailname == NULL) || (mailname[0] == '\0') ||
> (strlen(mailname) > MAX_UNAME) || (getpwnam(mailname) == NULL)) {
> - pass_entry = getpwuid(real_uid);
> + pass_entry = getpwuid(user_uid);
> if (pass_entry != NULL)
> mailname = pass_entry->pw_name;
> }
> @@ -279,7 +265,7 @@ writefile(const char *cwd, time_t runtim
> * that, /bin/sh.
> */
> if ((shell = getenv("SHELL")) == NULL || *shell == '\0') {
> - pass_entry = getpwuid(real_uid);
> + pass_entry = getpwuid(user_uid);
> if (pass_entry != NULL && *pass_entry->pw_shell != '\0')
> shell = pass_entry->pw_shell;
> else
> @@ -287,7 +273,7 @@ writefile(const char *cwd, time_t runtim
> }
>
> (void)fprintf(fp, "#!/bin/sh\n# atrun uid=%lu gid=%lu\n# mail %*s %d\n",
> - (unsigned long)real_uid, (unsigned long)real_gid,
> + (unsigned long)user_uid, (unsigned long)user_gid,
> MAX_UNAME, mailname, send_mail);
>
> /* Write out the umask at the time of invocation */
> @@ -394,9 +380,7 @@ writefile(const char *cwd, time_t runtim
> (void)close(fd2);
>
> /* Poke cron so it knows to reload the at spool. */
> - PRIV_START;
> poke_daemon(AT_DIR, RELOAD_AT);
> - PRIV_END;
>
> runtime = *localtime(&runtimer);
> strftime(timestr, TIMESIZE, "%a %b %e %T %Y", &runtime);
> @@ -485,7 +469,7 @@ list_jobs(int argc, char **argv, int cou
> for (i = 0; i < argc; i++) {
> if ((pw = getpwnam(argv[i])) == NULL)
> panic2(argv[i], ": invalid user name");
> - if (pw->pw_uid != real_uid && real_uid != 0)
> + if (pw->pw_uid != user_uid && user_uid != 0)
> panic("Only the superuser may display other
> users' jobs");
> uids[i] = pw->pw_uid;
> }
> @@ -494,16 +478,12 @@ list_jobs(int argc, char **argv, int cou
>
> shortformat = strcmp(ProgramName, "at") == 0;
>
> - PRIV_START;
> -
> if (chdir(AT_DIR) != 0)
> perr2("Cannot change to ", AT_DIR);
>
> if ((spool = opendir(".")) == NULL)
> perr2("Cannot open ", AT_DIR);
>
> - PRIV_END;
> -
> if (fstat(dirfd(spool), &stbuf) != 0)
> perr2("Cannot stat ", AT_DIR);
>
> @@ -520,19 +500,15 @@ list_jobs(int argc, char **argv, int cou
>
> /* Loop over every file in the directory. */
> while ((dirent = readdir(spool)) != NULL) {
> - PRIV_START;
> -
> if (stat(dirent->d_name, &stbuf) != 0)
> perr2("Cannot stat in ", AT_DIR);
>
> - PRIV_END;
> -
> /*
> * See it's a regular file and has its x bit turned on and
> * is the user's
> */
> if (!S_ISREG(stbuf.st_mode)
> - || ((stbuf.st_uid != real_uid) && !(real_uid == 0))
> + || ((stbuf.st_uid != user_uid) && !(user_uid == 0))
> || !(S_IXUSR & stbuf.st_mode || vflag))
> continue;
>
> @@ -638,16 +614,12 @@ process_jobs(int argc, char **argv, int
> int job_matches, jobs_len, uids_len;
> int error, i, ch, changed;
>
> - PRIV_START;
> -
> if (chdir(AT_DIR) != 0)
> perr2("Cannot change to ", AT_DIR);
>
> if ((spool = opendir(".")) == NULL)
> perr2("Cannot open ", AT_DIR);
>
> - PRIV_END;
> -
> /* Convert argv into a list of jobs and uids. */
> jobs = NULL;
> uids = NULL;
> @@ -663,7 +635,7 @@ process_jobs(int argc, char **argv, int
> *(ep + 2) == '\0' && l > 0 && l < INT_MAX)
> jobs[jobs_len++] = argv[i];
> else if ((pw = getpwnam(argv[i])) != NULL) {
> - if (real_uid != pw->pw_uid && real_uid != 0) {
> + if (user_uid != pw->pw_uid && user_uid != 0) {
> fprintf(stderr, "%s: Only the superuser"
> " may %s other users' jobs\n",
> ProgramName, what == ATRM
> @@ -679,13 +651,10 @@ process_jobs(int argc, char **argv, int
> /* Loop over every file in the directory */
> changed = 0;
> while ((dirent = readdir(spool)) != NULL) {
> -
> - PRIV_START;
> if (stat(dirent->d_name, &stbuf) != 0)
> perr2("Cannot stat in ", AT_DIR);
> - PRIV_END;
>
> - if (stbuf.st_uid != real_uid && real_uid != 0)
> + if (stbuf.st_uid != user_uid && user_uid != 0)
> continue;
>
> if (strtot(dirent->d_name, &ep, &runtimer) == -1)
> @@ -718,8 +687,6 @@ process_jobs(int argc, char **argv, int
> if (job_matches) {
> switch (what) {
> case ATRM:
> - PRIV_START;
> -
> if (!interactive ||
> (interactive && rmok(runtimer))) {
> if (unlink(dirent->d_name) == 0)
> @@ -731,18 +698,10 @@ process_jobs(int argc, char **argv, int
> "%s removed\n",
> dirent->d_name);
> }
> -
> - PRIV_END;
> -
> break;
>
> case CAT:
> - PRIV_START;
> -
> fp = fopen(dirent->d_name, "r");
> -
> - PRIV_END;
> -
> if (!fp)
> perr("Cannot open file");
>
> @@ -773,12 +732,10 @@ process_jobs(int argc, char **argv, int
>
> /* If we modied the spool, poke cron so it knows to reload. */
> if (changed) {
> - PRIV_START;
> if (chdir(CRONDIR) != 0)
> perror(CRONDIR);
> else
> poke_daemon(AT_DIR, RELOAD_AT);
> - PRIV_END;
> }
>
> return (error);
> @@ -870,22 +827,14 @@ ttime(char *arg)
> static int
> check_permission(void)
> {
> - int ok;
> - uid_t uid = geteuid();
> struct passwd *pw;
>
> - if ((pw = getpwuid(uid)) == NULL) {
> + if ((pw = getpwuid(user_uid)) == NULL) {
> perror("Cannot access password database");
> exit(EXIT_FAILURE);
> }
>
> - PRIV_START;
> -
> - ok = allowed(pw->pw_name, AT_ALLOW, AT_DENY);
> -
> - PRIV_END;
> -
> - return (ok);
> + return (allowed(pw->pw_name, AT_ALLOW, AT_DENY));
> }
>
> static __dead void
> @@ -942,7 +891,9 @@ main(int argc, char **argv)
> else
> ProgramName = argv[0];
>
> - RELINQUISH_PRIVS;
> + user_uid = getuid();
> + user_gid = getgid();
> + spool_gid = getegid();
>
> /* find out what this program is supposed to do */
> if (strcmp(ProgramName, "atq") == 0) {
> @@ -1046,8 +997,12 @@ main(int argc, char **argv)
> case AT:
> case BATCH:
> if (atinput != NULL) {
> + if (setegid(user_gid) != 0)
> + perr("setegid(user_gid)");
> if (freopen(atinput, "r", stdin) == NULL)
> perr("Cannot open input file");
> + if (setegid(spool_gid) != 0)
> + perr("setegid(spool_gid)");
> }
> break;
> default:
> Index: usr.bin/at/privs.h
> ===================================================================
> RCS file: usr.bin/at/privs.h
> diff -N usr.bin/at/privs.h
> --- usr.bin/at/privs.h 2 Jul 2010 23:40:09 -0000 1.10
> +++ /dev/null 1 Jan 1970 00:00:00 -0000
> @@ -1,131 +0,0 @@
> -/* $OpenBSD: privs.h,v 1.10 2010/07/02 23:40:09 krw Exp $ */
> -
> -/*
> - * privs.h - header for privileged operations
> - * Copyright (C) 1993 Thomas Koenig
> - *
> - * Redistribution and use in source and binary forms, with or without
> - * modification, are permitted provided that the following conditions
> - * are met:
> - * 1. Redistributions of source code must retain the above copyright
> - * notice, this list of conditions and the following disclaimer.
> - * 2. The name of the author(s) may not be used to endorse or promote
> - * products derived from this software without specific prior written
> - * permission.
> - *
> - * THIS SOFTWARE IS PROVIDED BY THE AUTHOR(S) ``AS IS'' AND ANY EXPRESS OR
> - * IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE IMPLIED WARRANTIES
> - * OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE ARE DISCLAIMED.
> - * IN NO EVENT SHALL THE AUTHOR(S) BE LIABLE FOR ANY DIRECT, INDIRECT,
> - * INCIDENTAL, SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT
> - * NOT LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF USE,
> - * DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY
> - * THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT
> - * (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE OF
> - * THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
> - */
> -
> -#ifndef _PRIVS_H
> -#define _PRIVS_H
> -
> -#include <unistd.h>
> -
> -/* Relinquish privileges temporarily for a setuid or setgid program
> - * with the option of getting them back later. This is done by
> - * utilizing POSIX saved user and groups ids (or setreuid amd setregid if
> - * POSIX saved ids are not available). Call RELINQUISH_PRIVS once
> - * at the beginning of the main program. This will cause all operations
> - * to be executed with the real userid. When you need the privileges
> - * of the setuid/setgid invocation, call PRIV_START; when you no longer
> - * need it, call PRIV_END. Note that it is an error to call PRIV_START
> - * and not PRIV_END within the same function.
> - *
> - * Use RELINQUISH_PRIVS_ROOT(a,b) if your program started out running
> - * as root, and you want to drop back the effective userid to a
> - * and the effective group id to b, with the option to get them back
> - * later.
> - *
> - * Problems: Do not use return between PRIV_START and PRIV_END; this
> - * will cause the program to continue running in an unprivileged
> - * state.
> - *
> - * It is NOT safe to call exec(), system() or popen() with a user-
> - * supplied program (i.e. without carefully checking PATH and any
> - * library load paths) with relinquished privileges; the called program
> - * can acquire them just as easily. Set both effective and real userid
> - * to the real userid before calling any of them.
> - */
> -
> -#ifndef MAIN_PROGRAM
> -extern
> -#endif
> -uid_t real_uid, effective_uid;
> -
> -#ifndef MAIN_PROGRAM
> -extern
> -#endif
> -gid_t real_gid, effective_gid;
> -
> -#ifdef HAVE_SAVED_UIDS
> -
> -#define RELINQUISH_PRIVS do { \
> - real_uid = getuid(); \
> - effective_uid = geteuid(); \
> - real_gid = getgid(); \
> - effective_gid = getegid(); \
> - setegid(real_gid); \
> - seteuid(real_uid); \
> -} while (0)
> -
> -#define RELINQUISH_PRIVS_ROOT(a, b) do { \
> - real_uid = (a); \
> - effective_uid = geteuid(); \
> - real_gid = (b); \
> - effective_gid = getegid(); \
> - setegid(real_gid); \
> - seteuid(real_uid); \
> -} while (0)
> -
> -#define PRIV_START do { \
> - seteuid(effective_uid); \
> - setegid(effective_gid); \
> -} while (0)
> -
> -#define PRIV_END do { \
> - setegid(real_gid); \
> - seteuid(real_uid); \
> -} while (0)
> -
> -#else /* HAVE_SAVED_UIDS */
> -
> -#define RELINQUISH_PRIVS do { \
> - real_uid = getuid(); \
> - effective_uid = geteuid(); \
> - real_gid = getgid(); \
> - effective_gid = getegid(); \
> - setregid(effective_gid, real_gid); \
> - setreuid(effective_uid, real_uid); \
> -} while (0)
> -
> -#define RELINQUISH_PRIVS_ROOT(a, b) do { \
> - real_uid = (a); \
> - effective_uid = geteuid(); \
> - real_gid = (b); \
> - effective_gid = getegid(); \
> - setregid(effective_gid, real_gid); \
> - setreuid(effective_uid, real_uid); \
> -} while (0)
> -
> -#define PRIV_START do { \
> - setreuid(real_uid, effective_uid); \
> - setregid(real_gid, effective_gid); \
> -} while (0)
> -
> -#define PRIV_END do { \
> - setregid(effective_gid, real_gid); \
> - setreuid(effective_uid, real_uid); \
> -} while (0)
> -
> -#endif /* HAVE_SAVED_UIDS */
> -
> -#endif /* _PRIVS_H */
>
