Proposing SELinux support in strace

[Dmitry V. Levin]

On Mon, Mar 29, 2021 at 05:36:42PM +0200, Renaud Métrich wrote:
> Thanks, all fixed, PR updated accordindly.
> 
> I introduced stripping of trailing newlines in the context (see 
> selinux.c), after digging into a CI issue for a few hours:
> 
> for some reason, on the CI, the context for processes (there is no file 
> contexts at all) have a trailing "\n" (actually it's "unconfined\n")!
> 
> I think it's safe to keep this stripping in the normal code, just in 
> case, that would avoid unintended newlines in strace output.

I also think it's OK.

Here is the next bunch of comments.

[...]
> +	if (*result == NULL) {
> +		char *res = xstrdup(*secontext);
> +		/*
> +		 * On the CI at least, the context may have a trailing \n,
> +		 * let's remove it just in case
> +		 */
> +		int index;
> +		for (index = strlen(res) - 1; index >= 0; index--) {
> +			if (res[index] != '\n')
> +				break;
> +			res[index] = '\0';
> +		}
> +		*result = res;

I don't feel comfortable with assigning the result of strlen(arbitrary string)
to a variable of type int.

How about this:
		size_t len = strlen(*secontext);
		for (; len > 0; --len) {
			if ((*secontext)[len - 1] != '\n')
				break;
		}
		*result = xstrndup(*secontext, len);

[...]
> diff --git a/tests/access.c b/tests/access.c
> index 8bdbb6265..b864993f5 100644
> --- a/tests/access.c
> +++ b/tests/access.c
> @@ -1,5 +1,5 @@
>  /*
> - * Copyright (c) 2016-2019 The strace developers.
> + * Copyright (c) 2016-2021 The strace developers.
>   * All rights reserved.
>   *
>   * SPDX-License-Identifier: GPL-2.0-or-later
> @@ -10,21 +10,36 @@
>  
>  #ifdef __NR_access
>  
> -# include <stdio.h>
> -# include <unistd.h>
> +#include <fcntl.h>
> +#include <stdio.h>
> +#include <unistd.h>
> +
> +#include "selinux.c"

We don't just prefer to indent preprocessor directives,
we actually fix preprocessor indentation before the release,
see e.g. commit v5.11~2.
Could you bring the indentation back, please?

> diff --git a/tests/faccessat.test b/tests/faccessat.test
> index 83f94ba40..2df711d3b 100755
> --- a/tests/faccessat.test
> +++ b/tests/faccessat.test
> @@ -14,6 +14,8 @@ check_prog sed
>  run_prog > /dev/null
>  run_strace -a23 --trace=faccessat "$@" $args > "$EXP"
>  
> -# Filter out faccessat() calls made by ld.so and libc.
> -sed -n '/^faccessat(-1, NULL,/,$p' < "$LOG" > "$OUT"
> -match_diff "$OUT" "$EXP"
> +# Removed this filtering since it breaks when enabling --secontext.
> +# It looks like it's due to having square brackets in the lines ...
> +## Filter out faccessat() calls made by ld.so and libc.
> +#sed -n '/faccessat(-1, NULL,/,$p' < "$LOG" > "$OUT"
> +match_diff "$LOG" "$EXP"

Sorry but this doesn't work properly: there are architectures like
aarch64 that don't have access() syscall so glibc uses
faccessat() there instead.

As result, all tests based on this faccessat.test fail on aarch64, e.g.
FAIL: faccessat
[...]
--- exp 2021-03-30 01:08:37.645513400 +0000
+++ log 2021-03-30 01:08:37.645513400 +0000
@@ -1,3 +1,4 @@
+faccessat(AT_FDCWD, "/etc/ld.so.preload", R_OK) = -1 ENOENT (No such file or directory)
 faccessat(-1, NULL, F_OK) = -1 EFAULT (Bad address)
 faccessat(-1, NULL, R_OK) = -1 EFAULT (Bad address)
 faccessat(-1, NULL, W_OK) = -1 EFAULT (Bad address)


-- 
ldv