From: Stephen Smalley <sds@tycho.nsa.gov>
To: Paul Moore <paul@paul-moore.com>, omosnace@redhat.com
Cc: selinux@vger.kernel.org, selinux@tycho.nsa.gov
Subject: Re: [PATCH v3] selinux: simplify mls_context_to_sid()
Date: Wed, 14 Nov 2018 10:23:55 -0500 [thread overview]
Message-ID: <03a3a23b-a580-8bf9-96c0-2f89b89d4166@tycho.nsa.gov> (raw)
In-Reply-To: <CAHC9VhSV0ZDYBffe+nCH7N8GZ-4ZkH6Wpt+R62Ca7kO4hYrdsQ@mail.gmail.com>
On 11/13/18 10:14 PM, Paul Moore wrote:
> On Tue, Nov 13, 2018 at 4:10 PM Stephen Smalley <sds@tycho.nsa.gov> wrote:
>> On 11/12/18 6:44 AM, Ondrej Mosnacek wrote:
>>> This function has only two callers, but only one of them actually needs
>>> the special logic at the beginning. Factoring this logic out into
>>> string_to_context_struct() allows us to drop the arguments 'oldc', 's',
>>> and 'def_sid'.
>>>
>>> Signed-off-by: Ondrej Mosnacek <omosnace@redhat.com>
>>> ---
>>>
>>> Changes in v3:
>>> - correct the comment about policy read lock
>>>
>>> Changes in v2:
>>> - also drop unneeded #include's from mls.c
>>>
>>> security/selinux/ss/mls.c | 49 +++++-----------------------------
>>> security/selinux/ss/mls.h | 5 +---
>>> security/selinux/ss/services.c | 32 +++++++++++++++++++---
>>> 3 files changed, 36 insertions(+), 50 deletions(-)
>>>
>>> diff --git a/security/selinux/ss/mls.c b/security/selinux/ss/mls.c
>>> index 2fe459df3c85..d1da928a7e77 100644
>>> --- a/security/selinux/ss/mls.c
>>> +++ b/security/selinux/ss/mls.c
>>> @@ -24,10 +24,7 @@
>>> #include <linux/string.h>
>>> #include <linux/errno.h>
>>> #include <net/netlabel.h>
>>> -#include "sidtab.h"
>>> #include "mls.h"
>>> -#include "policydb.h"
>>> -#include "services.h"
>>>
>>> /*
>>> * Return the length in bytes for the MLS fields of the
>>> @@ -223,20 +220,12 @@ int mls_context_isvalid(struct policydb *p, struct context *c)
>>> * This function modifies the string in place, inserting
>>> * NULL characters to terminate the MLS fields.
>>> *
>>> - * If a def_sid is provided and no MLS field is present,
>>> - * copy the MLS field of the associated default context.
>>> - * Used for upgraded to MLS systems where objects may lack
>>> - * MLS fields.
>>> - *
>>> - * Policy read-lock must be held for sidtab lookup.
>>> + * Policy read-lock must be held for policy data lookup.
>>> *
>>> */
>>> int mls_context_to_sid(struct policydb *pol,
>>> - char oldc,
>>> char *scontext,
>>> - struct context *context,
>>> - struct sidtab *s,
>>> - u32 def_sid)
>>> + struct context *context)
>>> {
>>> char *sensitivity, *cur_cat, *next_cat, *rngptr;
>>> struct level_datum *levdatum;
>>> @@ -244,29 +233,6 @@ int mls_context_to_sid(struct policydb *pol,
>>> int l, rc, i;
>>> char *rangep[2];
>>>
>>> - if (!pol->mls_enabled) {
>>> - if ((def_sid != SECSID_NULL && oldc) || (*scontext) == '\0')
>>> - return 0;
>>> - return -EINVAL;
>>> - }
>>> -
>>> - /*
>>> - * No MLS component to the security context, try and map to
>>> - * default if provided.
>>> - */
>>> - if (!oldc) {
>>> - struct context *defcon;
>>> -
>>> - if (def_sid == SECSID_NULL)
>>> - return -EINVAL;
>>> -
>>> - defcon = sidtab_search(s, def_sid);
>>> - if (!defcon)
>>> - return -EINVAL;
>>> -
>>> - return mls_context_cpy(context, defcon);
>>> - }
>>> -
>>> /*
>>> * If we're dealing with a range, figure out where the two parts
>>> * of the range begin.
>>> @@ -364,14 +330,11 @@ int mls_from_string(struct policydb *p, char *str, struct context *context,
>>> return -EINVAL;
>>>
>>> tmpstr = kstrdup(str, gfp_mask);
>>> - if (!tmpstr) {
>>> - rc = -ENOMEM;
>>> - } else {
>>> - rc = mls_context_to_sid(p, ':', tmpstr, context,
>>> - NULL, SECSID_NULL);
>>> - kfree(tmpstr);
>>> - }
>>> + if (!tmpstr)
>>> + return -ENOMEM;
>>>
>>> + rc = mls_context_to_sid(p, tmpstr, context);
>>> + kfree(tmpstr);
>>> return rc;
>>> }
>>>
>>> diff --git a/security/selinux/ss/mls.h b/security/selinux/ss/mls.h
>>> index 67093647576d..e2498f78e100 100644
>>> --- a/security/selinux/ss/mls.h
>>> +++ b/security/selinux/ss/mls.h
>>> @@ -33,11 +33,8 @@ int mls_range_isvalid(struct policydb *p, struct mls_range *r);
>>> int mls_level_isvalid(struct policydb *p, struct mls_level *l);
>>>
>>> int mls_context_to_sid(struct policydb *p,
>>> - char oldc,
>>> char *scontext,
>>> - struct context *context,
>>> - struct sidtab *s,
>>> - u32 def_sid);
>>> + struct context *context);
>>>
>>> int mls_from_string(struct policydb *p, char *str, struct context *context,
>>> gfp_t gfp_mask);
>>> diff --git a/security/selinux/ss/services.c b/security/selinux/ss/services.c
>>> index 12e414394530..ccad4334f99d 100644
>>> --- a/security/selinux/ss/services.c
>>> +++ b/security/selinux/ss/services.c
>>> @@ -1425,9 +1425,35 @@ static int string_to_context_struct(struct policydb *pol,
>>>
>>> ctx->type = typdatum->value;
>>>
>>> - rc = mls_context_to_sid(pol, oldc, p, ctx, sidtabp, def_sid);
>>> - if (rc)
>>> - goto out;
>>> + if (!pol->mls_enabled) {
>>> + rc = -EINVAL;
>>> + if ((def_sid == SECSID_NULL || !oldc) && (*p) != '\0')
>>> + goto out;
>>
>> I don't think this is your bug, but unless I'm mistaken, p could be OOB
>> and be dereferenced here. Seems to have been introduced by
>> 95ffe194204ae3cef88d0b59be209204bbe9b3be. Likely not caught in testing
>> since both Linux distributions and Android enable MLS to use the
>> category sets for isolation.
>
> Yep, and we should fix that in v4.20-rcX independent of this patch. I
> think if we simply remove the "(*scontext) == '\0'" from the check we
> should be okay; I believe the only time we would want to return 0 when
> not running a MLS policy would be when there is something in the MLS
> portion of the label.
Yes, that should restore the previous behavior. The desired behavior is
that if MLS is disabled, we reject the context as invalid if it has a
MLS field unless the context was fetched from an xattr, which we only
permit to provide filesystem compatibility between MLS and non-MLS systems.
next prev parent reply other threads:[~2018-11-14 15:21 UTC|newest]
Thread overview: 8+ messages / expand[flat|nested] mbox.gz Atom feed top
2018-11-12 11:44 [PATCH v3] selinux: simplify mls_context_to_sid() Ondrej Mosnacek
2018-11-13 21:12 ` Stephen Smalley
2018-11-14 3:14 ` Paul Moore
2018-11-14 8:02 ` Ondrej Mosnacek
2018-11-14 15:23 ` Stephen Smalley [this message]
2018-11-14 15:46 ` Stephen Smalley
2018-11-20 21:06 ` Paul Moore
2018-11-21 8:35 ` Ondrej Mosnacek
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=03a3a23b-a580-8bf9-96c0-2f89b89d4166@tycho.nsa.gov \
--to=sds@tycho.nsa.gov \
--cc=omosnace@redhat.com \
--cc=paul@paul-moore.com \
--cc=selinux@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).