Proposing SELinux support in strace

Renaud Métrich rmetrich at redhat.com
Mon Mar 15 17:09:34 UTC 2021


Hi Dmitry, thanks for the review.

Please see inline. Attached is the newest patch with your recommendations.

Thanks,

Renaud.

On 3/15/21 2:10 AM, Dmitry V. Levin wrote:
>
>> @@ -126,6 +128,18 @@ case "$arch" in
>>   			 else
>>   				st_cv_mpers=no
>>   			 fi])
>> +		AC_CACHE_CHECK([whether selinux runtime works with mpers_name personality],
>> +			[st_cv_selinux_runtime],
>> +			[
>> +			 saved_LIBS="$LIBS"
>> +			 LIBS="-lselinux $LIBS"
>> +			 AC_LINK_IFELSE([AC_LANG_PROGRAM([[#include <selinux/selinux.h>]],
>> +							 [[return 0]])],
>> +					[st_cv_selinux_runtime=yes],
>> +					[st_cv_selinux_runtime=no],
>> +					[st_cv_selinux_runtime=no])
>> +			 LIBS="$saved_LIBS"
>> +			])
>>   		if test $st_cv_mpers = yes; then
>>   			AC_DEFINE(HAVE_MPERS, [1],
>>   				  [Define to 1 if you have mpers_name mpers support])
> Since st_MPERS code is executed after st_SELINUX, its results can be used
> here.  If with_secontexts was set to "no" by st_SELINUX, there is no need
> to bother about selinux runtime here.  Also, if st_cv_runtime was set to
> "no" by earlier checks, there is no need to check selinux runtime either.
>
> For example:
>
>                 AS_IF([test "x$with_secontexts$st_cv_mpers$st_cv_runtime" = xyesyesyes],
>                       [AC_CACHE_CHECK([whether selinux runtime works with mpers_name personality],
>                         [st_cv_selinux_runtime],
>                         [saved_CPPFLAGS="$CPPFLAGS"
>                          saved_LDFLAGS="$LDFLAGS"
>                          saved_LIBS="$LIBS"
>                          CPPFLAGS="$CPPFLAGS libselinux_CPPFLAGS"
>                          LDFLAGS="$LDFLAGS libselinux_LDFLAGS"
>                          LIBS="$LIBS libselinux_LIBS"
>                          AC_LINK_IFELSE([AC_LANG_PROGRAM([[#include <selinux/selinux.h>]],
>                                                          [[return 0]])],
>                                         [st_cv_selinux_runtime=yes],
>                                         [st_cv_selinux_runtime=no],
>                                         [st_cv_selinux_runtime=no])
>                          LIBS="$saved_LIBS"
>                          LDFLAGS="$saved_LDFLAGS"
>                          CPPFLAGS="$saved_CPPFLAGS"
>                         ])
>                       ],
>                       [st_cv_selinux_runtime=no])
OK
> diff --git a/src/Makefile.am b/src/Makefile.am
>> index ee9415ea5..e5553918a 100644
>> --- a/src/Makefile.am
>> +++ b/src/Makefile.am
>> @@ -186,6 +186,7 @@ libstrace_a_SOURCES =	\
>>   	mmap_notify.c	\
>>   	mmap_notify.h	\
>>   	mmsghdr.c	\
>> +	mntns.c		\
>>   	mount.c		\
>>   	move_mount.c	\
>>   	mpers_type.h	\
>> @@ -303,6 +304,7 @@ libstrace_a_SOURCES =	\
>>   	sched_attr.h	\
>>   	scsi.c		\
>>   	seccomp.c	\
>> +	selinux.h	\
>>   	sendfile.c	\
>>   	sg_io_v3.c	\
>>   	sg_io_v4.c	\
>> @@ -406,6 +408,13 @@ strace_LDADD += $(libiberty_LIBS)
>>   endif
>>   endif
>>   
>> +if USE_SELINUX
>> +libstrace_a_SOURCES += selinux.c
>> +strace_CPPFLAGS += $(libselinux_CPPFLAGS)
>> +strace_LDFLAGS += $(libselinux_LDFLAGS)
>> +strace_LDADD += $(libselinux_LIBS)
>> +endif
>> +
>>   strace_CPPFLAGS += $(CODE_COVERAGE_CPPFLAGS)
>>   strace_CFLAGS += $(CODE_COVERAGE_CFLAGS)
>>   strace_LDADD += $(CODE_COVERAGE_LIBS)
> It's an interesting question whether you want to compile mntns.c when
> selinux support is not enabled.  Maybe some day in the future mntns.c might
> be used outside USE_SELINUX code, bit currently it looks like we would
> better off without mntns.c when USE_SELINUX is not enabled.
>
> In other words, let's move mntns.c and selinux.h into "if USE_SELINUX"
> case.
OK
>> diff --git a/src/access.c b/src/access.c
>> index 2936242f7..81b78889b 100644
>> --- a/src/access.c
>> +++ b/src/access.c
>> @@ -34,8 +34,10 @@ decode_faccessat(struct tcb *tcp)
>>   {
>>   	/* dirfd */
>>   	print_dirfd(tcp, tcp->u_arg[0]);
>> +#ifdef USE_SELINUX
>> +	tcp->dirfd = (int)(tcp->u_arg[0]);
>> +#endif
>>   	tprint_arg_next();
>> -
>>   	decode_access(tcp, 1);
>>   }
>> [...]
>> You've added a lot of such tcp->dirfd assignments.  Apparently,
>> they are added after every print_dirfd invocation except one (and that one
>> seems to be omitted by mistake).  If you want to cache dirfd processed by
>> print_dirfd, it would be much easier if you just moved this assignment
>> inside print_dirfd.

Initially I had this idea as well but then thought it was ugly to have 
this state saved in print_dirfd().

Additionally we need to really know when to set the dirfd: there may be 
cases where print_dirfd() is called but the next arguments to the 
syscall won't use that dirfd as "reference".

Hence it's safer to assign dirfd in the syscall handler itself.

>> diff --git a/src/defs.h b/src/defs.h
>> index 08a293130..673e86539 100644
>> --- a/src/defs.h
>> +++ b/src/defs.h
>> @@ -293,6 +293,17 @@ struct tcb {
>>   	 */
>>   	unsigned int pid_ns;
>>   
>> +	/*
>> +	 * The ID of the MOUNT namespace of this process
>> +	 * (inode number of /proc/<pid>/ns/mnt)
>> +	 * (0: not initialized)
>> +	 */
>> +	unsigned int mnt_ns;
>> +
>> +#ifdef USE_SELINUX
>> +	int dirfd; /* Use AT_FDCWD for 'not set' */
>> +#endif
>> +
>>   	struct mmap_cache_t *mmap_cache;
>>   
>>   	/*
> Since mnt_ns shouldn't be used outside USE_SELINUX yet, let's move it
> inside USE_SELINUX.
>
> Also, dirfd seems to be too short, I'd like to rename it to something more
> descriptive, e.g. last_dirfd.
>
OK
>> @@ -1046,8 +1057,13 @@ print_local_array_ex(struct tcb *tcp,
>>   extern kernel_ulong_t *
>>   fetch_indirect_syscall_args(struct tcb *, kernel_ulong_t addr, unsigned int n_args);
>>   
>> +extern unsigned int get_mnt_ns(struct tcb *tcp);
>> +extern unsigned int get_our_mnt_ns(void);
>> +
>>   extern void pidns_init(void);
>>   
>> +extern const char *pid_to_str(pid_t pid);
>> +
>>   /**
>>    * Returns the pid of the tracee as present in /proc of the tracer (can be
>>    * different from tcp->pid if /proc and the tracer process are in different PID
> You can move these declarations inside USE_SELINUX, but it's not
> necessary.
OK
>> diff --git a/src/mntns.c b/src/mntns.c
>> new file mode 100644
>> index 000000000..ac6cddbf6
>> --- /dev/null
>> +++ b/src/mntns.c
>> @@ -0,0 +1,72 @@
>> +/*
>> + * Copyright (c) 2021 The strace developers.
>> + * All rights reserved.
>> + *
>> + * SPDX-License-Identifier: LGPL-2.1-or-later
>> + */
>> +
>> +#include "defs.h"
>> +
>> +#include <fcntl.h>
>> +#include <sys/types.h>
>> +#include <sys/stat.h>
>> +
>> +#include "largefile_wrappers.h"
>> +#include "xstring.h"
>> +
>> +static unsigned int
>> +get_mnt_ns_of_pid(int pid)
>> +{
>> +	unsigned int ns = 0;
>> +	char path[PATH_MAX + 1];
>> +	xsprintf(path, "/proc/%s/ns/mnt", pid_to_str(pid));
>> +
>> +	int fd = open_file(path, O_RDONLY);
>> +	if (fd < 0)
>> +		return 0;
>> +	strace_stat_t st;
>> +	if (fstat_fd(fd, &st))
>> +		goto out;
>> +	ns = st.st_ino;
>> +out:
>> +	close(fd);
> Wouldn't it be easier to read if the last 5 lines were written this way:
>
> 	if (fstat_fd(fd, &st) == 0)
> 		ns = st.st_ino;
> 	close(fd);
>
> I'd go even further:
>
> 	if (fd >= 0) {
> 		strace_stat_t st;
> 		if (fstat_fd(fd, &st) == 0)
> 			ns = st.st_ino;
> 		close(fd);
> 	}
OK
>> +
>> +	return ns;
>> +}
>> +
>> +/**
>> + * Returns the MOUNT namespace of the tracee
>> + */
>> +unsigned int
>> +get_mnt_ns(struct tcb *tcp)
>> +{
>> +	if (tcp->mnt_ns)
>> +		return tcp->mnt_ns;
>> +
>> +	int proc_pid = 0;
>> +	translate_pid(NULL, tcp->pid, PT_TID, &proc_pid);
>> +
>> +	if (!proc_pid)
>> +		return 0;
>> +
>> +	tcp->mnt_ns = get_mnt_ns_of_pid(proc_pid);
> Wouldn't it be easier to read if the last 4 lines were written this way:
>
> 	if (proc_pid)
> 		tcp->mnt_ns = get_mnt_ns_of_pid(proc_pid);
>
OK
>> +
>> +	return tcp->mnt_ns;
>> +}
>> +
>> +/**
>> + * Returns the MOUNT namespace of strace
>> + */
>> +unsigned int
>> +get_our_mnt_ns(void)
>> +{
>> +	static unsigned int our_ns = 0;
>> +	static bool our_ns_initialised = false;
>> +
>> +	if (!our_ns_initialised) {
>> +		our_ns = get_mnt_ns_of_pid((int)getpid());
> I don't think this explicit cast is really needed here.
OK
>> +		our_ns_initialised = true;
>> +	}
>> +
>> +	return our_ns;
>> +}
> The rest of this file is OK
>
>> diff --git a/src/pidns.c b/src/pidns.c
>> index dbcf52b91..6221f902b 100644
>> --- a/src/pidns.c
>> +++ b/src/pidns.c
>> @@ -142,7 +142,7 @@ get_cached_proc_pid(unsigned int ns, int ns_pid, enum pid_type type)
>>    * Helper function, converts pid to string, or to "self" for pid == 0.
>>    * Uses static buffer for operation.
>>    */
>> -static const char *
>> +const char *
>>   pid_to_str(pid_t pid)
>>   {
>>   	if (!pid)
> OK
>
>> diff --git a/src/selinux.c b/src/selinux.c
>> new file mode 100644
>> index 000000000..0ea9e46ee
>> --- /dev/null
>> +++ b/src/selinux.c
>> @@ -0,0 +1,179 @@
>> +/*
>> + * Copyright (c) 2020-2021 The strace developers.
>> + * All rights reserved.
>> + *
>> + * SPDX-License-Identifier: LGPL-2.1-or-later
>> + */
>> +
>> +#include <fcntl.h>
>> +#include <limits.h>
>> +#include <selinux/selinux.h>
>> +#include <stdlib.h>
>> +
>> +#include "defs.h"
> "defs.h" must be included first.
>
> Also, I prefer to have <stdlib.h> included before <selinux/selinux.h>.
OK
>> +#include "number_set.h"
>> +#include "selinux.h"
>> +#include "xstring.h"
>> +
>> +#ifndef AT_FDCWD
>> +# define AT_FDCWD	-100
>> +#endif
>> +
>> +bool selinux_context = false;
>> +bool selinux_context_full = false;
>> +
>> +enum selinux_objtype {
>> +	SELINUX_PID,
>> +	SELINUX_PATH
>> +};
>> +
>> +typedef struct {
>> +	enum selinux_objtype type;
>> +	union {
>> +		pid_t pid;
>> +		char *path;
>> +	} obj;
>> +} selinux_obj_t;
>> +
>> +static int getcontext(selinux_obj_t *obj, char **context);
>> +
>> +/*
>> + * Retrieves the SELinux context of the given PID (extracted from the tcb).
>> + * Memory must be freed.
>> + * Returns 0 on success, -1 on failure.
>> + */
>> +int
>> +selinux_getpidcon(struct tcb *tcp, char **context)
>> +{
>> +	int proc_pid = 0;
>> +	translate_pid(NULL, tcp->pid, PT_TID, &proc_pid);
>> +	if (!proc_pid)
>> +		return -1;
> Let's check selinux_context first and return from this function
> if selinux_context is not enabled.
I don't understand. We need *proc_pid* to get the context, so the 
translation must be performed initially.
>> +
>> +	selinux_obj_t obj;
>> +	obj.type = SELINUX_PID;
>> +	obj.obj.pid = (pid_t)proc_pid;
>> +
>> +	return getcontext(&obj, context);
>> +}
>> +
>> +/*
>> + * Retrieves the SELinux context of the given path.
>> + * Memory must be freed.
>> + * Returns 0 on success, -1 on failure.
>> + */
>> +int
>> +selinux_getfilecon(struct tcb *tcp, char *path, char **context)
> Let's change "char *path" to "const char *path".
OK
>> +{
>> +	/*
>> +	 * Current limitation: we cannot query the path if we are in different
>> +	 * mount namespaces.
>> +	 */
>> +	if (get_mnt_ns(tcp) != get_our_mnt_ns())
>> +		return -1;
> Let's check selinux_context first and return from this function
> if selinux_context is not enabled.
Here, if we are not in the right context, then getfilecon() will return 
false positives, so I think the first thing to check is that we can 
indeed retrieve a valid context.
>> +
>> +	selinux_obj_t obj;
>> +	obj.type = SELINUX_PATH;
>> +	obj.obj.path = path;
>> +
>> +	if (path[0] == '/')
>> +		return getcontext(&obj, context);
>> +
>> +	char resolved[PATH_MAX + 1];
>> +	char pathtoresolve[2 * PATH_MAX + 2];
>> +	int proc_pid = 0;
>> +	translate_pid(NULL, tcp->pid, PT_TID, &proc_pid);
>> +	if (!proc_pid)
>> +		return -1;
>> +
>> +	/*
>> +	 * If we have a relative pathname and 'dirfd' == AT_FDCWD, we need to
>> +	 * prepend by the CWD.
>> +	 */
>> +	if (tcp->dirfd == AT_FDCWD) {
>> +		char linkpath[sizeof("/proc/%u/cwd") + sizeof(int)*3];
>> +		ssize_t n;
>> +
>> +		xsprintf(linkpath, "/proc/%u/cwd", proc_pid);
>> +		n = readlink(linkpath, resolved, sizeof(resolved) - 1);
>> +		/*
>> +		 * NB: if buffer is too small, readlink doesn't fail,
>> +		 * it returns truncated result.
>> +		 */
>> +		if (n == sizeof(resolved) - 1)
>> +			return -1;
>> +		resolved[n] = '\0';
>> +	} else {
>> +		if (number_set_array_is_empty(decode_fd_set, 0) ||
> Interesting.  This means that --decode-fds should have effect on
> selinux_getfilecon.  I wonder why.

 From my research, there is no way to get a path from a dirfd, hence we 
need implicitly --decode-fds for this to work, since the latter will 
have the mapping fd -> pathname for us.

But actually I removed the number_set_array_is_empty() and it just works ...

>> +		    getfdpath_pid(proc_pid, tcp->dirfd, resolved, sizeof(resolved)) < 0)
>> +			return -1;
> getfdpath_pid itself uses translate_pid to translate it's first argument.
>
OK, I'm wrong indeed.
>> +	}
>> +	xsprintf(pathtoresolve, "%s/%s", resolved, path);
>> +	if (realpath(pathtoresolve, resolved) == NULL) {
> Interesting.  Why realpath?  Would stat_file be enough?
No, because the path to check needs to be absolute, or else cwd of 
strace will be used for resolving the context, which is wrong.
>> +		/*
>> +		 * This can happen in multiple cases:
>> +		 * - there was an error different than ENOENT
>> +		 * - if path doesn't exist
>> +		 * - path is below root directory, e.g. root dir and cwd are
>> +		 *   '/home/<user>/chroot' and path is below root directory,
>> +		 *   e.g. '../../bin/ls'
>> +		 */
>> +		return -1;
>> +	}
>> +	obj.obj.path = resolved;
>> +
>> +	return getcontext(&obj, context);
>> +}
>> +
>> +static int
>> +getcontext(selinux_obj_t *obj, char **context)
>> +{
>> +	int res;
>> +	char *secontext;
>> +
>> +	if (!selinux_context)
>> +		goto fail;
>> +
>> +	switch (obj->type) {
>> +	case SELINUX_PID:
>> +		res = getpidcon(obj->obj.pid, &secontext);
>> +		break;
>> +	case SELINUX_PATH:
>> +		res = getfilecon(obj->obj.path, &secontext);
>> +		break;
>> +	default:
>> +		/* XXX Assert instead */
>> +		goto fail;
> This is the reason why I don't like this enum/case approach:
> it creates unreachable code.
OK changed into if/else
>> +	}
>> +
>> +	if (res == -1)
>> +		goto fail;
>> +
>> +	if (selinux_context_full) {
>> +		*context = xstrdup(secontext);
>> +	} else {
>> +		char *saveptr = NULL;
>> +		char *secontext_copy = xstrdup(secontext);
>> +		const char *token;
>> +		int i;
>> +
>> +		/*
>> +		 * We only want to keep the type (3rd field, ':' separator).
>> +		 */
>> +		*context = NULL;
>> +		for (token = strtok_r(secontext_copy, ":", &saveptr), i = 0;
>> +		     token; token = strtok_r(NULL, ":", &saveptr), i++) {
>> +			if (i == 2) {
>> +				*context = xstrdup(token);
>> +				break;
>> +			}
>> +		}
>> +		if (*context == NULL)
>> +			*context = xstrdup(secontext);
>> +		free(secontext_copy);
>> +	}
>> +	freecon(secontext);
>> +	return 0;
>> +fail:
>> +	return -1;
>> +}
> I suggest some rewriting, for example:
>
> #include "defs.h"
>
> #include <fcntl.h>
> #include <limits.h>
> #include <stdlib.h>
> #include <selinux/selinux.h>
>
> #include "number_set.h"
> #include "selinux.h"
> #include "xstring.h"
>
> #ifndef AT_FDCWD
> # define AT_FDCWD	-100
> #endif
>
> bool selinux_context = false;
> bool selinux_context_full = false;
>
> static int
> getcontext(int rc, char **secontext, char **result)
> {
> 	if (rc < 0)
> 		return rc;
>
> 	*result = NULL;
> 	if (!selinux_context_full) {
> 		char *saveptr = NULL;
> 		char *secontext_copy = xstrdup(*secontext);
> 		const char *token;
> 		unsigned int i;
>
> 		/*
> 		 * We only want to keep the type (3rd field, ':' separator).
> 		 */
> 		for (token = strtok_r(secontext_copy, ":", &saveptr), i = 0;
> 		     token; token = strtok_r(NULL, ":", &saveptr), i++) {
> 			if (i == 2) {
> 				*result = xstrdup(token);
> 				break;
> 			}
> 		}
> 		free(secontext_copy);
> 	}
>
> 	if (*result == NULL)
> 		*result = xstrdup(*secontext);
> 	freecon(*secontext);
> 	return 0;
> }
>
> /*
>   * Retrieves the SELinux context of the given PID (extracted from the tcb).
>   * Memory must be freed.
>   * Returns 0 on success, -1 on failure.
>   */
> int
> selinux_getpidcon(struct tcb *tcp, char **result)
> {
> 	if (!selinux_context)
> 		return -1;
>
> 	int proc_pid = 0;
> 	translate_pid(NULL, tcp->pid, PT_TID, &proc_pid);
> 	if (!proc_pid)
> 		return -1;
>
> 	char *secontext;
> 	return getcontext(getpidcon(proc_pid, &secontext), &secontext, result);
> }
>
> /*
>   * Retrieves the SELinux context of the given path.
>   * Memory must be freed.
>   * Returns 0 on success, -1 on failure.
>   */
> int
> selinux_getfilecon(struct tcb *tcp, const char *path, char **result)
> {
> 	if (!selinux_context)
> 		return -1;
>
> 	/*
> 	 * Current limitation: we cannot query the path if we are in different
> 	 * mount namespaces.
> 	 */
> 	if (get_mnt_ns(tcp) != get_our_mnt_ns())
> 		return -1;
>
> 	char *secontext;
>
> 	if (path[0] == '/')
> 		return getcontext(getfilecon(path, &secontext), &secontext, result);
>
> 	char resolved[PATH_MAX + 1];
>
> 	/*
> 	 * If we have a relative pathname and 'dirfd' == AT_FDCWD, we need to
> 	 * prepend by the CWD.
> 	 */
> 	if (tcp->last_dirfd == AT_FDCWD) {
> 		int proc_pid = 0;
> 		translate_pid(NULL, tcp->pid, PT_TID, &proc_pid);
> 		if (!proc_pid)
> 			return -1;
>
> 		char linkpath[sizeof("/proc/%u/cwd") + sizeof(int)*3];
> 		xsprintf(linkpath, "/proc/%u/cwd", proc_pid);
> 		ssize_t n = readlink(linkpath, resolved, sizeof(resolved) - 1);
> 		/*
> 		 * NB: if buffer is too small, readlink doesn't fail,
> 		 * it returns truncated result.
> 		 */
> 		if (n == sizeof(resolved) - 1)
> 			return -1;
> 		resolved[n] = '\0';
> 	} else {
> 		if (getfdpath_pid(tcp->pid, tcp->last_dirfd,
> 				  resolved, sizeof(resolved)) < 0)
> 			return -1;
> 	}
>
> 	char pathtoresolve[2 * PATH_MAX + 2];
> 	xsprintf(pathtoresolve, "%s/%s", resolved, path);
> 	/* XXX Why realpath?  Would stat_file be enough?  */
> 	if (realpath(pathtoresolve, resolved) == NULL) {
> 		/*
> 		 * This can happen in multiple cases:
> 		 * - there was an error different than ENOENT
> 		 * - if path doesn't exist
> 		 * - path is below root directory, e.g. root dir and cwd are
> 		 *   '/home/<user>/chroot' and path is below root directory,
> 		 *   e.g. '../../bin/ls'
> 		 */
> 		return -1;
> 	}
>
> 	return getcontext(getfilecon(resolved, &secontext), &secontext, result);
> }
OK, smarter code indeed.
> @@ -896,6 +922,13 @@ alloctcb(int pid)
>>   			tcp->pid = pid;
>>   #if SUPPORTED_PERSONALITIES > 1
>>   			tcp->currpers = current_personality;
>> +#endif
>> +#ifdef USE_SELINUX
>> +#ifndef AT_FDCWD
>> +# define AT_FDCWD>------100
>> +#endif
> What is this? :)
Well, that was defined multiple times in the pre 5.11 sources. I removed 
this right now.
>
>> @@ -3220,6 +3275,9 @@ next_event(void)
>>   			if (!tcp)
>>   				goto next_event_wait_next;
>>   		}
>> +#ifdef USE_SELINUX
>> +		tcp->dirfd = AT_FDCWD;
>> +#endif
>>   
>>   		if (cflag) {
>>   			tcp->stime.tv_sec = ru.ru_stime.tv_sec;
> I'm not sure this is the most optimal place to reset tcp->last_dirfd.
> For example, if you want to assign it on entering syscall and use it on
> exiting (e.g. in case of getdents below), this is not the right place
> to reset it.
Actually this has to be done in trace_syscall() upon entering the syscall.
>> diff --git a/src/util.c b/src/util.c
>> index 9cb555ecb..509b12f7f 100644
>> --- a/src/util.c
>> +++ b/src/util.c
>> @@ -26,6 +26,7 @@
>>   #include "largefile_wrappers.h"
>>   #include "number_set.h"
>>   #include "print_utils.h"
>> +#include "selinux.h"
>>   #include "static_assert.h"
>>   #include "string_to_uint.h"
>>   #include "xlat.h"
>> @@ -664,6 +665,13 @@ printfd_pid(struct tcb *tcp, pid_t pid, int fd)
>>   
>>   printed:
>>   		tprints(">");
>> +#ifdef USE_SELINUX
>> +		char *context;
>> +		if (!selinux_getfilecon(tcp, path, &context)) {
>> +			tprintf(" [%s]", context);
>> +			free(context);
>> +		}
>> +#endif
>>   	} else {
>>   		tprintf("%d", fd);
>>   	}
>> @@ -959,6 +967,14 @@ printpathn(struct tcb *const tcp, const kernel_ulong_t addr, unsigned int n)
>>   	else {
>>   		path[n++] = !nul_seen;
>>   		print_quoted_cstring(path, n);
>> +
>> +#ifdef USE_SELINUX
>> +		char *context;
>> +		if (nul_seen && !selinux_getfilecon(tcp, path, &context)) {
>> +			tprintf(" [%s]", context);
>> +			free(context);
>> +		}
>> +#endif
>>   	}
>>   
>>   	return nul_seen;
> This is OK, but there are few places in the code where printpathn is used
> in different contexts where this wouldn't be appropriate:
> - all readdir/getdents contexts, printpathn is used there relative to the
>    corresponding directory, a solution is to assign tcp->last_dirfd inside
>    SYS_FUNC(readdir) and xgetdents;
> - memfd_create and sock_ioctl use printpathn to print something that's not
>    a filesystem object at all, probably they should use printstrn instead.

I missed some cases indeed, I was checking print_dirfd() callers only.

I however didn't replace printpathn() for memfd_create and sock_ioctl by 
printstrn() because it should be another commit I guess.

>
>> diff --git a/strace.spec.in b/strace.spec.in
>> index c8f281f18..b3a2895d4 100644
>> --- a/strace.spec.in
>> +++ b/strace.spec.in
>> @@ -29,11 +29,13 @@ BuildRequires: pkgconfig(bluez)
>>   # Install binutils-devel to enable symbol demangling.
>>   %if 0%{?fedora} >= 20 || 0%{?centos} >= 6 || 0%{?rhel} >= 6
>>   %define buildrequires_stacktrace BuildRequires: elfutils-devel binutils-devel
>> +%define buildrequires_selinux BuildRequires: libselinux-devel
>>   %endif
>>   %if 0%{?suse_version} >= 1100
>>   %define buildrequires_stacktrace BuildRequires: libdw-devel binutils-devel
>>   %endif
>>   %{?buildrequires_stacktrace}
>> +%{?buildrequires_selinux}
>>   
>>   # OBS compatibility
>>   %{?!buildroot:BuildRoot: %_tmppath/buildroot-%name-%version-%release}
> Maybe opensuse also provides libselinux-devel that could be used here.
Indeed there is. Actually I tested on RHEL and Fedora only.
>
> I haven't reviewed the tests yet, just tried to build and run them.
> I needed to apply the following change to make them pass:
>
> diff --git a/tests/gen_tests.in b/tests/gen_tests.in
> index 3f41959a9..c674c9891 100644
> --- a/tests/gen_tests.in
> +++ b/tests/gen_tests.in
> @@ -10,8 +10,8 @@ _newselect-P	 -e trace=_newselect -P /dev/full 9>>/dev/full
>   accept	-a22
>   accept4	-a37
>   access	-a30 --trace-path=access_sample
> -access--secontext	--secontext --trace-path=access_sample -e trace=access
> -access--secontext_full	--secontext=full --trace-path=access_sample -e trace=access
> +access--secontext	-a30 --secontext --trace-path=access_sample -e trace=access
> +access--secontext_full	-a30 --secontext=full --trace-path=access_sample -e trace=access
OK, I wasn't sure about the need to "-aXX" option, so removed all this 
because I didn't see any difference on my systems.
> Also please note that you can use asprintf and functions from xmalloc.h
> in tests, not need to use complicated malloc's followed by asserts.
OK
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0002-ldv-s-comments-integration.patch
Type: text/x-patch
Size: 25860 bytes
Desc: not available
URL: <http://lists.strace.io/pipermail/strace-devel/attachments/20210315/d2818b2c/attachment.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: OpenPGP_signature
Type: application/pgp-signature
Size: 840 bytes
Desc: OpenPGP digital signature
URL: <http://lists.strace.io/pipermail/strace-devel/attachments/20210315/d2818b2c/attachment-0001.bin>


More information about the Strace-devel mailing list