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

[Stephen Smalley <sds@tycho.nsa.gov>]

On Tue, 2017-05-23 at 18:29 +0200, Sebastien Buisson wrote:
> Hi,
> 
> 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.
> 
> 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.
> 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 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).
> 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.

Not sure about your threat model but I think you are fighting a losing
battle there.  A malicious admin has too many ways to defeat your
checking.

Relying on the seqno also seems brittle; you could easily end up
causing a client to fail just because a policy update happened to be
installed at the same time, even though there was nothing wrong or
malicious about the policy update itself.