2017-05-23 21:53 GMT+02:00 Stephen Smalley <sds@xxxxxxxxxxxxx>: > 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. I do not have much experience in the domain, so every advice or idea is appreciated. If what I want to accomplish is implemented using the notification system, what would be the possibility for a malicious admin to change the policy or the way it is enforced without triggering a notification? Of course, the places in the SELinux kernel code from where to trigger the notification have to be thought thoroughly. > 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. I consider it is not a problem if one request must be resent, even if the policy update was legitimate. It would not disturb Lustre behavior, and it might be preferable to proceed to a resend instead of missing a potential fraud. Thanks, Sebastien.
