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

[Renaud Métrich]

Hello Dmitry,

Please find attached the "fixed" patch.

And inline for comments.

On 12/29/21 23:55, Dmitry V. Levin wrote:
> 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.
OK
> [...]
>> @@ -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.
Restored.
> [...]
>> @@ -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?
I fixed this as " a comma-separated list of items being ..."
> [...]
>> +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".
That cannot be: SELABEL_CTX_FILE is not a filename but an opaque number 
to select the right database. See /usr/include/selinux/label.h
>> +			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.
OK
> [...]
>> @@ -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';
My bad ... got fooled again by this weird behaviour.
>> +	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;
OK
> [...]
>> --- 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.
OK
> [...]
>> +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.
OK, but thought it was more clear.
>> +	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);
See above, it's an opaque number, not a filename.
>> +	}
>> +
>> +	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);
OK & for the rest as well.
>> +	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);
>
>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-Implement-displaying-of-expected-context-upon-mismat.patch
Type: text/x-patch
Size: 37246 bytes
Desc: not available
URL: <http://lists.strace.io/pipermail/strace-devel/attachments/20220102/a6243349/attachment.bin>
-------------- 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/20220102/a6243349/attachment-0001.bin>