<html>
<head>
<meta http-equiv="Content-Type" content="text/html; charset=UTF-8">
</head>
<body>
<p>Hello,</p>
<p>Could you please review the code again?</p>
<p>Thanks,</p>
<p>Renaud.<br>
</p>
<div class="moz-cite-prefix">On 11/21/20 9:08 PM, Renaud Métrich
wrote:<br>
</div>
<blockquote type="cite"
cite="mid:9e17d94c-612f-3ab3-d6bd-da5d14de101b@redhat.com">
<meta http-equiv="Content-Type" content="text/html; charset=UTF-8">
<p>Hi Dmitry,</p>
<p>Thank you for the review, I rewrote part of the code based on
your comments.</p>
<blockquote type="cite">
<pre class="moz-quote-pre" wrap="">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=
</pre>
<pre class="moz-quote-pre" wrap="">The initialization of libselinux_* after their first potential use
looks really odd.
</pre>
</blockquote>
<p>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.</p>
<p>I moved the initializatino before now and seems it provides
same result.<br>
</p>
<p> </p>
<blockquote type="cite">
<pre class="moz-quote-pre" wrap="">+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],
</pre>
<pre class="moz-quote-pre" wrap="">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.
</pre>
</blockquote>
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.<br>
<p> </p>
<blockquote type="cite">
<pre class="moz-quote-pre" wrap="">+bool selinux_context = false;
+bool selinux_context_typeonly = true;
</pre>
<pre class="moz-quote-pre" wrap="">I suggest changing selinux_context_typeonly to selinux_context_full
and assigning it to false.
</pre>
</blockquote>
OK<br>
<p> </p>
<blockquote type="cite">
<pre class="moz-quote-pre" wrap="">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.</pre>
</blockquote>
I create a helpers functions "selinux_getpidcon" and
"selinux_getfilecon" that now call internally a unique function
"getcontext" that does the work.<br>
<p> </p>
<blockquote type="cite">
<pre class="moz-quote-pre" wrap="">We have a local sleep executable, please use
run_prog ../sleep 0
instead.</pre>
</blockquote>
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.<br>
<blockquote type="cite">
<pre class="moz-quote-pre" wrap="">+cat >> "$EXP" << __EOF__
+\\[$processctx\\] execve\\("$(which sleep)" \\[$(filecontext $(which sleep) | typeonly)\\], .*
+__EOF__
+
+
+for lib in "/etc/ld.so.cache" "/lib64/libc.so.6"; do
</pre>
<pre class="moz-quote-pre" wrap="">This is fragile, and /lib64/libc.so.6 is very non-portable.
Do you really need to use them?
</pre>
</blockquote>
The test initially checks the context of the process at "execve"
and context of the executed file ("sleep").
<p>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.<br>
</p>
<p> </p>
<blockquote type="cite">
<blockquote type="cite" style="color: #007cff;">
<pre class="moz-quote-pre" wrap="">+
+match_grep "$LOG" "$EXP"
</pre>
</blockquote>
<pre class="moz-quote-pre" wrap="">match_grep based tests are usually more fragile than match_diff based,
please try to design a test that uses match_diff.
</pre>
</blockquote>
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.
<p>I'm using "openat" because it's the only way (apart from
"execve") to print some SELinux contexts for paths with "sleep".</p>
<p> </p>
<blockquote type="cite">
<pre class="moz-quote-pre" wrap="">By the way, is it correct to hook selinux_getfilecon into printpathn?</pre>
</blockquote>
I agree it's kind of a "hack", using "printpathn" is just the
simplest way to get SELinux contexts when a path is used.<br>
<blockquote type="cite">
<pre class="moz-quote-pre" wrap="">Also, do you want to display secontext associated with file descriptors?
</pre>
</blockquote>
Thanks to hooking "printpathn", the context for file descriptors
will also be printed, e.g.:
<p>[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><br>
</p>
<p>That's why hooking "printpathn" is great here.<br>
</p>
</blockquote>
</body>
</html>