AppArmor security module overview
Subsystems (23)
PATCH 38/41 AppArmor: Module and LSM hooks
In reply to [email protected]Module parameters, LSM hooks, initialization and teardown.
--- security/apparmor/lsm.c | 829 ++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 829 insertions(+)
security/apparmor/lsm.c new file +829 -0
@@ -0,0 +1,829 @@ +/* +* Copyright (C) 1998-2007 Novell/SUSE +* +* This program is free software; you can redistribute it and/or +* modify it under the terms of the GNU General Public License as +* published by the Free Software Foundation, version 2 of the +* License. +* +* AppArmor LSM interface +*/ + +#include <linux/security.h> +#include <linux/module.h> +#include <linux/mm.h> +#include <linux/mman.h> +#include <linux/mount.h> +#include <linux/namei.h> +#include <linux/ctype.h> + +#include "apparmor.h" +#include "inline.h" + +static int param_set_aabool (const char *val,struct kernel_param *kp); +static int param_get_aabool (char *buffer,struct kernel_param *kp); +#define param_check_aabool (name, p) __param_check(name, p, int) + +static int param_set_aauint (const char *val,struct kernel_param *kp); +static int param_get_aauint (char *buffer,struct kernel_param *kp); +#define param_check_aauint (name, p) __param_check(name, p, int) + +/* Flag values, also controllable via /sys/module/apparmor/parameters +* We define special types as we want to do additional mediation. +* +* Complain mode -- in complain mode access failures result in auditing only +* and task is allowed access. audit events are processed by userspace to +* generate policy. Default is 'enforce' (0). +* Value is also togglable per profile and referenced when global value is +* enforce. +*/ +int apparmor_complain =0 ; +module_param_named (complain, apparmor_complain, aabool,S_IRUSR |S_IWUSR ); +MODULE_PARM_DESC (apparmor_complain,"Toggle AppArmor complain mode" ); + +/* Debug mode */ +int apparmor_debug =0 ; +module_param_named (debug, apparmor_debug, aabool,S_IRUSR |S_IWUSR ); +MODULE_PARM_DESC (apparmor_debug,"Toggle AppArmor debug mode" ); + +/* Audit mode */ +int apparmor_audit =0 ; +module_param_named (audit, apparmor_audit, aabool,S_IRUSR |S_IWUSR ); +MODULE_PARM_DESC (apparmor_audit,"Toggle AppArmor audit mode" ); + +/* Syscall logging mode */ +int apparmor_logsyscall =0 ; +module_param_named (logsyscall, apparmor_logsyscall, aabool,S_IRUSR |S_IWUSR ); +MODULE_PARM_DESC (apparmor_logsyscall,"Toggle AppArmor logsyscall mode" ); + +/* Maximum pathname length before accesses will start getting rejected */ +unsigned int apparmor_path_max =2 *PATH_MAX ; +module_param_named (path_max, apparmor_path_max, aauint,S_IRUSR |S_IWUSR ); +MODULE_PARM_DESC (apparmor_path_max,"Maximum pathname length allowed" ); + +static int param_set_aabool (const char *val,struct kernel_param *kp) +{ +if (aa_task_context (current)) +return -EPERM ; +return param_set_bool (val, kp); +} + +static int param_get_aabool (char *buffer,struct kernel_param *kp) +{ +if (aa_task_context (current)) +return -EPERM ; +return param_get_bool (buffer, kp); +} + +static int param_set_aauint (const char *val,struct kernel_param *kp) +{ +if (aa_task_context (current)) +return -EPERM ; +return param_set_uint (val, kp); +} + +static int param_get_aauint (char *buffer,struct kernel_param *kp) +{ +if (aa_task_context (current)) +return -EPERM ; +return param_get_uint (buffer, kp); +} + +static int aa_reject_syscall (struct task_struct *task,gfp_t flags, +const char *name) +{ +struct aa_profile *profile =aa_get_profile (task); +int error =0 ; + +if (profile) { + error =aa_audit_syscallreject (profile, flags, name); +aa_put_profile (profile); + } + +return error; +} + +static int apparmor_ptrace (struct task_struct *parent, +struct task_struct *child) +{ +struct aa_task_context *cxt; +struct aa_task_context *child_cxt; +struct aa_profile *child_profile; +int error =0 ; + +/** +* parent can ptrace child when +* - parent is unconfined +* - parent is in complain mode +* - parent and child are confined by the same profile +*/ + +rcu_read_lock (); + cxt =aa_task_context (parent); + child_cxt =aa_task_context (child); + child_profile = child_cxt ? child_cxt->profile :NULL ; + error =aa_may_ptrace (cxt, child_profile); +if (cxt &&PROFILE_COMPLAIN (cxt->profile)) { +LOG_HINT (cxt->profile,GFP_ATOMIC ,HINT_PTRACE , +"pid=%d child=%d\n" , + current->pid, child->pid); + } +rcu_read_unlock (); + +return error; +} + +static int apparmor_capget (struct task_struct *task, +kernel_cap_t *effective, +kernel_cap_t *inheritable, +kernel_cap_t *permitted) +{ +return cap_capget (task, effective, inheritable, permitted); +} + +static int apparmor_capset_check (struct task_struct *task, +kernel_cap_t *effective, +kernel_cap_t *inheritable, +kernel_cap_t *permitted) +{ +return cap_capset_check (task, effective, inheritable, permitted); +} + +static void apparmor_capset_set (struct task_struct *task, +kernel_cap_t *effective, +kernel_cap_t *inheritable, +kernel_cap_t *permitted) +{ +cap_capset_set (task, effective, inheritable, permitted); +} + +static int apparmor_capable (struct task_struct *task,int cap) +{ +int error; + +/* cap_capable returns 0 on success, else -EPERM */ + error =cap_capable (task, cap); + +if (!error) { +struct aa_task_context *cxt; + +rcu_read_lock (); + cxt =aa_task_context (task); +if (cxt) + error =aa_capability (cxt, cap); +rcu_read_unlock (); + } + +return error; +} + +static int apparmor_sysctl (struct ctl_table *table,int op) +{ +int error =0 ; + +if ((op &002 ) && !capable (CAP_SYS_ADMIN )) + error =aa_reject_syscall (current,GFP_KERNEL , +"sysctl (write)" ); + +return error; +} + +static int apparmor_syslog (int type) +{ +return cap_syslog (type); +} + +static int apparmor_netlink_send (struct sock *sk,struct sk_buff *skb) +{ +return cap_netlink_send (sk, skb); +} + +static int apparmor_netlink_recv (struct sk_buff *skb,int cap) +{ +return cap_netlink_recv (skb, cap); +} + +static void apparmor_bprm_apply_creds (struct linux_binprm *bprm,int unsafe) +{ +cap_bprm_apply_creds (bprm, unsafe); +} + +static int apparmor_bprm_set_security (struct linux_binprm *bprm) +{ +/* handle capability bits with setuid, etc */ +cap_bprm_set_security (bprm); +/* already set based on script name */ +if (bprm->sh_bang) +return 0 ; +return aa_register (bprm); +} + +static int apparmor_bprm_secureexec (struct linux_binprm *bprm) +{ +int ret =cap_bprm_secureexec (bprm); + +if (!ret && (unsigned long )bprm->security &AA_SECURE_EXEC_NEEDED ) { +AA_DEBUG ("%s: secureexec required for %s\n" , + __FUNCTION__, bprm->filename); + ret =1 ; + } + +return ret; +} + +static int apparmor_sb_mount (char *dev_name,struct nameidata *nd,char *type, +unsigned long flags,void *data) +{ +return aa_reject_syscall (current,GFP_KERNEL ,"mount" ); +} + +static int apparmor_umount (struct vfsmount *mnt,int flags) +{ +return aa_reject_syscall (current,GFP_KERNEL ,"umount" ); +} + +static int apparmor_inode_mkdir (struct inode *dir,struct dentry *dentry, +struct vfsmount *mnt,int mask) +{ +struct aa_profile *profile; +int error =0 ; + +if (!mnt || !mediated_filesystem (dir)) + goto out; + + profile =aa_get_profile (current); + +if (profile) + error =aa_perm_dir (profile, dentry, mnt,"mkdir" ,MAY_WRITE ); + +aa_put_profile (profile); + +out: +return error; +} + +static int apparmor_inode_rmdir (struct inode *dir,struct dentry *dentry, +struct vfsmount *mnt) +{ +struct aa_profile *profile; +int error =0 ; + +if (!mnt || !mediated_filesystem (dir)) + goto out; + + profile =aa_get_profile (current); + +if (profile) + error =aa_perm_dir (profile, dentry, mnt,"rmdir" ,MAY_WRITE ); + +aa_put_profile (profile); + +out: +return error; +} + +static int aa_permission (struct inode *inode,struct dentry *dentry, +struct vfsmount *mnt,int mask,int check) +{ +int error =0 ; + +if (mnt &&mediated_filesystem (inode)) { +struct aa_profile *profile; + + profile =aa_get_profile (current); +if (profile) + error =aa_perm (profile, dentry, mnt, mask, check); +aa_put_profile (profile); + } +return error; +} + +static int apparmor_inode_create (struct inode *dir,struct dentry *dentry, +struct vfsmount *mnt,int mask) +{ +return aa_permission (dir, dentry, mnt,MAY_WRITE ,AA_CHECK_LEAF ); +} + +static int apparmor_inode_link (struct dentry *old_dentry, +struct vfsmount *old_mnt,struct inode *dir, +struct dentry *new_dentry, +struct vfsmount *new_mnt) +{ +int error =0 ; +struct aa_profile *profile; + +if (!old_mnt || !new_mnt || !mediated_filesystem (dir)) + goto out; + + profile =aa_get_profile (current); + +if (profile) + error =aa_link (profile, new_dentry, new_mnt, + old_dentry, old_mnt); + +aa_put_profile (profile); + +out: +return error; +} + +static int apparmor_inode_unlink (struct inode *dir,struct dentry *dentry, +struct vfsmount *mnt) +{ +int check =AA_CHECK_LEAF ; + +if (S_ISDIR (dentry->d_inode->i_mode)) + check |=AA_CHECK_DIR ; +return aa_permission (dir, dentry, mnt,MAY_WRITE , check); +} + +static int apparmor_inode_symlink (struct inode *dir,struct dentry *dentry, +struct vfsmount *mnt,const char *old_name) +{ +return aa_permission (dir, dentry, mnt,MAY_WRITE ,AA_CHECK_LEAF ); +} + +static int apparmor_inode_mknod (struct inode *dir,struct dentry *dentry, +struct vfsmount *mnt,int mode,dev_t dev) +{ +return aa_permission (dir, dentry, mnt,MAY_WRITE ,AA_CHECK_LEAF ); +} + +static int apparmor_inode_rename (struct inode *old_dir, +struct dentry *old_dentry, +struct vfsmount *old_mnt, +struct inode *new_dir, +struct dentry *new_dentry, +struct vfsmount *new_mnt) +{ +struct aa_profile *profile; +int error =0 ; + +if ((!old_mnt && !new_mnt) || !mediated_filesystem (old_dir)) + goto out; + + profile =aa_get_profile (current); + +if (profile) { +struct inode *inode = old_dentry->d_inode; +int check =AA_CHECK_LEAF ; + +if (inode &&S_ISDIR (inode->i_mode)) + check |=AA_CHECK_DIR ; +if (old_mnt) + error =aa_perm (profile, old_dentry, old_mnt, +MAY_READ |MAY_WRITE , check); + +if (!error && new_mnt) { + error =aa_perm (profile, new_dentry, new_mnt, +MAY_WRITE , check); + } + } + +aa_put_profile (profile); + +out: +return error; +} + +static int apparmor_inode_permission (struct inode *inode,int mask, +struct nameidata *nd) +{ +int check =0 ; + +if (!nd) +return 0 ; +if (S_ISDIR (inode->i_mode)) + check |=AA_CHECK_DIR ; + mask &= (MAY_READ |MAY_WRITE |MAY_EXEC ); + +/* Assume we are not checking a leaf directory. */ +return aa_permission (inode, nd->dentry, nd->mnt, mask, check); +} + +static int apparmor_inode_setattr (struct dentry *dentry,struct vfsmount *mnt, +struct iattr *iattr) +{ +int error =0 ; + +if (!mnt) + goto out; + +if (mediated_filesystem (dentry->d_inode)) { +struct aa_profile *profile; + + profile =aa_get_profile (current); +/* +* Mediate any attempt to change attributes of a file +* (chmod, chown, chgrp, etc) +*/ +if (profile) + error =aa_attr (profile, dentry, mnt, iattr); + +aa_put_profile (profile); + } + +out: +return error; +} + +static int aa_xattr_permission (struct dentry *dentry,struct vfsmount *mnt, +const char *name,const char *operation, +int mask,struct file *file) +{ +int error =0 ; + +if (mnt &&mediated_filesystem (dentry->d_inode)) { +struct aa_profile *profile =aa_get_profile (current); +int check = file ?AA_CHECK_FD :0 ; + +if (profile) + error =aa_perm_xattr (profile, dentry, mnt, name, + operation, mask, check); +aa_put_profile (profile); + } + +return error; +} + +static int apparmor_inode_setxattr (struct dentry *dentry,struct vfsmount *mnt, +char *name,void *value,size_t size, +int flags,struct file *file) +{ +return aa_xattr_permission (dentry, mnt, name,"xattr set" ,MAY_WRITE , + file); +} + +static int apparmor_inode_getxattr (struct dentry *dentry,struct vfsmount *mnt, +char *name,struct file *file) +{ +return aa_xattr_permission (dentry, mnt, name,"xattr get" ,MAY_READ , + file); +} + +static int apparmor_inode_listxattr (struct dentry *dentry,struct vfsmount *mnt, +struct file *file) +{ +return aa_xattr_permission (dentry, mnt,NULL ,"xattr list" ,MAY_READ , + file); +} + +static int apparmor_inode_removexattr (struct dentry *dentry, +struct vfsmount *mnt,char *name, +struct file *file) +{ +return aa_xattr_permission (dentry, mnt, name,"xattr remove" , +MAY_WRITE , file); +} + +static int apparmor_file_permission (struct file *file,int mask) +{ +struct aa_profile *profile; +struct aa_profile *file_profile = (struct aa_profile *)file->f_security; +int error =0 ; + +if (!file_profile) + goto out; + +/* +* If this file was opened under a different profile, we +* revalidate the access against the current profile. +*/ + profile =aa_get_profile (current); +if (profile && file_profile != profile) { +struct dentry *dentry = file->f_dentry; +struct vfsmount *mnt = file->f_vfsmnt; +struct inode *inode = dentry->d_inode; +int check =AA_CHECK_LEAF |AA_CHECK_FD ; + +/* +* FIXME: We should remember which profiles we revalidated +* against. +*/ +if (S_ISDIR (inode->i_mode)) + check |=AA_CHECK_DIR ; + mask &= (MAY_READ |MAY_WRITE |MAY_EXEC ); + error =aa_permission (inode, dentry, mnt, mask, check); + } +aa_put_profile (profile); + +out: +return error; +} + +static int apparmor_task_create (unsigned long clone_flags) +{ +struct aa_profile *profile; +int error =0 ; + + profile =aa_get_profile (current); +if (profile) { +/* Don't allow to create new namespaces. */ +if (clone_flags &CLONE_NEWNS ) + error = -EPERM ; + } +aa_put_profile (profile); + +return error; +} + +static int apparmor_file_alloc_security (struct file *file) +{ +struct aa_profile *profile; + + profile =aa_get_profile (current); +if (profile) + file->f_security = profile; + +return 0 ; +} + +static void apparmor_file_free_security (struct file *file) +{ +struct aa_profile *file_profile = (struct aa_profile *)file->f_security; + +aa_put_profile (file_profile); +} + +static inline int aa_mmap (struct file *file,unsigned long prot, +unsigned long flags) +{ +struct dentry *dentry; +int mask =0 ; + +if (!file || !file->f_security) +return 0 ; + +if (prot &PROT_READ ) + mask |=MAY_READ ; +/* Private mappings don't require write perms since they don't +* write back to the files */ +if ((prot &PROT_WRITE ) && !(flags &MAP_PRIVATE )) + mask |=MAY_WRITE ; +if (prot &PROT_EXEC ) + mask |=AA_EXEC_MMAP ; + + dentry = file->f_dentry; +return aa_permission (dentry->d_inode, dentry, file->f_vfsmnt, mask, +AA_CHECK_LEAF |AA_CHECK_FD ); +} + +static int apparmor_file_mmap (struct file *file,unsigned long reqprot, +unsigned long prot,unsigned long flags) +{ +return aa_mmap (file, prot, flags); +} + +static int apparmor_file_mprotect (struct vm_area_struct *vma, +unsigned long reqprot,unsigned long prot) +{ +return aa_mmap (vma->vm_file, prot, + !(vma->vm_flags &VM_SHARED ) ?MAP_PRIVATE :0 ); +} + +static int apparmor_task_alloc_security (struct task_struct *task) +{ +return aa_clone (task); +} + +/* +* Called from IRQ context from RCU callback. +*/ +static void apparmor_task_free_security (struct task_struct *task) +{ +aa_release (task); +} + +static int apparmor_task_post_setuid (uid_t id0,uid_t id1,uid_t id2, +int flags) +{ +return cap_task_post_setuid (id0, id1, id2, flags); +} + +static void apparmor_task_reparent_to_init (struct task_struct *task) +{ +cap_task_reparent_to_init (task); +} + +static int apparmor_getprocattr (struct task_struct *task,char *name, +char **value) +{ +unsigned len; +int error; +struct aa_profile *profile; + +/* AppArmor only supports the "current" process attribute */ +if (strcmp (name,"current" ) !=0 ) +return -EINVAL ; + +/* must be task querying itself or admin */ +if (current != task && !capable (CAP_SYS_ADMIN )) +return -EPERM ; + + profile =aa_get_profile (task); + error =aa_getprocattr (profile, value, &len); +aa_put_profile (profile); +if (!error) + error = len; + +return error; +} + +static int apparmor_setprocattr (struct task_struct *task,char *name, +void *value,size_t size) +{ +char *command, *args; +int error; + +if (strcmp (name,"current" ) !=0 || size ==0 || size >=PAGE_SIZE ) +return -EINVAL ; + args = value; + args[size] ='\0' ; + args =strstrip (args); + command =strsep (&args," " ); +if (!args) +return -EINVAL ; +while (isspace (*args)) + args++; +if (!*args) +return -EINVAL ; + +if (strcmp (command,"changehat" ) ==0 ) { +if (current != task) +return -EACCES ; + error =aa_setprocattr_changehat (args); + }else if (strcmp (command,"setprofile" )) { +struct aa_profile *profile; + +/* Only an unconfined process with admin capabilities +* may change the profile of another task. +*/ + +if (!capable (CAP_SYS_ADMIN )) +return -EACCES ; + + profile =aa_get_profile (current); +if (profile) { +aa_put_profile (profile); +AA_WARN (GFP_KERNEL , +"Attempt by confined task %s(%d) " +"[user %d] to assign profile to task %s(%d)" , + current->comm, + current->pid, + current->uid, + task->comm, + task->pid); +return -EACCES ; + } + error =aa_setprocattr_setprofile (task, args); + }else { +AA_ERROR ("Unknown setprocattr command '%.*s' " +"by task %s(%d) [user %d] for task %s(%d)" , + size <16 ? (int )size :16 , + command, + current->comm, + current->pid, + current->uid, + task->comm, + task->pid); + error = -EINVAL ; + } + +if (!error) + error = size; +return error; +} + +struct security_operations apparmor_ops = { + .ptrace = apparmor_ptrace, + .capget = apparmor_capget, + .capset_check = apparmor_capset_check, + .capset_set = apparmor_capset_set, + .sysctl = apparmor_sysctl, + .capable = apparmor_capable, + .syslog = apparmor_syslog, + + .netlink_send = apparmor_netlink_send, + .netlink_recv = apparmor_netlink_recv, + + .bprm_apply_creds = apparmor_bprm_apply_creds, + .bprm_set_security = apparmor_bprm_set_security, + .bprm_secureexec = apparmor_bprm_secureexec, + + .sb_mount = apparmor_sb_mount, + .sb_umount = apparmor_umount, + + .inode_mkdir = apparmor_inode_mkdir, + .inode_rmdir = apparmor_inode_rmdir, + .inode_create = apparmor_inode_create, + .inode_link = apparmor_inode_link, + .inode_unlink = apparmor_inode_unlink, + .inode_symlink = apparmor_inode_symlink, + .inode_mknod = apparmor_inode_mknod, + .inode_rename = apparmor_inode_rename, + .inode_permission = apparmor_inode_permission, + .inode_setattr = apparmor_inode_setattr, + .inode_setxattr = apparmor_inode_setxattr, + .inode_getxattr = apparmor_inode_getxattr, + .inode_listxattr = apparmor_inode_listxattr, + .inode_removexattr = apparmor_inode_removexattr, + .file_permission = apparmor_file_permission, + .file_alloc_security = apparmor_file_alloc_security, + .file_free_security = apparmor_file_free_security, + .file_mmap = apparmor_file_mmap, + .file_mprotect = apparmor_file_mprotect, + + .task_create = apparmor_task_create, + .task_alloc_security = apparmor_task_alloc_security, + .task_free_security = apparmor_task_free_security, + .task_post_setuid = apparmor_task_post_setuid, + .task_reparent_to_init = apparmor_task_reparent_to_init, + + .getprocattr = apparmor_getprocattr, + .setprocattr = apparmor_setprocattr, +}; + +static int __init apparmor_init (void ) +{ +int error; +const char *complainmsg =": complainmode enabled" ; + +if ((error =create_apparmorfs ())) { +AA_ERROR ("Unable to activate AppArmor filesystem\n" ); + goto createfs_out; + } + +if ((error =alloc_null_complain_profile ())){ +AA_ERROR ("Unable to allocate null complain profile\n" ); + goto alloc_out; + } + +if ((error =register_security (&apparmor_ops))) { +AA_ERROR ("Unable to load AppArmor\n" ); + goto register_security_out; + } + +AA_INFO (GFP_KERNEL ,"AppArmor initialized%s" , + apparmor_complain ? complainmsg :"" ); + +return error; + +register_security_out: +free_null_complain_profile (); + +alloc_out: +destroy_apparmorfs (); + +createfs_out: +return error; + +} + +static void __exit apparmor_exit(void) +{ +/* Remove and release all the profiles on the profile list. */ +mutex_lock (&aa_interface_lock); +write_lock (&profile_list_lock); +while (!list_empty (&profile_list)) { +struct aa_profile *profile = + list_entry(profile_list.next, struct aa_profile, list); + +/* Remove the profile from each task context it is on. */ +lock_profile (profile); + profile->isstale =1 ; +aa_unconfine_tasks (profile); +unlock_profile (profile); + +/* Release the profile itself. */ +list_del_init (&profile->list); +aa_put_profile (profile); + } +write_unlock (&profile_list_lock); + +/* FIXME: cleanup profiles references on files */ + +free_null_complain_profile (); + +/** +* Delay for an rcu cycle to make sure that all active task +* context readers have finished, and all profiles have been +* freed by their rcu callbacks. +*/ +synchronize_rcu (); + +destroy_apparmorfs (); +mutex_unlock (&aa_interface_lock); + +if (unregister_security (&apparmor_ops)) +AA_INFO (GFP_KERNEL ,"Unable to properly unregister " +"AppArmor" ); + +AA_INFO (GFP_KERNEL ,"AppArmor protection removed" ); +} + +module_init (apparmor_init); +module_exit (apparmor_exit); + +MODULE_DESCRIPTION ("AppArmor process confinement" ); +MODULE_AUTHOR ("Novell/Immunix, http://bugs.opensuse.org" ); +MODULE_LICENSE ("GPL" );
Re: [AppArmor 38/41] AppArmor: Module and LSM hooks
security/apparmor/lsm.c line 119
Quoted 7 lines
security/apparmor/lsm.c new file +7
-0+ +/** +* parent can ptrace child when +* - parent is unconfined +* - parent is in complain mode +* - parent and child are confined by the same profile +*/
Your profiles are name based. That means the same profile in a different namespace does different things. It would be a very odd case where it mattered but surely the parent ptrace child rule should also require that the parent and child are in the same namespace when using apparmor name based security.
security/apparmor/lsm.c line 142
Quoted 7 lines
security/apparmor/lsm.c new file +7
-0+static int apparmor_capget (struct task_struct *task, +kernel_cap_t *effective, +kernel_cap_t *inheritable, +kernel_cap_t *permitted) +{ +return cap_capget (task, effective, inheritable, permitted); +}
Pointless function should go away.
security/apparmor/lsm.c line 188
Quoted 9 lines
security/apparmor/lsm.c new file +9
-0+static int apparmor_sysctl (struct ctl_table *table,int op) +{ +int error =0 ; + +if ((op &002 ) && !capable (CAP_SYS_ADMIN )) + error =aa_reject_syscall (current,GFP_KERNEL , +"sysctl (write)" ); + +return error;
The usual file permission security override is DAC not ADMIN. What is the logic of this choice.
security/apparmor/lsm.c line 194
Quoted 6 lines
security/apparmor/lsm.c new file +6
-0+} + +static int apparmor_syslog (int type) +{ +return cap_syslog (type); +}
More pointless functions to delete.
Re: [AppArmor 38/41] AppArmor: Module and LSM hooks
Quoted 5 lines · Alan Cox
Quoted 5 lines
security/apparmor/lsm.c new file +5
-0+ +/** +* parent can ptrace child when +* - parent is unconfined +* - parent is in complain mode
Quoted 9 lines
security/apparmor/lsm.c line 119
Quoted 2 lines
security/apparmor/lsm.c new file +2
-0+* - parent and child are confined by the same profile +*/ Your profiles are name based. That means the same profile in a different namespace does different things. It would be a very odd case where it mattered but surely the parent ptrace child rule should also require that the parent and child are in the same namespace when using apparmor name based security.
you are right we should be requiring parent and child are in the same namespace. This has been fixed.
Quoted 10 lines
security/apparmor/lsm.c line 142
Quoted 7 lines
security/apparmor/lsm.c new file +7
-0+static int apparmor_capget (struct task_struct *task, +kernel_cap_t *effective, +kernel_cap_t *inheritable, +kernel_cap_t *permitted) +{ +return cap_capget (task, effective, inheritable, permitted); +}Pointless function should go away.
yes we had a few of those thanks for pointing it out.
Quoted 4 lines
Quoted 4 lines
security/apparmor/lsm.c new file +4
-0+static int apparmor_sysctl (struct ctl_table *table,int op) +{ +int error =0 ; +
Quoted 9 lines
security/apparmor/lsm.c line 188
Quoted 5 lines
security/apparmor/lsm.c new file +5
-0+if ((op &002 ) && !capable (CAP_SYS_ADMIN )) + error =aa_reject_syscall (current,GFP_KERNEL , +"sysctl (write)" ); + +return error;The usual file permission security override is DAC not ADMIN. What is the logic of this choice.
This was a very course grain check that was done to restrict access to sysctl's that could be potentially used to elevated priledge. The check is inconsistent with AppArmor's model and we should be modelling sysctl accesses as pathname access, and then we could be using standard mediation.
thanks for the review
john
PATCH 39/41 AppArmor: Profile loading and manipulation, pathname matching
In reply to [email protected]Pathname matching, transition table loading, profile loading and manipulation.
--- security/apparmor/match.c | 232 ++++++++++++ security/apparmor/match.h | 83 ++++ security/apparmor/module_interface.c | 641 +++++++++++++++++++++++++++++++++++ 3 files changed, 956 insertions(+)
security/apparmor/match.c new file +232 -0
@@ -0,0 +1,232 @@ +/* +* Copyright (C) 2007 Novell/SUSE +* +* This program is free software; you can redistribute it and/or +* modify it under the terms of the GNU General Public License as +* published by the Free Software Foundation, version 2 of the +* License. +* +* Regular expression transition table matching +*/ + +#include <linux/kernel.h> +#include <linux/slab.h> +#include <linux/errno.h> +#include "match.h" + +static struct table_header *unpack_table (void *blob,size_t bsize) +{ +struct table_header *table =NULL ; +struct table_header th; +size_t tsize; + +if (bsize <sizeof (struct table_header )) + goto out; + + th.td_id =ntohs (*(u16 *) (blob)); + th.td_flags =ntohs (*(u16 *) (blob +2 )); + th.td_lolen =ntohl (*(u32 *) (blob +8 )); + blob +=sizeof (struct table_header ); + +if (!(th.td_flags ==YYTD_DATA16 || th.td_flags ==YYTD_DATA32 || + th.td_flags ==YYTD_DATA8 )) + goto out; + + tsize =table_size (th.td_lolen, th.td_flags); +if (bsize < tsize) + goto out; + + table =kmalloc (tsize,GFP_KERNEL ); +if (table) { + *table = th; +if (th.td_flags ==YYTD_DATA8 ) +UNPACK_ARRAY (table->td_data, blob, th.td_lolen, + u8, ntohb); +else if (th.td_flags ==YYTD_DATA16 ) +UNPACK_ARRAY (table->td_data, blob, th.td_lolen, + u16, ntohs); +else +UNPACK_ARRAY (table->td_data, blob, th.td_lolen, + u32, ntohl); + } + +out: +return table; +} + +int unpack_dfa (struct aa_dfa *dfa,void *blob,size_t size) +{ +int hsize, i; +int error = -ENOMEM ; + +/* get dfa table set header */ +if (size <sizeof (struct table_set_header )) + goto fail; + +if (ntohl (*(u32 *)blob) !=YYTH_MAGIC ) + goto fail; + + hsize =ntohl (*(u32 *)(blob +4 )); +if (size < hsize) + goto fail; + + blob += hsize; + size -= hsize; + + error = -EPROTO ; +while (size >0 ) { +struct table_header *table; + table =unpack_table (blob, size); +if (!table) + goto fail; + +switch (table->td_id) { +case YYTD_ID_ACCEPT : +case YYTD_ID_BASE : + dfa->tables[table->td_id -1 ] = table; +if (table->td_flags !=YYTD_DATA32 ) + goto fail; +break ; +case YYTD_ID_DEF : +case YYTD_ID_NXT : +case YYTD_ID_CHK : + dfa->tables[table->td_id -1 ] = table; +if (table->td_flags !=YYTD_DATA16 ) + goto fail; +break ; +case YYTD_ID_EC : + dfa->tables[table->td_id -1 ] = table; +if (table->td_flags !=YYTD_DATA8 ) + goto fail; +break ; +default : +kfree (table); + goto fail; + } + + blob +=table_size (table->td_lolen, table->td_flags); + size -=table_size (table->td_lolen, table->td_flags); + } + +return 0 ; + +fail: +for (i =0 ; i <ARRAY_SIZE (dfa->tables); i++) { +if (dfa->tables[i]) { +kfree (dfa->tables[i]); + dfa->tables[i] =NULL ; + } + } +return error; +} + +/** +* verify_dfa - verify that all the transitions and states in the dfa tables +* are in bounds. +* @dfa: dfa to test +* +* assumes dfa has gone through the verification done by unpacking +*/ +int verify_dfa (struct aa_dfa *dfa) +{ +size_t i, state_count, trans_count; +int error = -EPROTO ; + +/* check that required tables exist */ +if (!(dfa->tables[YYTD_ID_ACCEPT -1 ] && + dfa->tables[YYTD_ID_DEF -1 ] && + dfa->tables[YYTD_ID_BASE -1 ] && + dfa->tables[YYTD_ID_NXT -1 ] && + dfa->tables[YYTD_ID_CHK -1 ])) + goto out; + +/* accept.size == default.size == base.size */ + state_count = dfa->tables[YYTD_ID_BASE -1 ]->td_lolen; +if (!(state_count == dfa->tables[YYTD_ID_DEF -1 ]->td_lolen && + state_count == dfa->tables[YYTD_ID_ACCEPT -1 ]->td_lolen)) + goto out; + +/* next.size == chk.size */ + trans_count = dfa->tables[YYTD_ID_NXT -1 ]->td_lolen; +if (trans_count != dfa->tables[YYTD_ID_CHK -1 ]->td_lolen) + goto out; + +/* if equivalence classes then its table size must be 256 */ +if (dfa->tables[YYTD_ID_EC -1 ] && + dfa->tables[YYTD_ID_EC -1 ]->td_lolen !=256 ) + goto out; + +for (i =0 ; i < state_count; i++) { +if (DEFAULT_TABLE (dfa)[i] >= state_count) + goto out; +if (BASE_TABLE (dfa)[i] >= trans_count +256 ) + goto out; + } + +for (i =0 ; i < trans_count ; i++) { +if (NEXT_TABLE (dfa)[i] >= state_count) + goto out; +if (CHECK_TABLE (dfa)[i] >= state_count) + goto out; + } + + error =0 ; +out: +return error; +} + +struct aa_dfa *aa_match_alloc (void ) +{ +return kzalloc (sizeof (struct aa_dfa ),GFP_KERNEL ); +} + +void aa_match_free (struct aa_dfa *dfa) +{ +if (dfa) { +int i; + +for (i =0 ; i <ARRAY_SIZE (dfa->tables); i++) +kfree (dfa->tables[i]); + } +kfree (dfa); +} + +/** +* aa_dfa_match - match @path against @dfa starting in @state +* @dfa: the dfa to match @path against +* @state: the state to start matching in +* @path: the path to match against the dfa +* +* aa_dfa_match will match the full path length and return the state it +* finished matching in. The final state is used to look up the accepting +* label. +*/ +unsigned int aa_dfa_match (struct aa_dfa *dfa,const char *str) +{ +u16 *def =DEFAULT_TABLE (dfa); +u32 *base =BASE_TABLE (dfa); +u16 *next =NEXT_TABLE (dfa); +u16 *check =CHECK_TABLE (dfa); +unsigned int state =1 , pos; + +/* current state is <state>, matching character *str */ +if (dfa->tables[YYTD_ID_EC -1 ]) { +u8 *equiv =EQUIV_TABLE (dfa); +while (*str) { + pos = base[state] + equiv[(u8)*str++]; +if (check[pos] == state) + state = next[pos]; +else + state = def[state]; + } + }else { +while (*str) { + pos = base[state] + (u8)*str++; +if (check[pos] == state) + state = next[pos]; +else + state = def[state]; + } + } +return ACCEPT_TABLE (dfa)[state]; +}
security/apparmor/match.h new file +83 -0
83 lines not on this page · raw message
security/apparmor/module_interface.c new file +641 -0
641 lines not on this page · raw message
Re: [AppArmor 39/41] AppArmor: Profile loading and manipulation, pathname matching
security/apparmor/match.c line 28
Quoted 3 lines
security/apparmor/match.c new file +3
-0+ th.td_id =ntohs (*(u16 *) (blob)); + th.td_flags =ntohs (*(u16 *) (blob +2 )); + th.td_lolen =ntohl (*(u32 *) (blob +8 ));
Use cpu_to and _to_cpu functions for here so it is clear the intended direction and endianness.
security/apparmor/module_interface.c line 64
Quoted 5 lines
security/apparmor/module_interface.c new file +5
-0+ +static inline int aa_inbounds (struct aa_ext *e,size_t size) +{ +return (e->pos + size <= e->end); +}
Can e->pos + size ever overflow. If so this check isn't safe.
Quoted 5 lines
+ *
aa_unpack_profile- unpack a serialized profile
+ * @e: serialized data extent information
+ * @error: error code returned if unpacking fails
+ */security/apparmor/module_interface.c new file +1
-0+static struct aa_profile *aa_unpack_profile (struct aa_ext *e)
security/apparmor/module_interface.c line 290
Quoted 10 lines
security/apparmor/module_interface.c new file +9
-0+{ +struct aa_profile *profile =NULL ; + +/* get optional subprofiles */ +if (aa_is_nameX (e,AA_LIST ,"hats" )) { +while (!aa_is_nameX (e,AA_LISTEND ,NULL )) { +struct aa_profile *subprofile; + subprofile =aa_unpack_profile (e); +if (IS_ERR (subprofile)) {
What bounds recursion here on invalid input ?
Re: [AppArmor 39/41] AppArmor: Profile loading and manipulation, pathname matching
In reply to [email protected][email protected] writes:
[didn't review code fully, just some stuff I noticed]
security/apparmor/match.h line 64
Quoted 4 lines
security/apparmor/match.h new file +4
-0+ +struct aa_dfa { +struct table_header *tables[YYTD_ID_NXT ]; +};
If that is passed in from user space you would need special compat code for 64bit kernels who support 32bit userland. Better to avoid pointers.
security/apparmor/module_interface.c line 289
Quoted 6 lines
security/apparmor/module_interface.c new file +6
-0+ +/* get optional subprofiles */ +if (aa_is_nameX (e,AA_LIST ,"hats" )) { +while (!aa_is_nameX (e,AA_LISTEND ,NULL )) { +struct aa_profile *subprofile; + subprofile =aa_unpack_profile (e);
Is there any check that would guard the recursion from stack overflow on malicious input?
security/apparmor/module_interface.c line 485
Quoted 11 lines
security/apparmor/module_interface.c new file +11
-0+/* +* Replacement needs to allocate a new aa_task_context for each +* task confined by old_profile. To do this the profile locks +* are only held when the actual switch is done per task. While +* looping to allocate a new aa_task_context the old_task list +* may get shorter if tasks exit/change their profile but will +* not get longer as new task will not use old_profile detecting +* that is stale. +*/ + do { + new_cxt = aa_alloc_task_context(GFP_KERNEL | __GFP_NOFAIL);
NOFAIL is usually a bad sign. It should be only used if there is no alternative.
-Andi
Re: [AppArmor 39/41] AppArmor: Profile loading and manipulation, pathname matching
Quoted 7 lines · Andi Kleen
security/apparmor/match.h line 64
Quoted 3 lines
security/apparmor/match.h new file +3
-0+struct aa_dfa { +struct table_header *tables[YYTD_ID_NXT ]; +};If that is passed in from user space you would need special compat code for 64bit kernels who support 32bit userland. Better to avoid pointers.
This struct is not passed between the kernel and user space; no problem here. In fact the profiles format used is fully machine independent.
Quoted 9 lines
security/apparmor/module_interface.c line 289
Quoted 6 lines
security/apparmor/module_interface.c new file +6
-0+ +/* get optional subprofiles */ +if (aa_is_nameX (e,AA_LIST ,"hats" )) { +while (!aa_is_nameX (e,AA_LISTEND ,NULL )) { +struct aa_profile *subprofile; + subprofile =aa_unpack_profile (e);Is there any check that would guard the recursion from stack overflow on malicious input?
It's nice to check for consistency though, so we're adding that. Profile loading is a trusted operation, at least so far, and so security wise we don't actually have to care --- if loading an invalid profile can bring down the system, then that's no worse than an arbitrary module that crashes the machine. Not sure if there will ever be user loadable profiles; at least at that point we had to care.
Quoted 6 lines
Quoted 6 lines
security/apparmor/module_interface.c new file +6
-0+/* +* Replacement needs to allocate a new aa_task_context for each +* task confined by old_profile. To do this the profile locks +* are only held when the actual switch is done per task. While +* looping to allocate a new aa_task_context the old_task list +* may get shorter if tasks exit/change their profile but will
Quoted 8 lines
security/apparmor/module_interface.c line 485
Quoted 5 lines
security/apparmor/module_interface.c new file +5
-0+* not get longer as new task will not use old_profile detecting +* that is stale. +*/ + do { + new_cxt = aa_alloc_task_context(GFP_KERNEL | __GFP_NOFAIL);NOFAIL is usually a bad sign. It should be only used if there is no alternative.
At this point there is no secure alternative to allocating a task context --- except killing the task, maybe.
Thanks,
Andreas
Re: [AppArmor 39/41] AppArmor: Profile loading and manipulation, pathname matching
Quoted 6 lines
It's nice to check for consistency though, so we're adding that. Profile loading is a trusted operation, at least so far, and so security wise we don't actually have to care --- if loading an invalid profile can bring down the system, then that's no worse than an arbitrary module that crashes the machine. Not sure if there will ever be user loadable profiles; at least at that point we had to care.
A security system that allows to crash the kernel is a little weird though. It would be better to check. Not that a recursion check is particularly expensive.
-Andi
Re: [AppArmor 39/41] AppArmor: Profile loading and manipulation, pathname matching
Quoted 11 lines · Andi Kleen
Quoted 6 lines
It's nice to check for consistency though, so we're adding that. Profile loading is a trusted operation, at least so far, and so security wise we don't actually have to care --- if loading an invalid profile can bring down the system, then that's no worse than an arbitrary module that crashes the machine. Not sure if there will ever be user loadable profiles; at least at that point we had to care.
A security system that allows to crash the kernel is a little weird though. It would be better to check. Not that a recursion check is particularly expensive.
Indeed. It will be fixed in the next rev.
thanks
john
Re: [AppArmor 39/41] AppArmor: Profile loading and manipulation, pathname matching
In reply to Andreas GruenbacherHi!
Quoted 10 lines
Quoted 9 lines
security/apparmor/module_interface.c line 289
Quoted 6 lines
security/apparmor/module_interface.c new file +6
-0+ +/* get optional subprofiles */ +if (aa_is_nameX (e,AA_LIST ,"hats" )) { +while (!aa_is_nameX (e,AA_LISTEND ,NULL )) { +struct aa_profile *subprofile; + subprofile =aa_unpack_profile (e);Is there any check that would guard the recursion from stack overflow on malicious input?
Quoted 6 lines
It's nice to check for consistency though, so we're adding that. Profile loading is a trusted operation, at least so far, and so security wise we don't actually have to care --- if loading an invalid profile can bring down the system, then that's no worse than an arbitrary module that crashes the machine. Not sure if there will ever be user loadable profiles; at least at that point we had to care.
It is not a _security_ problem, but face it, mount("/very weird filename") is not expected to crash the kernel, either. It is quality-of-impelmentation problem.
Signature · 2 lines
(english) http://www.livejournal.com/~pavelmachek (cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
Re: [AppArmor 39/41] AppArmor: Profile loading and manipulation, pathname matching
In reply to Andreas GruenbacherQuoted 4 lines
don't actually have to care --- if loading an invalid profile can bring down the system, then that's no worse than an arbitrary module that crashes the machine. Not sure if there will ever be user loadable profiles; at least at that point we had to care.
CAP_SYS_RAWIO is needed to do arbitary patching/loading in the capability model so if you are using lesser capabilities it is a (minor) capability rise but not a big problem, just ugly and wanting a fix
Quoted 15 lines
Quoted 6 lines
Quoted 6 lines
security/apparmor/module_interface.c new file +6
-0+/* +* Replacement needs to allocate a new aa_task_context for each +* task confined by old_profile. To do this the profile locks +* are only held when the actual switch is done per task. While +* looping to allocate a new aa_task_context the old_task list +* may get shorter if tasks exit/change their profile but will Quoted 8 lines
security/apparmor/module_interface.c line 485
Quoted 5 lines
security/apparmor/module_interface.c new file +5
-0+* not get longer as new task will not use old_profile detecting +* that is stale. +*/ + do { + new_cxt = aa_alloc_task_context(GFP_KERNEL | __GFP_NOFAIL);NOFAIL is usually a bad sign. It should be only used if there is no alternative.
Quoted 2 lines
At this point there is no secure alternative to allocating a task context --- except killing the task, maybe.
Can you count the number needed, preallocate them and then when you know for sure either succeed or fail the operation as a whole ?
Re: [AppArmor 39/41] AppArmor: Profile loading and manipulation, pathname matching
Quoted 24 lines · Alan Cox
Quoted 4 lines
don't actually have to care --- if loading an invalid profile can bring down the system, then that's no worse than an arbitrary module that crashes the machine. Not sure if there will ever be user loadable profiles; at least at that point we had to care.
CAP_SYS_RAWIOis needed to do arbitary patching/loading in the capability model so if you are using lesser capabilities it is a (minor) capability rise but not a big problem, just ugly and wanting a fixQuoted 15 lines
Quoted 14 lines
security/apparmor/module_interface.c line 485
Quoted 11 lines
security/apparmor/module_interface.c new file +11
-0+/* +* Replacement needs to allocate a new aa_task_context for each +* task confined by old_profile. To do this the profile locks +* are only held when the actual switch is done per task. While +* looping to allocate a new aa_task_context the old_task list +* may get shorter if tasks exit/change their profile but will +* not get longer as new task will not use old_profile detecting +* that is stale. +*/ + do { + new_cxt = aa_alloc_task_context(GFP_KERNEL | __GFP_NOFAIL);NOFAIL is usually a bad sign. It should be only used if there is no alternative.
Quoted 5 lines
Quoted 2 lines
At this point there is no secure alternative to allocating a task context --- except killing the task, maybe.
Can you count the number needed, preallocate them and then when you know for sure either succeed or fail the operation as a whole ?
No, to be accurate the count would have to be made with the profile lock held, which would then need to be released so as not to use GFP_ATOMIC for the allocations.
An iterative approach could be taken where we do something like repeat:
lock profile
count
if preallocated < count
unlock profile
if (! allocate count - preallocated)
Fail
goto repeat
do replacement
PATCH 40/41 AppArmor: all the rest
In reply to [email protected]All the things that didn't nicely fit in a category on their own: kbuild code, declararions and inline functions, /sys/kernel/security/apparmor filesystem for controlling apparmor from user space, profile list functions, locking documentation, /proc/$pid/task/$tid/attr/current access.
--- security/apparmor/Kconfig | 9 + security/apparmor/Makefile | 13 + security/apparmor/apparmor.h | 282 +++++++++++++++++++++++++++++++++++++++++ security/apparmor/apparmorfs.c | 248 ++++++++++++++++++++++++++++++++++++ security/apparmor/inline.h | 219 +++++++++++++++++++++++++++++++ security/apparmor/list.c | 94 +++++++++++++ security/apparmor/locking.txt | 59 ++++++++ security/apparmor/procattr.c | 143 ++++++++++++++++++++ 8 files changed, 1067 insertions(+)
security/apparmor/Kconfig new file +9 -0
@@ -0,0 +1,9 @@ +config SECURITY_APPARMOR + tristate "AppArmor support" + depends on SECURITY!=n + help + This enables the AppArmor security module. + Required userspace tools (if they are not included in your + distribution) and further information may be found at + <http://forge.novell.com/modules/xfmod/project/?apparmor> + If you are unsure how to answer this question, answer N.
security/apparmor/Makefile new file +13 -0
@@ -0,0 +1,13 @@ +# Makefile for AppArmor Linux Security Module +# +obj-$(CONFIG_SECURITY_APPARMOR) += apparmor.o + +apparmor-y := main.o list.o procattr.o lsm.o apparmorfs.o \ + module_interface.o match.o + +quiet_cmd_make-caps = GEN $@ +cmd_make-caps = sed -n -e "/CAP_FS_MASK/d" -e "s/^\#define[ \\t]\\+CAP_\\([A-Z0-9_]\\+\\)[ \\t]\\+\\([0-9]\\+\\)\$$/[\\2] = \"\\1\",/p" $< | tr A-Z a-z > $@ + +$(obj)/main.o : $(obj)/capability_names.h +$(obj)/capability_names.h : $(srctree)/include/linux/capability.h + $(call cmd,make-caps)
security/apparmor/apparmor.h new file +282 -0
282 lines not on this page · raw message
security/apparmor/apparmorfs.c new file +248 -0
248 lines not on this page · raw message
security/apparmor/inline.h new file +219 -0
219 lines not on this page · raw message
security/apparmor/list.c new file +94 -0
94 lines not on this page · raw message
security/apparmor/locking.txt new file +59 -0
59 lines not on this page · raw message
security/apparmor/procattr.c new file +143 -0
143 lines not on this page · raw message
Re: [AppArmor 40/41] AppArmor: all the rest
security/apparmor/inline.h line 175
Quoted 11 lines · On Thu, Apr 12, 2007 at 02:08:49AM -0700, [email protected] wrote:
security/apparmor/inline.h new file +11
-0+ }else if (profile1 > profile2) { +/* profile1 cannot be NULL here. */ +spin_lock_irqsave (&profile1->lock, profile1->int_flags); +if (profile2) +spin_lock (&profile2->lock); + + }else { +/* profile2 cannot be NULL here. */ +spin_lock_irqsave (&profile2->lock, profile2->int_flags); +spin_lock (&profile1->lock); + }
Ahem...
profile2 is locked individually. profile1 > profile2. profile1 is not locked. We try to lock both. profile1 is locked OK, flags (with interrupts disabled) are stored into it. We spin trying to lock profile2. Eventually, whoever had held profile2 unlocks it, restoring the flags from profile2. We happily grab the spinlock and move on. When we unlock the pair, we restore flags from profile1. I.e. we are left with interrupts disabled.
Re: [AppArmor 40/41] AppArmor: all the rest
Quoted 15 lines · Al Viro
security/apparmor/inline.h line 175
Quoted 11 lines · On Thu, Apr 12, 2007 at 02:08:49AM -0700, [email protected] wrote:
security/apparmor/inline.h new file +11
-0+ }else if (profile1 > profile2) { +/* profile1 cannot be NULL here. */ +spin_lock_irqsave (&profile1->lock, profile1->int_flags); +if (profile2) +spin_lock (&profile2->lock); + + }else { +/* profile2 cannot be NULL here. */ +spin_lock_irqsave (&profile2->lock, profile2->int_flags); +spin_lock (&profile1->lock); + }Ahem...
Quoted 6 lines
profile2 is locked individually. profile1 > profile2. profile1 is not locked. We try to lock both. profile1 is locked OK, flags (with interrupts disabled) are stored into it. We spin trying to lock profile2. Eventually, whoever had held profile2 unlocks it, restoring the flags from profile2. We happily grab the spinlock and move on. When we unlock the pair, we restore flags from profile1. I.e. we are left with interrupts disabled.
Please, ignore - shouldn't have posted without coffee... Flags would be for different CPUs in that case, obviously.
PATCH 41/41 Add AppArmor LSM to security/Makefile
In reply to [email protected]--- security/Kconfig | 1 + security/Makefile | 1 + 2 files changed, 2 insertions(+)
security/Kconfig +1 -0
@@ -94,6 +94,7 @@ config SECURITY_ROOTPLUG If you are unsure how to answer this question, answer N. source security/selinux/Kconfig +source security/apparmor/Kconfig endmenu
security/Makefile +1 -0
@@ -14,5 +14,6 @@ endif obj-$(CONFIG_SECURITY) += security.o dummy.o inode.o # Must precede capability.o in order to stack properly. obj-$(CONFIG_SECURITY_SELINUX) += selinux/built-in.o +obj-$(CONFIG_SECURITY_APPARMOR) += commoncap.o apparmor/ obj-$(CONFIG_SECURITY_CAPABILITIES) += commoncap.o capability.o obj-$(CONFIG_SECURITY_ROOTPLUG) += commoncap.o root_plug.o
Quoted 2 lines · [email protected] wrote:
This post contains patches to include the AppArmor application security framework, with request for inclusion.
question in general, these seems like a fairly invasive series of patches. back when I first started graduate school, I prototyped a relatively simple stackable file system on 2.4 that seems to be able to do a lot of what app armor does (or at least seems to be easily extensible to o everything app armor does) from an fs perspective, without having to modify the kernel at all, why not use that approach?
What I did back then was I took Erez Zadok's fist, created his base0fs, which basically does nothing, just passes everything directly down. I viewed it as a null stackable fs. I then modified it to
a) contain a set of rules (be able to add, delete, modify them)
b) fs's private lookup() determines if a dentry matches a rule and tags it (stored in dentry private data, but inode can be used as well)
c) fs's private permission() checks the rule data stored in private data to determine if the generic_permission should be even called.
what I didn't do is mmap or hard link, but those are additional checks one can do in those specific f_ops.
Basically, why force the VFS to change, when one can create an apparmorfs that stacks on top of whatever fs you want to use app armor rules with, create a namespace containing that new stacked fs only (to contain processes within the stacked fs).
This approach doesn't suffer from the normal stackable file system "issue" since there should be no complex what I'll call "cache coherency" issues due to the stackable "directory entry" namespace being the same exact as the underlying namespace and it passing down page cache operations directly to the lower fs. So one could stack on top of a file system one is using for both apparmor protected and unprotected processes.
I could imagine there might be an issue w/ how one deals with special file systems (ala proc and others). But the way we approached this in general, is that this is an aspect of containers. namely, each protected area is a container itself, and hence has its own private virtualized proc....
my code was mostly prototype level (As well as only for 2.4), but it would seem to very easily to duplicate in a production level quailty for 2.6.
anyways, just a thought
Hi!
Quoted 10 lines
AppArmor's Overall Design
=========================AppArmor protects systems from vulnerable software by confining processes, giving them "least privilege" access to the system's
resources: with least privilege, processes are allowed exactly what they need, nothing more, and nothing less. Systems are thus protected from bugs in applications that would lead to privilege escalation, such as remote system access because of a buffer overflow in a web server, etc.
Quoted 2 lines
AppArmor does this by defining application profiles which list allowed accesses, and assigning those profiles to processes. AppArmor does *not*
You can do the same with ptrace. If that's not fast enough... improve ptrace?
Quoted 3 lines
The corollary to this is that attacks against AppArmor that start with "assume some unconfined process does ..." are outside the AppArmor threat model. Any process that might do something malicious to an
IOW AppArmor is broken by design. (One reason is: operations by unconfined processes that did not use to be security sensitive before -- ln shadow random_name -- are security sensitive now.)
Pavel
Signature · 2 lines
(english) http://www.livejournal.com/~pavelmachek (cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
Quoted 2 lines · Pavel Machek
You can do the same with ptrace. If that's not fast enough... improve ptrace?
I did my Master's thesis on a system called Janus that tried using ptrace for this goal. The bottom line is that ptrace sucks for this purpose. It is a kludge. It is not the right approach. I do not know of any satisfactory way to improve ptrace for this purpose; you have to throw away ptrace and start over.
At the time I did the work, ptrace has all sorts of serious problems. Here are some of them. There was no way to follow fork securely. There was no way to deny a single system call without killing the process entirely. Performance was poor, because ptrace context-switches on every read() and write(). Handling of signals is a mess: ptrace overloads the signal mechanism to deliver its events, which in retrospect was a lousy design decision it makes the tracer complex and error-prone and makes it hard to maintain the transparency of tracing. ptrace breaks wait(), and consequently handling wait() and other signal-related system calls transparently and securely is ugly at best. Handling signals is probably feasible but a total mess, and that's the last thing you want in the security-critical part of your system. In addition, ptrace operates at the wrong level of abstraction and forces the user-level tracer to maintain a lot of shadow state that must be kept in sync with state held by the kernel. That's an opportunity for security holes. Also, ptrace has no way to force the tracee to die if the tracer unexpectedly dies, which is risky when using ptrace for security confinement. I haven't checked whether these problems are still present in the current implementation of ptrace, but I'd guess that many probably still are, because many are fundamental consequences of how ptrace works.
Before advocating ptrace for this purpose, I encourage you to study some of the relevant literature. Start with Chapter 4 of my Master's thesis.
http://www.cs.berkeley.edu/~daw/papers/janus-masters.ps
Then, read Tal Garfinkel's paper on system call interposition.
http://www.stanford.edu/~talg/papers/traps/abstract.html
Then, read about Ostia.
http://www.stanford.edu/~talg/papers/NDSS04/abstract.html
I think these may change your mind about the suitability of ptrace for this task.
Hi!
Quoted 11 lines
Quoted 2 lines
You can do the same with ptrace. If that's not fast enough... improve ptrace?
I did my Master's thesis on a system called Janus that tried using ptrace for this goal. The bottom line is that ptrace sucks for this purpose. It is a kludge. It is not the right approach. I do not know of any satisfactory way to improve ptrace for this purpose; you have to throw away ptrace and start over.
At the time I did the work, ptrace has all sorts of serious problems. Here are some of them. There was no way to follow fork securely.
Actually there is now. I did something similar called subterfugue and we solved this one.
Quoted 4 lines
There was no way to deny a single system call without killing the process entirely. Performance was poor, because ptrace context-switches on every
read()andwrite().
You can't deny a system call, but you can turn it into getpid(), then fake the return value. You probably could invent ptrace extensions, like solaris did, to avoid context switches.
Quoted 6 lines
Handling of signals is a mess: ptrace overloads the
signal mechanism to deliver its events, which in retrospect was a lousy design decision it makes the tracer complex and error-prone and makes it hard to maintain the transparency of tracing. ptrace breakswait(), and consequently handlingwait()and other signal-related system calls transparently and securely is ugly at best
We got this solved in linux, I believe.
Quoted 8 lines
. Handling signals is probably
feasible but a total mess, and that's the last thing you want in the security-critical part of your system. In addition, ptrace operates at the wrong level of abstraction and forces the user-level tracer to maintain a lot of shadow state that must be kept in sync with state held by the kernel. That's an opportunity for security holes. Also, ptrace has no way to force the tracee to die if the tracer unexpectedly dies, which is risky when using ptrace for security confinement.
I think we actually have a flag for "kill this if tracer dies"... or we could add it.
Quoted 8 lines
I haven't
checked whether these problems are still present in the current implementation of ptrace, but I'd guess that many probably still are, because many are fundamental consequences of how ptrace works.Before advocating ptrace for this purpose, I encourage you to study some of the relevant literature. Start with Chapter 4 of my Master's thesis.
I'm not advocating it. I've done this before, see subterfugue.org. It was a ugly hack, mostly because it was filename based. Unfortunately AA is also filename-based.
Pavel
Signature · 2 lines
(english) http://www.livejournal.com/~pavelmachek (cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
Quoted 5 lines · Pavel Machek
Quoted 1 line · David Wagner
There was no way to follow fork securely.
Actually there is now. I did something similar called subterfugue and we solved this one.
Yes, I saw that. I thought subterfugue was neat. The way that subterfugue was a clever hack -- albeit too clever by half, in my opinion. Dynamically re-writing the program on the fly to insert a trap after the fork() call, right? When the tracer has to do that kind of thing, I find it hard to get confidence that it will be secure. It seems all too easy to imagine ways that the tracee might be able to escape the tracing and break security. There are all sorts of corner cases to think about. What if the program is executing from shared memory? What if there are multiple threads running concurrently? What if the program is executing from a region of memory where a DMA is scheduled to asynchronously write to? Any of those cases could create a race condition (TOCTTOU) where the trap after the fork() gets removed before the program reaches that point of execution.
ptrace() seems like a fine answer for a debugger, but I think it's not such a great answer for a security tool where you have to be dead-certain there is no way to escape the sandbox. When I'm relying upon something for security, the last thing you want is to have to go through hairy kludgy kludgy contortions to make up for flaws in the interface. Complexity is the enemy of security.
I still think that ptrace() is not the best way to implement this kind of security tool, and I think it's entirely understandable that they did not use ptrace. I do not think it is a fair criticism of AppArmor to say "AppArmor should have used ptrace()".
Quoted 4 lines
Quoted 2 lines
Handling of signals is a mess: ptrace overloads the
signal mechanism to deliver its events, [...]We got this solved in linux, I believe.
Out of curiousity, how was this solved? It looked pretty fundamental to me.
Thanks for your comments...
I've posted on the subject before, and as noone seemed to truely relate to the concept I concequently dropped my effords, but as you seem to be half a step in the general right direction, this may be a good time to bring it up again.
If instead of 'least privilege' and fat profiles, you would opt for 'least authority' and (potentialy) thin profiles, this would make the whole also usable for systems initialy designed with security in mind.
To accomplish such a thing I proposed that the non 'at' family of calls for filesystems should be considdered to be priviledged operations (within a least authority context), and the 'at' family of operations should have invocations with uptree path tokens also be considdered priviledged (within a least authority context). The non uptree invocations of the 'at' family
should be considdered unpriviledged, and should thus always be allowed seperately from any profile.
With this distinction implemented, you could than potentialy use thin profiles and dynamicly exchange authority by passing along directory file handles when needed.
Where with the 'least priviledge' approach the profile would need to grant the each priviledge the application might need, but when you would be able to use both file handles, socket handles and directory handles as tokens of authorization that can be comunicated seperately and freely, without being stopped from being used by least priviledge profiles, the profiles needed will start out much thiner, and will dynamicly expand to just a limited subset of the fat least priviledge profile.
I believe that being able to be able to distinguish between the the least authority (no uptree) usage of open file/dir/socket handles and other operations is essential. If a profile blocks a program from using these open file/dir/socket handles in a way not violating least authority (that is no uptree tokens in at family calls), this would be a major design flaw.
Rob
Quoted 45 lines · On Thu, April 12, 2007 11:08, [email protected] wrote:
AppArmor's Overall Design
=========================AppArmor protects systems from vulnerable software by confining processes, giving them "least privilege" access to the system's
resources: with least privilege, processes are allowed exactly what they need, nothing more, and nothing less. Systems are thus protected from bugs in applications that would lead to privilege escalation, such as remote system access because of a buffer overflow in a web server, etc.AppArmor does this by defining application profiles which list allowed accesses, and assigning those profiles to processes. AppArmor does *not* require the user to confine all processes on the system. Rather, you need to provide AppArmor profiles for every process that is potentially subject to manipulation by the attacker. For instance, to defend against network attack, confine all process that access the network.
The corollary to this is that attacks against AppArmor that start with "assume some unconfined process does ..." are outside the AppArmor threat model. Any process that might do something malicious to an AppArmor system should be confined by an AppArmor profile.
The kernel manages many different kinds of resources. AppArmor currently controls access to two key resources among them: files, and POSIX Capabilities. (Additional protection for things like rlimits, interprocess communication, and network access are being worked on, and are expected to become available in a future version.)
File Access Control
-------------------Application profiles control file access by pathname: each profile contains a list of fully qualified pathnames (potentially containing globbing) and associated access modes: read (r), write (w), different kinds of execute (ix, Px, px, Ux, ux), create hard-link (l), and memory map for execution (m).
For example, the following two rules permit read access to any file below
/srv/www/htdocs/**.html, and memory map for execution (m), execute inheriting the current profile (ix), and read (r) access to/usr/sbin/suexec2:/srv/www/htdocs/**.html r, /usr/sbin/suexec2 mixr,