selinux.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Petr Lautrbach <plautrba@redhat.com>
To: selinux@vger.kernel.org
Cc: Petr Lautrbach <plautrba@redhat.com>,
	Stephen Smalley <sds@tycho.nsa.gov>
Subject: Re: [PATCH v4] libselinux: Eliminate use of security_compute_user()
Date: Mon, 10 Feb 2020 20:10:22 +0100	[thread overview]
Message-ID: <pjdr1z28di9.fsf@redhat.com> (raw)
In-Reply-To: <5e88f99a-555c-9467-4cb4-6949b7cfdc98@tycho.nsa.gov>


Stephen Smalley <sds@tycho.nsa.gov> writes:

> On 2/10/20 1:23 PM, Petr Lautrbach wrote:
>> get_ordered_context_list() code used to ask the kernel to compute the complete
>> set of reachable contexts using /sys/fs/selinux/user aka
>> security_compute_user(). This set can be so huge so that it doesn't fit into a
>> kernel page and security_compute_user() fails. Even if it doesn't fail,
>> get_ordered_context_list() throws away the vast majority of the returned
>> contexts because they don't match anything in
>> /etc/selinux/targeted/contexts/default_contexts or
>> /etc/selinux/targeted/contexts/users/
>> get_ordered_context_list() is rewritten to compute set of contexts based on
>> /etc/selinux/targeted/contexts/users/ and
>> /etc/selinux/targeted/contexts/default_contexts files and to return only valid
>> contexts, using security_check_context(), from this set.
>> Fixes: https://github.com/SELinuxProject/selinux/issues/28
>> Signed-off-by: Petr Lautrbach <plautrba@redhat.com>
>> ---
>
>> diff --git a/libselinux/src/get_context_list.c b/libselinux/src/get_context_list.c
>> index 689e46589f30..fb53fd436650 100644
>> --- a/libselinux/src/get_context_list.c
>> +++ b/libselinux/src/get_context_list.c
>> @@ -243,23 +222,84 @@ static int get_context_order(FILE * fp,
>>   		if (*end)
>>   			*end++ = 0;
>>   -		/* Check for a match in the reachable list. */
>> -		rc = find_partialcon(reachable, nreach, start);
>> -		if (rc < 0) {
>> -			/* No match, skip it. */
>> +		/* Check whether a new context is valid */
>> +		if (SIZE_MAX - user_len < strlen(start) + 2) {
>> +			fprintf(stderr, "%s: one of partial contexts is too big\n", __FUNCTION__);
>> +			errno = EINVAL;
>> +			rc = -1;
>> +			goto out;
>> +		}
>> +		usercon_len = user_len + strlen(start) + 2;
>> +		usercon_str = malloc(usercon_len);
>> +		if (!usercon_str) {
>> +			rc = -1;
>> +			goto out;
>> +		}
>> +
>> +		/* set range from fromcon in the new usercon */
>> +		snprintf(usercon_str, usercon_len, "%s:%s", user, start);
>> +		usercon = context_new(usercon_str);
>> +		if (!usercon) {
>> +			if (errno != EINVAL) {
>> +				free(usercon_str);
>> +				rc = -1;
>> +				goto out;
>> +			}
>> +			fprintf(stderr,
>> +				"%s: can't create a context from %s, skipping\n",
>> +				__FUNCTION__, usercon_str);
>> +			free(usercon_str);
>>   			start = end;
>>   			continue;
>
> Feel free to make this a fatal error too; I can't see a valid scenario for it.
> The only cases where context_new() can fail are a syntactically invalid context
> or out of memory.

My idea was that if there's a wrong entry, it would be better to skip it
and try to parse and use the rest.

>>   		}
>
> I think we could lift the free(usercon_str); to here or even immediately after
> the context_new() if we stopped including it in the error message above.  Then
> we don't have to repeat it below multiple times.

I'd like to preserve this string in the error message as it can help users with
investigation problem when there's a wrong syntax or typo.


>> +		if (context_range_set(usercon, fromlevel) != 0) {
>> +			free(usercon_str);
>> +			context_free(usercon);
>> +			rc = -1;
>> +			goto out;
>> +		}
>> +		free(usercon_str);
>> +		usercon_str = context_str(usercon);
>> +		if (!usercon_str) {
>> +			context_free(usercon);
>> +			rc = -1;
>> +			goto out;
>> +		}
>>   -		/* If a match is found and the entry is not already ordered
>> -		   (e.g. due to prior match in prior config file), then set
>> -		   the ordering for it. */
>> -		i = rc;
>> -		if (ordering[i] == nreach)
>> -			ordering[i] = (*nordered)++;
>> +		/* check whether usercon is already in reachable */
>> +		if (is_in_reachable(*reachable, usercon_str)) {
>> +			start = end;
>
> Still need a context_free(usercon); here in order to avoid leaking its memory.
>

I'm sorry I missed it. It will be fixed in the next patch version (hopefully)

>> +			continue;
>
> [...]


-- 
()  ascii ribbon campaign - against html e-mail 
/\  www.asciiribbon.org   - against proprietary attachments


      reply	other threads:[~2020-02-10 19:10 UTC|newest]

Thread overview: 3+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-02-10 18:23 [PATCH v4] libselinux: Eliminate use of security_compute_user() Petr Lautrbach
2020-02-10 18:40 ` Stephen Smalley
2020-02-10 19:10   ` Petr Lautrbach [this message]

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=pjdr1z28di9.fsf@redhat.com \
    --to=plautrba@redhat.com \
    --cc=sds@tycho.nsa.gov \
    --cc=selinux@vger.kernel.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).