diff -up ./src/copy_file.c.symbolic-link-attack-4 ./src/copy_file.c --- ./src/copy_file.c.symbolic-link-attack-4 2021-02-02 16:35:18.453036846 +0100 +++ ./src/copy_file.c 2021-02-02 16:38:09.430731749 +0100 @@ -23,6 +23,7 @@ #include +#include #include #include @@ -126,3 +127,35 @@ write_error: debug_return_int(-1); } } + +#ifdef HAVE_SELINUX +bool +sudo_check_temp_file(int tfd, const char *tfile, uid_t uid, struct stat *sb) +{ + struct stat sbuf; + debug_decl(sudo_check_temp_file, SUDO_DEBUG_UTIL); + + if (sb == NULL) + sb = &sbuf; + + if (fstat(tfd, sb) == -1) { + sudo_warn(U_("unable to stat %s"), tfile); + debug_return_bool(false); + } + if (!S_ISREG(sb->st_mode)) { + sudo_warnx(U_("%s: not a regular file"), tfile); + debug_return_bool(false); + } + if ((sb->st_mode & ALLPERMS) != (S_IRUSR|S_IWUSR)) { + sudo_warnx(U_("%s: bad file mode: 0%o"), tfile, + (unsigned int)(sb->st_mode & ALLPERMS)); + debug_return_bool(false); + } + if (sb->st_uid != uid) { + sudo_warnx(U_("%s is owned by uid %u, should be %u"), + tfile, (unsigned int)sb->st_uid, (unsigned int)uid); + debug_return_bool(false); + } + debug_return_bool(true); +} +#endif /* SELINUX */ diff -up ./src/sesh.c.symbolic-link-attack-4 ./src/sesh.c --- ./src/sesh.c.symbolic-link-attack-4 2021-02-02 16:35:18.450036887 +0100 +++ ./src/sesh.c 2021-02-02 16:38:52.907146897 +0100 @@ -134,7 +134,7 @@ main(int argc, char *argv[], char *envp[ static int sesh_sudoedit(int argc, char *argv[]) { - int i, oflags_dst, post, ret = SESH_ERR_FAILURE; + int i, oflags_src, oflags_dst, post, ret = SESH_ERR_FAILURE; int fd_src = -1, fd_dst = -1, follow = 0; ssize_t nread, nwritten; struct stat sb; @@ -178,10 +178,12 @@ sesh_sudoedit(int argc, char *argv[]) debug_return_int(SESH_ERR_BAD_PATHS); /* - * Use O_EXCL if we are not in the post editing stage - * so that it's ensured that the temporary files are - * created by us and that we are not opening any symlinks. + * In the pre-editing stage, use O_EXCL to ensure that the temporary + * files are created by us and that we are not opening any symlinks. + * 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); oflags_dst = O_WRONLY|O_CREAT|(post ? follow : O_EXCL); for (i = 0; i < argc - 1; i += 2) { const char *path_src = argv[i]; @@ -191,7 +193,7 @@ sesh_sudoedit(int argc, char *argv[]) * doesn't exist, that's OK, we'll create an empty * destination file. */ - if ((fd_src = open(path_src, O_RDONLY|follow, S_IRUSR|S_IWUSR)) < 0) { + if ((fd_src = open(path_src, oflags_src, S_IRUSR|S_IWUSR)) < 0) { if (errno != ENOENT) { sudo_warn("%s", path_src); if (post) { @@ -201,6 +203,14 @@ sesh_sudoedit(int argc, char *argv[]) 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)) { + ret = SESH_ERR_SOME_FILES; + goto nocleanup; + } + fcntl(fd_src, F_SETFL, fcntl(fd_src, F_GETFL, 0) & ~O_NONBLOCK); + } if ((fd_dst = open(path_dst, oflags_dst, post ? (S_IRUSR|S_IWUSR|S_IRGRP|S_IROTH) : (S_IRUSR|S_IWUSR))) < 0) { @@ -218,10 +228,7 @@ sesh_sudoedit(int argc, char *argv[]) off_t len_dst = -1; if (post) { - if (fstat(fd_src, &sb) != 0) { - ret = SESH_ERR_SOME_FILES; - goto nocleanup; - } + /* sudo_check_temp_file() filled in sb for us. */ len_src = sb.st_size; if (fstat(fd_dst, &sb) != 0) { ret = SESH_ERR_SOME_FILES; diff -up ./src/sudo_edit.c.symbolic-link-attack-4 ./src/sudo_edit.c --- ./src/sudo_edit.c.symbolic-link-attack-4 2021-02-02 16:35:18.452036860 +0100 +++ ./src/sudo_edit.c 2021-02-02 16:54:25.943429580 +0100 @@ -253,8 +253,10 @@ sudo_edit_mktemp(const char *ofile, char } else { len = asprintf(tfile, "%s/%s.XXXXXXXX", edit_tmpdir, cp); } - if (len == -1) - sudo_fatalx(U_("%s: %s"), __func__, U_("unable to allocate memory")); + if (len == -1) { + sudo_warnx(U_("%s: %s"), __func__, U_("unable to allocate memory")); + debug_return_int(-1); + } tfd = mkstemps(*tfile, suff ? strlen(suff) : 0); sudo_debug_printf(SUDO_DEBUG_INFO|SUDO_DEBUG_LINENO, "%s -> %s, fd %d", ofile, *tfile, tfd); @@ -757,7 +759,8 @@ bad: #ifdef HAVE_SELINUX static int -selinux_run_helper(char *argv[], char *envp[]) +selinux_run_helper(uid_t uid, gid_t gid, int ngroups, GETGROUPS_T *groups, + char *const argv[], char *const envp[]) { int status, ret = SESH_ERR_FAILURE; const char *sesh; @@ -777,8 +780,10 @@ selinux_run_helper(char *argv[], char *e break; case 0: /* child runs sesh in new context */ - if (selinux_setcon() == 0) + if (selinux_setcon() == 0) { + switch_user(uid, gid, ngroups, groups); execve(sesh, argv, envp); + } _exit(SESH_ERR_FAILURE); default: /* parent waits */ @@ -797,7 +802,7 @@ selinux_edit_create_tfiles(struct comman struct tempfile *tf, char *files[], int nfiles) { char **sesh_args, **sesh_ap; - int i, rc, sesh_nargs; + int i, rc, error, sesh_nargs, ret = -1; struct stat sb; debug_decl(selinux_edit_create_tfiles, SUDO_DEBUG_EDIT) @@ -809,7 +814,7 @@ selinux_edit_create_tfiles(struct comman 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"; @@ -817,7 +822,6 @@ selinux_edit_create_tfiles(struct comman *sesh_ap++ = "-h"; *sesh_ap++ = "0"; - /* XXX - temp files should be created with user's context */ for (i = 0; i < nfiles; i++) { char *tfile, *ofile = files[i]; int tfd; @@ -835,8 +839,7 @@ selinux_edit_create_tfiles(struct comman if (tfd == -1) { sudo_warn("mkstemps"); free(tfile); - free(sesh_args); - debug_return_int(-1); + goto done; } /* Helper will re-create temp file with proper security context. */ close(tfd); @@ -847,8 +850,10 @@ selinux_edit_create_tfiles(struct comman *sesh_ap = NULL; /* Run sesh -e [-h] 0 ... */ - rc = selinux_run_helper(sesh_args, command_details->envp); - switch (rc) { + error = selinux_run_helper(command_details->uid, command_details->gid, + command_details->ngroups, command_details->groups, sesh_args, + command_details->envp); + switch (error) { case SESH_SUCCESS: break; case SESH_ERR_BAD_PATHS: @@ -858,21 +863,34 @@ selinux_edit_create_tfiles(struct comman case SESH_ERR_KILLED: sudo_fatalx(U_("sesh: killed by a signal")); default: - sudo_fatalx(U_("sesh: unknown error %d"), rc); + sudo_fatalx(U_("sesh: unknown error %d"), error); + goto done; } - /* Chown to user's UID so they can edit the temporary files. */ for (i = 0; i < nfiles; i++) { - if (chown(tf[i].tfile, user_details.uid, user_details.gid) != 0) { - sudo_warn("unable to chown(%s) to %d:%d for editing", - tf[i].tfile, user_details.uid, user_details.gid); - } + int tfd = open(tf[i].tfile, O_RDONLY|O_NONBLOCK|O_NOFOLLOW); + if (tfd == -1) { + sudo_warn(U_("unable to open %s"), tf[i].tfile); + goto done; + } + if (!sudo_check_temp_file(tfd, tf[i].tfile, command_details->uid, NULL)) { + close(tfd); + goto done; + } + if (fchown(tfd, user_details.uid, user_details.gid) != 0) { + sudo_warn("unable to chown(%s) to %d:%d for editing", + tf[i].tfile, user_details.uid, user_details.gid); + close(tfd); + goto done; + } + close(tfd); } +done: /* Contents of tf will be freed by caller. */ free(sesh_args); - return (nfiles); + debug_return_int(ret); } static int @@ -880,7 +898,8 @@ selinux_edit_copy_tfiles(struct command_ struct tempfile *tf, int nfiles, struct timespec *times) { char **sesh_args, **sesh_ap; - int i, rc, sesh_nargs, ret = 1; + int i, rc, error, sesh_nargs, ret = 1; + int tfd = -1; struct timespec ts; struct stat sb; debug_decl(selinux_edit_copy_tfiles, SUDO_DEBUG_EDIT) @@ -901,33 +920,43 @@ selinux_edit_copy_tfiles(struct command_ /* Construct args for sesh -e 1 */ for (i = 0; i < nfiles; i++) { - if (stat(tf[i].tfile, &sb) == 0) { - mtim_get(&sb, ts); - if (tf[i].osize == sb.st_size && sudo_timespeccmp(&tf[i].omtim, &ts, ==)) { - /* - * If mtime and size match but the user spent no measurable - * time in the editor we can't tell if the file was changed. - */ - if (sudo_timespeccmp(×[0], ×[1], !=)) { - sudo_warnx(U_("%s unchanged"), tf[i].ofile); - unlink(tf[i].tfile); - continue; - } + if (tfd != -1) + close(tfd); + if ((tfd = open(tf[i].tfile, O_RDONLY|O_NONBLOCK|O_NOFOLLOW)) == -1) { + sudo_warn(U_("unable to open %s"), tf[i].tfile); + continue; + } + if (!sudo_check_temp_file(tfd, tf[i].tfile, user_details.uid, &sb)) + continue; + mtim_get(&sb, ts); + if (tf[i].osize == sb.st_size && sudo_timespeccmp(&tf[i].omtim, &ts, ==)) { + /* + * If mtime and size match but the user spent no measurable + * time in the editor we can't tell if the file was changed. + */ + if (sudo_timespeccmp(×[0], ×[1], !=)) { + sudo_warnx(U_("%s unchanged"), tf[i].ofile); + unlink(tf[i].tfile); + continue; } } *sesh_ap++ = tf[i].tfile; *sesh_ap++ = tf[i].ofile; - if (chown(tf[i].tfile, command_details->uid, command_details->gid) != 0) { + if (fchown(tfd, command_details->uid, command_details->gid) != 0) { sudo_warn("unable to chown(%s) back to %d:%d", tf[i].tfile, command_details->uid, command_details->gid); } } *sesh_ap = NULL; + if (tfd != -1) + close(tfd); if (sesh_ap - sesh_args > 3) { /* Run sesh -e 1 ... */ - rc = selinux_run_helper(sesh_args, command_details->envp); - switch (rc) { + error = selinux_run_helper(command_details->uid, command_details->gid, + command_details->ngroups, command_details->groups, sesh_args, + command_details->envp); + switch (error) { case SESH_SUCCESS: ret = 0; break; @@ -941,7 +970,7 @@ selinux_edit_copy_tfiles(struct command_ sudo_warnx(U_("sesh: killed by a signal")); break; default: - sudo_warnx(U_("sesh: unknown error %d"), rc); + sudo_warnx(U_("sesh: unknown error %d"), error); break; } if (ret != 0) @@ -963,7 +992,7 @@ sudo_edit(struct command_details *comman { struct command_details saved_command_details; char **nargv = NULL, **ap, **files = NULL; - int errors, i, ac, nargc, rc; + int errors, i, ac, nargc, ret; int editor_argc = 0, nfiles = 0; struct timespec times[2]; struct tempfile *tf = NULL; @@ -1058,7 +1087,7 @@ sudo_edit(struct command_details *comman command_details->ngroups = user_details.ngroups; command_details->groups = user_details.groups; command_details->argv = nargv; - rc = run_command(command_details); + ret = run_command(command_details); if (sudo_gettime_real(×[1]) == -1) { sudo_warn(U_("unable to read the clock")); goto cleanup; @@ -1080,14 +1109,16 @@ sudo_edit(struct command_details *comman else #endif errors = sudo_edit_copy_tfiles(command_details, tf, nfiles, times); - if (errors) - goto cleanup; + if (errors) { + /* Preserve the edited temporary files. */ + ret = W_EXITCODE(1, 0); + } for (i = 0; i < nfiles; i++) free(tf[i].tfile); free(tf); free(nargv); - debug_return_int(rc); + debug_return_int(ret); cleanup: /* Clean up temp files and return. */ diff -up ./src/sudo_exec.h.symbolic-link-attack-4 ./src/sudo_exec.h --- ./src/sudo_exec.h.symbolic-link-attack-4 2021-02-02 16:35:18.452036860 +0100 +++ ./src/sudo_exec.h 2021-02-02 16:35:18.454036833 +0100 @@ -1,7 +1,7 @@ /* * SPDX-License-Identifier: ISC * - * Copyright (c) 2010-2016 Todd C. Miller + * Copyright (c) 2010-2017, 2020-2021 Todd C. Miller * * Permission to use, copy, modify, and distribute this software for any * purpose with or without fee is hereby granted, provided that the above @@ -84,9 +84,11 @@ */ struct command_details; struct command_status; +struct stat; /* copy_file.c */ int sudo_copy_file(const char *src, int src_fd, off_t src_len, const char *dst, int dst_fd, off_t dst_len); +bool sudo_check_temp_file(int tfd, const char *tname, uid_t uid, struct stat *sb); /* exec.c */ void exec_cmnd(struct command_details *details, int errfd);