Re: [PATCH v3 1/2] selinux: add brief info to policydb

[William Roberts]

On Fri, May 12, 2017 at 3:22 PM, Paul Moore <paul@xxxxxxxxxxxxxx> wrote:
>
> On Thu, May 11, 2017 at 4:45 PM, Casey Schaufler <casey@xxxxxxxxxxxxxxxx> wrote:
> > On 5/11/2017 1:22 PM, Stephen Smalley wrote:
> >> On Thu, 2017-05-11 at 08:56 -0700, Casey Schaufler wrote:
> >>> On 5/11/2017 5:59 AM, Sebastien Buisson wrote:
> >>>> Add policybrief field to struct policydb. It holds a brief info
> >>>> of the policydb, in the following form:
> >>>> <0 or 1 for enforce>:<0 or 1 for checkreqprot>:<hashalg>=<checksum>
> >>>> Policy brief is computed every time the policy is loaded, and when
> >>>> enforce or checkreqprot are changed.
> >>>>
> >>>> Add security_policy_brief hook to give access to policy brief to
> >>>> the rest of the kernel. Lustre client makes use of this information
> >>>> to detect changes to the policy, and forward it to Lustre servers.
> >>>> Depending on how the policy is enforced on Lustre client side,
> >>>> Lustre servers can refuse connection.
> >>>>
> >>>> Signed-off-by: Sebastien Buisson <sbuisson@xxxxxxx>
> >>>> ---
> >>>>  include/linux/lsm_hooks.h           | 16 ++++++++
> >>>>  include/linux/security.h            |  7 ++++
> >>>>  security/security.c                 |  6 +++
> >>>>  security/selinux/hooks.c            |  7 ++++
> >>>>  security/selinux/include/security.h |  2 +
> >>>>  security/selinux/selinuxfs.c        |  2 +
> >>>>  security/selinux/ss/policydb.c      | 76
> >>>> +++++++++++++++++++++++++++++++++++++
> >>>>  security/selinux/ss/policydb.h      |  2 +
> >>>>  security/selinux/ss/services.c      | 62
> >>>> ++++++++++++++++++++++++++++++
> >>>>  9 files changed, 180 insertions(+)
> >>>>
> >>>> diff --git a/include/linux/lsm_hooks.h b/include/linux/lsm_hooks.h
> >>>> index 080f34e..9cac282 100644
> >>>> --- a/include/linux/lsm_hooks.h
> >>>> +++ b/include/linux/lsm_hooks.h
> >>>> @@ -1336,6 +1336,20 @@
> >>>>   * @inode we wish to get the security context of.
> >>>>   * @ctx is a pointer in which to place the allocated
> >>>> security context.
> >>>>   * @ctxlen points to the place to put the length of @ctx.
> >>>> + *
> >>>> + * Security hooks for policy brief
> >>>> + *
> >>>> + * @policy_brief:
> >>>> + *
> >>>> + * Returns a string containing a brief info of the
> >>>> policydb, in the
> >>>> + * following form:
> >>>> + * <0 or 1 for enforce>:<0 or 1 for
> >>>> checkreqprot>:<hashalg>=<checksum>
> >>>
> >>> This sure looks like SELinux specific information. If the Spiffy
> >>> security module has multiple values for enforcement (e.g. off,
> >>> soft and hard) this interface definition does not work. What is a
> >>> "checkreqprot", and what is it for?
> >>>
> >>> I expect that you have no interest (or incentive) in supporting
> >>> security modules other than SELinux, and that's OK. What's I'm
> >>> after is an interface that another security module could use if
> >>> someone where interested (or inspired) to do so.
> >>>
> >>> Rather than a string with predefined positional values (something
> >>> I was taught not to do when 1 MIPS and 1 MEG was a big computer)
> >>> you might use
> >>> "enforce=<value>:checkreqprot=<value>:hashalg=<checksum>"
> >>
> >> No objection to the above, although it makes his updating code for
> >> enforce/checkreqprot a bit uglier.
> >
> > Sure, but can you imagine trying to use find(1) if the
> > options where positional?
>
> Perhaps I'm suffering from audit induced PTSD, but I think we need to
> operate under the assumption that we are going to need to augment this
> at some point in the future (no good feature goes un-abused) and I'd
> much rather have some sort of name-value pairing to keep my sanity.  I
> also feel rather strongly that we should make it very explicit in the
> comments that the ordering of the fields in the string may change.
>
> >>> for SELinux and define @policy_brief as
> >>>
> >>>      A string containing colon separated name and value pairs
> >>>      that will be parsed and interpreted by the security module
> >>>      or modules.
> >>
> >> Actually, I'm not clear it will be parsed or interpreted by the
> >> security module(s).  I think he is just fetching it and then doing a
> >> simple comparison to check for inconsistencies between clients and the
> >> server, and optionally admins/users can read it and interpret it as
> >> they see fit.
> >
> > OK, in which case human eyes *need* the name as well as the value.
> > That, and strcmp(value, "enforce=0") is no harder than strcmp(value, "0").
>
> The initial use case may not require parsing the string, but who knows
> what will end up using this five years from now.  Once again, I agree
> with Casey, let's make sure it easily parsed and readable by admins;
> imagine this as the output of a file under /proc or /sys.
>
> >>> You already have it right for the "hashalg" field. If you want to
> >>> be really forward looking you could use names field names that
> >>> identify the security module that uses them, such as
> >>>
> >>>      "selinux/enforce=1:selinux/checkreqprot=0:selinux/MD5=8675309"
> >> That seems a bit verbose, particularly the duplicated selinux/ bit.
> >
> > True that, on the other hand
> >
> >         "selinux(enforce=1:checkreqprot=0:MD5=8675309)"
> >
> > would be harder to parse. Either works for me.
>
> I'm not sure I care too much about how the name-value pairs get
> namespaced, but considering the LSM stacking work that is happening,
> we should namespace it somehow.
>
> --
> paul moore
> www.paul-moore.com

(Take 2, clicks plain text mode) Darn, I cleaned my inbox out, and
forgot to comment. Couldn't you replace the hex string conversion and
kzalloc
with asprintf and the kernel format specifier extension phN as
documented under Documentation/printk-formats.txt.

It looks like asprintf flows down into vsnprintf and into the
pointer() routine properly.

asprintf prototype:
http://elixir.free-electrons.com/linux/latest/source/include/linux/kernel.h#L426

As an aside, I wonder if audit can ditch there hex escaping routine
and use something like *pE
as mentioned in that Documentation under "Raw buffer as an escaped string:"