Proposing SELinux support in strace
Renaud Métrich
rmetrich at redhat.com
Wed Mar 17 15:04:11 UTC 2021
Hi Dmitry, thanks again. All fixed now. Code coverage improved.
Regarding adding libselinux-dev, this didn't make a difference, so I
finally removed it.
Patch attached.
On 3/17/21 2:15 AM, Dmitry V. Levin wrote:
> 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.
>
>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-Print-SELinux-contexts-when-enabling-secontext-full-.patch
Type: text/x-patch
Size: 79593 bytes
Desc: not available
URL: <http://lists.strace.io/pipermail/strace-devel/attachments/20210317/6ab1e22a/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/20210317/6ab1e22a/attachment-0001.bin>
More information about the Strace-devel
mailing list