Proposing SELinux support in strace

Dmitry V. Levin ldv at altlinux.org
Thu Mar 18 02:49:35 UTC 2021


On Wed, Mar 17, 2021 at 04:04:11PM +0100, Renaud Métrich wrote:
> Hi Dmitry, thanks again. All fixed now. Code coverage improved.
> 
> Regarding adding libselinux-dev, this didn't make a difference, so I 
> finally removed it.
> 
> Patch attached.

Great.  Let's clear remaining few documentation and formatting bits.

> Subject: [PATCH] Print SELinux contexts when enabling "--secontext[=full]"
>  (not compatible with "-c").

I'm not sure whether this level of detail in the summary is really needed.
I suggest a bit more concise wording:

  Implement --secontext[=full] option to display SELinux contexts

> This is very useful when debugging SELinux issues, in particular when a
> process runs in an unexpected context or didn't transition properly, or
> typically when a file being opened has not the proper context.

A comma is needed after "in particular".

> Parameter "full" may be used to print the full context, as shown in the
> example below:

I suggest a slightly different wording:

  When --secontext=full is specified, strace will print the complete
  context (user, role, type and category) instead of just the type which
  is printed for --secontext option, as shown in the examples below:

[...]
> To implement this, a new "--with-libselinux" automake option has been
> introduced. It defaults to "check", which means automatic support on
> SELinux aware systems.

It's a configure option rather than an automake or autoconf.

> diff --git a/doc/strace.1.in b/doc/strace.1.in
> index 05c32d902..e310e0877 100644
> --- a/doc/strace.1.in
> +++ b/doc/strace.1.in
> @@ -53,6 +53,7 @@ strace \- trace system calls and signals
>  .OM \-P path
>  .OM \-p pid
>  .OP \-\-seccomp\-bpf
> +.if '@USE_SELINUX_FALSE@'#' .OP \-\-secontext[=full]

[=full] should not be bold, consider changing the line to
.if '@USE_SELINUX_FALSE@'#' .OP \-\-secontext\fR[=\fIfull\fR]

> @@ -1440,6 +1441,13 @@ In cases when seccomp-bpf filter setup failed,
>  .B strace
>  proceeds as usual and stops traced processes on every system call.
>  .TP
> +.B \-\-secontext[=full]

Likewise, [=full] should not be bold, consider changing the line to
.BR \-\-secontext "[=" \fIfull\fR "]"

> +Print SELinux contexts of processes and files in square brackets when SELinux is available and not disabled.

The line is too long, please wrap.

> +When
> +.BR full

This should rather be
.I full

> +is specified, print the complete context (user, role, type and category) instead of just the type.

The line is too long, please wrap.

> +The current implementation has limitations, in particular it does not support Mount namespaces. In this case, no context will be printed.

I suggest a slightly different wording:

  The current implementation has limitations, in particular,
  SELinux contexts cannot be printed when
  .B strace
  and the tracee belong to different mount namespaces.

> @@ -1164,6 +1182,11 @@ extern void print_ax25_addr(const void /* ax25_address */ *addr);
>  extern void print_x25_addr(const void /* struct x25_address */ *addr);
>  extern const char *get_sockaddr_by_inode(struct tcb *, int fd, unsigned long inode);
>  extern bool print_sockaddr_by_inode(struct tcb *, int fd, unsigned long inode);
> +
> +/**
> + * Prints dirfd file descriptor and saves it in tcp->last_dirfd (used when
> + * printing SELinux contexts).
> + */

I suggest a slightly different wording:

  Prints dirfd file descriptor and saves it in tcp->last_dirfd,
  the latter is used when printing SELinux contexts.

