Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1042060AbdDVGpx (ORCPT ); Sat, 22 Apr 2017 02:45:53 -0400 Received: from mail-ua0-f173.google.com ([209.85.217.173]:33892 "EHLO mail-ua0-f173.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1042021AbdDVGpv (ORCPT ); Sat, 22 Apr 2017 02:45:51 -0400 MIME-Version: 1.0 In-Reply-To: References: <1492640420-27345-1-git-send-email-tixxdz@gmail.com> <1492640420-27345-3-git-send-email-tixxdz@gmail.com> <5f07b121-62e1-b9a8-4faa-f6630b24ff48@schaufler-ca.com> From: Andy Lutomirski Date: Fri, 21 Apr 2017 23:45:29 -0700 Message-ID: Subject: Re: [PATCH v3 2/2] modules:capabilities: add a per-task modules autoload restriction To: Casey Schaufler Cc: Andy Lutomirski , Kees Cook , Djalal Harouni , Linux Kernel Mailing List , Andrew Morton , "Serge E. Hallyn" , "kernel-hardening@lists.openwall.com" , LSM List , Linux API , Dongsu Park , James Morris , Paul Moore , Tetsuo Handa , Greg Kroah-Hartman , Jonathan Corbet , Jessica Yu , Rusty Russell , Arnaldo Carvalho de Melo , Mauro Carvalho Chehab , Ingo Molnar , belakhdar abdeldjalil , Peter Zijlstra Content-Type: text/plain; charset=UTF-8 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 5746 Lines: 116 On Fri, Apr 21, 2017 at 5:13 PM, Casey Schaufler wrote: > On 4/21/2017 5:00 PM, Andy Lutomirski wrote: >> On Fri, Apr 21, 2017 at 4:52 PM, Casey Schaufler wrote: >>> On 4/21/2017 4:28 PM, Andy Lutomirski wrote: >>>> On Fri, Apr 21, 2017 at 4:19 PM, Kees Cook wrote: >>>>> 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 >>>> Well, that's interesting. That should presumably be reset on setuid >>>> exec or something. >>>> >>>>> PR_SET_THP_DISABLE >>>> Um. At least that's just a performance issue. >>>> >>>>> 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. >>>>> >>>> The list is just going to get longer. We should probably have controls for: >>>> >>>> - Use of perf. Unclear how fine grained they should be. >>>> >>>> - Creation of new user namespaces. Possibly also use of things like >>>> iptables without global privilege. >>>> >>>> - Ability to look up tasks owned by different uids (or maybe other >>>> tasks *at all*) by pid/tid. Conceptually, this is easy. The API is >>>> the only hard part, I think. >>>> >>>> - Ability to bind ports, maybe? >>> One of my longer term (i.e. after stacking) projects >>> is to create sensible access control on ports. Why shouldn't >>> they have owners and mode bits (or ACLs, if you prefer) >>> or real names. I kind of think we should be able to eliminate >>> the need for dbus without resorting to kdbus. >> My implicit_rights concept gives any type of access control you can >> use on inodes because they *are* inodes. So you get ACLs, etc. >> >> Brief summary for those who didn't read my old email: We add a new >> kind of filesystem object called a "right". It's a special kind of >> socket inode that can't be bound or connected but is instead created >> by a new syscall. It has a name, so "port:1234" might be a name of a >> right. >> >> To use an implicit right, you do whatever syscall you would do >> normally. The kernel looks for a right object at >> /dev/implicit_rights/. If that object exists, is a right of the >> correct type (i.e. the right's name matches ) and you have >> execute access, you win. Otherwise you lose. >> >> To avoid breaking existing distros, for things like modules_autoload, >> you would set a sysctl >> /proc/sys/kernel/required_implicit_rights/modules_autoload=1. With >> that set, to autoload a module without CAP_SYS_MODULE, you need the >> /dev/implicit_rights/modules_autoload. > > Sounds good. > >>> So I don't like the idea of treating that as a special case. >>> I'd rather see ports controlled properly. (Of course, the >>> SELinux crowd will point out they have this handled, but I >>> remain unconvinced of the overall solution) >> Agreed. But I think we should address all of these things together. > > What I don't want is to have to buy into a hundred things I > don't want in order to get the one thing I do. A General mechanism > is dandy, but I don't want to have to write a gazillion policy > lines for features I don't want in order to get a simple control. > The problem with SELinux is not the effort required to protect > what you care about, it's the effort required to do everything else. > The point is to make it super simple. chown, chmod and, if you want to get fancy, setfacl. You'll need a mkright tool, but that's trivial.