From 68c1073fe52680f31682d3381d8824f709e40ec7 Mon Sep 17 00:00:00 2001 From: "Todd C. Miller" Date: Sun, 10 Jan 2016 18:31:29 -0700 Subject: [PATCH] Rewritten sudoedit_checkdir support that checks all the dirs in the path and refuses to follow symlinks in writable directories. This is a better fix for CVE-2015-5602. Adapted from a diff by Ben Hutchings. Bug #707 --- doc/CONTRIBUTORS | 1 + doc/UPGRADE | 9 ++ doc/sudoers.cat | 17 ++-- doc/sudoers.man.in | 16 ++-- doc/sudoers.mdoc.in | 16 ++-- include/sudo_compat.h | 2 + src/sudo_edit.c | 186 +++++++++++++++++++++++++++++++----------- 7 files changed, 181 insertions(+), 66 deletions(-) diff --git a/doc/CONTRIBUTORS b/doc/CONTRIBUTORS index 47c98422b..b5f08004a 100644 --- a/doc/CONTRIBUTORS +++ b/doc/CONTRIBUTORS @@ -58,6 +58,7 @@ you believe you should be listed, please send a note to sudo@sudo.ws. Holloway, Nick Hoover, Adam Hunter, Michael T. + Hutchings, Ben Irrgang, Eric Jackson, Brian Jackson, John R. diff --git a/doc/UPGRADE b/doc/UPGRADE index 2f4a0db2f..133e70e50 100644 --- a/doc/UPGRADE +++ b/doc/UPGRADE @@ -1,6 +1,15 @@ Notes on upgrading from an older release ======================================== +o Upgrading from a version prior to 1.8.16: + + The meaning of the sudoedit_checkdir sudoers option has changed + in 1.8.16. Previously, it would only check the parent directory + of the file to be edited. In 1.8.16 and higher all directories + in the path to be edited are checked and sudoedit will refuse + to follow a symbolic link in a directory that is writable by + the invoking user. + o Upgrading from a version prior to 1.8.15: Prior to version 1.8.15, when env_reset was enabled (the default) diff --git a/doc/sudoers.cat b/doc/sudoers.cat index 31ecc0a69..c5ce069f5 100644 --- a/doc/sudoers.cat +++ b/doc/sudoers.cat @@ -1291,12 +1291,15 @@ SSUUDDOOEERRSS OOPPTTIIOONNSS system call. This flag is _o_f_f by default. sudoedit_checkdir - If set, ssuuddooeeddiitt will refuse to edit files located in a - directory that is writable by the invoking user unless - it is run by root. On many systems, this option - requires that the parent directory of the file to be - edited be readable by the target user. This flag is - _o_f_f by default. + If set, ssuuddooeeddiitt will check directories in the path to + be edited for writability by the invoking user. + Symbolic links will not be followed in writable + directories and ssuuddooeeddiitt will also refuse to edit a + file located in a writable directory. Theses + restrictions are not enforced when ssuuddooeeddiitt is invoked + as root. On many systems, this option requires that + all directories in the path to be edited be readable by + the target user. This flag is _o_f_f by default. sudoedit_follow By default, ssuuddooeeddiitt will not follow symbolic links when opening files. The _s_u_d_o_e_d_i_t___f_o_l_l_o_w option can be @@ -2495,4 +2498,4 @@ DDIISSCCLLAAIIMMEERR file distributed with ssuuddoo or https://www.sudo.ws/license.html for complete details. -Sudo 1.8.16 January 4, 2015 Sudo 1.8.16 +Sudo 1.8.16 January 9, 2016 Sudo 1.8.16 diff --git a/doc/sudoers.man.in b/doc/sudoers.man.in index 7875060d0..3c7c9728f 100644 --- a/doc/sudoers.man.in +++ b/doc/sudoers.man.in @@ -21,7 +21,7 @@ .\" Agency (DARPA) and Air Force Research Laboratory, Air Force .\" Materiel Command, USAF, under agreement number F39502-99-1-0512. .\" -.TH "SUDOERS" "5" "January 4, 2015" "Sudo @PACKAGE_VERSION@" "File Formats Manual" +.TH "SUDOERS" "5" "January 9, 2016" "Sudo @PACKAGE_VERSION@" "File Formats Manual" .nh .if n .ad l .SH "NAME" @@ -2747,10 +2747,16 @@ sudoedit_checkdir .br If set, \fBsudoedit\fR -will refuse to edit files located in a directory that is writable -by the invoking user unless it is run by root. -On many systems, this option requires that the parent directory -of the file to be edited be readable by the target user. +will check directories in the path to be edited for writability +by the invoking user. +Symbolic links will not be followed in writable directories and +\fBsudoedit\fR +will also refuse to edit a file located in a writable directory. +Theses restrictions are not enforced when +\fBsudoedit\fR +is invoked as root. +On many systems, this option requires that all directories +in the path to be edited be readable by the target user. This flag is \fIoff\fR by default. diff --git a/doc/sudoers.mdoc.in b/doc/sudoers.mdoc.in index 307428a25..4aff03ca8 100644 --- a/doc/sudoers.mdoc.in +++ b/doc/sudoers.mdoc.in @@ -19,7 +19,7 @@ .\" Agency (DARPA) and Air Force Research Laboratory, Air Force .\" Materiel Command, USAF, under agreement number F39502-99-1-0512. .\" -.Dd January 4, 2015 +.Dd January 9, 2016 .Dt SUDOERS @mansectform@ .Os Sudo @PACKAGE_VERSION@ .Sh NAME @@ -2581,10 +2581,16 @@ by default. .It sudoedit_checkdir If set, .Nm sudoedit -will refuse to edit files located in a directory that is writable -by the invoking user unless it is run by root. -On many systems, this option requires that the parent directory -of the file to be edited be readable by the target user. +will check directories in the path to be edited for writability +by the invoking user. +Symbolic links will not be followed in writable directories and +.Nm sudoedit +will also refuse to edit a file located in a writable directory. +Theses restrictions are not enforced when +.Nm sudoedit +is invoked as root. +On many systems, this option requires that all directories +in the path to be edited be readable by the target user. This flag is .Em off by default. diff --git a/include/sudo_compat.h b/include/sudo_compat.h index 7fb234eef..8ca8d5ba0 100644 --- a/include/sudo_compat.h +++ b/include/sudo_compat.h @@ -182,6 +182,8 @@ # ifndef UTIME_NOW # define UTIME_NOW -2L # endif +#endif +#if !defined(HAVE_OPENAT) || (!defined(HAVE_FUTIMENS) && !defined(HAVE_UTIMENSAT)) # ifndef AT_FDCWD # define AT_FDCWD -100 # endif diff --git a/src/sudo_edit.c b/src/sudo_edit.c index 3a9c06dfb..ce060d972 100644 --- a/src/sudo_edit.c +++ b/src/sudo_edit.c @@ -179,13 +179,15 @@ group_matches(gid_t target, gid_t gid, int ngroups, GETGROUPS_T *groups) } #ifndef HAVE_OPENAT -/* This does not support AT_FDCWD... */ static int sudo_openat(int dfd, const char *path, int flags, mode_t mode) { int fd, odfd; debug_decl(sudo_openat, SUDO_DEBUG_EDIT) + if (dfd == AT_FDCWD) + debug_return_int(open(path, flags, mode)); + /* Save cwd */ if ((odfd = open(".", O_RDONLY)) == -1) debug_return_int(-1); @@ -207,6 +209,64 @@ sudo_openat(int dfd, const char *path, int flags, mode_t mode) #define openat sudo_openat #endif /* HAVE_OPENAT */ +#ifdef O_NOFOLLOW +static int +sudo_edit_openat_nofollow(int dfd, char *path, int oflags, mode_t mode) +{ + debug_decl(sudo_edit_open_nofollow, SUDO_DEBUG_EDIT) + + debug_return_int(openat(dfd, path, oflags|O_NOFOLLOW, mode)); +} +#else +/* + * Returns true if fd and path don't match or path is a symlink. + * Used on older systems without O_NOFOLLOW. + */ +static bool +sudo_edit_is_symlink(int fd, char *path) +{ + struct stat sb1, sb2; + debug_decl(sudo_edit_is_symlink, SUDO_DEBUG_EDIT) + + /* + * Treat [fl]stat() failure like there was a symlink. + */ + if (fstat(fd, &sb1) == -1 || lstat(path, &sb2) == -1) + debug_return_bool(true); + + /* + * Make sure we did not open a link and that what we opened + * matches what is currently on the file system. + */ + if (S_ISLNK(sb2.st_mode) || + sb1.st_dev != sb2.st_dev || sb1.st_ino != sb2.st_ino) { + debug_return_bool(true); + } + + debug_return_bool(false); +} + +static int +sudo_edit_openat_nofollow(char *path, int oflags, mode_t mode) +{ + struct stat sb1, sb2; + int fd; + debug_decl(sudo_edit_openat_nofollow, SUDO_DEBUG_EDIT) + + fd = openat(dfd, path, oflags, mode); + if (fd == -1) + debug_return_int(-1); + + if (sudo_edit_is_symlink(fd, path)) { + close(fd); + fd = -1; + errno = ELOOP; + } + + debug_return_int(fd); +} +#endif /* O_NOFOLLOW */ + /* * Returns true if the directory described by sb is writable * by the user. We treat directories with the sticky bit as @@ -245,55 +305,100 @@ dir_is_writable(struct stat *sb, uid_t uid, gid_t gid, int ngroups, debug_return_bool(false); } +/* + * Directory open flags for use with openat(2) and fstat(2). + * Use O_PATH and O_DIRECTORY where possible. + */ +#if defined(O_PATH) && defined(O_DIRECTORY) +# define DIR_OPEN_FLAGS (O_PATH|O_DIRECTORY) +#elif defined(O_PATH) && !defined(O_DIRECTORY) +# define DIR_OPEN_FLAGS O_PATH +#elif !defined(O_PATH) && defined(O_DIRECTORY) +# define DIR_OPEN_FLAGS (O_RDONLY|O_DIRECTORY) +#else +# define DIR_OPEN_FLAGS (O_RDONLY|O_NONBLOCK) +#endif + static int sudo_edit_open_nonwritable(char *path, int oflags, mode_t mode) { - char *base, *dir; + int dfd, fd, dflags = DIR_OPEN_FLAGS; +#if defined(__linux__) && defined(O_PATH) + char *opath = path; +#endif + bool is_writable; struct stat sb; - int dfd, fd; debug_decl(sudo_edit_open_nonwritable, SUDO_DEBUG_EDIT) - base = strrchr(path, '/'); - if (base != NULL) { - *base++ = '\0'; - dir = path; +#if defined(__linux__) && defined(O_PATH) +restart: +#endif + if (path[0] == '/') { + dfd = open("/", dflags); + path++; } else { - base = path; - dir = "."; + dfd = open(".", dflags); + if (path[0] == '.' && path[1] == '/') + path += 2; } -#ifdef O_PATH - if ((dfd = open(dir, O_PATH)) != -1) { - /* Linux kernels < 3.6 can't do fstat on O_PATH fds. */ + if (dfd == -1) + debug_return_int(-1); + + for (;;) { + char *slash; + int subdfd; + + /* + * Look up one component at a time, avoiding symbolic links in + * writable directories. + */ if (fstat(dfd, &sb) == -1) { close(dfd); - dfd = open(dir, O_RDONLY); - if (fstat(dfd, &sb) == -1) { - close(dfd); - dfd = -1; +#if defined(__linux__) && defined(O_PATH) + /* Linux prior to 3.6 can't fstat an O_PATH fd */ + if (ISSET(dflags, O_PATH)) { + CLR(dflags, O_PATH); + path = opath; + goto restart; } +#endif + debug_return_int(-1); } - } -#else - if ((dfd = open(dir, O_RDONLY)) != -1) { - if (fstat(dfd, &sb) == -1) { +#ifndef O_DIRECTORY + if (!S_ISDIR(sb.st_mode)) { close(dfd); - dfd = -1; + errno = ENOTDIR; + debug_return_int(-1); } - } #endif - if (base != path) - base[-1] = '/'; /* restore path */ - if (dfd == -1) - debug_return_int(-1); + is_writable = dir_is_writable(&sb, user_details.uid, user_details.gid, + user_details.ngroups, user_details.groups); + + while (path[0] == '/') + path++; + slash = strchr(path, '/'); + if (slash == NULL) + break; + *slash = '\0'; + if (is_writable) + subdfd = sudo_edit_openat_nofollow(dfd, path, dflags, 0); + else + subdfd = openat(dfd, path, dflags, 0); + *slash = '/'; /* restore path */ + close(dfd); + if (subdfd == -1) + debug_return_int(-1); + path = slash + 1; + dfd = subdfd; + } - if (dir_is_writable(&sb, user_details.uid, user_details.gid, - user_details.ngroups, user_details.groups)) { + if (is_writable) { close(dfd); errno = EISDIR; debug_return_int(-1); } - fd = openat(dfd, base, oflags, mode); + fd = openat(dfd, path, oflags, mode); close(dfd); debug_return_int(fd); } @@ -332,27 +437,10 @@ sudo_edit_open(char *path, int oflags, mode_t mode, int sflags) if (!ISSET(oflags, O_NONBLOCK)) (void) fcntl(fd, F_SETFL, fcntl(fd, F_GETFL, 0) & ~O_NONBLOCK); - /* - * Treat [fl]stat() failure like an open() failure. - */ - if (fstat(fd, &sb1) == -1 || lstat(path, &sb2) == -1) { - const int serrno = errno; + if (!ISSET(sflags, CD_SUDOEDIT_FOLLOW) && sudo_edit_is_symlink(fd, path)) { close(fd); - errno = serrno; - debug_return_int(-1); - } - - /* - * Make sure we did not open a link and that what we opened - * matches what is currently on the file system. - */ - if (!ISSET(sflags, CD_SUDOEDIT_FOLLOW)) { - if (S_ISLNK(sb2.st_mode) || - sb1.st_dev != sb2.st_dev || sb1.st_ino != sb2.st_ino) { - close(fd); - errno = ELOOP; - debug_return_int(-1); - } + fd = -1; + errno = ELOOP; } debug_return_int(fd); -- 2.40.0