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

[Paul Moore]

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