Here is some feedback on the hooks in the current patch. This is by no means a complete assessment, just a start. We'd be willing to assist with implementing some of these proposed changes and additions, but you'll have to let us know what areas we can safely work on without conflicting with your ongoing changes. arch/i386/kernel/ptrace.c (sys_ptrace): SELinux performs a check between the SIDs of the "tracer" and the "tracee". Which process is the tracer and which is the tracee depends on the request (PTRACE_TRACEME vs. the others). So for SELinux, it would be best if two task_struct pointers were passed to the hook, and if the hook was called from the PTRACE_TRACEME block on (current->p_pptr, current) and after the child lookup on (current, child). Additionally, SELinux makes this same check in fs/proc/base.c:MAY_PTRACE to control access to the /proc/PID/mem files and in fs/exec.c:compute_creds (along the same lines as must_not_trace_exec) to control tracing across an execve (but this is also based on the bprm, as discussed below). fs/attr.c (notify_change): Currently, you only call the setattr hook if the inode defines its own setattr operation. It seems like you should also call it in inode_change_ok to cover the default case. fs/binfmt_elf.c (load_elf_binary): You call the permission hook after the ELF image is mapped. Why isn't this already handled by the open_exec permission check and the (future) mmap checks? Is this so that you can check the content of the image? Should you use a different hook? fs/exec.c: We need a security field in the linux_binprm struct as a temporary place for the new security attributes of a process during an execve, used in the same manner as the existing uid, gid, and capability set fields. fs/exec.c (flush_old_exec): We need a hook after the call to flush_old_files on current->files and bprm to check the set of open file descriptors that are being inherited by the transformed process and close any descriptors that should not be inherited. fs/exec.c (prepare_binprm): We need a hook on the bprm to compute the new process security field, temporarily storing it in the bprm until/if compute_creds is called by the binfmt code. fs/exec.c (compute_creds): We need a hook on the bprm to check sharing of state across an execve (similar to the existing checking performed for sharing state across a set[ug]id/capability enhancement exec), change the attributes of the process, and wakeup any sleeping waiters if the attributes changed so that the wait4 permissions can be rechecked. fs/file_table.c: You currently call the file free_security hook in fput and the alloc_security hook in fs/open.c:dentry_open. Shouldn't alloc_security be called in get_empty_filp so that it is allocated for every file? Shouldn't free_security also be called in put_filp? fs/namei.c: I would have suggested a hook after each successful call to i_op->lookup to allow the security module to set the security field of the inode when it is first looked up (since useful information like its inode number and mode should then be available, which isn't available in get_new_inode after the read_inode call for many file system types). But I'm not sure how to sync that with Serge's attach_pathlabel hooks - it seems like we may be trying to provide the same functionality but on different levels (inode-based vs. pathname-based). It also seems like Serge's attach_pathlabel hooks overlap somewhat with the idea of post-create hooks, but again dealing at different levels - the vfsmount isn't available in the vfs_create/mkdir/etc routines. fs/open.c (vfs_statfs): We need a statfs hook called by this function on the sb object to check permission to the file system. fs/read_write.c (sys_lseek, sys_llseek): We need a seek hook called on the struct file by these functions to check permission to the open file descriptor state (which can be shared among processes). fs/stat.c (sys_readlink): Why pass the buf and bufsiz to the readlink hook? fs/stat.c: We need a stat or getattr hook called on the inode by cp_old_stat, cp_new_stat, and cp_new_stat64 (or on the dentry by all of the *stat* system call entrypoints if you want the dentry accessible, but we just need the inode) to control access to the attributes of individual files in a directory. fs/super.c (general): We need a security field in struct super_block and security operations for these objects to control access to file systems. fs/super.c (sys_umount): Currently, you call the umount hook in sys_umount on the name and flags. We would prefer a hook in do_umount on the sb object itself. Otherwise, we have to repeat the lookup. Also, we need a hook to close files during the unmount that are being used for persistent label mappings (analagous to the existing DQUOT_OFF and acct_auto_close calls for quota files and accounting files). Also, you are currently using the user space pointers for the name in the call to the hook. If you are going to keep name-based hooks, you probably want to move the hook call and use the kernel's kname, since that it is used for the lookup. In general, I don't think we should pass any user pointers to hooks. fs/super.c (sys_mount): Again, we would prefer a hook in do_remount on the sb object, a hook in do_mount on the mount directory's dentry (before reading the superblock of the file system to be mounted), and a hook in do_mount on the new sb object (after reading the superblock, but before adding it to the file system name space). Same issues with user space pointers here too. kernel/exit.c (sys_wait4): We need a hook on p called here to verify that the parent is authorized to receive the exit status of the child. The hook would be called just before the flag gets set to 1. kernel/fork.c (do_fork): We would like a hook here to ensure that the process is allowed to fork (for cases where a process has no legitimate reason for forking at all). kernel/module.c: User space pointers again. The hooks should be called with the same kernel copy of the data that gets used later for the actual operation. kernel/sys.c,sched.c: We need hooks for a variety of process operations that can be performed on other processes: setpriority, getpriority, setpgid, setpgid, getsid, setscheduler, getscheduler, etc. mm/filemap.c (filemap_nopage): We would like a hook to allow revalidation of file permissions when a mapping is created. The security module would have to invalidate page cache entries for files when they are relabeled or when a policy change occurs that would affect them. On the subsequent page fault, we can then recheck access here. include/linux/sched.h (capable): SELinux currently has a permission check inserted into the capable function that mirrors the capability check with a corresponding permission check to the SELinux security server (both must authorize the operation). That allowed us to leverage the work already done by the Linux-Privs people to allow our policy to control many privileged operations without manually inserting our own hooks into the same locations. How is that supposed to work in the new scheme of things? I was surprised that the current patch tries to effectively replace some of the existing capable calls with separate hooks rather than just inserting a hook into capable() that passes the requested capability. Of course, that offers greater flexibility since you can pass parameters specific to each operation, but it seems like a high burden for your initial implementation. fs/fcntl.c (do_fcntl): We need a hook on the parameters to this function to authorize the operation. Also, we need a security field in struct fown_struct to save the attributes of the owning process, just as the uid/gid is currently saved, for later use in send_sigio_to_task. fs/fcntl.c (sys_fcntl64): We need a hook here to deal with a few additional operations not handled by do_fcntl. fs/fcntl.c (send_sigio_to_task): We need a hook here on fown and p to check signal permissions between the "owner" of the file descriptor and the target process. fs/locks.c (sys_flock): We need a hook here on filp and cmd. fs/ioctl.c (sys_ioctl): We need a hook here on file and cmd. fs/readdir.c (vfs_readdir): We need a call to the permission hook here to check read access. fs/select.c (do_select, do_pollfd): We need a poll hook here on the struct file. kernel/kmod.c (exec_usermodehelper): We need a hook here to set the security field for the kernel module loader, just as the uid,gid, and capability sets are set. That lets us distinguish between what the kernel module loader can do and what the process that triggered the kernel module loader can do. kernel/signal.c (bad_signal or send_sig_info): We need a hook here based on sig, info, and t. -- Stephen D. Smalley, NAI Labs ssmalleyat_private _______________________________________________ linux-security-module mailing list linux-security-moduleat_private http://mail.wirex.com/mailman/listinfo/linux-security-module
This archive was generated by hypermail 2b30 : Wed Apr 25 2001 - 12:18:05 PDT