Hi,
just my 2 cents:
On Wednesday 02 October 2013 00:22:31 Ryan Mallon wrote:
> The open_as_user() function in miscutils/crontab.c uses a child
> process to drop privileges and check that the file is readable
> by the specified user. If the child is successful then the (setuid)
> parent opens the file.
>
> This is racy because the file can be changed on disk (e.g.
> modified symlink) between the child exiting and the parent
> re-opening the file. This could allow an non-privileged user
> to pass a file to crontab that it does not have read access to.
> In practice this race is probably easy to win since a schedule
> must occur before the parent re-opens the file.
>
> Fix the race by using saving the effective uid/gid, and then
> setting the effective uid/gid to the specified user. The file is
> opened as the specified user, and then the old privileges are
> restored.
>
> Signed-off-by: Ryan Mallon <rmal...@gmail.com>
> ---
>
> diff --git a/miscutils/crontab.c b/miscutils/crontab.c
> index 4731d8d..b506935 100644
> --- a/miscutils/crontab.c
> +++ b/miscutils/crontab.c
> @@ -57,24 +57,32 @@ static void edit_file(const struct passwd *pas, const
> char *file)
>
> static int open_as_user(const struct passwd *pas, const char *file)
> {
> - pid_t pid;
> - char c;
> -
> - pid = xvfork();
> - if (pid) { /* PARENT */
> - if (wait4pid(pid) == 0) {
> - /* exitcode 0: child says it can read */
> - return open(file, O_RDONLY);
> - }
> - return -1;
> - }
> -
> - /* CHILD */
> - /* initgroups, setgid, setuid */
> - change_identity(pas);
> - /* We just try to read one byte. If it works, file is readable
> - * under this user. We signal that by exiting with 0. */
> - _exit(safe_read(xopen(file, O_RDONLY), &c, 1) < 0);
> + uid_t old_euid;
> + gid_t old_egid;
> + int fd, err;
> +
> + /* Save and drop privileges */
> + old_euid = geteuid();
> + old_egid = getegid();
Making this a function could eventually reduce size.
change_privileges(uid_t uid, gid_t gid)
{
if ( setegid(gid) < 0 || seteuid(uid) < 0)
bb_perror_msg_and_die("failed to set egid=%d euid=%d ", gid,
uid);
}
> + err = setegid(pas->pw_gid);
> + if (err)
> + bb_perror_msg_and_die("failed to drop privileges");
> + err = seteuid(pas->pw_uid);
> + if (err)
> + bb_perror_msg_and_die("failed to drop privileges");
> +
> + /* Open the file as the specified user */
> + fd = xopen(file, O_RDONLY);
> +
> + /* Restore privileges */
> + err = setegid(old_egid);
> + if (err)
> + bb_perror_msg_and_die("failed to restore privileges");
> + err = seteuid(old_euid);
> + if (err)
> + bb_perror_msg_and_die("failed to restore privileges");
> +
> + return fd;
> }
>
> int crontab_main(int argc, char **argv) MAIN_EXTERNALLY_VISIBLE;
>
Ciao,
Tito
_______________________________________________
busybox mailing list
busybox@busybox.net
http://lists.busybox.net/mailman/listinfo/busybox
