On Fri, 2015-12-25 at 04:35 +0000, Ben Hutchings wrote: > On Fri, 2015-12-25 at 03:09 +0000, Ben Hutchings wrote: > > On Fri, 2015-12-25 at 02:53 +0000, Ben Hutchings wrote: > > > Control: reopen -1 > > > > > > On Thu, 24 Dec 2015 05:19:31 +0000 Bdale Garbee <bd...@gag.com> wrote: > > > > Source: sudo > > > > Source-Version: 1.8.15-1 > > > > > > > > We believe that the bug you reported is fixed in the latest version of > > > > sudo, which is due to be installed in the Debian FTP archive. > > > [...] > > > > > > As Raphael already explained, the upstream change doesn't fix this. > > > > It *does* add a new configuration option, sudoedit_checkdir, which if > > enabled will defeat this attack. However, the upstream default is that > > it's disabled. Perhaps this should be changed in the Debian package? > > Actually, that option doesn't work either.
Attaching my fixes for sudoedit_checkdir. I intend to NMU with these changes later this week if I don't hear any objections. Ben. -- Ben Hutchings If God had intended Man to program, we'd have been born with serial I/O ports.
Description: CVE-2015-5602: Fix directory writability checks for sudoedit. Bug: https://bugzilla.sudo.ws/show_bug.cgi?id=707 Bug-Debian: https://bugs.debian.org/804149 Author: Ben Hutchings <b...@decadent.org.uk> This prevents access to a file in a writable directory *or* accessed through a symlink in a writable directory. --- a/src/sudo_edit.c +++ b/src/sudo_edit.c @@ -248,46 +248,52 @@ dir_is_writable(struct stat *sb, uid_t u static int sudo_edit_open_nonwritable(char *path, int oflags, mode_t mode) { - char *base, *dir; +#ifndef O_NOFOLLOW + error(1, "%s: Can't check writability without O_NOFOLLOW", __func__); +#else + char *sep; struct stat sb; - int dfd, fd; + int dfd, subdfd, fd; + int is_writable; debug_decl(sudo_edit_open_nonwritable, SUDO_DEBUG_EDIT) - base = strrchr(path, '/'); - if (base != NULL) { - *base++ = '\0'; - dir = path; + if (path[0] == '/') { + dfd = open("/", O_RDONLY); + ++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; - } - } + /* XXX It doesn't make any sense to allow editing relative paths */ + dfd = open(".", O_RDONLY); } -#else - if ((dfd = open(dir, O_RDONLY)) != -1) { + if (dfd == -1) + debug_return_int(-1); + + for (;;) { + /* + * Look up one component at a time, avoiding symbolic links in + * writable directories + */ + if (fstat(dfd, &sb) == -1) { close(dfd); - dfd = -1; + debug_return_int(-1); } + is_writable = dir_is_writable(&sb, user_details.uid, user_details.gid, + user_details.ngroups, user_details.groups); + + sep = strchr(path, '/'); + if (sep == NULL) + break; + *sep = '\0'; + subdfd = openat(dfd, path, O_RDONLY | (is_writable ? O_NOFOLLOW : 0), 0); + *sep = '/'; /* restore path */ + close(dfd); + if (subdfd == -1) + debug_return_int(-1); + path = sep + 1; + dfd = subdfd; } -#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)) { + if (is_writable) { close(dfd); errno = EISDIR; debug_return_int(-1); @@ -296,6 +302,7 @@ sudo_edit_open_nonwritable(char *path, i fd = openat(dfd, path, oflags, mode); close(dfd); debug_return_int(fd); +#endif } #ifdef O_NOFOLLOW
Description: CVE-2015-5602: Enable sudoedit directory writability checks by default Bug: https://bugzilla.sudo.ws/show_bug.cgi?id=707 Bug-Debian: https://bugs.debian.org/804149 Author: Ben Hutchings <b...@decadent.org.uk> --- a/plugins/sudoers/defaults.c +++ b/plugins/sudoers/defaults.c @@ -504,6 +504,7 @@ init_defaults(void) goto oom; def_set_utmp = true; def_pam_setcred = true; + def_sudoedit_checkdir = true; /* Finally do the lists (currently just environment tables). */ if (!init_envtables()) --- a/doc/sudoers.cat +++ b/doc/sudoers.cat @@ -1280,7 +1280,7 @@ SSUUDDOOEERRSS OOPPTTIIOONN 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. + _o_n 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 --- a/doc/sudoers.man.in +++ b/doc/sudoers.man.in @@ -2720,7 +2720,7 @@ by the invoking user unless it is run by 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 +\fIon\fR by default. .TP 18n sudoedit_follow --- a/doc/sudoers.mdoc.in +++ b/doc/sudoers.mdoc.in @@ -2554,7 +2554,7 @@ by the invoking user unless it is run by 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 +.Em on by default. .It sudoedit_follow By default,
signature.asc
Description: This is a digitally signed message part
