Proposing SELinux support in strace

[Renaud Métrich]

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.

Renaud.

On 3/29/21 2:50 AM, Dmitry V. Levin wrote:
> On Fri, Mar 26, 2021 at 10:16:27AM +0100, Renaud Métrich wrote:
>> Thanks, fixed. I additionally handled the buffer too small case, just in
>> case ...
>>
>> I didn't merge de commits yet, to keep track of changes, will do once
>> ready to merge.
> Thanks.  There are several style issues, see below.
>
> [...]
> diff --git a/tests/access--secontext.c b/tests/access--secontext.c
> new file mode 100644
> index 000000000..01fd12e35
> --- /dev/null
> +++ b/tests/access--secontext.c
> @@ -0,0 +1,20 @@
> +/*
> + * Copyright (c) 2021 The strace developers.
> + * All rights reserved.
> + *
> + * SPDX-License-Identifier: GPL-2.0-or-later
> + */
> +
> +#include "tests.h"
> +
> +#define TEST_SECONTEXT
> +
> +#ifndef HAVE_SELINUX_RUNTIME
> +
> +SKIP_MAIN_UNDEFINED("HAVE_SELINUX_RUNTIME")
> +
> +#else
> +
> +#include "access.c"
> +
> +#endif
>
> There are several minor style issues here (and in other similar files),
> I'd prefer if it was written this way:
>
> #include "tests.h"
>
> #ifdef HAVE_SELINUX_RUNTIME
>
> # define TEST_SECONTEXT
> # include "access.c"
>
> #else
>
> SKIP_MAIN_UNDEFINED("HAVE_SELINUX_RUNTIME")
>
> #endif
>
> [...]
> diff --git a/tests/dirfd.c b/tests/dirfd.c
> new file mode 100644
> index 000000000..517ddea0c
> --- /dev/null
> +++ b/tests/dirfd.c
> @@ -0,0 +1,47 @@
> +/*
> + * Copyright (c) 2021 The strace developers.
> + * All rights reserved.
> + *
> + * SPDX-License-Identifier: GPL-2.0-or-later
> + */
> +
> +#include "tests.h"
> +
> +#include <limits.h>
> +#include <unistd.h>
> +#include <sys/types.h>
>
> I don't think we need to include <sys/types.h> explicitly here.
> Note that in man-pages-5.11 a lot of redundant <sys/types.h> were removed.
>
> +#include <dirent.h>
> +
> +#include "xmalloc.h"
> +
> +int
> +get_dir_fd(const char *dir_path)
> +{
> +	DIR *dir = NULL;
> +	dir = opendir(dir_path);
>
> Let's merge the last two lines.
>
> +	if (dir == NULL)
> +		perror_msg_and_fail("opendir");
>
> Let's mention dir_path in the diagnostics.
>
> +	int dfd = dirfd(dir);
> +	if (dfd == -1)
> +		perror_msg_and_fail("dirfd");
> +	return dfd;
> +}
> +
> +int
> +get_curdir_fd(char **curdir)
> +{
> +	int res = get_dir_fd(".");
> +
> +	if (curdir != NULL) {
> +		char *buf = xmalloc(PATH_MAX);
> +		ssize_t n = readlink("/proc/self/cwd", buf, PATH_MAX);
> +		if (n == -1)
> +			perror_msg_and_skip("/proc is not available");
>
> Let's change the last line to
> 			perror_msg_and_skip("readlink: %s", "/proc/self/cwd");
>
> +		if (n >= PATH_MAX)
> +			perror_msg_and_fail("buffer is too small");
>
> There is no errno defined in this case so perror_* is not suitable,
> let's change the last line to
> 			error_msg_and_fail("readlink: %s: %s",
> 					   "/proc/self/cwd",
> 					   "symlink value is too long");
>
> [...]
> diff --git a/tests/execve--secontext.c.old b/tests/execve--secontext.c.old
> new file mode 100644
> index 000000000..64c34c7b9
> --- /dev/null
> +++ b/tests/execve--secontext.c.old
> @@ -0,0 +1,42 @@
>
> This looks as a something unintended.
>
> diff --git a/tests/execve--secontext.test b/tests/execve--secontext.test
> new file mode 100755
> index 000000000..14c9f33de
> --- /dev/null
> +++ b/tests/execve--secontext.test
> @@ -0,0 +1,18 @@
> +#!/bin/sh
> +#
> +# Check execve syscall decoding.
> +#
> +# Copyright (c) 2021 The strace developers.
> +# All rights reserved.
> +#
> +# SPDX-License-Identifier: GPL-2.0-or-later
> +
> +. "${srcdir=.}/init.sh"
> +
> +check_prog grep
> +run_prog > /dev/null
> +run_strace --secontext -eexecve $args > "$EXP"
> +
> +# Filter out execve() call made by strace.
> +grep -F test.execve < "$LOG" > "$OUT"
> +match_diff "$OUT" "$EXP"
>
> This looks very similar to tests/execve.test, could you parametrize it
> instead, see e.g. tests/ioctl.test as an example.
>
> diff --git a/tests/execve--secontext_full.test b/tests/execve--secontext_full.test
> new file mode 100755
> index 000000000..4f5ac8c54
> --- /dev/null
> +++ b/tests/execve--secontext_full.test
> @@ -0,0 +1,18 @@
> +#!/bin/sh
> +#
> +# Check execve syscall decoding.
> +#
> +# Copyright (c) 2021 The strace developers.
> +# All rights reserved.
> +#
> +# SPDX-License-Identifier: GPL-2.0-or-later
> +
> +. "${srcdir=.}/init.sh"
> +
> +check_prog grep
> +run_prog > /dev/null
> +run_strace --secontext=full -eexecve $args > "$EXP"
> +
> +# Filter out execve() call made by strace.
> +grep -F test.execve < "$LOG" > "$OUT"
> +match_diff "$OUT" "$EXP"
>
> Likewise.
>
>

-------------- 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/20210329/eaa150a2/attachment.bin>