From mboxrd@z Thu Jan 1 00:00:00 1970 From: Kees Cook Subject: Re: [PATCH v3 2/2] modules:capabilities: add a per-task modules autoload restriction Date: Fri, 21 Apr 2017 16:19:05 -0700 Message-ID: References: <1492640420-27345-1-git-send-email-tixxdz@gmail.com> <1492640420-27345-3-git-send-email-tixxdz@gmail.com> Mime-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Return-path: In-Reply-To: Sender: linux-api-owner-u79uwXL29TY76Z2rM5mHXA@public.gmane.org To: Andy Lutomirski Cc: Djalal Harouni , Linux Kernel Mailing List , Andrew Morton , "Serge E. Hallyn" , "kernel-hardening-ZwoEplunGu1jrUoiu81ncdBPR1lH4CV8@public.gmane.org" , LSM List , Linux API , Dongsu Park , Casey Schaufler , James Morris , Paul Moore , Tetsuo Handa , Greg Kroah-Hartman , Jonathan Corbet , Jessica Yu , Rusty Russell , Arnaldo Carvalho de Melo , Mauro List-Id: linux-api@vger.kernel.org On Wed, Apr 19, 2017 at 7:41 PM, Andy Lutomirski wrote: > On Wed, Apr 19, 2017 at 4:43 PM, Kees Cook wrote: >> On Wed, Apr 19, 2017 at 4:15 PM, Andy Lutomirski wrote: >>> On Wed, Apr 19, 2017 at 3:20 PM, Djalal Harouni 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