On Wed, Apr 17, 2019 at 12:27 PM Oleg Nesterov <oleg@xxxxxxxxxx> wrote: > On 04/17, Paul Moore wrote: > > > > On Wed, Apr 17, 2019 at 10:57 AM Oleg Nesterov <oleg@xxxxxxxxxx> wrote: > > > On 04/17, Paul Moore wrote: > > > > > > > > I'm tempted to simply return an error in selinux_setprocattr() if > > > > the task's credentials are not the same as its real_cred; > > > > > > What about other modules? I have no idea what smack_setprocattr() is, > > > but it too does prepare_creds/commit creds. > > > > > > it seems that the simplest workaround should simply add the additional > > > cred == real_cred into proc_pid_attr_write(). > > > > Yes, that is simple, but I worry about what other LSMs might want to > > do. While I believe failing if the effective creds are not the same > > as the real_creds is okay for SELinux (possibly Smack too), I worry > > about what other LSMs may want to do. After all, > > proc_pid_attr_write() doesn't change the the creds itself, that is > > something the specific LSMs do. > > Yes, but if proc_pid_attr_write() is called with cred != real_cred then > something is already wrong? True, or at least I would think so. Looking at the current tree there are three LSMs which implement setprocattr hooks: SELinux, Smack, and AppArmor. I know Casey has already mentioned that he wasn't able to trigger the problem in Smack, but looking at smack_setprocattr() I see the similar commit_creds() usage so I would expect the same problem in Smack; what say you Casey? Looking at apparmor_setprocattr(), it appears that it too could end up calling commit_creds() via aa_set_current_hat(). Since it looks like all three LSMs which implement the setprocattr hook are vulnerable I'm open to the idea that proc_pid_attr_write() is a better choice for the cred != read_cred check, but I would want to make sure John and Casey are okay with that. John? Casey? -- paul moore www.paul-moore.com
