I've made a quick review of the non-networking LSM hooks, and listed below some notes about problems I saw. When a fix was obvious and didn't appear to require further discussion, I also listed a FIX. Otherwise, I mention that the problem is left UNRESOLVED. I plan on creating a patch to implement the FIXes unless I hear objections to some of them. Here are my notes: As noted by Chris Wright, bprm_ops->alloc_security can be called multiple times in one code path. Furthermore, bprm_ops->free_security might be called without ever calling bprm_ops->alloc_security. FIX: Move the bprm_ops->alloc_security call to do_execve just prior to calling prepare_binprm (so that we know that it is always called once). Replace the current bprm_ops->alloc_security call with a new bprm_ops->set_security call. There is a race condition in get_empty_super if sb_ops->alloc_security uses blocking allocation (e.g. a super_block with a null s_dev field is found, sb_ops->alloc_security is called, it goes to sleep waiting for memory, and another process comes along and takes the same super_block. FIX: Move the sb_ops->alloc_security call after the setting of s_dev in read_super. inode_ops->alloc_security has no way to propagate errors back to the caller. FIX: Move this call from clean_inode to the callers of clean_inode, i.e. get_empty_inode and get_new_inode. inode_ops->readlink takes a user space pointer (buf) and a seemingly useless bufsize parameter. FIX: Remove these parameters. inode_ops->truncate seems redundant with inode_ops->setattr. Does it need to be kept as a separate hook? UNRESOLVED. inode_ops->permission is called prior to the DAC logic. FIX: Move inode_ops->permission call after the DAC logic. inode_ops->post_lookup is not called after a successful i_op->lookup call in fs/namei.c:lookup_hash. Was that intentional or just an oversight? UNRESOLVED. It would be nice to have a inode_ops->delete hook (called by fs/inode.c:iput when an inode is released and its hard link count is zero). This is more useful than post_unlink/post_rmdir hooks would be, because this is the point where we would actually clear the inode's security label in the persistent label mapping. FIX: Add such a hook. It might be useful to have explicit hooks in critical places like fs/proc/kcore.c:open_kcore and linux/drivers/char/mem.c:open_port. However, these locations are already covered by capable() calls, and they can also be controlled using file controls, so it isn't clear if this is necessary. UNRESOLVED. The file_ops->read, file_ops->readv, file_ops->write, and file_ops->writev hooks are not called by the kernel. Instead, file_ops->permission is used for the read (and readv and pread), write (and writev and pwrite), readdir, and sendfile operations. This is sufficient for access control purposes. Should the read, readv, write and writev hooks be removed from file_ops? Or should these hooks be used in these operations (with expanded parameters) and new hooks added for pread, pwrite, sendfile and readdir. UNRESOLVED. file_ops->lock doesn't currently take the cmd parameter. This might be useful for general purpose usage. FIX: Add the cmd parameter. file_ops->fcntl and file_ops->fcntl64 take the same parameters. The only difference is that fcntl64 has some additional possible cmd values. But this doesn't seem to require separate hooks. FIX: Coalesce these two hooks into a single hook, but continue to call it from both sys_fcntl and sys_fcntl64. file_ops->ioctl and file_ops->fcntl take a generic arg parameter that can be a user space pointer, but it can also be a simple integer value. For example, a fcntl(fd, F_SETFL, arg) request passes the new descriptor flags as a simple integer in the arg parameter. SELinux uses this value to determine if the process is trying to clear the O_APPEND flag on a file, and verifies that the process is allowed to write to the file in that case (since SELinux may have only granted the process append permission when the file was originally opened). So it seems desirable to pass this parameter, but we should add a note in security.h warning module writers that if this parameter is a pointer (as opposed to a simple value), then it is a user space pointer. FIX: Add a comment to security.h. task_ops->create is not called by the kernel. There is the separate task_ops->alloc_security hook, but they have different intended purposes. task_ops->create (for us at least) was intended to control the ability to fork new processes - a very coarse-grained resource limitation. task_ops->alloc_security is (naturally) called after the task structure has already been allocated to allocate the security blob. So, we could use task_ops->alloc_security to prevent a given process from forking at all, but the process would see -ENOMEM rather than -EACCES. Perhaps that isn't a big deal. Should we remove task_ops->create or place it at the beginning of do_fork? UNRESOLVED. (Nit) The module_ops hooks still say "name_user" and "mod_user" in the function prototypes and hook functions, even though these parameters have been changed to be the kernel copies rather than user space pointers. FIX: Remove the '_user' suffix. The reboot hook takes a void *arg parameter that is a user space pointer. FIX: Remove this parameter. The remount hook no longer appears to be useful as a separate hook from the mount hook. FIX: Remove this hook. Some people might want the umount hook call moved up to sys_umount so that the complete nd is available. Anyone? UNRESOLVED. The ptrace and capable hooks belong in the task_ops structure. FIX: Move them. The acct hook belongs in the file_ops structure. FIX: Move it. The setcapability hook is not called by the kernel and it has no parameters defined. My current thinking is that we should move the entire capset and capget system calls out of the base kernel into the capability plug (so the base kernel would have sys_ni_syscall for these entries, and the capability module would insert them into the syscall table just as the SELinux module does with its new calls). Both the interface and the logic of these calls seems like it should be encapsulated by the capability plug, so that the Linux-Privs people are free to evolve them as desired. So, if we move these system calls in their entirety into the module, then we probably don't need a setcapability hook at all. UNRESOLVED. -- 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 : Fri Aug 03 2001 - 11:06:07 PDT