findutils-selinux.patch critique (was Re: [patch] Recent findutils test suite failures)

[Stephen Smalley]

On Fri, 2005-11-25 at 09:35 +0000, Tim Waugh wrote:
> Forwarding this to a wider SELinux-knowledgeable audience.  James had
> asked me what patches we apply to findutils, so I mentioned SELinux
> and pointed him to the patch for an initial review.

IIRC, the findutils SELinux patch was originally created by a person
from MITRE who is no longer involved in the project.  Dan later rewrote
it for the new SELinux API in Linux 2.6.

> 1. I notice that SELinux-enablement creates two new tests, "-context"
>    and "--context".  I would eliminate "--context", leaving only
>    "-context".  Find predicates aren't GNU-style long options.  Find
>    is confusing enough with options as well as predicates, and so I
>    definitely don't want to further blur that line.

This suggestion seems fine.

> 2. I see that if you use "find -context" on an SELinux-enabled version
>    of find on a system where SELinux is not enabled, find will
>    immediately perform a fatal exit with an explanatory message.  I
>    won't say that that is definitely the wrong behaviour, but why did
>    you choose not to have -context simply always return false (and
>    perhaps issue one warning message)?

This may be a legacy issue.  Since getfilecon is implemented via
getxattr(2), and getxattr(2) does not require SELinux to be enabled, we
might want to drop this restriction.  The older SELinux API (before
migrating to using xattrs) would only have worked if SELinux was
enabled.

> 3. Security contexts are long multi-part strings.  Matching with glob
>    patterns is a reasonable approach.  Nevertheless, does the use of
>    glob patterns to match them reflect practice on other systems?

I'm not sure what we would compare against in terms of other systems
(Trusted Solaris?), or exactly how they would compare.  Some kind of
pattern matching (regex or glob patterns) seems necessary to deal with
searching for e.g. a particular TE type without being concerned about
other fields.

> 4. I notice that your patch slips in a definition for FNM_CASEFOLD.
>    That looks wrong to me.  gl_FUNC_FNMATCH_GNU should call
>    AC_GNU_SOURCE to ensure that <fnmatch.h> #defines FNM_CASEFOLD for
>    you.  This should be called by gl_INIT.  Is this (a) not working
>    correctly on some of your build systems, (b) not working on any of
>    your build systems, implying a findutils bug, or (c) there for
>    'historical reasons'?

I don't know - looks like you added these lines earlier this year upon
merging 4.2.15.

> 5. Findutils should probably default to compiling with SELinux support
>    on systems where <selinux/selinux.h> exists or --with-selinux is
>    specifified to configure, and compile without such support
>    otherwise.

Seems reasonable.

> 6. Is there a reason you turn off Automake gnits rules checking in the
>    SELinux patch?

No ideas here - looks like a change by Dan very recently from the cvs
log.

> 7. The configure magic for selinux can be fully generic and
>    self-contained I think: we could just have an selinux.m4 file.
>    That would also make it much easier to reuse it.

Seems fine.


> 8. Are the DejaGnu test cases for the SELinux patch in another patch
>    file somewhere else?  Perhaps it's in the same patch file as the
>    update to the Texinfo documentation. :) 

Likely doesn't exist yet.

-- 
Stephen Smalley
National Security Agency