| Message ID | alpine.LRH.2.20.1702131631490.8914@namei.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
James Morris wrote: > As the regsitration of LSMs is performed during init and then does > not change, we can mark all of the regsitration hooks as __ro_after_init. > > Signed-off-by: James Morris <james.l.morris@oracle.com> This patch makes LKM based LSMs (e.g. AKARI) impossible. I'm not happy with this patch.
On Mon, Feb 13, 2017 at 2:33 AM, Tetsuo Handa <penguin-kernel@i-love.sakura.ne.jp> wrote: > James Morris wrote: >> As the regsitration of LSMs is performed during init and then does >> not change, we can mark all of the regsitration hooks as __ro_after_init. >> >> Signed-off-by: James Morris <james.l.morris@oracle.com> > > This patch makes LKM based LSMs (e.g. AKARI) impossible. > I'm not happy with this patch. LKM based LSMs don't exist yet, and when they do, we may also have the "write rarely" infrastructure done, which LKM based LSMs can use to update the structures. -Kees
On 02/13/2017 06:59 AM, Kees Cook wrote: > On Mon, Feb 13, 2017 at 2:33 AM, Tetsuo Handa > <penguin-kernel@i-love.sakura.ne.jp> wrote: >> James Morris wrote: >>> As the regsitration of LSMs is performed during init and then does >>> not change, we can mark all of the regsitration hooks as __ro_after_init. >>> >>> Signed-off-by: James Morris <james.l.morris@oracle.com> >> >> This patch makes LKM based LSMs (e.g. AKARI) impossible. >> I'm not happy with this patch. > > LKM based LSMs don't exist yet, and when they do, we may also have the > "write rarely" infrastructure done, which LKM based LSMs can use to > update the structures. > > -Kees > Is someone actually working on the write rarely patches? If a version has been sent out, I don't recall seeing it. Thanks, Laura
On Mon, Feb 13, 2017 at 8:26 AM, Laura Abbott <labbott@redhat.com> wrote: > On 02/13/2017 06:59 AM, Kees Cook wrote: >> On Mon, Feb 13, 2017 at 2:33 AM, Tetsuo Handa >> <penguin-kernel@i-love.sakura.ne.jp> wrote: >>> James Morris wrote: >>>> As the regsitration of LSMs is performed during init and then does >>>> not change, we can mark all of the regsitration hooks as __ro_after_init. >>>> >>>> Signed-off-by: James Morris <james.l.morris@oracle.com> >>> >>> This patch makes LKM based LSMs (e.g. AKARI) impossible. >>> I'm not happy with this patch. >> >> LKM based LSMs don't exist yet, and when they do, we may also have the >> "write rarely" infrastructure done, which LKM based LSMs can use to >> update the structures. > > Is someone actually working on the write rarely patches? If a version > has been sent out, I don't recall seeing it. Still mostly just discussion. I've been toying with the PaX-style of it on x86, and I think Mark Rutland had some ideas for arm64, but I don't know if he's actually written code. -Kees
Hi, On Mon, Feb 13, 2017 at 09:34:32AM -0800, Kees Cook wrote: > On Mon, Feb 13, 2017 at 8:26 AM, Laura Abbott <labbott@redhat.com> wrote: > > On 02/13/2017 06:59 AM, Kees Cook wrote: > >> On Mon, Feb 13, 2017 at 2:33 AM, Tetsuo Handa > >> <penguin-kernel@i-love.sakura.ne.jp> wrote: > >>> James Morris wrote: > >>>> As the regsitration of LSMs is performed during init and then does > >>>> not change, we can mark all of the regsitration hooks as __ro_after_init. > >>>> > >>>> Signed-off-by: James Morris <james.l.morris@oracle.com> > >>> > >>> This patch makes LKM based LSMs (e.g. AKARI) impossible. > >>> I'm not happy with this patch. > >> > >> LKM based LSMs don't exist yet, and when they do, we may also have the > >> "write rarely" infrastructure done, which LKM based LSMs can use to > >> update the structures. > > > > Is someone actually working on the write rarely patches? If a version > > has been sent out, I don't recall seeing it. > > Still mostly just discussion. I've been toying with the PaX-style of > it on x86, and I think Mark Rutland had some ideas for arm64, but I > don't know if he's actually written code. While I had a rough idea [1] of what that could look like, I haven't written any code. Thanks, Mark. [1] http://www.openwall.com/lists/kernel-hardening/2016/11/18/3
On 2/13/2017 2:33 AM, Tetsuo Handa wrote: > James Morris wrote: >> As the regsitration of LSMs is performed during init and then does >> not change, we can mark all of the regsitration hooks as __ro_after_init. >> >> Signed-off-by: James Morris <james.l.morris@oracle.com> > This patch makes LKM based LSMs (e.g. AKARI) impossible. When a mechanism to do LKM based modules work is proposed it could include ifdef's around the __ro_after_init. I'm assuming that enabling LKM modules is something we'd want to make optional. > I'm not happy with this patch. > -- > To unsubscribe from this list: send the line "unsubscribe linux-security-module" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html >
On Mon, 13 Feb 2017, Kees Cook wrote: > On Mon, Feb 13, 2017 at 2:33 AM, Tetsuo Handa > <penguin-kernel@i-love.sakura.ne.jp> wrote: > > James Morris wrote: > >> As the regsitration of LSMs is performed during init and then does > >> not change, we can mark all of the regsitration hooks as __ro_after_init. > >> > >> Signed-off-by: James Morris <james.l.morris@oracle.com> > > > > This patch makes LKM based LSMs (e.g. AKARI) impossible. > > I'm not happy with this patch. > > LKM based LSMs don't exist yet, and when they do, we may also have the > "write rarely" infrastructure done, which LKM based LSMs can use to > update the structures. I think it would be a backwards step security-wise to allow dynamically loadable security modules. The security risks of security code in the kernel should be aggressively minimized.
James Morris wrote: > On Mon, 13 Feb 2017, Kees Cook wrote: > > On Mon, Feb 13, 2017 at 2:33 AM, Tetsuo Handa > > <penguin-kernel@...ove.sakura.ne.jp> wrote: > > > James Morris wrote: > > >> As the regsitration of LSMs is performed during init and then does > > >> not change, we can mark all of the regsitration hooks as __ro_after_init. > > >> > > >> Signed-off-by: James Morris <james.l.morris@...cle.com> > > > > > > This patch makes LKM based LSMs (e.g. AKARI) impossible. > > > I'm not happy with this patch. > > > > LKM based LSMs don't exist yet, and when they do, we may also have the > > "write rarely" infrastructure done, which LKM based LSMs can use to > > update the structures. > > I think it would be a backwards step security-wise to allow dynamically > loadable security modules. The security risks of security code in the > kernel should be aggressively minimized. If you do want secure kernels, build with CONFIG_MODULES=n for individual environment. Loadable kernel modules used by antivirus software temporarily modify syscall tables ( http://stackoverflow.com/questions/13876369/system-call-interception-in-linux-kernel-module-kernel-3-5 ) in order to register hooks for execve()/open()/close(). It is very frustrating for many users if you enforce CONFIG_MODULES=n or forbid post-__init registration of hooks. Marking LSM hooks as __ro_after_init might help mitigating modification of LSM hooks by innocent errors. A malicious attack can try to modify variables that control whether access controls are in enforcing mode or not. You need to eliminate such variables if you don't want access controls being disabled by malicious attacks. You want to eliminate more things such as security= kernel command line option, function pointers like security_hook_list, ability to update policy configuration without rebooting. Why SELinux is not the only security module? Why Smack, TOMOYO, AppArmor, Yama were proposed and became in-tree security modules? Why still other modules such as CaitSith, ptags, Timgad are proposed? Answer is very simple. In-tree modules (or modules enabled in distributor's kernels) cannot satisfy everybody's needs. But asking end users to rebuild their kernels is too painful. Disallowing dynamically loadable security modules is as silly idea as getting rid of LSM framework ( https://lwn.net/Articles/138042/ http://lkml.kernel.org/r/alpine.LFD.0.999.0710010854120.3579@woody.linux-foundation.org ) unless we accept whatever out-of-tree LSM modules and maintain them as in-tree modules and enable them in distributor's kernels. But such things won't happen. If we legally allow LKM based LSMs, we don't need to make security/ directory look like /dev/random . There will be vulnerabilities like DirtyCOW which might defeat protection for LSM. Security of kernel depends on correctness of all kernel code. Marking LSM hooks as __ro_after_init buys little compared to its sacrifice. We don't need to disallow dynamically loadable security modules.
On Tue, 14 Feb 2017, Tetsuo Handa wrote: > > I think it would be a backwards step security-wise to allow dynamically > > loadable security modules. The security risks of security code in the > > kernel should be aggressively minimized. > > If you do want secure kernels, build with CONFIG_MODULES=n for individual environment. The rationale for disallowing dynamically loaded LSMs in this case is so that the LSM hooks can be made read-only, to harden the kernel against LSM being used as an attack vector. Overwriting an LSM hook can provide an attacker with a security-critical control point at any of hundreds locations within the kernel. It's not about modules, per se. > > Loadable kernel modules used by antivirus software temporarily modify syscall tables > ( http://stackoverflow.com/questions/13876369/system-call-interception-in-linux-kernel-module-kernel-3-5 ) > in order to register hooks for execve()/open()/close(). It is very frustrating for > many users if you enforce CONFIG_MODULES=n or forbid post-__init registration of hooks. We don't cater to out of tree code. Additionally, I'd also seriously question whether the security benefits of kernel AV outweigh its security risks. > > Marking LSM hooks as __ro_after_init might help mitigating modification of > LSM hooks by innocent errors. A malicious attack can try to modify variables > that control whether access controls are in enforcing mode or not. You need to > eliminate such variables if you don't want access controls being disabled by > malicious attacks. You want to eliminate more things such as security= kernel > command line option, function pointers like security_hook_list, ability to > update policy configuration without rebooting. We are not trying to prevent every possible attack here, just the class of attack where someone uses a function pointer overwrite to compromise the kernel, via an LSM hook. We want to reduce the security risk introduced by LSM itself, i.e. providing hooks at security-critical locations all over the kernel. This is an important and useful goal, IMHO. > > Why SELinux is not the only security module? Why Smack, TOMOYO, AppArmor, > Yama were proposed and became in-tree security modules? Why still other > modules such as CaitSith, ptags, Timgad are proposed? Answer is very simple. > In-tree modules (or modules enabled in distributor's kernels) cannot satisfy > everybody's needs. But asking end users to rebuild their kernels is too painful. Once again, and this is a very widely accepted and long standing norm, the mainline kernel does not cater to out of tree code. > Disallowing dynamically loadable security modules is as silly idea as > getting rid of LSM framework ( https://lwn.net/Articles/138042/ > http://lkml.kernel.org/r/alpine.LFD.0.999.0710010854120.3579@woody.linux-foundation.org ) > unless we accept whatever out-of-tree LSM modules and maintain them as in-tree > modules and enable them in distributor's kernels. But such things won't happen. > If we legally allow LKM based LSMs, we don't need to make security/ directory > look like /dev/random . Dynamically loadable LSMs are legally allowed, we just don't cater to them in mainline. > There will be vulnerabilities like DirtyCOW which might defeat protection for > LSM. Security of kernel depends on correctness of all kernel code. As mentioned above, we are trying to harden the LSM framework against being an attack vector. We are not trying to harden it against an already compromised kernel. > Marking LSM hooks as __ro_after_init buys little compared to its sacrifice. On the contrary, I believe it provides useful hardening and in fact helps to mitigate the risk of the LSM API itself. > We don't need to disallow dynamically loadable security modules. It's not supported by mainline LSMs, for very good reasons (such as needing to start from a known security state during boot), per many previous threads on the topic. - James
On Tue, 14 Feb 2017, James Morris wrote: > As mentioned above, we are trying to harden the LSM framework against > being an attack vector. We are not trying to harden it against an already > compromised kernel. I should clarify here -- by already compromised, I mean specifically in terms of the attacker being able to bypass/change RO kernel pages.
James Morris wrote: > > Disallowing dynamically loadable security modules is as silly idea as > > getting rid of LSM framework ( https://lwn.net/Articles/138042/ > > http://lkml.kernel.org/r/alpine.LFD.0.999.0710010854120.3579@woody.linux-foundation.org ) > > unless we accept whatever out-of-tree LSM modules and maintain them as in-tree > > modules and enable them in distributor's kernels. But such things won't happen. > > If we legally allow LKM based LSMs, we don't need to make security/ directory > > look like /dev/random . > > Dynamically loadable LSMs are legally allowed, we just don't cater to them > in mainline. > I'm saying that this patch will make dynamically loadable LSMs illegal, for not allowing updating struct list_head prevents dynamically loadable LSMs from registering.
On Tue, 14 Feb 2017, Tetsuo Handa wrote: > James Morris wrote: > > > Disallowing dynamically loadable security modules is as silly idea as > > > getting rid of LSM framework ( https://lwn.net/Articles/138042/ > > > http://lkml.kernel.org/r/alpine.LFD.0.999.0710010854120.3579@woody.linux-foundation.org ) > > > unless we accept whatever out-of-tree LSM modules and maintain them as in-tree > > > modules and enable them in distributor's kernels. But such things won't happen. > > > If we legally allow LKM based LSMs, we don't need to make security/ directory > > > look like /dev/random . > > > > Dynamically loadable LSMs are legally allowed, we just don't cater to them > > in mainline. > > > I'm saying that this patch will make dynamically loadable LSMs illegal, for > not allowing updating struct list_head prevents dynamically loadable LSMs from > registering. The next patch set will include an option to allow writable hooks (for SELinux runtime disable).
diff --git a/security/apparmor/lsm.c b/security/apparmor/lsm.c index 709eacd..73aad35 100644 --- a/security/apparmor/lsm.c +++ b/security/apparmor/lsm.c @@ -587,7 +587,7 @@ static int apparmor_task_setrlimit(struct task_struct *task, return error; } -static struct security_hook_list apparmor_hooks[] = { +static struct security_hook_list apparmor_hooks[] __ro_after_init = { LSM_HOOK_INIT(ptrace_access_check, apparmor_ptrace_access_check), LSM_HOOK_INIT(ptrace_traceme, apparmor_ptrace_traceme), LSM_HOOK_INIT(capget, apparmor_capget), diff --git a/security/commoncap.c b/security/commoncap.c index 6d4d586..2c0cf0a 100644 --- a/security/commoncap.c +++ b/security/commoncap.c @@ -1070,7 +1070,7 @@ int cap_mmap_file(struct file *file, unsigned long reqprot, #ifdef CONFIG_SECURITY -struct security_hook_list capability_hooks[] = { +struct security_hook_list capability_hooks[] __ro_after_init = { LSM_HOOK_INIT(capable, cap_capable), LSM_HOOK_INIT(settime, cap_settime), LSM_HOOK_INIT(ptrace_access_check, cap_ptrace_access_check), diff --git a/security/loadpin/loadpin.c b/security/loadpin/loadpin.c index 1d82eae..aaef8d9 100644 --- a/security/loadpin/loadpin.c +++ b/security/loadpin/loadpin.c @@ -174,7 +174,7 @@ static int loadpin_read_file(struct file *file, enum kernel_read_file_id id) return 0; } -static struct security_hook_list loadpin_hooks[] = { +static struct security_hook_list loadpin_hooks[] __ro_after_init = { LSM_HOOK_INIT(sb_free_security, loadpin_sb_free_security), LSM_HOOK_INIT(kernel_read_file, loadpin_read_file), }; diff --git a/security/security.c b/security/security.c index d0e07f2..e913abb 100644 --- a/security/security.c +++ b/security/security.c @@ -1622,7 +1622,7 @@ int security_audit_rule_match(u32 secid, u32 field, u32 op, void *lsmrule, } #endif /* CONFIG_AUDIT */ -struct security_hook_heads security_hook_heads = { +struct security_hook_heads security_hook_heads __ro_after_init = { .binder_set_context_mgr = LIST_HEAD_INIT(security_hook_heads.binder_set_context_mgr), .binder_transaction = diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c index 9bc12bc..9599e97 100644 --- a/security/selinux/hooks.c +++ b/security/selinux/hooks.c @@ -6104,7 +6104,7 @@ static int selinux_key_getsecurity(struct key *key, char **_buffer) #endif -static struct security_hook_list selinux_hooks[] = { +static struct security_hook_list selinux_hooks[] __ro_after_init = { LSM_HOOK_INIT(binder_set_context_mgr, selinux_binder_set_context_mgr), LSM_HOOK_INIT(binder_transaction, selinux_binder_transaction), LSM_HOOK_INIT(binder_transfer_binder, selinux_binder_transfer_binder), diff --git a/security/smack/smack_lsm.c b/security/smack/smack_lsm.c index 60b4217..9bad3cc 100644 --- a/security/smack/smack_lsm.c +++ b/security/smack/smack_lsm.c @@ -4633,7 +4633,7 @@ static int smack_inode_getsecctx(struct inode *inode, void **ctx, u32 *ctxlen) return 0; } -static struct security_hook_list smack_hooks[] = { +static struct security_hook_list smack_hooks[] __ro_after_init = { LSM_HOOK_INIT(ptrace_access_check, smack_ptrace_access_check), LSM_HOOK_INIT(ptrace_traceme, smack_ptrace_traceme), LSM_HOOK_INIT(syslog, smack_syslog), diff --git a/security/tomoyo/tomoyo.c b/security/tomoyo/tomoyo.c index edc52d6..2677427 100644 --- a/security/tomoyo/tomoyo.c +++ b/security/tomoyo/tomoyo.c @@ -496,7 +496,7 @@ static int tomoyo_socket_sendmsg(struct socket *sock, struct msghdr *msg, * tomoyo_security_ops is a "struct security_operations" which is used for * registering TOMOYO. */ -static struct security_hook_list tomoyo_hooks[] = { +static struct security_hook_list tomoyo_hooks[] __ro_after_init = { LSM_HOOK_INIT(cred_alloc_blank, tomoyo_cred_alloc_blank), LSM_HOOK_INIT(cred_prepare, tomoyo_cred_prepare), LSM_HOOK_INIT(cred_transfer, tomoyo_cred_transfer), diff --git a/security/yama/yama_lsm.c b/security/yama/yama_lsm.c index 88271a3..fae3cd9 100644 --- a/security/yama/yama_lsm.c +++ b/security/yama/yama_lsm.c @@ -428,7 +428,7 @@ int yama_ptrace_traceme(struct task_struct *parent) return rc; } -static struct security_hook_list yama_hooks[] = { +static struct security_hook_list yama_hooks[] __ro_after_init = { LSM_HOOK_INIT(ptrace_access_check, yama_ptrace_access_check), LSM_HOOK_INIT(ptrace_traceme, yama_ptrace_traceme), LSM_HOOK_INIT(task_prctl, yama_task_prctl),
As the regsitration of LSMs is performed during init and then does not change, we can mark all of the regsitration hooks as __ro_after_init. Signed-off-by: James Morris <james.l.morris@oracle.com> --- security/apparmor/lsm.c | 2 +- security/commoncap.c | 2 +- security/loadpin/loadpin.c | 2 +- security/security.c | 2 +- security/selinux/hooks.c | 2 +- security/smack/smack_lsm.c | 2 +- security/tomoyo/tomoyo.c | 2 +- security/yama/yama_lsm.c | 2 +- 8 files changed, 8 insertions(+), 8 deletions(-)