Add directory writability checks for SELinux RBAC sudoedit.

These were never added to the SELinux RBAC path.

src/Makefile.in

@@ -133,7 +133,7 @@ IOBJS = $(OBJS:.o=.i) sesh.i
 
 POBJS = $(IOBJS:.i=.plog)
 
-SESH_OBJS = copy_file.o exec_common.o sesh.o
+SESH_OBJS = copy_file.o edit_open.o exec_common.o sesh.o
 
 CHECK_NOEXEC_OBJS = check_noexec.o exec_common.o
 
@@ -659,16 +659,18 @@ selinux.i: $(srcdir)/selinux.c $(incdir)/compat/stdbool.h \
 selinux.plog: selinux.i
 	rm -f $@; pvs-studio --cfg $(PVS_CFG) --sourcetree-root $(top_srcdir) --skip-cl-exe yes --source-file $(srcdir)/selinux.c --i-file $< --output-file $@
 sesh.o: $(srcdir)/sesh.c $(incdir)/compat/stdbool.h $(incdir)/sudo_compat.h \
-        $(incdir)/sudo_conf.h $(incdir)/sudo_debug.h $(incdir)/sudo_fatal.h \
+        $(incdir)/sudo_conf.h $(incdir)/sudo_debug.h $(incdir)/sudo_event.h \
-        $(incdir)/sudo_gettext.h $(incdir)/sudo_plugin.h \
+        $(incdir)/sudo_fatal.h $(incdir)/sudo_gettext.h \
-        $(incdir)/sudo_queue.h $(incdir)/sudo_util.h $(srcdir)/sudo_edit.h \
+        $(incdir)/sudo_plugin.h $(incdir)/sudo_queue.h $(incdir)/sudo_util.h \
-        $(srcdir)/sudo_exec.h $(top_builddir)/config.h
+        $(srcdir)/sudo.h $(srcdir)/sudo_edit.h $(srcdir)/sudo_exec.h \
+        $(top_builddir)/config.h $(top_builddir)/pathnames.h
 	$(CC) -c $(CPPFLAGS) $(CFLAGS) $(ASAN_CFLAGS) $(PIE_CFLAGS) $(SSP_CFLAGS) $(srcdir)/sesh.c
 sesh.i: $(srcdir)/sesh.c $(incdir)/compat/stdbool.h $(incdir)/sudo_compat.h \
-        $(incdir)/sudo_conf.h $(incdir)/sudo_debug.h $(incdir)/sudo_fatal.h \
+        $(incdir)/sudo_conf.h $(incdir)/sudo_debug.h $(incdir)/sudo_event.h \
-        $(incdir)/sudo_gettext.h $(incdir)/sudo_plugin.h \
+        $(incdir)/sudo_fatal.h $(incdir)/sudo_gettext.h \
-        $(incdir)/sudo_queue.h $(incdir)/sudo_util.h $(srcdir)/sudo_edit.h \
+        $(incdir)/sudo_plugin.h $(incdir)/sudo_queue.h $(incdir)/sudo_util.h \
-        $(srcdir)/sudo_exec.h $(top_builddir)/config.h
+        $(srcdir)/sudo.h $(srcdir)/sudo_edit.h $(srcdir)/sudo_exec.h \
+        $(top_builddir)/config.h $(top_builddir)/pathnames.h
 	$(CC) -E -o $@ $(CPPFLAGS) $<
 sesh.plog: sesh.i
 	rm -f $@; pvs-studio --cfg $(PVS_CFG) --sourcetree-root $(top_srcdir) --skip-cl-exe yes --source-file $(srcdir)/sesh.c --i-file $< --output-file $@

src/edit_open.c

@@ -401,4 +401,57 @@ sudo_edit_open(char *path, int oflags, mode_t mode, struct user_details *ud,
 }
 #endif /* O_NOFOLLOW */
 
