From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752634AbdK0WFB (ORCPT ); Mon, 27 Nov 2017 17:05:01 -0500 Received: from mail-io0-f173.google.com ([209.85.223.173]:38330 "EHLO mail-io0-f173.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751794AbdK0WE7 (ORCPT ); Mon, 27 Nov 2017 17:04:59 -0500 X-Google-Smtp-Source: AGs4zMasVvH1wgD+no/ntw3GJM37wfZmGeyXgmDo04sSHT4Mm0anVFIYD4PVvojZG77w5MWZMFZO1EBgNgBCWX73O34= MIME-Version: 1.0 In-Reply-To: References: <1511803118-2552-1-git-send-email-tixxdz@gmail.com> <1511803118-2552-6-git-send-email-tixxdz@gmail.com> From: Linus Torvalds Date: Mon, 27 Nov 2017 14:04:58 -0800 X-Google-Sender-Auth: q1oxxru2ohHmoclUr75OiFW_Vuw Message-ID: Subject: Re: [PATCH v5 next 5/5] net: modules: use request_module_cap() to load 'netdev-%s' modules To: Djalal Harouni Cc: Kees Cook , Andy Lutomirski , Andrew Morton , "Luis R. Rodriguez" , James Morris , Ben Hutchings , Solar Designer , Serge Hallyn , Jessica Yu , Rusty Russell , Linux Kernel Mailing List , LSM List , "kernel-hardening@lists.openwall.com" , Jonathan Corbet , Ingo Molnar , "David S. Miller" , Network Development , 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 Mon, Nov 27, 2017 at 1:41 PM, Djalal Harouni wrote: > > However, we are trying hard to abstract some semantics that are easy > to grasp, we are mutating capabilities and seccomp to have an > abstracted "yes/no" options for our endusers. Yes. Sadly, it looks like we actually do have users that just expect to load modules dynamically without any capabilities at all. So we can't actually disallow it by default at all, which imho makes this security option essentially useless. A security option that people can't use without breaking their system is pointless. We saw that with SELinux - people ended up just disabling it for _years_, simply because it ended up breaking so much in practice. And yes, it got fixed eventually, but at an incredibly high maintenance cost of all the crazy rules databases. > Alright, but I guess we are stuck, is there something better on how we > can do this or describe this ? So I wonder if we can perhaps look at the places that actually do "requerst_module()", and start filtering them on that basis. Some of them will already have checked for capabilities. Others clearly expect to juist work even _without_ capabilities (ie the bluetoothd case). So the whole "let's add a global config option" model is broken. There is no possible global rule. It will break things, which in turn mean that people won't turn it on (and we can't turn it on by default), which in turn makes this pointless. In other words, I really think that anything that just adds a mode flag cannot work. So instead of having one "modules_autoload_mode" thing, maybe the individual requerst_module() cases need to simply be audited. Put another way: I think the part of your patch series that does that "request_module_cap()" and makes the netdev modules use it is a good addition. It's the "mode" part I really don't agree with, because apparently we really need to default it to permissive. So how about instead: - add that "request_module_cap()" and make the networking code that already uses CAP_ADMIN_NET use it. - make "request_module()" itself default to being "request_module_cap(CAP_SYS_MODULE,..)" - make sure that when the capability check fails, we print an error message, and then for the ones that trigger, we will audit them and see if it's ok. Because that "mode" flag defaulting to off will just mean that the default case will remain the existing unsafe one, and that's bad. Opt-in really doesn't work. We've done it. Global flags for varied behavior really doesn't work. We've done that too. Different cases want different behavior, the global flag is just fundamentally broken. Linus From mboxrd@z Thu Jan 1 00:00:00 1970 From: Linus Torvalds Subject: Re: [PATCH v5 next 5/5] net: modules: use request_module_cap() to load 'netdev-%s' modules Date: Mon, 27 Nov 2017 14:04:58 -0800 Message-ID: References: <1511803118-2552-1-git-send-email-tixxdz@gmail.com> <1511803118-2552-6-git-send-email-tixxdz@gmail.com> Mime-Version: 1.0 Content-Type: text/plain; charset="UTF-8" Cc: Kees Cook , Andy Lutomirski , Andrew Morton , "Luis R. Rodriguez" , James Morris , Ben Hutchings , Solar Designer , Serge Hallyn , Jessica Yu , Rusty Russell , Linux Kernel Mailing List , LSM List , "kernel-hardening@lists.openwall.com" , Jonathan Corbet , Ingo Molnar , "David S. Miller" , Network Development , Peter Zijlstra To: Djalal Harouni Return-path: In-Reply-To: Sender: linux-kernel-owner@vger.kernel.org List-Id: netdev.vger.kernel.org On Mon, Nov 27, 2017 at 1:41 PM, Djalal Harouni wrote: > > However, we are trying hard to abstract some semantics that are easy > to grasp, we are mutating capabilities and seccomp to have an > abstracted "yes/no" options for our endusers. Yes. Sadly, it looks like we actually do have users that just expect to load modules dynamically without any capabilities at all. So we can't actually disallow it by default at all, which imho makes this security option essentially useless. A security option that people can't use without breaking their system is pointless. We saw that with SELinux - people ended up just disabling it for _years_, simply because it ended up breaking so much in practice. And yes, it got fixed eventually, but at an incredibly high maintenance cost of all the crazy rules databases. > Alright, but I guess we are stuck, is there something better on how we > can do this or describe this ? So I wonder if we can perhaps look at the places that actually do "requerst_module()", and start filtering them on that basis. Some of them will already have checked for capabilities. Others clearly expect to juist work even _without_ capabilities (ie the bluetoothd case). So the whole "let's add a global config option" model is broken. There is no possible global rule. It will break things, which in turn mean that people won't turn it on (and we can't turn it on by default), which in turn makes this pointless. In other words, I really think that anything that just adds a mode flag cannot work. So instead of having one "modules_autoload_mode" thing, maybe the individual requerst_module() cases need to simply be audited. Put another way: I think the part of your patch series that does that "request_module_cap()" and makes the netdev modules use it is a good addition. It's the "mode" part I really don't agree with, because apparently we really need to default it to permissive. So how about instead: - add that "request_module_cap()" and make the networking code that already uses CAP_ADMIN_NET use it. - make "request_module()" itself default to being "request_module_cap(CAP_SYS_MODULE,..)" - make sure that when the capability check fails, we print an error message, and then for the ones that trigger, we will audit them and see if it's ok. Because that "mode" flag defaulting to off will just mean that the default case will remain the existing unsafe one, and that's bad. Opt-in really doesn't work. We've done it. Global flags for varied behavior really doesn't work. We've done that too. Different cases want different behavior, the global flag is just fundamentally broken. Linus From mboxrd@z Thu Jan 1 00:00:00 1970 From: torvalds@linux-foundation.org (Linus Torvalds) Date: Mon, 27 Nov 2017 14:04:58 -0800 Subject: [PATCH v5 next 5/5] net: modules: use request_module_cap() to load 'netdev-%s' modules In-Reply-To: References: <1511803118-2552-1-git-send-email-tixxdz@gmail.com> <1511803118-2552-6-git-send-email-tixxdz@gmail.com> Message-ID: To: linux-security-module@vger.kernel.org List-Id: linux-security-module.vger.kernel.org On Mon, Nov 27, 2017 at 1:41 PM, Djalal Harouni wrote: > > However, we are trying hard to abstract some semantics that are easy > to grasp, we are mutating capabilities and seccomp to have an > abstracted "yes/no" options for our endusers. Yes. Sadly, it looks like we actually do have users that just expect to load modules dynamically without any capabilities at all. So we can't actually disallow it by default at all, which imho makes this security option essentially useless. A security option that people can't use without breaking their system is pointless. We saw that with SELinux - people ended up just disabling it for _years_, simply because it ended up breaking so much in practice. And yes, it got fixed eventually, but at an incredibly high maintenance cost of all the crazy rules databases. > Alright, but I guess we are stuck, is there something better on how we > can do this or describe this ? So I wonder if we can perhaps look at the places that actually do "requerst_module()", and start filtering them on that basis. Some of them will already have checked for capabilities. Others clearly expect to juist work even _without_ capabilities (ie the bluetoothd case). So the whole "let's add a global config option" model is broken. There is no possible global rule. It will break things, which in turn mean that people won't turn it on (and we can't turn it on by default), which in turn makes this pointless. In other words, I really think that anything that just adds a mode flag cannot work. So instead of having one "modules_autoload_mode" thing, maybe the individual requerst_module() cases need to simply be audited. Put another way: I think the part of your patch series that does that "request_module_cap()" and makes the netdev modules use it is a good addition. It's the "mode" part I really don't agree with, because apparently we really need to default it to permissive. So how about instead: - add that "request_module_cap()" and make the networking code that already uses CAP_ADMIN_NET use it. - make "request_module()" itself default to being "request_module_cap(CAP_SYS_MODULE,..)" - make sure that when the capability check fails, we print an error message, and then for the ones that trigger, we will audit them and see if it's ok. Because that "mode" flag defaulting to off will just mean that the default case will remain the existing unsafe one, and that's bad. Opt-in really doesn't work. We've done it. Global flags for varied behavior really doesn't work. We've done that too. Different cases want different behavior, the global flag is just fundamentally broken. Linus -- 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: linus971@gmail.com In-Reply-To: References: <1511803118-2552-1-git-send-email-tixxdz@gmail.com> <1511803118-2552-6-git-send-email-tixxdz@gmail.com> From: Linus Torvalds Date: Mon, 27 Nov 2017 14:04:58 -0800 Message-ID: Content-Type: text/plain; charset="UTF-8" Subject: [kernel-hardening] Re: [PATCH v5 next 5/5] net: modules: use request_module_cap() to load 'netdev-%s' modules To: Djalal Harouni Cc: Kees Cook , Andy Lutomirski , Andrew Morton , "Luis R. Rodriguez" , James Morris , Ben Hutchings , Solar Designer , Serge Hallyn , Jessica Yu , Rusty Russell , Linux Kernel Mailing List , LSM List , "kernel-hardening@lists.openwall.com" , Jonathan Corbet , Ingo Molnar , "David S. Miller" , Network Development , Peter Zijlstra List-ID: On Mon, Nov 27, 2017 at 1:41 PM, Djalal Harouni wrote: > > However, we are trying hard to abstract some semantics that are easy > to grasp, we are mutating capabilities and seccomp to have an > abstracted "yes/no" options for our endusers. Yes. Sadly, it looks like we actually do have users that just expect to load modules dynamically without any capabilities at all. So we can't actually disallow it by default at all, which imho makes this security option essentially useless. A security option that people can't use without breaking their system is pointless. We saw that with SELinux - people ended up just disabling it for _years_, simply because it ended up breaking so much in practice. And yes, it got fixed eventually, but at an incredibly high maintenance cost of all the crazy rules databases. > Alright, but I guess we are stuck, is there something better on how we > can do this or describe this ? So I wonder if we can perhaps look at the places that actually do "requerst_module()", and start filtering them on that basis. Some of them will already have checked for capabilities. Others clearly expect to juist work even _without_ capabilities (ie the bluetoothd case). So the whole "let's add a global config option" model is broken. There is no possible global rule. It will break things, which in turn mean that people won't turn it on (and we can't turn it on by default), which in turn makes this pointless. In other words, I really think that anything that just adds a mode flag cannot work. So instead of having one "modules_autoload_mode" thing, maybe the individual requerst_module() cases need to simply be audited. Put another way: I think the part of your patch series that does that "request_module_cap()" and makes the netdev modules use it is a good addition. It's the "mode" part I really don't agree with, because apparently we really need to default it to permissive. So how about instead: - add that "request_module_cap()" and make the networking code that already uses CAP_ADMIN_NET use it. - make "request_module()" itself default to being "request_module_cap(CAP_SYS_MODULE,..)" - make sure that when the capability check fails, we print an error message, and then for the ones that trigger, we will audit them and see if it's ok. Because that "mode" flag defaulting to off will just mean that the default case will remain the existing unsafe one, and that's bad. Opt-in really doesn't work. We've done it. Global flags for varied behavior really doesn't work. We've done that too. Different cases want different behavior, the global flag is just fundamentally broken. Linus