Messages in this thread | | | From | Kees Cook <> | Date | Fri, 21 Apr 2017 16:19:05 -0700 | Subject | Re: [PATCH v3 2/2] modules:capabilities: add a per-task modules autoload restriction |
| |
On Wed, Apr 19, 2017 at 7:41 PM, Andy Lutomirski <luto@kernel.org> wrote: > On Wed, Apr 19, 2017 at 4:43 PM, Kees Cook <keescook@chromium.org> wrote: >> On Wed, Apr 19, 2017 at 4:15 PM, Andy Lutomirski <luto@kernel.org> wrote: >>> On Wed, Apr 19, 2017 at 3:20 PM, Djalal Harouni <tixxdz@gmail.com> wrote: >>>> +/* Sets task's modules_autoload */ >>>> +static inline int task_set_modules_autoload(struct task_struct *task, >>>> + unsigned long value) >>>> +{ >>>> + if (value > MODULES_AUTOLOAD_DISABLED) >>>> + return -EINVAL; >>>> + else if (task->modules_autoload > value) >>>> + return -EPERM; >>>> + else if (task->modules_autoload < value) >>>> + task->modules_autoload = value; >>>> + >>>> + return 0; >>>> +} >>> >>> This needs to be more locked down. Otherwise someone could set this >>> and then run a setuid program. Admittedly, it would be quite odd if >>> this particular thing causes a problem, but the issue exists >>> nonetheless. >> >> Eeeh, I don't agree this needs to be changed. APIs provided by modules >> are different than the existing privilege-manipulation syscalls this >> concern stems from. Applications are already forced to deal with >> things being missing like this in the face of it simply not being >> built into the kernel. >> >> Having to hide this behind nnp seems like it'd reduce its utility... >> > > I think that adding an inherited boolean to task_struct that can be > set by unprivileged tasks and passed to privileged tasks is a terrible > precedent. Ideally someone would try to find all the existing things > like this and kill them off.
(Tristate, not boolean, but yeah.)
I see two others besides seccomp and nnp:
PR_MCE_KILL PR_SET_THP_DISABLE
I really don't think this needs nnp protection.
> I agree that I don't see how one would exploit this particular > feature, but I still think I dislike the approach. This is a slippery > slope to adding a boolean for perf_event_open(), unshare(), etc, and > we should solve these for real rather than half-arsing them IMO.
I disagree (obviously); this would be protecting the entire module autoload attack surface. That's hardly a specific control, and it's a demonstrably needed flag.
-Kees
-- Kees Cook Pixel Security
| |