+/*
+ * Verify that the parent dir of a new file exists and is not writable
+ * by the user.  This fails early so the user knows ahead of time if the
+ * edit won't succeed.  Additional checks are performed when copying the
+ * temporary file back to the origin so there are no TOCTOU issues.
+ * Does not modify the value of errno.
+ */
+bool
+sudo_edit_parent_valid(char *path, struct user_details *ud,
+    struct command_details *cd)
+{
+    const int serrno = errno;
+    const int sflags = cd->flags;
+    struct stat sb;
+    bool ret = false;
+    char *slash;
+    int dfd;
+    debug_decl(sudo_edit_parent_valid, SUDO_DEBUG_EDIT);
+
+    /* Get dirname of path (the slash is restored later). */
+    slash = strrchr(path, '/');
+    if (slash == NULL) {
+	/* cwd */
+	path = ".";
+    } else if (slash == path) {
+	path = "/";
+	slash = NULL;
+    } else {
+	*slash = '\0';
+    }
+
+    /*
+     * The parent directory is allowed to be a symbolic link unless
+     * *its* parent is writable and CD_SUDOEDIT_CHECK is set.
+     */
+    SET(cd->flags, CD_SUDOEDIT_FOLLOW);
+    dfd = sudo_edit_open(path, DIR_OPEN_FLAGS, S_IRUSR|S_IWUSR|S_IRGRP|S_IROTH,
+	ud, cd);
+    cd->flags = sflags;
+    if (dfd != -1) {
+	if (fstat(dfd, &sb) == 0 && S_ISDIR(sb.st_mode))
+	    ret = true;
+	close(dfd);
+    }
+    if (slash != NULL)
+	*slash = '/';
+
+    /* Restore errno. */
+    errno = serrno;
+
+    debug_return_bool(ret);
+}
+
 #endif /* HAVE_SETRESUID || HAVE_SETREUID || HAVE_SETEUID */

src/sesh.c

@@ -39,15 +39,9 @@
 # include "compat/stdbool.h"
 #endif /* HAVE_STDBOOL_H */
 
-#include "sudo_compat.h"
+#include "sudo.h"
-#include "sudo_conf.h"
-#include "sudo_debug.h"
-#include "sudo_edit.h"
 #include "sudo_exec.h"
-#include "sudo_fatal.h"
+#include "sudo_edit.h"
-#include "sudo_gettext.h"
-#include "sudo_plugin.h"
-#include "sudo_util.h"
 
 sudo_dso_public int main(int argc, char *argv[], char *envp[]);
 
@@ -130,20 +124,85 @@ main(int argc, char *argv[], char *envp[])
     _exit(ret);
 }
 