> diff --git a/src/dirent.c b/src/dirent.c
> index 3c2c09e9b..ca56f15cd 100644
> --- a/src/dirent.c
> +++ b/src/dirent.c
> @@ -106,6 +106,9 @@ SYS_FUNC(readdir)
>  	if (entering(tcp)) {
>  		/* fd */
>  		printfd(tcp, tcp->u_arg[0]);
> +#ifdef USE_SELINUX
> +		tcp->last_dirfd = (int)(tcp->u_arg[0]);

We usually format casts this way:
		tcp->last_dirfd = (int) tcp->u_arg[0];

> diff --git a/src/xgetdents.c b/src/xgetdents.c
> index 4de5fbc10..6a0144bbf 100644
> --- a/src/xgetdents.c
> +++ b/src/xgetdents.c
> @@ -122,6 +122,9 @@ xgetdents(struct tcb *const tcp, const unsigned int header_size,
>  {
>  	if (entering(tcp)) {
>  		printfd(tcp, tcp->u_arg[0]);
> +#ifdef USE_SELINUX
> +		tcp->last_dirfd = (int)(tcp->u_arg[0]);

Likewise.

> @@ -445,6 +455,14 @@ Miscellaneous:\n\
>    -d, --debug    enable debug output to stderr\n\
>    -h, --help     print help message\n\
>    --seccomp-bpf  enable seccomp-bpf filtering\n\
> +"
> +#ifdef USE_SELINUX
> +"\
> +  --secontext[=full]\n\
> +                 print SELinux contexts (type only unless 'full' is specified)\n\
> +"
> +#endif
> +"\
>    -V, --version  print version\n\
>  "
>  /* ancient, no one should use it

I think this should go to the end of "Output format:" section rather
"Miscellaneous:".

> @@ -46,6 +48,7 @@ check_e '-t and --absolute-timestamps cannot be provided simultaneously' -t --ti
>  check_e '-t and --absolute-timestamps cannot be provided simultaneously' --absolute-timestamps -ttt -p $$
>  check_e '-t and --absolute-timestamps cannot be provided simultaneously' -t --timestamps=ns -t -p $$
>  check_e '-t and --absolute-timestamps cannot be provided simultaneously' --timestamps=ns -t --absolute-timestamps=unix -p $$
> +[ -n "$compiled_with_secontext" ] && check_h "invalid --secontext argument: 'ss'" --secontext=ss

We prefer writing it this way:

[ -z "$compiled_with_secontext" ] ||
	check_h "invalid --secontext argument: 'ss'" --secontext=ss

> +#define CONTEXT_FORMAT_SPACE_BEFORE(string)	context_format(string, " [%s]")
> +#define CONTEXT_FORMAT_SPACE_AFTER(string)	context_format(string, "[%s] ")
> +#define CONTEXT_FORMAT_SPACE_BEFORE_QUOTED(string)	context_format(string, " \\[%s\\]")
> +#define CONTEXT_FORMAT_SPACE_AFTER_QUOTED(string)	context_format(string, "\\[%s\\] ")
> +
> +#ifdef PRINT_SECONTEXT_FULL
> +
> +#define SELINUX_FILECONTEXT(filename)	CONTEXT_FORMAT_SPACE_BEFORE(get_file_context_full(filename))
> +#define SELINUX_PIDCONTEXT(pid)		CONTEXT_FORMAT_SPACE_AFTER(get_pid_context_full(pid))
> +#define SELINUX_FILECONTEXT_QUOTED(filename)	CONTEXT_FORMAT_SPACE_BEFORE_QUOTED(get_file_context_full(filename))
> +#define SELINUX_PIDCONTEXT_QUOTED(pid)	CONTEXT_FORMAT_SPACE_AFTER_QUOTED(get_pid_context_full(pid))
> +
> +#else
> +
> +#define SELINUX_FILECONTEXT(filename)	CONTEXT_FORMAT_SPACE_BEFORE(get_file_context_short(filename))
> +#define SELINUX_PIDCONTEXT(pid)		CONTEXT_FORMAT_SPACE_AFTER(get_pid_context_short(pid))
> +#define SELINUX_FILECONTEXT_QUOTED(filename)	CONTEXT_FORMAT_SPACE_BEFORE_QUOTED(get_file_context_short(filename))
> +#define SELINUX_PIDCONTEXT_QUOTED(pid)	CONTEXT_FORMAT_SPACE_AFTER_QUOTED(get_pid_context_short(pid))

Some of these lines are too long, please wrap.

> +	char *newcontext = xasprintf("%s:%s:%s:%s", split[0], split[1], newtype, split[3]);

Likewise.


-- 
ldv


More information about the Strace-devel mailing list