[PATCH] LSPP audit enablement: storing selinux ocontext and scontext
Chris Wright
chrisw at osdl.org
Fri Oct 7 21:43:11 UTC 2005
* Dustin Kirkland (dustin.kirkland at us.ibm.com) wrote:
> I'm addressing Amy's concerns and attaching an updated patch with the
> editions discussed inline.
(Note: as mentioned on IRC, patch inline as well please).
> diff -uprN linux-2.6.13-rc6-mm2/include/linux/security.h linux-2.6.13-rc6-mm2-context_labels/include/linux/security.h
> --- linux-2.6.13-rc6-mm2/include/linux/security.h 2005-10-06 18:26:11.000000000 -0500
> +++ linux-2.6.13-rc6-mm2-context_labels/include/linux/security.h 2005-10-05 12:02:01.000000000 -0500
> @@ -792,6 +792,11 @@ struct swap_info_struct;
> * @ipcp contains the kernel IPC permission structure
> * @flag contains the desired (requested) permission set
> * Return 0 if permission is granted.
> + * @ipc_getsecurity:
> + * Copy the security label associated with the ipc object into
> + * @buffer. @buffer may be NULL to request the size of the buffer
> + * required. @size indicates the size of @buffer in bytes. Return
> + * number of bytes used/required on success.
Boy i don't like these kind of interfaces...it's racy, etc..
> * Security hooks for individual messages held in System V IPC message queues
> * @msg_msg_alloc_security:
> @@ -1140,6 +1145,7 @@ struct security_operations {
> void (*task_to_inode)(struct task_struct *p, struct inode *inode);
>
> int (*ipc_permission) (struct kern_ipc_perm * ipcp, short flag);
> + int (*ipc_getsecurity)(struct kern_ipc_perm *ipcp, void *buffer, size_t size);
>
> int (*msg_msg_alloc_security) (struct msg_msg * msg);
> void (*msg_msg_free_security) (struct msg_msg * msg);
> @@ -1775,6 +1781,11 @@ static inline int security_ipc_permissio
> return security_ops->ipc_permission (ipcp, flag);
> }
>
> +static inline int security_ipc_getsecurity(struct kern_ipc_perm *ipcp, void *buffer, size_t size)
> +{
> + return security_ops->ipc_getsecurity(ipcp, buffer, size);
> +}
> +
> static inline int security_msg_msg_alloc (struct msg_msg * msg)
> {
> return security_ops->msg_msg_alloc_security (msg);
> @@ -2405,6 +2416,11 @@ static inline int security_ipc_permissio
> return 0;
> }
>
> +static inline int security_ipc_getsecurity(struct kern_ipc_perm *ipcp, void *buffer, size_t size)
> +{
> + return -EOPNOTSUPP;
> +}
> +
> static inline int security_msg_msg_alloc (struct msg_msg * msg)
> {
> return 0;
> diff -uprN linux-2.6.13-rc6-mm2/ipc/msg.c linux-2.6.13-rc6-mm2-context_labels/ipc/msg.c
> --- linux-2.6.13-rc6-mm2/ipc/msg.c 2005-10-06 18:26:11.000000000 -0500
> +++ linux-2.6.13-rc6-mm2-context_labels/ipc/msg.c 2005-10-05 21:14:39.000000000 -0500
> @@ -443,10 +443,17 @@ asmlinkage long sys_msgctl (int msqid, i
> if (msq == NULL)
> goto out_up;
>
> + ipcp = &msq->q_perm;
> +
> + if (cmd == IPC_SET) {
> + if ((err = audit_ipc_security_context(ipcp)))
> + goto out_unlock_up;
> + }
> +
OK, some issues here. First, the checkid is to ensure it's valid object
still, so this shouldn't be above that. Second, I assume it's some
requirement to log attempted updates? Because adding a third test for
IPC_SET is not a good idea...too many special case logics. Third, this
thing is asking for some real trouble by doing kmalloc with spinlock
held. That's a no go.
> err = -EIDRM;
> if (msg_checkid(msq,msqid))
> goto out_unlock_up;
> - ipcp = &msq->q_perm;
> +
> err = -EPERM;
> if (current->euid != ipcp->cuid &&
> current->euid != ipcp->uid && !capable(CAP_SYS_ADMIN))
> diff -uprN linux-2.6.13-rc6-mm2/ipc/sem.c linux-2.6.13-rc6-mm2-context_labels/ipc/sem.c
> --- linux-2.6.13-rc6-mm2/ipc/sem.c 2005-10-06 18:26:11.000000000 -0500
> +++ linux-2.6.13-rc6-mm2-context_labels/ipc/sem.c 2005-10-06 11:01:38.000000000 -0500
> @@ -813,12 +813,18 @@ static int semctl_down(int semid, int se
> if(sma==NULL)
> return -EINVAL;
>
> + ipcp = &sma->sem_perm;
> +
> + if(cmd == IPC_SET) {
> + if ((err = audit_ipc_security_context(ipcp)))
> + goto out_unlock;
> + }
> +
Ditto
> if (sem_checkid(sma,semid)) {
> err=-EIDRM;
> goto out_unlock;
> }
> - ipcp = &sma->sem_perm;
> -
> +
> if (current->euid != ipcp->cuid &&
> current->euid != ipcp->uid && !capable(CAP_SYS_ADMIN)) {
> err=-EPERM;
> diff -uprN linux-2.6.13-rc6-mm2/ipc/shm.c linux-2.6.13-rc6-mm2-context_labels/ipc/shm.c
> --- linux-2.6.13-rc6-mm2/ipc/shm.c 2005-10-06 18:26:11.000000000 -0500
> +++ linux-2.6.13-rc6-mm2-context_labels/ipc/shm.c 2005-10-06 11:02:50.000000000 -0500
> @@ -611,6 +611,9 @@ asmlinkage long sys_shmctl (int shmid, i
> err=-EINVAL;
> if(shp==NULL)
> goto out_up;
> + err = audit_ipc_security_context(&(shp->shm_perm));
> + if(err)
> + goto out_unlock_up;
Ditto
> err = shm_checkid(shp,shmid);
> if(err)
> goto out_unlock_up;
> diff -uprN linux-2.6.13-rc6-mm2/ipc/util.c linux-2.6.13-rc6-mm2-context_labels/ipc/util.c
> --- linux-2.6.13-rc6-mm2/ipc/util.c 2005-10-06 18:26:11.000000000 -0500
> +++ linux-2.6.13-rc6-mm2-context_labels/ipc/util.c 2005-10-05 20:56:44.000000000 -0500
> @@ -26,6 +26,7 @@
> #include <linux/workqueue.h>
> #include <linux/seq_file.h>
> #include <linux/proc_fs.h>
> +#include <linux/audit.h>
>
> #include <asm/unistd.h>
>
> @@ -466,6 +467,7 @@ int ipcperms (struct kern_ipc_perm *ipcp
> { /* flag will most probably be 0 or S_...UGO from <linux/stat.h> */
> int requested_mode, granted_mode;
>
> + audit_ipc_security_context(ipcp);
Here object is valid, but lock is still held, so no sleeping (aka
kmalloc(GFP_KERNEL). Alos, this has some 'sprinkling' effect, but I
probably missed all the discussion looking for proper hooking locations.
Is that information written down somehwere to justify the locations?
> requested_mode = (flag >> 6) | (flag >> 3) | flag;
> granted_mode = ipcp->mode;
> if (current->euid == ipcp->cuid || current->euid == ipcp->uid)
> diff -uprN linux-2.6.13-rc6-mm2/kernel/audit.c linux-2.6.13-rc6-mm2-context_labels/kernel/audit.c
> --- linux-2.6.13-rc6-mm2/kernel/audit.c 2005-10-06 18:26:11.000000000 -0500
> +++ linux-2.6.13-rc6-mm2-context_labels/kernel/audit.c 2005-10-05 12:05:31.000000000 -0500
> @@ -142,7 +142,7 @@ static void audit_set_pid(struct audit_b
> nlh->nlmsg_pid = pid;
> }
>
> -static void audit_panic(const char *message)
> +void audit_panic(const char *message)
> {
> switch (audit_failure)
> {
> diff -uprN linux-2.6.13-rc6-mm2/kernel/auditsc.c linux-2.6.13-rc6-mm2-context_labels/kernel/auditsc.c
> --- linux-2.6.13-rc6-mm2/kernel/auditsc.c 2005-10-06 18:26:11.000000000 -0500
> +++ linux-2.6.13-rc6-mm2-context_labels/kernel/auditsc.c 2005-10-06 17:56:21.000000000 -0500
> @@ -43,6 +43,7 @@
> #include <linux/netlink.h>
> #include <linux/compiler.h>
> #include <asm/unistd.h>
> +#include <linux/security.h>
>
> /* 0 = no checking
> 1 = put_count checking
> @@ -99,6 +100,7 @@ struct audit_names {
> gid_t gid;
> dev_t rdev;
> unsigned flags;
> + char *security_context;
> };
>
> struct audit_aux_data {
> @@ -108,6 +110,12 @@ struct audit_aux_data {
>
> #define AUDIT_AUX_IPCPERM 0
>
> +struct audit_aux_data_security_context {
> + struct audit_aux_data d;
> + char *security_context;
> + size_t security_context_len;
> +};
> +
> struct audit_aux_data_ipcctl {
> struct audit_aux_data d;
> struct ipc_perm p;
> @@ -117,6 +125,8 @@ struct audit_aux_data_ipcctl {
> mode_t mode;
> };
>
> +static const char *audit_macxattr;
> +
> struct audit_aux_data_socketcall {
> struct audit_aux_data d;
> int nargs;
> @@ -657,10 +667,12 @@ static inline void audit_free_names(stru
> context->serial, context->major, context->in_syscall,
> context->name_count, context->put_count,
> context->ino_count);
> - for (i = 0; i < context->name_count; i++)
> + for (i = 0; i < context->name_count; i++) {
> printk(KERN_ERR "names[%d] = %p = %s\n", i,
> context->names[i].name,
> context->names[i].name);
> + kfree(context->names[i].security_context);
> + }
> dump_stack();
> return;
> }
> @@ -692,6 +704,14 @@ static inline void audit_free_aux(struct
> dput(axi->dentry);
> mntput(axi->mnt);
> }
> + if (
> + (aux->type == AUDIT_SUBJECT_CONTEXT) ||
> + (aux->type == AUDIT_OBJECT_CONTEXT)
> + ) {
> + struct audit_aux_data_security_context *axi = (void *)aux;
> + kfree(axi->security_context);
> + }
> +
> context->aux = aux->next;
> kfree(aux);
> }
> @@ -771,6 +791,37 @@ static inline void audit_free_context(st
> printk(KERN_ERR "audit: freed %d contexts\n", count);
> }
>
> +static void audit_log_task_security_context(struct audit_buffer *ab)
> +{
> + char *security_context;
> + ssize_t len = 0;
> +
> + len = security_getprocattr(current, "current", NULL, 0);
> + if (len < 0) {
> + if (len != -EINVAL)
> + audit_panic("security_getprocattr error in audit_log_task_security_context");
> + return;
> + }
> +
> + security_context = kmalloc(len, GFP_KERNEL);
> + if (!security_context) {
> + audit_panic("memory allocation error in audit_log_task_security_context");
> + return;
> + }
> +
> + len = security_getprocattr(current, "current", security_context, len);
> + if (len < 0 ) {
> + audit_panic("security_getprocattr error in audit_log_task_security_context");
> + goto out;
> + }
> +
> + audit_log_format(ab, " subj=%s", security_context);
> +
> +out:
> + kfree(security_context);
> + return;
> +}
> +
> static void audit_log_task_info(struct audit_buffer *ab)
> {
> char name[sizeof(current->comm)];
> @@ -797,6 +848,7 @@ static void audit_log_task_info(struct a
> vma = vma->vm_next;
> }
> up_read(&mm->mmap_sem);
> + audit_log_task_security_context(ab);
> }
>
> static void audit_log_exit(struct audit_context *context, unsigned int gfp_mask)
> @@ -869,7 +921,12 @@ static void audit_log_exit(struct audit_
> audit_log_d_path(ab, "path=", axi->dentry, axi->mnt);
> break; }
>
> + case AUDIT_OBJECT_CONTEXT: {
> + struct audit_aux_data_security_context *axi = (void *)aux;
> + audit_log_format(ab, " obj=%s", axi->security_context);
> + break; }
> }
> +
> audit_log_end(ab);
> }
>
> @@ -903,6 +960,11 @@ static void audit_log_exit(struct audit_
> context->names[i].gid,
> MAJOR(context->names[i].rdev),
> MINOR(context->names[i].rdev));
> + if (context->names[i].security_context) {
> + audit_log_format(ab, " obj=%s",
> + context->names[i].security_context);
> + }
> +
> audit_log_end(ab);
> }
> }
> @@ -1118,6 +1180,37 @@ void audit_putname(const char *name)
> #endif
> }
>
> +void audit_inode_security_context(int idx, const struct inode *inode)
> +{
> + struct audit_context *context = current->audit_context;
> + int len = 0;
> +
> + if (!audit_macxattr)
> + return;
> +
> + len = security_inode_getsecurity((struct inode *)inode, audit_macxattr, NULL, 0);
> + if (len < 0) {
> + if (len != -EOPNOTSUPP)
> + audit_panic("security_inode_getsecurity error in audit_inode_security_context");
> + return;
> + }
> +
> + context->names[idx].security_context = kmalloc(len, GFP_KERNEL);
Stack var is cheap, and might be easier to read ...
> + if (!(context->names[idx].security_context)) {
> + audit_panic("memory allocation error in audit_inode_security_context");
> + return;
> + }
> +
> + len = security_inode_getsecurity((struct inode *)inode, audit_macxattr,
Why the cast? Undermines the const interface.
> + context->names[idx].security_context, len);
> + if (len < 0) {
> + kfree(context->names[idx].security_context);
> + audit_panic("security_inode_getsecurity error in audit_inode_security_context");
> + }
> +
> + return;
> +}
-ETOOMANY audit_panics
> /* Store the inode and device from a lookup. Called from
> * fs/namei.c:path_lookup(). */
> void audit_inode(const char *name, const struct inode *inode, unsigned flags)
> @@ -1153,6 +1246,7 @@ void audit_inode(const char *name, const
> context->names[idx].uid = inode->i_uid;
> context->names[idx].gid = inode->i_gid;
> context->names[idx].rdev = inode->i_rdev;
> + audit_inode_security_context(idx, inode);
> }
>
> void auditsc_get_stamp(struct audit_context *ctx,
> @@ -1166,6 +1260,67 @@ void auditsc_get_stamp(struct audit_cont
> ctx->auditable = 1;
> }
>
> +int audit_ipc_security_context(struct kern_ipc_perm *ipcp)
> +{
> + struct audit_aux_data_security_context *ax;
> + struct audit_context *context = current->audit_context;
> + int len = 0;
> +
> + if (likely(!context))
> + return 0;
> +
> + ax = kmalloc(sizeof(*ax), GFP_KERNEL);
> + if (!ax) {
> + audit_panic("memory allocation error in audit_ipc_security_context");
> + return -ENOMEM;
> + }
> +
> + len = security_ipc_getsecurity(ipcp, NULL, 0);
> + if (len < 0) {
> + if (len != -EOPNOTSUPP)
> + audit_panic("security_ipc_getsecurity error in audit_ipc_security_context");
> + return len;
Leaks ax.
> + }
> +
> + ax->security_context = kmalloc(len, GFP_KERNEL);
> + if (!ax->security_context) {
> + audit_panic("memory allocation error in audit_ipc_security_context");
> + kfree(ax);
> + return -ENOMEM;
> + }
> +
> + len = security_ipc_getsecurity(ipcp, ax->security_context, len);
> + if (len < 0) {
> + audit_panic("security_ipc_getsecurity error in audit_ipc_security_context");
> + kfree(ax->security_context);
> + kfree(ax);
> + return len;
> + }
> +
> + ax->security_context_len = len;
> +
> + ax->d.type = AUDIT_OBJECT_CONTEXT;
> + ax->d.next = context->aux;
> + context->aux = (void *)ax;
> + return 0;
> +}
-ETOOMANY audit_panics.
You could avoid some overhead by doing it this way (flip allocations
upside down...psuedo code, probably buggy):
ret = 0
len = security_ipc_getsecurity(NULL)
if len <= 0
goto out;
ret = -ENOMEM
context = kmalloc(len)
if (!context)
goto out;
ret = security_ipc_getsecurity(context)
if ret != len { /* I still don't like this double call */
ret = -EFOOBAR
goto out_free;
}
ret = -ENOMEM;
ax = kmalloc(sizeof(struct aud_ctxt)
if (!ax)
goto out_free;
ax->security_context = context;
ax->security_context_len = len;
ax->...
context->aux = ax;
ret = 0;
goto out;
out_free;
kfree(context)
out:
return ret;
> +
> +/* Set the security XATTR name. This is needed for audit functions
> + * to obtain the security context of file system objects. */
> +int audit_set_macxattr(const char *name)
> +{
> + size_t len = strlen(name)+1;
> +
> + if (audit_macxattr)
> + return -EINVAL;
> +
> + audit_macxattr = kstrdup(name, GFP_KERNEL);
> + if (!audit_macxattr)
> + return -ENOMEM;
> +
> + return 0;
> +}
Do this in the security interface.
> int audit_set_loginuid(struct task_struct *task, uid_t loginuid)
> {
> if (task->audit_context) {
> @@ -1262,7 +1417,7 @@ int audit_avc_path(struct dentry *dentry
> if (likely(!context))
> return 0;
>
> - ax = kmalloc(sizeof(*ax), GFP_ATOMIC);
> + ax = kmalloc(sizeof(*ax), GFP_KERNEL);
No, for two reasons. One, unreleted change shouldn't be in this. Two,
it's wrong because we currently have no such restriction on avc_audit.
> if (!ax)
> return -ENOMEM;
>
> diff -uprN linux-2.6.13-rc6-mm2/security/dummy.c linux-2.6.13-rc6-mm2-context_labels/security/dummy.c
> --- linux-2.6.13-rc6-mm2/security/dummy.c 2005-10-06 18:26:12.000000000 -0500
> +++ linux-2.6.13-rc6-mm2-context_labels/security/dummy.c 2005-10-06 11:26:21.000000000 -0500
> @@ -557,6 +557,11 @@ static int dummy_ipc_permission (struct
> return 0;
> }
>
> +static int dummy_ipc_getsecurity(struct kern_ipc_perm *ipcp, void *buffer, size_t size)
> +{
> + return -EOPNOTSUPP;
> +}
> +
> static int dummy_msg_msg_alloc_security (struct msg_msg *msg)
> {
> return 0;
> @@ -907,6 +912,7 @@ void security_fixup_ops (struct security
> set_to_dummy_if_null(ops, task_reparent_to_init);
> set_to_dummy_if_null(ops, task_to_inode);
> set_to_dummy_if_null(ops, ipc_permission);
> + set_to_dummy_if_null(ops, ipc_getsecurity);
> set_to_dummy_if_null(ops, msg_msg_alloc_security);
> set_to_dummy_if_null(ops, msg_msg_free_security);
> set_to_dummy_if_null(ops, msg_queue_alloc_security);
> diff -uprN linux-2.6.13-rc6-mm2/security/selinux/hooks.c linux-2.6.13-rc6-mm2-context_labels/security/selinux/hooks.c
> --- linux-2.6.13-rc6-mm2/security/selinux/hooks.c 2005-10-06 18:26:12.000000000 -0500
> +++ linux-2.6.13-rc6-mm2-context_labels/security/selinux/hooks.c 2005-10-06 17:27:51.000000000 -0500
> @@ -116,6 +116,35 @@ static struct security_operations *secon
> static LIST_HEAD(superblock_security_head);
> static DEFINE_SPINLOCK(sb_security_lock);
>
> +/* Return security context for a given sid or just the context
> + length if the buffer is null or length is 0 */
> +static int selinux_getsecurity(u32 sid, void *buffer, size_t size)
> +{
> + char *context;
> + unsigned len;
> + int rc;
> +
> + if (buffer && !size)
> + return -ERANGE;
That's an interface change
> + rc = security_sid_to_context(sid, &context, &len);
> + if (rc)
> + return rc;
> +
> + if (!buffer)
> + goto getsecurity_exit;
> +
> + if (size < len) {
> + len = -ERANGE;
> + goto getsecurity_exit;
> + }
> + memcpy(buffer, context, len);
> +
> +getsecurity_exit:
> + kfree(context);
> + return len;
> +}
> +
> /* Allocate and free functions for each kind of security blob. */
>
> static int task_alloc_security(struct task_struct *task)
> @@ -2234,30 +2263,13 @@ static int selinux_inode_removexattr (st
> static int selinux_inode_getsecurity(struct inode *inode, const char *name, void *buffer, size_t size)
> {
> struct inode_security_struct *isec = inode->i_security;
> - char *context;
> - unsigned len;
> - int rc;
>
> /* Permission check handled by selinux_inode_getxattr hook.*/
>
> if (strcmp(name, XATTR_SELINUX_SUFFIX))
> return -EOPNOTSUPP;
>
> - rc = security_sid_to_context(isec->sid, &context, &len);
> - if (rc)
> - return rc;
> -
> - if (!buffer || !size) {
> - kfree(context);
> - return len;
> - }
> - if (size < len) {
> - kfree(context);
> - return -ERANGE;
> - }
> - memcpy(buffer, context, len);
> - kfree(context);
> - return len;
> + return(selinux_getsecurity(isec->sid, buffer, size));
Parens not needed
> }
>
> static int selinux_inode_setsecurity(struct inode *inode, const char *name,
> @@ -4027,6 +4039,13 @@ static int selinux_ipc_permission(struct
> return ipc_has_perm(ipcp, av);
> }
>
> +static int selinux_ipc_getsecurity(struct kern_ipc_perm *ipcp, void *buffer, size_t size)
> +{
> + struct ipc_security_struct *isec = ipcp->security;
> +
> + return(selinux_getsecurity(isec->sid, buffer, size));
same here.
> +}
> +
> /* module stacking operations */
> static int selinux_register_security (const char *name, struct security_operations *ops)
> {
> @@ -4068,8 +4087,7 @@ static int selinux_getprocattr(struct ta
> char *name, void *value, size_t size)
> {
> struct task_security_struct *tsec;
> - u32 sid, len;
> - char *context;
> + u32 sid;
> int error;
>
> if (current != p) {
> @@ -4078,9 +4096,6 @@ static int selinux_getprocattr(struct ta
> return error;
> }
>
> - if (!size)
> - return -ERANGE;
> -
> tsec = p->security;
>
> if (!strcmp(name, "current"))
> @@ -4097,16 +4112,7 @@ static int selinux_getprocattr(struct ta
> if (!sid)
> return 0;
>
> - error = security_sid_to_context(sid, &context, &len);
> - if (error)
> - return error;
> - if (len > size) {
> - kfree(context);
> - return -ERANGE;
> - }
> - memcpy(value, context, len);
> - kfree(context);
> - return len;
> + return(selinux_getsecurity(sid, value, size));
Ditto
> }
>
> static int selinux_setprocattr(struct task_struct *p,
> @@ -4301,6 +4307,7 @@ static struct security_operations selinu
> .task_to_inode = selinux_task_to_inode,
>
> .ipc_permission = selinux_ipc_permission,
> + .ipc_getsecurity = selinux_ipc_getsecurity,
>
> .msg_msg_alloc_security = selinux_msg_msg_alloc_security,
> .msg_msg_free_security = selinux_msg_msg_free_security,
> @@ -4381,6 +4388,10 @@ static __init int selinux_init(void)
> if (register_security (&selinux_ops))
> panic("SELinux: Unable to register with kernel.\n");
>
> + if (audit_set_macxattr(XATTR_SELINUX_SUFFIX)) {
> + printk(KERN_WARNING "SELinux: Unable to register xattr name with audit.\n");
> + }
> +
> if (selinux_enforcing) {
> printk(KERN_INFO "SELinux: Starting in enforcing mode\n");
> } else {
More information about the Linux-audit
mailing list