isa/sudo
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

Add security checks before using temp files for SELinux RBAC sudoedit.

Browse Source 
Otherwise, it may be possible for the user running sudoedit to
replace the newly-created temporary files with a symbolic link and
have sudoedit set the owner of an arbitrary file.
Problem reported by Matthias Gerstner of SUSE.
This commit is contained in:
Todd C. Miller
2021-01-06 10:16:00 -07:00
parent db1f27c035
commit 7cd36222e7
4 changed files with 121 additions and 49 deletions
Download Patch File Download Diff File Expand all files Collapse all files

35
src/copy_file.c
View File

@@ -1,7 +1,7 @@
/* /*
* SPDX-License-Identifier: ISC * SPDX-License-Identifier: ISC
* *
* Copyright (c) 2020 Todd C. Miller <Todd.Miller@sudo.ws> * Copyright (c) 2020-2021 Todd C. Miller <Todd.Miller@sudo.ws>
* *
* Permission to use, copy, modify, and distribute this software for any * Permission to use, copy, modify, and distribute this software for any
* purpose with or without fee is hereby granted, provided that the above * purpose with or without fee is hereby granted, provided that the above
@@ -23,6 +23,8 @@
#include <config.h> #include <config.h>
#include <sys/stat.h>
#include <stdlib.h> #include <stdlib.h>
#include <unistd.h> #include <unistd.h>
#include <errno.h> #include <errno.h>
@@ -134,3 +136,34 @@ write_error:
sudo_warn(U_("unable to write to %s"), dst); sudo_warn(U_("unable to write to %s"), dst);
debug_return_int(-1); 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, 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 */

27
src/sesh.c
View File

