Proposing SELinux support in strace

[Renaud Métrich]

Hi Dmitry,

Thank you for the review, I rewrote part of the code based on your comments.

> diff --git a/m4/st_selinux.m4 b/m4/st_selinux.m4
> new file mode 100644
> index 000000000..309b4a096
> --- /dev/null
> +++ b/m4/st_selinux.m4
> @@ -0,0 +1,68 @@
> +#!/usr/bin/m4
> +#
> +# Copyright (c) 2020 The strace developers.
> +# All rights reserved.
> +#
> +# SPDX-License-Identifier: LGPL-2.1-or-later
> +
> +AC_DEFUN([st_SELINUX], [dnl
> +
> +AC_ARG_WITH([libselinux],
> +	    [AS_HELP_STRING([--with-libselinux],
> +			    [use libselinux to collect security contexts])],
> +	    [case "${withval}" in
> +	     yes|no|check) ;;
> +	     *) with_libselinux=yes
> +		libselinux_CPPFLAGS="-I${withval}/include"
> +		libselinux_LDFLAGS="-L${withval}/lib" ;;
> +	     esac],
> +	    [with_libselinux=check]
> +)
> +
> +libselinux_CPPFLAGS=
> +libselinux_LDFLAGS=
> +libselinux_LIBS=
> The initialization of libselinux_* after their first potential use
> looks really odd.
>
I agree this was weird, in fact I copy/pasted the code from 
st_demangle.m4 which does similar affectation and didn't try to really 
understand how this worked.

I moved the initializatino before now and seems it provides same result.

> +use_libselinux=no
> +
> +AS_IF([test "x$with_libselinux" != xno],
> +      [saved_CPPFLAGS="$CPPFLAGS"
> +       CPPFLAGS="$CPPFLAGS $libselinux_CPPFLAGS"
> +       found_selinux_h=no
> +       AC_CHECK_HEADERS([selinux.h selinux/selinux.h],
> Do you really want to support selinux.h header?  If yes, than you cannot
> use selinux.h as the name for the strace header file, and you have to
> check which one of these two headers exist.  If unsure, just stick with
> selinux/selinux.h.
>
Since I wasn't sure if "selinux.h" could reside in another directory 
than selinux/ I added this. For sure having "selinux.h" as the name for 
the strace header file doesn't change anything since at configure step 
it doesn't seem to be sourced. Anyway I've removed "selinux.h" here.

> +bool selinux_context = false;
> +bool selinux_context_typeonly = true;
> I suggest changing selinux_context_typeonly to selinux_context_full
> and assigning it to false.
OK

> The only difference between selinux_getpidcon and selinux_getfilecon
> implementations is just one line of code:
>
> -	if (selinux_context && !getpidcon(pid, &secontext)) {
> +	if (getfilecon(path, &secontext) != -1) {
>
> I urge you to create a helper function and move everything except
> the getpidcon/getfilecon invocation there.
I create a helpers functions "selinux_getpidcon" and 
"selinux_getfilecon" that now call internally a unique function 
"getcontext" that does the work.

> We have a local sleep executable, please use
> run_prog ../sleep 0
> instead.
I cannot use "../sleep" because the test uses "runcon" to for executing 
as "initrc_t" context, which requires having a "standard label" (bin_t, 
etc.) which isn't the case for "../sleep" which has the context of the 
location of the GIT, e.g. "user_home_t" if it's in your local home 
directory.
> +cat >> "$EXP" << __EOF__
> +\\[$processctx\\] execve\\("$(which sleep)" \\[$(filecontext $(which sleep) | typeonly)\\], .*
> +__EOF__
> +
> +
> +for lib in "/etc/ld.so.cache" "/lib64/libc.so.6"; do
> This is fragile, and /lib64/libc.so.6 is very non-portable.
> Do you really need to use them?
The test initially checks the context of the process at "execve" and 
context of the executed file ("sleep").

Here I'm just adding some more context checks open "openat" syscall as 
well for robustness. These checks won't happen if the libraries do not 
exist. Additionally they do not check that libc uses openat, but that 
"runcon" and "sleep" use "openat" on ld.so.cache and libc.so.6.

>> +
>> +match_grep "$LOG" "$EXP"
> match_grep based tests are usually more fragile than match_diff based,
> please try to design a test that uses match_diff.
I don't see here how I could use match_diff because I'm not checking the 
whole strace, I just checking some of the "execve" and "openat" syscalls 
we can see.

I'm using "openat" because it's the only way (apart from "execve") to 
print some SELinux contexts for paths with "sleep".

> By the way, is it correct to hook selinux_getfilecon into printpathn?
I agree it's kind of a "hack", using "printpathn" is just the simplest 
way to get SELinux contexts when a path is used.
>
> Also, do you want to display secontext associated with file descriptors?
Thanks to hooking "printpathn", the context for file descriptors will 
also be printed, e.g.:

[unconfined_t] ... read(3</usr/lib64/libselinux.so.1> [lib_t], 
"\177ELF\2\1\1\0\0\0\0\0\0\0\0\0\3\0>\0\1\0\0\0\320\207\0\0\0\0\0\0"..., 
832) = 832 <0.000015>

That's why hooking "printpathn" is great here.

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.strace.io/pipermail/strace-devel/attachments/20201121/c5c5de54/attachment.htm>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: OpenPGP_0x5D129094FB6E4326.asc
Type: application/pgp-keys
Size: 3087 bytes
Desc: not available
URL: <http://lists.strace.io/pipermail/strace-devel/attachments/20201121/c5c5de54/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/20201121/c5c5de54/attachment-0001.bin>