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

From: Paul Moore <paul@paul-moore.com>
To: Sebastien Buisson <sbuisson.ddn@gmail.com>
Cc: Stephen Smalley <sds@tycho.nsa.gov>,
	James Morris <james.l.morris@oracle.com>,
	linux-security-module@vger.kernel.org,
	Sebastien Buisson <sbuisson@ddn.com>,
	linux-kernel@vger.kernel.org, selinux@tycho.nsa.gov
Subject: Re: [PATCH v6 1/2] selinux: add brief info to policydb
Date: Tue, 23 May 2017 15:11:30 -0400	[thread overview]
Message-ID: <CAHC9VhTBSfNO+st9S+wCvy5KQVxQ1U1CBJN1Sec9Jh=pb6VT3A@mail.gmail.com> (raw)
In-Reply-To: <CAPkE-bW_dLo2hoE+rA_sOgZnqcZD_9L1RiDvyDBAtxJm3AM5Wg@mail.gmail.com>

On Tue, May 23, 2017 at 12:29 PM, Sebastien Buisson
<sbuisson.ddn@gmail.com> wrote:
> Hi,

Hello.

> 2017-05-18 23:49 GMT+02:00 Paul Moore <paul@paul-moore.com>:
>> My apologies to you and Sebastien for not reviewing these patches sooner.
>
> It is ok, no problem.
> Thanks for all the advice from you and Stephen. I will try to take all
> this into account.

Thanks for your patience and understanding.

> As I understand it, I should not give the choice to allocate or not
> the string returned by security_policydb_brief(). The initial reason
> for this was that Lustre client code is expected to retrieve policy
> brief info hundreds or thousands of times per second, so saving on
> memory allocation would make sense. So if security_policydb_brief()
> necessarily allocates memory for the string returned, and I appreciate
> it helps maintenance and avoids complexity, it should not be called so
> often.
> One way to tackle this is to rely on the notification system: Lustre
> client code would call security_policydb_brief() only when it gets a
> change notification, and stores the current policy brief info
> internally.

This approach would not only save on the memory allocation, but it
would also save on the memory copy.  Granted, it's not a large chunk
of memory, but it is something.

FWIW, the notification code is now present in the selinux/next branch,
see commit 8f408ab64be6 ("selinux lsm IB/core: Implement LSM
notification system"); although you may need to augment the SELinux
code to generate a LSM_POLICY_CHANGE event when the state of
checkreqprot changes.  Presumably you could also update the
LSM_POLICY_CHANGE event so it sends the policy brief as the lone
argument to save you an additional call into the LSM.

> Another way could be to add another hook to check policy brief info
> validity. It would take a string as an input parameter, and return 0
> if it matches the current policy. So Lustre client code would
> systematically call this hook, and only call security_policydb_brief()
> when the policy has changed, to store the current value internally.

I'm not sure I like this approach as much as the one above, for a
variety of reasons.  Is this option more desirable from a Lustre point
of view?

> I have recently identified a new need from Lustre client code. We need
> to protect against the case where the policy is changed or set in
> permissive mode, and then set back to its previous state, to
> workaround policy check as carried out on server side based on policy
> brief info sent by client. In this scenario, the policy would only be
> the expected one by the time the client sends a request to the server
> (for instance a file open request), but not after that when SELinux
> actually checks the permissions on the client (via
> security_file_open() in this example).

I'm not sure I completely understand what you are trying to describe,
but more information is below.

> A solution to address this could be to add a new parameter to
> security_policydb_brief() hook, in the form of a pointer to an integer
> giving the current sequence number of the policy. That would
> complement the policy brief info, with the notion of change to the
> policy. I do not think it is desirable to include the sequence number
> in the policy brief info, as it is not the essence of the policy.
> Now with this sequence info in mind, the new hook to check policy
> brief info validity would only need to check the sequence, instead of
> the policy brief string. The current value of the sequence info should
> be stored by Lustre internally, and checked after SELinux permission
> checks. If a change is detected, Lustre client must stop normal
> processing and return an error for the current request.

Assuming that the Lustre code makes use of the LSM notification
system, it seems reasonable that Lustre could maintain it's own
sequence number (or whatever best suits the problem) and increment
that number whenever it receives a LSM_POLICY_CHANGE notification.

-- 
paul moore
www.paul-moore.com