@@ -1,7 +1,7 @@
/* /*
* SPDX-License-Identifier: ISC * SPDX-License-Identifier: ISC
* *
* Copyright (c) 2008, 2010-2018, 2020 Todd C. Miller <Todd.Miller@sudo.ws> * Copyright (c) 2008, 2010-2018, 2020-2021 Todd C. Miller <Todd.Miller@sudo.ws>
* *
* Permission to use, copy, modify, and distribute this software for any * Permission to use, copy, modify, and distribute this software for any
* purpose with or without fee is hereby granted, provided that the above * purpose with or without fee is hereby granted, provided that the above
@@ -132,7 +132,7 @@ main(int argc, char *argv[], char *envp[])
static int static int
sesh_sudoedit(int argc, char *argv[]) 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; int fd_src = -1, fd_dst = -1, follow = 0;
struct stat sb; struct stat sb;
struct timespec times[2]; struct timespec times[2];
@@ -174,10 +174,12 @@ sesh_sudoedit(int argc, char *argv[])
debug_return_int(SESH_ERR_BAD_PATHS); debug_return_int(SESH_ERR_BAD_PATHS);
/* /*
* Use O_EXCL if we are not in the post editing stage * In the pre-editing stage, use O_EXCL to ensure that the temporary
* so that it's ensured that the temporary files are * files are created by us and that we are not opening any symlinks.
* 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); oflags_dst = O_WRONLY|O_CREAT|(post ? follow : O_EXCL);
for (i = 0; i < argc - 1; i += 2) { for (i = 0; i < argc - 1; i += 2) {
const char *path_src = argv[i]; const char *path_src = argv[i];
@@ -187,7 +189,7 @@ sesh_sudoedit(int argc, char *argv[])
* doesn't exist, that's OK, we'll create an empty * doesn't exist, that's OK, we'll create an empty
* destination file. * 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) { if (errno != ENOENT) {
sudo_warn("%s", path_src); sudo_warn("%s", path_src);
if (post) { if (post) {
@@ -197,6 +199,14 @@ sesh_sudoedit(int argc, char *argv[])
goto cleanup_0; 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 ? if ((fd_dst = open(path_dst, oflags_dst, post ?
(S_IRUSR|S_IWUSR|S_IRGRP|S_IROTH) : (S_IRUSR|S_IWUSR))) < 0) { (S_IRUSR|S_IWUSR|S_IRGRP|S_IROTH) : (S_IRUSR|S_IWUSR))) < 0) {
@@ -214,10 +224,7 @@ sesh_sudoedit(int argc, char *argv[])
off_t len_dst = -1; off_t len_dst = -1;
if (post) { if (post) {
if (fstat(fd_src, &sb) != 0) { /* sudo_check_temp_file() filled in sb for us. */
ret = SESH_ERR_SOME_FILES;
goto nocleanup;
}
len_src = sb.st_size; len_src = sb.st_size;
if (fstat(fd_dst, &sb) != 0) { if (fstat(fd_dst, &sb) != 0) {
ret = SESH_ERR_SOME_FILES; ret = SESH_ERR_SOME_FILES;

104
src/sudo_edit.c
View File

@@ -1,7 +1,7 @@
/* /*
* SPDX-License-Identifier: ISC * SPDX-License-Identifier: ISC
* *
* Copyright (c) 2004-2008, 2010-2020 Todd C. Miller <Todd.Miller@sudo.ws> * Copyright (c) 2004-2008, 2010-2021 Todd C. Miller <Todd.Miller@sudo.ws>
* *
* Permission to use, copy, modify, and distribute this software for any * Permission to use, copy, modify, and distribute this software for any
* purpose with or without fee is hereby granted, provided that the above * purpose with or without fee is hereby granted, provided that the above
@@ -259,8 +259,10 @@ sudo_edit_mktemp(const char *ofile, char **tfile)
} else { } else {
len = asprintf(tfile, "%s/%s.XXXXXXXX", edit_tmpdir, cp); len = asprintf(tfile, "%s/%s.XXXXXXXX", edit_tmpdir, cp);
} }
if (len == -1) if (len == -1) {
sudo_fatalx(U_("%s: %s"), __func__, U_("unable to allocate memory")); sudo_warnx(U_("%s: %s"), __func__, U_("unable to allocate memory"));
debug_return_int(-1);
}
tfd = mkstemps(*tfile, suff ? strlen(suff) : 0); tfd = mkstemps(*tfile, suff ? strlen(suff) : 0);
sudo_debug_printf(SUDO_DEBUG_INFO|SUDO_DEBUG_LINENO, sudo_debug_printf(SUDO_DEBUG_INFO|SUDO_DEBUG_LINENO,
"%s -> %s, fd %d", ofile, *tfile, tfd); "%s -> %s, fd %d", ofile, *tfile, tfd);
@@ -735,7 +737,8 @@ bad:
#ifdef HAVE_SELINUX #ifdef HAVE_SELINUX
static int 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; int status, ret = SESH_ERR_FAILURE;
const char *sesh; const char *sesh;
@@ -755,8 +758,10 @@ selinux_run_helper(char *argv[], char *envp[])
break; break;
case 0: case 0:
/* child runs sesh in new context */ /* child runs sesh in new context */
if (selinux_setcon() == 0) if (selinux_setcon() == 0) {
switch_user(uid, gid, ngroups, groups);
execve(sesh, argv, envp); execve(sesh, argv, envp);
}
_exit(SESH_ERR_FAILURE); _exit(SESH_ERR_FAILURE);
default: default:
/* parent waits */ /* parent waits */
@@ -775,7 +780,7 @@ selinux_edit_create_tfiles(struct command_details *command_details,
struct tempfile *tf, char *files[], int nfiles) struct tempfile *tf, char *files[], int nfiles)
{ {
char **sesh_args, **sesh_ap; char **sesh_args, **sesh_ap;
int i, rc, sesh_nargs; int i, error, sesh_nargs, ret = -1;
struct stat sb; struct stat sb;
debug_decl(selinux_edit_create_tfiles, SUDO_DEBUG_EDIT); debug_decl(selinux_edit_create_tfiles, SUDO_DEBUG_EDIT);
@@ -787,7 +792,7 @@ selinux_edit_create_tfiles(struct command_details *command_details,
sesh_args = sesh_ap = reallocarray(NULL, sesh_nargs, sizeof(char *)); sesh_args = sesh_ap = reallocarray(NULL, sesh_nargs, sizeof(char *));
if (sesh_args == NULL) { if (sesh_args == NULL) {
sudo_warnx(U_("%s: %s"), __func__, U_("unable to allocate memory")); sudo_warnx(U_("%s: %s"), __func__, U_("unable to allocate memory"));
debug_return_int(-1); goto done;
} }
*sesh_ap++ = "sesh"; *sesh_ap++ = "sesh";
*sesh_ap++ = "-e"; *sesh_ap++ = "-e";
@@ -795,7 +800,6 @@ selinux_edit_create_tfiles(struct command_details *command_details,
*sesh_ap++ = "-h"; *sesh_ap++ = "-h";
*sesh_ap++ = "0"; *sesh_ap++ = "0";
/* XXX - temp files should be created with user's context */
for (i = 0; i < nfiles; i++) { for (i = 0; i < nfiles; i++) {
char *tfile, *ofile = files[i]; char *tfile, *ofile = files[i];
int tfd; int tfd;
@@ -813,8 +817,7 @@ selinux_edit_create_tfiles(struct command_details *command_details,
if (tfd == -1) { if (tfd == -1) {
sudo_warn("mkstemps"); sudo_warn("mkstemps");
free(tfile); free(tfile);
free(sesh_args); goto done;
debug_return_int(-1);
} }
/* Helper will re-create temp file with proper security context. */ /* Helper will re-create temp file with proper security context. */
close(tfd); close(tfd);
@@ -825,8 +828,10 @@ selinux_edit_create_tfiles(struct command_details *command_details,
*sesh_ap = NULL; *sesh_ap = NULL;
/* Run sesh -e [-h] 0 <o1> <t1> ... <on> <tn> */ /* Run sesh -e [-h] 0 <o1> <t1> ... <on> <tn> */
rc = selinux_run_helper(sesh_args, command_details->envp); error = selinux_run_helper(command_details->uid, command_details->gid,
switch (rc) { command_details->ngroups, command_details->groups, sesh_args,
command_details->envp);
switch (error) {
case SESH_SUCCESS: case SESH_SUCCESS:
break; break;
case SESH_ERR_BAD_PATHS: case SESH_ERR_BAD_PATHS:
@@ -836,21 +841,35 @@ selinux_edit_create_tfiles(struct command_details *command_details,
case SESH_ERR_KILLED: case SESH_ERR_KILLED:
sudo_fatalx("%s", U_("sesh: killed by a signal")); sudo_fatalx("%s", U_("sesh: killed by a signal"));
default: default:
sudo_fatalx(U_("sesh: unknown error %d"), rc); sudo_warnx(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++) { for (i = 0; i < nfiles; i++) {
if (chown(tf[i].tfile, user_details.uid, user_details.gid) != 0) { 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", sudo_warn("unable to chown(%s) to %d:%d for editing",
tf[i].tfile, user_details.uid, user_details.gid); tf[i].tfile, user_details.uid, user_details.gid);
close(tfd);
goto done;
} }
close(tfd);
} }
ret = nfiles;
done:
/* Contents of tf will be freed by caller. */ /* Contents of tf will be freed by caller. */
free(sesh_args); free(sesh_args);
return (nfiles); debug_return_int(ret);
} }
static int static int
@@ -858,7 +877,8 @@ selinux_edit_copy_tfiles(struct command_details *command_details,
struct tempfile *tf, int nfiles, struct timespec *times) struct tempfile *tf, int nfiles, struct timespec *times)
{ {
char **sesh_args, **sesh_ap; char **sesh_args, **sesh_ap;
int i, rc, sesh_nargs, ret = 1; int i, error, sesh_nargs, ret = 1;
int tfd = -1;
struct timespec ts; struct timespec ts;
struct stat sb; struct stat sb;
debug_decl(selinux_edit_copy_tfiles, SUDO_DEBUG_EDIT); debug_decl(selinux_edit_copy_tfiles, SUDO_DEBUG_EDIT);
@@ -879,33 +899,43 @@ selinux_edit_copy_tfiles(struct command_details *command_details,
/* Construct args for sesh -e 1 */ /* Construct args for sesh -e 1 */
for (i = 0; i < nfiles; i++) { for (i = 0; i < nfiles; i++) {
if (stat(tf[i].tfile, &sb) == 0) { if (tfd != -1)
mtim_get(&sb, ts); close(tfd);
if (tf[i].osize == sb.st_size && sudo_timespeccmp(&tf[i].omtim, &ts, ==)) { if ((tfd = open(tf[i].tfile, O_RDONLY|O_NONBLOCK|O_NOFOLLOW)) == -1) {
/* sudo_warn(U_("unable to open %s"), tf[i].tfile);
* If mtime and size match but the user spent no measurable continue;
* time in the editor we can't tell if the file was changed. }
*/ if (!sudo_check_temp_file(tfd, tf[i].tfile, user_details.uid, &sb))
if (sudo_timespeccmp(&times[0], &times[1], !=)) { continue;
sudo_warnx(U_("%s unchanged"), tf[i].ofile); mtim_get(&sb, ts);
unlink(tf[i].tfile); if (tf[i].osize == sb.st_size && sudo_timespeccmp(&tf[i].omtim, &ts, ==)) {
continue; /*
} * 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(&times[0], &times[1], !=)) {
sudo_warnx(U_("%s unchanged"), tf[i].ofile);
unlink(tf[i].tfile);
continue;
} }
} }
*sesh_ap++ = tf[i].tfile; *sesh_ap++ = tf[i].tfile;
*sesh_ap++ = tf[i].ofile; *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, sudo_warn("unable to chown(%s) back to %d:%d", tf[i].tfile,
command_details->uid, command_details->gid); command_details->uid, command_details->gid);
} }
} }
*sesh_ap = NULL; *sesh_ap = NULL;
if (tfd != -1)
close(tfd);
if (sesh_ap - sesh_args > 3) { if (sesh_ap - sesh_args > 3) {
/* Run sesh -e 1 <t1> <o1> ... <tn> <on> */ /* Run sesh -e 1 <t1> <o1> ... <tn> <on> */
rc = selinux_run_helper(sesh_args, command_details->envp); error = selinux_run_helper(command_details->uid, command_details->gid,
switch (rc) { command_details->ngroups, command_details->groups, sesh_args,
command_details->envp);
switch (error) {
case SESH_SUCCESS: case SESH_SUCCESS:
ret = 0; ret = 0;
break; break;
@@ -921,7 +951,7 @@ selinux_edit_copy_tfiles(struct command_details *command_details,
sudo_warnx("%s", U_("sesh: killed by a signal")); sudo_warnx("%s", U_("sesh: killed by a signal"));
break; break;
default: default:
sudo_warnx(U_("sesh: unknown error %d"), rc); sudo_warnx(U_("sesh: unknown error %d"), error);
break; break;
} }
if (ret != 0) if (ret != 0)
@@ -943,7 +973,7 @@ sudo_edit(struct command_details *command_details)
{ {
struct command_details saved_command_details; struct command_details saved_command_details;
char **nargv = NULL, **ap, **files = NULL; char **nargv = NULL, **ap, **files = NULL;
int errors, i, ac, nargc, rc; int errors, i, ac, nargc, ret;
int editor_argc = 0, nfiles = 0; int editor_argc = 0, nfiles = 0;
struct timespec times[2]; struct timespec times[2];
struct tempfile *tf = NULL; struct tempfile *tf = NULL;
@@ -1038,7 +1068,7 @@ sudo_edit(struct command_details *command_details)
command_details->ngroups = user_details.ngroups; command_details->ngroups = user_details.ngroups;
command_details->groups = user_details.groups; command_details->groups = user_details.groups;
command_details->argv = nargv; command_details->argv = nargv;
rc = run_command(command_details); ret = run_command(command_details);
if (sudo_gettime_real(&times[1]) == -1) { if (sudo_gettime_real(&times[1]) == -1) {
sudo_warn("%s", U_("unable to read the clock")); sudo_warn("%s", U_("unable to read the clock"));
goto cleanup; goto cleanup;
@@ -1062,14 +1092,14 @@ sudo_edit(struct command_details *command_details)
errors = sudo_edit_copy_tfiles(command_details, tf, nfiles, times); errors = sudo_edit_copy_tfiles(command_details, tf, nfiles, times);
if (errors) { if (errors) {
/* Preserve the edited temporary files. */ /* Preserve the edited temporary files. */
rc = W_EXITCODE(1, 0); ret = W_EXITCODE(1, 0);
} }
for (i = 0; i < nfiles; i++) for (i = 0; i < nfiles; i++)
free(tf[i].tfile); free(tf[i].tfile);
free(tf); free(tf);
free(nargv); free(nargv);
debug_return_int(rc); debug_return_int(ret);
cleanup: cleanup:
/* Clean up temp files and return. */ /* Clean up temp files and return. */

4
src/sudo_exec.h
View File

@@ -1,7 +1,7 @@
/* /*
* SPDX-License-Identifier: ISC * SPDX-License-Identifier: ISC
* *
* Copyright (c) 2010-2016 Todd C. Miller <Todd.Miller@sudo.ws> * Copyright (c) 2010-2017, 2020-2021 Todd C. Miller <Todd.Miller@sudo.ws>
* *
* Permission to use, copy, modify, and distribute this software for any * Permission to use, copy, modify, and distribute this software for any
* purpose with or without fee is hereby granted, provided that the above * purpose with or without fee is hereby granted, provided that the above
@@ -84,9 +84,11 @@
*/ */
struct command_details; struct command_details;
struct command_status; struct command_status;
struct stat;
/* copy_file.c */ /* 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); 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 */ /* exec.c */
void exec_cmnd(struct command_details *details, int errfd); void exec_cmnd(struct command_details *details, int errfd);