Skip to contentSkip to conversation

change lspp ipc auditing

PATCH Steve Grubb 3 participants 4 messages Mar 31, 2006 – Apr 1, 2006

Subsystems (3)
SELinux security module Security (LSM) Audit
lore
Share & tools
lore

Steve Grubb

author
#0 lore raw

Hi,

The patch below converts IPC auditing to collect sid's and convert to context string only if it needs to output an audit record. This patch depends on the inode audit change patch already being applied.

Signed-off-by Steve Grubb <[email protected]>
include/linux/security.h +0 -16
diff -urp linux-2.6.16.x86_64.orig/include/linux/security.h linux-2.6.16.x86_64/include/linux/security.h
@@ -869,11 +869,6 @@ 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.
  *
  * Security hooks for individual messages held in System V IPC message queues
  * @msg_msg_alloc_security:

@@ -1223,7 +1218,6 @@ 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);

@@ -1881,11 +1875,6 @@ 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);

@@ -2521,11 +2510,6 @@ 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;
include/linux/selinux.h +15 -0
diff -urp linux-2.6.16.x86_64.orig/include/linux/selinux.h linux-2.6.16.x86_64/include/linux/selinux.h
@@ -16,6 +16,7 @@
 struct selinux_audit_rule;
 struct audit_context;
 struct inode;
+struct kern_ipc_perm;
 
 #ifdef CONFIG_SECURITY_SELINUX
 

@@ -98,6 +99,15 @@ int selinux_ctxid_to_string(u32 ctxid, c
  */
 void selinux_get_inode_sid(const struct inode *inode, u32 *sid);
 
+/**
+ *     selinux_get_ipc_sid - get the ipc security context ID
+ *     @ipcp: ipc structure to get the sid from.
+ *     @sid: pointer to security context ID to be filled in.
+ *
+ *     Returns nothing
+ */
+void selinux_get_ipc_sid(const struct kern_ipc_perm *ipcp, u32 *sid);
+
 #else
 
 static inline int selinux_audit_rule_init(u32 field, u32 op,

@@ -141,6 +151,11 @@ static inline void selinux_get_inode_sid
 	*sid = 0;
 }
 
+static inline void selinux_get_ipc_sid(const struct kern_ipc_perm *ipcp, u32 *sid)
+{
+	*sid = 0;
+}
+
 #endif	/* CONFIG_SECURITY_SELINUX */
 
 #endif /* _LINUX_SELINUX_H */
kernel/auditsc.c +21 -47

129 lines not on this page · raw message

security/dummy.c +0 -6
diff -urp linux-2.6.16.x86_64.orig/security/dummy.c linux-2.6.16.x86_64/security/dummy.c
@@ -563,11 +563,6 @@ 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;

@@ -970,7 +965,6 @@ 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);
security/selinux/exports.c +11 -0
diff -urp linux-2.6.16.x86_64.orig/security/selinux/exports.c linux-2.6.16.x86_64/security/selinux/exports.c
@@ -15,6 +15,7 @@
 #include <linux/module.h>
 #include <linux/selinux.h>
 #include <linux/fs.h>
+#include <linux/ipc.h>
 
 #include "security.h"
 #include "objsec.h"

@@ -50,3 +51,13 @@ void selinux_get_inode_sid(const struct 
 	*sid = 0;
 }
 
+void selinux_get_ipc_sid(const struct kern_ipc_perm *ipcp, u32 *sid)
+{
+	if (selinux_enabled) {
+		struct ipc_security_struct *isec = ipcp->security;
+		*sid = isec->sid;
+		return;
+	}
+	*sid = 0;
+}
+
security/selinux/hooks.c +0 -8
diff -urp linux-2.6.16.x86_64.orig/security/selinux/hooks.c linux-2.6.16.x86_64/security/selinux/hooks.c
@@ -4016,13 +4016,6 @@ 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);
-}
-
 /* module stacking operations */
 static int selinux_register_security (const char *name, struct security_operations *ops)
 {

@@ -4285,7 +4278,6 @@ 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,
#1 lore raw
Quoted 3 lines · Steve Grubb

The patch below converts IPC auditing to collect sid's and convert to context string only if it needs to output an audit record. This patch depends on the inode audit change patch already being applied.

Looks pretty much like the version of this I submitted last night. It looks fine to me.

Point of clarification, though... We need to simplify for Al *exactly* what needs to be applied. There's a gang of patches flying around with IPC in the subject under multiple different threads, most of which are redundant.

As I see it there are two things that needs to happen with respect to IPC auditing...

(1) Steve's patch above (or my patch from last night) eliminates the char *ctx strings in the ipc audit records resulting in improved performance (and eliminating the memory leaks that resurrected this code a month ago)

(2) My ipc audit rework patch that splits the ipc audit functions into two separate functions, each recording something different... One audits the ipc object itself (which is what will record the SELinux context sid. And the second is called when permissions are changed on an ipc object (happens in IPC_SET operations). Steve has recommended a minor change to the naming of the audit record type from AUDIT_IPC_NEW_PERM to AUDIT_IPC_SET_PERM. That's acceptable by me. I'll repost this patch very soon.

:-Dustin

#2 lore raw
In reply to Steve Grubb
Quoted 13 lines · Steve Grubb

Hi,

The patch below converts IPC auditing to collect sid's and convert to context string only if it needs to output an audit record. This patch depends on the inode audit change patch already being applied.

Signed-off-by Steve Grubb <[email protected]>

diff -urp linux-2.6.16.x86_64.orig/kernel/auditsc.c linux-2.6.16.x86_64/kernel/auditsc.c

kernel/auditsc.c static void audit_log_exit(struct audit_ · line 743

Quoted 5 lines
kernel/auditsc.c +1 -1
@@ -734,16 +740,16 @@ static void audit_log_exit(struct audit_
 				context->names[i].osid, &ctx, &len)) { 
 				audit_log_format(ab, " obj=%u",
 						context->names[i].osid);
-				call_panic = 1;
+				call_panic = 2;

Why set it to 2? If you want a count of panic-related events, you likely want call_panic++; in each case, but you don't seem to use it anyway beyond being a simple boolean flag.

BTW, I personally have no strong opinion on whether to call audit_panic in this case. It does yield uglier code, and I'm sure that the kernel developers won't be happy to see additional code paths that can ultimately lead to a panic(), so if you think it unnecessary, feel free to drop.

Otherwise, the patch looks sane to me.

Signature · 2 lines
Stephen Smalley
National Security Agency
author
#3 lore raw
Quoted 1 line · Stephen Smalley

Why set it to 2?

I sometimes like those things so that I can printk them during debug to see which one is doing it. If they were both a "1" there's no way to distinguish which one tripped it.

Quoted 2 lines

BTW, I personally have no strong opinion on whether to call audit_panic in this case.

My feeling is that calling audit_panic does no good. In the case of sendfile, the data has already left the box and panic helps nothing. What we need to do is figure out how to close the loop manually just in case this ever happens. Maybe this should be added to the agenda for Monday's lspp telecon?

Thanks,
-Steve