Proposing SELinux support in strace
Dmitry V. Levin
ldv at altlinux.org
Wed Mar 17 01:15:59 UTC 2021
On Tue, Mar 16, 2021 at 02:43:29PM +0100, Renaud Métrich wrote:
> OK, done. Final (?) patch in attachment.
Not the final yet, but very close.
Unless you want to keep the feature buried in the source code, please
document it in doc/strace.1.in and mention --secontext in NEWS.
There are also a few minor issues that have to be addressed,
see below.
With regards to CI, have you tried to add libselinux-dev
to ci/install-dependencies.sh, does it make any difference?
> diff --git a/m4/mpers.m4 b/m4/mpers.m4
> index 510aabe84..6fb40926d 100644
> --- a/m4/mpers.m4
> +++ b/m4/mpers.m4
> @@ -63,9 +63,11 @@ pushdef([mpers_name], [$1])
> pushdef([MPERS_NAME], translit([$1], [a-z], [A-Z]))
> pushdef([HAVE_MPERS], [HAVE_]MPERS_NAME[_MPERS])
> pushdef([HAVE_RUNTIME], [HAVE_]MPERS_NAME[_RUNTIME])
> +pushdef([HAVE_SELINUX_RUNTIME], [HAVE_]MPERS_NAME[_SELINUX_RUNTIME])
> pushdef([MPERS_CFLAGS], [$cc_flags_$1])
> pushdef([st_cv_cc], [st_cv_$1_cc])
> pushdef([st_cv_runtime], [st_cv_$1_runtime])
> +pushdef([st_cv_selinux_runtime], [st_cv_$1_selinux_runtime])
> pushdef([st_cv_mpers], [st_cv_$1_mpers])
>
> pushdef([EXEEXT], MPERS_NAME[_EXEEXT])dnl
> @@ -126,6 +128,26 @@ case "$arch" in
> else
> st_cv_mpers=no
> fi])
> + 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])
> + )
The last [st_cv_selinux_runtime=no] should be the "else" branch of AS_IF.
> diff --git a/src/access.c b/src/access.c
> index 2936242f7..214a32f6d 100644
> --- a/src/access.c
> +++ b/src/access.c
> @@ -35,7 +35,6 @@ decode_faccessat(struct tcb *tcp)
> /* dirfd */
> print_dirfd(tcp, tcp->u_arg[0]);
> tprint_arg_next();
> -
> decode_access(tcp, 1);
> }
>
Please drop this.
> diff --git a/src/dirent.c b/src/dirent.c
> index 3c2c09e9b..ca56f15cd 100644
> --- a/src/dirent.c
> +++ b/src/dirent.c
> @@ -106,6 +106,9 @@ SYS_FUNC(readdir)
> if (entering(tcp)) {
> /* fd */
> printfd(tcp, tcp->u_arg[0]);
> +#ifdef USE_SELINUX
> + tcp->last_dirfd = (int)(tcp->u_arg[0]);
> +#endif
> tprint_arg_next();
> } else {
> /* dirp */
OK
> diff --git a/src/fanotify.c b/src/fanotify.c
> index 9e8071009..f60dcf09c 100644
> --- a/src/fanotify.c
> +++ b/src/fanotify.c
> @@ -57,8 +57,9 @@ SYS_FUNC(fanotify_mark)
> tprints(", ");
> if ((int) tcp->u_arg[argn] == FAN_NOFD)
> print_xlat_d(FAN_NOFD);
> - else
> + else {
> print_dirfd(tcp, tcp->u_arg[argn]);
> + }
> tprints(", ");
> printpath(tcp, tcp->u_arg[argn + 1]);
>
Please drop this.
> diff --git a/src/open.c b/src/open.c
> index b48fc087a..a364c56a5 100644
> --- a/src/open.c
> +++ b/src/open.c
> @@ -33,6 +33,9 @@ print_dirfd(struct tcb *tcp, int fd)
> print_xlat_d(AT_FDCWD);
> else
> printfd(tcp, fd);
> +#ifdef USE_SELINUX
> + tcp->last_dirfd = fd;
> +#endif
> }
>
> /*
OK
> diff --git a/src/selinux.c b/src/selinux.c
> new file mode 100644
> index 000000000..5e9fc9126
> --- /dev/null
> +++ b/src/selinux.c
> @@ -0,0 +1,128 @@
> +/*
> + * Copyright (c) 2020-2021 The strace developers.
> + * All rights reserved.
> + *
> + * SPDX-License-Identifier: LGPL-2.1-or-later
> + */
> +
> +#include "defs.h"
> +
> +#include <stdlib.h>
> +#include <fcntl.h>
> +#include <limits.h>
> +#include <selinux/selinux.h>
> +
> +#include "selinux.h"
> +#include "xstring.h"
> +
> +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 'last_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;
> + }
Since readlink can return a path that doesn't start with '/', let's be on
the safe side and check against that:
if (resolved[0] != '/')
return -1;
> +
> + char pathtoresolve[2 * PATH_MAX + 2];
> + xsprintf(pathtoresolve, "%s/%s", resolved, path);
> +
> + return getcontext(getfilecon(pathtoresolve, &secontext), &secontext, result);
> +}
[...]
> @@ -2321,6 +2356,17 @@ init(int argc, char *argv[])
> case GETOPT_SECCOMP:
> seccomp_filtering = true;
> break;
> +#ifdef USE_SELINUX
> + case GETOPT_SELINUX_CONTEXT:
> + selinux_context = true;
> + if (optarg) {
> + if (!strcmp(optarg, "full"))
> + selinux_context_full = true;
> + else
> + error_opt_arg(c, lopt, optarg);
> + }
> + break;
> +#endif
The "else" branch is not covered by the test suite.
> case GETOPT_QUAL_TRACE:
> qualify_trace(optarg);
> break;
> @@ -2503,6 +2549,11 @@ init(int argc, char *argv[])
> if (!number_set_array_is_empty(decode_fd_set, 0))
> error_msg("-y/--decode-fds has no effect "
> "with -c/--summary-only");
> +#ifdef USE_SELINUX
> + if (selinux_context)
> + error_msg("--secontext has no effect with "
> + "-c/--summary-only");
> +#endif
This is not covered by the test suite.
> diff --git a/src/syscall.c b/src/syscall.c
> index d143f257b..245cc7419 100644
> --- a/src/syscall.c
> +++ b/src/syscall.c
> @@ -24,6 +24,7 @@
> #include "poke.h"
> #include "retval.h"
> #include <limits.h>
> +#include <fcntl.h>
>
> /* for struct iovec */
> #include <sys/uio.h>
> @@ -1019,6 +1020,10 @@ syscall_exiting_finish(struct tcb *tcp)
> tcp->sys_func_rval = 0;
> free_tcb_priv_data(tcp);
>
> +#ifdef USE_SELINUX
> + tcp->last_dirfd = AT_FDCWD;
> +#endif
> +
> if (cflag)
> tcp->ltime = tcp->stime;
> }
OK
> diff --git a/src/xgetdents.c b/src/xgetdents.c
> index 4de5fbc10..6a0144bbf 100644
> --- a/src/xgetdents.c
> +++ b/src/xgetdents.c
> @@ -122,6 +122,9 @@ xgetdents(struct tcb *const tcp, const unsigned int header_size,
> {
> if (entering(tcp)) {
> printfd(tcp, tcp->u_arg[0]);
> +#ifdef USE_SELINUX
> + tcp->last_dirfd = (int)(tcp->u_arg[0]);
> +#endif
> tprints(", ");
> return 0;
> }
OK
> diff --git a/tests/Makefile.am b/tests/Makefile.am
> index b8efce824..8076719da 100644
> --- a/tests/Makefile.am
> +++ b/tests/Makefile.am
> @@ -292,6 +292,63 @@ check_PROGRAMS = $(PURE_EXECUTABLES) \
> zeroargc \
> # end of check_PROGRAMS
>
> +check_PROGRAMS += \
> + access--secontext \
> + access--secontext_full \
> + chmod--secontext \
> + chmod--secontext_full \
> + execve--secontext \
> + execve--secontext_full \
> + execveat--secontext \
> + execveat--secontext_full \
> + faccessat--secontext \
> + faccessat--secontext_full \
> + fanotify_mark--secontext \
> + fanotify_mark--secontext_full \
> + fchmod--secontext \
> + fchmod--secontext_full \
> + fchmodat--secontext \
> + fchmodat--secontext_full \
> + fchownat--secontext \
> + fchownat--secontext_full \
> + file_handle--secontext \
> + file_handle--secontext_full \
> + linkat--secontext \
> + linkat--secontext_full \
> + openat--secontext \
> + openat--secontext_full
Could you put this list to secontext_PROGRAMS and add
$(secontext_PROGRAMS) to check_PROGRAMS instead, please?
> diff --git a/tests/access--secontext.c b/tests/access--secontext.c
> new file mode 100644
> index 000000000..8ba2c02df
> --- /dev/null
> +++ b/tests/access--secontext.c
> @@ -0,0 +1,57 @@
> +/*
> + * Copyright (c) 2021 The strace developers.
> + * All rights reserved.
> + *
> + * SPDX-License-Identifier: GPL-2.0-or-later
> + */
> +
> +#include "tests.h"
> +#include "scno.h"
> +
> +/*
> + * This test is designed to be executed with the following strace options:
> + * --secontext[=full]
> + */
> +
> +#if defined __NR_access && defined HAVE_SELINUX_RUNTIME
> +
> +# include <fcntl.h>
> +# include <stdio.h>
> +# include <stdlib.h>
> +# include <unistd.h>
> +
> +# include "selinux.c"
> +
> +int
> +main(void)
> +{
> + static const char sample[] = "access_sample";
> +
> + unlink(sample);
> + if (open(sample, O_CREAT|O_RDONLY, 0400) == -1)
> + perror_msg_and_fail("open");
> +
> + long rc = syscall(__NR_access, sample, F_OK);
> + printf("%saccess(\"%s\"%s, F_OK) = %s\n",
> + SELINUX_MYCONTEXT(),
> + sample, SELINUX_FILECONTEXT(sample),
> + sprintrc(rc));
> +
> + if (unlink(sample) == -1)
> + perror_msg_and_fail("unlink");
> +
> + rc = syscall(__NR_access, sample, F_OK);
> + printf("%saccess(\"%s\", F_OK) = %ld %s (%m)\n",
> + SELINUX_MYCONTEXT(),
> + sample,
> + rc, errno2name());
Since you used sprintrc(rc) in the previous printf, you can use it here as well.
errno2name() was introduced before sprintrc(), in most cases where you
can see errno2name() it's just an old test that hasn't been converted
to sprintrc().
This applies to other tests, too.
> diff --git a/tests/selinux.c b/tests/selinux.c
> new file mode 100644
> index 000000000..9335f0534
> --- /dev/null
> +++ b/tests/selinux.c
> @@ -0,0 +1,192 @@
> +#include <assert.h>
> +#include <errno.h>
> +#include <limits.h>
> +#include <selinux/selinux.h>
> +#include <stdio.h>
> +#include <stdlib.h>
> +#include <string.h>
> +#include <unistd.h>
> +#include <xmalloc.h>
Since xmalloc.h is not a system header, please use "xmalloc.h".
[...]
> +char *
> +context_format(char *context, const char *fmt)
> +{
> + char *res;
> + int saved_errno = errno;
> +
> + if (context == NULL) {
> + res = strdup("");
In this and other places, please use xstrdup.
> + errno = saved_errno;
> + return res;
> + }
> +
> + res = xasprintf(fmt, context);
> + free(context);
> +
> + errno = saved_errno;
> + return res;
This code could be simplified, e.g.:
int saved_errno = errno;
char *res = context ? xasprintf(fmt, context) : xstrdup("");
free(context);
errno = saved_errno;
return res;
> +}
> +
> +char *
> +get_file_context_full(const char *filename)
> +{
> + int res;
> + char *secontext;
> + int saved_errno = errno;
> +
> + res = getfilecon(filename, &secontext);
> + if (res == -1)
> + goto fail;
> +
> + char *full_context = strdup(secontext);
> + freecon(secontext);
> +
> + errno = saved_errno;
> + return full_context;
> +
> +fail:
> + errno = saved_errno;
> + return NULL;
It seems to me that this code could be simplified, e.g.:
int saved_errno = errno;
char *full_context = NULL;
char *secontext;
if (getfilecon(filename, &secontext) == 0) {
full_context = xstrdup(secontext);
freecon(secontext);
}
errno = saved_errno;
return full_context;
This also applies to get_pid_context_full.
--
ldv
More information about the Strace-devel
mailing list