On Fri, Aug 10, 2018 at 7:01 PM Jann Horn <jannh@xxxxxxxxxx> wrote: > On Thu, Aug 9, 2018 at 4:07 AM Paul Moore <pmoore@xxxxxxxxxx> wrote: > > On Wed, Aug 8, 2018 at 9:56 PM Paul Moore <paul@xxxxxxxxxxxxxx> wrote: > > > On Mon, Aug 6, 2018 at 5:19 PM Jann Horn <jannh@xxxxxxxxxx> wrote: > > > > > > > > The intended behavior change for this patch is to reject any MLS strings > > > > that contain (trailing) garbage if p->mls_enabled is true. > > > > > > > > As suggested by Paul Moore, change mls_context_to_sid() so that the two > > > > parts of the range are extracted before the rest of the parsing. Because > > > > now we don't have to scan for two different separators simultaneously > > > > everywhere, we can actually switch to strchr() everywhere instead of the > > > > open-coded loops that scan for two separators at once. > > > > > > > > mls_context_to_sid() used to signal how much of the input string was parsed > > > > by updating `*scontext`. However, there is actually no case in which > > > > mls_context_to_sid() only parses a subset of the input and still returns > > > > a success (other than the buggy case with a second '-' in which it > > > > incorrectly claims to have consumed the entire string). Turn `scontext` > > > > into a simple pointer argument and stop redundantly checking whether the > > > > entire input was consumed in string_to_context_struct(). This also lets us > > > > remove the `scontext_len` argument from `string_to_context_struct()`. > > > > > > > > Signed-off-by: Jann Horn <jannh@xxxxxxxxxx> > > > > --- > > > > Refactored version of > > > > "[PATCH] selinux: stricter parsing in mls_context_to_sid()" based on > > > > Paul's comments. WDYT? > > > > I've thrown some inputs at it, and it seems to work. > > > > > > > > security/selinux/ss/mls.c | 178 ++++++++++++++------------------- > > > > security/selinux/ss/mls.h | 2 +- > > > > security/selinux/ss/services.c | 12 +-- > > > > 3 files changed, 82 insertions(+), 110 deletions(-) > > > > > > Thanks for the rework, this looks much better than what we currently > > > have. I have some comments/questions below ... > > > > > > > diff --git a/security/selinux/ss/mls.c b/security/selinux/ss/mls.c > > > > index 39475fb455bc..2fe459df3c85 100644 > > > > --- a/security/selinux/ss/mls.c > > > > +++ b/security/selinux/ss/mls.c > > > > @@ -218,9 +218,7 @@ int mls_context_isvalid(struct policydb *p, struct context *c) > > > > /* > > > > * Set the MLS fields in the security context structure > > > > * `context' based on the string representation in > > > > - * the string `*scontext'. Update `*scontext' to > > > > - * point to the end of the string representation of > > > > - * the MLS fields. > > > > + * the string `scontext'. > > > > * > > > > * This function modifies the string in place, inserting > > > > * NULL characters to terminate the MLS fields. > > > > @@ -235,22 +233,21 @@ int mls_context_isvalid(struct policydb *p, struct context *c) > > > > */ > > > > int mls_context_to_sid(struct policydb *pol, > > > > char oldc, > > > > - char **scontext, > > > > + char *scontext, > > > > struct context *context, > > > > struct sidtab *s, > > > > u32 def_sid) > > > > { > > > > - > > > > - char delim; > > > > - char *scontextp, *p, *rngptr; > > > > + char *sensitivity, *cur_cat, *next_cat, *rngptr; > > > > struct level_datum *levdatum; > > > > struct cat_datum *catdatum, *rngdatum; > > > > - int l, rc = -EINVAL; > > > > + int l, rc, i; > > > > + char *rangep[2]; > > > > > > > > if (!pol->mls_enabled) { > > > > - if (def_sid != SECSID_NULL && oldc) > > > > - *scontext += strlen(*scontext) + 1; > > > > - return 0; > > > > + if ((def_sid != SECSID_NULL && oldc) || (*scontext) == '\0') > > > > + return 0; > > > > + return -EINVAL; > > > > > > Why are we simply not always return 0 in the case where MLS is not > > > enabled in the policy? The mls_context_to_sid() is pretty much a > > > no-op in this case (even more so with your pat regardless of input and > > > I worry that returning EINVAL here is a deviation from current > > > behavior which could cause problems. > > > > Sorry, I was rephrasing the text above when I accidentally hit send. > > While my emails are generally a good source of typos, the above is > > pretty bad, so let me try again ... > > > > Why are we simply not always returning 0 in the case where MLS is not > > enabled in the policy? The mls_context_to_sid() function is pretty > > much a no-op in this case regardless of input and I worry that > > returning EINVAL here is a deviation from current behavior which could > > cause problems. > > Reverse call graph of mls_context_to_sid(): > > mls_context_to_sid() > mls_from_string() > selinux_audit_rule_init() > [...] > string_to_context_struct() > security_context_to_sid_core() > [...] > convert_context() > [...] > > string_to_context_struct() currently has the following code: > > rc = mls_context_to_sid(pol, oldc, &p, ctx, sidtabp, def_sid); > if (rc) > goto out; > > rc = -EINVAL; > if ((p - scontext) < scontext_len) > goto out; > > In my patch, I tried to preserve the behavior of > string_to_context_struct(), at the expense of slightly changing the > behavior of selinux_audit_rule_init(). > > But if you think that's a bad idea or unnecessary, say so and I'll change it. I'll be honest and mention that I kinda spaced on the change you made to string_to_context_struct() while I was looking over the mls_context_to_sid() changes. As you point out, I believe string_to_context_struct() will continue to work as expected for callers so that should be okay. The selinux_audit_rule_init() function is a little different, but looking at some of the callers, and thinking about it a bit more, it probably isn't a bad thing to return EINVAL as in your patch. The label isn't valid given that the system isn't in MLS mode and letting admins know that should be okay. So I guess we can leave this section of the patch as-is, but understand that we might need to tweak this if we end up breaking something important. As an aside, I believe my other comments on this patch still stand. It's a nice improvement but I think there are some other small things that need to be addressed. -- paul moore www.paul-moore.com _______________________________________________ Selinux mailing list Selinux@xxxxxxxxxxxxx To unsubscribe, send email to Selinux-leave@xxxxxxxxxxxxx. To get help, send an email containing "help" to Selinux-request@xxxxxxxxxxxxx.