Messages in this thread | | | From | Andy Lutomirski <> | Date | Wed, 19 Apr 2017 19:41:44 -0700 | Subject | Re: [PATCH v3 2/2] modules:capabilities: add a per-task modules autoload restriction |
| |
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.
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.
| |