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

[Sebastien Buisson]

Hi,

2017-05-18 23:49 GMT+02:00 Paul Moore <paul@xxxxxxxxxxxxxx>:
> 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.

Thanks,
Sebastien.