[PATCH] --secontext: Implement displaying of expected context upon mismatch

[Dmitry V. Levin]

On Wed, Dec 29, 2021 at 02:33:06PM +0100, Renaud Métrich wrote:
> Attached is the updated patch with all comments.
[...]
> --- a/NEWS
> +++ b/NEWS
> @@ -3,6 +3,8 @@ Noteworthy changes in release ?.?? (????-??-??)
>  
>  * Improvements
>    * Updated lists of ioctl commands from Linux 5.16.
> +  * Implemented --secontext=mismatch option to find mismatches in SELinux
> +    contexts.

If you have a look at the NEWS file, you'll see that almost every
"Improvements" section ends with "Updated lists of ioctl commands" line.
Let's keep this tradition, please add your line before rather than after
that line.

[...]
> @@ -259,8 +259,9 @@ is one of
>  .BR inject ,
>  .BR status ,
>  .BR quiet " (or " silent " or " silence " or " q ),
> +.if '@ENABLE_SECONTEXT_FALSE@'#' .BR secontext ,
>  .BR decode\-fds " (or " decode\-fd ),
> -.BR decode\-pids " (or " decode\-pid ),
> +.BR decode\-pids " (or " decode\-pid )
>  or
>  .BR kvm ,
>  and

Please do not remove that serial comma.

[...]
> @@ -1166,13 +1167,33 @@ strace's namespace, too.
>  .BR \-\-decode\-pids = comm
>  Print command names for PIDs.
>  .if '@ENABLE_SECONTEXT_FALSE@'#' .TP
> -.if '@ENABLE_SECONTEXT_FALSE@'#' .BR \-\-secontext [= full ]
> +.if '@ENABLE_SECONTEXT_FALSE@'#' .BR \-\-secontext\fR[=\fIformat\fR]
> +.if '@ENABLE_SECONTEXT_FALSE@'#' .TQ
> +.if '@ENABLE_SECONTEXT_FALSE@'#' .BR \-e\ secontext\fR=\fIformat\fR
>  .if '@ENABLE_SECONTEXT_FALSE@'#' When SELinux is available and is not disabled,
>  .if '@ENABLE_SECONTEXT_FALSE@'#' print in square brackets SELinux contexts of
> -.if '@ENABLE_SECONTEXT_FALSE@'#' processes, files, and descriptors.  When
> -.if '@ENABLE_SECONTEXT_FALSE@'#' .B full
> -.if '@ENABLE_SECONTEXT_FALSE@'#' is specified, print the complete context (user,
> -.if '@ENABLE_SECONTEXT_FALSE@'#' role, type and category) instead of just the type.
> +.if '@ENABLE_SECONTEXT_FALSE@'#' processes, files, and descriptors.  The
> +.if '@ENABLE_SECONTEXT_FALSE@'#' .I format
> +.if '@ENABLE_SECONTEXT_FALSE@'#' argument is a comma-separated list of with items
> +.if '@ENABLE_SECONTEXT_FALSE@'#' being one of the following:

Sorry, a comma-separated list of what?

[...]
> +static int
> +get_expected_filecontext(const char *realpath, char **result)
> +{
> +	static struct selabel_handle *hdl = NULL;
> +	static bool disabled = false;
> +
> +	if (disabled)
> +		return -1;
> +
> +	if (!hdl) {
> +		hdl = selabel_open(SELABEL_CTX_FILE, NULL, 0);
> +		if (!hdl) {
> +			perror_msg("could not open SELinux database, disabling "
> +				   "context mismatch checking");

Just an idea, maybe it would be more useful to print the file name
instead of "SELinux database".

> +			disabled = true;
> +			return -1;
> +		}
> +	}
> +
> +	struct stat statbuf;
> +	if (stat(realpath, &statbuf) == -1) {
> +		return -1;
> +	}

When checking a syscall return code, I usually prefer "< 0" to "== -1".
In most cases there is no practical difference, so it's a matter of
style rather than correctness.

[...]
> @@ -97,7 +155,32 @@ selinux_getfdcon(pid_t pid, int fd, char **result)
>  	xsprintf(linkpath, "/proc/%u/fd/%u", proc_pid, fd);
>  
>  	char *secontext;
> -	return getcontext(getfilecon(linkpath, &secontext), &secontext, result);
> +	int rc = getcontext(getfilecon(linkpath, &secontext), &secontext, result);
> +	if (rc == -1 || !is_number_in_set(SECONTEXT_MISMATCH, secontext_set))
> +		return rc;
> +
> +	/*
> +	 * We need to resolve the path, because selabel_lookup() doesn't
> +	 * resolve anything. Using readlink() is sufficient here.
> +	 */
> +
> +	char buf[PATH_MAX];
> +	ssize_t n = readlink(linkpath, buf, sizeof(buf));
> +	if ((size_t) n >= sizeof(buf))
> +		return 0;

As readlink() does not append a terminating null byte to buf,
it has to be added explicitly, e.g.

	buf[n] = '\0';

> +	char *expected;
> +	if (get_expected_filecontext(buf, &expected) == -1)
> +		return 0;
> +	if (strcmp(expected, *result) == 0) {
> +		free(expected);
> +		return 0;
> +	}
> +	char *final_result = xasprintf("%s!!%s", *result, expected);
> +	free(*result);
> +	free(expected);
> +	*result = final_result;

Please reorder free(*result) and free(expected), it would look a bit
nicer this way:

	free(expected);
	free(*result);
	*result = final_result;

[...]
> --- a/tests/linkat.c
> +++ b/tests/linkat.c
> @@ -15,6 +15,9 @@
>  #include <stdlib.h>
>  #include <unistd.h>
>  #include <sys/stat.h>
> +#ifdef PRINT_SECONTEXT_MISMATCH
> +# include <string.h>
> +#endif

To be honest, I wouldn't bother ifdef'ing inclusion of this basic header.

[...]
> +static char *
> +raw_expected_secontext_full_file(const char *filename)
> +{
> +	int saved_errno = errno;
> +	char *secontext;
> +
> +	static struct selabel_handle *hdl = NULL;

No need to explicitly initialize static pointers with NULL.

> +	if (!hdl) {
> +		hdl = selabel_open(SELABEL_CTX_FILE, NULL, 0);
> +		if (!hdl)
> +			perror_msg_and_skip("selabel_open");

Let's add the file name to this diagnostics, e.g.

			perror_msg_and_skip("selabel_open: %s",
					    SELABEL_CTX_FILE);

> +	}
> +
> +	char *resolved = realpath(filename, NULL);
> +	if (!resolved)
> +		perror_msg_and_fail("realpath");

Likewise, let's add the file name to this diagnostics, e.g.

		perror_msg_and_fail("realpath: %s", filename);

> +	struct stat statbuf;
> +	if (stat(resolved, &statbuf) == -1)
> +		perror_msg_and_fail("stat");

Likewise, let's add the file name to this diagnostics, e.g.

		perror_msg_and_fail("stat: %s", resolved);

> +	if (selabel_lookup(hdl, &secontext, resolved, statbuf.st_mode) == -1)
> +		perror_msg_and_skip("selabel_lookup");

Likewise, let's add the file name to this diagnostics, e.g.

		perror_msg_and_skip("selabel_lookup: %s", resolved);


-- 
ldv