From 3f0e5340b651da98251a58cc7923525d69f96032 Mon Sep 17 00:00:00 2001 From: Eugene Syromyatnikov Date: Fri, 1 Jul 2022 10:45:48 +0200 Subject: [PATCH] secontext: fix expected SELinux context check for unlinked FDs MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit selinux_getfdcon open-coded a part of getfdpath_pid since it tries to do the same job, figure out a path associated with an FD, for slightly different purpose: to get the expected SELinux context for it. As the previous commit shows, it's a bit more complicated in cases when the path ends with the " (deleted)" string, which is also used for designated unlinked paths in procfs. Otherwise, it may manifest in test failures such as this: [unconfined_u:unconfined_r:unconfined_t:s0-s0:c0.c1023] fchmod(4 [unconfined_u:object_r:admin_home_t:s0!!system_u:object_r:admin_home_t:s0], 0600) = 0 -[unconfined_u:unconfined_r:unconfined_t:s0-s0:c0.c1023] fchmod(4 [unconfined_u:object_r:admin_home_t:s0!!system_u:object_r:admin_home_t:s0], 051) = 0 -[unconfined_u:unconfined_r:unconfined_t:s0-s0:c0.c1023] fchmod(4 [unconfined_u:object_r:admin_home_t:s0!!system_u:object_r:admin_home_t:s0], 004) = 0 +[unconfined_u:unconfined_r:unconfined_t:s0-s0:c0.c1023] fchmod(4 [unconfined_u:object_r:admin_home_t:s0], 051) = 0 +[unconfined_u:unconfined_r:unconfined_t:s0-s0:c0.c1023] fchmod(4 [unconfined_u:object_r:admin_home_t:s0], 004) = 0 +++ exited with 0 +++ + fail_ '../../src/strace -a15 -y --secontext=full,mismatch -e trace=fchmod ../fchmod-y--secontext_full_mismatch output mismatch' + warn_ 'fchmod-y--secontext_full_mismatch.gen.test: failed test: ../../src/strace -a15 -y --secontext=full,mismatch -e trace=fchmod ../fchmod-y--secontext_full_mismatch output mismatch' + printf '%s\n' 'fchmod-y--secontext_full_mismatch.gen.test: failed test: ../../src/strace -a15 -y --secontext=full,mismatch -e trace=fchmod ../fchmod-y--secontext_full_mismatch output mismatch' fchmod-y--secontext_full_mismatch.gen.test: failed test: ../../src/strace -a15 -y --secontext=full,mismatch -e trace=fchmod ../fchmod-y--secontext_full_mismatch output mismatch + exit 1 FAIL fchmod-y--secontext_full_mismatch.gen.test (exit status: 1) that happens due to the fact that the get_expected_filecontext() call is made against the path with the " (deleted)" part, which is wrong (it is more wrong than shown above when a file with the path that ends with " (deleted)" exists). Moreover, it would be incorrect to call stat() on that path. Let's factor out the common part of the code and simply call it from selinux_getfdcon, then use the st_mode from the procfs link. * src/defs.h (get_proc_pid_fd_path): New declaration. * src/pathtrace.c (get)proc_pid_fd_path): New function, part of getfdpath_pid that performs link resolution and processing of the result. (getfdpath_pid): Call get_proc_pid_fd_path after PID resolution. * src/secontext.c (get_expected_filecontext): Add mode parameter, use it in selabel_lookup call instead of retrieveing file mode using stat() if it is not -1. (selinux_getfdcon): Call get_proc_pid_fd_path instead of open-coding path resolution code, call stat() on the procfs link and pass the retrieved st_mode to the get_expected_filecontext call. (selinux_getfilecon): Pass -1 as mode in the get_expected_filecontext call. Reported-by: Václav Kadlčík Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=2087693 --- src/defs.h | 15 +++++++++++++++ src/pathtrace.c | 26 ++++++++++++++++++-------- src/secontext.c | 35 +++++++++++++++++++++-------------- 3 files changed, 54 insertions(+), 22 deletions(-) Index: strace-5.18/src/defs.h =================================================================== --- strace-5.18.orig/src/defs.h 2022-07-12 18:22:01.563254140 +0200 +++ strace-5.18/src/defs.h 2022-07-12 18:22:06.202199392 +0200 @@ -785,6 +785,21 @@ return pathtrace_match_set(tcp, &global_path_set); } +/** + * Resolves a path for a fd procfs PID proc_pid (the one got from + * get_proc_pid()). + * + * @param proc_pid PID number in /proc, obtained with get_proc_pid(). + * @param fd FD to resolve path for. + * @param buf Buffer to store the resolved path in. + * @param bufsize The size of buf. + * @param deleted If non-NULL, set to true if the path associated with the FD + * seems to have been unlinked and to false otherwise. + * @return Number of bytes written including terminating '\0'. + */ +extern int get_proc_pid_fd_path(int proc_pid, int fd, char *buf, + unsigned bufsize, bool *deleted); + extern int getfdpath_pid(pid_t pid, int fd, char *buf, unsigned bufsize, bool *deleted); Index: strace-5.18/src/pathtrace.c =================================================================== --- strace-5.18.orig/src/pathtrace.c 2022-07-12 18:22:01.532254506 +0200 +++ strace-5.18/src/pathtrace.c 2022-07-12 18:22:06.202199392 +0200 @@ -77,11 +77,9 @@ set->paths_selected[set->num_selected++] = path; } -/* - * Get path associated with fd of a process with pid. - */ int -getfdpath_pid(pid_t pid, int fd, char *buf, unsigned bufsize, bool *deleted) +get_proc_pid_fd_path(int proc_pid, int fd, char *buf, unsigned bufsize, + bool *deleted) { char linkpath[sizeof("/proc/%u/fd/%u") + 2 * sizeof(int)*3]; ssize_t n; @@ -89,10 +87,6 @@ if (fd < 0) return -1; - int proc_pid = get_proc_pid(pid); - if (!proc_pid) - return -1; - xsprintf(linkpath, "/proc/%u/fd/%u", proc_pid, fd); n = readlink(linkpath, buf, bufsize - 1); if (n < 0) @@ -143,6 +137,22 @@ } /* + * Get path associated with fd of a process with pid. + */ +int +getfdpath_pid(pid_t pid, int fd, char *buf, unsigned bufsize, bool *deleted) +{ + if (fd < 0) + return -1; + + int proc_pid = get_proc_pid(pid); + if (!proc_pid) + return -1; + + return get_proc_pid_fd_path(proc_pid, fd, buf, bufsize, deleted); +} + +/* * Add a path to the set we're tracing. Also add the canonicalized * version of the path. Specifying NULL will delete all paths. */ Index: strace-5.18/src/secontext.c =================================================================== --- strace-5.18.orig/src/secontext.c 2022-07-12 18:22:01.564254128 +0200 +++ strace-5.18/src/secontext.c 2022-07-12 18:22:06.203199380 +0200 @@ -62,7 +62,7 @@ } static int -get_expected_filecontext(const char *path, char **secontext) +get_expected_filecontext(const char *path, char **secontext, int mode) { static struct selabel_handle *hdl; @@ -80,12 +80,7 @@ } } - strace_stat_t stb; - if (stat_file(path, &stb) < 0) { - return -1; - } - - return selabel_lookup(hdl, secontext, path, stb.st_mode); + return selabel_lookup(hdl, secontext, path, mode); } /* @@ -130,16 +125,22 @@ /* * We need to resolve the path, because selabel_lookup() doesn't - * resolve anything. Using readlink() is sufficient here. + * resolve anything. */ + char buf[PATH_MAX + 1]; + ssize_t n = get_proc_pid_fd_path(proc_pid, fd, buf, sizeof(buf), NULL); + if ((size_t) n >= (sizeof(buf) - 1)) + return 0; - char buf[PATH_MAX]; - ssize_t n = readlink(linkpath, buf, sizeof(buf)); - if ((size_t) n >= sizeof(buf)) + /* + * We retrieve stat() here since the path the procfs link resolves into + * may be reused by a different file with different context. + */ + strace_stat_t st; + if (stat_file(linkpath, &st)) return 0; - buf[n] = '\0'; - get_expected_filecontext(buf, expected); + get_expected_filecontext(buf, expected, st.st_mode); return 0; } @@ -190,7 +191,13 @@ if (!resolved) return 0; - get_expected_filecontext(resolved, expected); + strace_stat_t st; + if (stat_file(resolved, &st) < 0) + goto out; + + get_expected_filecontext(resolved, expected, st.st_mode); + +out: free(resolved); return 0;