v4: Finer granularity in the patches and other
cleanups suggested by Kees Cook.
Removed dead code created by the removal of SELinux
credential blob poisoning.
v3: Add ipc blob for SARA and task blob for Landlock.
Removing the SELinux cred blob pointer poisoning
results selinux_is_enabled() being unused, so it and
all it's overhead has been removed.
Broke up the cred infrastructure patch.
v2: Reduce the patchset to what is required to support
the proposed SARA and LandLock security modules
The SARA security module is intended to be used
in conjunction with other security modules. It requires
state to be maintained for the credential, which
in turn requires a mechanism for sharing the credential
security blob. It also uses the ipc security blob. The
module also requires mechanism for user space manipulation
of the credential information, hence an additional
subdirectory in /proc/.../attr.
The LandLock security module provides user configurable
policy in the secmark mechanism. It requires data in
the credential, file, inode and task security blobs. For
this to be used along side the existing "major" security
modules mechanism for sharing these blobs are provided.
A side effect of providing sharing of the crendential
security blob is that the TOMOYO module can be used at
the same time as the other "major" modules.
The mechanism for configuring which security modules are
enabled has to change when stacking in enabled. Any
module that uses just the security blobs that are shared
can be selected. Additionally, one other "major" module
can be selected.
The security module stacking issues around networking and
IPC are not addressed here as they are beyond what is
required for TOMOYO, SARA and LandLock.
git://github.com/cschaufler/lsm-stacking.git#stacking-4.19-rc2-saralock-v4
Signed-off-by: Casey Schaufler <casey@schaufler-ca.com>
---
Documentation/admin-guide/LSM/index.rst | 23 +-
fs/proc/base.c | 64 ++++-
fs/proc/internal.h | 1 +
include/linux/cred.h | 1 -
include/linux/lsm_hooks.h | 24 +-
include/linux/security.h | 15 +-
include/linux/selinux.h | 35 ---
kernel/cred.c | 13 -
security/Kconfig | 92 +++++++
security/apparmor/domain.c | 2 +-
security/apparmor/include/cred.h | 24 +-
security/apparmor/include/file.h | 9 +-
security/apparmor/include/lib.h | 4 +
security/apparmor/include/task.h | 18 +-
security/apparmor/lsm.c | 68 +++--
security/apparmor/task.c | 6 +-
security/security.c | 438 ++++++++++++++++++++++++++++++--
security/selinux/Makefile | 2 +-
security/selinux/exports.c | 23 --
security/selinux/hooks.c | 333 +++++++-----------------
security/selinux/include/audit.h | 3 -
security/selinux/include/objsec.h | 48 +++-
security/selinux/selinuxfs.c | 4 +-
security/selinux/ss/services.c | 1 -
security/selinux/xfrm.c | 4 +-
security/smack/smack.h | 55 +++-
security/smack/smack_access.c | 4 +-
security/smack/smack_lsm.c | 315 ++++++++---------------
security/smack/smackfs.c | 18 +-
security/tomoyo/common.h | 26 +-
security/tomoyo/domain.c | 4 +-
security/tomoyo/securityfs_if.c | 15 +-
security/tomoyo/tomoyo.c | 57 ++++-
33 files changed, 1098 insertions(+), 651 deletions(-)
@@ -1,35 +0,0 @@-/*- * SELinux services exported to the rest of the kernel.- *- * Author: James Morris <jmorris@redhat.com>- *- * Copyright (C) 2005 Red Hat, Inc., James Morris <jmorris@redhat.com>- * Copyright (C) 2006 Trusted Computer Solutions, Inc. <dgoeddel@trustedcs.com>- * Copyright (C) 2006 IBM Corporation, Timothy R. Chavez <tinytim@us.ibm.com>- *- * This program is free software; you can redistribute it and/or modify- * it under the terms of the GNU General Public License version 2,- * as published by the Free Software Foundation.- */-#ifndef _LINUX_SELINUX_H-#define _LINUX_SELINUX_H--struct selinux_audit_rule;-struct audit_context;-struct kern_ipc_perm;--#ifdef CONFIG_SECURITY_SELINUX--/**- * selinux_is_enabled - is SELinux enabled?- */-bool selinux_is_enabled(void);-#else--static inline bool selinux_is_enabled(void)-{- return false;-}-#endif /* CONFIG_SECURITY_SELINUX */--#endif /* _LINUX_SELINUX_H */
@@ -1,23 +0,0 @@-/*- * SELinux services exported to the rest of the kernel.- *- * Author: James Morris <jmorris@redhat.com>- *- * Copyright (C) 2005 Red Hat, Inc., James Morris <jmorris@redhat.com>- * Copyright (C) 2006 Trusted Computer Solutions, Inc. <dgoeddel@trustedcs.com>- * Copyright (C) 2006 IBM Corporation, Timothy R. Chavez <tinytim@us.ibm.com>- *- * This program is free software; you can redistribute it and/or modify- * it under the terms of the GNU General Public License version 2,- * as published by the Free Software Foundation.- */-#include <linux/module.h>-#include <linux/selinux.h>--#include "security.h"--bool selinux_is_enabled(void)-{- return selinux_enabled;-}-EXPORT_SYMBOL_GPL(selinux_is_enabled);
@@ -975,7 +975,7 @@ int apparmor_bprm_set_creds(struct linux_binprm *bprm)}aa_put_label(cred_label(bprm->cred));/* transfer reference, released when cred is freed */-cred_label(bprm->cred)=new;+set_cred_label(bprm->cred,new);done:aa_put_label(label);
@@ -81,7 +81,7 @@ int aa_replace_current_label(struct aa_label *label)*/aa_get_label(label);aa_put_label(cred_label(new));-cred_label(new)=label;+set_cred_label(new,label);commit_creds(new);return0;
@@ -138,7 +138,7 @@ int aa_set_current_hat(struct aa_label *label, u64 token)return-EACCES;}-cred_label(new)=aa_get_newest_label(label);+set_cred_label(new,aa_get_newest_label(label));/* clear exec on switching context */aa_put_label(ctx->onexec);ctx->onexec=NULL;
@@ -172,7 +172,7 @@ int aa_restore_previous_label(u64 token)return-ENOMEM;aa_put_label(cred_label(new));-cred_label(new)=aa_get_newest_label(ctx->previous);+set_cred_label(new,aa_get_newest_label(ctx->previous));AA_BUG(!cred_label(new));/* clear exec && prev information when restoring to previous context */aa_clear_task_ctx_trans(ctx);
@@ -3837,7 +3837,7 @@ static int selinux_file_send_sigiotask(struct task_struct *tsk,/* struct fown_struct is never outside the context of a struct file */file=container_of(fown,structfile,f_owner);-fsec=file->f_security;+fsec=selinux_file(file);if(!signum)perm=signal_to_av(SIGIO);/* as per send_sigio_to_task */
@@ -3861,7 +3861,7 @@ static int selinux_file_open(struct file *file)structfile_security_struct*fsec;structinode_security_struct*isec;-fsec=file->f_security;+fsec=selinux_file(file);isec=inode_security(file_inode(file));/**Saveinodelabelandpolicysequencenumber
@@ -4000,7 +4000,7 @@ static int selinux_kernel_module_from_file(struct file *file)ad.type=LSM_AUDIT_DATA_FILE;ad.u.file=file;-fsec=file->f_security;+fsec=selinux_file(file);if(sid!=fsec->sid){rc=avc_has_perm(&selinux_state,sid,fsec->sid,SECCLASS_FD,FD__USE,&ad);
@@ -166,7 +166,7 @@ static int smk_bu_task(struct task_struct *otp, int mode, int rc)staticintsmk_bu_inode(structinode*inode,intmode,intrc){structtask_smack*tsp=smack_cred(current_cred());-structinode_smack*isp=inode->i_security;+structinode_smack*isp=smack_inode(inode);characc[SMK_NUM_ACCESS_TYPE+1];if(isp->smk_flags&SMK_INODE_IMPURE)
@@ -198,7 +198,7 @@ static int smk_bu_file(struct file *file, int mode, int rc)structtask_smack*tsp=smack_cred(current_cred());structsmack_known*sskp=tsp->smk_task;structinode*inode=file_inode(file);-structinode_smack*isp=inode->i_security;+structinode_smack*isp=smack_inode(inode);characc[SMK_NUM_ACCESS_TYPE+1];if(isp->smk_flags&SMK_INODE_IMPURE)
Move management of the inode->i_security blob out
of the individual security modules and into the security
infrastructure. Instead of allocating the blobs from within
the modules the modules tell the infrastructure how much
space is required, and the space is allocated there.
Signed-off-by: Casey Schaufler <casey@schaufler-ca.com>
---
include/linux/lsm_hooks.h | 3 ++
security/security.c | 83 ++++++++++++++++++++++++++++++-
security/selinux/hooks.c | 32 +-----------
security/selinux/include/objsec.h | 5 +-
security/smack/smack_lsm.c | 70 ++++----------------------
5 files changed, 98 insertions(+), 95 deletions(-)
@@ -57,10 +57,7 @@ enum label_initialized {structinode_security_struct{structinode*inode;/* back pointer to inode object */-union{-structlist_headlist;/* list of inode_security_struct */-structrcu_headrcu;/* for freeing the inode_security_struct */-};+structlist_headlist;/* list of inode_security_struct */u32task_sid;/* SID of creating task */u32sid;/* SID of this object */u16sclass;/* security class of this object */
Two proposed security modules require the ability to
share security blobs with existing "major" security modules.
These modules, S.A.R.A and LandLock, provide significantly
different services than SELinux, Smack or AppArmor. Using
either in conjunction with the existing modules is quite
reasonable. S.A.R.A requires access to the cred, inode and
task blobs, while LandLock uses the cred, file, inode and
ipc blobs.
The use of the cred, file, inode, ipc and task blobs has been
abstracted in preceding patches in the series. This
patch teaches the affected security modules how to access
the part of the blob set aside for their use in the case
where blobs are shared. The configuration option
CONFIG_SECURITY_STACKING identifies systems where the
blobs may be shared.
The mechanism for selecting which security modules are
active has been changed to allow non-conflicting "major"
security modules to be used together. At this time the
TOMOYO module can safely be used with any of the others.
The two new modules would be non-conflicting as well.
Signed-off-by: Casey Schaufler <casey@schaufler-ca.com>
---
Documentation/admin-guide/LSM/index.rst | 14 +++--
include/linux/lsm_hooks.h | 2 +-
security/Kconfig | 81 +++++++++++++++++++++++++
security/apparmor/include/cred.h | 8 +++
security/apparmor/include/file.h | 9 ++-
security/apparmor/include/lib.h | 4 ++
security/apparmor/lsm.c | 8 ++-
security/security.c | 30 ++++++++-
security/selinux/hooks.c | 3 +-
security/selinux/include/objsec.h | 12 ++++
security/smack/smack.h | 13 ++++
security/smack/smack_lsm.c | 3 +-
security/tomoyo/common.h | 5 ++
security/tomoyo/tomoyo.c | 3 +-
14 files changed, 182 insertions(+), 13 deletions(-)
@@ -17,10 +17,16 @@ MAC extensions, other extensions can be built using the LSM to provide specific changes to system operation when these tweaks are not available in the core functionality of Linux itself.-The Linux capabilities modules will always be included. This may be-followed by any number of "minor" modules and at most one "major" module.-For more details on capabilities, see ``capabilities(7)`` in the Linux-man-pages project.+The Linux capabilities modules will always be included. For more details+on capabilities, see ``capabilities(7)`` in the Linux man-pages project.++Security modules that do not use the security data blobs maintained+by the LSM infrastructure are considered "minor" modules. These may be+included at compile time and stacked explicitly. Security modules that+use the LSM maintained security blobs are considered "major" modules.+These may only be stacked if the CONFIG_LSM_STACKED configuration+option is used. If this is chosen all of the security modules selected+will be used. A list of the active security modules can be found by reading``/sys/kernel/security/lsm``. This is a comma separated list, and
@@ -1544,7 +1544,9 @@ static int __init apparmor_init(void)interror;if(!finish){-if(apparmor_enabled&&security_module_enable("apparmor"))+if(apparmor_enabled&&+security_module_enable("apparmor",+IS_ENABLED(CONFIG_SECURITY_APPARMOR_STACKED)))security_add_blobs(&apparmor_blob_sizes);elseapparmor_enabled=false;
@@ -1552,7 +1554,9 @@ static int __init apparmor_init(void)return0;}-if(!apparmor_enabled||!security_module_enable("apparmor")){+if(!apparmor_enabled||+!security_module_enable("apparmor",+IS_ENABLED(CONFIG_SECURITY_APPARMOR_STACKED))){aa_info_message("AppArmor disabled by boot time parameter");apparmor_enabled=false;return0;
@@ -37,6 +37,7 @@/* Maximum number of letters for an LSM name string */#define SECURITY_NAME_MAX 10+#define MODULE_STACK "(stacking)"structsecurity_hook_headssecurity_hook_heads__lsm_ro_after_init;staticATOMIC_NOTIFIER_HEAD(lsm_notifier_chain);
On Fri, Sep 21, 2018 at 5:18 PM, Casey Schaufler [off-list ref] wrote:
Move management of the cred security blob out of the
security modules and into the security infrastructre.
Instead of allocating and freeing space the security
modules tell the infrastructure how much space they
require.
Signed-off-by: Casey Schaufler <casey@schaufler-ca.com>
When combined with my series, this gets slightly simpler:
- the double init call and the "finished" stuff goes away
- debugging output is controlled by "lsm.debug" param instead of a CONFIG
Regardless, for the overall logic, calculating the sizes, etc:
Reviewed-by: Kees Cook <redacted>
-Kees
--
Kees Cook
Pixel Security
On Fri, Sep 21, 2018 at 5:20 PM, Casey Schaufler [off-list ref] wrote:
Move management of the kern_ipc_perm->security and
msg_msg->security blobs out of the individual security
modules and into the security infrastructure. Instead
of allocating the blobs from within the modules the modules
tell the infrastructure how much space is required, and
the space is allocated there.
Signed-off-by: Casey Schaufler <casey@schaufler-ca.com>
Back in 2007 I made what turned out to be a rather serious
mistake in the implementation of the Smack security module.
The SELinux module used an interface in /proc to manipulate
the security context on processes. Rather than use a similar
interface, I used the same interface. The AppArmor team did
likewise. Now /proc/.../attr/current will tell you the
security "context" of the process, but it will be different
depending on the security module you're using.
This patch provides a subdirectory in /proc/.../attr for
Smack. Smack user space can use the "current" file in
this subdirectory and never have to worry about getting
SELinux attributes by mistake. Programs that use the
old interface will continue to work (or fail, as the case
may be) as before.
The proposed S.A.R.A security module is dependent on
the mechanism to create its own attr subdirectory.
The original implementation is by Kees Cook.
Signed-off-by: Casey Schaufler <casey@schaufler-ca.com>
Reviewed-by: Kees Cook <redacted>
---
Documentation/admin-guide/LSM/index.rst | 13 +++--
fs/proc/base.c | 64 +++++++++++++++++++++----
fs/proc/internal.h | 1 +
include/linux/security.h | 15 ++++--
security/security.c | 24 ++++++++--
5 files changed, 96 insertions(+), 21 deletions(-)
@@ -17,9 +17,8 @@ MAC extensions, other extensions can be built using the LSM to provide specific changes to system operation when these tweaks are not available in the core functionality of Linux itself.-Without a specific LSM built into the kernel, the default LSM will be the-Linux capabilities system. Most LSMs choose to extend the capabilities-system, building their checks on top of the defined capability hooks.+The Linux capabilities modules will always be included. This may be+followed by any number of "minor" modules and at most one "major" module. For more details on capabilities, see ``capabilities(7)`` in the Linux man-pages project.
@@ -30,6 +29,14 @@ order in which checks are made. The capability module will always be first, followed by any "minor" modules (e.g. Yama) and then the one "major" module (e.g. SELinux) if there is one configured.+Process attributes associated with "major" security modules should+be accessed and maintained using the special files in ``/proc/.../attr``.+A security module may maintain a module specific subdirectory there,+named after the module. ``/proc/.../attr/smack`` is provided by the Smack+security module and contains all its special files. The files directly+in ``/proc/.../attr`` remain as legacy interfaces for modules that provide+subdirectories.+..toctree:::maxdepth: 1
@@ -390,8 +390,10 @@ int security_sem_semctl(struct kern_ipc_perm *sma, int cmd);intsecurity_sem_semop(structkern_ipc_perm*sma,structsembuf*sops,unsignednsops,intalter);voidsecurity_d_instantiate(structdentry*dentry,structinode*inode);-intsecurity_getprocattr(structtask_struct*p,char*name,char**value);-intsecurity_setprocattr(constchar*name,void*value,size_tsize);+intsecurity_getprocattr(structtask_struct*p,constchar*lsm,char*name,+char**value);+intsecurity_setprocattr(constchar*lsm,constchar*name,void*value,+size_tsize);intsecurity_netlink_send(structsock*sk,structsk_buff*skb);intsecurity_ismaclabel(constchar*name);intsecurity_secid_to_secctx(u32secid,char**secdata,u32*seclen);
@@ -1139,15 +1141,18 @@ static inline int security_sem_semop(struct kern_ipc_perm *sma,return0;}-staticinlinevoidsecurity_d_instantiate(structdentry*dentry,structinode*inode)+staticinlinevoidsecurity_d_instantiate(structdentry*dentry,+structinode*inode){}-staticinlineintsecurity_getprocattr(structtask_struct*p,char*name,char**value)+staticinlineintsecurity_getprocattr(structtask_struct*p,constchar*lsm,+char*name,char**value){return-EINVAL;}-staticinlineintsecurity_setprocattr(char*name,void*value,size_tsize)+staticinlineintsecurity_setprocattr(constchar*lsm,char*name,+void*value,size_tsize){return-EINVAL;}
@@ -165,7 +165,7 @@ static int smk_bu_task(struct task_struct *otp, int mode, int rc)#ifdef CONFIG_SECURITY_SMACK_BRINGUPstaticintsmk_bu_inode(structinode*inode,intmode,intrc){-structtask_smack*tsp=current_security();+structtask_smack*tsp=smack_cred(current_cred());structinode_smack*isp=inode->i_security;characc[SMK_NUM_ACCESS_TYPE+1];
@@ -195,7 +195,7 @@ static int smk_bu_inode(struct inode *inode, int mode, int rc)#ifdef CONFIG_SECURITY_SMACK_BRINGUPstaticintsmk_bu_file(structfile*file,intmode,intrc){-structtask_smack*tsp=current_security();+structtask_smack*tsp=smack_cred(current_cred());structsmack_known*sskp=tsp->smk_task;structinode*inode=file_inode(file);structinode_smack*isp=inode->i_security;
@@ -225,7 +225,7 @@ static int smk_bu_file(struct file *file, int mode, int rc)staticintsmk_bu_credfile(conststructcred*cred,structfile*file,intmode,intrc){-structtask_smack*tsp=cred->security;+structtask_smack*tsp=smack_cred(cred);structsmack_known*sskp=tsp->smk_task;structinode*inode=file_inode(file);structinode_smack*isp=inode->i_security;
@@ -429,7 +429,7 @@ static int smk_ptrace_rule_check(struct task_struct *tracer,}rcu_read_lock();-tsp=__task_cred(tracer)->security;+tsp=smack_cred(__task_cred(tracer));tracer_known=smk_of_task(tsp);if((mode&PTRACE_MODE_ATTACH)&&
@@ -496,7 +496,7 @@ static int smack_ptrace_traceme(struct task_struct *ptp)intrc;structsmack_known*skp;-skp=smk_of_task(current_security());+skp=smk_of_task(smack_cred(current_cred()));rc=smk_ptrace_rule_check(ptp,skp,PTRACE_MODE_ATTACH,__func__);returnrc;
@@ -913,7 +913,7 @@ static int smack_sb_statfs(struct dentry *dentry)staticintsmack_bprm_set_creds(structlinux_binprm*bprm){structinode*inode=file_inode(bprm->file);-structtask_smack*bsp=bprm->cred->security;+structtask_smack*bsp=smack_cred(bprm->cred);structinode_smack*isp;structsuperblock_smack*sbsp;intrc;
@@ -1744,7 +1744,7 @@ static int smack_mmap_file(struct file *file,return-EACCES;mkp=isp->smk_mmap;-tsp=current_security();+tsp=smack_cred(current_cred());skp=smk_of_current();rc=0;
@@ -1840,7 +1840,7 @@ static int smack_file_send_sigiotask(struct task_struct *tsk,structfown_struct*fown,intsignum){structsmack_known*skp;-structsmack_known*tkp=smk_of_task(tsk->cred->security);+structsmack_known*tkp=smk_of_task(smack_cred(tsk->cred));structfile*file;intrc;structsmk_audit_infoad;
@@ -1888,7 +1888,7 @@ static int smack_file_receive(struct file *file)if(inode->i_sb->s_magic==SOCKFS_MAGIC){sock=SOCKET_I(inode);ssp=sock->sk->sk_security;-tsp=current_security();+tsp=smack_cred(current_cred());/**Ifthereceivingprocesscan'twritetothe*passedsocketorifthepassedsocketcan't
@@ -1930,7 +1930,7 @@ static int smack_file_receive(struct file *file)*/staticintsmack_file_open(structfile*file){-structtask_smack*tsp=file->f_cred->security;+structtask_smack*tsp=smack_cred(file->f_cred);structinode*inode=file_inode(file);structsmk_audit_infoad;intrc;
@@ -464,7 +464,7 @@ static int may_context_mount_sb_relabel(u32 sid,structsuperblock_security_struct*sbsec,conststructcred*cred){-conststructtask_security_struct*tsec=cred->security;+conststructtask_security_struct*tsec=selinux_cred(cred);intrc;rc=avc_has_perm(&selinux_state,
@@ -483,7 +483,7 @@ static int may_context_mount_inode_relabel(u32 sid,structsuperblock_security_struct*sbsec,conststructcred*cred){-conststructtask_security_struct*tsec=cred->security;+conststructtask_security_struct*tsec=selinux_cred(cred);intrc;rc=avc_has_perm(&selinux_state,tsec->sid,sbsec->sid,SECCLASS_FILESYSTEM,
@@ -1949,7 +1949,7 @@ static int may_create(struct inode *dir,structdentry*dentry,u16tclass){-conststructtask_security_struct*tsec=current_security();+conststructtask_security_struct*tsec=selinux_cred(current_cred());structinode_security_struct*dsec;structsuperblock_security_struct*sbsec;u32sid,newsid;
@@ -1971,7 +1971,7 @@ static int may_create(struct inode *dir,if(rc)returnrc;-rc=selinux_determine_inode_label(current_security(),dir,+rc=selinux_determine_inode_label(selinux_cred(current_cred()),dir,&dentry->d_name,tclass,&newsid);if(rc)returnrc;
@@ -2478,8 +2478,8 @@ static int selinux_bprm_set_creds(struct linux_binprm *bprm)if(bprm->called_set_creds)return0;-old_tsec=current_security();-new_tsec=bprm->cred->security;+old_tsec=selinux_cred(current_cred());+new_tsec=selinux_cred(bprm->cred);isec=inode_security(inode);/* Default to the current task SID. */
@@ -3498,7 +3498,7 @@ static int selinux_inode_copy_up(struct dentry *src, struct cred **new)return-ENOMEM;}-tsec=new_creds->security;+tsec=selinux_cred(new_creds);/* Get label from overlay inode and set it in create_sid */selinux_inode_getsecid(d_inode(src),&sid);tsec->create_sid=sid;
@@ -4564,7 +4564,7 @@ static int selinux_socket_create(int family, int type,staticintselinux_socket_post_create(structsocket*sock,intfamily,inttype,intprotocol,intkern){-conststructtask_security_struct*tsec=current_security();+conststructtask_security_struct*tsec=selinux_cred(current_cred());structinode_security_struct*isec=inode_security_novalidate(SOCK_INODE(sock));structsk_security_struct*sksec;u16sclass=socket_type_to_security_class(family,type,protocol);
@@ -5442,7 +5442,7 @@ static int selinux_secmark_relabel_packet(u32 sid)conststructtask_security_struct*__tsec;u32tsid;-__tsec=current_security();+__tsec=selinux_cred(current_cred());tsid=__tsec->sid;returnavc_has_perm(&selinux_state,
@@ -6379,7 +6379,7 @@ static int selinux_getprocattr(struct task_struct *p,unsignedlen;rcu_read_lock();-__tsec=__task_cred(p)->security;+__tsec=selinux_cred(__task_cred(p));if(current!=p){error=avc_has_perm(&selinux_state,
The SELinux specific credential poisioning only makes sense
if SELinux is managing the credentials. As the intent of this
patch set is to move the blob management out of the modules
and into the infrastructure, the SELinux specific code has
to go. The poisioning could be introduced into the infrastructure
at some later date.
Signed-off-by: Casey Schaufler <casey@schaufler-ca.com>
---
kernel/cred.c | 13 -------------
security/selinux/hooks.c | 6 ------
2 files changed, 19 deletions(-)
@@ -539,13 +557,17 @@ DEFINE_SRCU(tomoyo_ss);staticint__inittomoyo_init(void){structcred*cred=(structcred*)current_cred();+structtomoyo_domain_info**blob;if(!security_module_enable("tomoyo"))return0;+tomoyo_enabled=true;+/* register ourselves with the security framework */security_add_hooks(tomoyo_hooks,ARRAY_SIZE(tomoyo_hooks),"tomoyo");printk(KERN_INFO"TOMOYO Linux initialized\n");-cred->security=&tomoyo_kernel_domain;+blob=tomoyo_cred(cred);+*blob=&tomoyo_kernel_domain;tomoyo_mm_init();return0;}
Move management of the cred security blob out of the
security modules and into the security infrastructre.
Instead of allocating and freeing space the security
modules tell the infrastructure how much space they
require.
Signed-off-by: Casey Schaufler <casey@schaufler-ca.com>
---
include/linux/lsm_hooks.h | 14 ++++
security/Kconfig | 11 ++++
security/apparmor/lsm.c | 18 +++++
security/security.c | 106 +++++++++++++++++++++++++++++-
security/selinux/hooks.c | 58 +++++-----------
security/selinux/include/objsec.h | 2 +
security/smack/smack_lsm.c | 85 +++++++++---------------
security/tomoyo/common.h | 2 +-
security/tomoyo/tomoyo.c | 16 ++++-
9 files changed, 212 insertions(+), 100 deletions(-)
@@ -36,6 +36,17 @@ config SECURITY_WRITABLE_HOOKSbooldefaultn+configSECURITY_LSM_DEBUG+bool"Enable debugging of the LSM infrastructure"+depends onSECURITY+help+Thisallowsyoutochoosedebugmessagesrelatedto+securitymodulesconfiguredintoyourkernel.These+messagesmaybehelpfulindetermininghowasecurity+moduleisusingsecurityblobs.++Ifyouareunsurehowtoanswerthisquestion,answerN.+configSECURITYFSbool"Enable the securityfs filesystem"help
@@ -1455,6 +1462,7 @@ static int __init set_init_ctx(void)if(!ctx)return-ENOMEM;+lsm_early_cred(cred);set_cred_label(cred,aa_get_label(ns_unconfined(root_ns)));task_ctx(current)=ctx;
@@ -1540,8 +1548,18 @@ static inline int apparmor_init_sysctl(void)staticint__initapparmor_init(void){+staticintfinish;interror;+if(!finish){+if(apparmor_enabled&&security_module_enable("apparmor"))+security_add_blobs(&apparmor_blob_sizes);+else+apparmor_enabled=false;+finish=1;+return0;+}+if(!apparmor_enabled||!security_module_enable("apparmor")){aa_info_message("AppArmor disabled by boot time parameter");apparmor_enabled=false;
@@ -556,16 +560,26 @@ bool tomoyo_enabled;*/staticint__inittomoyo_init(void){+staticintfinish;structcred*cred=(structcred*)current_cred();structtomoyo_domain_info**blob;-if(!security_module_enable("tomoyo"))+if(!security_module_enable("tomoyo")){+tomoyo_enabled=false;return0;+}tomoyo_enabled=true;+if(!finish){+security_add_blobs(&tomoyo_blob_sizes);+finish=1;+return0;+}+/* register ourselves with the security framework */security_add_hooks(tomoyo_hooks,ARRAY_SIZE(tomoyo_hooks),"tomoyo");printk(KERN_INFO"TOMOYO Linux initialized\n");+lsm_early_cred(cred);blob=tomoyo_cred(cred);*blob=&tomoyo_kernel_domain;tomoyo_mm_init();
@@ -1842,7 +1845,8 @@ static int smack_file_send_sigiotask(struct task_struct *tsk,file=container_of(fown,structfile,f_owner);/* we don't log here as rc can be overriden */-skp=file->f_security;+blob=smack_file(file);+skp=*blob;rc=smk_access(skp,tkp,MAY_DELIVER,NULL);rc=smk_bu_note("sigiotask",skp,tkp,MAY_DELIVER,rc);if(rc!=0&&has_capability(tsk,CAP_MAC_OVERRIDE))
Move management of the file->f_security blob out of the
individual security modules and into the infrastructure.
The modules no longer allocate or free the data, instead
they tell the infrastructure how much space they require.
Signed-off-by: Casey Schaufler <casey@schaufler-ca.com>
---
include/linux/lsm_hooks.h | 1 +
security/apparmor/lsm.c | 19 +++++++-------
security/security.c | 54 +++++++++++++++++++++++++++++++++++---
security/selinux/hooks.c | 25 ++----------------
security/smack/smack.h | 2 +-
security/smack/smack_lsm.c | 14 +---------
6 files changed, 66 insertions(+), 49 deletions(-)
@@ -962,12 +994,28 @@ int security_file_permission(struct file *file, int mask)intsecurity_file_alloc(structfile*file){-returncall_int_hook(file_alloc_security,0,file);+intrc=lsm_file_alloc(file);++if(rc)+returnrc;+rc=call_int_hook(file_alloc_security,0,file);+if(unlikely(rc))+security_file_free(file);+returnrc;}voidsecurity_file_free(structfile*file){+void*blob;++if(!lsm_file_cache)+return;+call_void_hook(file_free_security,file);++blob=file->f_security;+file->f_security=NULL;+kmem_cache_free(lsm_file_cache,blob);}intsecurity_file_ioctl(structfile*file,unsignedintcmd,unsignedlongarg)
@@ -1085,7 +1133,7 @@ int security_cred_alloc_blank(struct cred *cred, gfp_t gfp)returnrc;rc=call_int_hook(cred_alloc_blank,0,cred,gfp);-if(rc)+if(unlikely(rc))security_cred_free(cred);returnrc;}
Move management of the task_struct->security blob out
of the individual security modules and into the security
infrastructure. Instead of allocating the blobs from within
the modules the modules tell the infrastructure how much
space is required, and the space is allocated there.
The only user of this blob is AppArmor. The AppArmor use
is abstracted to avoid future conflict.
Signed-off-by: Casey Schaufler <casey@schaufler-ca.com>
---
include/linux/lsm_hooks.h | 2 ++
security/apparmor/include/task.h | 18 +++--------
security/apparmor/lsm.c | 15 ++-------
security/security.c | 54 +++++++++++++++++++++++++++++++-
4 files changed, 62 insertions(+), 27 deletions(-)
Move management of the kern_ipc_perm->security and
msg_msg->security blobs out of the individual security
modules and into the security infrastructure. Instead
of allocating the blobs from within the modules the modules
tell the infrastructure how much space is required, and
the space is allocated there.
Signed-off-by: Casey Schaufler <casey@schaufler-ca.com>
---
include/linux/lsm_hooks.h | 2 +
security/security.c | 91 +++++++++++++++++++++++++++++++++--
security/selinux/hooks.c | 98 +++++---------------------------------
security/smack/smack.h | 4 +-
security/smack/smack_lsm.c | 32 ++-----------
5 files changed, 108 insertions(+), 119 deletions(-)
@@ -3837,7 +3837,7 @@ static int selinux_file_send_sigiotask(struct task_struct *tsk,/* struct fown_struct is never outside the context of a struct file */file=container_of(fown,structfile,f_owner);-fsec=file->f_security;+fsec=selinux_file(file);if(!signum)perm=signal_to_av(SIGIO);/* as per send_sigio_to_task */
@@ -3861,7 +3861,7 @@ static int selinux_file_open(struct file *file)structfile_security_struct*fsec;structinode_security_struct*isec;-fsec=file->f_security;+fsec=selinux_file(file);isec=inode_security(file_inode(file));/**Saveinodelabelandpolicysequencenumber
@@ -4000,7 +4000,7 @@ static int selinux_kernel_module_from_file(struct file *file)ad.type=LSM_AUDIT_DATA_FILE;ad.u.file=file;-fsec=file->f_security;+fsec=selinux_file(file);if(sid!=fsec->sid){rc=avc_has_perm(&selinux_state,sid,fsec->sid,SECCLASS_FD,FD__USE,&ad);
On Fri, Sep 21, 2018 at 5:17 PM, Casey Schaufler [off-list ref] wrote:
The SELinux specific credential poisioning only makes sense
if SELinux is managing the credentials. As the intent of this
patch set is to move the blob management out of the modules
and into the infrastructure, the SELinux specific code has
to go. The poisioning could be introduced into the infrastructure
at some later date.
Signed-off-by: Casey Schaufler <casey@schaufler-ca.com>
On Fri, Sep 21, 2018 at 5:17 PM, Casey Schaufler [off-list ref] wrote:
There are no longer users of selinux_is_enabled().
Remove it. As selinux_is_enabled() is the only reason
for include/linux/selinux.h remove that as well.
Signed-off-by: Casey Schaufler <casey@schaufler-ca.com>
On Fri, Sep 21, 2018 at 5:17 PM, Casey Schaufler [off-list ref] wrote:
Don't use the cred->security pointer directly.
Provide a helper function that provides the security blob pointer.
Signed-off-by: Casey Schaufler <casey@schaufler-ca.com>
On Fri, Sep 21, 2018 at 5:17 PM, Casey Schaufler [off-list ref] wrote:
Don't use the cred->security pointer directly.
Provide a helper function that provides the security blob pointer.
Signed-off-by: Casey Schaufler <casey@schaufler-ca.com>
On Fri, Sep 21, 2018 at 5:18 PM, Casey Schaufler [off-list ref] wrote:
Don't use the cred->security pointer directly.
Provide helper functions that provide the security blob pointer.
Signed-off-by: Casey Schaufler <casey@schaufler-ca.com>
On Fri, Sep 21, 2018 at 5:18 PM, Casey Schaufler [off-list ref] wrote:
Don't use the file->f_security pointer directly.
Provide a helper function that provides the security blob pointer.
Signed-off-by: Casey Schaufler <casey@schaufler-ca.com>
On Fri, Sep 21, 2018 at 5:19 PM, Casey Schaufler [off-list ref] wrote:
Move management of the file->f_security blob out of the
individual security modules and into the infrastructure.
The modules no longer allocate or free the data, instead
they tell the infrastructure how much space they require.
Signed-off-by: Casey Schaufler <casey@schaufler-ca.com>
On Fri, Sep 21, 2018 at 5:19 PM, Casey Schaufler [off-list ref] wrote:
quoted hunk
Move management of the inode->i_security blob out
of the individual security modules and into the security
infrastructure. Instead of allocating the blobs from within
the modules the modules tell the infrastructure how much
space is required, and the space is allocated there.
Signed-off-by: Casey Schaufler <casey@schaufler-ca.com>
---
include/linux/lsm_hooks.h | 3 ++
security/security.c | 83 ++++++++++++++++++++++++++++++-
security/selinux/hooks.c | 32 +-----------
security/selinux/include/objsec.h | 5 +-
security/smack/smack_lsm.c | 70 ++++----------------------
5 files changed, 98 insertions(+), 95 deletions(-)
@@ -311,6 +324,46 @@ int lsm_file_alloc(struct file *file)return0;}+/**+*lsm_inode_alloc-allocateacompositeinodeblob+*@inode:theinodethatneedsablob+*+*Allocatetheinodeblobforallthemodules+*+*Returns0,or-ENOMEMifmemorycan'tbeallocated.+*/+intlsm_inode_alloc(structinode*inode)+{+if(!lsm_inode_cache){+inode->i_security=NULL;+return0;+}++inode->i_security=kmem_cache_zalloc(lsm_inode_cache,GFP_NOFS);+if(inode->i_security==NULL)+return-ENOMEM;+return0;+}++/**+*lsm_early_inode-duringinitializationallocateacompositeinodeblob+*@inode:theinodethatneedsablob+*+*Allocatetheinodeblobforallthemodulesifit'snotalreadythere+*/+voidlsm_early_inode(structinode*inode)+{+intrc;++if(inode==NULL)+panic("%s: NULL inode.\n",__func__);+if(inode->i_security!=NULL)+return;+rc=lsm_inode_alloc(inode);+if(rc)+panic("%s: Early inode alloc failed.\n",__func__);+}
I'm still advising against using panic(), but I'll leave it up to James.
For everything else here:
Reviewed-by: Kees Cook <redacted>
-Kees
--
Kees Cook
Pixel Security
On Fri, Sep 21, 2018 at 5:19 PM, Casey Schaufler [off-list ref] wrote:
Move management of the task_struct->security blob out
of the individual security modules and into the security
infrastructure. Instead of allocating the blobs from within
the modules the modules tell the infrastructure how much
space is required, and the space is allocated there.
The only user of this blob is AppArmor. The AppArmor use
is abstracted to avoid future conflict.
Signed-off-by: Casey Schaufler <casey@schaufler-ca.com>
On Fri, Sep 21, 2018 at 5:19 PM, Casey Schaufler [off-list ref] wrote:
Don't use the ipc->security pointer directly.
Don't use the msg_msg->security pointer directly.
Provide helper functions that provides the security blob pointers.
Signed-off-by: Casey Schaufler <casey@schaufler-ca.com>
On Fri, Sep 21, 2018 at 5:19 PM, Casey Schaufler [off-list ref] wrote:
Don't use the ipc->security pointer directly.
Don't use the msg_msg->security pointer directly.
Provide helper functions that provides the security blob pointers.
Signed-off-by: Casey Schaufler <casey@schaufler-ca.com>
On Fri, Sep 21, 2018 at 4:59 PM, Casey Schaufler [off-list ref] wrote:
v4: Finer granularity in the patches and other
cleanups suggested by Kees Cook.
Removed dead code created by the removal of SELinux
credential blob poisoning.
Thanks for the splitting, this really does make it easier to review
(at least for me). I think this looks really good, though obviously
I'd like to refactor it slightly on top of my series. :)
One additional thought I had was about the blobs allocations: some are
separate kmem caches, and some are kmalloc. I'm thinking it might make
sense to use separate kmem caches for two reasons:
- they're going to always be the same size and are regularly
allocated/freed, so it may offer a performance benefit.
- they're explicitly not supposed to be exposed to userspace, so
hardened usercopy would protect them if they were not kmalloc()ed.
I'm excited about getting this landed!
-Kees
--
Kees Cook
Pixel Security
On Fri, Sep 21, 2018 at 4:59 PM, Casey Schaufler [off-list ref] wrote:
quoted
v4: Finer granularity in the patches and other
cleanups suggested by Kees Cook.
Removed dead code created by the removal of SELinux
credential blob poisoning.
Thanks for the splitting, this really does make it easier to review
(at least for me). I think this looks really good, though obviously
I'd like to refactor it slightly on top of my series. :)
Whichever goes on top is fine with me. What's one
more patch set merge, after all?
One additional thought I had was about the blobs allocations: some are
separate kmem caches, and some are kmalloc. I'm thinking it might make
sense to use separate kmem caches for two reasons:
I had seriously considered doing that. I can't see any reason
not to. It's something that could be done at any time, and with
all the other things that had to change it just didn't get in.
- they're going to always be the same size and are regularly
allocated/freed, so it may offer a performance benefit.
- they're explicitly not supposed to be exposed to userspace, so
hardened usercopy would protect them if they were not kmalloc()ed.
I'm excited about getting this landed!
Soon. Real soon. I hope. I would very much like for
someone from the SELinux camp to chime in, especially on
the selinux_is_enabled() removal.
On a somewhat related note, I will be out for the first three
weeks of October, returning just in time for the Linux Security
Summit in Edinburgh. My connectivity will be severely limited.
I don't expect to accomplish anything while I'm out.
On Sat, Sep 22, 2018 at 9:38 AM, Casey Schaufler [off-list ref] wrote:
On 9/21/2018 8:02 PM, Kees Cook wrote:
quoted
On Fri, Sep 21, 2018 at 4:59 PM, Casey Schaufler [off-list ref] wrote:
quoted
v4: Finer granularity in the patches and other
cleanups suggested by Kees Cook.
Removed dead code created by the removal of SELinux
credential blob poisoning.
Thanks for the splitting, this really does make it easier to review
(at least for me). I think this looks really good, though obviously
I'd like to refactor it slightly on top of my series. :)
Whichever goes on top is fine with me. What's one
more patch set merge, after all?
quoted
One additional thought I had was about the blobs allocations: some are
separate kmem caches, and some are kmalloc. I'm thinking it might make
sense to use separate kmem caches for two reasons:
I had seriously considered doing that. I can't see any reason
not to. It's something that could be done at any time, and with
all the other things that had to change it just didn't get in.
Yup; that is an easy future change. Not needed now!
quoted
- they're going to always be the same size and are regularly
allocated/freed, so it may offer a performance benefit.
- they're explicitly not supposed to be exposed to userspace, so
hardened usercopy would protect them if they were not kmalloc()ed.
I'm excited about getting this landed!
Soon. Real soon. I hope. I would very much like for
someone from the SELinux camp to chime in, especially on
the selinux_is_enabled() removal.
Agreed.
On a somewhat related note, I will be out for the first three
weeks of October, returning just in time for the Linux Security
Summit in Edinburgh. My connectivity will be severely limited.
I don't expect to accomplish anything while I'm out.
If you're okay with it, I can help with changes while you're out -- I
want to try to rebase it on my tree and see how it looks anyway. :)
-Kees
--
Kees Cook
Pixel Security
Soon. Real soon. I hope. I would very much like for
someone from the SELinux camp to chime in, especially on
the selinux_is_enabled() removal.
Agreed.
This patchset from Casey lands before the patchset from Kees, doesn't it?
OK, a few comments (if I didn't overlook something).
lsm_early_cred()/lsm_early_task() are called from only __init functions.
lsm_cred_alloc()/lsm_file_alloc() are called from only security/security.c .
lsm_early_inode() should be avoided because it is not appropriate to
call panic() when lsm_early_inode() is called after __init phase.
Since all free hooks are called when one of init hooks failed, each
free hook needs to check whether init hook was called. An example is
inode_free_security() in security/selinux/hooks.c (but not addressed in
this patch).
This patchset might fatally prevent LKM-based LSM modules, for LKM-based
LSMs cannot count on lsm_*_alloc() because size for lsm_*_alloc() cannot
be updated upon loading LKM-based LSMs. If security_file_free() is called
regardless of whether lsm_file_cache is defined, LKM-based LSMs can be
loaded using current behavior (apart from the fact that legitimate
interface for appending to security_hook_heads is currently missing).
How do you plan to handle LKM-based LSMs?
include/linux/lsm_hooks.h | 6 ++----
security/security.c | 31 ++++++-------------------------
security/smack/smack_lsm.c | 8 +++++++-
3 files changed, 15 insertions(+), 30 deletions(-)
Soon. Real soon. I hope. I would very much like for
someone from the SELinux camp to chime in, especially on
the selinux_is_enabled() removal.
Agreed.
This patchset from Casey lands before the patchset from Kees, doesn't it?
That is up for negotiation. We may end up combining them.
OK, a few comments (if I didn't overlook something).
lsm_early_cred()/lsm_early_task() are called from only __init functions.
True.
lsm_cred_alloc()/lsm_file_alloc() are called from only security/security.c .
Also true.
lsm_early_inode() should be avoided because it is not appropriate to
call panic() when lsm_early_inode() is called after __init phase.
You're correct. In fact, lsm_early_inode() isn't needed at all
until multiple inode using modules are supported.
Since all free hooks are called when one of init hooks failed, each
free hook needs to check whether init hook was called. An example is
inode_free_security() in security/selinux/hooks.c (but not addressed in
this patch).
I *think* that selinux_inode_free_security() is safe in this
case because the blob will be zeroed, hence isec->list will
be NULL.
This patchset might fatally prevent LKM-based LSM modules, for LKM-based
LSMs cannot count on lsm_*_alloc() because size for lsm_*_alloc() cannot
be updated upon loading LKM-based LSMs.
LKM based security modules will require dynamically sized blobs.
These can be added to the scheme used here. Each blob would get a
header identifying the modules for which it contains data. When an
LKM is registered if has to declare it's blob space requirements
and gets back the offsets. All alloc operations have to put their
marks in the header. All LKM blob users have to check that the blob
they are looking at has the required data.
module_cred(struct cred *cred) {
return cred->security + module_blob_sizes.lbs_cred;
}
becomes
module_cred(struct cred *cred) {
if (blob_includes(module_id))
return cred->security + module_blob_sizes.lbs_cred;
return NULL;
}
and the calling code needs to accept a NULL return.
Blobs can never get smaller because readjusting the offsets
isn't going to work, so unloading an LKM security module isn't
going to be as complete as you might like. There may be a way
around this if you unload all the LKM modules, but that's a
special case and there may be dragon lurking in the mist.
If security_file_free() is called
regardless of whether lsm_file_cache is defined, LKM-based LSMs can be
loaded using current behavior (apart from the fact that legitimate
interface for appending to security_hook_heads is currently missing).
How do you plan to handle LKM-based LSMs?
My position all along has been that I don't plan to handle LKM
based LSMs, but that I won't do anything to prevent someone else
from adding them later. I believe that I've done that. Several
designs, including a separate list for dynamically loaded modules
have been proposed. I think some of those would work.
Why does this make sense? If the lsm_file_cache isn't
initialized you can't have allocated any file blobs,
no module can have initialized a file blob, hence there
can be nothing for the module to do.
Since all free hooks are called when one of init hooks failed, each
free hook needs to check whether init hook was called. An example is
inode_free_security() in security/selinux/hooks.c (but not addressed in
this patch).
I *think* that selinux_inode_free_security() is safe in this
case because the blob will be zeroed, hence isec->list will
be NULL.
OK.
quoted
This patchset might fatally prevent LKM-based LSM modules, for LKM-based
LSMs cannot count on lsm_*_alloc() because size for lsm_*_alloc() cannot
be updated upon loading LKM-based LSMs.
LKM based security modules will require dynamically sized blobs.
These can be added to the scheme used here. Each blob would get a
header identifying the modules for which it contains data. When an
LKM is registered if has to declare it's blob space requirements
and gets back the offsets. All alloc operations have to put their
marks in the header. All LKM blob users have to check that the blob
they are looking at has the required data.
module_cred(struct cred *cred) {
return cred->security + module_blob_sizes.lbs_cred;
}
becomes
module_cred(struct cred *cred) {
if (blob_includes(module_id))
return cred->security + module_blob_sizes.lbs_cred;
return NULL;
}
and the calling code needs to accept a NULL return.
Not all of LKM-based LSMs use security blobs. And some of LKM-based LSMs
might use security blobs for only a few objects. For example, AKARI uses
inode security blob for remembering whether source address/port of an
accept()ed socket was already checked, only during accept() operation and
first socket operation on the accept()ed socket. Thus, there is no need
to waste memory by assigning blobs for all inode objects.
Blobs can never get smaller because readjusting the offsets
isn't going to work, so unloading an LKM security module isn't
going to be as complete as you might like. There may be a way
around this if you unload all the LKM modules, but that's a
special case and there may be dragon lurking in the mist.
If LKM-based LSMs who want to use security blobs have to check for
NULL return, they might choose "not using infrastructure managed
security blobs" and "using locally hashed blobs associated with
object's address" (like AKARI does).
quoted
If security_file_free() is called
regardless of whether lsm_file_cache is defined, LKM-based LSMs can be
loaded using current behavior (apart from the fact that legitimate
interface for appending to security_hook_heads is currently missing).
How do you plan to handle LKM-based LSMs?
My position all along has been that I don't plan to handle LKM
based LSMs, but that I won't do anything to prevent someone else
from adding them later. I believe that I've done that. Several
designs, including a separate list for dynamically loaded modules
have been proposed. I think some of those would work.
Though AKARI is not using security_file_free(), some of LKM-based LSMs
might want to use it. If file_free_security hook is called unconditionally,
such LKM-based LSMs can be registered/unregistered, without worrying about
inability to shrink sizes for blobs.
Why does this make sense? If the lsm_file_cache isn't
initialized you can't have allocated any file blobs,
no module can have initialized a file blob, hence there
can be nothing for the module to do.
For modules (not limited to LKM-based LSMs) which want to use
file blobs for only a few objects and avoid wasting memory by
allocating file blobs to all file objects.
Infrastructure based blob management fits well for LSM modules
which want to assign blobs to all objects (like SELinux). But
forcing infrastructure based blob management can become a huge
waste of memory for LSM modules which want to assign blobs to
only a few objects. Unconditionally calling file_free_security
hook (as with other hooks) preserves a room for allowing the
latter type of LSM modules without using infrastructure based
blob management.
From: Stephen Smalley <hidden> Date: 2018-09-24 21:12:09
On 09/23/2018 01:09 PM, Casey Schaufler wrote:
On 9/23/2018 8:59 AM, Tetsuo Handa wrote:
quoted
On 2018/09/23 11:43, Kees Cook wrote:
quoted
quoted
quoted
I'm excited about getting this landed!
Soon. Real soon. I hope. I would very much like for
someone from the SELinux camp to chime in, especially on
the selinux_is_enabled() removal.
Agreed.
This patchset from Casey lands before the patchset from Kees, doesn't it?
That is up for negotiation. We may end up combining them.
quoted
OK, a few comments (if I didn't overlook something).
lsm_early_cred()/lsm_early_task() are called from only __init functions.
True.
quoted
lsm_cred_alloc()/lsm_file_alloc() are called from only security/security.c .
Also true.
quoted
lsm_early_inode() should be avoided because it is not appropriate to
call panic() when lsm_early_inode() is called after __init phase.
You're correct. In fact, lsm_early_inode() isn't needed at all
until multiple inode using modules are supported.
quoted
Since all free hooks are called when one of init hooks failed, each
free hook needs to check whether init hook was called. An example is
inode_free_security() in security/selinux/hooks.c (but not addressed in
this patch).
I *think* that selinux_inode_free_security() is safe in this
case because the blob will be zeroed, hence isec->list will
be NULL.
That's not safe - look more closely at what list_empty_careful() tests,
and then think about what happens when list_del_init() gets called on
that isec->list. selinux_inode_free_security() presumes that
selinux_inode_alloc_security() has been called already. If you are
breaking that assumption, you have to fix it.
Is there a reason you can't make inode_alloc_security() return void
since you moved the allocation to the framework? Unfortunate that
inode_init_security name is already in use for another purpose since
essentially you have reduced these hooks to initialization only.
quoted
This patchset might fatally prevent LKM-based LSM modules, for LKM-based
LSMs cannot count on lsm_*_alloc() because size for lsm_*_alloc() cannot
be updated upon loading LKM-based LSMs.
LKM based security modules will require dynamically sized blobs.
These can be added to the scheme used here. Each blob would get a
header identifying the modules for which it contains data. When an
LKM is registered if has to declare it's blob space requirements
and gets back the offsets. All alloc operations have to put their
marks in the header. All LKM blob users have to check that the blob
they are looking at has the required data.
module_cred(struct cred *cred) {
return cred->security + module_blob_sizes.lbs_cred;
}
becomes
module_cred(struct cred *cred) {
if (blob_includes(module_id))
return cred->security + module_blob_sizes.lbs_cred;
return NULL;
}
and the calling code needs to accept a NULL return.
Blobs can never get smaller because readjusting the offsets
isn't going to work, so unloading an LKM security module isn't
going to be as complete as you might like. There may be a way
around this if you unload all the LKM modules, but that's a
special case and there may be dragon lurking in the mist.
quoted
If security_file_free() is called
regardless of whether lsm_file_cache is defined, LKM-based LSMs can be
loaded using current behavior (apart from the fact that legitimate
interface for appending to security_hook_heads is currently missing).
How do you plan to handle LKM-based LSMs?
My position all along has been that I don't plan to handle LKM
based LSMs, but that I won't do anything to prevent someone else
from adding them later. I believe that I've done that. Several
designs, including a separate list for dynamically loaded modules
have been proposed. I think some of those would work.
Why does this make sense? If the lsm_file_cache isn't
initialized you can't have allocated any file blobs,
no module can have initialized a file blob, hence there
can be nothing for the module to do.
Soon. Real soon. I hope. I would very much like for
someone from the SELinux camp to chime in, especially on
the selinux_is_enabled() removal.
Agreed.
This patchset from Casey lands before the patchset from Kees, doesn't it?
That is up for negotiation. We may end up combining them.
quoted
OK, a few comments (if I didn't overlook something).
?? lsm_early_cred()/lsm_early_task() are called from only __init functions.
True.
quoted
?? lsm_cred_alloc()/lsm_file_alloc() are called from only security/security.c .
Also true.
quoted
?? lsm_early_inode() should be avoided because it is not appropriate to
?? call panic() when lsm_early_inode() is called after __init phase.
You're correct. In fact, lsm_early_inode() isn't needed at all
until multiple inode using modules are supported.
quoted
?? Since all free hooks are called when one of init hooks failed, each
?? free hook needs to check whether init hook was called. An example is
?? inode_free_security() in security/selinux/hooks.c (but not addressed in
?? this patch).
I *think* that selinux_inode_free_security() is safe in this
case because the blob will be zeroed, hence isec->list will
be NULL.
That's not safe - look more closely at what list_empty_careful() tests, and then think about what happens when list_del_init() gets called on that isec->list.? selinux_inode_free_security() presumes that selinux_inode_alloc_security() has been called already.? If you are breaking that assumption, you have to fix it.
Yup. I misread the macro my first time around. Easy fix.
Is there a reason you can't make inode_alloc_security() return void since you moved the allocation to the framework?
No reason with any of the existing modules, But I could see someone
doing unnatural things during allocation that might result in a
failure.
Unfortunate that inode_init_security name is already in use for another purpose since essentially you have reduced these hooks to initialization only.
I considered that but decided that it makes more sense for the module hook names
to match the infrastructure name. Having security_inode_alloc() call
selinux_inode_setup_security() starts to get confusing.
Since all free hooks are called when one of init hooks failed, each
free hook needs to check whether init hook was called. An example is
inode_free_security() in security/selinux/hooks.c (but not addressed in
this patch).
I *think* that selinux_inode_free_security() is safe in this
case because the blob will be zeroed, hence isec->list will
be NULL.
OK.
quoted
quoted
This patchset might fatally prevent LKM-based LSM modules, for LKM-based
LSMs cannot count on lsm_*_alloc() because size for lsm_*_alloc() cannot
be updated upon loading LKM-based LSMs.
LKM based security modules will require dynamically sized blobs.
These can be added to the scheme used here. Each blob would get a
header identifying the modules for which it contains data. When an
LKM is registered if has to declare it's blob space requirements
and gets back the offsets. All alloc operations have to put their
marks in the header. All LKM blob users have to check that the blob
they are looking at has the required data.
module_cred(struct cred *cred) {
return cred->security + module_blob_sizes.lbs_cred;
}
becomes
module_cred(struct cred *cred) {
if (blob_includes(module_id))
return cred->security + module_blob_sizes.lbs_cred;
return NULL;
}
and the calling code needs to accept a NULL return.
Not all of LKM-based LSMs use security blobs. And some of LKM-based LSMs
might use security blobs for only a few objects. For example, AKARI uses
inode security blob for remembering whether source address/port of an
accept()ed socket was already checked, only during accept() operation and
first socket operation on the accept()ed socket. Thus, there is no need
to waste memory by assigning blobs for all inode objects.
The first question is why use an inode blob? Shouldn't you
be using a socket blob for this socket based information?
If you only want information part of the time you can declare
a pointer sized blob and manage what hangs off that as you will.
I personally think that the added complexity of conditional
blob management is more pain than it's worth, but if you want
a really big blob, but only on occasion, I could see doing it.
quoted
Blobs can never get smaller because readjusting the offsets
isn't going to work, so unloading an LKM security module isn't
going to be as complete as you might like. There may be a way
around this if you unload all the LKM modules, but that's a
special case and there may be dragon lurking in the mist.
If LKM-based LSMs who want to use security blobs have to check for
NULL return, they might choose "not using infrastructure managed
security blobs" and "using locally hashed blobs associated with
object's address" (like AKARI does).
I can't see how a check for NULL could possibly be a bigger
hassle than doing your own locally hashed blobs.
quoted
quoted
If security_file_free() is called
regardless of whether lsm_file_cache is defined, LKM-based LSMs can be
loaded using current behavior (apart from the fact that legitimate
interface for appending to security_hook_heads is currently missing).
How do you plan to handle LKM-based LSMs?
My position all along has been that I don't plan to handle LKM
based LSMs, but that I won't do anything to prevent someone else
from adding them later. I believe that I've done that. Several
designs, including a separate list for dynamically loaded modules
have been proposed. I think some of those would work.
Though AKARI is not using security_file_free(), some of LKM-based LSMs
might want to use it. If file_free_security hook is called unconditionally,
such LKM-based LSMs can be registered/unregistered, without worrying about
inability to shrink sizes for blobs.
The infrastructure wouldn't call unregistered hooks, so any module
that allocates additional memory attached to a blob is going to have
to deal with freeing that when it unregisters. Aside from that unregistration
should be a (not so) small matter of locking.
Why does this make sense? If the lsm_file_cache isn't
initialized you can't have allocated any file blobs,
no module can have initialized a file blob, hence there
can be nothing for the module to do.
For modules (not limited to LKM-based LSMs) which want to use
file blobs for only a few objects and avoid wasting memory by
allocating file blobs to all file objects.
Infrastructure based blob management fits well for LSM modules
which want to assign blobs to all objects (like SELinux). But
forcing infrastructure based blob management can become a huge
waste of memory for LSM modules which want to assign blobs to
only a few objects. Unconditionally calling file_free_security
hook (as with other hooks) preserves a room for allowing the
latter type of LSM modules without using infrastructure based
blob management.
There is a hypothetical issue here, but that would require abuse
of the infrastructure. Having a file_free_security hook that doesn't
free a security blob allocated by file_alloc_security may coincidentaly
be useful, but that's not the intent of the hook.
?? Since all free hooks are called when one of init hooks failed, each
?? free hook needs to check whether init hook was called. An example is
?? inode_free_security() in security/selinux/hooks.c (but not addressed in
?? this patch).
I *think* that selinux_inode_free_security() is safe in this
case because the blob will be zeroed, hence isec->list will
be NULL.
That's not safe - look more closely at what list_empty_careful() tests, and then think about what happens when list_del_init() gets called on that isec->list.? selinux_inode_free_security() presumes that selinux_inode_alloc_security() has been called already.? If you are breaking that assumption, you have to fix it.
Yup. I misread the macro my first time around. Easy fix.
Oh, I didn't notice that it is doing !list_empty_careful() than list_empty_careful().
Unsafe indeed. But easy to fix.
quoted
Is there a reason you can't make inode_alloc_security() return void since you moved the allocation to the framework?
No reason with any of the existing modules, But I could see someone
doing unnatural things during allocation that might result in a
failure.
Currently upstreamed LSM modules and AKARI would be OK. But I can't guarantee it
for future / not-yet-upstreamed LSM modules.
Not all of LKM-based LSMs use security blobs. And some of LKM-based LSMs
might use security blobs for only a few objects. For example, AKARI uses
inode security blob for remembering whether source address/port of an
accept()ed socket was already checked, only during accept() operation and
first socket operation on the accept()ed socket. Thus, there is no need
to waste memory by assigning blobs for all inode objects.
The first question is why use an inode blob? Shouldn't you
be using a socket blob for this socket based information?
Indeed. AKARI can as well use security_sk_free() using address of
"struct sock" as a key.
If you only want information part of the time you can declare
a pointer sized blob and manage what hangs off that as you will.
I personally think that the added complexity of conditional
blob management is more pain than it's worth, but if you want
a really big blob, but only on occasion, I could see doing it.
LKM based LSMs are too late for updating blob_sizes.* fields.
Even if they could, they after all have to somehow check whether
corresponding init hook was called. That's checking for NULL.
Why does this make sense? If the lsm_file_cache isn't
initialized you can't have allocated any file blobs,
no module can have initialized a file blob, hence there
can be nothing for the module to do.
For modules (not limited to LKM-based LSMs) which want to use
file blobs for only a few objects and avoid wasting memory by
allocating file blobs to all file objects.
Infrastructure based blob management fits well for LSM modules
which want to assign blobs to all objects (like SELinux). But
forcing infrastructure based blob management can become a huge
waste of memory for LSM modules which want to assign blobs to
only a few objects. Unconditionally calling file_free_security
hook (as with other hooks) preserves a room for allowing the
latter type of LSM modules without using infrastructure based
blob management.
There is a hypothetical issue here, but that would require abuse
of the infrastructure. Having a file_free_security hook that doesn't
free a security blob allocated by file_alloc_security may coincidentaly
be useful, but that's not the intent of the hook.
The free hook might be used for freeing resources which were not allocated
by alloc hook. Yama is using task_free hook without task_alloc hook.
Someone might want to use file_free hook without file_alloc hook.
Not all of LKM-based LSMs use security blobs. And some of LKM-based LSMs
might use security blobs for only a few objects. For example, AKARI uses
inode security blob for remembering whether source address/port of an
accept()ed socket was already checked, only during accept() operation and
first socket operation on the accept()ed socket. Thus, there is no need
to waste memory by assigning blobs for all inode objects.
The first question is why use an inode blob? Shouldn't you
be using a socket blob for this socket based information?
Indeed. AKARI can as well use security_sk_free() using address of
"struct sock" as a key.
quoted
If you only want information part of the time you can declare
a pointer sized blob and manage what hangs off that as you will.
I personally think that the added complexity of conditional
blob management is more pain than it's worth, but if you want
a really big blob, but only on occasion, I could see doing it.
LKM based LSMs are too late for updating blob_sizes.* fields.
That is true with the code in this patch set. As I mentioned,
changing the blob handling to include a header with real use
information would be required.
Even if they could, they after all have to somehow check whether
corresponding init hook was called. That's checking for NULL.
Why does this make sense? If the lsm_file_cache isn't
initialized you can't have allocated any file blobs,
no module can have initialized a file blob, hence there
can be nothing for the module to do.
For modules (not limited to LKM-based LSMs) which want to use
file blobs for only a few objects and avoid wasting memory by
allocating file blobs to all file objects.
Infrastructure based blob management fits well for LSM modules
which want to assign blobs to all objects (like SELinux). But
forcing infrastructure based blob management can become a huge
waste of memory for LSM modules which want to assign blobs to
only a few objects. Unconditionally calling file_free_security
hook (as with other hooks) preserves a room for allowing the
latter type of LSM modules without using infrastructure based
blob management.
There is a hypothetical issue here, but that would require abuse
of the infrastructure. Having a file_free_security hook that doesn't
free a security blob allocated by file_alloc_security may coincidentaly
be useful, but that's not the intent of the hook.
The free hook might be used for freeing resources which were not allocated
by alloc hook. Yama is using task_free hook without task_alloc hook.
Someone might want to use file_free hook without file_alloc hook.
OK, you're correct. Checking for an initialized kmem_cache isn't appropriate.
Instead of checking if the kmem_cache for file blobs
has been initialized check if the blob is NULL. This
allows non-blob using modules to do other kinds of
clean up in the security_file_free hooks.
Signed-off-by: Casey Schaufler <casey@schaufler-ca.com>
---
security/security.c | 9 ++++-----
1 file changed, 4 insertions(+), 5 deletions(-)
lsm_early_cred()/lsm_early_task() are called from only __init functions.
lsm_cred_alloc()/lsm_file_alloc() are called from only security/security.c .
lsm_early_inode() should be avoided because it is not appropriate to
call panic() when lsm_early_inode() is called after __init phase.
Since all free hooks are called when one of init hooks failed, each
free hook needs to check whether init hook was called.
The original changes are from Tetsuo Handa. I have made minor
changes in some places, but this is mostly his code.
Signed-off-by: Casey Schaufler <casey@schaufler-ca.com>
---
include/linux/lsm_hooks.h | 6 ++----
security/security.c | 27 ++++-----------------------
security/selinux/hooks.c | 5 ++++-
security/selinux/include/objsec.h | 2 ++
security/smack/smack_lsm.c | 8 +++++++-
5 files changed, 19 insertions(+), 29 deletions(-)
From: James Morris <jmorris@namei.org> Date: 2018-09-28 04:34:14
On Fri, 21 Sep 2018, Casey Schaufler wrote:
The SELinux specific credential poisioning only makes sense
if SELinux is managing the credentials. As the intent of this
patch set is to move the blob management out of the modules
and into the infrastructure, the SELinux specific code has
to go. The poisioning could be introduced into the infrastructure
at some later date.
If it's useful, it should be incorporated into core LSM, otherwise that's
a regression for SELinux.
--
James Morris
[off-list ref]
The SELinux specific credential poisioning only makes sense
if SELinux is managing the credentials. As the intent of this
patch set is to move the blob management out of the modules
and into the infrastructure, the SELinux specific code has
to go. The poisioning could be introduced into the infrastructure
at some later date.
If it's useful, it should be incorporated into core LSM, otherwise that's
a regression for SELinux
When I discussed this code with David Howells he indicated
that it was primarily used for debugging the original shared
credential implementation and that is was not especially
valuable any longer. If someone thinks it is valuable we
should consider doing it in the infrastructure for all the
blobs, not just the credential.
On Wed, Sep 26, 2018 at 2:57 PM, Casey Schaufler [off-list ref] wrote:
Instead of checking if the kmem_cache for file blobs
has been initialized check if the blob is NULL. This
allows non-blob using modules to do other kinds of
clean up in the security_file_free hooks.
Signed-off-by: Casey Schaufler <casey@schaufler-ca.com>
Reviewed-by: Kees Cook <redacted>
This looks like it should get folded into "LSM: Infrastructure
management of the file security".
-Kees
On Wed, Sep 26, 2018 at 2:57 PM, Casey Schaufler [off-list ref] wrote:
lsm_early_cred()/lsm_early_task() are called from only __init functions.
lsm_cred_alloc()/lsm_file_alloc() are called from only security/security.c .
lsm_early_inode() should be avoided because it is not appropriate to
call panic() when lsm_early_inode() is called after __init phase.
Since all free hooks are called when one of init hooks failed, each
free hook needs to check whether init hook was called.
The original changes are from Tetsuo Handa. I have made minor
changes in some places, but this is mostly his code.
Signed-off-by: Casey Schaufler <casey@schaufler-ca.com>
---
include/linux/lsm_hooks.h | 6 ++----
security/security.c | 27 ++++-----------------------
security/selinux/hooks.c | 5 ++++-
security/selinux/include/objsec.h | 2 ++
security/smack/smack_lsm.c | 8 +++++++-
5 files changed, 19 insertions(+), 29 deletions(-)
I've split this across the various commits they touch:
Infrastructure management of the cred security blob
LSM: Infrastructure management of the file security
LSM: Infrastructure management of the inode security
LSM: Infrastructure management of the task security
LSM: Blob sharing support for S.A.R.A and LandLock
Based on these changes, I've uploaded the "v4.1", or "Casey is on
vacation", tree here:
https://git.kernel.org/pub/scm/linux/kernel/git/kees/linux.git/log/?h=lsm/blob-sharing-v4.1
I'm going to work on a merged series for the "arbitrary ordering" and
"blob-sharing" trees next...
-Kees
--
Kees Cook
Pixel Security
From: James Morris <jmorris@namei.org> Date: 2018-10-04 01:02:57
On Fri, 21 Sep 2018, Kees Cook wrote:
On Fri, Sep 21, 2018 at 5:19 PM, Casey Schaufler [off-list ref] wrote:
quoted
+ * lsm_early_inode - during initialization allocate a composite inode blob
+ * @inode: the inode that needs a blob
+ *
+ * Allocate the inode blob for all the modules if it's not already there
+ */
+void lsm_early_inode(struct inode *inode)
+{
+ int rc;
+
+ if (inode == NULL)
+ panic("%s: NULL inode.\n", __func__);
+ if (inode->i_security != NULL)
+ return;
+ rc = lsm_inode_alloc(inode);
+ if (rc)
+ panic("%s: Early inode alloc failed.\n", __func__);
+}
I'm still advising against using panic(), but I'll leave it up to James.
Calling panic() is not appropriate here. Perhaps if it was during
boot-time initialization of LSM infrastructure, but not on the fly.
Use a WARN_ONCE then propagate the error back and fail the operation.
--
James Morris
[off-list ref]
On Fri, Sep 21, 2018 at 5:19 PM, Casey Schaufler [off-list ref] wrote:
quoted
+ * lsm_early_inode - during initialization allocate a composite inode blob
+ * @inode: the inode that needs a blob
+ *
+ * Allocate the inode blob for all the modules if it's not already there
+ */
+void lsm_early_inode(struct inode *inode)
+{
+ int rc;
+
+ if (inode == NULL)
+ panic("%s: NULL inode.\n", __func__);
+ if (inode->i_security != NULL)
+ return;
+ rc = lsm_inode_alloc(inode);
+ if (rc)
+ panic("%s: Early inode alloc failed.\n", __func__);
+}
I'm still advising against using panic(), but I'll leave it up to James.
Calling panic() is not appropriate here. Perhaps if it was during
boot-time initialization of LSM infrastructure, but not on the fly.
Tetsuo's patch makes this an __init function. It's only for doing
init time stuff like root inode initialization during start-up.
If it fails the caller is going to have to panic. This came straight
out of the SELinux system initialization code. I could go back to
having each LSM do it's own panic, but that seems silly.
Use a WARN_ONCE then propagate the error back and fail the operation.
On Mon, Oct 1, 2018 at 2:48 PM, Kees Cook [off-list ref] wrote:
On Wed, Sep 26, 2018 at 2:57 PM, Casey Schaufler [off-list ref] wrote:
quoted
lsm_early_cred()/lsm_early_task() are called from only __init functions.
lsm_cred_alloc()/lsm_file_alloc() are called from only security/security.c .
lsm_early_inode() should be avoided because it is not appropriate to
call panic() when lsm_early_inode() is called after __init phase.
Since all free hooks are called when one of init hooks failed, each
free hook needs to check whether init hook was called.
The original changes are from Tetsuo Handa. I have made minor
changes in some places, but this is mostly his code.
Signed-off-by: Casey Schaufler <casey@schaufler-ca.com>
---
include/linux/lsm_hooks.h | 6 ++----
security/security.c | 27 ++++-----------------------
security/selinux/hooks.c | 5 ++++-
security/selinux/include/objsec.h | 2 ++
security/smack/smack_lsm.c | 8 +++++++-
5 files changed, 19 insertions(+), 29 deletions(-)
I've split this across the various commits they touch:
Infrastructure management of the cred security blob
LSM: Infrastructure management of the file security
LSM: Infrastructure management of the inode security
LSM: Infrastructure management of the task security
LSM: Blob sharing support for S.A.R.A and LandLock
Based on these changes, I've uploaded the "v4.1", or "Casey is on
vacation", tree here:
https://git.kernel.org/pub/scm/linux/kernel/git/kees/linux.git/log/?h=lsm/blob-sharing-v4.1
I'm going to work on a merged series for the "arbitrary ordering" and
"blob-sharing" trees next...
Here is my v6 (v5 plus small fix I noticed) with my refactoring of
Casey's blob-sharing series on top:
https://git.kernel.org/pub/scm/linux/kernel/git/kees/linux.git/log/?h=lsm/ordering-v6-blob-sharing
procfs: add smack subdir to attrs
Smack: Abstract use of cred security blob
SELinux: Abstract use of cred security blob
SELinux: Remove cred security blob poisoning
SELinux: Remove unused selinux_is_enabled
AppArmor: Abstract use of cred security blob
TOMOYO: Abstract use of cred security blob
Infrastructure management of the cred security blob
SELinux: Abstract use of file security blob
Smack: Abstract use of file security blob
LSM: Infrastructure management of the file security
SELinux: Abstract use of inode security blob
Smack: Abstract use of inode security blob
LSM: Infrastructure management of the inode security
LSM: Infrastructure management of the task security
SELinux: Abstract use of ipc security blobs
Smack: Abstract use of ipc security blobs
LSM: Infrastructure management of the ipc security blob
TOMOYO: Update LSM flags to no longer be exclusive
Notably, the last patch from Casey's series is entirely removed.
Additionally all the per-LSM initialization changes were removed since
the blob size calculations now stay internal to security.c, done
during the "prepare" phase.
-Kees
--
Kees Cook
Pixel Security