+/*
+ * Destructively parse a string in the format:
+ *  uid:gid:groups,...
+ *
+ * On success, fills in ud and returns true, else false.
+ */
+static bool
+parse_user(char *userstr, struct user_details *ud)
+{
+    char *cp, *ep;
+    const char *errstr;
+    debug_decl(parse_user, SUDO_DEBUG_EDIT);
+
+    /* UID */
+    cp = userstr;
+    if ((ep = strchr(cp, ':')) == NULL) {
+	sudo_warnx(U_("%s: %s"), cp, U_("invalid value"));
+	debug_return_bool(false);
+    }
+    *ep++ = '\0';
+    ud->uid = sudo_strtoid(cp, &errstr);
+    if (errstr != NULL) {
+	sudo_warnx(U_("%s: %s"), cp, errstr);
+	debug_return_bool(false);
+    }
+
+    /* GID */
+    cp = ep;
+    if ((ep = strchr(cp, ':')) == NULL) {
+	sudo_warnx(U_("%s: %s"), cp, U_("invalid value"));
+	debug_return_bool(false);
+    }
+    *ep++ = '\0';
+    ud->gid = sudo_strtoid(cp, &errstr);
+    if (errstr != NULL) {
+	sudo_warnx(U_("%s: %s"), cp, errstr);
+	debug_return_bool(false);
+    }
+
+    /* group vector */
+    cp = ep;
+    ud->ngroups = sudo_parse_gids(cp, NULL, &ud->groups);
+    if (ud->ngroups == -1)
+	debug_return_bool(false);
+
+    debug_return_bool(true);
+}
+
 static int
 sesh_sudoedit(int argc, char *argv[])
 {
-    int i, oflags_src, oflags_dst, post, ret = SESH_ERR_FAILURE;
+    int i, post, ret = SESH_ERR_FAILURE;
-    int fd_src = -1, fd_dst = -1, follow = 0;
+    int fd_src = -1, fd_dst = -1;
-    struct stat sb;
+    struct command_details command_details;
+    struct user_details edit_user;
     struct timespec times[2];
+    struct stat sb;
     debug_decl(sesh_sudoedit, SUDO_DEBUG_EDIT);
 
+    memset(&edit_user, 0, sizeof(edit_user));
+    memset(&command_details, 0, sizeof(command_details));
+    command_details.flags = CD_SUDOEDIT_FOLLOW;
+
     /* Check for -h flag (don't follow links). */
-    if (strcmp(argv[2], "-h") == 0) {
+    if (argv[2] != NULL && strcmp(argv[2], "-h") == 0) {
 	argv++;
 	argc--;
-	follow = O_NOFOLLOW;
+	CLR(command_details.flags, CD_SUDOEDIT_FOLLOW);
+    }
+
+    /* Check for -w flag (disallow directories writable by the user). */
+    if (argv[2] != NULL && strcmp(argv[2], "-w") == 0) {
+	SET(command_details.flags, CD_SUDOEDIT_CHECKDIR);
+
+	/* Parse uid:gid:gid1,gid2,... */
+	if (argv[3] == NULL || !parse_user(argv[3], &edit_user))
+	    debug_return_int(SESH_ERR_FAILURE);
+	argv += 2;
+	argc -= 2;
     }
 
     if (argc < 3)
@@ -175,42 +234,87 @@ sesh_sudoedit(int argc, char *argv[])
 	debug_return_int(SESH_ERR_BAD_PATHS);
 
     /*
-     * In the pre-editing stage, use O_EXCL to ensure that the temporary
+     * sudoedit runs us with the effective user-ID and group-ID of
-     * files are created by us and that we are not opening any symlinks.
+     * the target user as well as with the target user's group list.
-     * In the post-editing stage, use O_NOFOLLOW so we don't follow symlinks
-     * when opening the temporary files.
      */
-    oflags_src = O_RDONLY|(post ? O_NONBLOCK|O_NOFOLLOW : follow);
+    command_details.uid = command_details.euid = geteuid();
-    oflags_dst = O_WRONLY|O_CREAT|(post ? follow : O_EXCL);
+    command_details.gid = command_details.egid = getegid();
+    command_details.ngroups = getgroups(0, NULL);
+    if (command_details.ngroups > 0) {
+	command_details.groups = reallocarray(NULL, command_details.ngroups,
+	    sizeof(GETGROUPS_T));
+	if (command_details.groups == NULL) {
+	    sudo_warnx(U_("%s: %s"), __func__,
+		U_("unable to allocate memory"));
+	    debug_return_int(SESH_ERR_FAILURE);
+	}
+	if (getgroups(command_details.ngroups, command_details.groups) < 0) {
+	    sudo_warn("%s", U_("unable to get group list"));
+	    debug_return_int(SESH_ERR_FAILURE);
+	}
+    }
+
     for (i = 0; i < argc - 1; i += 2) {
-	const char *path_src = argv[i];
+	char *path_src = argv[i];
-	const char *path_dst = argv[i + 1];
+	char *path_dst = argv[i + 1];
 	/*
-	 * Try to open the source file for reading. If it
+	 * Try to open the source (original or temporary) file for reading.
-	 * doesn't exist, that's OK, we'll create an empty
+	 * If it doesn't exist, we'll create an empty destination file.
-	 * destination file.
 	 */
-	if ((fd_src = open(path_src, oflags_src, S_IRUSR|S_IWUSR)) < 0) {
+	if (post) {
-	    if (errno != ENOENT) {
+	    fd_src = open(path_src, O_RDONLY|O_NONBLOCK|O_NOFOLLOW);
-		sudo_warn("%s", path_src);
+	} else {
+	    fd_src = sudo_edit_open(path_src, post ? O_RDONLY|O_NOFOLLOW : O_RDONLY,
+		S_IRUSR|S_IWUSR|S_IRGRP|S_IROTH, &edit_user, &command_details);
+	}
+	if (fd_src == -1) {
+	    if (post || errno != ENOENT) {
+		if (errno == ELOOP) {
+		    sudo_warnx(U_("%s: editing symbolic links is not "
+			"permitted"), path_src);
+		} else if (errno == EISDIR) {
+		    sudo_warnx(U_("%s: editing files in a writable directory "
+			"is not permitted"), path_src);
+		} else {
+		    sudo_warn("%s", path_src);
+		}
 		if (post) {
 		    ret = SESH_ERR_SOME_FILES;
 		    goto nocleanup;
 		} else
 		    goto cleanup_0;
 	    }
+	    /* New file, verify parent dir exists and is not writable. */
+	    if (!sudo_edit_parent_valid(path_src, &edit_user, &command_details))
+		goto cleanup_0;
 	}
 	if (post) {
 	    /* Make sure the temporary file is safe and has the proper owner. */
-	    if (!sudo_check_temp_file(fd_src, path_src, geteuid(), &sb)) {
+	    if (!sudo_check_temp_file(fd_src, path_src, command_details.uid, &sb)) {
 		ret = SESH_ERR_SOME_FILES;
 		goto nocleanup;
 	    }
-	    fcntl(fd_src, F_SETFL, fcntl(fd_src, F_GETFL, 0) & ~O_NONBLOCK);
+	    (void) fcntl(fd_src, F_SETFL, fcntl(fd_src, F_GETFL, 0) & ~O_NONBLOCK);
-	}
 
-	if ((fd_dst = open(path_dst, oflags_dst, post ?
+	    /* Create destination file. */
-	    (S_IRUSR|S_IWUSR|S_IRGRP|S_IROTH) : (S_IRUSR|S_IWUSR))) < 0) {
+	    fd_dst = sudo_edit_open(path_dst, O_WRONLY|O_CREAT,
+		S_IRUSR|S_IWUSR|S_IRGRP|S_IROTH, &edit_user, &command_details);
+	} else {
+	    if (fd_src == -1) {
+		/* New file. */
+		memset(&sb, 0, sizeof(sb));
+	    } else if (fstat(fd_src, &sb) == -1 || !S_ISREG(sb.st_mode)) {
+		sudo_warnx(U_("%s: not a regular file"), path_src);
+		goto cleanup_0;
+	    }
+
+	    /*
+	     * Create temporary file using O_EXCL to ensure that temporary
+	     * files are created by us and that we do not open any symlinks.
+	     */
+	    fd_dst = open(path_dst, O_WRONLY|O_CREAT|O_EXCL, S_IRUSR|S_IWUSR);
+	}
+	if (fd_dst == -1) {
 	    /* error - cleanup */
 	    sudo_warn("%s", path_dst);
 	    if (post) {
@@ -246,9 +350,7 @@ sesh_sudoedit(int argc, char *argv[])
 	}
 
 	if (!post) {
-	    if (fd_src == -1 || fstat(fd_src, &sb) != 0)
+	    /* Make mtime on temp file match src (sb filled in above). */
-		memset(&sb, 0, sizeof(sb));
-	    /* Make mtime on temp file match src. */
 	    mtim_get(&sb, times[0]);
 	    times[1].tv_sec = times[0].tv_sec;
 	    times[1].tv_nsec = times[0].tv_nsec;
@@ -276,7 +378,7 @@ nocleanup:
 	close(fd_dst);
     if (fd_src != -1)
 	close(fd_src);
-    return(ret);
+    debug_return_int(ret);
 cleanup_0:
     /* Remove temporary files (post=0) */
     for (i = 0; i < argc - 1; i += 2)
@@ -285,5 +387,5 @@ cleanup_0:
 	close(fd_dst);
     if (fd_src != -1)
 	close(fd_src);
-    return(SESH_ERR_NO_FILES);
+    debug_return_int(SESH_ERR_NO_FILES);
 }

src/sudo_edit.c

@@ -155,44 +155,13 @@ sudo_edit_create_tfiles(struct command_details *command_details,
 	ofd = sudo_edit_open(files[i], O_RDONLY,
 	    S_IRUSR|S_IWUSR|S_IRGRP|S_IROTH, &user_details, command_details);
 	if (ofd != -1 || errno == ENOENT) {
-	    if (ofd == -1) {
+	    if (ofd != -1) {
-		/*
-		 * New file, verify parent dir exists unless in cwd.
-		 * This fails early so the user knows ahead of time if the
-		 * edit won't succeed.  Additional checks are performed
-		 * when copying the temporary file back to the origin.
-		 */
-		char *slash = strrchr(files[i], '/');
-		if (slash != NULL && slash != files[i]) {
-		    const int sflags = command_details->flags;
-		    const int serrno = errno;
-		    int dfd;
-
-		    /*
-		     * The parent directory is allowed to be a symbolic
-		     * link as long as *its* parent is not writable.
-		     */
-		    *slash = '\0';
-		    SET(command_details->flags, CD_SUDOEDIT_FOLLOW);
-		    dfd = sudo_edit_open(files[i], DIR_OPEN_FLAGS,
-			S_IRUSR|S_IWUSR|S_IRGRP|S_IROTH, &user_details,
-			command_details);
-		    command_details->flags = sflags;
-		    if (dfd != -1) {
-			if (fstat(dfd, &sb) == 0 && S_ISDIR(sb.st_mode)) {
-			    memset(&sb, 0, sizeof(sb));
-			    rc = 0;
-			}
-			close(dfd);
-		    }
-		    *slash = '/';
-		    errno = serrno;
-		} else {
-		    memset(&sb, 0, sizeof(sb));
-		    rc = 0;
-		}
-	    } else {
 		rc = fstat(ofd, &sb);
+	    } else {
+		/* New file, verify parent dir exists and is not writable. */
+		memset(&sb, 0, sizeof(sb));
+		if (sudo_edit_parent_valid(files[i], &user_details, command_details))
+		    rc = 0;
 	    }
 	}
 	switch_user(ROOT_UID, user_details.egid,
@@ -382,11 +351,42 @@ selinux_run_helper(uid_t uid, gid_t gid, int ngroups, GETGROUPS_T *groups,
     debug_return_int(ret);
 }
 
+static char *
+selinux_fmt_sudo_user(void)
+{
+    char *cp, *user_str;
+    size_t user_size;
+    int i, len;
+    debug_decl(selinux_fmt_sudo_user, SUDO_DEBUG_EDIT);
+
+    user_size = (MAX_UID_T_LEN + 1) * (2 + user_details.ngroups);
+    if ((user_str = malloc(user_size)) == NULL)
+	debug_return_ptr(NULL);
+
+    /* UID:GID: */
+    len = snprintf(user_str, user_size, "%u:%u:",
+	(unsigned int)user_details.uid, (unsigned int)user_details.gid);
+    if (len < 0 || (size_t)len >= user_size)
+	sudo_fatalx(U_("internal error, %s overflow"), __func__);
+
+    /* Supplementary GIDs */
+    cp = user_str + len;
+    for (i = 0; i < user_details.ngroups; i++) {
+	len = snprintf(cp, user_size - (cp - user_str), "%s%u",
+	    i ? "," : "", (unsigned int)user_details.groups[i]);
+	if (len < 0 || (size_t)len >= user_size - (cp - user_str))
+	    sudo_fatalx(U_("internal error, %s overflow"), __func__);
+	cp += len;
+    }
+
+    debug_return_ptr(user_str);
+}
+
 static int
 selinux_edit_create_tfiles(struct command_details *command_details,
     struct tempfile *tf, char *files[], int nfiles)
 {
-    char **sesh_args, **sesh_ap;
+    char **sesh_args, **sesh_ap, *user_str = NULL;
     int i, error, sesh_nargs, ret = -1;
     struct stat sb;
     debug_decl(selinux_edit_create_tfiles, SUDO_DEBUG_EDIT);
@@ -395,7 +395,7 @@ selinux_edit_create_tfiles(struct command_details *command_details,
 	debug_return_int(0);
 
     /* Construct common args for sesh */
-    sesh_nargs = 4 + (nfiles * 2) + 1;
+    sesh_nargs = 6 + (nfiles * 2) + 1;
     sesh_args = sesh_ap = reallocarray(NULL, sesh_nargs, sizeof(char *));
     if (sesh_args == NULL) {
 	sudo_warnx(U_("%s: %s"), __func__, U_("unable to allocate memory"));
@@ -405,6 +405,14 @@ selinux_edit_create_tfiles(struct command_details *command_details,
     *sesh_ap++ = "-e";
     if (!ISSET(command_details->flags, CD_SUDOEDIT_FOLLOW))
 	*sesh_ap++ = "-h";
+    if (ISSET(command_details->flags, CD_SUDOEDIT_CHECKDIR)) {
+	if ((user_str = selinux_fmt_sudo_user()) == NULL) {
+	    sudo_warnx(U_("%s: %s"), __func__, U_("unable to allocate memory"));
+	    goto done;
+	}
+	*sesh_ap++ = "-w";
+	*sesh_ap++ = user_str;
+    }
     *sesh_ap++ = "0";
 
     for (i = 0; i < nfiles; i++) {
@@ -475,6 +483,7 @@ selinux_edit_create_tfiles(struct command_details *command_details,
 done:
     /* Contents of tf will be freed by caller. */
     free(sesh_args);
+    free(user_str);
 
     debug_return_int(ret);
 }
@@ -483,7 +492,7 @@ static int
 selinux_edit_copy_tfiles(struct command_details *command_details,
     struct tempfile *tf, int nfiles, struct timespec *times)
 {
-    char **sesh_args, **sesh_ap;
+    char **sesh_args, **sesh_ap, *user_str = NULL;
     int i, error, sesh_nargs, ret = 1;
     int tfd = -1;
     struct timespec ts;
@@ -494,14 +503,22 @@ selinux_edit_copy_tfiles(struct command_details *command_details,
 	debug_return_int(0);
 
     /* Construct common args for sesh */
-    sesh_nargs = 3 + (nfiles * 2) + 1;
+    sesh_nargs = 5 + (nfiles * 2) + 1;
     sesh_args = sesh_ap = reallocarray(NULL, sesh_nargs, sizeof(char *));
     if (sesh_args == NULL) {
 	sudo_warnx(U_("%s: %s"), __func__, U_("unable to allocate memory"));
-	debug_return_int(-1);
+	goto done;
     }
     *sesh_ap++ = "sesh";
     *sesh_ap++ = "-e";
+    if (ISSET(command_details->flags, CD_SUDOEDIT_CHECKDIR)) {
+	if ((user_str = selinux_fmt_sudo_user()) == NULL) {
+	    sudo_warnx(U_("%s: %s"), __func__, U_("unable to allocate memory"));
+	    goto done;
+	}
+	*sesh_ap++ = "-w";
+	*sesh_ap++ = user_str;
+    }
     *sesh_ap++ = "1";
 
     /* Construct args for sesh -e 1 */
@@ -564,7 +581,11 @@ selinux_edit_copy_tfiles(struct command_details *command_details,
 	if (ret != 0)
 	    sudo_warnx(U_("contents of edit session left in %s"), edit_tmpdir);
     }
+
+done:
+    /* Contents of tf will be freed by caller. */
     free(sesh_args);
+    free(user_str);
 
     debug_return_int(ret);
 }

src/sudo_edit.h

@@ -49,5 +49,6 @@ bool sudo_check_temp_file(int tfd, const char *tname, uid_t uid, struct stat *sb
 void switch_user(uid_t euid, gid_t egid, int ngroups, GETGROUPS_T *groups);
 int sudo_edit_open(char *path, int oflags, mode_t mode, struct user_details *ud, struct command_details *cd);
 int dir_is_writable(int dfd, struct user_details *ud, struct command_details *cd);
+bool sudo_edit_parent_valid(char *path, struct user_details *ud, struct command_details *cd);
 
 #endif /* SUDO_EDIT_H */