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