audit: implement container identifier

---
 include/linux/audit.h | 49 +++++++++++++++++++++++------------
 include/linux/sched.h |  7 +----
 init/init_task.c      |  3 +--
 init/main.c           |  2 ++
 kernel/audit.c        | 71 +++++++++++++++++++++++++++++++++++++++++++++++++--
 kernel/audit.h        |  5 ++++
 kernel/auditsc.c      | 26 ++++++++++---------
 kernel/fork.c         |  1 -
 8 files changed, 124 insertions(+), 40 deletions(-)

include/linux/audit.h +32 ~~-17~~

index 97d0925454df..4fbda55f3cf2 100644
@@ -95,6 +95,16 @@ struct audit_ntp_data {
 struct audit_ntp_data {};
 #endif
 
+struct audit_task_info {
+	kuid_t			loginuid;
+	unsigned int		sessionid;
+#ifdef CONFIG_AUDITSYSCALL
+	struct audit_context	*ctx;
+#endif
+};
+
+extern struct audit_task_info init_struct_audit;
+
 extern int is_audit_feature_set(int which);
 
 extern int __init audit_register_class(int class, unsigned *list);

@@ -131,6 +141,9 @@ struct audit_ntp_data {
 #ifdef CONFIG_AUDIT
 /* These are defined in audit.c */
 				/* Public API */
+extern int  audit_alloc(struct task_struct *task);
+extern void audit_free(struct task_struct *task);
+extern void __init audit_task_init(void);
 extern __printf(4, 5)
 void audit_log(struct audit_context *ctx, gfp_t gfp_mask, int type,
 	       const char *fmt, ...);

@@ -173,12 +186,16 @@ extern void		    audit_log_key(struct audit_buffer *ab,
 
 static inline kuid_t audit_get_loginuid(struct task_struct *tsk)
 {
-	return tsk->loginuid;
+	if (!tsk->audit)
+		return INVALID_UID;
+	return tsk->audit->loginuid;
 }
 
 static inline unsigned int audit_get_sessionid(struct task_struct *tsk)
 {
-	return tsk->sessionid;
+	if (!tsk->audit)
+		return AUDIT_SID_UNSET;
+	return tsk->audit->sessionid;
 }
 
 extern u32 audit_enabled;

@@ -186,6 +203,14 @@ static inline unsigned int audit_get_sessionid(struct task_struct *tsk)
 extern int audit_signal_info(int sig, struct task_struct *t);
 
 #else /* CONFIG_AUDIT */
+static inline int audit_alloc(struct task_struct *task)
+{
+	return 0;
+}
+static inline void audit_free(struct task_struct *task)
+{ }
+static inline void __init audit_task_init(void)
+{ }
 static inline __printf(4, 5)
 void audit_log(struct audit_context *ctx, gfp_t gfp_mask, int type,
 	       const char *fmt, ...)

@@ -257,8 +282,6 @@ static inline int audit_signal_info(int sig, struct task_struct *t)
 
 /* These are defined in auditsc.c */
 				/* Public API */
-extern int  audit_alloc(struct task_struct *task);
-extern void __audit_free(struct task_struct *task);
 extern void __audit_syscall_entry(int major, unsigned long a0, unsigned long a1,
 				  unsigned long a2, unsigned long a3);
 extern void __audit_syscall_exit(int ret_success, long ret_value);

@@ -281,12 +304,14 @@ extern void audit_seccomp_actions_logged(const char *names,
 
 static inline void audit_set_context(struct task_struct *task, struct audit_context *ctx)
 {
-	task->audit_context = ctx;
+	task->audit->ctx = ctx;
 }
 
 static inline struct audit_context *audit_context(void)
 {
-	return current->audit_context;
+	if (!current->audit)
+		return NULL;
+	return current->audit->ctx;
 }
 
 static inline bool audit_dummy_context(void)

@@ -294,11 +319,7 @@ static inline bool audit_dummy_context(void)
 	void *p = audit_context();
 	return !p || *(int *)p;
 }
-static inline void audit_free(struct task_struct *task)
-{
-	if (unlikely(task->audit_context))
-		__audit_free(task);
-}
+
 static inline void audit_syscall_entry(int major, unsigned long a0,
 				       unsigned long a1, unsigned long a2,
 				       unsigned long a3)

@@ -523,12 +544,6 @@ static inline void audit_ntp_log(const struct audit_ntp_data *ad)
 extern int audit_n_rules;
 extern int audit_signals;
 #else /* CONFIG_AUDITSYSCALL */
-static inline int audit_alloc(struct task_struct *task)
-{
-	return 0;
-}
-static inline void audit_free(struct task_struct *task)
-{ }
 static inline void audit_syscall_entry(int major, unsigned long a0,
 				       unsigned long a1, unsigned long a2,
 				       unsigned long a3)

include/linux/sched.h +1 -6

index 8dc1811487f5..a936d162513a 100644
@@ -31,7 +31,6 @@
 #include <linux/rseq.h>
 
 /* task_struct member predeclarations (sorted alphabetically): */
-struct audit_context;
 struct backing_dev_info;
 struct bio_list;
 struct blk_plug;

@@ -940,11 +939,7 @@ struct task_struct {
 	struct callback_head		*task_works;
 
 #ifdef CONFIG_AUDIT
-#ifdef CONFIG_AUDITSYSCALL
-	struct audit_context		*audit_context;
-#endif
-	kuid_t				loginuid;
-	unsigned int			sessionid;
+	struct audit_task_info		*audit;
 #endif
 	struct seccomp			seccomp;

init/init_task.c +1 -2

index 7ab773b9b3cd..6496bbe5c56e 100644
@@ -124,8 +124,7 @@ struct task_struct init_task
 	.thread_group	= LIST_HEAD_INIT(init_task.thread_group),
 	.thread_node	= LIST_HEAD_INIT(init_signals.thread_head),
 #ifdef CONFIG_AUDIT
-	.loginuid	= INVALID_UID,
-	.sessionid	= AUDIT_SID_UNSET,
+	.audit		= &init_struct_audit,
 #endif
 #ifdef CONFIG_PERF_EVENTS
 	.perf_event_mutex = __MUTEX_INITIALIZER(init_task.perf_event_mutex),

init/main.c +2 -0

index 96f8d5af52d6..dbcaa49bbaea 100644
@@ -93,6 +93,7 @@
 #include <linux/rodata_test.h>
 #include <linux/jump_label.h>
 #include <linux/mem_encrypt.h>
+#include <linux/audit.h>
 
 #include <asm/io.h>
 #include <asm/bugs.h>

@@ -771,6 +772,7 @@ asmlinkage __visible void __init start_kernel(void)
 	nsfs_init();
 	cpuset_init();
 	cgroup_init();
+	audit_task_init();
 	taskstats_init_early();
 	delayacct_init();

kernel/audit.c +69 -2

83 lines not on this page · raw message

kernel/audit.h +5 -0

index 6fb7160412d4..7f623ef216e6 100644
@@ -251,6 +251,8 @@ extern void audit_log_d_path_exe(struct audit_buffer *ab,
 extern unsigned int audit_serial(void);
 extern int auditsc_get_stamp(struct audit_context *ctx,
 			      struct timespec64 *t, unsigned int *serial);
+extern int audit_alloc_syscall(struct task_struct *tsk);
+extern void audit_free_syscall(struct task_struct *tsk);
 
 extern void audit_put_watch(struct audit_watch *watch);
 extern void audit_get_watch(struct audit_watch *watch);

@@ -292,6 +294,9 @@ extern void audit_filter_inodes(struct task_struct *tsk,
 extern struct list_head *audit_killed_trees(void);
 #else /* CONFIG_AUDITSYSCALL */
 #define auditsc_get_stamp(c, t, s) 0
+#define audit_alloc_syscall(t) 0
+#define audit_free_syscall(t) {}
+
 #define audit_put_watch(w) {}
 #define audit_get_watch(w) {}
 #define audit_to_watch(k, p, l, o) (-EINVAL)

kernel/auditsc.c +14 ~~-12~~

index 4effe01ebbe2..10679da36bb6 100644
@@ -903,23 +903,25 @@ static inline struct audit_context *audit_alloc_context(enum audit_state state)
 	return context;
 }
 
-/**
- * audit_alloc - allocate an audit context block for a task
+/*
+ * audit_alloc_syscall - allocate an audit context block for a task
  * @tsk: task
  *
  * Filter on the task information and allocate a per-task audit context
  * if necessary.  Doing so turns on system call auditing for the
- * specified task.  This is called from copy_process, so no lock is
- * needed.
+ * specified task.  This is called from copy_process via audit_alloc, so
+ * no lock is needed.
  */
-int audit_alloc(struct task_struct *tsk)
+int audit_alloc_syscall(struct task_struct *tsk)
 {
 	struct audit_context *context;
 	enum audit_state     state;
 	char *key = NULL;
 
-	if (likely(!audit_ever_enabled))
+	if (likely(!audit_ever_enabled)) {
+		audit_set_context(tsk, NULL);
 		return 0; /* Return if not auditing. */
+	}
 
 	state = audit_filter_task(tsk, &key);
 	if (state == AUDIT_DISABLED) {

@@ -929,7 +931,7 @@ int audit_alloc(struct task_struct *tsk)
 
 	if (!(context = audit_alloc_context(state))) {
 		kfree(key);
-		audit_log_lost("out of memory in audit_alloc");
+		audit_log_lost("out of memory in audit_alloc_syscall");
 		return -ENOMEM;
 	}
 	context->filterkey = key;

@@ -1574,14 +1576,15 @@ static void audit_log_exit(void)
 }
 
 /**
- * __audit_free - free a per-task audit context
+ * audit_free_syscall - free per-task audit context info
  * @tsk: task whose audit context block to free
  *
- * Called from copy_process and do_exit
+ * Called from audit_free
  */
-void __audit_free(struct task_struct *tsk)
+void audit_free_syscall(struct task_struct *tsk)
 {
-	struct audit_context *context = tsk->audit_context;
+	struct audit_task_info *info = tsk->audit;
+	struct audit_context *context = info->ctx;
 
 	if (!context)
 		return;

@@ -1604,7 +1607,6 @@ void __audit_free(struct task_struct *tsk)
 		if (context->current_state == AUDIT_RECORD_CONTEXT)
 			audit_log_exit();
 	}
-
 	audit_set_context(tsk, NULL);
 	audit_free_context(context);
 }

kernel/fork.c +0 -1

index d8ae0f1b4148..ef9c123e8ae8 100644
@@ -1938,7 +1938,6 @@ static __latent_entropy struct task_struct *copy_process(
 	posix_cpu_timers_init(p);
 
 	p->io_context = NULL;
-	audit_set_context(p, NULL);
 	cgroup_fork(p);
 #ifdef CONFIG_NUMA
 	p->mempolicy = mpol_dup(p->mempolicy);

Signature · 1 line

1.8.3.1

Richard Guy Briggs

author

2019-09-19 01:22 UTC #2 lore raw

PATCH v7 2/21 audit: add container id

Implement the proc fs write to set the audit container identifier of a process, emitting an AUDIT_CONTAINER_OP record to document the event.

This is a write from the container orchestrator task to a proc entry of the form /proc/PID/audit_containerid where PID is the process ID of the newly created task that is to become the first task in a container, or an additional task added to a container.

The write expects up to a u64 value (unset: 18446744073709551615).

The writer must have capability CAP_AUDIT_CONTROL.

This will produce a record such as this:
type=CONTAINER_OP msg=audit(2018-06-06 12:39:29.636:26949) : op=set opid=2209 contid=123456 old-contid=18446744073709551615 pid=628 auid=root uid=root tty=ttyS0 ses=1 subj=unconfined_u:unconfined_r:unconfined_t:s0-s0:c0.c1023 comm=bash exe=/usr/bin/bash res=yes

The "op" field indicates an initial set. The "pid" to "ses" fields are the orchestrator while the "opid" field is the object's PID, the process being "contained". New and old audit container identifier values are given in the "contid" fields, while res indicates its success.

It is not permitted to unset the audit container identifier. A child inherits its parent's audit container identifier.

Please see the github audit kernel issue for the main feature:
https://github.com/linux-audit/audit-kernel/issues/90
Please see the github audit userspace issue for supporting additions:
https://github.com/linux-audit/audit-userspace/issues/51
Please see the github audit testsuiite issue for the test case:
https://github.com/linux-audit/audit-testsuite/issues/64
Please see the github audit wiki for the feature overview:
https://github.com/linux-audit/audit-kernel/wiki/RFE-Audit-Container-ID

Signed-off-by Richard Guy Briggs <[email protected]>

Acked-by Steve Grubb <[email protected]>

Signed-off-by Richard Guy Briggs <[email protected]>

---
 fs/proc/base.c             | 36 +++++++++++++++++++++++
 include/linux/audit.h      | 25 ++++++++++++++++
 include/uapi/linux/audit.h |  2 ++
 kernel/audit.c             | 73 ++++++++++++++++++++++++++++++++++++++++++++++
 kernel/audit.h             |  1 +
 kernel/auditsc.c           |  4 +++
 6 files changed, 141 insertions(+)

fs/proc/base.c +36 -0

index ebea9501afb8..e2e7c9f4702f 100644
@@ -1307,6 +1307,40 @@ static ssize_t proc_sessionid_read(struct file * file, char __user * buf,
 	.read		= proc_sessionid_read,
 	.llseek		= generic_file_llseek,
 };
+
+static ssize_t proc_contid_write(struct file *file, const char __user *buf,
+				   size_t count, loff_t *ppos)
+{
+	struct inode *inode = file_inode(file);
+	u64 contid;
+	int rv;
+	struct task_struct *task = get_proc_task(inode);
+
+	if (!task)
+		return -ESRCH;
+	if (*ppos != 0) {
+		/* No partial writes. */
+		put_task_struct(task);
+		return -EINVAL;
+	}
+
+	rv = kstrtou64_from_user(buf, count, 10, &contid);
+	if (rv < 0) {
+		put_task_struct(task);
+		return rv;
+	}
+
+	rv = audit_set_contid(task, contid);
+	put_task_struct(task);
+	if (rv < 0)
+		return rv;
+	return count;
+}
+
+static const struct file_operations proc_contid_operations = {
+	.write		= proc_contid_write,
+	.llseek		= generic_file_llseek,
+};
 #endif
 
 #ifdef CONFIG_FAULT_INJECTION

@@ -3067,6 +3101,7 @@ static int proc_stack_depth(struct seq_file *m, struct pid_namespace *ns,
 #ifdef CONFIG_AUDIT
 	REG("loginuid",   S_IWUSR|S_IRUGO, proc_loginuid_operations),
 	REG("sessionid",  S_IRUGO, proc_sessionid_operations),
+	REG("audit_containerid", S_IWUSR, proc_contid_operations),
 #endif
 #ifdef CONFIG_FAULT_INJECTION
 	REG("make-it-fail", S_IRUGO|S_IWUSR, proc_fault_inject_operations),

@@ -3467,6 +3502,7 @@ static int proc_tid_comm_permission(struct inode *inode, int mask)
 #ifdef CONFIG_AUDIT
 	REG("loginuid",  S_IWUSR|S_IRUGO, proc_loginuid_operations),
 	REG("sessionid",  S_IRUGO, proc_sessionid_operations),
+	REG("audit_containerid", S_IWUSR, proc_contid_operations),
 #endif
 #ifdef CONFIG_FAULT_INJECTION
 	REG("make-it-fail", S_IRUGO|S_IWUSR, proc_fault_inject_operations),

include/linux/audit.h +25 -0

index 4fbda55f3cf2..f2e3b81f2942 100644
@@ -98,6 +98,7 @@ struct audit_ntp_data {
 struct audit_task_info {
 	kuid_t			loginuid;
 	unsigned int		sessionid;
+	u64			contid;
 #ifdef CONFIG_AUDITSYSCALL
 	struct audit_context	*ctx;
 #endif

@@ -198,6 +199,15 @@ static inline unsigned int audit_get_sessionid(struct task_struct *tsk)
 	return tsk->audit->sessionid;
 }
 
+extern int audit_set_contid(struct task_struct *tsk, u64 contid);
+
+static inline u64 audit_get_contid(struct task_struct *tsk)
+{
+	if (!tsk->audit)
+		return AUDIT_CID_UNSET;
+	return tsk->audit->contid;
+}
+
 extern u32 audit_enabled;
 
 extern int audit_signal_info(int sig, struct task_struct *t);

@@ -262,6 +272,11 @@ static inline unsigned int audit_get_sessionid(struct task_struct *tsk)
 	return AUDIT_SID_UNSET;
 }
 
+static inline u64 audit_get_contid(struct task_struct *tsk)
+{
+	return AUDIT_CID_UNSET;
+}
+
 #define audit_enabled AUDIT_OFF
 
 static inline int audit_signal_info(int sig, struct task_struct *t)

@@ -676,6 +691,16 @@ static inline bool audit_loginuid_set(struct task_struct *tsk)
 	return uid_valid(audit_get_loginuid(tsk));
 }
 
+static inline bool audit_contid_valid(u64 contid)
+{
+	return contid != AUDIT_CID_UNSET;
+}
+
+static inline bool audit_contid_set(struct task_struct *tsk)
+{
+	return audit_contid_valid(audit_get_contid(tsk));
+}
+
 static inline void audit_log_string(struct audit_buffer *ab, const char *buf)
 {
 	audit_log_n_string(ab, buf, strlen(buf));

include/uapi/linux/audit.h +2 -0

index c89c6495983d..5d0ea2a6783e 100644
@@ -71,6 +71,7 @@
 #define AUDIT_TTY_SET		1017	/* Set TTY auditing status */
 #define AUDIT_SET_FEATURE	1018	/* Turn an audit feature on or off */
 #define AUDIT_GET_FEATURE	1019	/* Get which features are enabled */
+#define AUDIT_CONTAINER_OP	1020	/* Define the container id and info */
 
 #define AUDIT_FIRST_USER_MSG	1100	/* Userspace messages mostly uninteresting to kernel */
 #define AUDIT_USER_AVC		1107	/* We filter this differently */

@@ -488,6 +489,7 @@ struct audit_tty_status {
 
 #define AUDIT_UID_UNSET (unsigned int)-1
 #define AUDIT_SID_UNSET ((unsigned int)-1)
+#define AUDIT_CID_UNSET ((u64)-1)
 
 /* audit_rule_data supports filter rules with both integer and string
  * fields.  It corresponds with AUDIT_ADD_RULE, AUDIT_DEL_RULE and

kernel/audit.c +73 -0

91 lines not on this page · raw message

kernel/audit.h +1 -0

index 7f623ef216e6..16bd03b88e0d 100644
@@ -135,6 +135,7 @@ struct audit_context {
 	kuid_t		    target_uid;
 	unsigned int	    target_sessionid;
 	u32		    target_sid;
+	u64		    target_cid;
 	char		    target_comm[TASK_COMM_LEN];
 
 	struct audit_tree_refs *trees, *first_trees;

kernel/auditsc.c +4 -0

index 10679da36bb6..0e2d50533959 100644
@@ -113,6 +113,7 @@ struct audit_aux_data_pids {
 	kuid_t			target_uid[AUDIT_AUX_PIDS];
 	unsigned int		target_sessionid[AUDIT_AUX_PIDS];
 	u32			target_sid[AUDIT_AUX_PIDS];
+	u64			target_cid[AUDIT_AUX_PIDS];
 	char 			target_comm[AUDIT_AUX_PIDS][TASK_COMM_LEN];
 	int			pid_count;
 };

@@ -2375,6 +2376,7 @@ void __audit_ptrace(struct task_struct *t)
 	context->target_uid = task_uid(t);
 	context->target_sessionid = audit_get_sessionid(t);
 	security_task_getsecid(t, &context->target_sid);
+	context->target_cid = audit_get_contid(t);
 	memcpy(context->target_comm, t->comm, TASK_COMM_LEN);
 }

@@ -2402,6 +2404,7 @@ int audit_signal_info_syscall(struct task_struct *t)
 		ctx->target_uid = t_uid;
 		ctx->target_sessionid = audit_get_sessionid(t);
 		security_task_getsecid(t, &ctx->target_sid);
+		ctx->target_cid = audit_get_contid(t);
 		memcpy(ctx->target_comm, t->comm, TASK_COMM_LEN);
 		return 0;
 	}

@@ -2423,6 +2426,7 @@ int audit_signal_info_syscall(struct task_struct *t)
 	axp->target_uid[axp->pid_count] = t_uid;
 	axp->target_sessionid[axp->pid_count] = audit_get_sessionid(t);
 	security_task_getsecid(t, &axp->target_sid[axp->pid_count]);
+	axp->target_cid[axp->pid_count] = audit_get_contid(t);
 	memcpy(axp->target_comm[axp->pid_count], t->comm, TASK_COMM_LEN);
 	axp->pid_count++;

Signature · 1 line

1.8.3.1

Richard Guy Briggs

author

2019-09-19 01:22 UTC #3 lore raw

PATCH v7 3/21 audit: read container ID of a process

Add support for reading the audit container identifier from the proc filesystem.

This is a read from the proc entry of the form /proc/PID/audit_containerid where PID is the process ID of the task whose audit container identifier is sought.

The read expects up to a u64 value (unset: 18446744073709551615).

This read requires CAP_AUDIT_CONTROL.

Signed-off-by Richard Guy Briggs <[email protected]>

---
 fs/proc/base.c | 25 ++++++++++++++++++++++---
 1 file changed, 22 insertions(+), 3 deletions(-)

fs/proc/base.c +22 -3

index e2e7c9f4702f..26091800180c 100644
@@ -1224,7 +1224,7 @@ static ssize_t oom_score_adj_write(struct file *file, const char __user *buf,
 };
 
 #ifdef CONFIG_AUDIT
-#define TMPBUFLEN 11
+#define TMPBUFLEN 21
 static ssize_t proc_loginuid_read(struct file * file, char __user * buf,
 				  size_t count, loff_t *ppos)
 {

@@ -1308,6 +1308,24 @@ static ssize_t proc_sessionid_read(struct file * file, char __user * buf,
 	.llseek		= generic_file_llseek,
 };
 
+static ssize_t proc_contid_read(struct file *file, char __user *buf,
+				  size_t count, loff_t *ppos)
+{
+	struct inode *inode = file_inode(file);
+	struct task_struct *task = get_proc_task(inode);
+	ssize_t length;
+	char tmpbuf[TMPBUFLEN];
+
+	if (!task)
+		return -ESRCH;
+	/* if we don't have caps, reject */
+	if (!capable(CAP_AUDIT_CONTROL))
+		return -EPERM;
+	length = scnprintf(tmpbuf, TMPBUFLEN, "%llu", audit_get_contid(task));
+	put_task_struct(task);
+	return simple_read_from_buffer(buf, count, ppos, tmpbuf, length);
+}
+
 static ssize_t proc_contid_write(struct file *file, const char __user *buf,
 				   size_t count, loff_t *ppos)
 {

@@ -1338,6 +1356,7 @@ static ssize_t proc_contid_write(struct file *file, const char __user *buf,
 }
 
 static const struct file_operations proc_contid_operations = {
+	.read		= proc_contid_read,
 	.write		= proc_contid_write,
 	.llseek		= generic_file_llseek,
 };

@@ -3101,7 +3120,7 @@ static int proc_stack_depth(struct seq_file *m, struct pid_namespace *ns,
 #ifdef CONFIG_AUDIT
 	REG("loginuid",   S_IWUSR|S_IRUGO, proc_loginuid_operations),
 	REG("sessionid",  S_IRUGO, proc_sessionid_operations),
-	REG("audit_containerid", S_IWUSR, proc_contid_operations),
+	REG("audit_containerid", S_IWUSR|S_IRUSR, proc_contid_operations),
 #endif
 #ifdef CONFIG_FAULT_INJECTION
 	REG("make-it-fail", S_IRUGO|S_IWUSR, proc_fault_inject_operations),

@@ -3502,7 +3521,7 @@ static int proc_tid_comm_permission(struct inode *inode, int mask)
 #ifdef CONFIG_AUDIT
 	REG("loginuid",  S_IWUSR|S_IRUGO, proc_loginuid_operations),
 	REG("sessionid",  S_IRUGO, proc_sessionid_operations),
-	REG("audit_containerid", S_IWUSR, proc_contid_operations),
+	REG("audit_containerid", S_IWUSR|S_IRUSR, proc_contid_operations),
 #endif
 #ifdef CONFIG_FAULT_INJECTION
 	REG("make-it-fail", S_IRUGO|S_IWUSR, proc_fault_inject_operations),

Signature · 1 line

1.8.3.1

Richard Guy Briggs

author

2019-09-19 01:22 UTC #4 lore raw

PATCH v7 4/21 audit: convert to contid list to check for orch/engine ownership

Store the audit container identifier in a refcounted kernel object that is added to the master list of audit container identifiers. This will allow multiple container orchestrators/engines to work on the same machine without danger of inadvertantly re-using an existing identifier. It will also allow an orchestrator to inject a process into an existing container by checking if the original container owner is the one injecting the task. A hash table list is used to optimize searches.

Signed-off-by Richard Guy Briggs <[email protected]>

---
 include/linux/audit.h | 26 ++++++++++++++--
 kernel/audit.c        | 86 ++++++++++++++++++++++++++++++++++++++++++++++++---
 kernel/audit.h        |  8 +++++
 3 files changed, 112 insertions(+), 8 deletions(-)

include/linux/audit.h +23 -3

index f2e3b81f2942..e317807cdd3e 100644
@@ -95,10 +95,18 @@ struct audit_ntp_data {
 struct audit_ntp_data {};
 #endif
 
+struct audit_cont {
+	struct list_head	list;
+	u64			id;
+	struct task_struct	*owner;
+	refcount_t              refcount;
+	struct rcu_head         rcu;
+};
+
 struct audit_task_info {
 	kuid_t			loginuid;
 	unsigned int		sessionid;
-	u64			contid;
+	struct audit_cont	*cont;
 #ifdef CONFIG_AUDITSYSCALL
 	struct audit_context	*ctx;
 #endif

@@ -203,11 +211,15 @@ static inline unsigned int audit_get_sessionid(struct task_struct *tsk)
 
 static inline u64 audit_get_contid(struct task_struct *tsk)
 {
-	if (!tsk->audit)
+	if (!tsk->audit || !tsk->audit->cont)
 		return AUDIT_CID_UNSET;
-	return tsk->audit->contid;
+	return tsk->audit->cont->id;
 }
 
+extern struct audit_cont *audit_cont(struct task_struct *tsk);
+
+extern void audit_cont_put(struct audit_cont *cont);
+
 extern u32 audit_enabled;
 
 extern int audit_signal_info(int sig, struct task_struct *t);

@@ -277,6 +289,14 @@ static inline u64 audit_get_contid(struct task_struct *tsk)
 	return AUDIT_CID_UNSET;
 }
 
+static inline struct audit_cont *audit_cont(struct task_struct *tsk)
+{
+	return NULL;
+}
+
+static inline void audit_cont_put(struct audit_cont *cont)
+{ }
+
 #define audit_enabled AUDIT_OFF
 
 static inline int audit_signal_info(int sig, struct task_struct *t)

kernel/audit.c +81 -5

142 lines not on this page · raw message

kernel/audit.h +8 -0

index 16bd03b88e0d..e4a31aa92dfe 100644
@@ -211,6 +211,14 @@ static inline int audit_hash_ino(u32 ino)
 	return (ino & (AUDIT_INODE_BUCKETS-1));
 }
 
+#define AUDIT_CONTID_BUCKETS	32
+extern struct list_head audit_contid_hash[AUDIT_CONTID_BUCKETS];
+
+static inline int audit_hash_contid(u64 contid)
+{
+	return (contid & (AUDIT_CONTID_BUCKETS-1));
+}
+
 /* Indicates that audit should log the full pathname. */
 #define AUDIT_NAME_FULL -1

Signature · 1 line

1.8.3.1

Richard Guy Briggs

author

2019-09-19 01:22 UTC #11 lore raw

PATCH v7 5/21 audit: log drop of contid on exit of last task

Since we are tracking the life of each audit container indentifier, we can match the creation event with the destruction event. Log the destruction of the audit container identifier when the last process in that container exits.

Signed-off-by Richard Guy Briggs <[email protected]>

---
 kernel/audit.c   | 32 ++++++++++++++++++++++++++++++++
 kernel/audit.h   |  2 ++
 kernel/auditsc.c |  2 ++
 3 files changed, 36 insertions(+)

kernel/audit.c +32 -0

index ea0899130cc1..53d13d638c63 100644
@@ -2503,6 +2503,38 @@ int audit_set_contid(struct task_struct *task, u64 contid)
 	return rc;
 }
 
+void audit_log_container_drop(void)
+{
+	struct audit_buffer *ab;
+	uid_t uid;
+	struct tty_struct *tty;
+	char comm[sizeof(current->comm)];
+
+	if (!current->audit || !current->audit->cont ||
+	    refcount_read(&current->audit->cont->refcount) > 1)
+		return;
+	ab = audit_log_start(audit_context(), GFP_KERNEL, AUDIT_CONTAINER_OP);
+	if (!ab)
+		return;
+
+	uid = from_kuid(&init_user_ns, task_uid(current));
+	tty = audit_get_tty();
+	audit_log_format(ab,
+			 "op=drop opid=%d contid=%llu old-contid=%llu pid=%d uid=%u auid=%u tty=%s ses=%u",
+			 task_tgid_nr(current), audit_get_contid(current),
+			 audit_get_contid(current), task_tgid_nr(current), uid,
+			 from_kuid(&init_user_ns, audit_get_loginuid(current)),
+			 tty ? tty_name(tty) : "(none)",
+       			 audit_get_sessionid(current));
+	audit_put_tty(tty);
+	audit_log_task_context(ab);
+	audit_log_format(ab, " comm=");
+	audit_log_untrustedstring(ab, get_task_comm(comm, current));
+	audit_log_d_path_exe(ab, current->mm);
+	audit_log_format(ab, " res=1");
+	audit_log_end(ab);
+}
+
 /**
  * audit_log_end - end one audit record
  * @ab: the audit_buffer

kernel/audit.h +2 -0

index e4a31aa92dfe..162de8366b32 100644
@@ -255,6 +255,8 @@ extern void audit_log_d_path_exe(struct audit_buffer *ab,
 extern struct tty_struct *audit_get_tty(void);
 extern void audit_put_tty(struct tty_struct *tty);
 
+extern void audit_log_container_drop(void);
+
 /* audit watch/mark/tree functions */
 #ifdef CONFIG_AUDITSYSCALL
 extern unsigned int audit_serial(void);

kernel/auditsc.c +2 -0

index 0e2d50533959..bd855794ad26 100644
@@ -1568,6 +1568,8 @@ static void audit_log_exit(void)
 
 	audit_log_proctitle();
 
+	audit_log_container_drop();
+
 	/* Send end of event record to help user space know we are finished */
 	ab = audit_log_start(context, GFP_KERNEL, AUDIT_EOE);
 	if (ab)

Signature · 1 line

1.8.3.1

Richard Guy Briggs

author

2019-09-19 01:22 UTC #14 lore raw

PATCH v7 6/21 audit: contid limit of 32k imposed to avoid DoS

Set an arbitrary limit on the number of audit container identifiers to limit abuse.

Signed-off-by Richard Guy Briggs <[email protected]>

---
 kernel/audit.c | 8 ++++++++
 kernel/audit.h | 4 ++++
 2 files changed, 12 insertions(+)

kernel/audit.c +8 -0

index 53d13d638c63..329916534dd2 100644
@@ -139,6 +139,7 @@ struct audit_net {
 struct list_head audit_inode_hash[AUDIT_INODE_BUCKETS];
 /* Hash for contid-based rules */
 struct list_head audit_contid_hash[AUDIT_CONTID_BUCKETS];
+int audit_contid_count = 0;
 
 static struct kmem_cache *audit_buffer_cache;

@@ -2384,6 +2385,7 @@ void audit_cont_put(struct audit_cont *cont)
 		put_task_struct(cont->owner);
 		list_del_rcu(&cont->list);
 		kfree_rcu(cont, rcu);
+		audit_contid_count--;
 	}
 }

@@ -2456,6 +2458,11 @@ int audit_set_contid(struct task_struct *task, u64 contid)
 					goto conterror;
 				}
 			}
+		/* Set max contids */
+		if (audit_contid_count > AUDIT_CONTID_COUNT) {
+			rc = -ENOSPC;
+			goto conterror;
+		}
 		if (!newcont) {
 			newcont = kmalloc(sizeof(struct audit_cont), GFP_ATOMIC);
 			if (newcont) {

@@ -2465,6 +2472,7 @@ int audit_set_contid(struct task_struct *task, u64 contid)
 				newcont->owner = current;
 				refcount_set(&newcont->refcount, 1);
 				list_add_rcu(&newcont->list, &audit_contid_hash[h]);
+				audit_contid_count++;
 			} else {
 				rc = -ENOMEM;
 				goto conterror;

kernel/audit.h +4 -0

index 162de8366b32..543f1334ba47 100644
@@ -219,6 +219,10 @@ static inline int audit_hash_contid(u64 contid)
 	return (contid & (AUDIT_CONTID_BUCKETS-1));
 }
 
+extern int audit_contid_count;
+
+#define AUDIT_CONTID_COUNT	1 << 16
+
 /* Indicates that audit should log the full pathname. */
 #define AUDIT_NAME_FULL -1

Signature · 1 line

1.8.3.1

Richard Guy Briggs

author

2019-09-19 01:22 UTC #23 lore raw

PATCH v7 7/21 audit: log container info of syscalls

Create a new audit record AUDIT_CONTAINER_ID to document the audit container identifier of a process if it is present.

Called from audit_log_exit(), syscalls are covered.

A sample raw event:
type=SYSCALL msg=audit(1519924845.499:257): arch=c000003e syscall=257 success=yes exit=3 a0=ffffff9c a1=56374e1cef30 a2=241 a3=1b6 items=2 ppid=606 pid=635 auid=0 uid=0 gid=0 euid=0 suid=0 fsuid=0 egid=0 sgid=0 fsgid=0 tty=pts0 ses=3 comm="bash" exe="/usr/bin/bash" subj=unconfined_u:unconfined_r:unconfined_t:s0-s0:c0.c1023 key="tmpcontainerid" type=CWD msg=audit(1519924845.499:257): cwd="/root"
type=PATH msg=audit(1519924845.499:257): item=0 name="/tmp/" inode=13863 dev=00:27 mode=041777 ouid=0 ogid=0 rdev=00:00 obj=system_u:object_r:tmp_t:s0 nametype= PARENT cap_fp=0 cap_fi=0 cap_fe=0 cap_fver=0
type=PATH msg=audit(1519924845.499:257): item=1 name="/tmp/tmpcontainerid" inode=17729 dev=00:27 mode=0100644 ouid=0 ogid=0 rdev=00:00 obj=unconfined_u:object_r:user_tmp_t:s0 nametype=CREATE cap_fp=0 cap_fi=0 cap_fe=0 cap_fver=0
type=PROCTITLE msg=audit(1519924845.499:257): proctitle=62617368002D6300736C65657020313B206563686F2074657374203E202F746D702F746D70636F6E7461696E65726964
type=CONTAINER_ID msg=audit(1519924845.499:257): contid=123458

Signed-off-by Richard Guy Briggs <[email protected]>

Acked-by Steve Grubb <[email protected]>

---
 include/linux/audit.h      |  5 +++++
 include/uapi/linux/audit.h |  1 +
 kernel/audit.c             | 20 ++++++++++++++++++++
 kernel/auditsc.c           | 20 ++++++++++++++------
 4 files changed, 40 insertions(+), 6 deletions(-)

include/linux/audit.h +5 -0

index e317807cdd3e..0c18d8e30620 100644
@@ -220,6 +220,8 @@ static inline u64 audit_get_contid(struct task_struct *tsk)
 
 extern void audit_cont_put(struct audit_cont *cont);
 
+extern void audit_log_container_id(struct audit_context *context, u64 contid);
+
 extern u32 audit_enabled;
 
 extern int audit_signal_info(int sig, struct task_struct *t);

@@ -297,6 +299,9 @@ static inline struct audit_cont *audit_cont(struct task_struct *tsk)
 static inline void audit_cont_put(struct audit_cont *cont)
 { }
 
+static inline void audit_log_container_id(struct audit_context *context, u64 contid)
+{ }
+
 #define audit_enabled AUDIT_OFF
 
 static inline int audit_signal_info(int sig, struct task_struct *t)

include/uapi/linux/audit.h +1 -0

index 5d0ea2a6783e..4ed080f28b47 100644
@@ -117,6 +117,7 @@
 #define AUDIT_FANOTIFY		1331	/* Fanotify access decision */
 #define AUDIT_TIME_INJOFFSET	1332	/* Timekeeping offset injected */
 #define AUDIT_TIME_ADJNTPVAL	1333	/* NTP value adjustment */
+#define AUDIT_CONTAINER_ID	1334	/* Container ID */
 
 #define AUDIT_AVC		1400	/* SE Linux avc denial or grant */
 #define AUDIT_SELINUX_ERR	1401	/* Internal SE Linux Errors */

kernel/audit.c +20 -0

index 329916534dd2..adfb3e6a7f0c 100644
@@ -2127,6 +2127,26 @@ void audit_log_session_info(struct audit_buffer *ab)
 	audit_log_format(ab, "auid=%u ses=%u", auid, sessionid);
 }
 
+/*
+ * audit_log_container_id - report container info
+ * @context: task or local context for record
+ * @contid: container ID to report
+ */
+void audit_log_container_id(struct audit_context *context, u64 contid)
+{
+	struct audit_buffer *ab;
+
+	if (!audit_contid_valid(contid))
+		return;
+	/* Generate AUDIT_CONTAINER_ID record with container ID */
+	ab = audit_log_start(context, GFP_KERNEL, AUDIT_CONTAINER_ID);
+	if (!ab)
+		return;
+	audit_log_format(ab, "contid=%llu", contid);
+	audit_log_end(ab);
+}
+EXPORT_SYMBOL(audit_log_container_id);
+
 void audit_log_key(struct audit_buffer *ab, char *key)
 {
 	audit_log_format(ab, " key=");

kernel/auditsc.c +14 -6

index bd855794ad26..ac438fcff807 100644
@@ -1534,7 +1534,7 @@ static void audit_log_exit(void)
 	for (aux = context->aux_pids; aux; aux = aux->next) {
 		struct audit_aux_data_pids *axs = (void *)aux;
 
-		for (i = 0; i < axs->pid_count; i++)
+		for (i = 0; i < axs->pid_count; i++) {
 			if (audit_log_pid_context(context, axs->target_pid[i],
 						  axs->target_auid[i],
 						  axs->target_uid[i],

@@ -1542,14 +1542,20 @@ static void audit_log_exit(void)
 						  axs->target_sid[i],
 						  axs->target_comm[i]))
 				call_panic = 1;
+			audit_log_container_id(context, axs->target_cid[i]);
+		}
 	}
 
-	if (context->target_pid &&
-	    audit_log_pid_context(context, context->target_pid,
-				  context->target_auid, context->target_uid,
-				  context->target_sessionid,
-				  context->target_sid, context->target_comm))
+	if (context->target_pid) {
+		if (audit_log_pid_context(context, context->target_pid,
+					  context->target_auid,
+					  context->target_uid,
+					  context->target_sessionid,
+					  context->target_sid,
+					  context->target_comm))
 			call_panic = 1;
+		audit_log_container_id(context, context->target_cid);
+	}
 
 	if (context->pwd.dentry && context->pwd.mnt) {
 		ab = audit_log_start(context, GFP_KERNEL, AUDIT_CWD);

@@ -1568,6 +1574,8 @@ static void audit_log_exit(void)
 
 	audit_log_proctitle();
 
+	audit_log_container_id(context, audit_get_contid(current));
+
 	audit_log_container_drop();
 
 	/* Send end of event record to help user space know we are finished */

Signature · 1 line

1.8.3.1

Richard Guy Briggs

author

2019-09-19 01:22 UTC #24 lore raw

PATCH v7 8/21 audit: add contid support for signalling the audit daemon

Add audit container identifier support to the action of signalling the audit daemon.

Since this would need to add an element to the audit_sig_info struct, a new record type AUDIT_SIGNAL_INFO2 was created with a new audit_sig_info2 struct. Corresponding support is required in the userspace code to reflect the new record request and reply type. An older userspace won't break since it won't know to request this record type.

Signed-off-by Richard Guy Briggs <[email protected]>

---
 include/linux/audit.h       |  7 +++++++
 include/uapi/linux/audit.h  |  1 +
 kernel/audit.c              | 28 ++++++++++++++++++++++++++++
 kernel/audit.h              |  1 +
 security/selinux/nlmsgtab.c |  1 +
 5 files changed, 38 insertions(+)

include/linux/audit.h +7 -0

index 0c18d8e30620..7b640c4da4ee 100644
@@ -23,6 +23,13 @@ struct audit_sig_info {
 	char		ctx[0];
 };
 
+struct audit_sig_info2 {
+	uid_t		uid;
+	pid_t		pid;
+	u64		cid;
+	char		ctx[0];
+};
+
 struct audit_buffer;
 struct audit_context;
 struct inode;

include/uapi/linux/audit.h +1 -0

index 4ed080f28b47..693ec6e0288b 100644
@@ -72,6 +72,7 @@
 #define AUDIT_SET_FEATURE	1018	/* Turn an audit feature on or off */
 #define AUDIT_GET_FEATURE	1019	/* Get which features are enabled */
 #define AUDIT_CONTAINER_OP	1020	/* Define the container id and info */
+#define AUDIT_SIGNAL_INFO2	1021	/* Get info auditd signal sender */
 
 #define AUDIT_FIRST_USER_MSG	1100	/* Userspace messages mostly uninteresting to kernel */
 #define AUDIT_USER_AVC		1107	/* We filter this differently */

kernel/audit.c +28 -0

index adfb3e6a7f0c..df3db29f5a8a 100644
@@ -125,6 +125,7 @@ struct audit_net {
 kuid_t		audit_sig_uid = INVALID_UID;
 pid_t		audit_sig_pid = -1;
 u32		audit_sig_sid = 0;
+u64		audit_sig_cid = AUDIT_CID_UNSET;
 
 /* Records can be lost in several ways:
    0) [suppressed in audit_alloc]

@@ -1094,6 +1095,7 @@ static int audit_netlink_ok(struct sk_buff *skb, u16 msg_type)
 	case AUDIT_ADD_RULE:
 	case AUDIT_DEL_RULE:
 	case AUDIT_SIGNAL_INFO:
+	case AUDIT_SIGNAL_INFO2:
 	case AUDIT_TTY_GET:
 	case AUDIT_TTY_SET:
 	case AUDIT_TRIM:

@@ -1257,6 +1259,7 @@ static int audit_receive_msg(struct sk_buff *skb, struct nlmsghdr *nlh)
 	struct audit_buffer	*ab;
 	u16			msg_type = nlh->nlmsg_type;
 	struct audit_sig_info   *sig_data;
+	struct audit_sig_info2  *sig_data2;
 	char			*ctx = NULL;
 	u32			len;

@@ -1516,6 +1519,30 @@ static int audit_receive_msg(struct sk_buff *skb, struct nlmsghdr *nlh)
 				 sig_data, sizeof(*sig_data) + len);
 		kfree(sig_data);
 		break;
+	case AUDIT_SIGNAL_INFO2:
+		len = 0;
+		if (audit_sig_sid) {
+			err = security_secid_to_secctx(audit_sig_sid, &ctx, &len);
+			if (err)
+				return err;
+		}
+		sig_data2 = kmalloc(sizeof(*sig_data2) + len, GFP_KERNEL);
+		if (!sig_data2) {
+			if (audit_sig_sid)
+				security_release_secctx(ctx, len);
+			return -ENOMEM;
+		}
+		sig_data2->uid = from_kuid(&init_user_ns, audit_sig_uid);
+		sig_data2->pid = audit_sig_pid;
+		if (audit_sig_sid) {
+			memcpy(sig_data2->ctx, ctx, len);
+			security_release_secctx(ctx, len);
+		}
+		sig_data2->cid = audit_sig_cid;
+		audit_send_reply(skb, seq, AUDIT_SIGNAL_INFO2, 0, 0,
+				 sig_data2, sizeof(*sig_data2) + len);
+		kfree(sig_data2);
+		break;
 	case AUDIT_TTY_GET: {
 		struct audit_tty_status s;
 		unsigned int t;

@@ -2384,6 +2411,7 @@ int audit_signal_info(int sig, struct task_struct *t)
 		else
 			audit_sig_uid = uid;
 		security_task_getsecid(current, &audit_sig_sid);
+		audit_sig_cid = audit_get_contid(current);
 	}
 
 	return audit_signal_info_syscall(t);

kernel/audit.h +1 -0

index 543f1334ba47..c9a118716ced 100644
@@ -350,6 +350,7 @@ static inline int audit_signal_info_syscall(struct task_struct *t)
 extern pid_t audit_sig_pid;
 extern kuid_t audit_sig_uid;
 extern u32 audit_sig_sid;
+extern u64 audit_sig_cid;
 
 extern int audit_filter(int msgtype, unsigned int listtype);

security/selinux/nlmsgtab.c +1 -0

index 58345ba0528e..bf21979e7737 100644
@@ -132,6 +132,7 @@ struct nlmsg_perm {
 	{ AUDIT_DEL_RULE,	NETLINK_AUDIT_SOCKET__NLMSG_WRITE    },
 	{ AUDIT_USER,		NETLINK_AUDIT_SOCKET__NLMSG_RELAY    },
 	{ AUDIT_SIGNAL_INFO,	NETLINK_AUDIT_SOCKET__NLMSG_READ     },
+	{ AUDIT_SIGNAL_INFO2,	NETLINK_AUDIT_SOCKET__NLMSG_READ     },
 	{ AUDIT_TRIM,		NETLINK_AUDIT_SOCKET__NLMSG_WRITE    },
 	{ AUDIT_MAKE_EQUIV,	NETLINK_AUDIT_SOCKET__NLMSG_WRITE    },
 	{ AUDIT_TTY_GET,	NETLINK_AUDIT_SOCKET__NLMSG_READ     },

Signature · 1 line

1.8.3.1

Richard Guy Briggs

author

2019-09-19 01:22 UTC #28 lore raw

PATCH v7 9/21 audit: add support for non-syscall auxiliary records

Standalone audit records have the timestamp and serial number generated on the fly and as such are unique, making them standalone. This new function audit_alloc_local() generates a local audit context that will be used only for a standalone record and its auxiliary record(s). The context is discarded immediately after the local associated records are produced.

Signed-off-by Richard Guy Briggs <[email protected]>

---
 include/linux/audit.h |  8 ++++++++
 kernel/audit.h        |  1 +
 kernel/auditsc.c      | 35 ++++++++++++++++++++++++++++++-----
 3 files changed, 39 insertions(+), 5 deletions(-)

include/linux/audit.h +8 -0

index 7b640c4da4ee..e849058cb662 100644
@@ -329,6 +329,8 @@ static inline int audit_signal_info(int sig, struct task_struct *t)
 
 /* These are defined in auditsc.c */
 				/* Public API */
+extern struct audit_context *audit_alloc_local(gfp_t gfpflags);
+extern void audit_free_context(struct audit_context *context);
 extern void __audit_syscall_entry(int major, unsigned long a0, unsigned long a1,
 				  unsigned long a2, unsigned long a3);
 extern void __audit_syscall_exit(int ret_success, long ret_value);

@@ -591,6 +593,12 @@ static inline void audit_ntp_log(const struct audit_ntp_data *ad)
 extern int audit_n_rules;
 extern int audit_signals;
 #else /* CONFIG_AUDITSYSCALL */
+static inline struct audit_context *audit_alloc_local(gfp_t gfpflags)
+{
+	return NULL;
+}
+static inline void audit_free_context(struct audit_context *context)
+{ }
 static inline void audit_syscall_entry(int major, unsigned long a0,
 				       unsigned long a1, unsigned long a2,
 				       unsigned long a3)

kernel/audit.h +1 -0

index c9a118716ced..1bba13bdffd0 100644
@@ -98,6 +98,7 @@ struct audit_proctitle {
 struct audit_context {
 	int		    dummy;	/* must be the first element */
 	int		    in_syscall;	/* 1 if task is in a syscall */
+	bool		    local;	/* local context needed */
 	enum audit_state    state, current_state;
 	unsigned int	    serial;     /* serial number for record */
 	int		    major;      /* syscall number */

kernel/auditsc.c +30 -5

index ac438fcff807..3138c88887c7 100644
@@ -890,11 +890,13 @@ static inline void audit_free_aux(struct audit_context *context)
 	}
 }
 
-static inline struct audit_context *audit_alloc_context(enum audit_state state)
+static inline struct audit_context *audit_alloc_context(enum audit_state state,
+							gfp_t gfpflags)
 {
 	struct audit_context *context;
 
-	context = kzalloc(sizeof(*context), GFP_KERNEL);
+	/* We can be called in atomic context via audit_tg() */
+	context = kzalloc(sizeof(*context), gfpflags);
 	if (!context)
 		return NULL;
 	context->state = state;

@@ -930,7 +932,8 @@ int audit_alloc_syscall(struct task_struct *tsk)
 		return 0;
 	}
 
-	if (!(context = audit_alloc_context(state))) {
+	context = audit_alloc_context(state, GFP_KERNEL);
+	if (!context) {
 		kfree(key);
 		audit_log_lost("out of memory in audit_alloc_syscall");
 		return -ENOMEM;

@@ -942,8 +945,29 @@ int audit_alloc_syscall(struct task_struct *tsk)
 	return 0;
 }
 
-static inline void audit_free_context(struct audit_context *context)
+struct audit_context *audit_alloc_local(gfp_t gfpflags)
 {
+	struct audit_context *context = NULL;
+
+	if (!audit_ever_enabled)
+		goto out; /* Return if not auditing. */
+	context = audit_alloc_context(AUDIT_RECORD_CONTEXT, gfpflags);
+	if (!context) {
+		audit_log_lost("out of memory in audit_alloc_local");
+		goto out;
+	}
+	context->serial = audit_serial();
+	ktime_get_coarse_real_ts64(&context->ctime);
+	context->local = true;
+out:
+	return context;
+}
+EXPORT_SYMBOL(audit_alloc_local);
+
+void audit_free_context(struct audit_context *context)
+{
+	if (!context)
+		return;
 	audit_free_module(context);
 	audit_free_names(context);
 	unroll_tree_refs(context, NULL, 0);

@@ -954,6 +978,7 @@ static inline void audit_free_context(struct audit_context *context)
 	audit_proctitle_free(context);
 	kfree(context);
 }
+EXPORT_SYMBOL(audit_free_context);
 
 static int audit_log_pid_context(struct audit_context *context, pid_t pid,
 				 kuid_t auid, kuid_t uid, unsigned int sessionid,

@@ -2182,7 +2207,7 @@ void __audit_inode_child(struct inode *parent,
 int auditsc_get_stamp(struct audit_context *ctx,
 		       struct timespec64 *t, unsigned int *serial)
 {
-	if (!ctx->in_syscall)
+	if (!ctx->in_syscall && !ctx->local)
 		return 0;
 	if (!ctx->serial)
 		ctx->serial = audit_serial();

Signature · 1 line

1.8.3.1

Richard Guy Briggs

author

2019-09-19 01:22 UTC #29 lore raw

PATCH v7 10/21 audit: add containerid support for user records

Add audit container identifier auxiliary record to user event standalone records.

Signed-off-by Richard Guy Briggs <[email protected]>

---
 kernel/audit.c | 13 ++++++-------
 1 file changed, 6 insertions(+), 7 deletions(-)

kernel/audit.c +6 -7

index df3db29f5a8a..7cdb76b38966 100644
@@ -1140,12 +1140,6 @@ static void audit_log_common_recv_msg(struct audit_context *context,
 	audit_log_task_context(*ab);
 }
 
-static inline void audit_log_user_recv_msg(struct audit_buffer **ab,
-					   u16 msg_type)
-{
-	audit_log_common_recv_msg(NULL, ab, msg_type);
-}
-
 int is_audit_feature_set(int i)
 {
 	return af.features & AUDIT_FEATURE_TO_MASK(i);

@@ -1408,13 +1402,16 @@ static int audit_receive_msg(struct sk_buff *skb, struct nlmsghdr *nlh)
 
 		err = audit_filter(msg_type, AUDIT_FILTER_USER);
 		if (err == 1) { /* match or error */
+			struct audit_context *context;
+
 			err = 0;
 			if (msg_type == AUDIT_USER_TTY) {
 				err = tty_audit_push();
 				if (err)
 					break;
 			}
-			audit_log_user_recv_msg(&ab, msg_type);
+			context = audit_alloc_local(GFP_KERNEL);
+			audit_log_common_recv_msg(context, &ab, msg_type);
 			if (msg_type != AUDIT_USER_TTY)
 				audit_log_format(ab, " msg='%.*s'",
 						 AUDIT_MESSAGE_TEXT_MAX,

@@ -1430,6 +1427,8 @@ static int audit_receive_msg(struct sk_buff *skb, struct nlmsghdr *nlh)
 				audit_log_n_untrustedstring(ab, data, size);
 			}
 			audit_log_end(ab);
+			audit_log_container_id(context, audit_get_contid(current));
+			audit_free_context(context);
 		}
 		break;
 	case AUDIT_ADD_RULE:

Signature · 1 line

1.8.3.1

Richard Guy Briggs

author

2019-09-19 01:22 UTC #30 lore raw

PATCH v7 11/21 audit: add containerid filtering

Implement audit container identifier filtering using the AUDIT_CONTID field name to send an 8-character string representing a u64 since the value field is only u32.

Sending it as two u32 was considered, but gathering and comparing two fields was more complex.

The feature indicator is AUDIT_FEATURE_BITMAP_CONTAINERID.

Please see the github audit kernel issue for the contid filter feature:
https://github.com/linux-audit/audit-kernel/issues/91
Please see the github audit userspace issue for filter additions:
https://github.com/linux-audit/audit-userspace/issues/40
Please see the github audit testsuiite issue for the test case:
https://github.com/linux-audit/audit-testsuite/issues/64
Please see the github audit wiki for the feature overview:
https://github.com/linux-audit/audit-kernel/wiki/RFE-Audit-Container-ID

Signed-off-by Richard Guy Briggs <[email protected]>

---
 include/linux/audit.h      |  1 +
 include/uapi/linux/audit.h |  5 ++++-
 kernel/audit.h             |  1 +
 kernel/auditfilter.c       | 46 ++++++++++++++++++++++++++++++++++++++++++++++
 kernel/auditsc.c           |  4 ++++
 5 files changed, 56 insertions(+), 1 deletion(-)

include/linux/audit.h +1 -0

index e849058cb662..575fff6ea7c9 100644
@@ -69,6 +69,7 @@ struct audit_field {
 	u32				type;
 	union {
 		u32			val;
+		u64			val64;
 		kuid_t			uid;
 		kgid_t			gid;
 		struct {

include/uapi/linux/audit.h +4 -1

index 693ec6e0288b..f34108759e8f 100644
@@ -268,6 +268,7 @@
 #define AUDIT_LOGINUID_SET	24
 #define AUDIT_SESSIONID	25	/* Session ID */
 #define AUDIT_FSTYPE	26	/* FileSystem Type */
+#define AUDIT_CONTID	27	/* Container ID */
 
 				/* These are ONLY useful when checking
 				 * at syscall exit time (AUDIT_AT_EXIT). */

@@ -349,6 +350,7 @@ enum {
 #define AUDIT_FEATURE_BITMAP_SESSIONID_FILTER	0x00000010
 #define AUDIT_FEATURE_BITMAP_LOST_RESET		0x00000020
 #define AUDIT_FEATURE_BITMAP_FILTER_FS		0x00000040
+#define AUDIT_FEATURE_BITMAP_CONTAINERID	0x00000080
 
 #define AUDIT_FEATURE_BITMAP_ALL (AUDIT_FEATURE_BITMAP_BACKLOG_LIMIT | \
 				  AUDIT_FEATURE_BITMAP_BACKLOG_WAIT_TIME | \

@@ -356,7 +358,8 @@ enum {
 				  AUDIT_FEATURE_BITMAP_EXCLUDE_EXTEND | \
 				  AUDIT_FEATURE_BITMAP_SESSIONID_FILTER | \
 				  AUDIT_FEATURE_BITMAP_LOST_RESET | \
-				  AUDIT_FEATURE_BITMAP_FILTER_FS)
+				  AUDIT_FEATURE_BITMAP_FILTER_FS | \
+				  AUDIT_FEATURE_BITMAP_CONTAINERID)
 
 /* deprecated: AUDIT_VERSION_* */
 #define AUDIT_VERSION_LATEST 		AUDIT_FEATURE_BITMAP_ALL

kernel/audit.h +1 -0

index 1bba13bdffd0..c9b73abfd6a0 100644
@@ -229,6 +229,7 @@ static inline int audit_hash_contid(u64 contid)
 
 extern int audit_match_class(int class, unsigned syscall);
 extern int audit_comparator(const u32 left, const u32 op, const u32 right);
+extern int audit_comparator64(const u64 left, const u32 op, const u64 right);
 extern int audit_uid_comparator(kuid_t left, u32 op, kuid_t right);
 extern int audit_gid_comparator(kgid_t left, u32 op, kgid_t right);
 extern int parent_len(const char *path);

kernel/auditfilter.c +46 -0

82 lines not on this page · raw message

kernel/auditsc.c +4 -0

index 3138c88887c7..a658fe775b86 100644
@@ -629,6 +629,10 @@ static int audit_filter_rules(struct task_struct *tsk,
 				result = audit_comparator(ctx->sockaddr->ss_family,
 							  f->op, f->val);
 			break;
+		case AUDIT_CONTID:
+			result = audit_comparator64(audit_get_contid(tsk),
+						    f->op, f->val64);
+			break;
 		case AUDIT_SUBJ_USER:
 		case AUDIT_SUBJ_ROLE:
 		case AUDIT_SUBJ_TYPE:

Signature · 1 line

1.8.3.1

Richard Guy Briggs

author

2019-09-19 01:22 UTC #31 lore raw

PATCH v7 12/21 audit: add support for containerid to network namespaces

Audit events could happen in a network namespace outside of a task context due to packets received from the net that trigger an auditing rule prior to being associated with a running task. The network namespace could be in use by multiple containers by association to the tasks in that network namespace. We still want a way to attribute these events to any potential containers. Keep a list per network namespace to track these audit container identifiiers.

Add/increment the audit container identifier on:
- initial setting of the audit container identifier via /proc
- clone/fork call that inherits an audit container identifier
- unshare call that inherits an audit container identifier
- setns call that inherits an audit container identifier Delete/decrement the audit container identifier on:
- an inherited audit container identifier dropped when child set
- process exit
- unshare call that drops a net namespace
- setns call that drops a net namespace

Please see the github audit kernel issue for contid net support:
https://github.com/linux-audit/audit-kernel/issues/92
Please see the github audit testsuiite issue for the test case:
https://github.com/linux-audit/audit-testsuite/issues/64
Please see the github audit wiki for the feature overview:
https://github.com/linux-audit/audit-kernel/wiki/RFE-Audit-Container-ID

Signed-off-by Richard Guy Briggs <[email protected]>

---
 include/linux/audit.h | 19 +++++++++++
 kernel/audit.c        | 87 +++++++++++++++++++++++++++++++++++++++++++++++++--
 kernel/nsproxy.c      |  4 +++
 3 files changed, 108 insertions(+), 2 deletions(-)

include/linux/audit.h +19 -0

index 575fff6ea7c9..73e3ab38e3e0 100644
@@ -13,6 +13,7 @@
 #include <linux/ptrace.h>
 #include <linux/namei.h>  /* LOOKUP_* */
 #include <uapi/linux/audit.h>
+#include <linux/refcount.h>
 
 #define AUDIT_INO_UNSET ((unsigned long)-1)
 #define AUDIT_DEV_UNSET ((dev_t)-1)

@@ -122,6 +123,13 @@ struct audit_task_info {
 
 extern struct audit_task_info init_struct_audit;
 
+struct audit_contid {
+	struct list_head	list;
+	u64			id;
+	refcount_t		refcount;
+	struct rcu_head		rcu;
+};
+
 extern int is_audit_feature_set(int which);
 
 extern int __init audit_register_class(int class, unsigned *list);

@@ -229,6 +237,10 @@ static inline u64 audit_get_contid(struct task_struct *tsk)
 extern void audit_cont_put(struct audit_cont *cont);
 
 extern void audit_log_container_id(struct audit_context *context, u64 contid);
+extern void audit_netns_contid_add(struct net *net, u64 contid);
+extern void audit_netns_contid_del(struct net *net, u64 contid);
+extern void audit_switch_task_namespaces(struct nsproxy *ns,
+					 struct task_struct *p);
 
 extern u32 audit_enabled;

@@ -309,6 +321,13 @@ static inline void audit_cont_put(struct audit_cont *cont)
 
 static inline void audit_log_container_id(struct audit_context *context, u64 contid)
 { }
+static inline void audit_netns_contid_add(struct net *net, u64 contid)
+{ }
+static inline void audit_netns_contid_del(struct net *net, u64 contid)
+{ }
+static inline void audit_switch_task_namespaces(struct nsproxy *ns,
+						struct task_struct *p)
+{ }
 
 #define audit_enabled AUDIT_OFF

kernel/audit.c +85 -2

140 lines not on this page · raw message

kernel/nsproxy.c +4 -0

index c815f58e6bc0..bbdb5bbf5446 100644
@@ -23,6 +23,7 @@
 #include <linux/syscalls.h>
 #include <linux/cgroup.h>
 #include <linux/perf_event.h>
+#include <linux/audit.h>
 
 static struct kmem_cache *nsproxy_cachep;

@@ -136,6 +137,7 @@ int copy_namespaces(unsigned long flags, struct task_struct *tsk)
 	struct nsproxy *old_ns = tsk->nsproxy;
 	struct user_namespace *user_ns = task_cred_xxx(tsk, user_ns);
 	struct nsproxy *new_ns;
+	u64 contid = audit_get_contid(tsk);
 
 	if (likely(!(flags & (CLONE_NEWNS | CLONE_NEWUTS | CLONE_NEWIPC |
 			      CLONE_NEWPID | CLONE_NEWNET |

@@ -163,6 +165,7 @@ int copy_namespaces(unsigned long flags, struct task_struct *tsk)
 		return  PTR_ERR(new_ns);
 
 	tsk->nsproxy = new_ns;
+	audit_netns_contid_add(new_ns->net_ns, contid);
 	return 0;
 }

@@ -220,6 +223,7 @@ void switch_task_namespaces(struct task_struct *p, struct nsproxy *new)
 	ns = p->nsproxy;
 	p->nsproxy = new;
 	task_unlock(p);
+	audit_switch_task_namespaces(ns, p);
 
 	if (ns && atomic_dec_and_test(&ns->count))
 		free_nsproxy(ns);

Signature · 1 line

1.8.3.1

Richard Guy Briggs

author

2019-09-19 01:22 UTC #33 lore raw

PATCH v7 13/21 audit: NETFILTER_PKT: record each container ID associated with a netNS

Add audit container identifier auxiliary record(s) to NETFILTER_PKT event standalone records. Iterate through all potential audit container identifiers associated with a network namespace.

Signed-off-by Richard Guy Briggs <[email protected]>

---
 include/linux/audit.h    |  5 +++++
 kernel/audit.c           | 39 +++++++++++++++++++++++++++++++++++++++
 net/netfilter/nft_log.c  | 11 +++++++++--
 net/netfilter/xt_AUDIT.c | 11 +++++++++--
 4 files changed, 62 insertions(+), 4 deletions(-)

include/linux/audit.h +5 -0

index 73e3ab38e3e0..dcd92f964120 100644
@@ -241,6 +241,8 @@ static inline u64 audit_get_contid(struct task_struct *tsk)
 extern void audit_netns_contid_del(struct net *net, u64 contid);
 extern void audit_switch_task_namespaces(struct nsproxy *ns,
 					 struct task_struct *p);
+extern void audit_log_netns_contid_list(struct net *net,
+					struct audit_context *context);
 
 extern u32 audit_enabled;

@@ -328,6 +330,9 @@ static inline void audit_netns_contid_del(struct net *net, u64 contid)
 static inline void audit_switch_task_namespaces(struct nsproxy *ns,
 						struct task_struct *p)
 { }
+static inline void audit_log_netns_contid_list(struct net *net,
+					       struct audit_context *context)
+{ }
 
 #define audit_enabled AUDIT_OFF

kernel/audit.c +39 -0

index e0c27bc39925..9ce7a1ec7a92 100644
@@ -450,6 +450,45 @@ void audit_switch_task_namespaces(struct nsproxy *ns, struct task_struct *p)
 		audit_netns_contid_add(new->net_ns, contid);
 }
 
+/**
+ * audit_log_netns_contid_list - List contids for the given network namespace
+ * @net: the network namespace of interest
+ * @context: the audit context to use
+ *
+ * Description:
+ * Issues a CONTAINER_ID record with a CSV list of contids associated
+ * with a network namespace to accompany a NETFILTER_PKT record.
+ */
+void audit_log_netns_contid_list(struct net *net, struct audit_context *context)
+{
+	struct audit_buffer *ab = NULL;
+	struct audit_contid *cont;
+	struct audit_net *aunet;
+
+	/* Generate AUDIT_CONTAINER_ID record with container ID CSV list */
+	rcu_read_lock();
+	aunet = net_generic(net, audit_net_id);
+	if (!aunet)
+		goto out;
+	list_for_each_entry_rcu(cont, &aunet->contid_list, list) {
+		if (!ab) {
+			ab = audit_log_start(context, GFP_ATOMIC,
+					     AUDIT_CONTAINER_ID);
+			if (!ab) {
+				audit_log_lost("out of memory in audit_log_netns_contid_list");
+				goto out;
+			}
+			audit_log_format(ab, "contid=");
+		} else
+			audit_log_format(ab, ",");
+		audit_log_format(ab, "%llu", cont->id);
+	}
+	audit_log_end(ab);
+out:
+	rcu_read_unlock();
+}
+EXPORT_SYMBOL(audit_log_netns_contid_list);
+
 void audit_panic(const char *message)
 {
 	switch (audit_failure) {

net/netfilter/nft_log.c +9 -2

index fe4831f2258f..98d1e7e1a83c 100644
@@ -66,13 +66,16 @@ static void nft_log_eval_audit(const struct nft_pktinfo *pkt)
 	struct sk_buff *skb = pkt->skb;
 	struct audit_buffer *ab;
 	int fam = -1;
+	struct audit_context *context;
+	struct net *net;
 
 	if (!audit_enabled)
 		return;
 
-	ab = audit_log_start(NULL, GFP_ATOMIC, AUDIT_NETFILTER_PKT);
+	context = audit_alloc_local(GFP_ATOMIC);
+	ab = audit_log_start(context, GFP_ATOMIC, AUDIT_NETFILTER_PKT);
 	if (!ab)
-		return;
+		goto errout;
 
 	audit_log_format(ab, "mark=%#x", skb->mark);

@@ -99,6 +102,10 @@ static void nft_log_eval_audit(const struct nft_pktinfo *pkt)
 		audit_log_format(ab, " saddr=? daddr=? proto=-1");
 
 	audit_log_end(ab);
+	net = xt_net(&pkt->xt);
+	audit_log_netns_contid_list(net, context);
+errout:
+	audit_free_context(context);
 }
 
 static void nft_log_eval(const struct nft_expr *expr,

net/netfilter/xt_AUDIT.c +9 -2

index 9cdc16b0d0d8..ecf868a1abde 100644
@@ -68,10 +68,13 @@ static bool audit_ip6(struct audit_buffer *ab, struct sk_buff *skb)
 {
 	struct audit_buffer *ab;
 	int fam = -1;
+	struct audit_context *context;
+	struct net *net;
 
 	if (audit_enabled == AUDIT_OFF)
-		goto errout;
-	ab = audit_log_start(NULL, GFP_ATOMIC, AUDIT_NETFILTER_PKT);
+		goto out;
+	context = audit_alloc_local(GFP_ATOMIC);
+	ab = audit_log_start(context, GFP_ATOMIC, AUDIT_NETFILTER_PKT);
 	if (ab == NULL)
 		goto errout;

@@ -101,7 +104,11 @@ static bool audit_ip6(struct audit_buffer *ab, struct sk_buff *skb)
 
 	audit_log_end(ab);
 
+	net = xt_net(par);
+	audit_log_netns_contid_list(net, context);
 errout:
+	audit_free_context(context);
+out:
 	return XT_CONTINUE;
 }

Signature · 1 line

1.8.3.1

Richard Guy Briggs

author

2019-09-19 01:22 UTC #35 lore raw

PATCH v7 14/21 audit: contid check descendancy and nesting

?fixup! audit: convert to contid list to check for orch/engine ownership

Require the target task to be a descendant of the container orchestrator/engine.

You would only change the audit container ID from one set or inherited value to another if you were nesting containers.

If changing the contid, the container orchestrator/engine must be a descendant and not same orchestrator as the one that set it so it is not possible to change the contid of another orchestrator's container.

Signed-off-by Richard Guy Briggs <[email protected]>

---
 kernel/audit.c | 70 +++++++++++++++++++++++++++++++++++++++++++++++++++-------
 1 file changed, 62 insertions(+), 8 deletions(-)

kernel/audit.c +62 -8

index 9ce7a1ec7a92..69fe1e9af7cb 100644
@@ -2560,6 +2560,39 @@ static struct task_struct *audit_cont_owner(struct task_struct *tsk)
 }
 
 /*
+ * task_is_descendant - walk up a process family tree looking for a match
+ * @parent: the process to compare against while walking up from child
+ * @child: the process to start from while looking upwards for parent
+ *
+ * Returns 1 if child is a descendant of parent, 0 if not.
+ */
+static int task_is_descendant(struct task_struct *parent,
+			      struct task_struct *child)
+{
+	int rc = 0;
+	struct task_struct *walker = child;
+
+	if (!parent || !child)
+		return 0;
+
+	rcu_read_lock();
+	if (!thread_group_leader(parent))
+		parent = rcu_dereference(parent->group_leader);
+	while (walker->pid > 0) {
+		if (!thread_group_leader(walker))
+			walker = rcu_dereference(walker->group_leader);
+		if (walker == parent) {
+			rc = 1;
+			break;
+		}
+		walker = rcu_dereference(walker->real_parent);
+	}
+	rcu_read_unlock();
+
+	return rc;
+}
+
+/*
  * audit_set_contid - set current task's audit contid
  * @task: target task
  * @contid: contid value

@@ -2587,22 +2620,43 @@ int audit_set_contid(struct task_struct *task, u64 contid)
 	oldcontid = audit_get_contid(task);
 	read_lock(&tasklist_lock);
 	/* Don't allow the contid to be unset */
-	if (!audit_contid_valid(contid))
+	if (!audit_contid_valid(contid)) {
 		rc = -EINVAL;
+		goto unlock;
+	}
 	/* Don't allow the contid to be set to the same value again */
-	else if (contid == oldcontid) {
+	if (contid == oldcontid) {
 		rc = -EADDRINUSE;
+		goto unlock;
+	}
 	/* if we don't have caps, reject */
-	else if (!capable(CAP_AUDIT_CONTROL))
+	if (!capable(CAP_AUDIT_CONTROL)) {
 		rc = -EPERM;
-	/* if task has children or is not single-threaded, deny */
-	else if (!list_empty(&task->children))
+		goto unlock;
+	}
+	/* if task has children, deny */
+	if (!list_empty(&task->children)) {
 		rc = -EBUSY;
-	else if (!(thread_group_leader(task) && thread_group_empty(task)))
+		goto unlock;
+	}
+	/* if task is not single-threaded, deny */
+	if (!(thread_group_leader(task) && thread_group_empty(task))) {
 		rc = -EALREADY;
-	/* if contid is already set, deny */
-	else if (audit_contid_set(task))
+		goto unlock;
+	}
+	/* if task is not descendant, block */
+	if (task == current) {
+		rc = -EBADSLT;
+		goto unlock;
+	}
+	if (!task_is_descendant(current, task)) {
+		rc = -EXDEV;
+		goto unlock;
+	}
+	/* only allow contid setting again if nesting */
+	if (audit_contid_set(task) && current == audit_cont_owner(task))
 		rc = -ECHILD;
+unlock:
 	read_unlock(&tasklist_lock);
 	if (!rc) {
 		struct audit_cont *oldcont = audit_cont(task);

Signature · 1 line

1.8.3.1

Richard Guy Briggs

author

2019-09-19 01:22 UTC #39 lore raw

PATCH v7 15/21 sched: pull task_is_descendant into kernel/sched/core.c

Since the task_is_descendant() function is used in YAMA and in audit, pull the function into kernel/core/sched.c

Signed-off-by Richard Guy Briggs <[email protected]>

---
 include/linux/sched.h    |  3 +++
 kernel/audit.c           | 33 ---------------------------------
 kernel/sched/core.c      | 33 +++++++++++++++++++++++++++++++++
 security/yama/yama_lsm.c | 33 ---------------------------------
 4 files changed, 36 insertions(+), 66 deletions(-)

include/linux/sched.h +3 -0

index a936d162513a..b251f018f4db 100644
@@ -1988,4 +1988,7 @@ static inline void rseq_syscall(struct pt_regs *regs)
 
 const struct cpumask *sched_trace_rd_span(struct root_domain *rd);
 
+extern int task_is_descendant(struct task_struct *parent,
+			      struct task_struct *child);
+
 #endif

kernel/audit.c +0 ~~-33~~

index 69fe1e9af7cb..4fe7678304dd 100644
@@ -2560,39 +2560,6 @@ static struct task_struct *audit_cont_owner(struct task_struct *tsk)
 }
 
 /*
- * task_is_descendant - walk up a process family tree looking for a match
- * @parent: the process to compare against while walking up from child
- * @child: the process to start from while looking upwards for parent
- *
- * Returns 1 if child is a descendant of parent, 0 if not.
- */
-static int task_is_descendant(struct task_struct *parent,
-			      struct task_struct *child)
-{
-	int rc = 0;
-	struct task_struct *walker = child;
-
-	if (!parent || !child)
-		return 0;
-
-	rcu_read_lock();
-	if (!thread_group_leader(parent))
-		parent = rcu_dereference(parent->group_leader);
-	while (walker->pid > 0) {
-		if (!thread_group_leader(walker))
-			walker = rcu_dereference(walker->group_leader);
-		if (walker == parent) {
-			rc = 1;
-			break;
-		}
-		walker = rcu_dereference(walker->real_parent);
-	}
-	rcu_read_unlock();
-
-	return rc;
-}
-
-/*
  * audit_set_contid - set current task's audit contid
  * @task: target task
  * @contid: contid value

kernel/sched/core.c +33 -0

index 2b037f195473..7ba9e07381fa 100644
@@ -7509,6 +7509,39 @@ void dump_cpu_task(int cpu)
 }
 
 /*
+ * task_is_descendant - walk up a process family tree looking for a match
+ * @parent: the process to compare against while walking up from child
+ * @child: the process to start from while looking upwards for parent
+ *
+ * Returns 1 if child is a descendant of parent, 0 if not.
+ */
+int task_is_descendant(struct task_struct *parent,
+			      struct task_struct *child)
+{
+	int rc = 0;
+	struct task_struct *walker = child;
+
+	if (!parent || !child)
+		return 0;
+
+	rcu_read_lock();
+	if (!thread_group_leader(parent))
+		parent = rcu_dereference(parent->group_leader);
+	while (walker->pid > 0) {
+		if (!thread_group_leader(walker))
+			walker = rcu_dereference(walker->group_leader);
+		if (walker == parent) {
+			rc = 1;
+			break;
+		}
+		walker = rcu_dereference(walker->real_parent);
+	}
+	rcu_read_unlock();
+
+	return rc;
+}
+
+/*
  * Nice levels are multiplicative, with a gentle 10% change for every
  * nice level changed. I.e. when a CPU-bound task goes from nice 0 to
  * nice 1, it will get ~10% less CPU time than another CPU-bound task

security/yama/yama_lsm.c +0 ~~-33~~

index 94dc346370b1..25eae205eae8 100644
@@ -263,39 +263,6 @@ static int yama_task_prctl(int option, unsigned long arg2, unsigned long arg3,
 }
 
 /**
- * task_is_descendant - walk up a process family tree looking for a match
- * @parent: the process to compare against while walking up from child
- * @child: the process to start from while looking upwards for parent
- *
- * Returns 1 if child is a descendant of parent, 0 if not.
- */
-static int task_is_descendant(struct task_struct *parent,
-			      struct task_struct *child)
-{
-	int rc = 0;
-	struct task_struct *walker = child;
-
-	if (!parent || !child)
-		return 0;
-
-	rcu_read_lock();
-	if (!thread_group_leader(parent))
-		parent = rcu_dereference(parent->group_leader);
-	while (walker->pid > 0) {
-		if (!thread_group_leader(walker))
-			walker = rcu_dereference(walker->group_leader);
-		if (walker == parent) {
-			rc = 1;
-			break;
-		}
-		walker = rcu_dereference(walker->real_parent);
-	}
-	rcu_read_unlock();
-
-	return rc;
-}
-
-/**
  * ptracer_exception_found - tracer registered as exception for this tracee
  * @tracer: the task_struct of the process attempting ptrace
  * @tracee: the task_struct of the process to be ptraced

Signature · 1 line

1.8.3.1

Richard Guy Briggs

author

2019-09-19 01:22 UTC #41 lore raw

PATCH v7 16/21 audit: add support for contid set/get by netlink

Add the ability to get and set the audit container identifier using an audit netlink message using message types AUDIT_SET_CONTID 1023 and AUDIT_GET_CONTID 1022 in addition to using the proc filesystem. The message format includes the data structure:

struct audit_contid_status {
       pid_t   pid;
       u64     id;
};

Signed-off-by Richard Guy Briggs <[email protected]>

---
 include/uapi/linux/audit.h |  2 ++
 kernel/audit.c             | 40 ++++++++++++++++++++++++++++++++++++++++
 kernel/audit.h             |  5 +++++
 3 files changed, 47 insertions(+)

include/uapi/linux/audit.h +2 -0

index f34108759e8f..e26729fc9943 100644
@@ -73,6 +73,8 @@
 #define AUDIT_GET_FEATURE	1019	/* Get which features are enabled */
 #define AUDIT_CONTAINER_OP	1020	/* Define the container id and info */
 #define AUDIT_SIGNAL_INFO2	1021	/* Get info auditd signal sender */
+#define AUDIT_GET_CONTID	1022	/* Get contid of a task */
+#define AUDIT_SET_CONTID	1023	/* Set contid of a task */
 
 #define AUDIT_FIRST_USER_MSG	1100	/* Userspace messages mostly uninteresting to kernel */
 #define AUDIT_USER_AVC		1107	/* We filter this differently */

kernel/audit.c +40 -0

index 4fe7678304dd..df92de20ed73 100644
@@ -1216,6 +1216,8 @@ static int audit_netlink_ok(struct sk_buff *skb, u16 msg_type)
 	case AUDIT_TTY_SET:
 	case AUDIT_TRIM:
 	case AUDIT_MAKE_EQUIV:
+	case AUDIT_GET_CONTID:
+	case AUDIT_SET_CONTID:
 		/* Only support auditd and auditctl in initial pid namespace
 		 * for now. */
 		if (task_active_pid_ns(current) != &init_pid_ns)

@@ -1273,6 +1275,23 @@ static int audit_get_feature(struct sk_buff *skb)
 	return 0;
 }
 
+static int audit_get_contid_status(struct sk_buff *skb)
+{
+	struct nlmsghdr *nlh = nlmsg_hdr(skb);
+	u32 seq = nlh->nlmsg_seq;
+	void *data = nlmsg_data(nlh);
+	struct audit_contid_status cs;
+
+	cs.pid = ((struct audit_contid_status *)data)->pid;
+	if (!cs.pid)
+		cs.pid = task_tgid_nr(current);
+	rcu_read_lock();
+	cs.id = audit_get_contid(find_task_by_vpid(cs.pid));
+	rcu_read_unlock();
+	audit_send_reply(skb, seq, AUDIT_GET_CONTID, 0, 0, &cs, sizeof(cs));
+	return 0;
+}
+
 static void audit_log_feature_change(int which, u32 old_feature, u32 new_feature,
 				     u32 old_lock, u32 new_lock, int res)
 {

@@ -1700,6 +1719,27 @@ static int audit_receive_msg(struct sk_buff *skb, struct nlmsghdr *nlh)
 		audit_log_end(ab);
 		break;
 	}
+	case AUDIT_SET_CONTID: {
+		struct audit_contid_status *s = data;
+		struct task_struct *tsk;
+
+		/* check if new data is valid */
+		if (nlmsg_len(nlh) < sizeof(*s))
+			return -EINVAL;
+		tsk = find_get_task_by_vpid(s->pid);
+		if (!tsk)
+			return -EINVAL;
+
+		err = audit_set_contid(tsk, s->id);
+		put_task_struct(tsk);
+		return err;
+		break;
+	}
+	case AUDIT_GET_CONTID:
+		err = audit_get_contid_status(skb);
+		if (err)
+			return err;
+		break;
 	default:
 		err = -EINVAL;
 		break;

kernel/audit.h +5 -0

index c9b73abfd6a0..25732fbc47a4 100644
@@ -224,6 +224,11 @@ static inline int audit_hash_contid(u64 contid)
 
 #define AUDIT_CONTID_COUNT	1 << 16
 
+struct audit_contid_status {
+	pid_t	pid;
+	u64	id;
+};
+
 /* Indicates that audit should log the full pathname. */
 #define AUDIT_NAME_FULL -1

Signature · 1 line

1.8.3.1

Richard Guy Briggs

author

2019-09-19 01:22 UTC #43 lore raw

PATCH v7 17/21 audit: add support for loginuid/sessionid set/get by netlink

Add the ability to get and set the login uid and to get the session id using an audit netlink message using message types AUDIT_GET_LOGINUID 1024, AUDIT_SET_LOGINUID 1025 and AUDIT_GET_SESSIONID 1026 in addition to using the proc filesystem.

Signed-off-by Richard Guy Briggs <[email protected]>

---
 include/uapi/linux/audit.h |  3 +++
 kernel/audit.c             | 62 ++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 65 insertions(+)

include/uapi/linux/audit.h +3 -0

index e26729fc9943..eef42c8eea77 100644
@@ -75,6 +75,9 @@
 #define AUDIT_SIGNAL_INFO2	1021	/* Get info auditd signal sender */
 #define AUDIT_GET_CONTID	1022	/* Get contid of a task */
 #define AUDIT_SET_CONTID	1023	/* Set contid of a task */
+#define AUDIT_GET_LOGINUID	1024	/* Get loginuid of a task */
+#define AUDIT_SET_LOGINUID	1025	/* Set loginuid of a task */
+#define AUDIT_GET_SESSIONID	1026	/* Set sessionid of a task */
 
 #define AUDIT_FIRST_USER_MSG	1100	/* Userspace messages mostly uninteresting to kernel */
 #define AUDIT_USER_AVC		1107	/* We filter this differently */

kernel/audit.c +62 -0

86 lines not on this page · raw message

Signature · 1 line

1.8.3.1

Richard Guy Briggs

author

2019-09-19 01:22 UTC #45 lore raw

PATCH v7 18/21 audit: track container nesting

Track the parent container of a container to be able to filter and report nesting.

Now that we have a way to track and check the parent container of a container, fixup other patches, or squash all nesting fixes together.

fixup! audit: add container id
fixup! audit: log drop of contid on exit of last task
fixup! audit: log container info of syscalls
fixup! audit: add containerid filtering
fixup! audit: NETFILTER_PKT: record each container ID associated with a netNS
fixup! audit: convert to contid list to check for orch/engine ownership softirq (for netfilter) audit: protect contid list lock from softirq

Signed-off-by Richard Guy Briggs <[email protected]>

---
 include/linux/audit.h |  1 +
 kernel/audit.c        | 67 ++++++++++++++++++++++++++++++++++++++++++---------
 kernel/audit.h        |  3 +++
 kernel/auditfilter.c  | 20 ++++++++++++++-
 kernel/auditsc.c      |  2 +-
 5 files changed, 79 insertions(+), 14 deletions(-)

include/linux/audit.h +1 -0

index dcd92f964120..1ce27af686ea 100644
@@ -110,6 +110,7 @@ struct audit_cont {
 	struct task_struct	*owner;
 	refcount_t              refcount;
 	struct rcu_head         rcu;
+	struct audit_cont	*parent;
 };
 
 struct audit_task_info {

kernel/audit.c +55 ~~-12~~

149 lines not on this page · raw message

kernel/audit.h +3 -0

index 25732fbc47a4..89b7de323c13 100644
@@ -220,6 +220,8 @@ static inline int audit_hash_contid(u64 contid)
 	return (contid & (AUDIT_CONTID_BUCKETS-1));
 }
 
+extern spinlock_t audit_contid_list_lock;
+
 extern int audit_contid_count;
 
 #define AUDIT_CONTID_COUNT	1 << 16

@@ -235,6 +237,7 @@ struct audit_contid_status {
 extern int audit_match_class(int class, unsigned syscall);
 extern int audit_comparator(const u32 left, const u32 op, const u32 right);
 extern int audit_comparator64(const u64 left, const u32 op, const u64 right);
+extern int audit_contid_comparator(const u64 left, const u32 op, const u64 right);
 extern int audit_uid_comparator(kuid_t left, u32 op, kuid_t right);
 extern int audit_gid_comparator(kgid_t left, u32 op, kgid_t right);
 extern int parent_len(const char *path);

kernel/auditfilter.c +19 -1

index 9606f973fe33..513d57d03637 100644
@@ -1297,6 +1297,24 @@ int audit_gid_comparator(kgid_t left, u32 op, kgid_t right)
 	}
 }
 
+int audit_contid_comparator(u64 left, u32 op, u64 right)
+{
+	struct audit_cont *cont = NULL;
+	int h;
+	int result = 0;
+	unsigned long flags;
+
+	h = audit_hash_contid(left);
+	spin_lock_irqsave(&audit_contid_list_lock, flags);
+	list_for_each_entry_rcu(cont, &audit_contid_hash[h], list) {
+		result = audit_comparator64(cont->id, op, right);
+		if (result)
+			break;
+	}
+	spin_unlock_irqrestore(&audit_contid_list_lock, flags);
+	return result;
+}
+
 /**
  * parent_len - find the length of the parent portion of a pathname
  * @path: pathname of which to determine length

@@ -1388,7 +1406,7 @@ int audit_filter(int msgtype, unsigned int listtype)
 							  f->op, f->val);
 				break;
 			case AUDIT_CONTID:
-				result = audit_comparator64(audit_get_contid(current),
+				result = audit_contid_comparator(audit_get_contid(current),
 							    f->op, f->val64);
 				break;
 			case AUDIT_MSGTYPE:

kernel/auditsc.c +1 -1

index a658fe775b86..6bf6d8b9dfd1 100644
@@ -630,7 +630,7 @@ static int audit_filter_rules(struct task_struct *tsk,
 							  f->op, f->val);
 			break;
 		case AUDIT_CONTID:
-			result = audit_comparator64(audit_get_contid(tsk),
+			result = audit_contid_comparator(audit_get_contid(tsk),
 						    f->op, f->val64);
 			break;
 		case AUDIT_SUBJ_USER:

Signature · 1 line

1.8.3.1

Richard Guy Briggs

author

2019-09-19 01:22 UTC #47 lore raw

PATCH v7 19/21 audit: check cont depth

Set an arbitrary limit on the depth of audit container identifier nesting to limit abuse.

Signed-off-by Richard Guy Briggs <[email protected]>

---
 kernel/audit.c | 21 +++++++++++++++++++++
 kernel/audit.h |  2 ++
 2 files changed, 23 insertions(+)

kernel/audit.c +21 -0

index 848fd1c8c579..a70c9184e5d9 100644
@@ -2667,6 +2667,22 @@ int audit_signal_info(int sig, struct task_struct *t)
 	return audit_signal_info_syscall(t);
 }
 
+static int audit_contid_depth(struct audit_cont *cont)
+{
+	struct audit_cont *parent;
+	int depth = 1;
+
+	if (!cont)
+		return 0;
+
+	parent = cont->parent;
+	while (parent) {
+		depth++;
+		parent = parent->parent;
+	}
+	return depth;
+}
+
 struct audit_cont *audit_cont(struct task_struct *tsk)
 {
 	if (!tsk->audit || !tsk->audit->cont)

@@ -2785,6 +2801,11 @@ int audit_set_contid(struct task_struct *task, u64 contid)
 			rc = -ENOSPC;
 			goto conterror;
 		}
+		/* Set max contid depth */
+		if (audit_contid_depth(audit_cont(current->real_parent)) >= AUDIT_CONTID_DEPTH) {
+			rc = -EMLINK;
+			goto conterror;
+		}
 		if (!newcont) {
 			newcont = kmalloc(sizeof(struct audit_cont), GFP_ATOMIC);
 			if (newcont) {

kernel/audit.h +2 -0

index 89b7de323c13..cb25341c1a0f 100644
@@ -231,6 +231,8 @@ struct audit_contid_status {
 	u64	id;
 };
 
+#define AUDIT_CONTID_DEPTH	5
+
 /* Indicates that audit should log the full pathname. */
 #define AUDIT_NAME_FULL -1

Signature · 1 line

1.8.3.1

Richard Guy Briggs

author

2019-09-19 01:22 UTC #48 lore raw

PATCH v7 20/21 audit: add capcontid to set contid outside init_user_ns

Provide a mechanism similar to CAP_AUDIT_CONTROL to explicitly give a process in a non-init user namespace the capability to set audit container identifiers.

Use audit netlink message types AUDIT_GET_CAPCONTID 1027 and AUDIT_SET_CAPCONTID 1028. The message format includes the data structure:

struct audit_capcontid_status {
        pid_t   pid;
        u32     enable;
};

Signed-off-by Richard Guy Briggs <[email protected]>

---
 include/linux/audit.h      | 14 +++++++
 include/uapi/linux/audit.h |  2 +
 kernel/audit.c             | 98 +++++++++++++++++++++++++++++++++++++++++++++-
 kernel/audit.h             |  5 +++
 4 files changed, 117 insertions(+), 2 deletions(-)

include/linux/audit.h +14 -0

index 1ce27af686ea..dcc53e62e266 100644
@@ -117,6 +117,7 @@ struct audit_task_info {
 	kuid_t			loginuid;
 	unsigned int		sessionid;
 	struct audit_cont	*cont;
+	u32			capcontid;
 #ifdef CONFIG_AUDITSYSCALL
 	struct audit_context	*ctx;
 #endif

@@ -224,6 +225,14 @@ static inline unsigned int audit_get_sessionid(struct task_struct *tsk)
 	return tsk->audit->sessionid;
 }
 
+static inline u32 audit_get_capcontid(struct task_struct *tsk)
+{
+	if (!tsk->audit)
+		return 0;
+	return tsk->audit->capcontid;
+}
+
+extern int audit_set_capcontid(struct task_struct *tsk, u32 enable);
 extern int audit_set_contid(struct task_struct *tsk, u64 contid);
 
 static inline u64 audit_get_contid(struct task_struct *tsk)

@@ -309,6 +318,11 @@ static inline unsigned int audit_get_sessionid(struct task_struct *tsk)
 	return AUDIT_SID_UNSET;
 }
 
+static inline u32 audit_get_capcontid(struct task_struct *tsk)
+{
+	return 0;
+}
+
 static inline u64 audit_get_contid(struct task_struct *tsk)
 {
 	return AUDIT_CID_UNSET;

include/uapi/linux/audit.h +2 -0

index eef42c8eea77..011b0a8ee9b2 100644
@@ -78,6 +78,8 @@
 #define AUDIT_GET_LOGINUID	1024	/* Get loginuid of a task */
 #define AUDIT_SET_LOGINUID	1025	/* Set loginuid of a task */
 #define AUDIT_GET_SESSIONID	1026	/* Set sessionid of a task */
+#define AUDIT_GET_CAPCONTID	1027	/* Get cap_contid of a task */
+#define AUDIT_SET_CAPCONTID	1028	/* Set cap_contid of a task */
 
 #define AUDIT_FIRST_USER_MSG	1100	/* Userspace messages mostly uninteresting to kernel */
 #define AUDIT_USER_AVC		1107	/* We filter this differently */

kernel/audit.c +96 -2

128 lines not on this page · raw message

kernel/audit.h +5 -0

index cb25341c1a0f..ac4694e88485 100644
@@ -231,6 +231,11 @@ struct audit_contid_status {
 	u64	id;
 };
 
+struct audit_capcontid_status {
+	pid_t	pid;
+	u32	enable;
+};
+
 #define AUDIT_CONTID_DEPTH	5
 
 /* Indicates that audit should log the full pathname. */

Signature · 1 line

1.8.3.1

Richard Guy Briggs

author

2019-09-19 01:22 UTC #71 lore raw

PATCH v7 21/21 audit: add proc interface for capcontid

Add a /proc interface to capcontid for testing purposes. This isn't intended to be merged upstream. Container orchestrators/engines are expected to link to libaudit to use the functions audit_set_capcontid() and audit_get_capcontid.

Signed-off-by Richard Guy Briggs <[email protected]>

---
 fs/proc/base.c | 55 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 55 insertions(+)

fs/proc/base.c +55 -0

index 26091800180c..283ef8e006e7 100644
@@ -1360,6 +1360,59 @@ static ssize_t proc_contid_write(struct file *file, const char __user *buf,
 	.write		= proc_contid_write,
 	.llseek		= generic_file_llseek,
 };
+
+static ssize_t proc_capcontid_read(struct file *file, char __user *buf,
+				  size_t count, loff_t *ppos)
+{
+	struct inode *inode = file_inode(file);
+	struct task_struct *task = get_proc_task(inode);
+	ssize_t length;
+	char tmpbuf[TMPBUFLEN];
+
+	if (!task)
+		return -ESRCH;
+	/* if we don't have caps, reject */
+	if (!capable(CAP_AUDIT_CONTROL) && !audit_get_capcontid(current))
+		return -EPERM;
+	length = scnprintf(tmpbuf, TMPBUFLEN, "%u", audit_get_capcontid(task));
+	put_task_struct(task);
+	return simple_read_from_buffer(buf, count, ppos, tmpbuf, length);
+}
+
+static ssize_t proc_capcontid_write(struct file *file, const char __user *buf,
+				   size_t count, loff_t *ppos)
+{
+	struct inode *inode = file_inode(file);
+	u32 capcontid;
+	int rv;
+	struct task_struct *task = get_proc_task(inode);
+
+	if (!task)
+		return -ESRCH;
+	if (*ppos != 0) {
+		/* No partial writes. */
+		put_task_struct(task);
+		return -EINVAL;
+	}
+
+	rv = kstrtou32_from_user(buf, count, 10, &capcontid);
+	if (rv < 0) {
+		put_task_struct(task);
+		return rv;
+	}
+
+	rv = audit_set_capcontid(task, capcontid);
+	put_task_struct(task);
+	if (rv < 0)
+		return rv;
+	return count;
+}
+
+static const struct file_operations proc_capcontid_operations = {
+	.read		= proc_capcontid_read,
+	.write		= proc_capcontid_write,
+	.llseek		= generic_file_llseek,
+};
 #endif
 
 #ifdef CONFIG_FAULT_INJECTION

@@ -3121,6 +3174,7 @@ static int proc_stack_depth(struct seq_file *m, struct pid_namespace *ns,
 	REG("loginuid",   S_IWUSR|S_IRUGO, proc_loginuid_operations),
 	REG("sessionid",  S_IRUGO, proc_sessionid_operations),
 	REG("audit_containerid", S_IWUSR|S_IRUSR, proc_contid_operations),
+	REG("audit_capcontainerid", S_IWUSR|S_IRUSR|S_IRUSR, proc_capcontid_operations),
 #endif
 #ifdef CONFIG_FAULT_INJECTION
 	REG("make-it-fail", S_IRUGO|S_IWUSR, proc_fault_inject_operations),

@@ -3522,6 +3576,7 @@ static int proc_tid_comm_permission(struct inode *inode, int mask)
 	REG("loginuid",  S_IWUSR|S_IRUGO, proc_loginuid_operations),
 	REG("sessionid",  S_IRUGO, proc_sessionid_operations),
 	REG("audit_containerid", S_IWUSR|S_IRUSR, proc_contid_operations),
+	REG("audit_capcontainerid", S_IWUSR|S_IRUSR|S_IRUSR, proc_capcontid_operations),
 #endif
 #ifdef CONFIG_FAULT_INJECTION
 	REG("make-it-fail", S_IRUGO|S_IWUSR, proc_fault_inject_operations),

Signature · 1 line

1.8.3.1

Neil Horman

2019-09-26 14:46 UTC #5 lore raw

Re: [PATCH ghak90 V7 04/21] audit: convert to contid list to check for orch/engine ownership

Quoted 44 lines · Richard Guy Briggs

Store the audit container identifier in a refcounted kernel object that is added to the master list of audit container identifiers. This will allow multiple container orchestrators/engines to work on the same machine without danger of inadvertantly re-using an existing identifier. It will also allow an orchestrator to inject a process into an existing container by checking if the original container owner is the one injecting the task. A hash table list is used to optimize searches.
Signed-off-by Richard Guy Briggs <[email protected]>
---
 include/linux/audit.h | 26 ++++++++++++++--
 kernel/audit.c        | 86 ++++++++++++++++++++++++++++++++++++++++++++++++---
 kernel/audit.h        |  8 +++++
 3 files changed, 112 insertions(+), 8 deletions(-)
include/linux/audit.h +9 -2
index f2e3b81f2942..e317807cdd3e 100644
@@ -95,10 +95,18 @@ struct audit_ntp_data {
 struct audit_ntp_data {};
 #endif
 
+struct audit_cont {
+	struct list_head	list;
+	u64			id;
+	struct task_struct	*owner;
+	refcount_t              refcount;
+	struct rcu_head         rcu;
+};
+
 struct audit_task_info {
 	kuid_t			loginuid;
 	unsigned int		sessionid;
-	u64			contid;
+	struct audit_cont	*cont;
 #ifdef CONFIG_AUDITSYSCALL
 	struct audit_context	*ctx;
 #endif
@@ -203,11 +211,15 @@ static inline unsigned int audit_get_sessionid(struct task_struct *tsk)
 
 static inline u64 audit_get_contid(struct task_struct *tsk)
 {
-	if (!tsk->audit)

include/linux/audit.h static inline unsigned int audit_get_sessionid(struct task_struct *tsk) · line 222

Quoted 10 lines

include/linux/audit.h +6 -1

+	if (!tsk->audit || !tsk->audit->cont)
 		return AUDIT_CID_UNSET;
-	return tsk->audit->contid;
+	return tsk->audit->cont->id;
 }
 
+extern struct audit_cont *audit_cont(struct task_struct *tsk);
+
+extern void audit_cont_put(struct audit_cont *cont);
+

I see that you manual increment this refcount at various call sites, why no corresponding audit_contid_hold function?

Neil

Quoted 196 lines

include/linux/audit.h +8 -0

 extern u32 audit_enabled;
 
 extern int audit_signal_info(int sig, struct task_struct *t);

@@ -277,6 +289,14 @@ static inline u64 audit_get_contid(struct task_struct *tsk)
 	return AUDIT_CID_UNSET;
 }
 
+static inline struct audit_cont *audit_cont(struct task_struct *tsk)
+{
+	return NULL;
+}
+
+static inline void audit_cont_put(struct audit_cont *cont)
+{ }
+
 #define audit_enabled AUDIT_OFF
 
 static inline int audit_signal_info(int sig, struct task_struct *t)

kernel/audit.c +81 -5

index a36ea57cbb61..ea0899130cc1 100644
@@ -137,6 +137,8 @@ struct audit_net {
 
 /* Hash for inode-based rules */
 struct list_head audit_inode_hash[AUDIT_INODE_BUCKETS];
+/* Hash for contid-based rules */
+struct list_head audit_contid_hash[AUDIT_CONTID_BUCKETS];
 
 static struct kmem_cache *audit_buffer_cache;

@@ -204,6 +206,8 @@ struct audit_reply {
 
 static struct kmem_cache *audit_task_cache;
 
+static DEFINE_SPINLOCK(audit_contid_list_lock);
+
 void __init audit_task_init(void)
 {
 	audit_task_cache = kmem_cache_create("audit_task",

@@ -231,7 +235,9 @@ int audit_alloc(struct task_struct *tsk)
 	}
 	info->loginuid = audit_get_loginuid(current);
 	info->sessionid = audit_get_sessionid(current);
-	info->contid = audit_get_contid(current);
+	info->cont = audit_cont(current);
+	if (info->cont)
+		refcount_inc(&info->cont->refcount);
 	tsk->audit = info;
 
 	ret = audit_alloc_syscall(tsk);

@@ -246,7 +252,7 @@ int audit_alloc(struct task_struct *tsk)
 struct audit_task_info init_struct_audit = {
 	.loginuid = INVALID_UID,
 	.sessionid = AUDIT_SID_UNSET,
-	.contid = AUDIT_CID_UNSET,
+	.cont = NULL,
 #ifdef CONFIG_AUDITSYSCALL
 	.ctx = NULL,
 #endif

@@ -266,6 +272,9 @@ void audit_free(struct task_struct *tsk)
 	/* Freeing the audit_task_info struct must be performed after
 	 * audit_log_exit() due to need for loginuid and sessionid.
 	 */
+	spin_lock(&audit_contid_list_lock); 
+	audit_cont_put(tsk->audit->cont);
+	spin_unlock(&audit_contid_list_lock); 
 	info = tsk->audit;
 	tsk->audit = NULL;
 	kmem_cache_free(audit_task_cache, info);

@@ -1657,6 +1666,9 @@ static int __init audit_init(void)
 	for (i = 0; i < AUDIT_INODE_BUCKETS; i++)
 		INIT_LIST_HEAD(&audit_inode_hash[i]);
 
+	for (i = 0; i < AUDIT_CONTID_BUCKETS; i++)
+		INIT_LIST_HEAD(&audit_contid_hash[i]);
+
 	mutex_init(&audit_cmd_mutex.lock);
 	audit_cmd_mutex.owner = NULL;

@@ -2356,6 +2368,32 @@ int audit_signal_info(int sig, struct task_struct *t)
 	return audit_signal_info_syscall(t);
 }
 
+struct audit_cont *audit_cont(struct task_struct *tsk)
+{
+	if (!tsk->audit || !tsk->audit->cont)
+		return NULL;
+	return tsk->audit->cont;
+}
+
+/* audit_contid_list_lock must be held by caller */
+void audit_cont_put(struct audit_cont *cont)
+{
+	if (!cont)
+		return;
+	if (refcount_dec_and_test(&cont->refcount)) {
+		put_task_struct(cont->owner);
+		list_del_rcu(&cont->list);
+		kfree_rcu(cont, rcu);
+	}
+}
+
+static struct task_struct *audit_cont_owner(struct task_struct *tsk)
+{
+	if (tsk->audit && tsk->audit->cont)
+		return tsk->audit->cont->owner;
+	return NULL;
+}
+
 /*
  * audit_set_contid - set current task's audit contid
  * @task: target task

@@ -2382,9 +2420,12 @@ int audit_set_contid(struct task_struct *task, u64 contid)
 	}
 	oldcontid = audit_get_contid(task);
 	read_lock(&tasklist_lock);
-	/* Don't allow the audit containerid to be unset */
+	/* Don't allow the contid to be unset */
 	if (!audit_contid_valid(contid))
 		rc = -EINVAL;
+	/* Don't allow the contid to be set to the same value again */
+	else if (contid == oldcontid) {
+		rc = -EADDRINUSE;
 	/* if we don't have caps, reject */
 	else if (!capable(CAP_AUDIT_CONTROL))
 		rc = -EPERM;

@@ -2397,8 +2438,43 @@ int audit_set_contid(struct task_struct *task, u64 contid)
 	else if (audit_contid_set(task))
 		rc = -ECHILD;
 	read_unlock(&tasklist_lock);
-	if (!rc)
-		task->audit->contid = contid;
+	if (!rc) {
+		struct audit_cont *oldcont = audit_cont(task);
+		struct audit_cont *cont = NULL;
+		struct audit_cont *newcont = NULL;
+		int h = audit_hash_contid(contid);
+
+		spin_lock(&audit_contid_list_lock);
+		list_for_each_entry_rcu(cont, &audit_contid_hash[h], list)
+			if (cont->id == contid) {
+				/* task injection to existing container */
+				if (current == cont->owner) {
+					refcount_inc(&cont->refcount);
+					newcont = cont;
+				} else {
+					rc = -ENOTUNIQ;
+					goto conterror;
+				}
+			}
+		if (!newcont) {
+			newcont = kmalloc(sizeof(struct audit_cont), GFP_ATOMIC);
+			if (newcont) {
+				INIT_LIST_HEAD(&newcont->list);
+				newcont->id = contid;
+				get_task_struct(current);
+				newcont->owner = current;
+				refcount_set(&newcont->refcount, 1);
+				list_add_rcu(&newcont->list, &audit_contid_hash[h]);
+			} else {
+				rc = -ENOMEM;
+				goto conterror;
+			}
+		}
+		task->audit->cont = newcont;
+		audit_cont_put(oldcont);
+conterror:
+		spin_unlock(&audit_contid_list_lock);
+	}
 	task_unlock(task);
 
 	if (!audit_enabled)

kernel/audit.h +8 -0

index 16bd03b88e0d..e4a31aa92dfe 100644
@@ -211,6 +211,14 @@ static inline int audit_hash_ino(u32 ino)
 	return (ino & (AUDIT_INODE_BUCKETS-1));
 }
 
+#define AUDIT_CONTID_BUCKETS	32
+extern struct list_head audit_contid_hash[AUDIT_CONTID_BUCKETS];
+
+static inline int audit_hash_contid(u64 contid)
+{
+	return (contid & (AUDIT_CONTID_BUCKETS-1));
+}
+
 /* Indicates that audit should log the full pathname. */
 #define AUDIT_NAME_FULL -1

Signature · 1 line

1.8.3.1

Neil Horman

2019-09-27 12:51 UTC #15 lore raw

Re: [PATCH ghak90 V7 06/21] audit: contid limit of 32k imposed to avoid DoS

Quoted 30 lines · Richard Guy Briggs

Set an arbitrary limit on the number of audit container identifiers to limit abuse.

Signed-off-by Richard Guy Briggs <[email protected]>

---
 kernel/audit.c | 8 ++++++++
 kernel/audit.h | 4 ++++
 2 files changed, 12 insertions(+)

kernel/audit.c +2 -0

index 53d13d638c63..329916534dd2 100644
@@ -139,6 +139,7 @@ struct audit_net {
 struct list_head audit_inode_hash[AUDIT_INODE_BUCKETS];
 /* Hash for contid-based rules */
 struct list_head audit_contid_hash[AUDIT_CONTID_BUCKETS];
+int audit_contid_count = 0;
 
 static struct kmem_cache *audit_buffer_cache;

@@ -2384,6 +2385,7 @@ void audit_cont_put(struct audit_cont *cont)
 		put_task_struct(cont->owner);
 		list_del_rcu(&cont->list);
 		kfree_rcu(cont, rcu);
+		audit_contid_count--;
 	}
 }

kernel/audit.c int audit_set_contid(struct task_struct *task, u64 contid) · line 2465

Quoted 8 lines

kernel/audit.c +5 -0

@@ -2456,6 +2458,11 @@ int audit_set_contid(struct task_struct *task, u64 contid)
 					goto conterror;
 				}
 			}
+		/* Set max contids */
+		if (audit_contid_count > AUDIT_CONTID_COUNT) {
+			rc = -ENOSPC;
+			goto conterror;
+		}

You should check for audit_contid_count == AUDIT_CONTID_COUNT here, no? or at least >=, since you increment it below. Otherwise its possible that you will exceed it by one in the full condition.

Quoted 16 lines

kernel/audit.c +1 -0

 		if (!newcont) {
 			newcont = kmalloc(sizeof(struct audit_cont), GFP_ATOMIC);
 			if (newcont) {

@@ -2465,6 +2472,7 @@ int audit_set_contid(struct task_struct *task, u64 contid)
 				newcont->owner = current;
 				refcount_set(&newcont->refcount, 1);
 				list_add_rcu(&newcont->list, &audit_contid_hash[h]);
+				audit_contid_count++;
 			} else {
 				rc = -ENOMEM;
 				goto conterror;

kernel/audit.h +0 -0

index 162de8366b32..543f1334ba47 100644

kernel/audit.h static inline int audit_hash_contid(u64 contid) · line 225

Quoted 7 lines

kernel/audit.h +4 -0

@@ -219,6 +219,10 @@ static inline int audit_hash_contid(u64 contid)
 	return (contid & (AUDIT_CONTID_BUCKETS-1));
 }
 
+extern int audit_contid_count;
+
+#define AUDIT_CONTID_COUNT	1 << 16
+

Just to ask the question, since it wasn't clear in the changelog, what abuse are you avoiding here? Ostensibly you should be able to create as many container ids as you have space for, and the simple creation of container ids doesn't seem like the resource strain I would be concerned about here, given that an orchestrator can still create as many containers as the system will otherwise allow, which will consume significantly more ram/disk/etc.

Quoted 7 lines

kernel/audit.h +0 -0

 /* Indicates that audit should log the full pathname. */
 #define AUDIT_NAME_FULL -1

Signature · 1 line

1.8.3.1

Paul Moore

maintainer

2019-10-11 00:38 UTC #8 lore raw

Re: [PATCH ghak90 V7 04/21] audit: convert to contid list to check for orch/engine ownership

Quoted 8 lines · Richard Guy Briggs

Store the audit container identifier in a refcounted kernel object that is added to the master list of audit container identifiers. This will allow multiple container orchestrators/engines to work on the same machine without danger of inadvertantly re-using an existing identifier. It will also allow an orchestrator to inject a process into an existing container by checking if the original container owner is the one injecting the task. A hash table list is used to optimize searches.

Quoted 6 lines

Signed-off-by Richard Guy Briggs <[email protected]>

---
 include/linux/audit.h | 26 ++++++++++++++--
 kernel/audit.c        | 86 ++++++++++++++++++++++++++++++++++++++++++++++++---
 kernel/audit.h        |  8 +++++
 3 files changed, 112 insertions(+), 8 deletions(-)

One general comment before we go off into the weeds on this ... I can understand why you wanted to keep this patch separate from the earlier patches, but as we get closer to having mergeable code this should get folded into the previous patches. For example, there shouldn't be a change in audit_task_info where you change the contid field from a u64 to struct pointer, it should be a struct pointer from the start.

It's also disappointing that idr appears to only be for 32-bit ID values, if we had a 64-bit idr I think we could simplify this greatly.

Quoted 5 lines

include/linux/audit.h +0 -0
index f2e3b81f2942..e317807cdd3e 100644

include/linux/audit.h struct audit_ntp_data { · line 104

Quoted 10 lines

include/linux/audit.h +7 -0

@@ -95,10 +95,18 @@ struct audit_ntp_data {
 struct audit_ntp_data {};
 #endif
 
+struct audit_cont {
+       struct list_head        list;
+       u64                     id;
+       struct task_struct      *owner;
+       refcount_t              refcount;
+       struct rcu_head         rcu;
+};

It seems as though in most of the code you are using "contid", any reason why didn't stick with that naming scheme here, e.g. "struct audit_contid"?

include/linux/audit.h struct audit_ntp_data { · line 109

Quoted 5 lines

include/linux/audit.h +1 -1

 struct audit_task_info {
        kuid_t                  loginuid;
        unsigned int            sessionid;
-       u64                     contid;
+       struct audit_cont       *cont;

Same, why not stick with "contid"?

include/linux/audit.h static inline unsigned int audit_get_sessionid(struct task_struct *tsk) · line 217

Quoted 13 lines

include/linux/audit.h +2 -2

 #ifdef CONFIG_AUDITSYSCALL
        struct audit_context    *ctx;
 #endif

@@ -203,11 +211,15 @@ static inline unsigned int audit_get_sessionid(struct task_struct *tsk)
 
 static inline u64 audit_get_contid(struct task_struct *tsk)
 {
-       if (!tsk->audit)
+       if (!tsk->audit || !tsk->audit->cont)
                return AUDIT_CID_UNSET;
-       return tsk->audit->contid;
+       return tsk->audit->cont->id;
 }

Assuming for a moment that we implement an audit_contid_get() (see Neil's comment as well as mine below), we probably need to name this something different so we don't all lose our minds when we read this code. On the plus side we can probably preface it with an underscore since it is a static, in which case _audit_contid_get() might be okay, but I'm open to suggestions.

include/linux/audit.h static inline unsigned int audit_get_sessionid(struct task_struct *tsk) · line 221

Quoted 3 lines

include/linux/audit.h +3 -0

+extern struct audit_cont *audit_cont(struct task_struct *tsk);
+
+extern void audit_cont_put(struct audit_cont *cont);

More of the "contid" vs "cont".

Quoted 32 lines

include/linux/audit.h +8 -0

 extern u32 audit_enabled;
 
 extern int audit_signal_info(int sig, struct task_struct *t);

@@ -277,6 +289,14 @@ static inline u64 audit_get_contid(struct task_struct *tsk)
        return AUDIT_CID_UNSET;
 }
 
+static inline struct audit_cont *audit_cont(struct task_struct *tsk)
+{
+       return NULL;
+}
+
+static inline void audit_cont_put(struct audit_cont *cont)
+{ }
+
 #define audit_enabled AUDIT_OFF
 
 static inline int audit_signal_info(int sig, struct task_struct *t)

kernel/audit.c +2 -0

index a36ea57cbb61..ea0899130cc1 100644
@@ -137,6 +137,8 @@ struct audit_net {
 
 /* Hash for inode-based rules */
 struct list_head audit_inode_hash[AUDIT_INODE_BUCKETS];
+/* Hash for contid-based rules */
+struct list_head audit_contid_hash[AUDIT_CONTID_BUCKETS];
 
 static struct kmem_cache *audit_buffer_cache;

kernel/audit.c struct audit_reply { · line 209

Quoted 4 lines

kernel/audit.c +1 -0

@@ -204,6 +206,8 @@ struct audit_reply {
 
 static struct kmem_cache *audit_task_cache;
 
+static DEFINE_SPINLOCK(audit_contid_list_lock);

Since it looks like this protectects audit_contid_hash, I think it would be better to move it up underneath audit_contid_hash.

kernel/audit.c int audit_alloc(struct task_struct *tsk) · line 240

Quoted 11 lines

kernel/audit.c +3 -1

 void __init audit_task_init(void)
 {
        audit_task_cache = kmem_cache_create("audit_task",

@@ -231,7 +235,9 @@ int audit_alloc(struct task_struct *tsk)
        }
        info->loginuid = audit_get_loginuid(current);
        info->sessionid = audit_get_sessionid(current);
-       info->contid = audit_get_contid(current);
+       info->cont = audit_cont(current);
+       if (info->cont)
+               refcount_inc(&info->cont->refcount);

See the other comments about a "get" function, but I think we need a RCU read lock around the above, no?

kernel/audit.c int audit_alloc(struct task_struct *tsk) · line 255

Quoted 9 lines

kernel/audit.c +1 -1

        tsk->audit = info;
 
        ret = audit_alloc_syscall(tsk);

@@ -246,7 +252,7 @@ int audit_alloc(struct task_struct *tsk)
 struct audit_task_info init_struct_audit = {
        .loginuid = INVALID_UID,
        .sessionid = AUDIT_SID_UNSET,
-       .contid = AUDIT_CID_UNSET,
+       .cont = NULL,

More "cont" vs "contid".

kernel/audit.c void audit_free(struct task_struct *tsk) · line 277

Quoted 10 lines

kernel/audit.c +3 -0

 #ifdef CONFIG_AUDITSYSCALL
        .ctx = NULL,
 #endif

@@ -266,6 +272,9 @@ void audit_free(struct task_struct *tsk)
        /* Freeing the audit_task_info struct must be performed after
         * audit_log_exit() due to need for loginuid and sessionid.
         */
+       spin_lock(&audit_contid_list_lock);
+       audit_cont_put(tsk->audit->cont);
+       spin_unlock(&audit_contid_list_lock);

Perhaps this will make sense as I get further into the patchset, but why not move the spin lock operations into audit_[cont/contid]_put()?

Quoted 25 lines

kernel/audit.c +11 -0

        info = tsk->audit;
        tsk->audit = NULL;
        kmem_cache_free(audit_task_cache, info);

@@ -1657,6 +1666,9 @@ static int __init audit_init(void)
        for (i = 0; i < AUDIT_INODE_BUCKETS; i++)
                INIT_LIST_HEAD(&audit_inode_hash[i]);
 
+       for (i = 0; i < AUDIT_CONTID_BUCKETS; i++)
+               INIT_LIST_HEAD(&audit_contid_hash[i]);
+
        mutex_init(&audit_cmd_mutex.lock);
        audit_cmd_mutex.owner = NULL;

@@ -2356,6 +2368,32 @@ int audit_signal_info(int sig, struct task_struct *t)
        return audit_signal_info_syscall(t);
 }
 
+struct audit_cont *audit_cont(struct task_struct *tsk)
+{
+       if (!tsk->audit || !tsk->audit->cont)
+               return NULL;
+       return tsk->audit->cont;
+}
+
+/* audit_contid_list_lock must be held by caller */

kernel/audit.c int audit_signal_info(int sig, struct task_struct *t) · line 2388

Quoted 10 lines

kernel/audit.c +10 -0

+void audit_cont_put(struct audit_cont *cont)
+{
+       if (!cont)
+               return;
+       if (refcount_dec_and_test(&cont->refcount)) {
+               put_task_struct(cont->owner);
+               list_del_rcu(&cont->list);
+               kfree_rcu(cont, rcu);
+       }
+}

I tend to agree with Neil's previous comment; if we've got a audit_[cont/contid]_put(), why not an audit_[cont/contid]_get()?

kernel/audit.c int audit_signal_info(int sig, struct task_struct *t) · line 2395

Quoted 6 lines

kernel/audit.c +6 -0

+static struct task_struct *audit_cont_owner(struct task_struct *tsk)
+{
+       if (tsk->audit && tsk->audit->cont)
+               return tsk->audit->cont->owner;
+       return NULL;
+}

I'm not sure if this is possible (I haven't make my way through the entire patchset) and the function above isn't used in this patch (why is it here?), but it seems like it would be safer to convert this into an audit_contid_isowner() function that simply returns 1/0 depending on if the passed task_struct is the owner or not of a passed audit container ID value?

Quoted 7 lines

kernel/audit.c +0 -0

 /*
  * audit_set_contid - set current task's audit contid
  * @task: target task

@@ -2382,9 +2420,12 @@ int audit_set_contid(struct task_struct *task, u64 contid)
        }
        oldcontid = audit_get_contid(task);
        read_lock(&tasklist_lock);

kernel/audit.c int audit_set_contid(struct task_struct *task, u64 contid) · line 2431

Quoted 10 lines

kernel/audit.c +4 -1

-       /* Don't allow the audit containerid to be unset */
+       /* Don't allow the contid to be unset */
        if (!audit_contid_valid(contid))
                rc = -EINVAL;
+       /* Don't allow the contid to be set to the same value again */
+       else if (contid == oldcontid) {
+               rc = -EADDRINUSE;
        /* if we don't have caps, reject */
        else if (!capable(CAP_AUDIT_CONTROL))
                rc = -EPERM;

RCU read lock? It's a bit dicey since I believe the tasklist_lock is going to provide us the safety we need, but if we are going to claim that the audit container ID list is protected by RCU we should probably use it.

kernel/audit.c int audit_set_contid(struct task_struct *task, u64 contid) · line 2442

Quoted 8 lines

kernel/audit.c +2 -2

@@ -2397,8 +2438,43 @@ int audit_set_contid(struct task_struct *task, u64 contid)
        else if (audit_contid_set(task))
                rc = -ECHILD;
        read_unlock(&tasklist_lock);
-       if (!rc)
-               task->audit->contid = contid;
+       if (!rc) {
+               struct audit_cont *oldcont = audit_cont(task);

Previously we held the tasklist_lock to protect the audit container ID associated with the struct, should we still be holding it here?

Regardless, I worry that the lock dependencies between the tasklist_lock and the audit_contid_list_lock are going to be tricky. It might be nice to document the relationship in a comment up near where you declare audit_contid_list_lock.

kernel/audit.c int audit_set_contid(struct task_struct *task, u64 contid) · line 2451

Quoted 9 lines

kernel/audit.c +9 -0

+               struct audit_cont *cont = NULL;
+               struct audit_cont *newcont = NULL;
+               int h = audit_hash_contid(contid);
+
+               spin_lock(&audit_contid_list_lock);
+               list_for_each_entry_rcu(cont, &audit_contid_hash[h], list)
+                       if (cont->id == contid) {
+                               /* task injection to existing container */
+                               if (current == cont->owner) {

I understand the desire to limit a given audit container ID to the orchestrator that created it, but are we certain that we can track audit container ID "ownership" via a single instance of a task_struct?
What happens when the orchestrator stops/restarts/crashes? Do we
even care?

kernel/audit.c int audit_set_contid(struct task_struct *task, u64 contid) · line 2453

Quoted 2 lines

kernel/audit.c +2 -0

+                                       refcount_inc(&cont->refcount);
+                                       newcont = cont;

We can bail out of the loop here, yes?

Quoted 36 lines

kernel/audit.c +24 -0

+                               } else {
+                                       rc = -ENOTUNIQ;
+                                       goto conterror;
+                               }
+                       }
+               if (!newcont) {
+                       newcont = kmalloc(sizeof(struct audit_cont), GFP_ATOMIC);
+                       if (newcont) {
+                               INIT_LIST_HEAD(&newcont->list);
+                               newcont->id = contid;
+                               get_task_struct(current);
+                               newcont->owner = current;
+                               refcount_set(&newcont->refcount, 1);
+                               list_add_rcu(&newcont->list, &audit_contid_hash[h]);
+                       } else {
+                               rc = -ENOMEM;
+                               goto conterror;
+                       }
+               }
+               task->audit->cont = newcont;
+               audit_cont_put(oldcont);
+conterror:
+               spin_unlock(&audit_contid_list_lock);
+       }
        task_unlock(task);
 
        if (!audit_enabled)

kernel/audit.h +1 -0

index 16bd03b88e0d..e4a31aa92dfe 100644
@@ -211,6 +211,14 @@ static inline int audit_hash_ino(u32 ino)
        return (ino & (AUDIT_INODE_BUCKETS-1));
 }
 
+#define AUDIT_CONTID_BUCKETS   32

kernel/audit.h static inline int audit_hash_ino(u32 ino) · line 224

Quoted 10 lines

kernel/audit.h +7 -0

+extern struct list_head audit_contid_hash[AUDIT_CONTID_BUCKETS];
+
+static inline int audit_hash_contid(u64 contid)
+{
+       return (contid & (AUDIT_CONTID_BUCKETS-1));
+}
+
 /* Indicates that audit should log the full pathname. */
 #define AUDIT_NAME_FULL -1

--
paul moore
www.paul-moore.com

Paul Moore

maintainer

2019-10-11 00:38 UTC #12 lore raw

Re: [PATCH ghak90 V7 05/21] audit: log drop of contid on exit of last task

Quoted 41 lines · Richard Guy Briggs

Signed-off-by Richard Guy Briggs <[email protected]>

---
 kernel/audit.c   | 32 ++++++++++++++++++++++++++++++++
 kernel/audit.h   |  2 ++
 kernel/auditsc.c |  2 ++
 3 files changed, 36 insertions(+)

kernel/audit.c +21 -0

index ea0899130cc1..53d13d638c63 100644
@@ -2503,6 +2503,38 @@ int audit_set_contid(struct task_struct *task, u64 contid)
        return rc;
 }
 
+void audit_log_container_drop(void)
+{
+       struct audit_buffer *ab;
+       uid_t uid;
+       struct tty_struct *tty;
+       char comm[sizeof(current->comm)];
+
+       if (!current->audit || !current->audit->cont ||
+           refcount_read(&current->audit->cont->refcount) > 1)
+               return;
+       ab = audit_log_start(audit_context(), GFP_KERNEL, AUDIT_CONTAINER_OP);
+       if (!ab)
+               return;
+
+       uid = from_kuid(&init_user_ns, task_uid(current));
+       tty = audit_get_tty();
+       audit_log_format(ab,
+                        "op=drop opid=%d contid=%llu old-contid=%llu pid=%d uid=%u auid=%u tty=%s ses=%u",
+                        task_tgid_nr(current), audit_get_contid(current),
+                        audit_get_contid(current), task_tgid_nr(current), uid,
+                        from_kuid(&init_user_ns, audit_get_loginuid(current)),

kernel/audit.c int audit_set_contid(struct task_struct *task, u64 contid) · line 2536

Quoted 10 lines

kernel/audit.c +10 -0

+                        tty ? tty_name(tty) : "(none)",
+                                audit_get_sessionid(current));
+       audit_put_tty(tty);
+       audit_log_task_context(ab);
+       audit_log_format(ab, " comm=");
+       audit_log_untrustedstring(ab, get_task_comm(comm, current));
+       audit_log_d_path_exe(ab, current->mm);
+       audit_log_format(ab, " res=1");
+       audit_log_end(ab);
+}

Why can't we just do this in audit_cont_put()? Is it because we call audit_cont_put() in the new audit_free() function? What if we were to do it in __audit_free()/audit_free_syscall()?

Paul Moore

maintainer

2019-10-11 00:38 UTC #16 lore raw

Re: [PATCH ghak90 V7 06/21] audit: contid limit of 32k imposed to avoid DoS

Quoted 14 lines · Neil Horman

Quoted 13 lines · Richard Guy Briggs
Set an arbitrary limit on the number of audit container identifiers to limit abuse.
Signed-off-by Richard Guy Briggs <[email protected]>
---
 kernel/audit.c | 8 ++++++++
 kernel/audit.h | 4 ++++
 2 files changed, 12 insertions(+)
kernel/audit.c +0 -0
index 53d13d638c63..329916534dd2 100644

...

Quoted 16 lines

kernel/audit.c +1 -0

@@ -2465,6 +2472,7 @@ int audit_set_contid(struct task_struct *task, u64 contid)
                              newcont->owner = current;
                              refcount_set(&newcont->refcount, 1);
                              list_add_rcu(&newcont->list, &audit_contid_hash[h]);
+                             audit_contid_count++;
                      } else {
                              rc = -ENOMEM;
                              goto conterror;

kernel/audit.h +0 -0

index 162de8366b32..543f1334ba47 100644
@@ -219,6 +219,10 @@ static inline int audit_hash_contid(u64 contid)
      return (contid & (AUDIT_CONTID_BUCKETS-1));
 }

Quoted 12 lines

kernel/audit.h static inline int audit_hash_contid(u64 contid) · line 225
Quoted 4 lines
kernel/audit.h +4 -0
+extern int audit_contid_count;
+
+#define AUDIT_CONTID_COUNT   1 << 16
+
Just to ask the question, since it wasn't clear in the changelog, what abuse are you avoiding here? Ostensibly you should be able to create as many container ids as you have space for, and the simple creation of container ids doesn't seem like the resource strain I would be concerned about here, given that an orchestrator can still create as many containers as the system will otherwise allow, which will consume significantly more ram/disk/etc.

I've got a similar question. Up to this point in the patchset, there is a potential issue of hash bucket chain lengths and traversing them with a spinlock held, but it seems like we shouldn't be putting an arbitrary limit on audit container IDs unless we have a good reason for it. If for some reason we do want to enforce a limit, it should probably be a tunable value like a sysctl, or similar.

Paul Moore

maintainer

2019-10-11 00:39 UTC #25 lore raw

Re: [PATCH ghak90 V7 08/21] audit: add contid support for signalling the audit daemon

Quoted 109 lines · Richard Guy Briggs

Add audit container identifier support to the action of signalling the audit daemon.

Signed-off-by Richard Guy Briggs <[email protected]>

---
 include/linux/audit.h       |  7 +++++++
 include/uapi/linux/audit.h  |  1 +
 kernel/audit.c              | 28 ++++++++++++++++++++++++++++
 kernel/audit.h              |  1 +
 security/selinux/nlmsgtab.c |  1 +
 5 files changed, 38 insertions(+)

include/linux/audit.h +7 -0

index 0c18d8e30620..7b640c4da4ee 100644
@@ -23,6 +23,13 @@ struct audit_sig_info {
        char            ctx[0];
 };
 
+struct audit_sig_info2 {
+       uid_t           uid;
+       pid_t           pid;
+       u64             cid;
+       char            ctx[0];
+};
+
 struct audit_buffer;
 struct audit_context;
 struct inode;

include/uapi/linux/audit.h +1 -0

index 4ed080f28b47..693ec6e0288b 100644
@@ -72,6 +72,7 @@
 #define AUDIT_SET_FEATURE      1018    /* Turn an audit feature on or off */
 #define AUDIT_GET_FEATURE      1019    /* Get which features are enabled */
 #define AUDIT_CONTAINER_OP     1020    /* Define the container id and info */
+#define AUDIT_SIGNAL_INFO2     1021    /* Get info auditd signal sender */
 
 #define AUDIT_FIRST_USER_MSG   1100    /* Userspace messages mostly uninteresting to kernel */
 #define AUDIT_USER_AVC         1107    /* We filter this differently */

kernel/audit.c +27 -0

index adfb3e6a7f0c..df3db29f5a8a 100644
@@ -125,6 +125,7 @@ struct audit_net {
 kuid_t         audit_sig_uid = INVALID_UID;
 pid_t          audit_sig_pid = -1;
 u32            audit_sig_sid = 0;
+u64            audit_sig_cid = AUDIT_CID_UNSET;
 
 /* Records can be lost in several ways:
    0) [suppressed in audit_alloc]

@@ -1094,6 +1095,7 @@ static int audit_netlink_ok(struct sk_buff *skb, u16 msg_type)
        case AUDIT_ADD_RULE:
        case AUDIT_DEL_RULE:
        case AUDIT_SIGNAL_INFO:
+       case AUDIT_SIGNAL_INFO2:
        case AUDIT_TTY_GET:
        case AUDIT_TTY_SET:
        case AUDIT_TRIM:

@@ -1257,6 +1259,7 @@ static int audit_receive_msg(struct sk_buff *skb, struct nlmsghdr *nlh)
        struct audit_buffer     *ab;
        u16                     msg_type = nlh->nlmsg_type;
        struct audit_sig_info   *sig_data;
+       struct audit_sig_info2  *sig_data2;
        char                    *ctx = NULL;
        u32                     len;

@@ -1516,6 +1519,30 @@ static int audit_receive_msg(struct sk_buff *skb, struct nlmsghdr *nlh)
                                 sig_data, sizeof(*sig_data) + len);
                kfree(sig_data);
                break;
+       case AUDIT_SIGNAL_INFO2:
+               len = 0;
+               if (audit_sig_sid) {
+                       err = security_secid_to_secctx(audit_sig_sid, &ctx, &len);
+                       if (err)
+                               return err;
+               }
+               sig_data2 = kmalloc(sizeof(*sig_data2) + len, GFP_KERNEL);
+               if (!sig_data2) {
+                       if (audit_sig_sid)
+                               security_release_secctx(ctx, len);
+                       return -ENOMEM;
+               }
+               sig_data2->uid = from_kuid(&init_user_ns, audit_sig_uid);
+               sig_data2->pid = audit_sig_pid;
+               if (audit_sig_sid) {
+                       memcpy(sig_data2->ctx, ctx, len);
+                       security_release_secctx(ctx, len);
+               }
+               sig_data2->cid = audit_sig_cid;
+               audit_send_reply(skb, seq, AUDIT_SIGNAL_INFO2, 0, 0,
+                                sig_data2, sizeof(*sig_data2) + len);
+               kfree(sig_data2);
+               break;
        case AUDIT_TTY_GET: {
                struct audit_tty_status s;
                unsigned int t;

kernel/audit.c int audit_signal_info(int sig, struct task_struct *t) · line 2415

Quoted 5 lines

kernel/audit.c +1 -0

@@ -2384,6 +2411,7 @@ int audit_signal_info(int sig, struct task_struct *t)
                else
                        audit_sig_uid = uid;
                security_task_getsecid(current, &audit_sig_sid);
+               audit_sig_cid = audit_get_contid(current);
        }

I've been wondering something as I've been working my way through these patches and this patch seems like a good spot to discuss this ... Now that we have the concept of an audit container ID "lifetime" in the kernel, when do we consider the ID gone? Is it when the last process in the container exits, or is it when we generate the last audit record which could possibly contain the audit container ID? This patch would appear to support the former, but if we wanted the latter we would need to grab a reference to the audit container ID struct so it wouldn't "die" on us before we could emit the signal info record.

Paul Moore

maintainer

2019-10-11 00:39 UTC #32 lore raw

Re: [PATCH ghak90 V7 12/21] audit: add support for containerid to network namespaces

Quoted 33 lines · Richard Guy Briggs

Audit events could happen in a network namespace outside of a task context due to packets received from the net that trigger an auditing rule prior to being associated with a running task. The network namespace could be in use by multiple containers by association to the tasks in that network namespace. We still want a way to attribute these events to any potential containers. Keep a list per network namespace to track these audit container identifiiers.
Add/increment the audit container identifier on:
- initial setting of the audit container identifier via /proc
- clone/fork call that inherits an audit container identifier
- unshare call that inherits an audit container identifier
- setns call that inherits an audit container identifier Delete/decrement the audit container identifier on:
- an inherited audit container identifier dropped when child set
- process exit
- unshare call that drops a net namespace
- setns call that drops a net namespace
Please see the github audit kernel issue for contid net support:
https://github.com/linux-audit/audit-kernel/issues/92
Please see the github audit testsuiite issue for the test case:
https://github.com/linux-audit/audit-testsuite/issues/64
Please see the github audit wiki for the feature overview:
https://github.com/linux-audit/audit-kernel/wiki/RFE-Audit-Container-ID
Signed-off-by Richard Guy Briggs <[email protected]>
Acked-by Neil Horman <[email protected]>
Reviewed-by Ondrej Mosnacek <[email protected]>
---
 include/linux/audit.h | 19 +++++++++++
 kernel/audit.c        | 87 +++++++++++++++++++++++++++++++++++++++++++++++++--
 kernel/nsproxy.c      |  4 +++
 3 files changed, 108 insertions(+), 2 deletions(-)

...

Quoted 19 lines

kernel/audit.c +11 -0

index 7cdb76b38966..e0c27bc39925 100644
@@ -373,6 +381,75 @@ static struct sock *audit_get_sk(const struct net *net)
        return aunet->sk;
 }
 
+void audit_netns_contid_add(struct net *net, u64 contid)
+{
+       struct audit_net *aunet;
+       struct list_head *contid_list;
+       struct audit_contid *cont;
+
+       if (!net)
+               return;
+       if (!audit_contid_valid(contid))
+               return;
+       aunet = net_generic(net, audit_net_id);

kernel/audit.c static struct sock *audit_get_sk(const struct net *net) · line 404

Quoted 10 lines

kernel/audit.c +10 -0

+       if (!aunet)
+               return;
+       contid_list = &aunet->contid_list;
+       spin_lock(&aunet->contid_list_lock);
+       list_for_each_entry_rcu(cont, contid_list, list)
+               if (cont->id == contid) {
+                       refcount_inc(&cont->refcount);
+                       goto out;
+               }
+       cont = kmalloc(sizeof(struct audit_contid), GFP_ATOMIC);

kmalloc(sizeof(*cont), GFP_ATOMIC)

Quoted 9 lines

kernel/audit.c +9 -0

+       if (cont) {
+               INIT_LIST_HEAD(&cont->list);
+               cont->id = contid;
+               refcount_set(&cont->refcount, 1);
+               list_add_rcu(&cont->list, contid_list);
+       }
+out:
+       spin_unlock(&aunet->contid_list_lock);
+}

Paul Moore

maintainer

2019-10-11 00:39 UTC #34 lore raw

Re: [PATCH ghak90 V7 13/21] audit: NETFILTER_PKT: record each container ID associated with a netNS

Quoted 7 lines · Richard Guy Briggs

Add audit container identifier auxiliary record(s) to NETFILTER_PKT event standalone records. Iterate through all potential audit container identifiers associated with a network namespace.
Signed-off-by Richard Guy Briggs <[email protected]>
Acked-by Neil Horman <[email protected]>
Reviewed-by Ondrej Mosnacek <[email protected]>

Quoted 6 lines

---
 include/linux/audit.h    |  5 +++++
 kernel/audit.c           | 39 +++++++++++++++++++++++++++++++++++++++
 net/netfilter/nft_log.c  | 11 +++++++++--
 net/netfilter/xt_AUDIT.c | 11 +++++++++--
 4 files changed, 62 insertions(+), 4 deletions(-)

This should be squashed together with patch 12/21; neither patch makes sense by themselves.

Quoted 129 lines

include/linux/audit.h +5 -0

index 73e3ab38e3e0..dcd92f964120 100644
@@ -241,6 +241,8 @@ static inline u64 audit_get_contid(struct task_struct *tsk)
 extern void audit_netns_contid_del(struct net *net, u64 contid);
 extern void audit_switch_task_namespaces(struct nsproxy *ns,
                                         struct task_struct *p);
+extern void audit_log_netns_contid_list(struct net *net,
+                                       struct audit_context *context);
 
 extern u32 audit_enabled;

@@ -328,6 +330,9 @@ static inline void audit_netns_contid_del(struct net *net, u64 contid)
 static inline void audit_switch_task_namespaces(struct nsproxy *ns,
                                                struct task_struct *p)
 { }
+static inline void audit_log_netns_contid_list(struct net *net,
+                                              struct audit_context *context)
+{ }
 
 #define audit_enabled AUDIT_OFF

kernel/audit.c +39 -0

index e0c27bc39925..9ce7a1ec7a92 100644
@@ -450,6 +450,45 @@ void audit_switch_task_namespaces(struct nsproxy *ns, struct task_struct *p)
                audit_netns_contid_add(new->net_ns, contid);
 }
 
+/**
+ * audit_log_netns_contid_list - List contids for the given network namespace
+ * @net: the network namespace of interest
+ * @context: the audit context to use
+ *
+ * Description:
+ * Issues a CONTAINER_ID record with a CSV list of contids associated
+ * with a network namespace to accompany a NETFILTER_PKT record.
+ */
+void audit_log_netns_contid_list(struct net *net, struct audit_context *context)
+{
+       struct audit_buffer *ab = NULL;
+       struct audit_contid *cont;
+       struct audit_net *aunet;
+
+       /* Generate AUDIT_CONTAINER_ID record with container ID CSV list */
+       rcu_read_lock();
+       aunet = net_generic(net, audit_net_id);
+       if (!aunet)
+               goto out;
+       list_for_each_entry_rcu(cont, &aunet->contid_list, list) {
+               if (!ab) {
+                       ab = audit_log_start(context, GFP_ATOMIC,
+                                            AUDIT_CONTAINER_ID);
+                       if (!ab) {
+                               audit_log_lost("out of memory in audit_log_netns_contid_list");
+                               goto out;
+                       }
+                       audit_log_format(ab, "contid=");
+               } else
+                       audit_log_format(ab, ",");
+               audit_log_format(ab, "%llu", cont->id);
+       }
+       audit_log_end(ab);
+out:
+       rcu_read_unlock();
+}
+EXPORT_SYMBOL(audit_log_netns_contid_list);
+
 void audit_panic(const char *message)
 {
        switch (audit_failure) {

net/netfilter/nft_log.c +9 -2

index fe4831f2258f..98d1e7e1a83c 100644
@@ -66,13 +66,16 @@ static void nft_log_eval_audit(const struct nft_pktinfo *pkt)
        struct sk_buff *skb = pkt->skb;
        struct audit_buffer *ab;
        int fam = -1;
+       struct audit_context *context;
+       struct net *net;
 
        if (!audit_enabled)
                return;
 
-       ab = audit_log_start(NULL, GFP_ATOMIC, AUDIT_NETFILTER_PKT);
+       context = audit_alloc_local(GFP_ATOMIC);
+       ab = audit_log_start(context, GFP_ATOMIC, AUDIT_NETFILTER_PKT);
        if (!ab)
-               return;
+               goto errout;
 
        audit_log_format(ab, "mark=%#x", skb->mark);

@@ -99,6 +102,10 @@ static void nft_log_eval_audit(const struct nft_pktinfo *pkt)
                audit_log_format(ab, " saddr=? daddr=? proto=-1");
 
        audit_log_end(ab);
+       net = xt_net(&pkt->xt);
+       audit_log_netns_contid_list(net, context);
+errout:
+       audit_free_context(context);
 }
 
 static void nft_log_eval(const struct nft_expr *expr,

net/netfilter/xt_AUDIT.c +5 -2

index 9cdc16b0d0d8..ecf868a1abde 100644
@@ -68,10 +68,13 @@ static bool audit_ip6(struct audit_buffer *ab, struct sk_buff *skb)
 {
        struct audit_buffer *ab;
        int fam = -1;
+       struct audit_context *context;
+       struct net *net;
 
        if (audit_enabled == AUDIT_OFF)
-               goto errout;
-       ab = audit_log_start(NULL, GFP_ATOMIC, AUDIT_NETFILTER_PKT);
+               goto out;
+       context = audit_alloc_local(GFP_ATOMIC);
+       ab = audit_log_start(context, GFP_ATOMIC, AUDIT_NETFILTER_PKT);
        if (ab == NULL)
                goto errout;

@@ -101,7 +104,11 @@ static bool audit_ip6(struct audit_buffer *ab, struct sk_buff *skb)

net/netfilter/xt_AUDIT.c static bool audit_ip6(struct audit_buffer *ab, struct sk_buff *skb) · line 114

Quoted 10 lines

net/netfilter/xt_AUDIT.c +4 -0

        audit_log_end(ab);
 
+       net = xt_net(par);
+       audit_log_netns_contid_list(net, context);
 errout:
+       audit_free_context(context);
+out:
        return XT_CONTINUE;
 }

--
paul moore
www.paul-moore.com

Paul Moore

maintainer

2019-10-11 00:40 UTC #36 lore raw

Re: [PATCH ghak90 V7 14/21] audit: contid check descendancy and nesting

Quoted 1 line · Richard Guy Briggs

?fixup! audit: convert to contid list to check for orch/engine ownership

Quoted 9 lines

Require the target task to be a descendant of the container orchestrator/engine.
You would only change the audit container ID from one set or inherited value to another if you were nesting containers.
If changing the contid, the container orchestrator/engine must be a descendant and not same orchestrator as the one that set it so it is not possible to change the contid of another orchestrator's container.

Did you mean to say that the container orchestrator must be an ancestor of the target, and the same orchestrator as the one that set the target process' audit container ID?

Or maybe I'm missing something about what you are trying to do?

Quoted 101 lines

Signed-off-by Richard Guy Briggs <[email protected]>

---
 kernel/audit.c | 70 +++++++++++++++++++++++++++++++++++++++++++++++++++-------
 1 file changed, 62 insertions(+), 8 deletions(-)

kernel/audit.c +62 -8

index 9ce7a1ec7a92..69fe1e9af7cb 100644
@@ -2560,6 +2560,39 @@ static struct task_struct *audit_cont_owner(struct task_struct *tsk)
 }
 
 /*
+ * task_is_descendant - walk up a process family tree looking for a match
+ * @parent: the process to compare against while walking up from child
+ * @child: the process to start from while looking upwards for parent
+ *
+ * Returns 1 if child is a descendant of parent, 0 if not.
+ */
+static int task_is_descendant(struct task_struct *parent,
+                             struct task_struct *child)
+{
+       int rc = 0;
+       struct task_struct *walker = child;
+
+       if (!parent || !child)
+               return 0;
+
+       rcu_read_lock();
+       if (!thread_group_leader(parent))
+               parent = rcu_dereference(parent->group_leader);
+       while (walker->pid > 0) {
+               if (!thread_group_leader(walker))
+                       walker = rcu_dereference(walker->group_leader);
+               if (walker == parent) {
+                       rc = 1;
+                       break;
+               }
+               walker = rcu_dereference(walker->real_parent);
+       }
+       rcu_read_unlock();
+
+       return rc;
+}
+
+/*
  * audit_set_contid - set current task's audit contid
  * @task: target task
  * @contid: contid value

@@ -2587,22 +2620,43 @@ int audit_set_contid(struct task_struct *task, u64 contid)
        oldcontid = audit_get_contid(task);
        read_lock(&tasklist_lock);
        /* Don't allow the contid to be unset */
-       if (!audit_contid_valid(contid))
+       if (!audit_contid_valid(contid)) {
                rc = -EINVAL;
+               goto unlock;
+       }
        /* Don't allow the contid to be set to the same value again */
-       else if (contid == oldcontid) {
+       if (contid == oldcontid) {
                rc = -EADDRINUSE;
+               goto unlock;
+       }
        /* if we don't have caps, reject */
-       else if (!capable(CAP_AUDIT_CONTROL))
+       if (!capable(CAP_AUDIT_CONTROL)) {
                rc = -EPERM;
-       /* if task has children or is not single-threaded, deny */
-       else if (!list_empty(&task->children))
+               goto unlock;
+       }
+       /* if task has children, deny */
+       if (!list_empty(&task->children)) {
                rc = -EBUSY;
-       else if (!(thread_group_leader(task) && thread_group_empty(task)))
+               goto unlock;
+       }
+       /* if task is not single-threaded, deny */
+       if (!(thread_group_leader(task) && thread_group_empty(task))) {
                rc = -EALREADY;
-       /* if contid is already set, deny */
-       else if (audit_contid_set(task))
+               goto unlock;
+       }
+       /* if task is not descendant, block */
+       if (task == current) {
+               rc = -EBADSLT;
+               goto unlock;
+       }
+       if (!task_is_descendant(current, task)) {
+               rc = -EXDEV;
+               goto unlock;
+       }
+       /* only allow contid setting again if nesting */
+       if (audit_contid_set(task) && current == audit_cont_owner(task))
                rc = -ECHILD;
+unlock:
        read_unlock(&tasklist_lock);
        if (!rc) {
                struct audit_cont *oldcont = audit_cont(task);

Paul Moore

maintainer

2019-10-11 00:40 UTC #40 lore raw

Re: [PATCH ghak90 V7 15/21] sched: pull task_is_descendant into kernel/sched/core.c

Quoted 10 lines · Richard Guy Briggs

Since the task_is_descendant() function is used in YAMA and in audit, pull the function into kernel/core/sched.c
Signed-off-by Richard Guy Briggs <[email protected]>
---
 include/linux/sched.h    |  3 +++
 kernel/audit.c           | 33 ---------------------------------
 kernel/sched/core.c      | 33 +++++++++++++++++++++++++++++++++
 security/yama/yama_lsm.c | 33 ---------------------------------
 4 files changed, 36 insertions(+), 66 deletions(-)

I'm not really reviewing this as I'm still a little confused from patch 14/21, but if 14/21 works out as correct this patch should be squashed into that one.

Quoted 134 lines

include/linux/sched.h +3 -0

index a936d162513a..b251f018f4db 100644
@@ -1988,4 +1988,7 @@ static inline void rseq_syscall(struct pt_regs *regs)
 
 const struct cpumask *sched_trace_rd_span(struct root_domain *rd);
 
+extern int task_is_descendant(struct task_struct *parent,
+                             struct task_struct *child);
+
 #endif

kernel/audit.c +0 ~~-33~~

index 69fe1e9af7cb..4fe7678304dd 100644
@@ -2560,39 +2560,6 @@ static struct task_struct *audit_cont_owner(struct task_struct *tsk)
 }
 
 /*
- * task_is_descendant - walk up a process family tree looking for a match
- * @parent: the process to compare against while walking up from child
- * @child: the process to start from while looking upwards for parent
- *
- * Returns 1 if child is a descendant of parent, 0 if not.
- */
-static int task_is_descendant(struct task_struct *parent,
-                             struct task_struct *child)
-{
-       int rc = 0;
-       struct task_struct *walker = child;
-
-       if (!parent || !child)
-               return 0;
-
-       rcu_read_lock();
-       if (!thread_group_leader(parent))
-               parent = rcu_dereference(parent->group_leader);
-       while (walker->pid > 0) {
-               if (!thread_group_leader(walker))
-                       walker = rcu_dereference(walker->group_leader);
-               if (walker == parent) {
-                       rc = 1;
-                       break;
-               }
-               walker = rcu_dereference(walker->real_parent);
-       }
-       rcu_read_unlock();
-
-       return rc;
-}
-
-/*
  * audit_set_contid - set current task's audit contid
  * @task: target task
  * @contid: contid value

kernel/sched/core.c +33 -0

index 2b037f195473..7ba9e07381fa 100644
@@ -7509,6 +7509,39 @@ void dump_cpu_task(int cpu)
 }
 
 /*
+ * task_is_descendant - walk up a process family tree looking for a match
+ * @parent: the process to compare against while walking up from child
+ * @child: the process to start from while looking upwards for parent
+ *
+ * Returns 1 if child is a descendant of parent, 0 if not.
+ */
+int task_is_descendant(struct task_struct *parent,
+                             struct task_struct *child)
+{
+       int rc = 0;
+       struct task_struct *walker = child;
+
+       if (!parent || !child)
+               return 0;
+
+       rcu_read_lock();
+       if (!thread_group_leader(parent))
+               parent = rcu_dereference(parent->group_leader);
+       while (walker->pid > 0) {
+               if (!thread_group_leader(walker))
+                       walker = rcu_dereference(walker->group_leader);
+               if (walker == parent) {
+                       rc = 1;
+                       break;
+               }
+               walker = rcu_dereference(walker->real_parent);
+       }
+       rcu_read_unlock();
+
+       return rc;
+}
+
+/*
  * Nice levels are multiplicative, with a gentle 10% change for every
  * nice level changed. I.e. when a CPU-bound task goes from nice 0 to
  * nice 1, it will get ~10% less CPU time than another CPU-bound task

security/yama/yama_lsm.c +0 ~~-26~~

index 94dc346370b1..25eae205eae8 100644
@@ -263,39 +263,6 @@ static int yama_task_prctl(int option, unsigned long arg2, unsigned long arg3,
 }
 
 /**
- * task_is_descendant - walk up a process family tree looking for a match
- * @parent: the process to compare against while walking up from child
- * @child: the process to start from while looking upwards for parent
- *
- * Returns 1 if child is a descendant of parent, 0 if not.
- */
-static int task_is_descendant(struct task_struct *parent,
-                             struct task_struct *child)
-{
-       int rc = 0;
-       struct task_struct *walker = child;
-
-       if (!parent || !child)
-               return 0;
-
-       rcu_read_lock();
-       if (!thread_group_leader(parent))
-               parent = rcu_dereference(parent->group_leader);
-       while (walker->pid > 0) {
-               if (!thread_group_leader(walker))
-                       walker = rcu_dereference(walker->group_leader);
-               if (walker == parent) {
-                       rc = 1;
-                       break;
-               }
-               walker = rcu_dereference(walker->real_parent);

security/yama/yama_lsm.c static int yama_task_prctl(int option, unsigned long arg2, unsigned long arg3, · line 268

Quoted 10 lines

security/yama/yama_lsm.c +0 -7

-       }
-       rcu_read_unlock();
-
-       return rc;
-}
-
-/**
  * ptracer_exception_found - tracer registered as exception for this tracee
  * @tracer: the task_struct of the process attempting ptrace
  * @tracee: the task_struct of the process to be ptraced

--
paul moore
www.paul-moore.com

Paul Moore

maintainer

2019-10-11 00:40 UTC #42 lore raw

Re: [PATCH ghak90 V7 16/21] audit: add support for contid set/get by netlink

Quoted 10 lines · Richard Guy Briggs

Add the ability to get and set the audit container identifier using an audit netlink message using message types AUDIT_SET_CONTID 1023 and AUDIT_GET_CONTID 1022 in addition to using the proc filesystem. The message format includes the data structure:
struct audit_contid_status {
       pid_t   pid;
       u64     id;
};

Quoted 6 lines

Signed-off-by Richard Guy Briggs <[email protected]>

---
 include/uapi/linux/audit.h |  2 ++
 kernel/audit.c             | 40 ++++++++++++++++++++++++++++++++++++++++
 kernel/audit.h             |  5 +++++
 3 files changed, 47 insertions(+)

I'm not a fan of having multiple interfaces to do one thing if it can be avoided. Presumably the argument for the netlink API is the container folks don't want to have to mount /proc inside containers which are going to host nested orchestrators? Can you reasonably run a fully fledged orchestrator without a valid /proc?

Paul Moore

maintainer

2019-10-11 00:40 UTC #44 lore raw

Re: [PATCH ghak90 V7 17/21] audit: add support for loginuid/sessionid set/get by netlink

Quoted 10 lines · Richard Guy Briggs

Add the ability to get and set the login uid and to get the session id using an audit netlink message using message types AUDIT_GET_LOGINUID 1024, AUDIT_SET_LOGINUID 1025 and AUDIT_GET_SESSIONID 1026 in addition to using the proc filesystem.
Signed-off-by Richard Guy Briggs <[email protected]>
---
 include/uapi/linux/audit.h |  3 +++
 kernel/audit.c             | 62 ++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 65 insertions(+)

This is completely independent of the audit container ID work, yes? If so, it shouldn't be part of this patchset.

--
paul moore
www.paul-moore.com

Paul Moore

maintainer

2019-10-11 00:40 UTC #46 lore raw

Re: [PATCH ghak90 V7 18/21] audit: track container nesting

Quoted 13 lines · Richard Guy Briggs

Track the parent container of a container to be able to filter and report nesting.
Now that we have a way to track and check the parent container of a container, fixup other patches, or squash all nesting fixes together.
fixup! audit: add container id
fixup! audit: log drop of contid on exit of last task
fixup! audit: log container info of syscalls
fixup! audit: add containerid filtering
fixup! audit: NETFILTER_PKT: record each container ID associated with a netNS
fixup! audit: convert to contid list to check for orch/engine ownership softirq (for netfilter) audit: protect contid list lock from softirq

Quoted 8 lines

Signed-off-by Richard Guy Briggs <[email protected]>

---
 include/linux/audit.h |  1 +
 kernel/audit.c        | 67 ++++++++++++++++++++++++++++++++++++++++++---------
 kernel/audit.h        |  3 +++
 kernel/auditfilter.c  | 20 ++++++++++++++-
 kernel/auditsc.c      |  2 +-
 5 files changed, 79 insertions(+), 14 deletions(-)

This is my last comment of the patchset because this is where it starts to get a little weird. I know we've talked about fixup! patches some in the past, but perhaps I didn't do a very good job communicating my poin; let me try again.

Submitting a fixup patch is okay if you've already posted a (lengthy) patchset and there was a small nit that someone uncovered that needed to be fixed prior to merging, assuming everyone (this includes the reviewer, the patch author, and the maintainer) is okay with the author posting the fix as fixup! patch then go for it. Done this way, fixup patches can save a lot of development, testing, and review time. However, in my opinion it is wrong to submit a patchset that has fixup patches as part of the original posting. In this case fixup patches have the opposite effect: the patchset becomes more complicated, reviews take longer, and the likelihood of missing important details increases.

When in doubt, don't submit separate fixup patches, fold them into the original patches instead.

--
paul moore
www.paul-moore.com

Richard Guy Briggs

author

2019-10-19 01:39 UTC #49 lore raw

Re: [PATCH ghak90 V7 20/21] audit: add capcontid to set contid outside init_user_ns

Quoted 11 lines · Richard Guy Briggs

Provide a mechanism similar to CAP_AUDIT_CONTROL to explicitly give a process in a non-init user namespace the capability to set audit container identifiers.
Use audit netlink message types AUDIT_GET_CAPCONTID 1027 and AUDIT_SET_CAPCONTID 1028. The message format includes the data structure:
struct audit_capcontid_status {
        pid_t   pid;
        u32     enable;
};

Paul, can I get a review of the general idea here to see if you're ok with this way of effectively extending CAP_AUDIT_CONTROL for the sake of setting contid from beyond the init user namespace where capable() can't reach and ns_capable() is meaningless for these purposes?

Last weekend was Canadian Thanksgiving where I took an extra day for an annual bike trip and I'm buried to my neck in a complete kitchen gut (down to 1920 structural double brick and knob/tube wiring), but I've got fixes or responses to almost everything else you've raised which I'll post shortly.

Thanks!

Quoted 216 lines

Signed-off-by Richard Guy Briggs <[email protected]>

---
 include/linux/audit.h      | 14 +++++++
 include/uapi/linux/audit.h |  2 +
 kernel/audit.c             | 98 +++++++++++++++++++++++++++++++++++++++++++++-
 kernel/audit.h             |  5 +++
 4 files changed, 117 insertions(+), 2 deletions(-)

include/linux/audit.h +14 -0

index 1ce27af686ea..dcc53e62e266 100644
@@ -117,6 +117,7 @@ struct audit_task_info {
 	kuid_t			loginuid;
 	unsigned int		sessionid;
 	struct audit_cont	*cont;
+	u32			capcontid;
 #ifdef CONFIG_AUDITSYSCALL
 	struct audit_context	*ctx;
 #endif

@@ -224,6 +225,14 @@ static inline unsigned int audit_get_sessionid(struct task_struct *tsk)
 	return tsk->audit->sessionid;
 }
 
+static inline u32 audit_get_capcontid(struct task_struct *tsk)
+{
+	if (!tsk->audit)
+		return 0;
+	return tsk->audit->capcontid;
+}
+
+extern int audit_set_capcontid(struct task_struct *tsk, u32 enable);
 extern int audit_set_contid(struct task_struct *tsk, u64 contid);
 
 static inline u64 audit_get_contid(struct task_struct *tsk)

@@ -309,6 +318,11 @@ static inline unsigned int audit_get_sessionid(struct task_struct *tsk)
 	return AUDIT_SID_UNSET;
 }
 
+static inline u32 audit_get_capcontid(struct task_struct *tsk)
+{
+	return 0;
+}
+
 static inline u64 audit_get_contid(struct task_struct *tsk)
 {
 	return AUDIT_CID_UNSET;

include/uapi/linux/audit.h +2 -0

index eef42c8eea77..011b0a8ee9b2 100644
@@ -78,6 +78,8 @@
 #define AUDIT_GET_LOGINUID	1024	/* Get loginuid of a task */
 #define AUDIT_SET_LOGINUID	1025	/* Set loginuid of a task */
 #define AUDIT_GET_SESSIONID	1026	/* Set sessionid of a task */
+#define AUDIT_GET_CAPCONTID	1027	/* Get cap_contid of a task */
+#define AUDIT_SET_CAPCONTID	1028	/* Set cap_contid of a task */
 
 #define AUDIT_FIRST_USER_MSG	1100	/* Userspace messages mostly uninteresting to kernel */
 #define AUDIT_USER_AVC		1107	/* We filter this differently */

kernel/audit.c +96 -2

index a70c9184e5d9..7160da464849 100644
@@ -1192,6 +1192,14 @@ static int audit_netlink_ok(struct sk_buff *skb, u16 msg_type)
 	case AUDIT_GET_SESSIONID:
 		return 0;
 		break;
+	case AUDIT_GET_CAPCONTID:
+	case AUDIT_SET_CAPCONTID:
+	case AUDIT_GET_CONTID:
+	case AUDIT_SET_CONTID:
+		if (!netlink_capable(skb, CAP_AUDIT_CONTROL) && !audit_get_capcontid(current))
+			return -EPERM;
+		return 0;
+		break;
 	default:  /* do more checks below */
 		break;
 	}

@@ -1227,8 +1235,6 @@ static int audit_netlink_ok(struct sk_buff *skb, u16 msg_type)
 	case AUDIT_TTY_SET:
 	case AUDIT_TRIM:
 	case AUDIT_MAKE_EQUIV:
-	case AUDIT_GET_CONTID:
-	case AUDIT_SET_CONTID:
 	case AUDIT_SET_LOGINUID:
 		/* Only support auditd and auditctl in initial pid namespace
 		 * for now. */

@@ -1304,6 +1310,23 @@ static int audit_get_contid_status(struct sk_buff *skb)
 	return 0;
 }
 
+static int audit_get_capcontid_status(struct sk_buff *skb)
+{
+	struct nlmsghdr *nlh = nlmsg_hdr(skb);
+	u32 seq = nlh->nlmsg_seq;
+	void *data = nlmsg_data(nlh);
+	struct audit_capcontid_status cs;
+
+	cs.pid = ((struct audit_capcontid_status *)data)->pid;
+	if (!cs.pid)
+		cs.pid = task_tgid_nr(current);
+	rcu_read_lock();
+	cs.enable = audit_get_capcontid(find_task_by_vpid(cs.pid));
+	rcu_read_unlock();
+	audit_send_reply(skb, seq, AUDIT_GET_CAPCONTID, 0, 0, &cs, sizeof(cs));
+	return 0;
+}
+
 struct audit_loginuid_status { uid_t loginuid; };
 
 static int audit_get_loginuid_status(struct sk_buff *skb)

@@ -1779,6 +1802,27 @@ static int audit_receive_msg(struct sk_buff *skb, struct nlmsghdr *nlh)
 		if (err)
 			return err;
 		break;
+	case AUDIT_SET_CAPCONTID: {
+		struct audit_capcontid_status *s = data;
+		struct task_struct *tsk;
+
+		/* check if new data is valid */
+		if (nlmsg_len(nlh) < sizeof(*s))
+			return -EINVAL;
+		tsk = find_get_task_by_vpid(s->pid);
+		if (!tsk)
+			return -EINVAL;
+
+		err = audit_set_capcontid(tsk, s->enable);
+		put_task_struct(tsk);
+		return err;
+		break;
+	}
+	case AUDIT_GET_CAPCONTID:
+		err = audit_get_capcontid_status(skb);
+		if (err)
+			return err;
+		break;
 	case AUDIT_SET_LOGINUID: {
 		uid_t *loginuid = data;
 		kuid_t kloginuid;

@@ -2711,6 +2755,56 @@ static struct task_struct *audit_cont_owner(struct task_struct *tsk)
 	return NULL;
 }
 
+int audit_set_capcontid(struct task_struct *task, u32 enable)
+{
+	u32 oldcapcontid;
+	int rc = 0;
+	struct audit_buffer *ab;
+	uid_t uid;
+	struct tty_struct *tty;
+	char comm[sizeof(current->comm)];
+
+	if (!task->audit)
+		return -ENOPROTOOPT;
+	oldcapcontid = audit_get_capcontid(task);
+	/* if task is not descendant, block */
+	if (task == current)
+		rc = -EBADSLT;
+	else if (!task_is_descendant(current, task))
+		rc = -EXDEV;
+	else if (current_user_ns() == &init_user_ns) {
+		if (!capable(CAP_AUDIT_CONTROL) && !audit_get_capcontid(current))
+			rc = -EPERM;
+	}
+	if (!rc)
+		task->audit->capcontid = enable;
+
+	if (!audit_enabled)
+		return rc;
+
+	ab = audit_log_start(audit_context(), GFP_KERNEL, AUDIT_SET_CAPCONTID);
+	if (!ab)
+		return rc;
+
+	uid = from_kuid(&init_user_ns, task_uid(current));
+	tty = audit_get_tty();
+	audit_log_format(ab,
+			 "opid=%d capcontid=%u old-capcontid=%u pid=%d uid=%u auid=%u tty=%s ses=%u",
+			 task_tgid_nr(task), enable, oldcapcontid,
+			 task_tgid_nr(current), uid,
+			 from_kuid(&init_user_ns, audit_get_loginuid(current)),
+			 tty ? tty_name(tty) : "(none)",
+			 audit_get_sessionid(current));
+	audit_put_tty(tty);
+	audit_log_task_context(ab);
+	audit_log_format(ab, " comm=");
+	audit_log_untrustedstring(ab, get_task_comm(comm, current));
+	audit_log_d_path_exe(ab, current->mm);
+	audit_log_format(ab, " res=%d", !rc);
+	audit_log_end(ab);
+	return rc;
+}
+
 /*
  * audit_set_contid - set current task's audit contid
  * @task: target task

kernel/audit.h +5 -0

index cb25341c1a0f..ac4694e88485 100644
@@ -231,6 +231,11 @@ struct audit_contid_status {
 	u64	id;
 };
 
+struct audit_capcontid_status {
+	pid_t	pid;
+	u32	enable;
+};
+
 #define AUDIT_CONTID_DEPTH	5
 
 /* Indicates that audit should log the full pathname. */

Signature · 1 line

1.8.3.1

- RGB

--
Richard Guy Briggs <[email protected]>
Sr. S/W Engineer, Kernel Security, Base Operating Systems Remote, Ottawa, Red Hat Canada
IRC: rgb, SunRaycer
Voice: +1.647.777.2635, Internal: (81) 32635

Paul Moore

maintainer

2019-10-21 19:53 UTC #50 lore raw

Re: [PATCH ghak90 V7 20/21] audit: add capcontid to set contid outside init_user_ns

Quoted 13 lines · Richard Guy Briggs

Quoted 11 lines · Richard Guy Briggs
Provide a mechanism similar to CAP_AUDIT_CONTROL to explicitly give a process in a non-init user namespace the capability to set audit container identifiers.
Use audit netlink message types AUDIT_GET_CAPCONTID 1027 and AUDIT_SET_CAPCONTID 1028. The message format includes the data structure:
struct audit_capcontid_status {
        pid_t   pid;
        u32     enable;
};

Quoted 4 lines

Paul, can I get a review of the general idea here to see if you're ok with this way of effectively extending CAP_AUDIT_CONTROL for the sake of setting contid from beyond the init user namespace where capable() can't reach and ns_capable() is meaningless for these purposes?

I think my previous comment about having both the procfs and netlink interfaces apply here. I don't see why we need two different APIs at the start; explain to me why procfs isn't sufficient. If the argument is simply the desire to avoid mounting procfs in the container, how many container orchestrators can function today without a valid /proc?

Signature · 2 lines

paul moore
www.paul-moore.com

Richard Guy Briggs

author

2019-10-21 21:38 UTC #51 lore raw

Re: [PATCH ghak90 V7 20/21] audit: add capcontid to set contid outside init_user_ns

Quoted 19 lines · Paul Moore

Quoted 13 lines · Richard Guy Briggs
Quoted 11 lines · Richard Guy Briggs
Provide a mechanism similar to CAP_AUDIT_CONTROL to explicitly give a process in a non-init user namespace the capability to set audit container identifiers.
Use audit netlink message types AUDIT_GET_CAPCONTID 1027 and AUDIT_SET_CAPCONTID 1028. The message format includes the data structure:
struct audit_capcontid_status {
        pid_t   pid;
        u32     enable;
};
Quoted 4 lines
Paul, can I get a review of the general idea here to see if you're ok with this way of effectively extending CAP_AUDIT_CONTROL for the sake of setting contid from beyond the init user namespace where capable() can't reach and ns_capable() is meaningless for these purposes?

Quoted 5 lines

I think my previous comment about having both the procfs and netlink interfaces apply here. I don't see why we need two different APIs at the start; explain to me why procfs isn't sufficient. If the argument is simply the desire to avoid mounting procfs in the container, how many container orchestrators can function today without a valid /proc?

Ok, sorry, I meant to address that question from a previous patch comment at the same time.

It was raised by Eric Biederman that the proc filesystem interface for audit had its limitations and he had suggested an audit netlink interface made more sense.

The intent was to switch to the audit netlink interface for contid, capcontid and to add the audit netlink interface for loginuid and sessionid while deprecating the proc interface for loginuid and sessionid. This was alluded to in the cover letter, but not very clear, I'm afraid. I have patches to remove the contid and loginuid/sessionid interfaces in another tree which is why I had forgotten to outline that plan more explicitly in the cover letter.

Quoted 1 line

paul moore

- RGB

Paul Moore

maintainer

2019-10-21 21:43 UTC #52 lore raw

Re: [PATCH ghak90 V7 20/21] audit: add capcontid to set contid outside init_user_ns

Quoted 26 lines · Richard Guy Briggs

Quoted 19 lines · Paul Moore
Quoted 13 lines · Richard Guy Briggs
Quoted 11 lines · Richard Guy Briggs
Provide a mechanism similar to CAP_AUDIT_CONTROL to explicitly give a process in a non-init user namespace the capability to set audit container identifiers.
Use audit netlink message types AUDIT_GET_CAPCONTID 1027 and AUDIT_SET_CAPCONTID 1028. The message format includes the data structure:
struct audit_capcontid_status {
        pid_t   pid;
        u32     enable;
};
Quoted 4 lines
Paul, can I get a review of the general idea here to see if you're ok with this way of effectively extending CAP_AUDIT_CONTROL for the sake of setting contid from beyond the init user namespace where capable() can't reach and ns_capable() is meaningless for these purposes?
Quoted 5 lines
I think my previous comment about having both the procfs and netlink interfaces apply here. I don't see why we need two different APIs at the start; explain to me why procfs isn't sufficient. If the argument is simply the desire to avoid mounting procfs in the container, how many container orchestrators can function today without a valid /proc?

Quoted 6 lines

Ok, sorry, I meant to address that question from a previous patch comment at the same time.
It was raised by Eric Biederman that the proc filesystem interface for audit had its limitations and he had suggested an audit netlink interface made more sense.

I'm sure you've got it handy, so I'm going to be lazy and ask: archive pointer to Eric's comments? Just a heads-up, I'm really *not* a fan of using the netlink interface for this, so unless Eric presents a super compelling reason for why we shouldn't use procfs I'm inclined to stick with /proc.

Quoted 7 lines

The intent was to switch to the audit netlink interface for contid, capcontid and to add the audit netlink interface for loginuid and sessionid while deprecating the proc interface for loginuid and sessionid. This was alluded to in the cover letter, but not very clear, I'm afraid. I have patches to remove the contid and loginuid/sessionid interfaces in another tree which is why I had forgotten to outline that plan more explicitly in the cover letter.

Signature · 2 lines

paul moore
www.paul-moore.com

Richard Guy Briggs

author

2019-10-21 23:57 UTC #53 lore raw

Re: [PATCH ghak90 V7 20/21] audit: add capcontid to set contid outside init_user_ns

Quoted 34 lines · Paul Moore

Quoted 26 lines · Richard Guy Briggs
Quoted 19 lines · Paul Moore
Quoted 13 lines · Richard Guy Briggs
Quoted 11 lines · Richard Guy Briggs
Provide a mechanism similar to CAP_AUDIT_CONTROL to explicitly give a process in a non-init user namespace the capability to set audit container identifiers.
Use audit netlink message types AUDIT_GET_CAPCONTID 1027 and AUDIT_SET_CAPCONTID 1028. The message format includes the data structure:
struct audit_capcontid_status {
        pid_t   pid;
        u32     enable;
};
Quoted 4 lines
Paul, can I get a review of the general idea here to see if you're ok with this way of effectively extending CAP_AUDIT_CONTROL for the sake of setting contid from beyond the init user namespace where capable() can't reach and ns_capable() is meaningless for these purposes?
Quoted 5 lines
I think my previous comment about having both the procfs and netlink interfaces apply here. I don't see why we need two different APIs at the start; explain to me why procfs isn't sufficient. If the argument is simply the desire to avoid mounting procfs in the container, how many container orchestrators can function today without a valid /proc?
Quoted 6 lines
Ok, sorry, I meant to address that question from a previous patch comment at the same time.
It was raised by Eric Biederman that the proc filesystem interface for audit had its limitations and he had suggested an audit netlink interface made more sense.

Quoted 5 lines

I'm sure you've got it handy, so I'm going to be lazy and ask: archive pointer to Eric's comments? Just a heads-up, I'm really *not* a fan of using the netlink interface for this, so unless Eric presents a super compelling reason for why we shouldn't use procfs I'm inclined to stick with /proc.

It was actually a video call with Eric and Steve where that was recommended, so I can't provide you with any first-hand communication about it. I'll get more details...

So, with that out of the way, could you please comment on the general idea of what was intended to be the central idea of this mechanism to be able to nest containers beyond the initial user namespace (knowing that a /proc interface is available and the audit netlink interface isn't necessary for it to work and the latter can be easily removed)?

Quoted 9 lines

Quoted 7 lines
The intent was to switch to the audit netlink interface for contid, capcontid and to add the audit netlink interface for loginuid and sessionid while deprecating the proc interface for loginuid and sessionid. This was alluded to in the cover letter, but not very clear, I'm afraid. I have patches to remove the contid and loginuid/sessionid interfaces in another tree which is why I had forgotten to outline that plan more explicitly in the cover letter.
paul moore

- RGB

Paul Moore

maintainer

2019-10-22 00:31 UTC #54 lore raw

Re: [PATCH ghak90 V7 20/21] audit: add capcontid to set contid outside init_user_ns

Quoted 41 lines · Richard Guy Briggs

Quoted 34 lines · Paul Moore
Quoted 26 lines · Richard Guy Briggs
Quoted 19 lines · Paul Moore
Quoted 13 lines · Richard Guy Briggs
Quoted 11 lines · Richard Guy Briggs
Provide a mechanism similar to CAP_AUDIT_CONTROL to explicitly give a process in a non-init user namespace the capability to set audit container identifiers.
Use audit netlink message types AUDIT_GET_CAPCONTID 1027 and AUDIT_SET_CAPCONTID 1028. The message format includes the data structure:
struct audit_capcontid_status {
        pid_t   pid;
        u32     enable;
};
Quoted 4 lines
Paul, can I get a review of the general idea here to see if you're ok with this way of effectively extending CAP_AUDIT_CONTROL for the sake of setting contid from beyond the init user namespace where capable() can't reach and ns_capable() is meaningless for these purposes?
Quoted 5 lines
I think my previous comment about having both the procfs and netlink interfaces apply here. I don't see why we need two different APIs at the start; explain to me why procfs isn't sufficient. If the argument is simply the desire to avoid mounting procfs in the container, how many container orchestrators can function today without a valid /proc?
Quoted 6 lines
Ok, sorry, I meant to address that question from a previous patch comment at the same time.
It was raised by Eric Biederman that the proc filesystem interface for audit had its limitations and he had suggested an audit netlink interface made more sense.
Quoted 5 lines
I'm sure you've got it handy, so I'm going to be lazy and ask: archive pointer to Eric's comments? Just a heads-up, I'm really *not* a fan of using the netlink interface for this, so unless Eric presents a super compelling reason for why we shouldn't use procfs I'm inclined to stick with /proc.

Quoted 3 lines

It was actually a video call with Eric and Steve where that was recommended, so I can't provide you with any first-hand communication about it. I'll get more details...

Yeah, that sort of information really needs to be on the list.

Quoted 5 lines

So, with that out of the way, could you please comment on the general idea of what was intended to be the central idea of this mechanism to be able to nest containers beyond the initial user namespace (knowing that a /proc interface is available and the audit netlink interface isn't necessary for it to work and the latter can be easily removed)?

I'm not entirely clear what you are asking about, are you asking why I care about nesting container orchestrators? Simply put, it is not uncommon for the LXC/LXD folks to see nested container orchestrators, so I felt it was important to support that use case. When we originally started this effort we probably should have done a better job reaching out to the LXC/LXD folks, we may have caught this earlier. Regardless, we caught it, and it looks like we are on our way to supporting it (that's good).

Are you asking why I prefer the procfs approach to setting/getting the audit container ID? For one, it makes it easier for a LSM to enforce the audit container ID operations independent of the other audit control APIs. It also provides a simpler interface for container orchestrators. Both seem like desirable traits as far as I'm concerned.

Quoted 7 lines

Quoted 7 lines
Quoted 7 lines
The intent was to switch to the audit netlink interface for contid, capcontid and to add the audit netlink interface for loginuid and sessionid while deprecating the proc interface for loginuid and sessionid. This was alluded to in the cover letter, but not very clear, I'm afraid. I have patches to remove the contid and loginuid/sessionid interfaces in another tree which is why I had forgotten to outline that plan more explicitly in the cover letter.

Signature · 2 lines

paul moore
www.paul-moore.com

Neil Horman

2019-10-22 12:13 UTC #55 lore raw

Re: [PATCH ghak90 V7 20/21] audit: add capcontid to set contid outside init_user_ns

Quoted 63 lines · Paul Moore

Quoted 41 lines · Richard Guy Briggs
Quoted 34 lines · Paul Moore
Quoted 26 lines · Richard Guy Briggs
Quoted 19 lines · Paul Moore
Quoted 13 lines · Richard Guy Briggs
Quoted 11 lines · Richard Guy Briggs
Provide a mechanism similar to CAP_AUDIT_CONTROL to explicitly give a process in a non-init user namespace the capability to set audit container identifiers.
Use audit netlink message types AUDIT_GET_CAPCONTID 1027 and AUDIT_SET_CAPCONTID 1028. The message format includes the data structure:
struct audit_capcontid_status {
        pid_t   pid;
        u32     enable;
};
Quoted 4 lines
Paul, can I get a review of the general idea here to see if you're ok with this way of effectively extending CAP_AUDIT_CONTROL for the sake of setting contid from beyond the init user namespace where capable() can't reach and ns_capable() is meaningless for these purposes?
Quoted 5 lines
I think my previous comment about having both the procfs and netlink interfaces apply here. I don't see why we need two different APIs at the start; explain to me why procfs isn't sufficient. If the argument is simply the desire to avoid mounting procfs in the container, how many container orchestrators can function today without a valid /proc?
Quoted 6 lines
Ok, sorry, I meant to address that question from a previous patch comment at the same time.
It was raised by Eric Biederman that the proc filesystem interface for audit had its limitations and he had suggested an audit netlink interface made more sense.
Quoted 5 lines
I'm sure you've got it handy, so I'm going to be lazy and ask: archive pointer to Eric's comments? Just a heads-up, I'm really *not* a fan of using the netlink interface for this, so unless Eric presents a super compelling reason for why we shouldn't use procfs I'm inclined to stick with /proc.
Quoted 3 lines
It was actually a video call with Eric and Steve where that was recommended, so I can't provide you with any first-hand communication about it. I'll get more details...
Yeah, that sort of information really needs to be on the list.
Quoted 5 lines
So, with that out of the way, could you please comment on the general idea of what was intended to be the central idea of this mechanism to be able to nest containers beyond the initial user namespace (knowing that a /proc interface is available and the audit netlink interface isn't necessary for it to work and the latter can be easily removed)?
I'm not entirely clear what you are asking about, are you asking why I care about nesting container orchestrators? Simply put, it is not uncommon for the LXC/LXD folks to see nested container orchestrators, so I felt it was important to support that use case. When we originally started this effort we probably should have done a better job reaching out to the LXC/LXD folks, we may have caught this earlier. Regardless, we caught it, and it looks like we are on our way to supporting it (that's good).

Quoted 7 lines

Are you asking why I prefer the procfs approach to setting/getting the audit container ID? For one, it makes it easier for a LSM to enforce the audit container ID operations independent of the other audit control APIs. It also provides a simpler interface for container orchestrators. Both seem like desirable traits as far as I'm concerned.

I agree that one api is probably the best approach here, but I actually think that the netlink interface is the more flexible approach. Its a little more work for userspace (you have to marshal your data into a netlink message before sending it, and wait for an async response), but thats a well known pattern, and it provides significantly more flexibility for the kernel. LSM already has a hook to audit netlink messages in sock_sendmsg, so thats not a problem, and if you use netlink, you get the advantage of being able to broadcast messages within your network namespaces, facilitating any needed orchestrator co-ordination. To do the same thing with a filesystem api, you need to use the fanotify api, which IIRC doesn't work on proc.

Neil

Quoted 12 lines

Quoted 7 lines
Quoted 7 lines
Quoted 7 lines
The intent was to switch to the audit netlink interface for contid, capcontid and to add the audit netlink interface for loginuid and sessionid while deprecating the proc interface for loginuid and sessionid. This was alluded to in the cover letter, but not very clear, I'm afraid. I have patches to remove the contid and loginuid/sessionid interfaces in another tree which is why I had forgotten to outline that plan more explicitly in the cover letter.
Signature · 2 lines
paul moore
www.paul-moore.com

Paul Moore

maintainer

2019-10-22 14:04 UTC #56 lore raw

Re: [PATCH ghak90 V7 20/21] audit: add capcontid to set contid outside init_user_ns

Quoted 71 lines · Neil Horman

Quoted 63 lines · Paul Moore
Quoted 41 lines · Richard Guy Briggs
Quoted 34 lines · Paul Moore
Quoted 26 lines · Richard Guy Briggs
Quoted 19 lines · Paul Moore
Quoted 13 lines · Richard Guy Briggs
Quoted 11 lines · Richard Guy Briggs
Provide a mechanism similar to CAP_AUDIT_CONTROL to explicitly give a process in a non-init user namespace the capability to set audit container identifiers.
Use audit netlink message types AUDIT_GET_CAPCONTID 1027 and AUDIT_SET_CAPCONTID 1028. The message format includes the data structure:
struct audit_capcontid_status {
        pid_t   pid;
        u32     enable;
};
Quoted 4 lines
Paul, can I get a review of the general idea here to see if you're ok with this way of effectively extending CAP_AUDIT_CONTROL for the sake of setting contid from beyond the init user namespace where capable() can't reach and ns_capable() is meaningless for these purposes?
Quoted 5 lines
I think my previous comment about having both the procfs and netlink interfaces apply here. I don't see why we need two different APIs at the start; explain to me why procfs isn't sufficient. If the argument is simply the desire to avoid mounting procfs in the container, how many container orchestrators can function today without a valid /proc?
Quoted 6 lines
Ok, sorry, I meant to address that question from a previous patch comment at the same time.
It was raised by Eric Biederman that the proc filesystem interface for audit had its limitations and he had suggested an audit netlink interface made more sense.
Quoted 5 lines
I'm sure you've got it handy, so I'm going to be lazy and ask: archive pointer to Eric's comments? Just a heads-up, I'm really *not* a fan of using the netlink interface for this, so unless Eric presents a super compelling reason for why we shouldn't use procfs I'm inclined to stick with /proc.
Quoted 3 lines
It was actually a video call with Eric and Steve where that was recommended, so I can't provide you with any first-hand communication about it. I'll get more details...
Yeah, that sort of information really needs to be on the list.
Quoted 5 lines
So, with that out of the way, could you please comment on the general idea of what was intended to be the central idea of this mechanism to be able to nest containers beyond the initial user namespace (knowing that a /proc interface is available and the audit netlink interface isn't necessary for it to work and the latter can be easily removed)?
I'm not entirely clear what you are asking about, are you asking why I care about nesting container orchestrators? Simply put, it is not uncommon for the LXC/LXD folks to see nested container orchestrators, so I felt it was important to support that use case. When we originally started this effort we probably should have done a better job reaching out to the LXC/LXD folks, we may have caught this earlier. Regardless, we caught it, and it looks like we are on our way to supporting it (that's good).
Quoted 7 lines
Are you asking why I prefer the procfs approach to setting/getting the audit container ID? For one, it makes it easier for a LSM to enforce the audit container ID operations independent of the other audit control APIs. It also provides a simpler interface for container orchestrators. Both seem like desirable traits as far as I'm concerned.

Quoted 7 lines

I agree that one api is probably the best approach here, but I actually think that the netlink interface is the more flexible approach. Its a little more work for userspace (you have to marshal your data into a netlink message before sending it, and wait for an async response), but thats a well known pattern, and it provides significantly more flexibility for the kernel. LSM already has a hook to audit netlink messages in sock_sendmsg, so thats not a problem ...

Look closely at how the LSM controls for netlink work and you'll see a number of problems; basically command level granularity it hard. On the other hand, per-file granularity it easy.

Quoted 4 lines

... and if you use
netlink, you get the advantage of being able to broadcast messages within your network namespaces, facilitating any needed orchestrator co-ordination.

Please don't read this comment as support of the netlink approach, but I don't think we want to use the multicast netlink; we would want it to be more of client/server model so that we could enforce access controls a bit easier. Besides, is this even a use case?

Quoted 2 lines

To do the same thing with a filesystem api, you need to use the fanotify api, which IIRC doesn't work on proc.

Once again, I'm not sure this is a problem we are trying to solve (broadcasting audit container ID across multiple tasks), is it? Access to the audit container ID in userspace is something I've always thought needs to be tightly controlled to prevent abuse.

Signature · 2 lines

paul moore
www.paul-moore.com

Richard Guy Briggs

author

2019-10-22 14:27 UTC #58 lore raw

Re: [PATCH ghak90 V7 20/21] audit: add capcontid to set contid outside init_user_ns

Quoted 54 lines · Paul Moore

Quoted 41 lines · Richard Guy Briggs
Quoted 34 lines · Paul Moore
Quoted 26 lines · Richard Guy Briggs
Quoted 19 lines · Paul Moore
Quoted 13 lines · Richard Guy Briggs
Quoted 11 lines · Richard Guy Briggs
Provide a mechanism similar to CAP_AUDIT_CONTROL to explicitly give a process in a non-init user namespace the capability to set audit container identifiers.
Use audit netlink message types AUDIT_GET_CAPCONTID 1027 and AUDIT_SET_CAPCONTID 1028. The message format includes the data structure:
struct audit_capcontid_status {
        pid_t   pid;
        u32     enable;
};
Quoted 4 lines
Paul, can I get a review of the general idea here to see if you're ok with this way of effectively extending CAP_AUDIT_CONTROL for the sake of setting contid from beyond the init user namespace where capable() can't reach and ns_capable() is meaningless for these purposes?
Quoted 5 lines
I think my previous comment about having both the procfs and netlink interfaces apply here. I don't see why we need two different APIs at the start; explain to me why procfs isn't sufficient. If the argument is simply the desire to avoid mounting procfs in the container, how many container orchestrators can function today without a valid /proc?
Quoted 6 lines
Ok, sorry, I meant to address that question from a previous patch comment at the same time.
It was raised by Eric Biederman that the proc filesystem interface for audit had its limitations and he had suggested an audit netlink interface made more sense.
Quoted 5 lines
I'm sure you've got it handy, so I'm going to be lazy and ask: archive pointer to Eric's comments? Just a heads-up, I'm really *not* a fan of using the netlink interface for this, so unless Eric presents a super compelling reason for why we shouldn't use procfs I'm inclined to stick with /proc.
Quoted 3 lines
It was actually a video call with Eric and Steve where that was recommended, so I can't provide you with any first-hand communication about it. I'll get more details...
Yeah, that sort of information really needs to be on the list.
Quoted 5 lines
So, with that out of the way, could you please comment on the general idea of what was intended to be the central idea of this mechanism to be able to nest containers beyond the initial user namespace (knowing that a /proc interface is available and the audit netlink interface isn't necessary for it to work and the latter can be easily removed)?

Quoted 8 lines

I'm not entirely clear what you are asking about, are you asking why I care about nesting container orchestrators? Simply put, it is not uncommon for the LXC/LXD folks to see nested container orchestrators, so I felt it was important to support that use case. When we originally started this effort we probably should have done a better job reaching out to the LXC/LXD folks, we may have caught this earlier. Regardless, we caught it, and it looks like we are on our way to supporting it (that's good).

I'm not asking why you care about container orchestrators.

Quoted 6 lines

Are you asking why I prefer the procfs approach to setting/getting the audit container ID? For one, it makes it easier for a LSM to enforce the audit container ID operations independent of the other audit control APIs. It also provides a simpler interface for container orchestrators. Both seem like desirable traits as far as I'm concerned.

I'd like to leave the proc/netlink decision/debate out of this discussion, though it does need to happen and I was hoping that would happen on the loginuid/sessionid proc/netlink patch thread.

I'd like your perspective on how the capcontid feature was implemented (aside from the proc/netlink api issue which was intended to be consistent across loginuid/sessionid/contid/capcontid). Do you see this feature as potentially solving the nested container issue in child user namespaces?

Quoted 9 lines

Quoted 7 lines
Quoted 7 lines
Quoted 7 lines
The intent was to switch to the audit netlink interface for contid, capcontid and to add the audit netlink interface for loginuid and sessionid while deprecating the proc interface for loginuid and sessionid. This was alluded to in the cover letter, but not very clear, I'm afraid. I have patches to remove the contid and loginuid/sessionid interfaces in another tree which is why I had forgotten to outline that plan more explicitly in the cover letter.
paul moore

- RGB

Paul Moore

maintainer

2019-10-22 14:34 UTC #59 lore raw

Re: [PATCH ghak90 V7 20/21] audit: add capcontid to set contid outside init_user_ns

Quoted 5 lines · Richard Guy Briggs

I'd like your perspective on how the capcontid feature was implemented (aside from the proc/netlink api issue which was intended to be consistent across loginuid/sessionid/contid/capcontid). Do you see this feature as potentially solving the nested container issue in child user namespaces?

The patchset is a bit messy at this point in the stack due to the "fixup!" confusion and a few other things which I already mentioned so I don't really want to comment too much on that until I can see everything in a reasonable patch stack. Let's leave that for the next draft.

Signature · 2 lines

paul moore
www.paul-moore.com

Richard Guy Briggs

author

2019-10-22 20:06 UTC #57 lore raw

Re: [PATCH ghak90 V7 20/21] audit: add capcontid to set contid outside init_user_ns

Quoted 71 lines · Neil Horman

Quoted 63 lines · Paul Moore
Quoted 41 lines · Richard Guy Briggs
Quoted 34 lines · Paul Moore
Quoted 26 lines · Richard Guy Briggs
Quoted 19 lines · Paul Moore
Quoted 13 lines · Richard Guy Briggs
Quoted 11 lines · Richard Guy Briggs
Provide a mechanism similar to CAP_AUDIT_CONTROL to explicitly give a process in a non-init user namespace the capability to set audit container identifiers.
Use audit netlink message types AUDIT_GET_CAPCONTID 1027 and AUDIT_SET_CAPCONTID 1028. The message format includes the data structure:
struct audit_capcontid_status {
        pid_t   pid;
        u32     enable;
};
Quoted 4 lines
Paul, can I get a review of the general idea here to see if you're ok with this way of effectively extending CAP_AUDIT_CONTROL for the sake of setting contid from beyond the init user namespace where capable() can't reach and ns_capable() is meaningless for these purposes?
Quoted 5 lines
I think my previous comment about having both the procfs and netlink interfaces apply here. I don't see why we need two different APIs at the start; explain to me why procfs isn't sufficient. If the argument is simply the desire to avoid mounting procfs in the container, how many container orchestrators can function today without a valid /proc?
Quoted 6 lines
Ok, sorry, I meant to address that question from a previous patch comment at the same time.
It was raised by Eric Biederman that the proc filesystem interface for audit had its limitations and he had suggested an audit netlink interface made more sense.
Quoted 5 lines
I'm sure you've got it handy, so I'm going to be lazy and ask: archive pointer to Eric's comments? Just a heads-up, I'm really *not* a fan of using the netlink interface for this, so unless Eric presents a super compelling reason for why we shouldn't use procfs I'm inclined to stick with /proc.
Quoted 3 lines
It was actually a video call with Eric and Steve where that was recommended, so I can't provide you with any first-hand communication about it. I'll get more details...
Yeah, that sort of information really needs to be on the list.
Quoted 5 lines
So, with that out of the way, could you please comment on the general idea of what was intended to be the central idea of this mechanism to be able to nest containers beyond the initial user namespace (knowing that a /proc interface is available and the audit netlink interface isn't necessary for it to work and the latter can be easily removed)?
I'm not entirely clear what you are asking about, are you asking why I care about nesting container orchestrators? Simply put, it is not uncommon for the LXC/LXD folks to see nested container orchestrators, so I felt it was important to support that use case. When we originally started this effort we probably should have done a better job reaching out to the LXC/LXD folks, we may have caught this earlier. Regardless, we caught it, and it looks like we are on our way to supporting it (that's good).
Quoted 6 lines
Are you asking why I prefer the procfs approach to setting/getting the audit container ID? For one, it makes it easier for a LSM to enforce the audit container ID operations independent of the other audit control APIs. It also provides a simpler interface for container orchestrators. Both seem like desirable traits as far as I'm concerned.

Quoted 11 lines

I agree that one api is probably the best approach here, but I actually think that the netlink interface is the more flexible approach. Its a little more work for userspace (you have to marshal your data into a netlink message before sending it, and wait for an async response), but thats a well known pattern, and it provides significantly more flexibility for the kernel. LSM already has a hook to audit netlink messages in sock_sendmsg, so thats not a problem, and if you use netlink, you get the advantage of being able to broadcast messages within your network namespaces, facilitating any needed orchestrator co-ordination. To do the same thing with a filesystem api, you need to use the fanotify api, which IIRC doesn't work on proc.

One api was the intent, deprecating proc for loginuid and sessionid if netlink was the chosen way to go.

I don't think we had discussed the possibility or need to use netlink multicast for this purpose and see it as a liability to limiting access to only those processes that need it.

Quoted 11 lines

Neil
Quoted 9 lines
Quoted 7 lines
Quoted 7 lines
Quoted 7 lines
The intent was to switch to the audit netlink interface for contid, capcontid and to add the audit netlink interface for loginuid and sessionid while deprecating the proc interface for loginuid and sessionid. This was alluded to in the cover letter, but not very clear, I'm afraid. I have patches to remove the contid and loginuid/sessionid interfaces in another tree which is why I had forgotten to outline that plan more explicitly in the cover letter.
paul moore

- RGB

Richard Guy Briggs

author

2019-10-24 21:00 UTC #60 lore raw

Re: [PATCH ghak90 V7 20/21] audit: add capcontid to set contid outside init_user_ns

Quoted 42 lines · Paul Moore

Quoted 41 lines · Richard Guy Briggs
Quoted 39 lines · Paul Moore
Quoted 26 lines · Richard Guy Briggs
Quoted 19 lines · Paul Moore
Quoted 13 lines · Richard Guy Briggs
Quoted 11 lines · Richard Guy Briggs
Provide a mechanism similar to CAP_AUDIT_CONTROL to explicitly give a process in a non-init user namespace the capability to set audit container identifiers.
Use audit netlink message types AUDIT_GET_CAPCONTID 1027 and AUDIT_SET_CAPCONTID 1028. The message format includes the data structure:
struct audit_capcontid_status {
        pid_t   pid;
        u32     enable;
};
Quoted 4 lines
Paul, can I get a review of the general idea here to see if you're ok with this way of effectively extending CAP_AUDIT_CONTROL for the sake of setting contid from beyond the init user namespace where capable() can't reach and ns_capable() is meaningless for these purposes?
Quoted 5 lines
I think my previous comment about having both the procfs and netlink interfaces apply here. I don't see why we need two different APIs at the start; explain to me why procfs isn't sufficient. If the argument is simply the desire to avoid mounting procfs in the container, how many container orchestrators can function today without a valid /proc?
Quoted 6 lines
Ok, sorry, I meant to address that question from a previous patch comment at the same time.
It was raised by Eric Biederman that the proc filesystem interface for audit had its limitations and he had suggested an audit netlink interface made more sense.
I'm sure you've got it handy, so I'm going to be lazy and ask: archive pointer to Eric's comments? Just a heads-up, I'm really *not* a fan of using the netlink interface for this, so unless Eric presents a super compelling reason for why we shouldn't use procfs I'm inclined to stick with /proc.

Quoted 5 lines

Quoted 3 lines
It was actually a video call with Eric and Steve where that was recommended, so I can't provide you with any first-hand communication about it. I'll get more details...
Yeah, that sort of information really needs to be on the list.

Here's the note I had from that meeting:

- Eric raised the issue that using /proc is likely to get more and more hoary due to mount namespaces and suggested that we use a netlink
audit message (or a new syscall) to set the audit container identifier and since the loginuid is a similar type of operation, that it should be migrated over to a similar mechanism to get it away from /proc. Get could be done with a netlink audit message that triggers an audit log message to deliver the information. I'm reluctant to further pollute the syscall space if we can find another method. The netlink audit message makes sense since any audit-enabled service is likely to already have an audit socket open.

I don't have more detailed notes about what Eric said specifically.

Quoted 22 lines

Quoted 5 lines
So, with that out of the way, could you please comment on the general idea of what was intended to be the central idea of this mechanism to be able to nest containers beyond the initial user namespace (knowing that a /proc interface is available and the audit netlink interface isn't necessary for it to work and the latter can be easily removed)?
I'm not entirely clear what you are asking about, are you asking why I care about nesting container orchestrators? Simply put, it is not uncommon for the LXC/LXD folks to see nested container orchestrators, so I felt it was important to support that use case. When we originally started this effort we probably should have done a better job reaching out to the LXC/LXD folks, we may have caught this earlier. Regardless, we caught it, and it looks like we are on our way to supporting it (that's good).
Are you asking why I prefer the procfs approach to setting/getting the audit container ID? For one, it makes it easier for a LSM to enforce the audit container ID operations independent of the other audit control APIs. It also provides a simpler interface for container orchestrators. Both seem like desirable traits as far as I'm concerned.

Quoted 11 lines

Quoted 7 lines
Quoted 7 lines
Quoted 7 lines
The intent was to switch to the audit netlink interface for contid, capcontid and to add the audit netlink interface for loginuid and sessionid while deprecating the proc interface for loginuid and sessionid. This was alluded to in the cover letter, but not very clear, I'm afraid. I have patches to remove the contid and loginuid/sessionid interfaces in another tree which is why I had forgotten to outline that plan more explicitly in the cover letter.
Signature · 2 lines
paul moore
www.paul-moore.com

- RGB

--
Richard Guy Briggs <[email protected]>
Sr. S/W Engineer, Kernel Security, Base Operating Systems Remote, Ottawa, Red Hat Canada
IRC: rgb, SunRaycer
Voice: +1.647.777.2635, Internal: (81) 32635

Richard Guy Briggs

author

2019-10-24 21:23 UTC #17 lore raw

Re: [PATCH ghak90 V7 06/21] audit: contid limit of 32k imposed to avoid DoS

Quoted 47 lines · Paul Moore

Quoted 14 lines · Neil Horman
Quoted 13 lines · Richard Guy Briggs
Set an arbitrary limit on the number of audit container identifiers to limit abuse.
Signed-off-by Richard Guy Briggs <[email protected]>
---
 kernel/audit.c | 8 ++++++++
 kernel/audit.h | 4 ++++
 2 files changed, 12 insertions(+)
kernel/audit.c +0 -0
index 53d13d638c63..329916534dd2 100644
...
Quoted 16 lines
Quoted 16 lines
kernel/audit.c +1 -0
@@ -2465,6 +2472,7 @@ int audit_set_contid(struct task_struct *task, u64 contid)
                              newcont->owner = current;
                              refcount_set(&newcont->refcount, 1);
                              list_add_rcu(&newcont->list, &audit_contid_hash[h]);
+                             audit_contid_count++;
                      } else {
                              rc = -ENOMEM;
                              goto conterror;
kernel/audit.h +0 -0
index 162de8366b32..543f1334ba47 100644
@@ -219,6 +219,10 @@ static inline int audit_hash_contid(u64 contid)
      return (contid & (AUDIT_CONTID_BUCKETS-1));
 }
 
Quoted 12 lines
kernel/audit.h static inline int audit_hash_contid(u64 contid) · line 225
Quoted 4 lines
kernel/audit.h +4 -0
+extern int audit_contid_count;
+
+#define AUDIT_CONTID_COUNT   1 << 16
+
Just to ask the question, since it wasn't clear in the changelog, what abuse are you avoiding here? Ostensibly you should be able to create as many container ids as you have space for, and the simple creation of container ids doesn't seem like the resource strain I would be concerned about here, given that an orchestrator can still create as many containers as the system will otherwise allow, which will consume significantly more ram/disk/etc.

Quoted 6 lines

I've got a similar question. Up to this point in the patchset, there is a potential issue of hash bucket chain lengths and traversing them with a spinlock held, but it seems like we shouldn't be putting an arbitrary limit on audit container IDs unless we have a good reason for it. If for some reason we do want to enforce a limit, it should probably be a tunable value like a sysctl, or similar.

Can you separate and clarify the concerns here?

I plan to move this patch to the end of the patchset and make it optional, possibly adding a tuning mechanism. Like the migration from /proc to netlink for loginuid/sessionid/contid/capcontid, this was Eric Biederman's concern and suggested mitigation.

As for the first issue of the bucket chain length traversal while holding the list spin-lock, would you prefer to use the rcu lock to traverse the list and then only hold the spin-lock when modifying the list, and possibly even make the spin-lock more fine-grained per list?

Quoted 1 line

paul moore

- RGB

Richard Guy Briggs

author

2019-10-24 22:08 UTC #37 lore raw

Re: [PATCH ghak90 V7 14/21] audit: contid check descendancy and nesting

Quoted 11 lines · Paul Moore

Quoted 1 line · Richard Guy Briggs
?fixup! audit: convert to contid list to check for orch/engine ownership
?
Quoted 6 lines
Require the target task to be a descendant of the container orchestrator/engine.
You would only change the audit container ID from one set or inherited value to another if you were nesting containers.

Quoted 7 lines

Quoted 3 lines
If changing the contid, the container orchestrator/engine must be a descendant and not same orchestrator as the one that set it so it is not possible to change the contid of another orchestrator's container.
Did you mean to say that the container orchestrator must be an ancestor of the target, and the same orchestrator as the one that set the target process' audit container ID?

Not quite, the first half yes, but the second half: if it was already set by that orchestrator, it can't be set again. If it is a different orchestrator that is a descendant of the orchestrator that set it, then allow the action.

Quoted 1 line

Or maybe I'm missing something about what you are trying to do?

Does that help clarify it?

Quoted 97 lines

Signed-off-by Richard Guy Briggs <[email protected]>

---
 kernel/audit.c | 70 +++++++++++++++++++++++++++++++++++++++++++++++++++-------
 1 file changed, 62 insertions(+), 8 deletions(-)

kernel/audit.c +61 -8

index 9ce7a1ec7a92..69fe1e9af7cb 100644
@@ -2560,6 +2560,39 @@ static struct task_struct *audit_cont_owner(struct task_struct *tsk)
 }
 
 /*
+ * task_is_descendant - walk up a process family tree looking for a match
+ * @parent: the process to compare against while walking up from child
+ * @child: the process to start from while looking upwards for parent
+ *
+ * Returns 1 if child is a descendant of parent, 0 if not.
+ */
+static int task_is_descendant(struct task_struct *parent,
+                             struct task_struct *child)
+{
+       int rc = 0;
+       struct task_struct *walker = child;
+
+       if (!parent || !child)
+               return 0;
+
+       rcu_read_lock();
+       if (!thread_group_leader(parent))
+               parent = rcu_dereference(parent->group_leader);
+       while (walker->pid > 0) {
+               if (!thread_group_leader(walker))
+                       walker = rcu_dereference(walker->group_leader);
+               if (walker == parent) {
+                       rc = 1;
+                       break;
+               }
+               walker = rcu_dereference(walker->real_parent);
+       }
+       rcu_read_unlock();
+
+       return rc;
+}
+
+/*
  * audit_set_contid - set current task's audit contid
  * @task: target task
  * @contid: contid value

@@ -2587,22 +2620,43 @@ int audit_set_contid(struct task_struct *task, u64 contid)
        oldcontid = audit_get_contid(task);
        read_lock(&tasklist_lock);
        /* Don't allow the contid to be unset */
-       if (!audit_contid_valid(contid))
+       if (!audit_contid_valid(contid)) {
                rc = -EINVAL;
+               goto unlock;
+       }
        /* Don't allow the contid to be set to the same value again */
-       else if (contid == oldcontid) {
+       if (contid == oldcontid) {
                rc = -EADDRINUSE;
+               goto unlock;
+       }
        /* if we don't have caps, reject */
-       else if (!capable(CAP_AUDIT_CONTROL))
+       if (!capable(CAP_AUDIT_CONTROL)) {
                rc = -EPERM;
-       /* if task has children or is not single-threaded, deny */
-       else if (!list_empty(&task->children))
+               goto unlock;
+       }
+       /* if task has children, deny */
+       if (!list_empty(&task->children)) {
                rc = -EBUSY;
-       else if (!(thread_group_leader(task) && thread_group_empty(task)))
+               goto unlock;
+       }
+       /* if task is not single-threaded, deny */
+       if (!(thread_group_leader(task) && thread_group_empty(task))) {
                rc = -EALREADY;
-       /* if contid is already set, deny */
-       else if (audit_contid_set(task))
+               goto unlock;
+       }
+       /* if task is not descendant, block */
+       if (task == current) {
+               rc = -EBADSLT;
+               goto unlock;
+       }
+       if (!task_is_descendant(current, task)) {
+               rc = -EXDEV;
+               goto unlock;
+       }
+       /* only allow contid setting again if nesting */
+       if (audit_contid_set(task) && current == audit_cont_owner(task))
                rc = -ECHILD;

Quoted 8 lines

kernel/audit.c int audit_set_contid(struct task_struct *task, u64 contid) · line 2662
Quoted 4 lines
kernel/audit.c +1 -0
+unlock:
        read_unlock(&tasklist_lock);
        if (!rc) {
                struct audit_cont *oldcont = audit_cont(task);
--
paul moore
www.paul-moore.com

- RGB

Richard Guy Briggs

author

2019-10-25 19:20 UTC #26 lore raw

Re: [PATCH ghak90 V7 08/21] audit: add contid support for signalling the audit daemon

Quoted 118 lines · Paul Moore

Quoted 109 lines · Richard Guy Briggs

Add audit container identifier support to the action of signalling the audit daemon.

Signed-off-by Richard Guy Briggs <[email protected]>

---
 include/linux/audit.h       |  7 +++++++
 include/uapi/linux/audit.h  |  1 +
 kernel/audit.c              | 28 ++++++++++++++++++++++++++++
 kernel/audit.h              |  1 +
 security/selinux/nlmsgtab.c |  1 +
 5 files changed, 38 insertions(+)

include/linux/audit.h +7 -0

index 0c18d8e30620..7b640c4da4ee 100644
@@ -23,6 +23,13 @@ struct audit_sig_info {
        char            ctx[0];
 };
 
+struct audit_sig_info2 {
+       uid_t           uid;
+       pid_t           pid;
+       u64             cid;
+       char            ctx[0];
+};
+
 struct audit_buffer;
 struct audit_context;
 struct inode;

include/uapi/linux/audit.h +1 -0

index 4ed080f28b47..693ec6e0288b 100644
@@ -72,6 +72,7 @@
 #define AUDIT_SET_FEATURE      1018    /* Turn an audit feature on or off */
 #define AUDIT_GET_FEATURE      1019    /* Get which features are enabled */
 #define AUDIT_CONTAINER_OP     1020    /* Define the container id and info */
+#define AUDIT_SIGNAL_INFO2     1021    /* Get info auditd signal sender */
 
 #define AUDIT_FIRST_USER_MSG   1100    /* Userspace messages mostly uninteresting to kernel */
 #define AUDIT_USER_AVC         1107    /* We filter this differently */

kernel/audit.c +27 -0

index adfb3e6a7f0c..df3db29f5a8a 100644
@@ -125,6 +125,7 @@ struct audit_net {
 kuid_t         audit_sig_uid = INVALID_UID;
 pid_t          audit_sig_pid = -1;
 u32            audit_sig_sid = 0;
+u64            audit_sig_cid = AUDIT_CID_UNSET;
 
 /* Records can be lost in several ways:
    0) [suppressed in audit_alloc]

@@ -1094,6 +1095,7 @@ static int audit_netlink_ok(struct sk_buff *skb, u16 msg_type)
        case AUDIT_ADD_RULE:
        case AUDIT_DEL_RULE:
        case AUDIT_SIGNAL_INFO:
+       case AUDIT_SIGNAL_INFO2:
        case AUDIT_TTY_GET:
        case AUDIT_TTY_SET:
        case AUDIT_TRIM:

@@ -1257,6 +1259,7 @@ static int audit_receive_msg(struct sk_buff *skb, struct nlmsghdr *nlh)
        struct audit_buffer     *ab;
        u16                     msg_type = nlh->nlmsg_type;
        struct audit_sig_info   *sig_data;
+       struct audit_sig_info2  *sig_data2;
        char                    *ctx = NULL;
        u32                     len;

@@ -1516,6 +1519,30 @@ static int audit_receive_msg(struct sk_buff *skb, struct nlmsghdr *nlh)
                                 sig_data, sizeof(*sig_data) + len);
                kfree(sig_data);
                break;
+       case AUDIT_SIGNAL_INFO2:
+               len = 0;
+               if (audit_sig_sid) {
+                       err = security_secid_to_secctx(audit_sig_sid, &ctx, &len);
+                       if (err)
+                               return err;
+               }
+               sig_data2 = kmalloc(sizeof(*sig_data2) + len, GFP_KERNEL);
+               if (!sig_data2) {
+                       if (audit_sig_sid)
+                               security_release_secctx(ctx, len);
+                       return -ENOMEM;
+               }
+               sig_data2->uid = from_kuid(&init_user_ns, audit_sig_uid);
+               sig_data2->pid = audit_sig_pid;
+               if (audit_sig_sid) {
+                       memcpy(sig_data2->ctx, ctx, len);
+                       security_release_secctx(ctx, len);
+               }
+               sig_data2->cid = audit_sig_cid;
+               audit_send_reply(skb, seq, AUDIT_SIGNAL_INFO2, 0, 0,
+                                sig_data2, sizeof(*sig_data2) + len);
+               kfree(sig_data2);
+               break;
        case AUDIT_TTY_GET: {
                struct audit_tty_status s;
                unsigned int t;

kernel/audit.c int audit_signal_info(int sig, struct task_struct *t) · line 2415

Quoted 5 lines

kernel/audit.c +1 -0

@@ -2384,6 +2411,7 @@ int audit_signal_info(int sig, struct task_struct *t)
                else
                        audit_sig_uid = uid;
                security_task_getsecid(current, &audit_sig_sid);
+               audit_sig_cid = audit_get_contid(current);
        }

I've been wondering something as I've been working my way through these patches and this patch seems like a good spot to discuss this

Quoted 8 lines

... Now that we have the concept of an audit container ID "lifetime" in the kernel, when do we consider the ID gone? Is it when the last process in the container exits, or is it when we generate the last audit record which could possibly contain the audit container ID? This patch would appear to support the former, but if we wanted the latter we would need to grab a reference to the audit container ID struct so it wouldn't "die" on us before we could emit the signal info record.

Are you concerned with the availability of the data when the audit signal info record is generated, when the kernel last deals with a particular contid or when userspace thinks there will be no more references to it?

I've got a bit of a dilemma with this one...

In fact, the latter situation you describe isn't a concern at present to be able to deliver the information since the value is copied into the audit signal global internal variables before the signalling task dies and the audit signal info record is created from those copied (cached) values when requested from userspace.

So the issue raised above I don't think is a problem. However, patch 18 (which wasn't reviewed because it was a patch to a number of preceeding patches) changes the reporting approach to give a chain of nested contids which isn't reflected in the same level of reporting for the audit signal patch/mechanism. Solving this is a bit more complex. We could have the audit signal internal caching store a pointer to the relevant container object and bump its refcount to ensure it doesn't vanish until we are done with it, but the audit signal info binary record format already has a variable length due to the selinux context at the end of that struct and adding a second variable length element to it would make it more complicated (but not impossible) to handle.

Quoted 1 line

paul moore

- RGB

--
Richard Guy Briggs <[email protected]>
Sr. S/W Engineer, Kernel Security, Base Operating Systems Remote, Ottawa, Red Hat Canada
IRC: rgb, SunRaycer
Voice: +1.647.777.2635, Internal: (81) 32635

Richard Guy Briggs

author

2019-10-25 19:43 UTC #13 lore raw

Re: [PATCH ghak90 V7 05/21] audit: log drop of contid on exit of last task

Quoted 48 lines · Paul Moore

Quoted 47 lines · Richard Guy Briggs

Signed-off-by Richard Guy Briggs <[email protected]>

---
 kernel/audit.c   | 32 ++++++++++++++++++++++++++++++++
 kernel/audit.h   |  2 ++
 kernel/auditsc.c |  2 ++
 3 files changed, 36 insertions(+)

kernel/audit.c +27 -0

index ea0899130cc1..53d13d638c63 100644
@@ -2503,6 +2503,38 @@ int audit_set_contid(struct task_struct *task, u64 contid)
        return rc;
 }
 
+void audit_log_container_drop(void)
+{
+       struct audit_buffer *ab;
+       uid_t uid;
+       struct tty_struct *tty;
+       char comm[sizeof(current->comm)];
+
+       if (!current->audit || !current->audit->cont ||
+           refcount_read(&current->audit->cont->refcount) > 1)
+               return;
+       ab = audit_log_start(audit_context(), GFP_KERNEL, AUDIT_CONTAINER_OP);
+       if (!ab)
+               return;
+
+       uid = from_kuid(&init_user_ns, task_uid(current));
+       tty = audit_get_tty();
+       audit_log_format(ab,
+                        "op=drop opid=%d contid=%llu old-contid=%llu pid=%d uid=%u auid=%u tty=%s ses=%u",
+                        task_tgid_nr(current), audit_get_contid(current),
+                        audit_get_contid(current), task_tgid_nr(current), uid,
+                        from_kuid(&init_user_ns, audit_get_loginuid(current)),
+                        tty ? tty_name(tty) : "(none)",
+                                audit_get_sessionid(current));
+       audit_put_tty(tty);
+       audit_log_task_context(ab);
+       audit_log_format(ab, " comm=");
+       audit_log_untrustedstring(ab, get_task_comm(comm, current));

Quoted 8 lines

kernel/audit.c int audit_set_contid(struct task_struct *task, u64 contid) · line 2536
Quoted 4 lines
kernel/audit.c +4 -0
+       audit_log_d_path_exe(ab, current->mm);
+       audit_log_format(ab, " res=1");
+       audit_log_end(ab);
+}
Why can't we just do this in audit_cont_put()? Is it because we call audit_cont_put() in the new audit_free() function? What if we were to do it in __audit_free()/audit_free_syscall()?

The intent was to put this before the EOE record of a syscall so we could fill out all the fields similarly to op=set, but this could stand alone dropping or nulling a bunch of fields.

It would also never get printed if we left it before the EOE and had the audit signal info record keep a reference to it.

Hmmm...

Quoted 1 line

paul moore

- RGB

Richard Guy Briggs

author

2019-10-25 20:00 UTC #6 lore raw

Re: [PATCH ghak90 V7 04/21] audit: convert to contid list to check for orch/engine ownership

Quoted 51 lines · Neil Horman

Quoted 50 lines · Richard Guy Briggs
Store the audit container identifier in a refcounted kernel object that is added to the master list of audit container identifiers. This will allow multiple container orchestrators/engines to work on the same machine without danger of inadvertantly re-using an existing identifier. It will also allow an orchestrator to inject a process into an existing container by checking if the original container owner is the one injecting the task. A hash table list is used to optimize searches.
Signed-off-by Richard Guy Briggs <[email protected]>
---
 include/linux/audit.h | 26 ++++++++++++++--
 kernel/audit.c        | 86 ++++++++++++++++++++++++++++++++++++++++++++++++---
 kernel/audit.h        |  8 +++++
 3 files changed, 112 insertions(+), 8 deletions(-)
include/linux/audit.h +11 -3
index f2e3b81f2942..e317807cdd3e 100644
@@ -95,10 +95,18 @@ struct audit_ntp_data {
 struct audit_ntp_data {};
 #endif
 
+struct audit_cont {
+	struct list_head	list;
+	u64			id;
+	struct task_struct	*owner;
+	refcount_t              refcount;
+	struct rcu_head         rcu;
+};
+
 struct audit_task_info {
 	kuid_t			loginuid;
 	unsigned int		sessionid;
-	u64			contid;
+	struct audit_cont	*cont;
 #ifdef CONFIG_AUDITSYSCALL
 	struct audit_context	*ctx;
 #endif
@@ -203,11 +211,15 @@ static inline unsigned int audit_get_sessionid(struct task_struct *tsk)
 
 static inline u64 audit_get_contid(struct task_struct *tsk)
 {
-	if (!tsk->audit)
+	if (!tsk->audit || !tsk->audit->cont)
 		return AUDIT_CID_UNSET;
-	return tsk->audit->contid;
+	return tsk->audit->cont->id;
 }
 

Quoted 6 lines

include/linux/audit.h static inline unsigned int audit_get_sessionid(struct task_struct *tsk) · line 222
Quoted 4 lines
include/linux/audit.h +4 -0
+extern struct audit_cont *audit_cont(struct task_struct *tsk);
+
+extern void audit_cont_put(struct audit_cont *cont);
+
I see that you manual increment this refcount at various call sites, why no corresponding audit_contid_hold function?

I was trying to avoid the get function due to having one site where I needed the pointer for later but didn't need a refcount to it so that I could release the refcount it if it was replaced by another cont object. A hold function would just contain one line that would call the refcount_inc(). If I did convert things over to a get function, it would hide some of this extra conditional code in the main calling function, but in one place I could just call put immediately to neutralize that unneeded refcount.

Would you see any issue with that extra get/put refcount that would only happen in the case of changing a contid in a nesting situation?

Quoted 194 lines

Neil

Quoted 192 lines

include/linux/audit.h +8 -0

 extern u32 audit_enabled;
 
 extern int audit_signal_info(int sig, struct task_struct *t);

@@ -277,6 +289,14 @@ static inline u64 audit_get_contid(struct task_struct *tsk)
 	return AUDIT_CID_UNSET;
 }
 
+static inline struct audit_cont *audit_cont(struct task_struct *tsk)
+{
+	return NULL;
+}
+
+static inline void audit_cont_put(struct audit_cont *cont)
+{ }
+
 #define audit_enabled AUDIT_OFF
 
 static inline int audit_signal_info(int sig, struct task_struct *t)

kernel/audit.c +81 -5

index a36ea57cbb61..ea0899130cc1 100644
@@ -137,6 +137,8 @@ struct audit_net {
 
 /* Hash for inode-based rules */
 struct list_head audit_inode_hash[AUDIT_INODE_BUCKETS];
+/* Hash for contid-based rules */
+struct list_head audit_contid_hash[AUDIT_CONTID_BUCKETS];
 
 static struct kmem_cache *audit_buffer_cache;

@@ -204,6 +206,8 @@ struct audit_reply {
 
 static struct kmem_cache *audit_task_cache;
 
+static DEFINE_SPINLOCK(audit_contid_list_lock);
+
 void __init audit_task_init(void)
 {
 	audit_task_cache = kmem_cache_create("audit_task",

@@ -231,7 +235,9 @@ int audit_alloc(struct task_struct *tsk)
 	}
 	info->loginuid = audit_get_loginuid(current);
 	info->sessionid = audit_get_sessionid(current);
-	info->contid = audit_get_contid(current);
+	info->cont = audit_cont(current);
+	if (info->cont)
+		refcount_inc(&info->cont->refcount);
 	tsk->audit = info;
 
 	ret = audit_alloc_syscall(tsk);

@@ -246,7 +252,7 @@ int audit_alloc(struct task_struct *tsk)
 struct audit_task_info init_struct_audit = {
 	.loginuid = INVALID_UID,
 	.sessionid = AUDIT_SID_UNSET,
-	.contid = AUDIT_CID_UNSET,
+	.cont = NULL,
 #ifdef CONFIG_AUDITSYSCALL
 	.ctx = NULL,
 #endif

@@ -266,6 +272,9 @@ void audit_free(struct task_struct *tsk)
 	/* Freeing the audit_task_info struct must be performed after
 	 * audit_log_exit() due to need for loginuid and sessionid.
 	 */
+	spin_lock(&audit_contid_list_lock); 
+	audit_cont_put(tsk->audit->cont);
+	spin_unlock(&audit_contid_list_lock); 
 	info = tsk->audit;
 	tsk->audit = NULL;
 	kmem_cache_free(audit_task_cache, info);

@@ -1657,6 +1666,9 @@ static int __init audit_init(void)
 	for (i = 0; i < AUDIT_INODE_BUCKETS; i++)
 		INIT_LIST_HEAD(&audit_inode_hash[i]);
 
+	for (i = 0; i < AUDIT_CONTID_BUCKETS; i++)
+		INIT_LIST_HEAD(&audit_contid_hash[i]);
+
 	mutex_init(&audit_cmd_mutex.lock);
 	audit_cmd_mutex.owner = NULL;

@@ -2356,6 +2368,32 @@ int audit_signal_info(int sig, struct task_struct *t)
 	return audit_signal_info_syscall(t);
 }
 
+struct audit_cont *audit_cont(struct task_struct *tsk)
+{
+	if (!tsk->audit || !tsk->audit->cont)
+		return NULL;
+	return tsk->audit->cont;
+}
+
+/* audit_contid_list_lock must be held by caller */
+void audit_cont_put(struct audit_cont *cont)
+{
+	if (!cont)
+		return;
+	if (refcount_dec_and_test(&cont->refcount)) {
+		put_task_struct(cont->owner);
+		list_del_rcu(&cont->list);
+		kfree_rcu(cont, rcu);
+	}
+}
+
+static struct task_struct *audit_cont_owner(struct task_struct *tsk)
+{
+	if (tsk->audit && tsk->audit->cont)
+		return tsk->audit->cont->owner;
+	return NULL;
+}
+
 /*
  * audit_set_contid - set current task's audit contid
  * @task: target task

@@ -2382,9 +2420,12 @@ int audit_set_contid(struct task_struct *task, u64 contid)
 	}
 	oldcontid = audit_get_contid(task);
 	read_lock(&tasklist_lock);
-	/* Don't allow the audit containerid to be unset */
+	/* Don't allow the contid to be unset */
 	if (!audit_contid_valid(contid))
 		rc = -EINVAL;
+	/* Don't allow the contid to be set to the same value again */
+	else if (contid == oldcontid) {
+		rc = -EADDRINUSE;
 	/* if we don't have caps, reject */
 	else if (!capable(CAP_AUDIT_CONTROL))
 		rc = -EPERM;

@@ -2397,8 +2438,43 @@ int audit_set_contid(struct task_struct *task, u64 contid)
 	else if (audit_contid_set(task))
 		rc = -ECHILD;
 	read_unlock(&tasklist_lock);
-	if (!rc)
-		task->audit->contid = contid;
+	if (!rc) {
+		struct audit_cont *oldcont = audit_cont(task);
+		struct audit_cont *cont = NULL;
+		struct audit_cont *newcont = NULL;
+		int h = audit_hash_contid(contid);
+
+		spin_lock(&audit_contid_list_lock);
+		list_for_each_entry_rcu(cont, &audit_contid_hash[h], list)
+			if (cont->id == contid) {
+				/* task injection to existing container */
+				if (current == cont->owner) {
+					refcount_inc(&cont->refcount);
+					newcont = cont;
+				} else {
+					rc = -ENOTUNIQ;
+					goto conterror;
+				}
+			}
+		if (!newcont) {
+			newcont = kmalloc(sizeof(struct audit_cont), GFP_ATOMIC);
+			if (newcont) {
+				INIT_LIST_HEAD(&newcont->list);
+				newcont->id = contid;
+				get_task_struct(current);
+				newcont->owner = current;
+				refcount_set(&newcont->refcount, 1);
+				list_add_rcu(&newcont->list, &audit_contid_hash[h]);
+			} else {
+				rc = -ENOMEM;
+				goto conterror;
+			}
+		}
+		task->audit->cont = newcont;
+		audit_cont_put(oldcont);
+conterror:
+		spin_unlock(&audit_contid_list_lock);
+	}
 	task_unlock(task);
 
 	if (!audit_enabled)

kernel/audit.h +8 -0

index 16bd03b88e0d..e4a31aa92dfe 100644
@@ -211,6 +211,14 @@ static inline int audit_hash_ino(u32 ino)
 	return (ino & (AUDIT_INODE_BUCKETS-1));
 }
 
+#define AUDIT_CONTID_BUCKETS	32
+extern struct list_head audit_contid_hash[AUDIT_CONTID_BUCKETS];
+
+static inline int audit_hash_contid(u64 contid)
+{
+	return (contid & (AUDIT_CONTID_BUCKETS-1));
+}
+
 /* Indicates that audit should log the full pathname. */
 #define AUDIT_NAME_FULL -1

Quoted 4 lines

Quoted 4 lines
Signature · 1 line
1.8.3.1

- RGB

--
Richard Guy Briggs <[email protected]>
Sr. S/W Engineer, Kernel Security, Base Operating Systems Remote, Ottawa, Red Hat Canada
IRC: rgb, SunRaycer
Voice: +1.647.777.2635, Internal: (81) 32635

Richard Guy Briggs

author

2019-10-25 20:15 UTC #22 lore raw

Re: [PATCH ghak90 V7 06/21] audit: contid limit of 32k imposed to avoid DoS

Quoted 30 lines · Neil Horman

Quoted 29 lines · Richard Guy Briggs

Set an arbitrary limit on the number of audit container identifiers to limit abuse.

Signed-off-by Richard Guy Briggs <[email protected]>

---
 kernel/audit.c | 8 ++++++++
 kernel/audit.h | 4 ++++
 2 files changed, 12 insertions(+)

kernel/audit.c +2 -0

index 53d13d638c63..329916534dd2 100644
@@ -139,6 +139,7 @@ struct audit_net {
 struct list_head audit_inode_hash[AUDIT_INODE_BUCKETS];
 /* Hash for contid-based rules */
 struct list_head audit_contid_hash[AUDIT_CONTID_BUCKETS];
+int audit_contid_count = 0;
 
 static struct kmem_cache *audit_buffer_cache;

@@ -2384,6 +2385,7 @@ void audit_cont_put(struct audit_cont *cont)
 		put_task_struct(cont->owner);
 		list_del_rcu(&cont->list);
 		kfree_rcu(cont, rcu);
+		audit_contid_count--;
 	}
 }

Quoted 12 lines

kernel/audit.c int audit_set_contid(struct task_struct *task, u64 contid) · line 2465
Quoted 9 lines
kernel/audit.c +5 -0
@@ -2456,6 +2458,11 @@ int audit_set_contid(struct task_struct *task, u64 contid)
 					goto conterror;
 				}
 			}
+		/* Set max contids */
+		if (audit_contid_count > AUDIT_CONTID_COUNT) {
+			rc = -ENOSPC;
+			goto conterror;
+		}
You should check for audit_contid_count == AUDIT_CONTID_COUNT here, no? or at least >=, since you increment it below. Otherwise its possible that you will exceed it by one in the full condition.

Yes, agreed.

Quoted 19 lines

kernel/audit.c +1 -0

 		if (!newcont) {
 			newcont = kmalloc(sizeof(struct audit_cont), GFP_ATOMIC);
 			if (newcont) {

@@ -2465,6 +2472,7 @@ int audit_set_contid(struct task_struct *task, u64 contid)
 				newcont->owner = current;
 				refcount_set(&newcont->refcount, 1);
 				list_add_rcu(&newcont->list, &audit_contid_hash[h]);
+				audit_contid_count++;
 			} else {
 				rc = -ENOMEM;
 				goto conterror;

kernel/audit.h +0 -0

index 162de8366b32..543f1334ba47 100644
@@ -219,6 +219,10 @@ static inline int audit_hash_contid(u64 contid)
 	return (contid & (AUDIT_CONTID_BUCKETS-1));
 }

Quoted 11 lines

kernel/audit.h static inline int audit_hash_contid(u64 contid) · line 225
Quoted 4 lines
kernel/audit.h +4 -0
+extern int audit_contid_count;
+
+#define AUDIT_CONTID_COUNT	1 << 16
+
Just to ask the question, since it wasn't clear in the changelog, what abuse are you avoiding here? Ostensibly you should be able to create as many container ids as you have space for, and the simple creation of container ids doesn't seem like the resource strain I would be concerned about here, given that an orchestrator can still create as many containers as the system will otherwise allow, which will consume significantly more ram/disk/etc.

Agreed. I'm not a huge fan of this, but included it as an optional patch to address resource abuse concerns of Eric Beiderman. I'll push it to the end of the patchset and make it clear it is optional unless I hear a compelling reason to keep it.

A similar argument was used to make the audit queue length tunable parameter unlimited.

Quoted 5 lines

Quoted 5 lines
kernel/audit.h +0 -0
 /* Indicates that audit should log the full pathname. */
 #define AUDIT_NAME_FULL -1
 
Signature · 1 line
1.8.3.1

- RGB

--
Richard Guy Briggs <[email protected]>
Sr. S/W Engineer, Kernel Security, Base Operating Systems Remote, Ottawa, Red Hat Canada
IRC: rgb, SunRaycer
Voice: +1.647.777.2635, Internal: (81) 32635

Richard Guy Briggs

author

2019-10-25 21:00 UTC #9 lore raw

Re: [PATCH ghak90 V7 04/21] audit: convert to contid list to check for orch/engine ownership

Quoted 16 lines · Paul Moore

Quoted 8 lines · Richard Guy Briggs
Store the audit container identifier in a refcounted kernel object that is added to the master list of audit container identifiers. This will allow multiple container orchestrators/engines to work on the same machine without danger of inadvertantly re-using an existing identifier. It will also allow an orchestrator to inject a process into an existing container by checking if the original container owner is the one injecting the task. A hash table list is used to optimize searches.
Quoted 6 lines
Signed-off-by Richard Guy Briggs <[email protected]>
---
 include/linux/audit.h | 26 ++++++++++++++--
 kernel/audit.c        | 86 ++++++++++++++++++++++++++++++++++++++++++++++++---
 kernel/audit.h        |  8 +++++
 3 files changed, 112 insertions(+), 8 deletions(-)

Quoted 6 lines

One general comment before we go off into the weeds on this ... I can understand why you wanted to keep this patch separate from the earlier patches, but as we get closer to having mergeable code this should get folded into the previous patches. For example, there shouldn't be a change in audit_task_info where you change the contid field from a u64 to struct pointer, it should be a struct pointer from the start.

I should have marked this patchset as RFC even though it was v7 due to a lot of new ideas/code that was added with uncertainties needing comment and direction.

Quoted 2 lines

It's also disappointing that idr appears to only be for 32-bit ID values, if we had a 64-bit idr I think we could simplify this greatly.

Perhaps. I do still see value in letting the orchestrator choose the value.

Quoted 8 lines

include/linux/audit.h +0 -0

index f2e3b81f2942..e317807cdd3e 100644
@@ -95,10 +95,18 @@ struct audit_ntp_data {
 struct audit_ntp_data {};
 #endif

Quoted 11 lines

include/linux/audit.h struct audit_ntp_data { · line 104
Quoted 7 lines
include/linux/audit.h +7 -0
+struct audit_cont {
+       struct list_head        list;
+       u64                     id;
+       struct task_struct      *owner;
+       refcount_t              refcount;
+       struct rcu_head         rcu;
+};
It seems as though in most of the code you are using "contid", any reason why didn't stick with that naming scheme here, e.g. "struct audit_contid"?

I was using contid to refer to the value itself and cont to refer to the refcounted object. I find cont a bit too terse, so I'm still thinking of changing it. Perhaps contobj?

Quoted 7 lines

include/linux/audit.h struct audit_ntp_data { · line 109

Quoted 5 lines

include/linux/audit.h +1 -1

 struct audit_task_info {
        kuid_t                  loginuid;
        unsigned int            sessionid;
-       u64                     contid;
+       struct audit_cont       *cont;

Same, why not stick with "contid"?

^^^

Quoted 5 lines

include/linux/audit.h +0 -0

 #ifdef CONFIG_AUDITSYSCALL
        struct audit_context    *ctx;
 #endif

@@ -203,11 +211,15 @@ static inline unsigned int audit_get_sessionid(struct task_struct *tsk)

Quoted 15 lines

include/linux/audit.h static inline unsigned int audit_get_sessionid(struct task_struct *tsk) · line 217
Quoted 8 lines
include/linux/audit.h +2 -2
 static inline u64 audit_get_contid(struct task_struct *tsk)
 {
-       if (!tsk->audit)
+       if (!tsk->audit || !tsk->audit->cont)
                return AUDIT_CID_UNSET;
-       return tsk->audit->contid;
+       return tsk->audit->cont->id;
 }
Assuming for a moment that we implement an audit_contid_get() (see Neil's comment as well as mine below), we probably need to name this something different so we don't all lose our minds when we read this code. On the plus side we can probably preface it with an underscore since it is a static, in which case _audit_contid_get() might be okay, but I'm open to suggestions.

I'm fine with the "_" prefix, can you point to precedent or convention?

Quoted 5 lines

include/linux/audit.h static inline unsigned int audit_get_sessionid(struct task_struct *tsk) · line 221
Quoted 3 lines
include/linux/audit.h +3 -0
+extern struct audit_cont *audit_cont(struct task_struct *tsk);
+
+extern void audit_cont_put(struct audit_cont *cont);
More of the "contid" vs "cont".

^^^

Quoted 31 lines

include/linux/audit.h +8 -0

 extern u32 audit_enabled;
 
 extern int audit_signal_info(int sig, struct task_struct *t);

@@ -277,6 +289,14 @@ static inline u64 audit_get_contid(struct task_struct *tsk)
        return AUDIT_CID_UNSET;
 }
 
+static inline struct audit_cont *audit_cont(struct task_struct *tsk)
+{
+       return NULL;
+}
+
+static inline void audit_cont_put(struct audit_cont *cont)
+{ }
+
 #define audit_enabled AUDIT_OFF
 
 static inline int audit_signal_info(int sig, struct task_struct *t)

kernel/audit.c +2 -0

index a36ea57cbb61..ea0899130cc1 100644
@@ -137,6 +137,8 @@ struct audit_net {
 
 /* Hash for inode-based rules */
 struct list_head audit_inode_hash[AUDIT_INODE_BUCKETS];
+/* Hash for contid-based rules */
+struct list_head audit_contid_hash[AUDIT_CONTID_BUCKETS];
 
 static struct kmem_cache *audit_buffer_cache;

Quoted 8 lines

kernel/audit.c struct audit_reply { · line 209
Quoted 5 lines
kernel/audit.c +1 -0
@@ -204,6 +206,8 @@ struct audit_reply {
 
 static struct kmem_cache *audit_task_cache;
 
+static DEFINE_SPINLOCK(audit_contid_list_lock);
Since it looks like this protectects audit_contid_hash, I think it would be better to move it up underneath audit_contid_hash.

Agreed.

Quoted 6 lines

kernel/audit.c +0 -0

 void __init audit_task_init(void)
 {
        audit_task_cache = kmem_cache_create("audit_task",

@@ -231,7 +235,9 @@ int audit_alloc(struct task_struct *tsk)
        }
        info->loginuid = audit_get_loginuid(current);

Quoted 8 lines

kernel/audit.c int audit_alloc(struct task_struct *tsk) · line 240
Quoted 5 lines
kernel/audit.c +3 -1
        info->sessionid = audit_get_sessionid(current);
-       info->contid = audit_get_contid(current);
+       info->cont = audit_cont(current);
+       if (info->cont)
+               refcount_inc(&info->cont->refcount);
See the other comments about a "get" function, but I think we need a RCU read lock around the above, no?

The rcu read lock is to protect the list rather than the cont object itself, the latter of which is protected by its refcount.

Quoted 11 lines

kernel/audit.c int audit_alloc(struct task_struct *tsk) · line 255

Quoted 9 lines

kernel/audit.c +1 -1

        tsk->audit = info;
 
        ret = audit_alloc_syscall(tsk);

@@ -246,7 +252,7 @@ int audit_alloc(struct task_struct *tsk)
 struct audit_task_info init_struct_audit = {
        .loginuid = INVALID_UID,
        .sessionid = AUDIT_SID_UNSET,
-       .contid = AUDIT_CID_UNSET,
+       .cont = NULL,

More "cont" vs "contid".

^^^

Quoted 5 lines

kernel/audit.c +0 -0

 #ifdef CONFIG_AUDITSYSCALL
        .ctx = NULL,
 #endif

@@ -266,6 +272,9 @@ void audit_free(struct task_struct *tsk)
        /* Freeing the audit_task_info struct must be performed after

Quoted 8 lines

kernel/audit.c void audit_free(struct task_struct *tsk) · line 277
Quoted 5 lines
kernel/audit.c +3 -0
         * audit_log_exit() due to need for loginuid and sessionid.
         */
+       spin_lock(&audit_contid_list_lock);
+       audit_cont_put(tsk->audit->cont);
+       spin_unlock(&audit_contid_list_lock);
Perhaps this will make sense as I get further into the patchset, but why not move the spin lock operations into audit_[cont/contid]_put()?

audit_cont_put() is recursive in patch 18/21, which would have been evident if 18/21 was squashed into this one as you pointed out there...

Quoted 30 lines

kernel/audit.c +16 -0

        info = tsk->audit;
        tsk->audit = NULL;
        kmem_cache_free(audit_task_cache, info);

@@ -1657,6 +1666,9 @@ static int __init audit_init(void)
        for (i = 0; i < AUDIT_INODE_BUCKETS; i++)
                INIT_LIST_HEAD(&audit_inode_hash[i]);
 
+       for (i = 0; i < AUDIT_CONTID_BUCKETS; i++)
+               INIT_LIST_HEAD(&audit_contid_hash[i]);
+
        mutex_init(&audit_cmd_mutex.lock);
        audit_cmd_mutex.owner = NULL;

@@ -2356,6 +2368,32 @@ int audit_signal_info(int sig, struct task_struct *t)
        return audit_signal_info_syscall(t);
 }
 
+struct audit_cont *audit_cont(struct task_struct *tsk)
+{
+       if (!tsk->audit || !tsk->audit->cont)
+               return NULL;
+       return tsk->audit->cont;
+}
+
+/* audit_contid_list_lock must be held by caller */
+void audit_cont_put(struct audit_cont *cont)
+{
+       if (!cont)
+               return;
+       if (refcount_dec_and_test(&cont->refcount)) {

Quoted 8 lines

kernel/audit.c int audit_signal_info(int sig, struct task_struct *t) · line 2388
Quoted 5 lines
kernel/audit.c +5 -0
+               put_task_struct(cont->owner);
+               list_del_rcu(&cont->list);
+               kfree_rcu(cont, rcu);
+       }
+}
I tend to agree with Neil's previous comment; if we've got a audit_[cont/contid]_put(), why not an audit_[cont/contid]_get()?

^^^

Quoted 5 lines

kernel/audit.c +5 -0

+static struct task_struct *audit_cont_owner(struct task_struct *tsk)
+{
+       if (tsk->audit && tsk->audit->cont)
+               return tsk->audit->cont->owner;
+       return NULL;

Quoted 8 lines

kernel/audit.c int audit_signal_info(int sig, struct task_struct *t) · line 2394
Quoted 1 line
kernel/audit.c +1 -0
+}
I'm not sure if this is possible (I haven't make my way through the entire patchset) and the function above isn't used in this patch (why is it here?), but it seems like it would be safer to convert this into an audit_contid_isowner() function that simply returns 1/0 depending on if the passed task_struct is the owner or not of a passed audit container ID value?

Agreed since it is only ever compared with current. It can be moved to 14/21.

Quoted 14 lines

kernel/audit.c +4 -1

 /*
  * audit_set_contid - set current task's audit contid
  * @task: target task

@@ -2382,9 +2420,12 @@ int audit_set_contid(struct task_struct *task, u64 contid)
        }
        oldcontid = audit_get_contid(task);
        read_lock(&tasklist_lock);
-       /* Don't allow the audit containerid to be unset */
+       /* Don't allow the contid to be unset */
        if (!audit_contid_valid(contid))
                rc = -EINVAL;
+       /* Don't allow the contid to be set to the same value again */
+       else if (contid == oldcontid) {
+               rc = -EADDRINUSE;

Quoted 8 lines

kernel/audit.c int audit_set_contid(struct task_struct *task, u64 contid) · line 2431
Quoted 3 lines
kernel/audit.c +0 -0
        /* if we don't have caps, reject */
        else if (!capable(CAP_AUDIT_CONTROL))
                rc = -EPERM;
RCU read lock? It's a bit dicey since I believe the tasklist_lock is going to provide us the safety we need, but if we are going to claim that the audit container ID list is protected by RCU we should probably use it.

Yes, perhaps, but to protect the task read, not the list, until it is accessed. Getting the contid value or cont pointer via the task does not involve the list. The cont pointer is protected by its refcount.

Quoted 11 lines

kernel/audit.c int audit_set_contid(struct task_struct *task, u64 contid) · line 2442
Quoted 8 lines
kernel/audit.c +2 -2
@@ -2397,8 +2438,43 @@ int audit_set_contid(struct task_struct *task, u64 contid)
        else if (audit_contid_set(task))
                rc = -ECHILD;
        read_unlock(&tasklist_lock);
-       if (!rc)
-               task->audit->contid = contid;
+       if (!rc) {
+               struct audit_cont *oldcont = audit_cont(task);
Previously we held the tasklist_lock to protect the audit container ID associated with the struct, should we still be holding it here?

We held the tasklist_lock to protect access to the target task's child/parent/thread relationships.

Quoted 4 lines

Regardless, I worry that the lock dependencies between the tasklist_lock and the audit_contid_list_lock are going to be tricky. It might be nice to document the relationship in a comment up near where you declare audit_contid_list_lock.

I don't think there should be a conflict between the two.

The contid_list_lock doesn't care if the cont object is associated to a particular task.

Quoted 10 lines

kernel/audit.c int audit_set_contid(struct task_struct *task, u64 contid) · line 2451

Quoted 9 lines

kernel/audit.c +9 -0

+               struct audit_cont *cont = NULL;
+               struct audit_cont *newcont = NULL;
+               int h = audit_hash_contid(contid);
+
+               spin_lock(&audit_contid_list_lock);
+               list_for_each_entry_rcu(cont, &audit_contid_hash[h], list)
+                       if (cont->id == contid) {
+                               /* task injection to existing container */
+                               if (current == cont->owner) {

Quoted 3 lines

I understand the desire to limit a given audit container ID to the orchestrator that created it, but are we certain that we can track audit container ID "ownership" via a single instance of a task_struct?

Are you suggesting that a task_struct representing a task may be replaced for a specific task? I don't believe that will ever happen.

Quoted 2 lines

What happens when the orchestrator stops/restarts/crashes? Do we
even care?

Reap all of its containers?

Quoted 4 lines

kernel/audit.c int audit_set_contid(struct task_struct *task, u64 contid) · line 2453
Quoted 2 lines
kernel/audit.c +2 -0
+                                       refcount_inc(&cont->refcount);
+                                       newcont = cont;
We can bail out of the loop here, yes?

Yes, that would be a performance improvement, but not functional bug, thanks. :-)

Quoted 35 lines

kernel/audit.c +24 -0

+                               } else {
+                                       rc = -ENOTUNIQ;
+                                       goto conterror;
+                               }
+                       }
+               if (!newcont) {
+                       newcont = kmalloc(sizeof(struct audit_cont), GFP_ATOMIC);
+                       if (newcont) {
+                               INIT_LIST_HEAD(&newcont->list);
+                               newcont->id = contid;
+                               get_task_struct(current);
+                               newcont->owner = current;
+                               refcount_set(&newcont->refcount, 1);
+                               list_add_rcu(&newcont->list, &audit_contid_hash[h]);
+                       } else {
+                               rc = -ENOMEM;
+                               goto conterror;
+                       }
+               }
+               task->audit->cont = newcont;
+               audit_cont_put(oldcont);
+conterror:
+               spin_unlock(&audit_contid_list_lock);
+       }
        task_unlock(task);
 
        if (!audit_enabled)

kernel/audit.h +0 -0

index 16bd03b88e0d..e4a31aa92dfe 100644
@@ -211,6 +211,14 @@ static inline int audit_hash_ino(u32 ino)
        return (ino & (AUDIT_INODE_BUCKETS-1));
 }

Quoted 15 lines

kernel/audit.h static inline int audit_hash_ino(u32 ino) · line 224

Quoted 11 lines

kernel/audit.h +8 -0

+#define AUDIT_CONTID_BUCKETS   32
+extern struct list_head audit_contid_hash[AUDIT_CONTID_BUCKETS];
+
+static inline int audit_hash_contid(u64 contid)
+{
+       return (contid & (AUDIT_CONTID_BUCKETS-1));
+}
+
 /* Indicates that audit should log the full pathname. */
 #define AUDIT_NAME_FULL -1

--
paul moore
www.paul-moore.com

- RGB

--
Richard Guy Briggs <[email protected]>
Sr. S/W Engineer, Kernel Security, Base Operating Systems Remote, Ottawa, Red Hat Canada
IRC: rgb, SunRaycer
Voice: +1.647.777.2635, Internal: (81) 32635

Neil Horman

2019-10-28 12:20 UTC #7 lore raw

Re: [PATCH ghak90 V7 04/21] audit: convert to contid list to check for orch/engine ownership

Quoted 59 lines · Richard Guy Briggs

Quoted 51 lines · Neil Horman
Quoted 50 lines · Richard Guy Briggs
Store the audit container identifier in a refcounted kernel object that is added to the master list of audit container identifiers. This will allow multiple container orchestrators/engines to work on the same machine without danger of inadvertantly re-using an existing identifier. It will also allow an orchestrator to inject a process into an existing container by checking if the original container owner is the one injecting the task. A hash table list is used to optimize searches.
Signed-off-by Richard Guy Briggs <[email protected]>
---
 include/linux/audit.h | 26 ++++++++++++++--
 kernel/audit.c        | 86 ++++++++++++++++++++++++++++++++++++++++++++++++---
 kernel/audit.h        |  8 +++++
 3 files changed, 112 insertions(+), 8 deletions(-)
include/linux/audit.h +11 -3
index f2e3b81f2942..e317807cdd3e 100644
@@ -95,10 +95,18 @@ struct audit_ntp_data {
 struct audit_ntp_data {};
 #endif
 
+struct audit_cont {
+	struct list_head	list;
+	u64			id;
+	struct task_struct	*owner;
+	refcount_t              refcount;
+	struct rcu_head         rcu;
+};
+
 struct audit_task_info {
 	kuid_t			loginuid;
 	unsigned int		sessionid;
-	u64			contid;
+	struct audit_cont	*cont;
 #ifdef CONFIG_AUDITSYSCALL
 	struct audit_context	*ctx;
 #endif
@@ -203,11 +211,15 @@ static inline unsigned int audit_get_sessionid(struct task_struct *tsk)
 
 static inline u64 audit_get_contid(struct task_struct *tsk)
 {
-	if (!tsk->audit)
+	if (!tsk->audit || !tsk->audit->cont)
 		return AUDIT_CID_UNSET;
-	return tsk->audit->contid;
+	return tsk->audit->cont->id;
 }
 
Quoted 6 lines
include/linux/audit.h static inline unsigned int audit_get_sessionid(struct task_struct *tsk) · line 222
Quoted 4 lines
include/linux/audit.h +4 -0
+extern struct audit_cont *audit_cont(struct task_struct *tsk);
+
+extern void audit_cont_put(struct audit_cont *cont);
+
I see that you manual increment this refcount at various call sites, why no corresponding audit_contid_hold function?

Quoted 9 lines

I was trying to avoid the get function due to having one site where I needed the pointer for later but didn't need a refcount to it so that I could release the refcount it if it was replaced by another cont object. A hold function would just contain one line that would call the refcount_inc(). If I did convert things over to a get function, it would hide some of this extra conditional code in the main calling function, but in one place I could just call put immediately to neutralize that unneeded refcount.

Ok, but this pattern:

static inline u64 __audit_contid_get(struct audit_cont *c) {
        return c->id;
}

audit_contid_get(struct audit_cont *c) {
refcount_hold(c)
return __audit_contid_get(c)
}

Squares that up, doesn't it? It gives you an internal non refcount holding version then to use.

Quoted 3 lines

Would you see any issue with that extra get/put refcount that would only happen in the case of changing a contid in a nesting situation?

No, I personally wouldn't have an issue with it, but the above would make it pretty readable I think

Quoted 209 lines

Quoted 194 lines

Neil

Quoted 192 lines

include/linux/audit.h +8 -0

 extern u32 audit_enabled;
 
 extern int audit_signal_info(int sig, struct task_struct *t);

@@ -277,6 +289,14 @@ static inline u64 audit_get_contid(struct task_struct *tsk)
 	return AUDIT_CID_UNSET;
 }
 
+static inline struct audit_cont *audit_cont(struct task_struct *tsk)
+{
+	return NULL;
+}
+
+static inline void audit_cont_put(struct audit_cont *cont)
+{ }
+
 #define audit_enabled AUDIT_OFF
 
 static inline int audit_signal_info(int sig, struct task_struct *t)

kernel/audit.c +81 -5

index a36ea57cbb61..ea0899130cc1 100644
@@ -137,6 +137,8 @@ struct audit_net {
 
 /* Hash for inode-based rules */
 struct list_head audit_inode_hash[AUDIT_INODE_BUCKETS];
+/* Hash for contid-based rules */
+struct list_head audit_contid_hash[AUDIT_CONTID_BUCKETS];
 
 static struct kmem_cache *audit_buffer_cache;

@@ -204,6 +206,8 @@ struct audit_reply {
 
 static struct kmem_cache *audit_task_cache;
 
+static DEFINE_SPINLOCK(audit_contid_list_lock);
+
 void __init audit_task_init(void)
 {
 	audit_task_cache = kmem_cache_create("audit_task",

@@ -231,7 +235,9 @@ int audit_alloc(struct task_struct *tsk)
 	}
 	info->loginuid = audit_get_loginuid(current);
 	info->sessionid = audit_get_sessionid(current);
-	info->contid = audit_get_contid(current);
+	info->cont = audit_cont(current);
+	if (info->cont)
+		refcount_inc(&info->cont->refcount);
 	tsk->audit = info;
 
 	ret = audit_alloc_syscall(tsk);

@@ -246,7 +252,7 @@ int audit_alloc(struct task_struct *tsk)
 struct audit_task_info init_struct_audit = {
 	.loginuid = INVALID_UID,
 	.sessionid = AUDIT_SID_UNSET,
-	.contid = AUDIT_CID_UNSET,
+	.cont = NULL,
 #ifdef CONFIG_AUDITSYSCALL
 	.ctx = NULL,
 #endif

@@ -266,6 +272,9 @@ void audit_free(struct task_struct *tsk)
 	/* Freeing the audit_task_info struct must be performed after
 	 * audit_log_exit() due to need for loginuid and sessionid.
 	 */
+	spin_lock(&audit_contid_list_lock); 
+	audit_cont_put(tsk->audit->cont);
+	spin_unlock(&audit_contid_list_lock); 
 	info = tsk->audit;
 	tsk->audit = NULL;
 	kmem_cache_free(audit_task_cache, info);

@@ -1657,6 +1666,9 @@ static int __init audit_init(void)
 	for (i = 0; i < AUDIT_INODE_BUCKETS; i++)
 		INIT_LIST_HEAD(&audit_inode_hash[i]);
 
+	for (i = 0; i < AUDIT_CONTID_BUCKETS; i++)
+		INIT_LIST_HEAD(&audit_contid_hash[i]);
+
 	mutex_init(&audit_cmd_mutex.lock);
 	audit_cmd_mutex.owner = NULL;

@@ -2356,6 +2368,32 @@ int audit_signal_info(int sig, struct task_struct *t)
 	return audit_signal_info_syscall(t);
 }
 
+struct audit_cont *audit_cont(struct task_struct *tsk)
+{
+	if (!tsk->audit || !tsk->audit->cont)
+		return NULL;
+	return tsk->audit->cont;
+}
+
+/* audit_contid_list_lock must be held by caller */
+void audit_cont_put(struct audit_cont *cont)
+{
+	if (!cont)
+		return;
+	if (refcount_dec_and_test(&cont->refcount)) {
+		put_task_struct(cont->owner);
+		list_del_rcu(&cont->list);
+		kfree_rcu(cont, rcu);
+	}
+}
+
+static struct task_struct *audit_cont_owner(struct task_struct *tsk)
+{
+	if (tsk->audit && tsk->audit->cont)
+		return tsk->audit->cont->owner;
+	return NULL;
+}
+
 /*
  * audit_set_contid - set current task's audit contid
  * @task: target task

@@ -2382,9 +2420,12 @@ int audit_set_contid(struct task_struct *task, u64 contid)
 	}
 	oldcontid = audit_get_contid(task);
 	read_lock(&tasklist_lock);
-	/* Don't allow the audit containerid to be unset */
+	/* Don't allow the contid to be unset */
 	if (!audit_contid_valid(contid))
 		rc = -EINVAL;
+	/* Don't allow the contid to be set to the same value again */
+	else if (contid == oldcontid) {
+		rc = -EADDRINUSE;
 	/* if we don't have caps, reject */
 	else if (!capable(CAP_AUDIT_CONTROL))
 		rc = -EPERM;

@@ -2397,8 +2438,43 @@ int audit_set_contid(struct task_struct *task, u64 contid)
 	else if (audit_contid_set(task))
 		rc = -ECHILD;
 	read_unlock(&tasklist_lock);
-	if (!rc)
-		task->audit->contid = contid;
+	if (!rc) {
+		struct audit_cont *oldcont = audit_cont(task);
+		struct audit_cont *cont = NULL;
+		struct audit_cont *newcont = NULL;
+		int h = audit_hash_contid(contid);
+
+		spin_lock(&audit_contid_list_lock);
+		list_for_each_entry_rcu(cont, &audit_contid_hash[h], list)
+			if (cont->id == contid) {
+				/* task injection to existing container */
+				if (current == cont->owner) {
+					refcount_inc(&cont->refcount);
+					newcont = cont;
+				} else {
+					rc = -ENOTUNIQ;
+					goto conterror;
+				}
+			}
+		if (!newcont) {
+			newcont = kmalloc(sizeof(struct audit_cont), GFP_ATOMIC);
+			if (newcont) {
+				INIT_LIST_HEAD(&newcont->list);
+				newcont->id = contid;
+				get_task_struct(current);
+				newcont->owner = current;
+				refcount_set(&newcont->refcount, 1);
+				list_add_rcu(&newcont->list, &audit_contid_hash[h]);
+			} else {
+				rc = -ENOMEM;
+				goto conterror;
+			}
+		}
+		task->audit->cont = newcont;
+		audit_cont_put(oldcont);
+conterror:
+		spin_unlock(&audit_contid_list_lock);
+	}
 	task_unlock(task);
 
 	if (!audit_enabled)

kernel/audit.h +8 -0

index 16bd03b88e0d..e4a31aa92dfe 100644
@@ -211,6 +211,14 @@ static inline int audit_hash_ino(u32 ino)
 	return (ino & (AUDIT_INODE_BUCKETS-1));
 }
 
+#define AUDIT_CONTID_BUCKETS	32
+extern struct list_head audit_contid_hash[AUDIT_CONTID_BUCKETS];
+
+static inline int audit_hash_contid(u64 contid)
+{
+	return (contid & (AUDIT_CONTID_BUCKETS-1));
+}
+
 /* Indicates that audit should log the full pathname. */
 #define AUDIT_NAME_FULL -1

Quoted 4 lines

Quoted 4 lines
Signature · 1 line
1.8.3.1

- RGB

--
Richard Guy Briggs <[email protected]>
Sr. S/W Engineer, Kernel Security, Base Operating Systems Remote, Ottawa, Red Hat Canada
IRC: rgb, SunRaycer
Voice: +1.647.777.2635, Internal: (81) 32635

Paul Moore

maintainer

2019-10-30 20:27 UTC #61 lore raw

Re: [PATCH ghak90 V7 20/21] audit: add capcontid to set contid outside init_user_ns

Quoted 12 lines · Richard Guy Briggs

Here's the note I had from that meeting:
- Eric raised the issue that using /proc is likely to get more and more hoary due to mount namespaces and suggested that we use a netlink
audit message (or a new syscall) to set the audit container identifier and since the loginuid is a similar type of operation, that it should be migrated over to a similar mechanism to get it away from /proc. Get could be done with a netlink audit message that triggers an audit log message to deliver the information. I'm reluctant to further pollute the syscall space if we can find another method. The netlink audit message makes sense since any audit-enabled service is likely to already have an audit socket open.

Thanks for the background info on the off-list meeting. I would encourage you to have discussions like this on-list in the future; if that isn't possible, hosting a public call would okay-ish, but a distant second.

At this point in time I'm not overly concerned about /proc completely going away in namespaces/containers that are full featured enough to host a container orchestrator. If/when reliance on procfs becomes an issue, we can look at alternate APIs, but given the importance of /proc to userspace (including to audit) I suspect we are going to see it persist for some time. I would prefer to see you to drop the audit container ID netlink API portions of this patchset and focus on the procfs API.

Also, for the record, removing the audit loginuid from procfs is not something to take lightly, if at all; like it or not, it's part of the kernel API.

Signature · 2 lines

paul moore
www.paul-moore.com

Paul Moore

maintainer

2019-10-30 20:32 UTC #38 lore raw

Re: [PATCH ghak90 V7 14/21] audit: contid check descendancy and nesting

Quoted 20 lines · Richard Guy Briggs

Quoted 11 lines · Paul Moore
Quoted 1 line · Richard Guy Briggs
?fixup! audit: convert to contid list to check for orch/engine ownership
?
Quoted 6 lines
Require the target task to be a descendant of the container orchestrator/engine.
You would only change the audit container ID from one set or inherited value to another if you were nesting containers.
Quoted 7 lines
Quoted 3 lines
If changing the contid, the container orchestrator/engine must be a descendant and not same orchestrator as the one that set it so it is not possible to change the contid of another orchestrator's container.
Did you mean to say that the container orchestrator must be an ancestor of the target, and the same orchestrator as the one that set the target process' audit container ID?

Quoted 8 lines

Not quite, the first half yes, but the second half: if it was already set by that orchestrator, it can't be set again. If it is a different orchestrator that is a descendant of the orchestrator that set it, then allow the action.
Quoted 1 line
Or maybe I'm missing something about what you are trying to do?
Does that help clarify it?

I think so, it's pretty much as you stated originally: "Require the target task to be a descendant of the container orchestrator/engine". It's possible I misread something in the patch, or got lost in all the ?fixup! patching. I'll take a closer look at the next revision of the patchset to make sure the code makes sense to me, but the logic seems reasonable.

Signature · 2 lines

paul moore
www.paul-moore.com

Richard Guy Briggs

author

2019-10-30 22:03 UTC #62 lore raw

Re: [PATCH ghak90 V7 20/21] audit: add capcontid to set contid outside init_user_ns

Quoted 14 lines · Paul Moore

Quoted 12 lines · Richard Guy Briggs
Here's the note I had from that meeting:
- Eric raised the issue that using /proc is likely to get more and more hoary due to mount namespaces and suggested that we use a netlink
audit message (or a new syscall) to set the audit container identifier and since the loginuid is a similar type of operation, that it should be migrated over to a similar mechanism to get it away from /proc. Get could be done with a netlink audit message that triggers an audit log message to deliver the information. I'm reluctant to further pollute the syscall space if we can find another method. The netlink audit message makes sense since any audit-enabled service is likely to already have an audit socket open.

Quoted 4 lines

Thanks for the background info on the off-list meeting. I would encourage you to have discussions like this on-list in the future; if that isn't possible, hosting a public call would okay-ish, but a distant second.

I'm still trying to get Eric's attention to get him to weigh in here and provide a more eloquent representation of his ideas and concerns. Some of it was related to CRIU(sp?) issues which we've already of which we've already seen similar concerns in namespace identifiers including the device identity to qualify it.

Quoted 8 lines

At this point in time I'm not overly concerned about /proc completely going away in namespaces/containers that are full featured enough to host a container orchestrator. If/when reliance on procfs becomes an issue, we can look at alternate APIs, but given the importance of /proc to userspace (including to audit) I suspect we are going to see it persist for some time. I would prefer to see you to drop the audit container ID netlink API portions of this patchset and focus on the procfs API.

I've already refactored the code to put the netlink bits at the end as completely optional pieces for completeness so they won't get in the way of the real substance of this patchset. The nesting depth and total number of containers checks have also been punted to the end of the patchset to get them out of the way of discussion.

Quoted 3 lines

Also, for the record, removing the audit loginuid from procfs is not something to take lightly, if at all; like it or not, it's part of the kernel API.

Oh, I'm quite aware of how important this change is and it was discussed with Steve Grubb who saw the concern and value of considering such a disruptive change. Removing proc support for auid/ses would be a long-term deprecation if accepted.

Really, I should have labelled the v7 patchset as RFC since there were so many new and disruptive ideas presented in it.

Quoted 2 lines

paul moore
www.paul-moore.com

- RGB

Paul Moore

maintainer

2019-10-31 13:59 UTC #63 lore raw

Re: [PATCH ghak90 V7 20/21] audit: add capcontid to set contid outside init_user_ns

Quoted 20 lines · Richard Guy Briggs

Quoted 14 lines · Paul Moore
Quoted 12 lines · Richard Guy Briggs
Here's the note I had from that meeting:
- Eric raised the issue that using /proc is likely to get more and more hoary due to mount namespaces and suggested that we use a netlink
audit message (or a new syscall) to set the audit container identifier and since the loginuid is a similar type of operation, that it should be migrated over to a similar mechanism to get it away from /proc. Get could be done with a netlink audit message that triggers an audit log message to deliver the information. I'm reluctant to further pollute the syscall space if we can find another method. The netlink audit message makes sense since any audit-enabled service is likely to already have an audit socket open.
Quoted 4 lines
Thanks for the background info on the off-list meeting. I would encourage you to have discussions like this on-list in the future; if that isn't possible, hosting a public call would okay-ish, but a distant second.

Quoted 5 lines

I'm still trying to get Eric's attention to get him to weigh in here and provide a more eloquent representation of his ideas and concerns. Some of it was related to CRIU(sp?) issues which we've already of which we've already seen similar concerns in namespace identifiers including the device identity to qualify it.

Okay, let's leave this open until we hear from Eric to see if he has any additional information, but it's going to need to be pretty compelling.

Quoted 9 lines

Quoted 8 lines
At this point in time I'm not overly concerned about /proc completely going away in namespaces/containers that are full featured enough to host a container orchestrator. If/when reliance on procfs becomes an issue, we can look at alternate APIs, but given the importance of /proc to userspace (including to audit) I suspect we are going to see it persist for some time. I would prefer to see you to drop the audit container ID netlink API portions of this patchset and focus on the procfs API.

Quoted 5 lines

I've already refactored the code to put the netlink bits at the end as completely optional pieces for completeness so they won't get in the way of the real substance of this patchset. The nesting depth and total number of containers checks have also been punted to the end of the patchset to get them out of the way of discussion.

That's fine, but if we do decide to drop the netlink API after hearing from Eric, please drop those from the patchset. Keeping the patchset small and focused should be a goal, and including rejected/dead patches (even at the end) doesn't help move towards that goal.

Quoted 8 lines

Quoted 3 lines
Also, for the record, removing the audit loginuid from procfs is not something to take lightly, if at all; like it or not, it's part of the kernel API.
Oh, I'm quite aware of how important this change is and it was discussed with Steve Grubb who saw the concern and value of considering such a disruptive change. Removing proc support for auid/ses would be a long-term deprecation if accepted.

As I mentioned, that comment was more "for the record" than you in particular; I know we've talked a lot over the years about kernel API stability and I'm confident you are aware of the pitfalls there. :)

Signature · 2 lines

paul moore
www.paul-moore.com

Steve Grubb

2019-10-31 14:50 UTC #64 lore raw

Re: [PATCH ghak90 V7 20/21] audit: add capcontid to set contid outside init_user_ns

Hello,

TLDR; I see a lot of benefit to switching away from procfs for setting auid & sessionid.

Quoted 3 lines · EDT Richard Guy Briggs

Quoted 3 lines
Also, for the record, removing the audit loginuid from procfs is not something to take lightly, if at all; like it or not, it's part of the kernel API.

It can also be used by tools to iterate processes related to one user or session. I use this in my Intrusion Prevention System which will land in audit user space at some point in the future.

Quoted 3 lines

Oh, I'm quite aware of how important this change is and it was discussed with Steve Grubb who saw the concern and value of considering such a disruptive change.

Actually, I advocated for syscall. I think the gist of Eric's idea was that / proc is the intersection of many nasty problems. By relying on it, you can't simplify the API to reduce the complexity. Almost no program actually needs access to /proc. ps does. But almost everything else is happy without it. For example, when you setup chroot jails, you may have to add /dev/random or / dev/null, but almost never /proc. What does force you to add /proc is any entry point daemon like sshd because it needs to set the loginuid. If we switch away from /proc, then sshd or crond will no longer /require/ procfs to be available which again simplifies the system design.

Quoted 2 lines

Removing proc support for auid/ses would be a
long-term deprecation if accepted.

It might need to just be turned into readonly for a while. But then again, perhaps auid and session should be part of /proc/<pid>/status? Maybe this can be done independently and ahead of the container work so there is a migration path for things that read auid or session. TBH, maybe this should have been done from the beginning.

-Steve

Paul Moore

maintainer

2019-10-31 23:37 UTC #65 lore raw

Re: [PATCH ghak90 V7 20/21] audit: add capcontid to set contid outside init_user_ns

Quoted 8 lines · Steve Grubb

Quoted 3 lines · EDT Richard Guy Briggs
Quoted 3 lines
Also, for the record, removing the audit loginuid from procfs is not something to take lightly, if at all; like it or not, it's part of the kernel API.
It can also be used by tools to iterate processes related to one user or session. I use this in my Intrusion Prevention System which will land in audit user space at some point in the future.

Let's try to stay focused on the audit container ID functionality; I fear if we start bringing in other unrelated issues we are never going to land these patches.

Quoted 7 lines

Quoted 3 lines
Oh, I'm quite aware of how important this change is and it was discussed with Steve Grubb who saw the concern and value of considering such a disruptive change.
Actually, I advocated for syscall. I think the gist of Eric's idea was that / proc is the intersection of many nasty problems. By relying on it, you can't simplify the API to reduce the complexity.

I guess complexity is relative in a sense, but reading and writing a number from a file in procfs seems awfully simple to me.

Quoted 7 lines

Almost no program actually needs
access to /proc. ps does. But almost everything else is happy without it. For example, when you setup chroot jails, you may have to add /dev/random or / dev/null, but almost never /proc. What does force you to add /proc is any entry point daemon like sshd because it needs to set the loginuid. If we switch away from /proc, then sshd or crond will no longer /require/ procfs to be available which again simplifies the system design.

It's not that simple, there are plenty of container use cases beyond ps which require procfs:

Most LSM aware applications require procfs to view and manage some LSM state (e.g. /proc/self/attr).

System containers, containers that run their own init/systemd/etc., require a working procfs.

Nested container orchestrators often run in system containers, which require a working procfs (see above).

I'm sure there are plenty others, but these are the ones that came immediately to mind.

Signature · 2 lines

paul moore
www.paul-moore.com

Duncan Roe

2019-11-01 01:02 UTC #66 lore raw

Re: [PATCH ghak90 V7 20/21] audit: add capcontid to set contid outside init_user_ns

In reply to Steve Grubb

Quoted 15 lines · Steve Grubb

Hello,
TLDR; I see a lot of benefit to switching away from procfs for setting auid & sessionid.
Quoted 3 lines · EDT Richard Guy Briggs
Quoted 3 lines
Also, for the record, removing the audit loginuid from procfs is not something to take lightly, if at all; like it or not, it's part of the kernel API.
It can also be used by tools to iterate processes related to one user or session. I use this in my Intrusion Prevention System which will land in audit user space at some point in the future.

Quoted 7 lines

Quoted 3 lines
Oh, I'm quite aware of how important this change is and it was discussed with Steve Grubb who saw the concern and value of considering such a disruptive change.
Actually, I advocated for syscall. I think the gist of Eric's idea was that / proc is the intersection of many nasty problems. By relying on it, you can't simplify the API to reduce the complexity. Almost no program actually needs

                                             ^^^^^^ ^^ ^^^^^^^ ^^^^^^^^ ^^^^^

Quoted 2 lines

access to /proc. ps does. But almost everything else is happy without it. For ^^^^^^ ^^ ^^^^^^ ^^ ^^^^^

Eh?? *top* needs /proc/ps, as do most of the programs in package procps-ng. Then there's lsof, pgrep (which doesn't fail but can't find anything) and even lilo (for Slackware ;)

Quoted 10 lines

example, when you setup chroot jails, you may have to add /dev/random or / dev/null, but almost never /proc. What does force you to add /proc is any entry point daemon like sshd because it needs to set the loginuid. If we switch away from /proc, then sshd or crond will no longer /require/ procfs to be available which again simplifies the system design.
Quoted 2 lines
Removing proc support for auid/ses would be a
long-term deprecation if accepted.

Quoted 8 lines

It might need to just be turned into readonly for a while. But then again, perhaps auid and session should be part of /proc/<pid>/status? Maybe this can be done independently and ahead of the container work so there is a migration path for things that read auid or session. TBH, maybe this should have been done from the beginning.
-Steve

Cheers ... Duncan.

Richard Guy Briggs

author

2019-11-01 15:09 UTC #67 lore raw

Re: [PATCH ghak90 V7 20/21] audit: add capcontid to set contid outside init_user_ns

In reply to Steve Grubb

Quoted 28 lines · Steve Grubb

Hello,
TLDR; I see a lot of benefit to switching away from procfs for setting auid & sessionid.
Quoted 3 lines · EDT Richard Guy Briggs
Quoted 3 lines
Also, for the record, removing the audit loginuid from procfs is not something to take lightly, if at all; like it or not, it's part of the kernel API.
It can also be used by tools to iterate processes related to one user or session. I use this in my Intrusion Prevention System which will land in audit user space at some point in the future.
Quoted 3 lines
Oh, I'm quite aware of how important this change is and it was discussed with Steve Grubb who saw the concern and value of considering such a disruptive change.
Actually, I advocated for syscall. I think the gist of Eric's idea was that / proc is the intersection of many nasty problems. By relying on it, you can't simplify the API to reduce the complexity. Almost no program actually needs access to /proc. ps does. But almost everything else is happy without it. For example, when you setup chroot jails, you may have to add /dev/random or / dev/null, but almost never /proc. What does force you to add /proc is any entry point daemon like sshd because it needs to set the loginuid. If we switch away from /proc, then sshd or crond will no longer /require/ procfs to be available which again simplifies the system design.

Quoted 8 lines

Quoted 2 lines
Removing proc support for auid/ses would be a
long-term deprecation if accepted.
It might need to just be turned into readonly for a while. But then again, perhaps auid and session should be part of /proc/<pid>/status? Maybe this can be done independently and ahead of the container work so there is a migration path for things that read auid or session. TBH, maybe this should have been done from the beginning.

How about making loginuid/contid/capcontid writable only via netlink but still provide the /proc interface for reading? Deprecation of proc can be left as a decision for later. This way sshd/crond/getty don't need /proc, but the info is still there for tools that want to read it.

Quoted 1 line

-Steve

- RGB

Steve Grubb

2019-11-01 15:13 UTC #68 lore raw

Re: [PATCH ghak90 V7 20/21] audit: add capcontid to set contid outside init_user_ns

Quoted 42 lines · EDT Richard Guy Briggs

Quoted 34 lines · Steve Grubb
Hello,
TLDR; I see a lot of benefit to switching away from procfs for setting auid & sessionid.
Quoted 4 lines · EDT Richard Guy Briggs
Quoted 4 lines
Also, for the record, removing the audit loginuid from procfs is not something to take lightly, if at all; like it or not, it's part of the
kernel API.
It can also be used by tools to iterate processes related to one user or session. I use this in my Intrusion Prevention System which will land in audit user space at some point in the future.
Quoted 4 lines
Oh, I'm quite aware of how important this change is and it was discussed
with Steve Grubb who saw the concern and value of considering such a disruptive change.
Actually, I advocated for syscall. I think the gist of Eric's idea was that / proc is the intersection of many nasty problems. By relying on it, you can't simplify the API to reduce the complexity. Almost no program actually needs access to /proc. ps does. But almost everything else is happy without it. For example, when you setup chroot jails, you may have to add /dev/random or / dev/null, but almost never /proc. What does force you to add /proc is any entry point daemon like sshd because it needs to set the loginuid. If we switch away from /proc, then sshd or crond will no longer /require/ procfs to be available which again simplifies the system design.
Quoted 2 lines
Removing proc support for auid/ses would be a
long-term deprecation if accepted.
Quoted 6 lines
It might need to just be turned into readonly for a while. But then again,
perhaps auid and session should be part of /proc/<pid>/status? Maybe this can be done independently and ahead of the container work so there is a migration path for things that read auid or session. TBH, maybe this should have been done from the beginning.

Quoted 4 lines

How about making loginuid/contid/capcontid writable only via netlink but still provide the /proc interface for reading? Deprecation of proc can be left as a decision for later. This way sshd/crond/getty don't need /proc, but the info is still there for tools that want to read it.

This also sounds good to me. But I still think loginuid and audit sessionid should get written in /proc/<pid>/status so that all process information is consolidated in one place.

-Steve

Richard Guy Briggs

author

2019-11-01 15:21 UTC #69 lore raw

Re: [PATCH ghak90 V7 20/21] audit: add capcontid to set contid outside init_user_ns

Quoted 43 lines · Steve Grubb

Quoted 42 lines · EDT Richard Guy Briggs
Quoted 40 lines · Steve Grubb
Hello,
TLDR; I see a lot of benefit to switching away from procfs for setting auid & sessionid.
Quoted 4 lines · EDT Richard Guy Briggs
Quoted 4 lines
Also, for the record, removing the audit loginuid from procfs is not something to take lightly, if at all; like it or not, it's part of the
kernel API.
It can also be used by tools to iterate processes related to one user or session. I use this in my Intrusion Prevention System which will land in audit user space at some point in the future.
Quoted 4 lines
Oh, I'm quite aware of how important this change is and it was discussed
with Steve Grubb who saw the concern and value of considering such a disruptive change.
Actually, I advocated for syscall. I think the gist of Eric's idea was that / proc is the intersection of many nasty problems. By relying on it, you can't simplify the API to reduce the complexity. Almost no program actually needs access to /proc. ps does. But almost everything else is happy without it. For example, when you setup chroot jails, you may have to add /dev/random or / dev/null, but almost never /proc. What does force you to add /proc is any entry point daemon like sshd because it needs to set the loginuid. If we switch away from /proc, then sshd or crond will no longer /require/ procfs to be available which again simplifies the system design.
Quoted 2 lines
Removing proc support for auid/ses would be a
long-term deprecation if accepted.
It might need to just be turned into readonly for a while. But then again,
perhaps auid and session should be part of /proc/<pid>/status? Maybe this can be done independently and ahead of the container work so there is a migration path for things that read auid or session. TBH, maybe this should have been done from the beginning.

Quoted 8 lines

Quoted 4 lines
How about making loginuid/contid/capcontid writable only via netlink but still provide the /proc interface for reading? Deprecation of proc can be left as a decision for later. This way sshd/crond/getty don't need /proc, but the info is still there for tools that want to read it.
This also sounds good to me. But I still think loginuid and audit sessionid should get written in /proc/<pid>/status so that all process information is consolidated in one place.

I don't have a problem adding auid/sessionid to /proc/<pid>/status with other related information, but it is disruptive to deprecate the existing interface which could be a seperate step.

Quoted 1 line

-Steve

- RGB

--
Richard Guy Briggs <[email protected]>
Sr. S/W Engineer, Kernel Security, Base Operating Systems Remote, Ottawa, Red Hat Canada
IRC: rgb, SunRaycer
Voice: +1.647.777.2635, Internal: (81) 32635

Paul Moore

maintainer

2019-11-01 16:22 UTC #70 lore raw

Re: [PATCH ghak90 V7 20/21] audit: add capcontid to set contid outside init_user_ns

Quoted 38 lines · Richard Guy Briggs

Quoted 28 lines · Steve Grubb
Hello,
TLDR; I see a lot of benefit to switching away from procfs for setting auid & sessionid.
Quoted 3 lines · EDT Richard Guy Briggs
Quoted 3 lines
Also, for the record, removing the audit loginuid from procfs is not something to take lightly, if at all; like it or not, it's part of the kernel API.
It can also be used by tools to iterate processes related to one user or session. I use this in my Intrusion Prevention System which will land in audit user space at some point in the future.
Quoted 3 lines
Oh, I'm quite aware of how important this change is and it was discussed with Steve Grubb who saw the concern and value of considering such a disruptive change.
Actually, I advocated for syscall. I think the gist of Eric's idea was that / proc is the intersection of many nasty problems. By relying on it, you can't simplify the API to reduce the complexity. Almost no program actually needs access to /proc. ps does. But almost everything else is happy without it. For example, when you setup chroot jails, you may have to add /dev/random or / dev/null, but almost never /proc. What does force you to add /proc is any entry point daemon like sshd because it needs to set the loginuid. If we switch away from /proc, then sshd or crond will no longer /require/ procfs to be available which again simplifies the system design.
Quoted 8 lines
Quoted 2 lines
Removing proc support for auid/ses would be a
long-term deprecation if accepted.
It might need to just be turned into readonly for a while. But then again, perhaps auid and session should be part of /proc/<pid>/status? Maybe this can be done independently and ahead of the container work so there is a migration path for things that read auid or session. TBH, maybe this should have been done from the beginning.

Quoted 4 lines

How about making loginuid/contid/capcontid writable only via netlink but still provide the /proc interface for reading? Deprecation of proc can be left as a decision for later. This way sshd/crond/getty don't need /proc, but the info is still there for tools that want to read it.

Just so there is no confusion for the next patchset: I think it would be a mistake to include any changes to loginuid in your next patchset, even as a "RFC" at the end. Also, barring some shocking comments from Eric relating to the imminent death of /proc in containers, I think it would also be a mistake to include the netlink API.

Let's keep it small and focused :)

Signature · 2 lines

paul moore
www.paul-moore.com

Paul Moore

maintainer

2019-11-08 17:41 UTC #27 lore raw

Re: [PATCH ghak90 V7 08/21] audit: add contid support for signalling the audit daemon

Quoted 141 lines · Richard Guy Briggs

Quoted 118 lines · Paul Moore

Quoted 109 lines · Richard Guy Briggs

Add audit container identifier support to the action of signalling the audit daemon.

Signed-off-by Richard Guy Briggs <[email protected]>

---
 include/linux/audit.h       |  7 +++++++
 include/uapi/linux/audit.h  |  1 +
 kernel/audit.c              | 28 ++++++++++++++++++++++++++++
 kernel/audit.h              |  1 +
 security/selinux/nlmsgtab.c |  1 +
 5 files changed, 38 insertions(+)

include/linux/audit.h +7 -0

index 0c18d8e30620..7b640c4da4ee 100644
@@ -23,6 +23,13 @@ struct audit_sig_info {
        char            ctx[0];
 };
 
+struct audit_sig_info2 {
+       uid_t           uid;
+       pid_t           pid;
+       u64             cid;
+       char            ctx[0];
+};
+
 struct audit_buffer;
 struct audit_context;
 struct inode;

include/uapi/linux/audit.h +1 -0

index 4ed080f28b47..693ec6e0288b 100644
@@ -72,6 +72,7 @@
 #define AUDIT_SET_FEATURE      1018    /* Turn an audit feature on or off */
 #define AUDIT_GET_FEATURE      1019    /* Get which features are enabled */
 #define AUDIT_CONTAINER_OP     1020    /* Define the container id and info */
+#define AUDIT_SIGNAL_INFO2     1021    /* Get info auditd signal sender */
 
 #define AUDIT_FIRST_USER_MSG   1100    /* Userspace messages mostly uninteresting to kernel */
 #define AUDIT_USER_AVC         1107    /* We filter this differently */

kernel/audit.c +27 -0

index adfb3e6a7f0c..df3db29f5a8a 100644
@@ -125,6 +125,7 @@ struct audit_net {
 kuid_t         audit_sig_uid = INVALID_UID;
 pid_t          audit_sig_pid = -1;
 u32            audit_sig_sid = 0;
+u64            audit_sig_cid = AUDIT_CID_UNSET;
 
 /* Records can be lost in several ways:
    0) [suppressed in audit_alloc]

@@ -1094,6 +1095,7 @@ static int audit_netlink_ok(struct sk_buff *skb, u16 msg_type)
        case AUDIT_ADD_RULE:
        case AUDIT_DEL_RULE:
        case AUDIT_SIGNAL_INFO:
+       case AUDIT_SIGNAL_INFO2:
        case AUDIT_TTY_GET:
        case AUDIT_TTY_SET:
        case AUDIT_TRIM:

@@ -1257,6 +1259,7 @@ static int audit_receive_msg(struct sk_buff *skb, struct nlmsghdr *nlh)
        struct audit_buffer     *ab;
        u16                     msg_type = nlh->nlmsg_type;
        struct audit_sig_info   *sig_data;
+       struct audit_sig_info2  *sig_data2;
        char                    *ctx = NULL;
        u32                     len;

@@ -1516,6 +1519,30 @@ static int audit_receive_msg(struct sk_buff *skb, struct nlmsghdr *nlh)
                                 sig_data, sizeof(*sig_data) + len);
                kfree(sig_data);
                break;
+       case AUDIT_SIGNAL_INFO2:
+               len = 0;
+               if (audit_sig_sid) {
+                       err = security_secid_to_secctx(audit_sig_sid, &ctx, &len);
+                       if (err)
+                               return err;
+               }
+               sig_data2 = kmalloc(sizeof(*sig_data2) + len, GFP_KERNEL);
+               if (!sig_data2) {
+                       if (audit_sig_sid)
+                               security_release_secctx(ctx, len);
+                       return -ENOMEM;
+               }
+               sig_data2->uid = from_kuid(&init_user_ns, audit_sig_uid);
+               sig_data2->pid = audit_sig_pid;
+               if (audit_sig_sid) {
+                       memcpy(sig_data2->ctx, ctx, len);
+                       security_release_secctx(ctx, len);
+               }
+               sig_data2->cid = audit_sig_cid;
+               audit_send_reply(skb, seq, AUDIT_SIGNAL_INFO2, 0, 0,
+                                sig_data2, sizeof(*sig_data2) + len);
+               kfree(sig_data2);
+               break;
        case AUDIT_TTY_GET: {
                struct audit_tty_status s;
                unsigned int t;

kernel/audit.c int audit_signal_info(int sig, struct task_struct *t) · line 2415

Quoted 5 lines

kernel/audit.c +1 -0

@@ -2384,6 +2411,7 @@ int audit_signal_info(int sig, struct task_struct *t)
                else
                        audit_sig_uid = uid;
                security_task_getsecid(current, &audit_sig_sid);
+               audit_sig_cid = audit_get_contid(current);
        }

I've been wondering something as I've been working my way through these patches and this patch seems like a good spot to discuss this

Quoted 8 lines

... Now that we have the concept of an audit container ID "lifetime" in the kernel, when do we consider the ID gone? Is it when the last process in the container exits, or is it when we generate the last audit record which could possibly contain the audit container ID? This patch would appear to support the former, but if we wanted the latter we would need to grab a reference to the audit container ID struct so it wouldn't "die" on us before we could emit the signal info record.

I've got a bit of a dilemma with this one...

Quoted 11 lines

So the issue raised above I don't think is a problem. However, patch 18 (which wasn't reviewed because it was a patch to a number of preceeding patches) changes the reporting approach to give a chain of nested contids which isn't reflected in the same level of reporting for the audit signal patch/mechanism. Solving this is a bit more complex. We could have the audit signal internal caching store a pointer to the relevant container object and bump its refcount to ensure it doesn't vanish until we are done with it, but the audit signal info binary record format already has a variable length due to the selinux context at the end of that struct and adding a second variable length element to it would make it more complicated (but not impossible) to handle.

[side note #1: Sorry for the delay, travel/conferences have limited my time and I felt we needed to focus on the larger issue of netlink/procfs first. Back to the other topics ...]

[side note #2: I just realized that one can shorten "audit container ID" to ACID, I think that's going to be my favorite realization of the day :)]

My concern wasn't really about the availability of the data, since as you said, it is copied into the record buffer, but rather a delay between when the audit container ID (ACID) disappears from the tracking/list db in the kernel to when it is emitted in an audit record from the kernel. During this time is seems like it could be possible for the orchestrator to reintroduce the same ACID value and if someone is not taking into account the full audit history they could get confused (the full audit history should show the proper creation/destruction events in the correct order). Ultimately I'm not sure it is a major issue, and fixing it is likely to be really ugly, but I think it would be good to add some comments in the code regarding what we guarantee as far as ACID lifetimes are concerned.

Signature · 2 lines

paul moore
www.paul-moore.com

Paul Moore

maintainer

2019-11-08 17:49 UTC #18 lore raw

Re: [PATCH ghak90 V7 06/21] audit: contid limit of 32k imposed to avoid DoS

Quoted 48 lines · Richard Guy Briggs

Quoted 47 lines · Paul Moore
Quoted 14 lines · Neil Horman
Quoted 13 lines · Richard Guy Briggs
Set an arbitrary limit on the number of audit container identifiers to limit abuse.
Signed-off-by Richard Guy Briggs <[email protected]>
---
 kernel/audit.c | 8 ++++++++
 kernel/audit.h | 4 ++++
 2 files changed, 12 insertions(+)
kernel/audit.c +0 -0
index 53d13d638c63..329916534dd2 100644
...
Quoted 28 lines
Quoted 13 lines
kernel/audit.c +1 -0
@@ -2465,6 +2472,7 @@ int audit_set_contid(struct task_struct *task, u64 contid)
                              newcont->owner = current;
                              refcount_set(&newcont->refcount, 1);
                              list_add_rcu(&newcont->list, &audit_contid_hash[h]);
+                             audit_contid_count++;
                      } else {
                              rc = -ENOMEM;
                              goto conterror;
kernel/audit.h +0 -0
index 162de8366b32..543f1334ba47 100644
kernel/audit.h static inline int audit_hash_contid(u64 contid) · line 225
Quoted 7 lines
kernel/audit.h +4 -0
@@ -219,6 +219,10 @@ static inline int audit_hash_contid(u64 contid)
      return (contid & (AUDIT_CONTID_BUCKETS-1));
 }
 
+extern int audit_contid_count;
+
+#define AUDIT_CONTID_COUNT   1 << 16
+
Just to ask the question, since it wasn't clear in the changelog, what abuse are you avoiding here? Ostensibly you should be able to create as many container ids as you have space for, and the simple creation of container ids doesn't seem like the resource strain I would be concerned about here, given that an orchestrator can still create as many containers as the system will otherwise allow, which will consume significantly more ram/disk/etc.

Quoted 8 lines

Quoted 6 lines
I've got a similar question. Up to this point in the patchset, there is a potential issue of hash bucket chain lengths and traversing them with a spinlock held, but it seems like we shouldn't be putting an arbitrary limit on audit container IDs unless we have a good reason for it. If for some reason we do want to enforce a limit, it should probably be a tunable value like a sysctl, or similar.
Can you separate and clarify the concerns here?

"Why are you doing this?" is about as simple as I can pose the question.

Quoted 4 lines

I plan to move this patch to the end of the patchset and make it optional, possibly adding a tuning mechanism. Like the migration from /proc to netlink for loginuid/sessionid/contid/capcontid, this was Eric Biederman's concern and suggested mitigation.

Okay, let's just drop it. I *really* don't like this approach of tossing questionable stuff at the end of the patchset; I get why you are doing it, but I think we really need to focus on keeping this changeset small. If the number of ACIDs (heh) become unwieldy the right solution is to improve the algorithms/structures, if we can't do that for some reason, *then* we can fall back to a limiting knob in a latter release.

Quoted 4 lines

As for the first issue of the bucket chain length traversal while holding the list spin-lock, would you prefer to use the rcu lock to traverse the list and then only hold the spin-lock when modifying the list, and possibly even make the spin-lock more fine-grained per list?

Until we have a better idea of how this is going to be used, I think it's okay for now. It's also internal to the kernel so we can change it at any time. My comments about the locking/structs was only to try and think of some reason why one might want to limit the number of ACIDs since neither you or Eric provided any reasoning that I could see.

Signature · 2 lines

paul moore
www.paul-moore.com

Paul Moore

maintainer

2019-11-08 18:26 UTC #10 lore raw

Re: [PATCH ghak90 V7 04/21] audit: convert to contid list to check for orch/engine ownership

Quoted 28 lines · Richard Guy Briggs

Quoted 16 lines · Paul Moore
Quoted 8 lines · Richard Guy Briggs
Store the audit container identifier in a refcounted kernel object that is added to the master list of audit container identifiers. This will allow multiple container orchestrators/engines to work on the same machine without danger of inadvertantly re-using an existing identifier. It will also allow an orchestrator to inject a process into an existing container by checking if the original container owner is the one injecting the task. A hash table list is used to optimize searches.
Quoted 6 lines
Signed-off-by Richard Guy Briggs <[email protected]>
---
 include/linux/audit.h | 26 ++++++++++++++--
 kernel/audit.c        | 86 ++++++++++++++++++++++++++++++++++++++++++++++++---
 kernel/audit.h        |  8 +++++
 3 files changed, 112 insertions(+), 8 deletions(-)
Quoted 6 lines
One general comment before we go off into the weeds on this ... I can understand why you wanted to keep this patch separate from the earlier patches, but as we get closer to having mergeable code this should get folded into the previous patches. For example, there shouldn't be a change in audit_task_info where you change the contid field from a u64 to struct pointer, it should be a struct pointer from the start.
I should have marked this patchset as RFC even though it was v7 due to a lot of new ideas/code that was added with uncertainties needing comment and direction.

Quoted 5 lines

Quoted 2 lines
It's also disappointing that idr appears to only be for 32-bit ID values, if we had a 64-bit idr I think we could simplify this greatly.
Perhaps. I do still see value in letting the orchestrator choose the value.

Agreed. I was just thinking out loud that it seems like much of what we need could be a generic library mechanism similar to, but not quite like, the existing idr code.

Quoted 20 lines

Quoted 8 lines
Quoted 8 lines
include/linux/audit.h +0 -0
index f2e3b81f2942..e317807cdd3e 100644
@@ -95,10 +95,18 @@ struct audit_ntp_data {
 struct audit_ntp_data {};
 #endif
 
Quoted 11 lines
include/linux/audit.h struct audit_ntp_data { · line 104
Quoted 7 lines
include/linux/audit.h +7 -0
+struct audit_cont {
+       struct list_head        list;
+       u64                     id;
+       struct task_struct      *owner;
+       refcount_t              refcount;
+       struct rcu_head         rcu;
+};
It seems as though in most of the code you are using "contid", any reason why didn't stick with that naming scheme here, e.g. "struct audit_contid"?

Quoted 3 lines

I was using contid to refer to the value itself and cont to refer to the refcounted object. I find cont a bit too terse, so I'm still thinking of changing it. Perhaps contobj?

Yes, just "cont" is a bit too ambiguous considering we have both integer values and structures being passed around. Whatever you decide on, a common base with separate suffixes seems like a good idea.

FWIW, I still think the "audit container ID" : "ACID" thing is kinda funny ;)

Quoted 18 lines

Quoted 17 lines
include/linux/audit.h static inline unsigned int audit_get_sessionid(struct task_struct *tsk) · line 217
Quoted 10 lines
include/linux/audit.h +2 -2
@@ -203,11 +211,15 @@ static inline unsigned int audit_get_sessionid(struct task_struct *tsk)
 
 static inline u64 audit_get_contid(struct task_struct *tsk)
 {
-       if (!tsk->audit)
+       if (!tsk->audit || !tsk->audit->cont)
                return AUDIT_CID_UNSET;
-       return tsk->audit->contid;
+       return tsk->audit->cont->id;
 }
Assuming for a moment that we implement an audit_contid_get() (see Neil's comment as well as mine below), we probably need to name this something different so we don't all lose our minds when we read this code. On the plus side we can probably preface it with an underscore since it is a static, in which case _audit_contid_get() might be okay, but I'm open to suggestions.

Quoted 1 line

I'm fine with the "_" prefix, can you point to precedent or convention?

Generally kernel functions which are "special"/private/unsafe/etc. have a one, or two, underscore prefix. If you don't want to add the prefix, that's fine, but please change the name as mentioned previously.

Quoted 12 lines

Quoted 11 lines
include/linux/audit.h int audit_alloc(struct task_struct *tsk) · line 240
Quoted 8 lines
include/linux/audit.h +3 -1
@@ -231,7 +235,9 @@ int audit_alloc(struct task_struct *tsk)
        }
        info->loginuid = audit_get_loginuid(current);
        info->sessionid = audit_get_sessionid(current);
-       info->contid = audit_get_contid(current);
+       info->cont = audit_cont(current);
+       if (info->cont)
+               refcount_inc(&info->cont->refcount);
See the other comments about a "get" function, but I think we need a RCU read lock around the above, no?

Quoted 2 lines

The rcu read lock is to protect the list rather than the cont object itself, the latter of which is protected by its refcount.

What protects you from info->cont going away between when you fetch the pointer via audit_cont() to when you dereference it in refcount_inc()?

Quoted 12 lines

Quoted 11 lines
include/linux/audit.h int audit_set_contid(struct task_struct *task, u64 contid) · line 2442
Quoted 8 lines
include/linux/audit.h +2 -2
@@ -2397,8 +2438,43 @@ int audit_set_contid(struct task_struct *task, u64 contid)
        else if (audit_contid_set(task))
                rc = -ECHILD;
        read_unlock(&tasklist_lock);
-       if (!rc)
-               task->audit->contid = contid;
+       if (!rc) {
+               struct audit_cont *oldcont = audit_cont(task);
Previously we held the tasklist_lock to protect the audit container ID associated with the struct, should we still be holding it here?

Quoted 2 lines

We held the tasklist_lock to protect access to the target task's child/parent/thread relationships.

What protects us in the case of simultaneous calls to audit_set_contid()?

Quoted 9 lines

Quoted 4 lines
Regardless, I worry that the lock dependencies between the tasklist_lock and the audit_contid_list_lock are going to be tricky. It might be nice to document the relationship in a comment up near where you declare audit_contid_list_lock.
I don't think there should be a conflict between the two.
The contid_list_lock doesn't care if the cont object is associated to a particular task.

Please document the relationship between the two, I worry we could easily run into lockdep problems without a clearly defined ordering.

Quoted 14 lines

Quoted 10 lines

include/linux/audit.h int audit_set_contid(struct task_struct *task, u64 contid) · line 2451

Quoted 9 lines

include/linux/audit.h +9 -0

+               struct audit_cont *cont = NULL;
+               struct audit_cont *newcont = NULL;
+               int h = audit_hash_contid(contid);
+
+               spin_lock(&audit_contid_list_lock);
+               list_for_each_entry_rcu(cont, &audit_contid_hash[h], list)
+                       if (cont->id == contid) {
+                               /* task injection to existing container */
+                               if (current == cont->owner) {

Quoted 3 lines

I understand the desire to limit a given audit container ID to the orchestrator that created it, but are we certain that we can track audit container ID "ownership" via a single instance of a task_struct?

Quoted 7 lines

Are you suggesting that a task_struct representing a task may be replaced for a specific task? I don't believe that will ever happen.
Quoted 2 lines
What happens when the orchestrator stops/restarts/crashes? Do we
even care?
Reap all of its containers?

These were genuine questions, I'm not suggesting anything in particular, I'm just curious about how we handle an orchestrator that isn't continuously running ... is this possible? Do we care?

Signature · 2 lines

paul moore
www.paul-moore.com

Richard Guy Briggs

author

2019-12-17 18:45 UTC #19 lore raw

Re: [PATCH ghak90 V7 06/21] audit: contid limit of 32k imposed to avoid DoS