From c50cead833bc0a6f05461855555e31df0b10b2c4 Mon Sep 17 00:00:00 2001 From: "Todd C. Miller" Date: Fri, 23 Oct 2015 14:04:35 -0600 Subject: [PATCH] Add directory writability checks for sudoedit. --- NEWS | 3 + config.h.in | 3 + configure | 3 + configure.ac | 2 +- doc/sudoers.cat | 10 +- doc/sudoers.man.in | 14 ++- doc/sudoers.mdoc.in | 12 +- plugins/sudoers/def_data.c | 4 + plugins/sudoers/def_data.h | 6 +- plugins/sudoers/def_data.in | 3 + plugins/sudoers/policy.c | 4 + plugins/sudoers/sudoers_version.h | 2 +- src/sudo.c | 5 + src/sudo.h | 1 + src/sudo_edit.c | 178 ++++++++++++++++++++++++++++-- 15 files changed, 232 insertions(+), 18 deletions(-) diff --git a/NEWS b/NEWS index 61dbb8536..ccc052cd1 100644 --- a/NEWS +++ b/NEWS @@ -74,6 +74,9 @@ What's new in Sudo 1.8.15 * Fixed challenge/response style BSD authentication. + * Added a sudoers option to prevent sudoedit from editing files + located in a directory that is writable by the invoking user. + What's new in Sudo 1.8.14p3 * Fixed a bug introduced in sudo 1.8.14p2 that prevented sudo diff --git a/config.h.in b/config.h.in index 241479e61..23ab0bff6 100644 --- a/config.h.in +++ b/config.h.in @@ -481,6 +481,9 @@ /* Define to 1 if you have the `nss_search' function. */ #undef HAVE_NSS_SEARCH +/* Define to 1 if you have the `openat' function. */ +#undef HAVE_OPENAT + /* Define to 1 if you have the `openpty' function. */ #undef HAVE_OPENPTY diff --git a/configure b/configure index 7ad1bf445..1f6ecc5cb 100755 --- a/configure +++ b/configure @@ -2655,6 +2655,7 @@ as_fn_append ac_func_list " nl_langinfo" as_fn_append ac_func_list " strftime" as_fn_append ac_func_list " pread" as_fn_append ac_func_list " pwrite" +as_fn_append ac_func_list " openat" as_fn_append ac_func_list " seteuid" # Check that the precious variables saved in the cache have kept the same # value. @@ -18075,6 +18076,8 @@ done + + for ac_func in getgrouplist do : ac_fn_c_check_func "$LINENO" "getgrouplist" "ac_cv_func_getgrouplist" diff --git a/configure.ac b/configure.ac index 23c9158df..7cbff15e0 100644 --- a/configure.ac +++ b/configure.ac @@ -2384,7 +2384,7 @@ dnl dnl Function checks dnl AC_FUNC_GETGROUPS -AC_CHECK_FUNCS_ONCE([killpg nl_langinfo strftime pread pwrite]) +AC_CHECK_FUNCS_ONCE([killpg nl_langinfo strftime pread pwrite openat]) AC_CHECK_FUNCS([getgrouplist], [], [ case "$host_os" in aix*) diff --git a/doc/sudoers.cat b/doc/sudoers.cat index 201d2a466..ce77649c9 100644 --- a/doc/sudoers.cat +++ b/doc/sudoers.cat @@ -1267,6 +1267,14 @@ SSUUDDOOEERRSS OOPPTTIIOONNSS that support either the setreuid(2) or setresuid(2) 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. + 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 enabled to allow ssuuddooeeddiitt to open symbolic links. It @@ -2464,4 +2472,4 @@ DDIISSCCLLAAIIMMEERR file distributed with ssuuddoo or http://www.sudo.ws/license.html for complete details. -Sudo 1.8.15 September 25, 2015 Sudo 1.8.15 +Sudo 1.8.15 October 23, 2015 Sudo 1.8.15 diff --git a/doc/sudoers.man.in b/doc/sudoers.man.in index 251d9a2c6..88908cd81 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" "September 25, 2015" "Sudo @PACKAGE_VERSION@" "File Formats Manual" +.TH "SUDOERS" "5" "October 23, 2015" "Sudo @PACKAGE_VERSION@" "File Formats Manual" .nh .if n .ad l .SH "NAME" @@ -2700,6 +2700,18 @@ This flag is \fIoff\fR by default. .TP 18n +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. +This flag is +\fIoff\fR +by default. +.TP 18n sudoedit_follow By default, \fBsudoedit\fR diff --git a/doc/sudoers.mdoc.in b/doc/sudoers.mdoc.in index fbfe57734..bf88dcc41 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 September 25, 2015 +.Dd October 23, 2015 .Dt SUDOERS @mansectform@ .Os Sudo @PACKAGE_VERSION@ .Sh NAME @@ -2536,6 +2536,16 @@ system call. This flag is .Em off 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. +This flag is +.Em off +by default. .It sudoedit_follow By default, .Nm sudoedit diff --git a/plugins/sudoers/def_data.c b/plugins/sudoers/def_data.c index 40c509a59..6d75d3907 100644 --- a/plugins/sudoers/def_data.c +++ b/plugins/sudoers/def_data.c @@ -386,6 +386,10 @@ struct sudo_defs_types sudo_defs_table[] = { "use_netgroups", T_FLAG, N_("Enable sudoers netgroup support"), NULL, + }, { + "sudoedit_checkdir", T_FLAG, + N_("Check the parent directory for writability when editing files with sudoedit"), + NULL, }, { "sudoedit_follow", T_FLAG, N_("Follow symbolic links when editing files with sudoedit"), diff --git a/plugins/sudoers/def_data.h b/plugins/sudoers/def_data.h index 7fbe1d804..6faa8f075 100644 --- a/plugins/sudoers/def_data.h +++ b/plugins/sudoers/def_data.h @@ -180,8 +180,10 @@ #define I_MAXSEQ 89 #define def_use_netgroups (sudo_defs_table[90].sd_un.flag) #define I_USE_NETGROUPS 90 -#define def_sudoedit_follow (sudo_defs_table[91].sd_un.flag) -#define I_SUDOEDIT_FOLLOW 91 +#define def_sudoedit_checkdir (sudo_defs_table[91].sd_un.flag) +#define I_SUDOEDIT_CHECKDIR 91 +#define def_sudoedit_follow (sudo_defs_table[92].sd_un.flag) +#define I_SUDOEDIT_FOLLOW 92 enum def_tuple { never, diff --git a/plugins/sudoers/def_data.in b/plugins/sudoers/def_data.in index 55a2126ad..72d35df50 100644 --- a/plugins/sudoers/def_data.in +++ b/plugins/sudoers/def_data.in @@ -286,6 +286,9 @@ maxseq use_netgroups T_FLAG "Enable sudoers netgroup support" +sudoedit_checkdir + T_FLAG + "Check the parent directory for writability when editing files with sudoedit" sudoedit_follow T_FLAG "Follow symbolic links when editing files with sudoedit" diff --git a/plugins/sudoers/policy.c b/plugins/sudoers/policy.c index 4b8e6539a..06a1b5713 100644 --- a/plugins/sudoers/policy.c +++ b/plugins/sudoers/policy.c @@ -438,6 +438,10 @@ sudoers_policy_exec_setup(char *argv[], char *envp[], mode_t cmnd_umask, if (ISSET(sudo_mode, MODE_EDIT)) { if ((command_info[info_len++] = strdup("sudoedit=true")) == NULL) goto oom; + if (def_sudoedit_checkdir) { + if ((command_info[info_len++] = strdup("sudoedit_checkdir=true")) == NULL) + goto oom; + } if (def_sudoedit_follow) { if ((command_info[info_len++] = strdup("sudoedit_follow=true")) == NULL) goto oom; diff --git a/plugins/sudoers/sudoers_version.h b/plugins/sudoers/sudoers_version.h index ce9539416..37e708f02 100644 --- a/plugins/sudoers/sudoers_version.h +++ b/plugins/sudoers/sudoers_version.h @@ -63,7 +63,7 @@ * 42 sudo 1.8.6, Support for empty Runas_List (with or without a colon) to mean the invoking user. Support for Solaris Privilege Sets (PRIVS= and LIMITPRIVS=). * 43 sudo 1.8.7, Support for specifying a digest along with the command. * 44 sudo 1.8.13, added MAIL/NOMAIL tags. - * 45 sudo 1.8.15, added FOLLOW/NOFOLLOW tags and sudoedit_follow Default. + * 45 sudo 1.8.15, added FOLLOW/NOFOLLOW tags as well as sudoedit_follow and sudoedit_checkdir Defaults. */ #ifndef SUDOERS_VERSION_H diff --git a/src/sudo.c b/src/sudo.c index c8ae36517..578642bea 100644 --- a/src/sudo.c +++ b/src/sudo.c @@ -727,6 +727,11 @@ command_info_to_details(char * const info[], struct command_details *details) SET(details->flags, CD_SUDOEDIT); break; } + if (strncmp("sudoedit_checkdir=", info[i], sizeof("sudoedit_checkdir=") - 1) == 0) { + if (sudo_strtobool(info[i] + sizeof("sudoedit_checkdir=") - 1) == true) + SET(details->flags, CD_SUDOEDIT_CHECKDIR); + break; + } if (strncmp("sudoedit_follow=", info[i], sizeof("sudoedit_follow=") - 1) == 0) { if (sudo_strtobool(info[i] + sizeof("sudoedit_follow=") - 1) == true) SET(details->flags, CD_SUDOEDIT_FOLLOW); diff --git a/src/sudo.h b/src/sudo.h index c1692d7be..1c00b1bfc 100644 --- a/src/sudo.h +++ b/src/sudo.h @@ -128,6 +128,7 @@ struct user_details { #define CD_EXEC_BG 0x04000 #define CD_SUDOEDIT_COPY 0x08000 #define CD_SUDOEDIT_FOLLOW 0x10000 +#define CD_SUDOEDIT_CHECKDIR 0x20000 struct preserved_fd { TAILQ_ENTRY(preserved_fd) entries; diff --git a/src/sudo_edit.c b/src/sudo_edit.c index f6cfa338b..ca8442103 100644 --- a/src/sudo_edit.c +++ b/src/sudo_edit.c @@ -31,6 +31,7 @@ #endif /* HAVE_STRINGS_H */ #include #include +#include #include #include #include @@ -154,28 +155,180 @@ sudo_edit_mktemp(const char *ofile, char **tfile) debug_return_int(tfd); } +static bool +group_matches(gid_t target, gid_t gid, int ngroups, GETGROUPS_T *groups) +{ + int i; + debug_decl(group_matches, SUDO_DEBUG_EDIT) + + if (target == gid) { + sudo_debug_printf(SUDO_DEBUG_INFO|SUDO_DEBUG_LINENO, + "user gid %u matches directory gid %u", (unsigned int)gid, + (unsigned int)target); + debug_return_bool(true); + } + for (i = 0; i < ngroups; i++) { + if (target == groups[i]) { + sudo_debug_printf(SUDO_DEBUG_INFO|SUDO_DEBUG_LINENO, + "user gid %u matches directory gid %u", (unsigned int)gid, + (unsigned int)target); + debug_return_bool(true); + } + } + debug_return_bool(false); +} + +#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) + + /* Save cwd */ + if ((odfd = open(".", O_RDONLY)) == -1) + debug_return_int(-1); + + if (fchdir(dfd) == -1) { + close(odfd); + debug_return_int(-1); + } + + fd = open(path, flags, mode); + + /* Restore cwd */ + if (fchdir(odfd) == -1) + sudo_fatal(_("unable to restore current working directory")); + close(odfd); + + debug_return_int(fd); +} +#define openat sudo_openat +#endif /* HAVE_OPENAT */ + +/* + * Returns true if the directory described by sb is writable + * by the user. We treat directories with the sticky bit as + * unwritable unless they are owned by the user. + */ +static bool +dir_is_writable(struct stat *sb, uid_t uid, gid_t gid, int ngroups, + GETGROUPS_T *groups) +{ + debug_decl(dir_is_writable, SUDO_DEBUG_EDIT) + + /* If the user owns the dir we always consider it writable. */ + if (sb->st_uid == uid) { + sudo_debug_printf(SUDO_DEBUG_INFO|SUDO_DEBUG_LINENO, + "user uid %u matches directory uid %u", (unsigned int)uid, + (unsigned int)sb->st_uid); + debug_return_bool(true); + } + + /* Other writable? */ + if (ISSET(sb->st_mode, S_IWOTH)) { + sudo_debug_printf(SUDO_DEBUG_INFO|SUDO_DEBUG_LINENO, + "directory is writable by other"); + debug_return_bool(true); + } + + /* Group writable? */ + if (ISSET(sb->st_mode, S_IWGRP)) { + if (group_matches(sb->st_gid, gid, ngroups, groups)) { + sudo_debug_printf(SUDO_DEBUG_INFO|SUDO_DEBUG_LINENO, + "directory is writable by one of the user's groups"); + debug_return_bool(true); + } + } + + debug_return_bool(false); +} + +static int +sudo_edit_open_nonwritable(char *path, int oflags, mode_t mode) +{ + char *base, *dir; + 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; + } else { + base = path; + dir = "."; + } +#ifdef O_PATH + if ((dfd = open(dir, O_PATH)) != -1) { + /* Linux kernels < 3.6 can't do fstat on O_PATH fds. */ + if (fstat(dfd, &sb) == -1) { + close(dfd); + dfd = open(dir, O_RDONLY); + if (fstat(dfd, &sb) == -1) { + close(dfd); + dfd = -1; + } + } + } +#else + if ((dfd = open(dir, O_RDONLY)) != -1) { + if (fstat(dfd, &sb) == -1) { + close(dfd); + dfd = -1; + } + } +#endif + if (base != path) + base[-1] = '/'; /* restore path */ + if (dfd == -1) + debug_return_int(-1); + + if (dir_is_writable(&sb, user_details.uid, user_details.gid, + user_details.ngroups, user_details.groups)) { + close(dfd); + errno = ENOTDIR; + debug_return_int(-1); + } + + fd = openat(dfd, path, oflags, mode); + close(dfd); + debug_return_int(fd); +} + #ifdef O_NOFOLLOW static int -sudo_edit_open(const char *path, int oflags, mode_t mode, int sflags) +sudo_edit_open(char *path, int oflags, mode_t mode, int sflags) { int fd; + debug_decl(sudo_edit_open, SUDO_DEBUG_EDIT) if (!ISSET(sflags, CD_SUDOEDIT_FOLLOW)) oflags |= O_NOFOLLOW; - fd = open(path, oflags|O_NONBLOCK, mode); + if (ISSET(sflags, CD_SUDOEDIT_CHECKDIR) && user_details.uid != 0) + fd = sudo_edit_open_nonwritable(path, oflags|O_NONBLOCK, mode); + else + fd = open(path, oflags|O_NONBLOCK, mode); if (fd != -1 && !ISSET(oflags, O_NONBLOCK)) (void) fcntl(fd, F_SETFL, fcntl(fd, F_GETFL, 0) & ~O_NONBLOCK); - return fd; + debug_return_int(fd); } #else static int -sudo_edit_open(const char *path, int oflags, mode_t mode, int sflags) +sudo_edit_open(char *path, int oflags, mode_t mode, int sflags) { struct stat sb1, sb2; int fd; + debug_decl(sudo_edit_open, SUDO_DEBUG_EDIT) - if ((fd = open(path, oflags|O_NONBLOCK, mode)) == -1) - return -1; + if (ISSET(sflags, CD_SUDOEDIT_CHECKDIR) && user_details.uid != 0) + fd = sudo_edit_open_nonwritable(path, oflags|O_NONBLOCK, mode); + else + fd = open(path, oflags|O_NONBLOCK, mode); + if (fd == -1) + debug_return_int(-1); if (!ISSET(oflags, O_NONBLOCK)) (void) fcntl(fd, F_SETFL, fcntl(fd, F_GETFL, 0) & ~O_NONBLOCK); @@ -186,7 +339,7 @@ sudo_edit_open(const char *path, int oflags, mode_t mode, int sflags) const int serrno = errno; close(fd); errno = serrno; - return -1; + debug_return_int(-1); } /* @@ -198,11 +351,11 @@ sudo_edit_open(const char *path, int oflags, mode_t mode, int sflags) sb1.st_dev != sb2.st_dev || sb1.st_ino != sb2.st_ino) { close(fd); errno = ELOOP; - return -1; + debug_return_int(-1); } } - return fd; + debug_return_int(fd); } #endif /* O_NOFOLLOW */ @@ -214,7 +367,7 @@ sudo_edit_open(const char *path, int oflags, mode_t mode, int sflags) */ static int sudo_edit_create_tfiles(struct command_details *command_details, - struct tempfile *tf, char * const files[], int nfiles) + struct tempfile *tf, char *files[], int nfiles) { int i, j, tfd, ofd, rc; char buf[BUFSIZ]; @@ -252,6 +405,9 @@ sudo_edit_create_tfiles(struct command_details *command_details, if (ofd == -1 && errno == ELOOP) { sudo_warnx(U_("%s: editing symbolic links is not permitted"), files[i]); + } else if (ofd == -1 && errno == ENOTDIR) { + sudo_warnx(U_("%s: editing files in a writable directory is not permitted"), + files[i]); } else { sudo_warn("%s", files[i]); } @@ -406,7 +562,7 @@ sudo_edit_copy_tfiles(struct command_details *command_details, #ifdef HAVE_SELINUX static int selinux_edit_create_tfiles(struct command_details *command_details, - struct tempfile *tf, char * const files[], int nfiles) + struct tempfile *tf, char *files[], int nfiles) { char **sesh_args, **sesh_ap; int i, rc, sesh_nargs; -- 2.40.0