From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S967513AbdDSXnZ (ORCPT ); Wed, 19 Apr 2017 19:43:25 -0400 Received: from mail-io0-f176.google.com ([209.85.223.176]:36538 "EHLO mail-io0-f176.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S966769AbdDSXnU (ORCPT ); Wed, 19 Apr 2017 19:43:20 -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> From: Kees Cook Date: Wed, 19 Apr 2017 16:43:18 -0700 X-Google-Sender-Auth: HjLnDh9flv8-GpUlQGoVKw4VjHg Message-ID: Subject: Re: [PATCH v3 2/2] modules:capabilities: add a per-task modules autoload restriction To: Andy Lutomirski Cc: Djalal Harouni , Linux Kernel Mailing List , Andrew Morton , "Serge E. Hallyn" , "kernel-hardening@lists.openwall.com" , 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 Carvalho Chehab , Ingo Molnar , zendyani@gmail.com, 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 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... -Kees -- Kees Cook Pixel Security 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: Wed, 19 Apr 2017 16:43:18 -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: owner-linux-security-module@vger.kernel.org To: Andy Lutomirski Cc: Djalal Harouni , Linux Kernel Mailing List , Andrew Morton , "Serge E. Hallyn" , "kernel-hardening@lists.openwall.com" , 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 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... -Kees -- Kees Cook Pixel Security From mboxrd@z Thu Jan 1 00:00:00 1970 From: keescook@chromium.org (Kees Cook) Date: Wed, 19 Apr 2017 16:43:18 -0700 Subject: [PATCH v3 2/2] modules:capabilities: add a per-task modules autoload restriction In-Reply-To: References: <1492640420-27345-1-git-send-email-tixxdz@gmail.com> <1492640420-27345-3-git-send-email-tixxdz@gmail.com> Message-ID: To: linux-security-module@vger.kernel.org List-Id: linux-security-module.vger.kernel.org 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... -Kees -- Kees Cook Pixel Security -- To unsubscribe from this list: send the line "unsubscribe linux-security-module" in the body of a message to majordomo at vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html From mboxrd@z Thu Jan 1 00:00:00 1970 MIME-Version: 1.0 Sender: keescook@google.com In-Reply-To: References: <1492640420-27345-1-git-send-email-tixxdz@gmail.com> <1492640420-27345-3-git-send-email-tixxdz@gmail.com> From: Kees Cook Date: Wed, 19 Apr 2017 16:43:18 -0700 Message-ID: Content-Type: text/plain; charset=UTF-8 Subject: [kernel-hardening] Re: [PATCH v3 2/2] modules:capabilities: add a per-task modules autoload restriction To: Andy Lutomirski Cc: Djalal Harouni , Linux Kernel Mailing List , Andrew Morton , "Serge E. Hallyn" , "kernel-hardening@lists.openwall.com" , 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 Carvalho Chehab , Ingo Molnar , zendyani@gmail.com, Peter Zijlstra List-ID: 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... -Kees -- Kees Cook Pixel Security