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