Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1427129AbdDVANj (ORCPT ); Fri, 21 Apr 2017 20:13:39 -0400 Received: from nm24.bullet.mail.ne1.yahoo.com ([98.138.90.87]:44626 "EHLO nm24.bullet.mail.ne1.yahoo.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1426701AbdDVANf (ORCPT ); Fri, 21 Apr 2017 20:13:35 -0400 X-Yahoo-Newman-Id: 630966.72239.bm@smtp221.mail.ne1.yahoo.com X-Yahoo-Newman-Property: ymail-3 X-YMail-OSG: lanIIhcVM1kM_8fjtZ7niRL2aRvN_fDBXHXTwqiUhM7zj6f y1Tc5PIvRyX.Lgv4e2TJvc19kVEe3L51.v9eMW6PElzJ.75ZhGBzzL.NZavr KBvm50GkckJNoDW2yuOQh3hQum6EP5sworp2APSHVuj8ihvdSwKLw4KAwt_t WEVompTzoMgzSkVNi4F6r02ojtDkGdlthGuJR7GmSoxihWlBpjHJ2s0JZ1dh bjrUdis4PQZyTO9ybREvqZ_RIrMR19NR73u25lebG1cWifLULVIVakTpyTxs NlaOlWsGyB8Z1caYW9BiAAlhOLaGcXQodHFhx7Te5FSNYlVLG_gmrFFZQOBY x14utKckvqPTSzAAoay2De4GwzLeBwS4Qv53ADd2B8NiIAWshY4qDxtelDrs DHCrTQzjjdrtHxDqq9NLo0OhQfjZDl0qHQy83l8CMzlEC9Qg4b.0ScJzwOI8 CCGsnFvprMRwCx2HSFe2lrNuTeY_ysB00DZdbGpMkkESFeXSIzz1ldPkUFhv N3jmk6TZzwmct X-Yahoo-SMTP: OIJXglSswBDfgLtXluJ6wiAYv6_cnw-- Subject: Re: [PATCH v3 2/2] modules:capabilities: add a per-task modules autoload restriction To: Andy Lutomirski 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> Cc: 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 From: Casey Schaufler Message-ID: Date: Fri, 21 Apr 2017 17:13:21 -0700 User-Agent: Mozilla/5.0 (Windows NT 10.0; WOW64; rv:45.0) Gecko/20100101 Thunderbird/45.8.0 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 5403 Lines: 110 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.