Proposing SELinux support in strace

[Renaud Métrich]

Hello,

Could you please review the code again?

Thanks,

Renaud.

On 11/21/20 9:08 PM, Renaud Métrich wrote:
>
> 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/20201209/67bd43dc/attachment.htm>
-------------- 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/20201209/67bd43dc/attachment.bin>