audit: implement container identifier

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

/**
+ * audit_set_contid - set current task's audit contid
+ * @contid: contid value
+ *
+ * Returns 0 on success, -EPERM on permission failure.
+ *
+ * Called (set) from fs/proc/base.c::proc_contid_write().
+ */

kernel/audit.c +25 -0

+int audit_set_contid(struct task_struct *task, u64 contid)
+{
+	u64 oldcontid;
+	int rc = 0;
+	struct audit_buffer *ab;
+	uid_t uid;
+	struct tty_struct *tty;
+	char comm[sizeof(current->comm)];
+
+	task_lock(task);
+	/* Can't set if audit disabled */
+	if (!task->audit) {
+		task_unlock(task);
+		return -ENOPROTOOPT;
+	}
+	oldcontid = audit_get_contid(task);
+	read_lock(&tasklist_lock);
+	/* Don't allow the audit containerid to be unset */
+	if (!audit_contid_valid(contid))
+		rc = -EINVAL;
+	/* if we don't have caps, reject */
+	else if (!capable(CAP_AUDIT_CONTROL))
+		rc = -EPERM;
+	/* if task has children or is not single-threaded, deny */
+	else if (!list_empty(&task->children))

Quoted 2 lines · Richard Guy Briggs

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

...

Quoted 41 lines

/**
+ * audit_set_contid - set current task's audit contid
+ * @contid: contid value
+ *
+ * Returns 0 on success, -EPERM on permission failure.
+ *
+ * Called (set) from fs/proc/base.c::proc_contid_write().
+ */

kernel/audit.c +33 -0

+int audit_set_contid(struct task_struct *task, u64 contid)
+{
+     u64 oldcontid;
+     int rc = 0;
+     struct audit_buffer *ab;
+     uid_t uid;
+     struct tty_struct *tty;
+     char comm[sizeof(current->comm)];
+
+     task_lock(task);
+     /* Can't set if audit disabled */
+     if (!task->audit) {
+             task_unlock(task);
+             return -ENOPROTOOPT;
+     }
+     oldcontid = audit_get_contid(task);
+     read_lock(&tasklist_lock);
+     /* Don't allow the audit containerid to be unset */
+     if (!audit_contid_valid(contid))
+             rc = -EINVAL;
+     /* if we don't have caps, reject */
+     else if (!capable(CAP_AUDIT_CONTROL))
+             rc = -EPERM;
+     /* if task has children or is not single-threaded, deny */
+     else if (!list_empty(&task->children))
+             rc = -EBUSY;
+     else if (!(thread_group_leader(task) && thread_group_empty(task)))
+             rc = -EALREADY;
+     read_unlock(&tasklist_lock);
+     if (!rc)
+             task->audit->contid = contid;
+     task_unlock(task);
+

Review

Quoted 2 lines

(quoted diff) +2 -0

+     if (!audit_enabled)
+             return rc;

...but it is allowed to change it (assuming capable(CAP_AUDIT_CONTROL), of course)? Seems like this might be more immediately useful since we still live in the world of majority privileged containers if we didn't allow changing it, in addition to un-setting it.

From a practical perspective, un-setting the audit container ID is

Quoted 48 lines · Tycho Andersen

Quoted 2 lines · Richard Guy Briggs

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

...

Quoted 41 lines

/**
+ * audit_set_contid - set current task's audit contid
+ * @contid: contid value
+ *
+ * Returns 0 on success, -EPERM on permission failure.
+ *
+ * Called (set) from fs/proc/base.c::proc_contid_write().
+ */

kernel/audit.c +33 -0

+int audit_set_contid(struct task_struct *task, u64 contid)
+{
+     u64 oldcontid;
+     int rc = 0;
+     struct audit_buffer *ab;
+     uid_t uid;
+     struct tty_struct *tty;
+     char comm[sizeof(current->comm)];
+
+     task_lock(task);
+     /* Can't set if audit disabled */
+     if (!task->audit) {
+             task_unlock(task);
+             return -ENOPROTOOPT;
+     }
+     oldcontid = audit_get_contid(task);
+     read_lock(&tasklist_lock);
+     /* Don't allow the audit containerid to be unset */
+     if (!audit_contid_valid(contid))
+             rc = -EINVAL;
+     /* if we don't have caps, reject */
+     else if (!capable(CAP_AUDIT_CONTROL))
+             rc = -EPERM;
+     /* if task has children or is not single-threaded, deny */
+     else if (!list_empty(&task->children))
+             rc = -EBUSY;
+     else if (!(thread_group_leader(task) && thread_group_empty(task)))
+             rc = -EALREADY;
+     read_unlock(&tasklist_lock);
+     if (!rc)
+             task->audit->contid = contid;
+     task_unlock(task);
+

Quoted 8 lines

Review

Quoted 2 lines

(quoted diff) +2 -0

+     if (!audit_enabled)
+             return rc;

...but it is allowed to change it (assuming capable(CAP_AUDIT_CONTROL), of course)? Seems like this might be more immediately useful since we still live in the world of majority privileged containers if we didn't allow changing it, in addition to un-setting it.

The idea is that only container orchestrators should be able to set/modify the audit container ID, and since setting the audit container ID can have a significant effect on the records captured (and their routing to multiple daemons when we get there) modifying the audit container ID is akin to modifying the audit configuration which is why it is gated by CAP_AUDIT_CONTROL. The current thinking is that you would only change the audit container ID from one set/inherited value to another if you were nesting containers, in which case the nested container orchestrator would need to be granted CAP_AUDIT_CONTROL (which everyone to date seems to agree is a workable compromise).

Quoted 58 lines · Paul Moore

Quoted 48 lines · Tycho Andersen

Quoted 2 lines · Richard Guy Briggs

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

...

Quoted 41 lines

/**
+ * audit_set_contid - set current task's audit contid
+ * @contid: contid value
+ *
+ * Returns 0 on success, -EPERM on permission failure.
+ *
+ * Called (set) from fs/proc/base.c::proc_contid_write().
+ */

kernel/audit.c +33 -0

+int audit_set_contid(struct task_struct *task, u64 contid)
+{
+     u64 oldcontid;
+     int rc = 0;
+     struct audit_buffer *ab;
+     uid_t uid;
+     struct tty_struct *tty;
+     char comm[sizeof(current->comm)];
+
+     task_lock(task);
+     /* Can't set if audit disabled */
+     if (!task->audit) {
+             task_unlock(task);
+             return -ENOPROTOOPT;
+     }
+     oldcontid = audit_get_contid(task);
+     read_lock(&tasklist_lock);
+     /* Don't allow the audit containerid to be unset */
+     if (!audit_contid_valid(contid))
+             rc = -EINVAL;
+     /* if we don't have caps, reject */
+     else if (!capable(CAP_AUDIT_CONTROL))
+             rc = -EPERM;
+     /* if task has children or is not single-threaded, deny */
+     else if (!list_empty(&task->children))
+             rc = -EBUSY;
+     else if (!(thread_group_leader(task) && thread_group_empty(task)))
+             rc = -EALREADY;
+     read_unlock(&tasklist_lock);
+     if (!rc)
+             task->audit->contid = contid;
+     task_unlock(task);
+

Quoted 8 lines

Review

Quoted 2 lines

(quoted diff) +2 -0

+     if (!audit_enabled)
+             return rc;

...but it is allowed to change it (assuming capable(CAP_AUDIT_CONTROL), of course)? Seems like this might be more immediately useful since we still live in the world of majority privileged containers if we didn't allow changing it, in addition to un-setting it.

Quoted 11 lines

The idea is that only container orchestrators should be able to set/modify the audit container ID, and since setting the audit container ID can have a significant effect on the records captured (and their routing to multiple daemons when we get there) modifying the audit container ID is akin to modifying the audit configuration which is why it is gated by CAP_AUDIT_CONTROL. The current thinking is that you would only change the audit container ID from one set/inherited value to another if you were nesting containers, in which case the nested container orchestrator would need to be granted CAP_AUDIT_CONTROL (which everyone to date seems to agree is a workable compromise).

But then don't you want some kind of ns_capable() instead (probably not the obvious one, though...)? With capable(), you can't really nest using the audit-id and user namespaces together.

Quoted 72 lines · Tycho Andersen

Quoted 69 lines · Paul Moore

Quoted 48 lines · Tycho Andersen

Quoted 2 lines · Richard Guy Briggs

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

...

Quoted 41 lines

/**
+ * audit_set_contid - set current task's audit contid
+ * @contid: contid value
+ *
+ * Returns 0 on success, -EPERM on permission failure.
+ *
+ * Called (set) from fs/proc/base.c::proc_contid_write().
+ */

kernel/audit.c +33 -0

+int audit_set_contid(struct task_struct *task, u64 contid)
+{
+     u64 oldcontid;
+     int rc = 0;
+     struct audit_buffer *ab;
+     uid_t uid;
+     struct tty_struct *tty;
+     char comm[sizeof(current->comm)];
+
+     task_lock(task);
+     /* Can't set if audit disabled */
+     if (!task->audit) {
+             task_unlock(task);
+             return -ENOPROTOOPT;
+     }
+     oldcontid = audit_get_contid(task);
+     read_lock(&tasklist_lock);
+     /* Don't allow the audit containerid to be unset */
+     if (!audit_contid_valid(contid))
+             rc = -EINVAL;
+     /* if we don't have caps, reject */
+     else if (!capable(CAP_AUDIT_CONTROL))
+             rc = -EPERM;
+     /* if task has children or is not single-threaded, deny */
+     else if (!list_empty(&task->children))
+             rc = -EBUSY;
+     else if (!(thread_group_leader(task) && thread_group_empty(task)))
+             rc = -EALREADY;
+     read_unlock(&tasklist_lock);
+     if (!rc)
+             task->audit->contid = contid;
+     task_unlock(task);
+

Quoted 8 lines

Review

Quoted 2 lines

(quoted diff) +2 -0

+     if (!audit_enabled)
+             return rc;

...but it is allowed to change it (assuming capable(CAP_AUDIT_CONTROL), of course)? Seems like this might be more immediately useful since we still live in the world of majority privileged containers if we didn't allow changing it, in addition to un-setting it.

The idea is that only container orchestrators should be able to set/modify the audit container ID, and since setting the audit container ID can have a significant effect on the records captured (and their routing to multiple daemons when we get there) modifying the audit container ID is akin to modifying the audit configuration which is why it is gated by CAP_AUDIT_CONTROL. The current thinking is that you would only change the audit container ID from one set/inherited value to another if you were nesting containers, in which case the nested container orchestrator would need to be granted CAP_AUDIT_CONTROL (which everyone to date seems to agree is a workable compromise).

Quoted 3 lines

But then don't you want some kind of ns_capable() instead (probably not the obvious one, though...)? With capable(), you can't really nest using the audit-id and user namespaces together.

You want capable() and not ns_capable() because you want to ensure that the orchestrator has the rights in the init_ns as changes to the audit container ID could have an auditing impact that spans the entire system.

Quoted 6 lines

Quoted 6 lines

The current thinking
is that you would only change the audit container ID from one set/inherited value to another if you were nesting containers, in which case the nested container orchestrator would need to be granted CAP_AUDIT_CONTROL (which everyone to date seems to agree is a workable compromise).

Quoted 73 lines · Paul Moore

Quoted 72 lines · Tycho Andersen

Quoted 69 lines · Paul Moore

Quoted 48 lines · Tycho Andersen

Quoted 2 lines · Richard Guy Briggs

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

...

Quoted 41 lines

/**
+ * audit_set_contid - set current task's audit contid
+ * @contid: contid value
+ *
+ * Returns 0 on success, -EPERM on permission failure.
+ *
+ * Called (set) from fs/proc/base.c::proc_contid_write().
+ */

kernel/audit.c +33 -0

+int audit_set_contid(struct task_struct *task, u64 contid)
+{
+     u64 oldcontid;
+     int rc = 0;
+     struct audit_buffer *ab;
+     uid_t uid;
+     struct tty_struct *tty;
+     char comm[sizeof(current->comm)];
+
+     task_lock(task);
+     /* Can't set if audit disabled */
+     if (!task->audit) {
+             task_unlock(task);
+             return -ENOPROTOOPT;
+     }
+     oldcontid = audit_get_contid(task);
+     read_lock(&tasklist_lock);
+     /* Don't allow the audit containerid to be unset */
+     if (!audit_contid_valid(contid))
+             rc = -EINVAL;
+     /* if we don't have caps, reject */
+     else if (!capable(CAP_AUDIT_CONTROL))
+             rc = -EPERM;
+     /* if task has children or is not single-threaded, deny */
+     else if (!list_empty(&task->children))
+             rc = -EBUSY;
+     else if (!(thread_group_leader(task) && thread_group_empty(task)))
+             rc = -EALREADY;
+     read_unlock(&tasklist_lock);
+     if (!rc)
+             task->audit->contid = contid;
+     task_unlock(task);
+

Quoted 8 lines

Review

Quoted 2 lines

(quoted diff) +2 -0

+     if (!audit_enabled)
+             return rc;

...but it is allowed to change it (assuming capable(CAP_AUDIT_CONTROL), of course)? Seems like this might be more immediately useful since we still live in the world of majority privileged containers if we didn't allow changing it, in addition to un-setting it.

The idea is that only container orchestrators should be able to set/modify the audit container ID, and since setting the audit container ID can have a significant effect on the records captured (and their routing to multiple daemons when we get there) modifying the audit container ID is akin to modifying the audit configuration which is why it is gated by CAP_AUDIT_CONTROL. The current thinking is that you would only change the audit container ID from one set/inherited value to another if you were nesting containers, in which case the nested container orchestrator would need to be granted CAP_AUDIT_CONTROL (which everyone to date seems to agree is a workable compromise).

Quoted 8 lines

Quoted 3 lines

But then don't you want some kind of ns_capable() instead (probably not the obvious one, though...)? With capable(), you can't really nest using the audit-id and user namespaces together.

You want capable() and not ns_capable() because you want to ensure that the orchestrator has the rights in the init_ns as changes to the audit container ID could have an auditing impact that spans the entire system.

Ok but,

Quoted 6 lines

Quoted 6 lines

Quoted 6 lines

The current thinking
is that you would only change the audit container ID from one set/inherited value to another if you were nesting containers, in which case the nested container orchestrator would need to be granted CAP_AUDIT_CONTROL (which everyone to date seems to agree is a workable compromise).

won't work in user namespaced containers, because they will never be capable(CAP_AUDIT_CONTROL); so I don't think this will work for nesting as is. But maybe nobody cares :)

Quoted 92 lines · Tycho Andersen

Quoted 73 lines · Paul Moore

Quoted 72 lines · Tycho Andersen

Quoted 69 lines · Paul Moore

Quoted 48 lines · Tycho Andersen

Quoted 2 lines · Richard Guy Briggs

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

...

Quoted 41 lines

/**
+ * audit_set_contid - set current task's audit contid
+ * @contid: contid value
+ *
+ * Returns 0 on success, -EPERM on permission failure.
+ *
+ * Called (set) from fs/proc/base.c::proc_contid_write().
+ */

(quoted diff) +33 -0

+int audit_set_contid(struct task_struct *task, u64 contid)
+{
+     u64 oldcontid;
+     int rc = 0;
+     struct audit_buffer *ab;
+     uid_t uid;
+     struct tty_struct *tty;
+     char comm[sizeof(current->comm)];
+
+     task_lock(task);
+     /* Can't set if audit disabled */
+     if (!task->audit) {
+             task_unlock(task);
+             return -ENOPROTOOPT;
+     }
+     oldcontid = audit_get_contid(task);
+     read_lock(&tasklist_lock);
+     /* Don't allow the audit containerid to be unset */
+     if (!audit_contid_valid(contid))
+             rc = -EINVAL;
+     /* if we don't have caps, reject */
+     else if (!capable(CAP_AUDIT_CONTROL))
+             rc = -EPERM;
+     /* if task has children or is not single-threaded, deny */
+     else if (!list_empty(&task->children))
+             rc = -EBUSY;
+     else if (!(thread_group_leader(task) && thread_group_empty(task)))
+             rc = -EALREADY;
+     read_unlock(&tasklist_lock);
+     if (!rc)
+             task->audit->contid = contid;
+     task_unlock(task);
+

Quoted 8 lines

Review

Quoted 2 lines

(quoted diff) +2 -0

+     if (!audit_enabled)
+             return rc;

...but it is allowed to change it (assuming capable(CAP_AUDIT_CONTROL), of course)? Seems like this might be more immediately useful since we still live in the world of majority privileged containers if we didn't allow changing it, in addition to un-setting it.

The idea is that only container orchestrators should be able to set/modify the audit container ID, and since setting the audit container ID can have a significant effect on the records captured (and their routing to multiple daemons when we get there) modifying the audit container ID is akin to modifying the audit configuration which is why it is gated by CAP_AUDIT_CONTROL. The current thinking is that you would only change the audit container ID from one set/inherited value to another if you were nesting containers, in which case the nested container orchestrator would need to be granted CAP_AUDIT_CONTROL (which everyone to date seems to agree is a workable compromise).

Quoted 8 lines

Quoted 3 lines

But then don't you want some kind of ns_capable() instead (probably not the obvious one, though...)? With capable(), you can't really nest using the audit-id and user namespaces together.

You want capable() and not ns_capable() because you want to ensure that the orchestrator has the rights in the init_ns as changes to the audit container ID could have an auditing impact that spans the entire system.

Ok but,

Quoted 6 lines

Quoted 6 lines

Quoted 6 lines

The current thinking
is that you would only change the audit container ID from one set/inherited value to another if you were nesting containers, in which case the nested container orchestrator would need to be granted CAP_AUDIT_CONTROL (which everyone to date seems to agree is a workable compromise).

Quoted 3 lines

won't work in user namespaced containers, because they will never be capable(CAP_AUDIT_CONTROL); so I don't think this will work for nesting as is. But maybe nobody cares :)

That's fun :)

To be honest, I've never been a big fan of supporting nested containers from an audit perspective, so I'm not really too upset about this. The k8s/cri-o folks seem okay with this, or at least I haven't heard any objections; lxc folks, what do you have to say?

Quoted 6 lines · Paul Moore

Quoted 5 lines · Tycho Andersen

Quoted 4 lines · Paul Moore

Quoted 3 lines · Tycho Andersen

Quoted 2 lines · Paul Moore

Quoted 1 line · Tycho Andersen

On Mon, Apr 08, 2019 at 11:39:09PM -0400, Richard Guy Briggs wrote:

Quoted 11 lines

Quoted 10 lines

Quoted 6 lines

Quoted 6 lines

Quoted 6 lines

The current thinking
is that you would only change the audit container ID from one set/inherited value to another if you were nesting containers, in which case the nested container orchestrator would need to be granted CAP_AUDIT_CONTROL (which everyone to date seems to agree is a workable compromise).

won't work in user namespaced containers, because they will never be capable(CAP_AUDIT_CONTROL); so I don't think this will work for nesting as is. But maybe nobody cares :)

Quoted 6 lines

That's fun :)

To be honest, I've never been a big fan of supporting nested containers from an audit perspective, so I'm not really too upset about this. The k8s/cri-o folks seem okay with this, or at least I haven't heard any objections; lxc folks, what do you have to say?

I actually thought the answer to this (when last I looked, "some time" ago) was that userspace should track an audit message saying "task X in container Y is changing its auditid to Z", and then decide to also track Z. This should be doable, but a lot of extra work in userspace.

Per-userns containerids would also work. So task X1 is in containerid 1 on the host and creates a new task Y in new userns; it continues to be reported in init_user_ns as containerid 1 forever; but in its own userns it can request to be known as some other containerid. Audit socks would be per-userns, allowing root in a container to watch for audit events in its own (and descendent) namespaces.

But again I'm sure we've gone over all this in the last few years.

I suppose we can look at this as a "first step", and talk about making it user-ns-nestable later. But agreed it's not useful in a lot of situations as is.

[REMINDER: It is an "*audit* container ID" and not a general "container ID" ;) Smiley aside, I'm not kidding about that part.]

I'm not interested in supporting/merging something that isn't useful; if this doesn't work for your use case then we need to figure out what would work. It sounds like nested containers are much more common in the lxc world, can you elaborate a bit more on this?

As far as the possible solutions you mention above, I'm not sure I like the per-userns audit container IDs, I'd much rather just emit the necessary tracking information via the audit record stream and let the log analysis tools figure it out. However, the bigger question is how to limit (re)setting the audit container ID when you are in a non-init userns. For reasons already mentioned, using capable() is a non starter for everything but the initial userns, and using ns_capable() is equally poor as it essentially allows any userns the ability to munge it's audit container ID (obviously not good). It appears we need a different method for controlling access to the audit container ID.

Quoted 3 lines · Paul Moore

[REMINDER: It is an "*audit* container ID" and not a general "container ID" ;) Smiley aside, I'm not kidding about that part.]

This sort of seems like a distinction without a difference; presumably audit is going to want to differentiate between everything that people in userspace call a container. So you'll have to support all this insanity anyway, even if it's "not a container ID".

Quoted 5 lines

I'm not interested in supporting/merging something that isn't useful; if this doesn't work for your use case then we need to figure out what would work. It sounds like nested containers are much more common in the lxc world, can you elaborate a bit more on this?

Quoted 11 lines

As far as the possible solutions you mention above, I'm not sure I like the per-userns audit container IDs, I'd much rather just emit the necessary tracking information via the audit record stream and let the log analysis tools figure it out. However, the bigger question is how to limit (re)setting the audit container ID when you are in a non-init userns. For reasons already mentioned, using capable() is a non starter for everything but the initial userns, and using ns_capable() is equally poor as it essentially allows any userns the ability to munge it's audit container ID (obviously not good). It appears we need a different method for controlling access to the audit container ID.

One option would be to make it a string, and have it be append only. That should be safe with no checks.

I know there was a long thread about what type to make this thing. I think you could accomplish the append-only-ness with a u64 if you had some rule about only allowing setting lower order bits than those that are already set. With 4 bits for simplicity:

1100 # initial container id
1100 -> 1011 # not allowed
1100 -> 1101 # allowed, but now 1101 is set in stone since there are

             # no lower order bits left

There are probably fancier ways to do it if you actually understand math :)

Since userns nesting is limited to 32 levels (right now, IIRC), and you have 64 bits, this might be reasonable. You could just teach container engines to use the first say N bits for themselves, with a 1 bit for the barrier at the end.

Quoted 9 lines · Tycho Andersen

Quoted 3 lines · Paul Moore

[REMINDER: It is an "*audit* container ID" and not a general "container ID" ;) Smiley aside, I'm not kidding about that part.]

This sort of seems like a distinction without a difference; presumably audit is going to want to differentiate between everything that people in userspace call a container. So you'll have to support all this insanity anyway, even if it's "not a container ID".

That's not quite right. Audit doesn't care about what a container is, or is not, it also doesn't care if the "audit container ID" actually matches the ID used by the container engine in userspace and I think that is a very important line to draw. Audit is simply given a value which it calls the "audit container ID", it ensures that the value is inherited appropriately (e.g. children inherit their parent's audit container ID), and it uses the value in audit records to provide some additional context for log analysis. The distinction isn't limited to the value itself, but also to how it is used; it is an "audit container ID" and not a "container ID" because this value is exclusively for use by the audit subsystem. We are very intentionally not adding a generic container ID to the kernel. If the kernel does ever grow a general purpose container ID we will be one of the first ones in line to make use of it, but we are not going to be the ones to generically add containers to the kernel. Enough people already hate audit ;)

Quoted 25 lines

Quoted 5 lines

I'm not interested in supporting/merging something that isn't useful; if this doesn't work for your use case then we need to figure out what would work. It sounds like nested containers are much more common in the lxc world, can you elaborate a bit more on this?

Quoted 11 lines

As far as the possible solutions you mention above, I'm not sure I like the per-userns audit container IDs, I'd much rather just emit the necessary tracking information via the audit record stream and let the log analysis tools figure it out. However, the bigger question is how to limit (re)setting the audit container ID when you are in a non-init userns. For reasons already mentioned, using capable() is a non starter for everything but the initial userns, and using ns_capable() is equally poor as it essentially allows any userns the ability to munge it's audit container ID (obviously not good). It appears we need a different method for controlling access to the audit container ID.

One option would be to make it a string, and have it be append only. That should be safe with no checks.

I know there was a long thread about what type to make this thing. I think you could accomplish the append-only-ness with a u64 if you had some rule about only allowing setting lower order bits than those that are already set. With 4 bits for simplicity:

Quoted 7 lines

1100 # initial container id
1100 -> 1011 # not allowed
1100 -> 1101 # allowed, but now 1101 is set in stone since there are

             # no lower order bits left

There are probably fancier ways to do it if you actually understand math :)

;)

Quoted 4 lines

Since userns nesting is limited to 32 levels (right now, IIRC), and you have 64 bits, this might be reasonable. You could just teach container engines to use the first say N bits for themselves, with a 1 bit for the barrier at the end.

I like the creativity, but I worry that at some point these limitations are going to be raised (limits have a funny way of doing that over time) and we will be in trouble. I say "trouble" because I want to be able to quickly do an audit container ID comparison and we're going to pay a penalty for these larger values (we'll need this when we add multiple auditd support and the requisite record routing).

Thinking about this makes me also realize we probably need to think a bit longer about audit container ID conflicts between orchestrators. Right now we just take the value that is given to us by the orchestrator, but if we want to allow multiple container orchestrators to work without some form of cooperation in userspace (I think we have to assume the orchestrators will not talk to each other) we likely need to have some way to block reuse of an audit container ID. We would either need to prevent the orchestrator from explicitly setting an audit container ID to a currently in use value, or instead generate the audit container ID in the kernel upon an event triggered by the orchestrator (e.g. a write to a /proc file). I suspect we should start looking at the idr code, I think we will need to make use of it.

paul moore
www.paul-moore.com

Quoted 75 lines · Paul Moore

Quoted 9 lines · Tycho Andersen

Quoted 3 lines · Paul Moore

[REMINDER: It is an "*audit* container ID" and not a general "container ID" ;) Smiley aside, I'm not kidding about that part.]

This sort of seems like a distinction without a difference; presumably audit is going to want to differentiate between everything that people in userspace call a container. So you'll have to support all this insanity anyway, even if it's "not a container ID".

That's not quite right. Audit doesn't care about what a container is, or is not, it also doesn't care if the "audit container ID" actually matches the ID used by the container engine in userspace and I think that is a very important line to draw. Audit is simply given a value which it calls the "audit container ID", it ensures that the value is inherited appropriately (e.g. children inherit their parent's audit container ID), and it uses the value in audit records to provide some additional context for log analysis. The distinction isn't limited to the value itself, but also to how it is used; it is an "audit container ID" and not a "container ID" because this value is exclusively for use by the audit subsystem. We are very intentionally not adding a generic container ID to the kernel. If the kernel does ever grow a general purpose container ID we will be one of the first ones in line to make use of it, but we are not going to be the ones to generically add containers to the kernel. Enough people already hate audit ;)

Quoted 25 lines

Quoted 5 lines

I'm not interested in supporting/merging something that isn't useful; if this doesn't work for your use case then we need to figure out what would work. It sounds like nested containers are much more common in the lxc world, can you elaborate a bit more on this?

Quoted 11 lines

As far as the possible solutions you mention above, I'm not sure I like the per-userns audit container IDs, I'd much rather just emit the necessary tracking information via the audit record stream and let the log analysis tools figure it out. However, the bigger question is how to limit (re)setting the audit container ID when you are in a non-init userns. For reasons already mentioned, using capable() is a non starter for everything but the initial userns, and using ns_capable() is equally poor as it essentially allows any userns the ability to munge it's audit container ID (obviously not good). It appears we need a different method for controlling access to the audit container ID.

One option would be to make it a string, and have it be append only. That should be safe with no checks.

I know there was a long thread about what type to make this thing. I think you could accomplish the append-only-ness with a u64 if you had some rule about only allowing setting lower order bits than those that are already set. With 4 bits for simplicity:

Quoted 7 lines

1100 # initial container id
1100 -> 1011 # not allowed
1100 -> 1101 # allowed, but now 1101 is set in stone since there are

             # no lower order bits left

There are probably fancier ways to do it if you actually understand math :)

;)

Quoted 4 lines

Since userns nesting is limited to 32 levels (right now, IIRC), and you have 64 bits, this might be reasonable. You could just teach container engines to use the first say N bits for themselves, with a 1 bit for the barrier at the end.

I like the creativity, but I worry that at some point these limitations are going to be raised (limits have a funny way of doing that over time) and we will be in trouble. I say "trouble" because I want to be able to quickly do an audit container ID comparison and we're going to pay a penalty for these larger values (we'll need this when we add multiple auditd support and the requisite record routing).

Quoted 12 lines

Thinking about this makes me also realize we probably need to think a bit longer about audit container ID conflicts between orchestrators. Right now we just take the value that is given to us by the orchestrator, but if we want to allow multiple container orchestrators to work without some form of cooperation in userspace (I think we have to assume the orchestrators will not talk to each other) we likely need to have some way to block reuse of an audit container ID. We would either need to prevent the orchestrator from explicitly setting an audit container ID to a currently in use value, or instead generate the audit container ID in the kernel upon an event triggered by the orchestrator (e.g. a write to a /proc file). I suspect we should start looking at the idr code, I think we will need to make use of it.

My first reaction to using the IDR code is that once an idr is given up, it can be reused. I suppose we request IDRs and then never give them up to avoid reuse...

I already had some ideas of preventing an existing ID from being reused,

but that makes the practice of some container engines injecting processes into existing containers difficult if not impossible.

Quoted 102 lines · Richard Guy Briggs

Quoted 88 lines · Paul Moore

Quoted 10 lines · Tycho Andersen

Quoted 2 lines · Paul Moore

[REMINDER: It is an "*audit* container ID" and not a general "container ID" ;) Smiley aside, I'm not kidding about that part.]

This sort of seems like a distinction without a difference; presumably
audit is going to want to differentiate between everything that people
in userspace call a container. So you'll have to support all this insanity anyway, even if it's "not a container ID".

That's not quite right. Audit doesn't care about what a container is, or is not, it also doesn't care if the "audit container ID" actually matches the ID used by the container engine in userspace and I think that is a very important line to draw. Audit is simply given a value which it calls the "audit container ID", it ensures that the value is inherited appropriately (e.g. children inherit their parent's audit container ID), and it uses the value in audit records to provide some additional context for log analysis. The distinction isn't limited to the value itself, but also to how it is used; it is an "audit container ID" and not a "container ID" because this value is exclusively for use by the audit subsystem. We are very intentionally not adding a generic container ID to the kernel. If the kernel does ever grow a general purpose container ID we will be one of the first ones in line to make use of it, but we are not going to be the ones to generically add containers to the kernel. Enough people already hate audit ;)

Quoted 35 lines

Quoted 17 lines

I'm not interested in supporting/merging something that isn't useful;
if this doesn't work for your use case then we need to figure out what
would work. It sounds like nested containers are much more common in
the lxc world, can you elaborate a bit more on this?

As far as the possible solutions you mention above, I'm not sure I like the per-userns audit container IDs, I'd much rather just emit the
necessary tracking information via the audit record stream and let the
log analysis tools figure it out. However, the bigger question is how
to limit (re)setting the audit container ID when you are in a non-init

Quoted 8 lines

userns. For reasons already mentioned, using capable() is a non starter for everything but the initial userns, and using ns_capable()
is equally poor as it essentially allows any userns the ability to munge it's audit container ID (obviously not good). It appears we need a different method for controlling access to the audit container
ID.

One option would be to make it a string, and have it be append only. That should be safe with no checks.

I know there was a long thread about what type to make this thing. I think you could accomplish the append-only-ness with a u64 if you had some rule about only allowing setting lower order bits than those that
are already set. With 4 bits for simplicity:

Quoted 8 lines

1100 # initial container id
1100 -> 1011 # not allowed
1100 -> 1101 # allowed, but now 1101 is set in stone since there are

             # no lower order bits left

There are probably fancier ways to do it if you actually understand math :)

;)

Quoted 5 lines

Since userns nesting is limited to 32 levels (right now, IIRC), and you have 64 bits, this might be reasonable. You could just teach container engines to use the first say N bits for themselves, with a 1
bit for the barrier at the end.

I like the creativity, but I worry that at some point these limitations are going to be raised (limits have a funny way of doing that over time) and we will be in trouble. I say "trouble" because I want to be able to quickly do an audit container ID comparison and we're going to pay a penalty for these larger values (we'll need this when we add multiple auditd support and the requisite record routing).

Quoted 12 lines

Thinking about this makes me also realize we probably need to think a bit longer about audit container ID conflicts between orchestrators. Right now we just take the value that is given to us by the orchestrator, but if we want to allow multiple container orchestrators to work without some form of cooperation in userspace (I think we have to assume the orchestrators will not talk to each other) we likely need to have some way to block reuse of an audit container ID. We would either need to prevent the orchestrator from explicitly setting an audit container ID to a currently in use value, or instead generate the audit container ID in the kernel upon an event triggered by the orchestrator (e.g. a write to a /proc file). I suspect we should start looking at the idr code, I think we will need to make use of it.

Quoted 3 lines

My first reaction to using the IDR code is that once an idr is given up, it can be reused. I suppose we request IDRs and then never give them up to avoid reuse...

I'm not sure we ever what to guarantee that an audit container ID won't be reused during the lifetime of the system, it is a discrete integer after all. What I think we do want to guarantee is that we won't allow an unintentional audit container ID collision between different orchestrators; if a single orchestrator wants to reuse an audit container ID then that is its choice.

Quoted 1 line

I already had some ideas of preventing an existing ID from being reused,

Cool. I only made the idr suggestion since it is used for PIDs and solves a very similar problem.

Quoted 2 lines

but that makes the practice of some container engines injecting processes into existing containers difficult if not impossible.

Yes, we'll need some provision to indicate which orchestrator "controls" that particular audit container ID, and allow that orchestrator to reuse that particular audit container ID (until all those containers disappear and the audit container ID is given back to the pool).

Hello Paul,

I am curious about this. We seemed to be close to getting this patch pulled in. A lot of people are waiting for it. Can you summarize what you think the patches need and who we think needs to do it? I'm lost. Does LXC people need to propose something? Does Richard? Someone else? Who's got the ball?

Quoted 127 lines · Richard Guy Briggs

Quoted 102 lines · Paul Moore

Quoted 88 lines · Tycho Andersen

Quoted 10 lines · Paul Moore

Quoted 2 lines

[REMINDER: It is an "*audit* container ID" and not a general "container ID" ;) Smiley aside, I'm not kidding about that part.]

This sort of seems like a distinction without a difference; presumably
audit is going to want to differentiate between everything that people
in userspace call a container. So you'll have to support all this insanity anyway, even if it's "not a container ID".

That's not quite right. Audit doesn't care about what a container is, or is not, it also doesn't care if the "audit container ID" actually matches the ID used by the container engine in userspace and I think that is a very important line to draw. Audit is simply given a value which it calls the "audit container ID", it ensures that the value is inherited appropriately (e.g. children inherit their parent's audit container ID), and it uses the value in audit records to provide some additional context for log analysis. The distinction isn't limited to the value itself, but also to how it is used; it is an "audit container ID" and not a "container ID" because this value is exclusively for use by the audit subsystem. We are very intentionally not adding a generic container ID to the kernel. If the kernel does ever grow a general purpose container ID we will be one of the first ones in line to make use of it, but we are not going to be the ones to generically add containers to the kernel. Enough people already hate audit ;)

Quoted 35 lines

Quoted 17 lines

I'm not interested in supporting/merging something that isn't useful;
if this doesn't work for your use case then we need to figure out what
would work. It sounds like nested containers are much more common in
the lxc world, can you elaborate a bit more on this?

As far as the possible solutions you mention above, I'm not sure I like the per-userns audit container IDs, I'd much rather just emit the
necessary tracking information via the audit record stream and let the
log analysis tools figure it out. However, the bigger question is how
to limit (re)setting the audit container ID when you are in a non-init

Quoted 8 lines

userns. For reasons already mentioned, using capable() is a non starter for everything but the initial userns, and using ns_capable()
is equally poor as it essentially allows any userns the ability to munge it's audit container ID (obviously not good). It appears we need a different method for controlling access to the audit container
ID.

One option would be to make it a string, and have it be append only. That should be safe with no checks.

I know there was a long thread about what type to make this thing. I think you could accomplish the append-only-ness with a u64 if you had some rule about only allowing setting lower order bits than those that
are already set. With 4 bits for simplicity:

Quoted 8 lines

1100 # initial container id
1100 -> 1011 # not allowed
1100 -> 1101 # allowed, but now 1101 is set in stone since there are

             # no lower order bits left

There are probably fancier ways to do it if you actually understand math :)

;)

Quoted 5 lines

Since userns nesting is limited to 32 levels (right now, IIRC), and you have 64 bits, this might be reasonable. You could just teach container engines to use the first say N bits for themselves, with a 1
bit for the barrier at the end.

I like the creativity, but I worry that at some point these limitations are going to be raised (limits have a funny way of doing that over time) and we will be in trouble. I say "trouble" because I want to be able to quickly do an audit container ID comparison and we're going to pay a penalty for these larger values (we'll need this when we add multiple auditd support and the requisite record routing).

Quoted 12 lines

Thinking about this makes me also realize we probably need to think a bit longer about audit container ID conflicts between orchestrators. Right now we just take the value that is given to us by the orchestrator, but if we want to allow multiple container orchestrators to work without some form of cooperation in userspace (I think we have to assume the orchestrators will not talk to each other) we likely need to have some way to block reuse of an audit container ID. We would either need to prevent the orchestrator from explicitly setting an audit container ID to a currently in use value, or instead generate the audit container ID in the kernel upon an event triggered by the orchestrator (e.g. a write to a /proc file). I suspect we should start looking at the idr code, I think we will need to make use of it.

Quoted 3 lines

My first reaction to using the IDR code is that once an idr is given up, it can be reused. I suppose we request IDRs and then never give them up to avoid reuse...

I'm not sure we ever what to guarantee that an audit container ID won't be reused during the lifetime of the system, it is a discrete integer after all. What I think we do want to guarantee is that we won't allow an unintentional audit container ID collision between different orchestrators; if a single orchestrator wants to reuse an audit container ID then that is its choice.

Quoted 1 line

I already had some ideas of preventing an existing ID from being reused,

Cool. I only made the idr suggestion since it is used for PIDs and solves a very similar problem.

Quoted 2 lines

but that makes the practice of some container engines injecting processes into existing containers difficult if not impossible.

Yes, we'll need some provision to indicate which orchestrator "controls" that particular audit container ID, and allow that orchestrator to reuse that particular audit container ID (until all those containers disappear and the audit container ID is given back to the pool).

Quoted 6 lines · Steve Grubb

Hello Paul,

I am curious about this. We seemed to be close to getting this patch pulled in. A lot of people are waiting for it. Can you summarize what you think the patches need and who we think needs to do it? I'm lost. Does LXC people need to propose something? Does Richard? Someone else? Who's got the ball?

[My apologies, this was lost in my inbox and I just not noticed it.]

Please don't top post on things sent to the mailing lists Steve, you know better than that.

Yes, things were moving along well, but upon talking with the LXC folks it appears we underestimated the importance of nested orchestrators. I suspect my reply to Dan on the 4th covered your questions, if you didn't see it, here is the relevant snippet:

"To be clear, that's where we are at: we need to figure out what the kernel API would look like to support nested container orchestrators. My gut feeling is that this isn't going to be terribly complicated compared to the rest of the audit container ID work, but it is going to be some work. We had a discussion about some potential solutions in the cover letter and it sounds like Richard is working up some ideas now, let's wait to see what that looks like."

... and that is where we are at. I'm looking forward to seeing Richard's next patchset.

Quoted 123 lines · EDT Paul Moore

Quoted 122 lines · Richard Guy Briggs

Quoted 102 lines · Paul Moore

Quoted 88 lines · Tycho Andersen

Quoted 10 lines · Paul Moore

Quoted 2 lines

[REMINDER: It is an "*audit* container ID" and not a general "container ID" ;) Smiley aside, I'm not kidding about that part.]

This sort of seems like a distinction without a difference; presumably
audit is going to want to differentiate between everything that people
in userspace call a container. So you'll have to support all this insanity anyway, even if it's "not a container ID".

That's not quite right. Audit doesn't care about what a container is, or is not, it also doesn't care if the "audit container ID" actually matches the ID used by the container engine in userspace and I think that is a very important line to draw. Audit is simply given a value which it calls the "audit container ID", it ensures that the value is inherited appropriately (e.g. children inherit their parent's audit container ID), and it uses the value in audit records to provide some additional context for log analysis. The distinction isn't limited to the value itself, but also to how it is used; it is an "audit container ID" and not a "container ID" because this value is exclusively for use by the audit subsystem. We are very intentionally not adding a generic container ID to the kernel. If the kernel does ever grow a general purpose container ID we will be one of the first ones in line to make use of it, but we are not going to be the ones to generically add containers to the kernel. Enough people already hate audit ;)

Quoted 35 lines

Quoted 17 lines

I'm not interested in supporting/merging something that isn't useful;
if this doesn't work for your use case then we need to figure out what
would work. It sounds like nested containers are much more common in
the lxc world, can you elaborate a bit more on this?

As far as the possible solutions you mention above, I'm not sure I like the per-userns audit container IDs, I'd much rather just emit the
necessary tracking information via the audit record stream and let the
log analysis tools figure it out. However, the bigger question is how
to limit (re)setting the audit container ID when you are in a non-init

Quoted 8 lines

userns. For reasons already mentioned, using capable() is a non starter for everything but the initial userns, and using ns_capable()
is equally poor as it essentially allows any userns the ability to munge it's audit container ID (obviously not good). It appears we need a different method for controlling access to the audit container
ID.

One option would be to make it a string, and have it be append only. That should be safe with no checks.

I know there was a long thread about what type to make this thing. I think you could accomplish the append-only-ness with a u64 if you had some rule about only allowing setting lower order bits than those that
are already set. With 4 bits for simplicity:

Quoted 8 lines

1100 # initial container id
1100 -> 1011 # not allowed
1100 -> 1101 # allowed, but now 1101 is set in stone since there are

             # no lower order bits left

There are probably fancier ways to do it if you actually understand math :)

;)

Quoted 5 lines

Since userns nesting is limited to 32 levels (right now, IIRC), and you have 64 bits, this might be reasonable. You could just teach container engines to use the first say N bits for themselves, with a 1
bit for the barrier at the end.

I like the creativity, but I worry that at some point these limitations are going to be raised (limits have a funny way of doing that over time) and we will be in trouble. I say "trouble" because I want to be able to quickly do an audit container ID comparison and we're going to pay a penalty for these larger values (we'll need this when we add multiple auditd support and the requisite record routing).

Quoted 12 lines

Thinking about this makes me also realize we probably need to think a bit longer about audit container ID conflicts between orchestrators. Right now we just take the value that is given to us by the orchestrator, but if we want to allow multiple container orchestrators to work without some form of cooperation in userspace (I think we have to assume the orchestrators will not talk to each other) we likely need to have some way to block reuse of an audit container ID. We would either need to prevent the orchestrator from explicitly setting an audit container ID to a currently in use value, or instead generate the audit container ID in the kernel upon an event triggered by the orchestrator (e.g. a write to a /proc file). I suspect we should start looking at the idr code, I think we will need to make use of it.

Quoted 3 lines

My first reaction to using the IDR code is that once an idr is given up, it can be reused. I suppose we request IDRs and then never give them up to avoid reuse...

I'm not sure we ever what to guarantee that an audit container ID won't be reused during the lifetime of the system, it is a discrete integer after all. What I think we do want to guarantee is that we won't allow an unintentional audit container ID collision between different orchestrators; if a single orchestrator wants to reuse an audit container ID then that is its choice.

Quoted 1 line

I already had some ideas of preventing an existing ID from being reused,

Cool. I only made the idr suggestion since it is used for PIDs and solves a very similar problem.

Quoted 2 lines

but that makes the practice of some container engines injecting processes into existing containers difficult if not impossible.

Quoted 5 lines

Quoted 5 lines

Yes, we'll need some provision to indicate which orchestrator "controls" that particular audit container ID, and allow that orchestrator to reuse that particular audit container ID (until all those containers disappear and the audit container ID is given back to the pool).

paul moore

Quoted 9 lines · Tycho Andersen

Quoted 3 lines · Paul Moore

[REMINDER: It is an "*audit* container ID" and not a general "container ID" ;) Smiley aside, I'm not kidding about that part.]

This sort of seems like a distinction without a difference; presumably audit is going to want to differentiate between everything that people in userspace call a container. So you'll have to support all this insanity anyway, even if it's "not a container ID".

That's not quite right. Audit doesn't care about what a container is, or is not, it also doesn't care if the "audit container ID" actually matches the ID used by the container engine in userspace and I think that is a very important line to draw. Audit is simply given a value which it calls the "audit container ID", it ensures that the value is inherited appropriately (e.g. children inherit their parent's audit container ID), and it uses the value in audit records to provide some additional context for log analysis. The distinction isn't limited to the value itself, but also to how it is used; it is an "audit container ID" and not a "container ID" because this value is exclusively for use by the audit subsystem. We are very intentionally not adding a generic container ID to the kernel. If the kernel does ever grow a general purpose container ID we will be one of the first ones in line to make use of it, but we are not going to be the ones to generically add containers to the kernel. Enough people already hate audit ;)

Quoted 25 lines

Quoted 5 lines

I'm not interested in supporting/merging something that isn't useful; if this doesn't work for your use case then we need to figure out what would work. It sounds like nested containers are much more common in the lxc world, can you elaborate a bit more on this?

Quoted 11 lines

As far as the possible solutions you mention above, I'm not sure I like the per-userns audit container IDs, I'd much rather just emit the necessary tracking information via the audit record stream and let the log analysis tools figure it out. However, the bigger question is how to limit (re)setting the audit container ID when you are in a non-init userns. For reasons already mentioned, using capable() is a non starter for everything but the initial userns, and using ns_capable() is equally poor as it essentially allows any userns the ability to munge it's audit container ID (obviously not good). It appears we need a different method for controlling access to the audit container ID.

One option would be to make it a string, and have it be append only. That should be safe with no checks.

I know there was a long thread about what type to make this thing. I think you could accomplish the append-only-ness with a u64 if you had some rule about only allowing setting lower order bits than those that are already set. With 4 bits for simplicity:

Quoted 7 lines

1100 # initial container id
1100 -> 1011 # not allowed
1100 -> 1101 # allowed, but now 1101 is set in stone since there are

             # no lower order bits left

There are probably fancier ways to do it if you actually understand math :)

;)

Quoted 4 lines

Since userns nesting is limited to 32 levels (right now, IIRC), and you have 64 bits, this might be reasonable. You could just teach container engines to use the first say N bits for themselves, with a 1 bit for the barrier at the end.

I like the creativity, but I worry that at some point these limitations are going to be raised (limits have a funny way of doing that over time) and we will be in trouble. I say "trouble" because I want to be able to quickly do an audit container ID comparison and we're going to pay a penalty for these larger values (we'll need this when we add multiple auditd support and the requisite record routing).

Thinking about this makes me also realize we probably need to think a bit longer about audit container ID conflicts between orchestrators. Right now we just take the value that is given to us by the orchestrator, but if we want to allow multiple container orchestrators to work without some form of cooperation in userspace (I think we have to assume the orchestrators will not talk to each other) we likely need to have some way to block reuse of an audit container ID. We would either need to prevent the orchestrator from explicitly setting an audit container ID to a currently in use value, or instead generate the audit container ID in the kernel upon an event triggered by the orchestrator (e.g. a write to a /proc file). I suspect we should start looking at the idr code, I think we will need to make use of it.

paul moore
www.paul-moore.com

Quoted 77 lines · Paul Moore

Quoted 10 lines · Tycho Andersen

Quoted 4 lines · Paul Moore

[REMINDER: It is an "*audit* container ID" and not a general "container ID" ;) Smiley aside, I'm not kidding about that part.]

This sort of seems like a distinction without a difference; presumably audit is going to want to differentiate between everything that people in userspace call a container. So you'll have to support all this insanity anyway, even if it's "not a container ID".

That's not quite right. Audit doesn't care about what a container is, or is not, it also doesn't care if the "audit container ID" actually matches the ID used by the container engine in userspace and I think that is a very important line to draw. Audit is simply given a value which it calls the "audit container ID", it ensures that the value is inherited appropriately (e.g. children inherit their parent's audit container ID), and it uses the value in audit records to provide some additional context for log analysis. The distinction isn't limited to the value itself, but also to how it is used; it is an "audit container ID" and not a "container ID" because this value is exclusively for use by the audit subsystem. We are very intentionally not adding a generic container ID to the kernel. If the kernel does ever grow a general purpose container ID we will be one of the first ones in line to make use of it, but we are not going to be the ones to generically add containers to the kernel. Enough people already hate audit ;)

Quoted 26 lines

Quoted 6 lines

I'm not interested in supporting/merging something that isn't useful; if this doesn't work for your use case then we need to figure out what would work. It sounds like nested containers are much more common in the lxc world, can you elaborate a bit more on this?

Quoted 11 lines

As far as the possible solutions you mention above, I'm not sure I like the per-userns audit container IDs, I'd much rather just emit the necessary tracking information via the audit record stream and let the log analysis tools figure it out. However, the bigger question is how to limit (re)setting the audit container ID when you are in a non-init userns. For reasons already mentioned, using capable() is a non starter for everything but the initial userns, and using ns_capable() is equally poor as it essentially allows any userns the ability to munge it's audit container ID (obviously not good). It appears we need a different method for controlling access to the audit container ID.

One option would be to make it a string, and have it be append only. That should be safe with no checks.

I know there was a long thread about what type to make this thing. I think you could accomplish the append-only-ness with a u64 if you had some rule about only allowing setting lower order bits than those that are already set. With 4 bits for simplicity:

Quoted 7 lines

1100 # initial container id
1100 -> 1011 # not allowed
1100 -> 1101 # allowed, but now 1101 is set in stone since there are

      # no lower order bits left

There are probably fancier ways to do it if you actually understand math :)

;)

Quoted 4 lines

Since userns nesting is limited to 32 levels (right now, IIRC), and you have 64 bits, this might be reasonable. You could just teach container engines to use the first say N bits for themselves, with a 1 bit for the barrier at the end.

I like the creativity, but I worry that at some point these limitations are going to be raised (limits have a funny way of doing that over time) and we will be in trouble. I say "trouble" because I want to be able to quickly do an audit container ID comparison and we're going to pay a penalty for these larger values (we'll need this when we add multiple auditd support and the requisite record routing).

Quoted 12 lines

Thinking about this makes me also realize we probably need to think a bit longer about audit container ID conflicts between orchestrators. Right now we just take the value that is given to us by the orchestrator, but if we want to allow multiple container orchestrators to work without some form of cooperation in userspace (I think we have to assume the orchestrators will not talk to each other) we likely need to have some way to block reuse of an audit container ID. We would either need to prevent the orchestrator from explicitly setting an audit container ID to a currently in use value, or instead generate the audit container ID in the kernel upon an event triggered by the orchestrator (e.g. a write to a /proc file). I suspect we should start looking at the idr code, I think we will need to make use of it.

To address this, I'd suggest that it is enforced to only allow the setting of descendants and to maintain a master list of audit container identifiers (with a hash table if necessary later) that includes the container owner.

This also allows the orchestrator/engine to inject processes into existing containers by checking that the audit container identifier is only used again by the same owner.

I have working code for both.

On 2019-05-30 19:26, Paul Moore wrote:

Quoted 7 lines

I like the creativity, but I worry that at some point these limitations are going to be raised (limits have a funny way of doing that over time) and we will be in trouble. I say "trouble" because I want to be able to quickly do an audit container ID comparison and we're going to pay a penalty for these larger values (we'll need this when we add multiple auditd support and the requisite record routing).

Quoted 12 lines

Thinking about this makes me also realize we probably need to think a bit longer about audit container ID conflicts between orchestrators. Right now we just take the value that is given to us by the orchestrator, but if we want to allow multiple container orchestrators to work without some form of cooperation in userspace (I think we have to assume the orchestrators will not talk to each other) we likely need to have some way to block reuse of an audit container ID. We would either need to prevent the orchestrator from explicitly setting an audit container ID to a currently in use value, or instead generate the audit container ID in the kernel upon an event triggered by the orchestrator (e.g. a write to a /proc file). I suspect we should start looking at the idr code, I think we will need to make use of it.

To address this, I'd suggest that it is enforced to only allow the setting of descendants and to maintain a master list of audit container identifiers (with a hash table if necessary later) that includes the container owner.

Quoted 1 line · Richard Guy Briggs

On 2019-05-30 19:26, Paul Moore wrote:

...

Quoted 20 lines

Quoted 7 lines

I like the creativity, but I worry that at some point these limitations are going to be raised (limits have a funny way of doing that over time) and we will be in trouble. I say "trouble" because I want to be able to quickly do an audit container ID comparison and we're going to pay a penalty for these larger values (we'll need this when we add multiple auditd support and the requisite record routing).

Quoted 12 lines

Thinking about this makes me also realize we probably need to think a bit longer about audit container ID conflicts between orchestrators. Right now we just take the value that is given to us by the orchestrator, but if we want to allow multiple container orchestrators to work without some form of cooperation in userspace (I think we have to assume the orchestrators will not talk to each other) we likely need to have some way to block reuse of an audit container ID. We would either need to prevent the orchestrator from explicitly setting an audit container ID to a currently in use value, or instead generate the audit container ID in the kernel upon an event triggered by the orchestrator (e.g. a write to a /proc file). I suspect we should start looking at the idr code, I think we will need to make use of it.

Quoted 4 lines

To address this, I'd suggest that it is enforced to only allow the setting of descendants and to maintain a master list of audit container identifiers (with a hash table if necessary later) that includes the container owner.

We're discussing the audit container ID management policy elsewhere in this thread so I won't comment on that here, but I did want to say that we will likely need something better than a simple list of audit container IDs from the start. It's common for systems to have thousands of containers now (or multiple thousands), which tells me that a list is a poor choice. You mentioned a hash table, so I would suggest starting with that over the list for the initial patchset.

paul moore

Quoted 30 lines · Paul Moore

Quoted 1 line · Richard Guy Briggs

On 2019-05-30 19:26, Paul Moore wrote:

...

Quoted 20 lines

Quoted 7 lines

I like the creativity, but I worry that at some point these limitations are going to be raised (limits have a funny way of doing that over time) and we will be in trouble. I say "trouble" because I want to be able to quickly do an audit container ID comparison and we're going to pay a penalty for these larger values (we'll need this when we add multiple auditd support and the requisite record routing).

Quoted 12 lines

Thinking about this makes me also realize we probably need to think a bit longer about audit container ID conflicts between orchestrators. Right now we just take the value that is given to us by the orchestrator, but if we want to allow multiple container orchestrators to work without some form of cooperation in userspace (I think we have to assume the orchestrators will not talk to each other) we likely need to have some way to block reuse of an audit container ID. We would either need to prevent the orchestrator from explicitly setting an audit container ID to a currently in use value, or instead generate the audit container ID in the kernel upon an event triggered by the orchestrator (e.g. a write to a /proc file). I suspect we should start looking at the idr code, I think we will need to make use of it.

Quoted 4 lines

To address this, I'd suggest that it is enforced to only allow the setting of descendants and to maintain a master list of audit container identifiers (with a hash table if necessary later) that includes the container owner.

Quoted 7 lines

We're discussing the audit container ID management policy elsewhere in this thread so I won't comment on that here, but I did want to say that we will likely need something better than a simple list of audit container IDs from the start. It's common for systems to have thousands of containers now (or multiple thousands), which tells me that a list is a poor choice. You mentioned a hash table, so I would suggest starting with that over the list for the initial patchset.

I saw that as an internal incremental improvement that did not affect the API, so I wanted to keep things a bit simpler (as you've requested in the past) to get this going, and add that enhancement later.

I'll start working on it now. The hash table would simply point to lists anyways unless you can recommend a better approach.

Quoted 39 lines · Richard Guy Briggs

Quoted 30 lines · Paul Moore

Quoted 1 line · Richard Guy Briggs

On 2019-05-30 19:26, Paul Moore wrote:

...

Quoted 20 lines

Quoted 7 lines

I like the creativity, but I worry that at some point these limitations are going to be raised (limits have a funny way of doing that over time) and we will be in trouble. I say "trouble" because I want to be able to quickly do an audit container ID comparison and we're going to pay a penalty for these larger values (we'll need this when we add multiple auditd support and the requisite record routing).

Quoted 12 lines

Thinking about this makes me also realize we probably need to think a bit longer about audit container ID conflicts between orchestrators. Right now we just take the value that is given to us by the orchestrator, but if we want to allow multiple container orchestrators to work without some form of cooperation in userspace (I think we have to assume the orchestrators will not talk to each other) we likely need to have some way to block reuse of an audit container ID. We would either need to prevent the orchestrator from explicitly setting an audit container ID to a currently in use value, or instead generate the audit container ID in the kernel upon an event triggered by the orchestrator (e.g. a write to a /proc file). I suspect we should start looking at the idr code, I think we will need to make use of it.

Quoted 4 lines

To address this, I'd suggest that it is enforced to only allow the setting of descendants and to maintain a master list of audit container identifiers (with a hash table if necessary later) that includes the container owner.

Quoted 7 lines

We're discussing the audit container ID management policy elsewhere in this thread so I won't comment on that here, but I did want to say that we will likely need something better than a simple list of audit container IDs from the start. It's common for systems to have thousands of containers now (or multiple thousands), which tells me that a list is a poor choice. You mentioned a hash table, so I would suggest starting with that over the list for the initial patchset.

Quoted 3 lines

I saw that as an internal incremental improvement that did not affect the API, so I wanted to keep things a bit simpler (as you've requested in the past) to get this going, and add that enhancement later.

In general a simple approach is a good way to start when the problem/use-case is not very well understood; in other words, don't spend a lot of time/effort optimizing something you don't yet understand. In this case we know that people want to deploy a *lot* of containers on a single system so we should design the data structures appropriately. A list is simply not a good fit here, I believe/hope you know that too.

Quoted 2 lines

I'll start working on it now. The hash table would simply point to lists anyways unless you can recommend a better approach.

I assume when you say "point to lists" you are talking about using lists for the hash buckets? If so, yes that should be fine at this point. In general if the per-bucket lists become a bottleneck we can look at the size of the table (or make it tunable) or even use a different approach entirely. Ultimately the data store is an implementation detail private to the audit subsystem in the kernel so we should be able to change it as necessary without breaking anything.

paul moore

Quoted 7 lines · Serge E. Hallyn

Quoted 6 lines · Paul Moore

Quoted 5 lines · Tycho Andersen

Quoted 4 lines · Paul Moore

Quoted 3 lines · Tycho Andersen

Quoted 2 lines · Paul Moore

Quoted 1 line · Tycho Andersen

On Mon, Apr 08, 2019 at 11:39:09PM -0400, Richard Guy Briggs wrote:

...

Quoted 30 lines

Quoted 11 lines

Quoted 10 lines

Quoted 6 lines

Quoted 6 lines

Quoted 6 lines

The current thinking
is that you would only change the audit container ID from one set/inherited value to another if you were nesting containers, in which case the nested container orchestrator would need to be granted CAP_AUDIT_CONTROL (which everyone to date seems to agree is a workable compromise).

won't work in user namespaced containers, because they will never be capable(CAP_AUDIT_CONTROL); so I don't think this will work for nesting as is. But maybe nobody cares :)

Quoted 6 lines

That's fun :)

To be honest, I've never been a big fan of supporting nested containers from an audit perspective, so I'm not really too upset about this. The k8s/cri-o folks seem okay with this, or at least I haven't heard any objections; lxc folks, what do you have to say?

I actually thought the answer to this (when last I looked, "some time" ago) was that userspace should track an audit message saying "task X in container Y is changing its auditid to Z", and then decide to also track Z. This should be doable, but a lot of extra work in userspace.

Per-userns containerids would also work. So task X1 is in containerid 1 on the host and creates a new task Y in new userns; it continues to be reported in init_user_ns as containerid 1 forever; but in its own userns it can request to be known as some other containerid. Audit socks would be per-userns, allowing root in a container to watch for audit events in its own (and descendent) namespaces.

Quoted 5 lines

But again I'm sure we've gone over all this in the last few years.

I suppose we can look at this as a "first step", and talk about making it user-ns-nestable later. But agreed it's not useful in a lot of situations as is.

[REMINDER: It is an "*audit* container ID" and not a general "container ID" ;) Smiley aside, I'm not kidding about that part.]

I'm not interested in supporting/merging something that isn't useful; if this doesn't work for your use case then we need to figure out what would work. It sounds like nested containers are much more common in the lxc world, can you elaborate a bit more on this?

As far as the possible solutions you mention above, I'm not sure I like the per-userns audit container IDs, I'd much rather just emit the necessary tracking information via the audit record stream and let the log analysis tools figure it out. However, the bigger question is how to limit (re)setting the audit container ID when you are in a non-init userns. For reasons already mentioned, using capable() is a non starter for everything but the initial userns, and using ns_capable() is equally poor as it essentially allows any userns the ability to munge it's audit container ID (obviously not good). It appears we need a different method for controlling access to the audit container ID.

Punting this to a LSM hook is an obvious thing to do, and something we might want to do anyway, but currently audit doesn't rely on the LSM for proper/safe operation and I'm not sure I want to change that now.

The next obvious thing is to create some sort of access control knob in audit itself. Perhaps an auditctl operation that would allow the administrator to specify which containers, via their corresponding audit container IDs, are allowed to change their audit container ID? The permission granting would need to be done in the init userns, but it would allow containers with a non-init userns the ability to change their audit container ID. We would probably still want a ns_capable(CAP_AUDIT_CONTROL) restriction in this case.

Does anyone else have any other ideas?

Signature · 2 lines

paul moore
www.paul-moore.com

On 2019-05-30 15:29, Paul Moore wrote:

Quoted 8 lines

[REMINDER: It is an "*audit* container ID" and not a general "container ID" ;) Smiley aside, I'm not kidding about that part.]

I'm not interested in supporting/merging something that isn't useful; if this doesn't work for your use case then we need to figure out what would work. It sounds like nested containers are much more common in the lxc world, can you elaborate a bit more on this?

Quoted 11 lines

As far as the possible solutions you mention above, I'm not sure I like the per-userns audit container IDs, I'd much rather just emit the necessary tracking information via the audit record stream and let the log analysis tools figure it out. However, the bigger question is how to limit (re)setting the audit container ID when you are in a non-init userns. For reasons already mentioned, using capable() is a non starter for everything but the initial userns, and using ns_capable() is equally poor as it essentially allows any userns the ability to munge it's audit container ID (obviously not good). It appears we need a different method for controlling access to the audit container ID.

We're not quite ready yet for multiple audit daemons and possibly not yet for audit namespaces, but this is starting to look a lot like the latter.

From my perspective we create namespaces to allow us to redefine a

If we can't trust ns_capable() then why are we passing on CAP_AUDIT_CONTROL? It is being passed down and not stripped purposely by the orchestrator/engine. If ns_capable() isn't inherited how is it gained otherwise? Can it be inserted by cotainer image? I think the answer is "no". Either we trust ns_capable() or we have audit namespaces (recommend based on user namespace) (or both).

At this point I would say we are at an impasse unless we trust ns_capable() or we implement audit namespaces.

I don't think another mechanism to trust nested orchestrators/engines will buy us anything.

Am I missing something?

Quoted 1 line · Richard Guy Briggs

On 2019-05-30 15:29, Paul Moore wrote:

...

Quoted 20 lines

Quoted 19 lines

[REMINDER: It is an "*audit* container ID" and not a general "container ID" ;) Smiley aside, I'm not kidding about that part.]

I'm not interested in supporting/merging something that isn't useful; if this doesn't work for your use case then we need to figure out what would work. It sounds like nested containers are much more common in the lxc world, can you elaborate a bit more on this?

As far as the possible solutions you mention above, I'm not sure I like the per-userns audit container IDs, I'd much rather just emit the necessary tracking information via the audit record stream and let the log analysis tools figure it out. However, the bigger question is how to limit (re)setting the audit container ID when you are in a non-init userns. For reasons already mentioned, using capable() is a non starter for everything but the initial userns, and using ns_capable() is equally poor as it essentially allows any userns the ability to munge it's audit container ID (obviously not good). It appears we need a different method for controlling access to the audit container ID.

Quoted 3 lines

We're not quite ready yet for multiple audit daemons and possibly not yet for audit namespaces, but this is starting to look a lot like the latter.

A few quick comments on audit namespaces: the audit container ID is not envisioned as a new namespace (even in nested form) and neither do I consider running multiple audit daemons to be a new namespace.

From my perspective we create namespaces to allow us to redefine a global resource for some subset of the system, e.g. providing a unique /tmp for some number of processes on the system. While it may be tempting to think of the audit container ID as something we could "namespace", especially when multiple audit daemons are concerned, in some ways this would be counter productive; the audit container ID is intended to be a global ID that can be used to associate audit event

records with a "container" where the "container" is defined by an orchestrator outside the audit subsystem. The global nature of the audit container ID allows us to maintain a sane(ish) view of the system in the audit log, if we were to "namespace" the audit container ID such that the value was no longer guaranteed to be unique throughout the system, we would need to additionally track the audit namespace along with the audit container ID which starts to border on insanity IMHO.

Quoted 6 lines

If we can't trust ns_capable() then why are we passing on CAP_AUDIT_CONTROL? It is being passed down and not stripped purposely by the orchestrator/engine. If ns_capable() isn't inherited how is it gained otherwise? Can it be inserted by cotainer image? I think the answer is "no". Either we trust ns_capable() or we have audit namespaces (recommend based on user namespace) (or both).

My thinking is that since ns_capable() checks the credentials with respect to the current user namespace we can't rely on it to control access since it would be possible for a privileged process running inside an unprivileged container to manipulate the audit container ID (containerized process has CAP_AUDIT_CONTROL, e.g. running as root in the container, while the container itself does not).

Quoted 2 lines

At this point I would say we are at an impasse unless we trust ns_capable() or we implement audit namespaces.

I'm not sure how we can trust ns_capable(), but if you can think of a way I would love to hear it. I'm also not sure how namespacing audit is helpful (see my above comments), but if you think it is please explain.

Quoted 4 lines

I don't think another mechanism to trust nested orchestrators/engines will buy us anything.

Am I missing something?

Based on your questions/comments above it looks like your understanding of ns_capable() does not match mine; if I'm thinking about ns_capable() incorrectly, please educate me.

Signature · 2 lines

paul moore
www.paul-moore.com

Quoted 1 line · Paul Moore

On Mon, Jul 8, 2019 at 2:06 PM Richard Guy Briggs <[email protected]> wrote:

Quoted 7 lines

Quoted 6 lines

If we can't trust ns_capable() then why are we passing on CAP_AUDIT_CONTROL? It is being passed down and not stripped purposely by the orchestrator/engine. If ns_capable() isn't inherited how is it gained otherwise? Can it be inserted by cotainer image? I think the answer is "no". Either we trust ns_capable() or we have audit namespaces (recommend based on user namespace) (or both).

Quoted 6 lines

My thinking is that since ns_capable() checks the credentials with respect to the current user namespace we can't rely on it to control access since it would be possible for a privileged process running inside an unprivileged container to manipulate the audit container ID (containerized process has CAP_AUDIT_CONTROL, e.g. running as root in the container, while the container itself does not).

What makes an unprivileged container unprivileged? "root", or "CAP_*"?

If CAP_AUDIT_CONTROL is granted, does "root" matter?

Does it matter what user namespace it is in?

I understand that root is *gained* in an
unprivileged user namespace, but capabilities are inherited or permitted and that process either has it or it doesn't and an unprivileged user namespace can't gain a capability that has been rescinded. Different subsystems use the userid or capabilities or both to determine privileges.

In this case, is the userid relevant?

Quoted 7 lines

Quoted 2 lines

At this point I would say we are at an impasse unless we trust ns_capable() or we implement audit namespaces.

I'm not sure how we can trust ns_capable(), but if you can think of a way I would love to hear it. I'm also not sure how namespacing audit is helpful (see my above comments), but if you think it is please explain.

So if we are not namespacing, why do we not trust capabilities?

Quoted 2 lines · Richard Guy Briggs

Quoted 1 line · Paul Moore

On Mon, Jul 8, 2019 at 2:06 PM Richard Guy Briggs <[email protected]> wrote:

...

Quoted 14 lines

Quoted 13 lines

Quoted 6 lines

If we can't trust ns_capable() then why are we passing on CAP_AUDIT_CONTROL? It is being passed down and not stripped purposely by the orchestrator/engine. If ns_capable() isn't inherited how is it gained otherwise? Can it be inserted by cotainer image? I think the answer is "no". Either we trust ns_capable() or we have audit namespaces (recommend based on user namespace) (or both).

My thinking is that since ns_capable() checks the credentials with respect to the current user namespace we can't rely on it to control access since it would be possible for a privileged process running inside an unprivileged container to manipulate the audit container ID (containerized process has CAP_AUDIT_CONTROL, e.g. running as root in the container, while the container itself does not).

Quoted 1 line

What makes an unprivileged container unprivileged? "root", or "CAP_*"?

My understanding is that when most people refer to an unprivileged container they are referring to a container run without capabilities or a container run by a user other than root. I'm sure there are better definitions out there, by folks much smarter than me on these things, but that's my working definition.

Quoted 1 line

If CAP_AUDIT_CONTROL is granted, does "root" matter?

Our discussions here have been about capabilities, not UIDs. The only reason root might matter is that it generally has the full capability set.

Quoted 1 line

Does it matter what user namespace it is in?

What likely matters is what check is called: capable() or ns_capable(). Those can yield very different results.

Quoted 6 lines

I understand that root is *gained* in an
unprivileged user namespace, but capabilities are inherited or permitted and that process either has it or it doesn't and an unprivileged user namespace can't gain a capability that has been rescinded. Different subsystems use the userid or capabilities or both to determine privileges.

Once again, I believe the important thing to focus on here is capable() vs ns_capable(). We can't safely rely on ns_capable() for the audit container ID policy since that is easily met inside the container regardless of the process' creds which started the container.

Quoted 1 line

In this case, is the userid relevant?

We don't do UID checks, we do capability checks, so yes, the UID is irrelevant.

Quoted 8 lines

Quoted 7 lines

Quoted 2 lines

At this point I would say we are at an impasse unless we trust ns_capable() or we implement audit namespaces.

I'm not sure how we can trust ns_capable(), but if you can think of a way I would love to hear it. I'm also not sure how namespacing audit is helpful (see my above comments), but if you think it is please explain.

Quoted 1 line

So if we are not namespacing, why do we not trust capabilities?

We can trust capable(CAP_AUDIT_CONTROL) for enforcing audit container ID policy, we can not trust ns_capable(CAP_AUDIT_CONTROL).

paul moore

On 2019-07-16 19:30, Paul Moore wrote:

Quoted 2 lines

We can trust capable(CAP_AUDIT_CONTROL) for enforcing audit container ID policy, we can not trust ns_capable(CAP_AUDIT_CONTROL).

Ok. So does a process in a non-init user namespace have two (or more) sets of capabilities stored in creds, one in the init_user_ns, and one in current_user_ns? Or does it get stripped of all its capabilities in init_user_ns once it has its own set in current_user_ns? If the former, then we can use capable(). If the latter, we need another mechanism, as you have suggested might be needed.

If some random unprivileged user wants to fire up a container orchestrator/engine in his own user namespace, then audit needs to be namespaced. Can we safely discard this scenario for now?

Quoted 1 line · Richard Guy Briggs

On 2019-07-16 19:30, Paul Moore wrote:

...

Quoted 9 lines

Quoted 2 lines

We can trust capable(CAP_AUDIT_CONTROL) for enforcing audit container ID policy, we can not trust ns_capable(CAP_AUDIT_CONTROL).

Ok. So does a process in a non-init user namespace have two (or more) sets of capabilities stored in creds, one in the init_user_ns, and one in current_user_ns? Or does it get stripped of all its capabilities in init_user_ns once it has its own set in current_user_ns? If the former, then we can use capable(). If the latter, we need another mechanism, as you have suggested might be needed.

Unfortunately I think the problem is that ultimately we need to allow any container orchestrator that has been given privileges to manage the audit container ID to also grant that privilege to any of the child process/containers it manages. I don't believe we can do that with capabilities based on the code I've looked at, and the discussions I've had, but if you find a way I would leave to hear it.

Quoted 3 lines

If some random unprivileged user wants to fire up a container orchestrator/engine in his own user namespace, then audit needs to be namespaced. Can we safely discard this scenario for now?

I think the only time we want to allow a container orchestrator to manage the audit container ID is if it has been granted that privilege by someone who has that privilege already. In the zero-container, or single-level of containers, case this is relatively easy, and we can accomplish it using CAP_AUDIT_CONTROL as the privilege. If we start nesting container orchestrators it becomes more complicated as we need to be able to support granting and inheriting this privilege in a manner; this is why I suggested a new mechanism *may* be necessary.

Paul Moore <[email protected]> writes:

On Wed, Jul 17, 2019 at 8:52 PM Richard Guy Briggs <[email protected]> wrote:

On 2019-07-16 19:30, Paul Moore wrote:

...

We can trust capable(CAP_AUDIT_CONTROL) for enforcing audit container ID policy, we can not trust ns_capable(CAP_AUDIT_CONTROL).

Ok. So does a process in a non-init user namespace have two (or more) sets of capabilities stored in creds, one in the init_user_ns, and one in current_user_ns? Or does it get stripped of all its capabilities in init_user_ns once it has its own set in current_user_ns? If the former, then we can use capable(). If the latter, we need another mechanism, as you have suggested might be needed.

Unfortunately I think the problem is that ultimately we need to allow any container orchestrator that has been given privileges to manage the audit container ID to also grant that privilege to any of the child process/containers it manages. I don't believe we can do that with capabilities based on the code I've looked at, and the discussions I've had, but if you find a way I would leave to hear it.

If some random unprivileged user wants to fire up a container orchestrator/engine in his own user namespace, then audit needs to be namespaced. Can we safely discard this scenario for now?

I think the only time we want to allow a container orchestrator to manage the audit container ID is if it has been granted that privilege by someone who has that privilege already. In the zero-container, or single-level of containers, case this is relatively easy, and we can accomplish it using CAP_AUDIT_CONTROL as the privilege. If we start nesting container orchestrators it becomes more complicated as we need to be able to support granting and inheriting this privilege in a manner; this is why I suggested a new mechanism *may* be necessary.

Let me segway a bit and see if I can get this conversation out of the rut it seems to have drifted into.

Unprivileged containers and nested containers exist today and are going to become increasingly common. Let that be a given.

As I recall the interesting thing for audit to log is actions by privileged processes. Audit can log more but generally configuring logging by of the actions of unprivileged users is effectively a self DOS.

So I think the initial implementation can safely ignore actions of nested containers and unprivileged containers because you don't care about their actions.

If we start allow running audit in a container then we need to deal with all of the nesting issues but until then I don't think you folks care.

Or am I wrong. Do the requirements for securely auditing things from the kernel care about the actions of unprivileged users?

Eric

--
Linux-audit mailing list
[email protected]
https://www.redhat.com/mailman/listinfo/linux-audit

Quoted 29 lines · Paul Moore

Quoted 1 line · Richard Guy Briggs

On 2019-07-16 19:30, Paul Moore wrote:

...

Quoted 11 lines

Quoted 3 lines

We can trust capable(CAP_AUDIT_CONTROL) for enforcing audit container ID policy, we can not trust ns_capable(CAP_AUDIT_CONTROL).

Ok. So does a process in a non-init user namespace have two (or more) sets of capabilities stored in creds, one in the init_user_ns, and one in current_user_ns? Or does it get stripped of all its capabilities in init_user_ns once it has its own set in current_user_ns? If the former, then we can use capable(). If the latter, we need another mechanism, as you have suggested might be needed.

Unfortunately I think the problem is that ultimately we need to allow any container orchestrator that has been given privileges to manage the audit container ID to also grant that privilege to any of the child process/containers it manages. I don't believe we can do that with capabilities based on the code I've looked at, and the discussions I've had, but if you find a way I would leave to hear it.

Quoted 3 lines

If some random unprivileged user wants to fire up a container orchestrator/engine in his own user namespace, then audit needs to be namespaced. Can we safely discard this scenario for now?

Quoted 9 lines

I think the only time we want to allow a container orchestrator to manage the audit container ID is if it has been granted that privilege by someone who has that privilege already. In the zero- container, or single-level of containers, case this is relatively easy, and we can accomplish it using CAP_AUDIT_CONTROL as the privilege. If we start nesting container orchestrators it becomes more complicated as we need to be able to support granting and inheriting this privilege in a manner; this is why I suggested a new mechanism *may* be necessary.

Let me segway a bit and see if I can get this conversation out of the rut it seems to have drifted into.

Unprivileged containers and nested containers exist today and are going to become increasingly common. Let that be a given.

As I recall the interesting thing for audit to log is actions by privileged processes. Audit can log more but generally configuring logging by of the actions of unprivileged users is effectively a self DOS.

So I think the initial implementation can safely ignore actions of nested containers and unprivileged containers because you don't care about their actions.

If we start allow running audit in a container then we need to deal with all of the nesting issues but until then I don't think you folks care.

Or am I wrong. Do the requirements for securely auditing things from the kernel care about the actions of unprivileged users?

Quoted 12 lines · Paul Moore

Quoted 11 lines · Richard Guy Briggs

Quoted 9 lines · Paul Moore

Quoted 2 lines · Richard Guy Briggs

At this point I would say we are at an impasse unless we trust ns_capable() or we implement audit namespaces.

I'm not sure how we can trust ns_capable(), but if you can think of a way I would love to hear it. I'm also not sure how namespacing audit is helpful (see my above comments), but if you think it is please explain.

Quoted 4 lines

Quoted 1 line

So if we are not namespacing, why do we not trust capabilities?

We can trust capable(CAP_AUDIT_CONTROL) for enforcing audit container ID policy, we can not trust ns_capable(CAP_AUDIT_CONTROL).

Ok. So does a process in a non-init user namespace have two (or more) sets of capabilities stored in creds, one in the init_user_ns, and one in current_user_ns? Or does it get stripped of all its capabilities in init_user_ns once it has its own set in current_user_ns? If the former, then we can use capable(). If the latter, we need another mechanism, as you have suggested might be needed.

Quoted 48 lines · Tycho Andersen

Quoted 2 lines · Richard Guy Briggs

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

...

Quoted 41 lines

/**
+ * audit_set_contid - set current task's audit contid
+ * @contid: contid value
+ *
+ * Returns 0 on success, -EPERM on permission failure.
+ *
+ * Called (set) from fs/proc/base.c::proc_contid_write().
+ */

kernel/audit.c +33 -0

+int audit_set_contid(struct task_struct *task, u64 contid)
+{
+     u64 oldcontid;
+     int rc = 0;
+     struct audit_buffer *ab;
+     uid_t uid;
+     struct tty_struct *tty;
+     char comm[sizeof(current->comm)];
+
+     task_lock(task);
+     /* Can't set if audit disabled */
+     if (!task->audit) {
+             task_unlock(task);
+             return -ENOPROTOOPT;
+     }
+     oldcontid = audit_get_contid(task);
+     read_lock(&tasklist_lock);
+     /* Don't allow the audit containerid to be unset */
+     if (!audit_contid_valid(contid))
+             rc = -EINVAL;
+     /* if we don't have caps, reject */
+     else if (!capable(CAP_AUDIT_CONTROL))
+             rc = -EPERM;
+     /* if task has children or is not single-threaded, deny */
+     else if (!list_empty(&task->children))
+             rc = -EBUSY;
+     else if (!(thread_group_leader(task) && thread_group_empty(task)))
+             rc = -EALREADY;
+     read_unlock(&tasklist_lock);
+     if (!rc)
+             task->audit->contid = contid;
+     task_unlock(task);
+

Quoted 8 lines

Review

Quoted 2 lines

(quoted diff) +2 -0

+     if (!audit_enabled)
+             return rc;

...but it is allowed to change it (assuming capable(CAP_AUDIT_CONTROL), of course)? Seems like this might be more immediately useful since we still live in the world of majority privileged containers if we didn't allow changing it, in addition to un-setting it.

The idea is that only container orchestrators should be able to set/modify the audit container ID, and since setting the audit container ID can have a significant effect on the records captured (and their routing to multiple daemons when we get there) modifying the audit container ID is akin to modifying the audit configuration which is why it is gated by CAP_AUDIT_CONTROL. The current thinking is that you would only change the audit container ID from one set/inherited value to another if you were nesting containers, in which case the nested container orchestrator would need to be granted CAP_AUDIT_CONTROL (which everyone to date seems to agree is a workable compromise). We did consider allowing for a chain of nested audit container IDs, but the implications of doing so are significant (implementation mess, runtime cost, etc.) so we are leaving that out of this effort.

From a practical perspective, un-setting the audit container ID is pretty much the same as changing it from one set value to another so most of the above applies to that case as well.

Signature · 2 lines

paul moore
www.paul-moore.com

On 2019-05-29 11:29, Paul Moore wrote:

Quoted 6 lines

The idea is that only container orchestrators should be able to set/modify the audit container ID, and since setting the audit container ID can have a significant effect on the records captured (and their routing to multiple daemons when we get there) modifying the audit container ID is akin to modifying the audit configuration which is why it is gated by CAP_AUDIT_CONTROL. The current thinking

Quoted 8 lines

is that you would only change the audit container ID from one set/inherited value to another if you were nesting containers, in which case the nested container orchestrator would need to be granted CAP_AUDIT_CONTROL (which everyone to date seems to agree is a workable compromise). We did consider allowing for a chain of nested audit container IDs, but the implications of doing so are significant (implementation mess, runtime cost, etc.) so we are leaving that out of this effort.

We had previously discussed the idea of restricting orchestrators/engines from only being able to set the audit container identifier on their own descendants, but it was discarded. I've added a check to ensure this is now enforced.

I've also added a check to ensure that a process can't set its own audit container identifier ...

... and that if the identifier is already set, then the orchestrator/engine must be in a descendant user namespace from the orchestrator that set the previously inherited audit container identifier.

Quoted 1 line · Richard Guy Briggs

On 2019-05-29 11:29, Paul Moore wrote:

...

Quoted 15 lines

Quoted 6 lines

The idea is that only container orchestrators should be able to set/modify the audit container ID, and since setting the audit container ID can have a significant effect on the records captured (and their routing to multiple daemons when we get there) modifying the audit container ID is akin to modifying the audit configuration which is why it is gated by CAP_AUDIT_CONTROL. The current thinking

Quoted 8 lines

is that you would only change the audit container ID from one set/inherited value to another if you were nesting containers, in which case the nested container orchestrator would need to be granted CAP_AUDIT_CONTROL (which everyone to date seems to agree is a workable compromise). We did consider allowing for a chain of nested audit container IDs, but the implications of doing so are significant (implementation mess, runtime cost, etc.) so we are leaving that out of this effort.

Quoted 4 lines

We had previously discussed the idea of restricting orchestrators/engines from only being able to set the audit container identifier on their own descendants, but it was discarded. I've added a check to ensure this is now enforced.

When we weren't allowing nested orchestrators it wasn't necessary, but with the move to support nesting I believe this will be a requirement. We might also need/want to restrict audit container ID changes if a descendant is acting as a container orchestrator and managing one or more audit container IDs; although I'm less certain of the need for this.

Quoted 2 lines

I've also added a check to ensure that a process can't set its own audit container identifier ...

What does this protect against, or what problem does this solve? Considering how easy it is to fork/exec, it seems like this could be trivially bypassed.

Quoted 4 lines

... and that if the identifier is already set, then the orchestrator/engine must be in a descendant user namespace from the orchestrator that set the previously inherited audit container identifier.

You lost me here ... although I don't like the idea of relying on X namespace inheritance for a hard coded policy on setting the audit container ID; we've worked hard to keep this independent of any definition of a "container" and it would sadden me greatly if we had to go back on that.

paul moore

Quoted 25 lines · Paul Moore

Quoted 1 line · Richard Guy Briggs

On 2019-05-29 11:29, Paul Moore wrote:

...

Quoted 15 lines

Quoted 6 lines

The idea is that only container orchestrators should be able to set/modify the audit container ID, and since setting the audit container ID can have a significant effect on the records captured (and their routing to multiple daemons when we get there) modifying the audit container ID is akin to modifying the audit configuration which is why it is gated by CAP_AUDIT_CONTROL. The current thinking

Quoted 8 lines

is that you would only change the audit container ID from one set/inherited value to another if you were nesting containers, in which case the nested container orchestrator would need to be granted CAP_AUDIT_CONTROL (which everyone to date seems to agree is a workable compromise). We did consider allowing for a chain of nested audit container IDs, but the implications of doing so are significant (implementation mess, runtime cost, etc.) so we are leaving that out of this effort.

Quoted 4 lines

We had previously discussed the idea of restricting orchestrators/engines from only being able to set the audit container identifier on their own descendants, but it was discarded. I've added a check to ensure this is now enforced.

Quoted 6 lines

When we weren't allowing nested orchestrators it wasn't necessary, but with the move to support nesting I believe this will be a requirement. We might also need/want to restrict audit container ID changes if a descendant is acting as a container orchestrator and managing one or more audit container IDs; although I'm less certain of the need for this.

I was of the opinion it was necessary before with single-layer parallel orchestrators/engines.

Quoted 6 lines

Quoted 2 lines

I've also added a check to ensure that a process can't set its own audit container identifier ...

What does this protect against, or what problem does this solve? Considering how easy it is to fork/exec, it seems like this could be trivially bypassed.

Well, for starters, it would remove one layer of nesting. It would separate the functional layers of processes.

Other than that, it seems
like a gut feeling that it is just wrong to allow it. It seems like a layer violation that one container orchestrator/engine could set its own audit container identifier and then set its children as well. It would be its own parent.

It would make it harder to verify adherance to descendancy and inheritance rules.

Quoted 5 lines

Quoted 4 lines

... and that if the identifier is already set, then the orchestrator/engine must be in a descendant user namespace from the orchestrator that set the previously inherited audit container identifier.

Quoted 5 lines

You lost me here ... although I don't like the idea of relying on X namespace inheritance for a hard coded policy on setting the audit container ID; we've worked hard to keep this independent of any definition of a "container" and it would sadden me greatly if we had to go back on that.

This would seem to be the one concession I'm reluctantly making to try to solve this nested container orchestrator/engine challenge.

Would backing off on that descendant user namespace requirement and only require that a nested audit container identifier only be permitted on a descendant task be sufficient? It may for this use case, but I suspect not for additional audit daemons (we're not there yet) and message routing to those daemons.

The one difference here is that it does not depend on this if the audit container identifier has not already been set.

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 Serge Hallyn <[email protected]>

Acked-by Steve Grubb <[email protected]>

Acked-by Neil Horman <[email protected]>

Reviewed-by Ondrej Mosnacek <[email protected]>

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

fs/proc/base.c +36 -0

index ddef482f1334..43fd0c4b87de 100644
@@ -1294,6 +1294,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

@@ -3033,6 +3067,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),

@@ -3431,6 +3466,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 bde346e73f0c..301337776193 100644
@@ -89,6 +89,7 @@ struct audit_field {
 struct audit_task_info {
 	kuid_t			loginuid;
 	unsigned int		sessionid;
+	u64			contid;
 #ifdef CONFIG_AUDITSYSCALL
 	struct audit_context	*ctx;
 #endif

@@ -189,6 +190,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;
 #else /* CONFIG_AUDIT */
 static inline int audit_alloc(struct task_struct *task)

@@ -250,6 +260,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
 #endif /* CONFIG_AUDIT */
 

@@ -606,6 +621,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 3901c51c0b93..4a6a8bf1de32 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 */

@@ -485,6 +486,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 +69 -0

index 3fb09783cd4a..182b0f2c183d 100644
@@ -244,6 +244,7 @@ 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);
 	tsk->audit = info;
 
 	ret = audit_alloc_syscall(tsk);

@@ -258,6 +259,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,
 #ifdef CONFIG_AUDITSYSCALL
 	.ctx = NULL,
 #endif

@@ -2341,6 +2343,73 @@ int audit_set_loginuid(kuid_t loginuid)
 }
 
 /**
+ * audit_set_contid - set current task's audit contid
+ * @contid: contid value
+ *
+ * Returns 0 on success, -EPERM on permission failure.
+ *
+ * Called (set) from fs/proc/base.c::proc_contid_write().
+ */
+int audit_set_contid(struct task_struct *task, u64 contid)
+{
+	u64 oldcontid;
+	int rc = 0;
+	struct audit_buffer *ab;
+	uid_t uid;
+	struct tty_struct *tty;
+	char comm[sizeof(current->comm)];
+
+	task_lock(task);
+	/* Can't set if audit disabled */
+	if (!task->audit) {
+		task_unlock(task);
+		return -ENOPROTOOPT;
+	}
+	oldcontid = audit_get_contid(task);
+	read_lock(&tasklist_lock);
+	/* Don't allow the audit containerid to be unset */
+	if (!audit_contid_valid(contid))
+		rc = -EINVAL;
+	/* if we don't have caps, reject */
+	else if (!capable(CAP_AUDIT_CONTROL))
+		rc = -EPERM;
+	/* if task has children or is not single-threaded, deny */
+	else if (!list_empty(&task->children))
+		rc = -EBUSY;
+	else if (!(thread_group_leader(task) && thread_group_empty(task)))
+		rc = -EALREADY;
+	read_unlock(&tasklist_lock);
+	if (!rc)
+		task->audit->contid = contid;
+	task_unlock(task);
+
+	if (!audit_enabled)
+		return rc;
+
+	ab = audit_log_start(audit_context(), GFP_KERNEL, AUDIT_CONTAINER_OP);
+	if (!ab)
+		return rc;
+
+	uid = from_kuid(&init_user_ns, task_uid(current));
+	tty = audit_get_tty();
+	audit_log_format(ab,
+			 "op=set opid=%d contid=%llu old-contid=%llu pid=%d uid=%u auid=%u tty=%s ses=%u",
+			 task_tgid_nr(task), contid, oldcontid,
+			 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_log_end - end one audit record
  * @ab: the audit_buffer
  *

kernel/audit.h +1 -0

index c00e2ee3c6b3..e2912924af0d 100644
@@ -148,6 +148,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 fd7ca983de4f..1f7edf035b16 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;
 };

@@ -2368,6 +2369,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);
 }
 

@@ -2408,6 +2410,7 @@ int audit_signal_info(int sig, 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;
 	}

@@ -2429,6 +2432,7 @@ int audit_signal_info(int sig, 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++;

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]>

Acked-by Serge Hallyn <[email protected]>

Acked-by Neil Horman <[email protected]>

Reviewed-by Ondrej Mosnacek <[email protected]>

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

fs/proc/base.c +22 -3

index 43fd0c4b87de..acc70239d0cb 100644
@@ -1211,7 +1211,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)
 {

@@ -1295,6 +1295,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)
 {

@@ -1325,6 +1343,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,
 };

@@ -3067,7 +3086,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),

@@ -3466,7 +3485,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),

Quoted 7 lines · Richard Guy Briggs

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.

Quoted 3 lines

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

This read requires CAP_AUDIT_CONTROL.

This scares me. As this seems to make it easy to reuse an audit
containerid for non-audit purporses.

I would think it would be safer and easier to poke audit and ask it to log a message with your audit container id.

Eric

Quoted 64 lines

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

Acked-by Serge Hallyn <[email protected]>

Acked-by Neil Horman <[email protected]>

Reviewed-by Ondrej Mosnacek <[email protected]>

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

fs/proc/base.c +21 -2

index 43fd0c4b87de..acc70239d0cb 100644
@@ -1211,7 +1211,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)
 {

@@ -1295,6 +1295,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)
 {

@@ -1325,6 +1343,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,
 };

@@ -3067,7 +3086,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),

@@ -3466,7 +3485,7 @@ static int proc_tid_comm_permission(struct inode *inode, int mask)

fs/proc/base.c static int proc_stack_depth(struct seq_file *m, struct pid_namespace *ns, · line 3092

Quoted 8 lines

fs/proc/base.c +1 -1

 #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),

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

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 Serge Hallyn <[email protected]>

Acked-by Steve Grubb <[email protected]>

Acked-by Neil Horman <[email protected]>

Reviewed-by Ondrej Mosnacek <[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(-)

kernel/audit.c +6 -0

index 182b0f2c183d..3e0af53f3c4d 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_contid - report container info
+ * @context: task or local context for record
+ * @contid: container ID to report
+ */
+void audit_log_contid(struct audit_context *context, u64 contid)

kernel/audit.c +3 -0

+       audit_log_end(ab);
+}
+EXPORT_SYMBOL(audit_log_contid);

Quoted 33 lines · Richard Guy Briggs

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

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 Serge Hallyn <[email protected]>

Acked-by Steve Grubb <[email protected]>

Acked-by Neil Horman <[email protected]>

Reviewed-by Ondrej Mosnacek <[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(-)

...

Quoted 23 lines

kernel/audit.c +15 -0

index 182b0f2c183d..3e0af53f3c4d 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_contid - report container info
+ * @context: task or local context for record
+ * @contid: container ID to report
+ */
+void audit_log_contid(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;

kernel/audit.c void audit_log_session_info(struct audit_buffer *ab) · line 2145

Quoted 1 line

kernel/audit.c +1 -0

+       audit_log_format(ab, "contid=%llu", (unsigned long long)contid);

We have a consistency problem regarding how to output the u64 contid values; this function uses an explicit cast, others do not. According to Documentation/core-api/printk-formats.rst the recommendation for u64 is %llu (or %llx, if you want hex). Looking quickly through the printk code this appears to still be correct. I suggest we get rid of the cast (like it was in v5).

kernel/audit.c void audit_log_session_info(struct audit_buffer *ab) · line 2148

Quoted 4 lines

kernel/audit.c +3 -0

+       audit_log_end(ab);
+}
+EXPORT_SYMBOL(audit_log_contid);

--
paul moore
www.paul-moore.com

Quoted 60 lines · Paul Moore

Quoted 33 lines · Richard Guy Briggs

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

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 Serge Hallyn <[email protected]>

Acked-by Steve Grubb <[email protected]>

Acked-by Neil Horman <[email protected]>

Reviewed-by Ondrej Mosnacek <[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(-)

...

Quoted 23 lines

kernel/audit.c +15 -0

index 182b0f2c183d..3e0af53f3c4d 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_contid - report container info
+ * @context: task or local context for record
+ * @contid: container ID to report
+ */
+void audit_log_contid(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;

Quoted 8 lines

kernel/audit.c void audit_log_session_info(struct audit_buffer *ab) · line 2145

Quoted 1 line

kernel/audit.c +1 -0

+       audit_log_format(ab, "contid=%llu", (unsigned long long)contid);

We have a consistency problem regarding how to output the u64 contid values; this function uses an explicit cast, others do not. According to Documentation/core-api/printk-formats.rst the recommendation for u64 is %llu (or %llx, if you want hex). Looking quickly through the printk code this appears to still be correct. I suggest we get rid of the cast (like it was in v5).

IIRC it was me who suggested to add the casts. I didn't realize that the kernel actually guarantees that "%llu" will always work with u64. Taking that into account I rescind my request to add the cast. Sorry for the false alarm.

Quoted 7 lines

kernel/audit.c void audit_log_session_info(struct audit_buffer *ab) · line 2148

Quoted 3 lines

kernel/audit.c +3 -0

+       audit_log_end(ab);
+}
+EXPORT_SYMBOL(audit_log_contid);

--
paul moore
www.paul-moore.com

--
Ondrej Mosnacek <omosnace at redhat dot com>
Software Engineer, Security Technologies
Red Hat, Inc.

--
Linux-audit mailing list
[email protected]
https://www.redhat.com/mailman/listinfo/linux-audit

Quoted 70 lines · Ondrej Mosnacek

Quoted 68 lines · Paul Moore

Quoted 33 lines · Richard Guy Briggs

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

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 Serge Hallyn <[email protected]>

Acked-by Steve Grubb <[email protected]>

Acked-by Neil Horman <[email protected]>

Reviewed-by Ondrej Mosnacek <[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(-)

...

Quoted 14 lines

kernel/audit.c +6 -0

index 182b0f2c183d..3e0af53f3c4d 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_contid - report container info
+ * @context: task or local context for record
+ * @contid: container ID to report
+ */
+void audit_log_contid(struct audit_context *context, u64 contid)

kernel/audit.c void audit_log_session_info(struct audit_buffer *ab) · line 2145

Quoted 10 lines

kernel/audit.c +10 -0

+{
+       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", (unsigned long long)contid);

We have a consistency problem regarding how to output the u64 contid values; this function uses an explicit cast, others do not. According to Documentation/core-api/printk-formats.rst the recommendation for u64 is %llu (or %llx, if you want hex). Looking quickly through the printk code this appears to still be correct. I suggest we get rid of the cast (like it was in v5).

Quoted 4 lines

IIRC it was me who suggested to add the casts. I didn't realize that the kernel actually guarantees that "%llu" will always work with u64. Taking that into account I rescind my request to add the cast. Sorry for the false alarm.

Yeah, just remove the cast.

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              | 27 +++++++++++++++++++++++++++
 kernel/audit.h              |  1 +
 kernel/auditsc.c            |  1 +
 security/selinux/nlmsgtab.c |  1 +
 6 files changed, 38 insertions(+)

include/linux/audit.h +7 -0

index 43438192ca2a..c2dec9157463 100644
@@ -37,6 +37,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 55fde9970762..10cc67926cf1 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 3e0af53f3c4d..87e1d367f98c 100644
@@ -138,6 +138,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]

@@ -1097,6 +1098,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:

@@ -1260,6 +1262,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;
 

@@ -1519,6 +1522,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.h +1 -0

index e2912924af0d..c5ac6436317e 100644
@@ -345,6 +345,7 @@ extern void audit_filter_inodes(struct task_struct *tsk,
 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);
 

kernel/auditsc.c +1 -0

index eea445b7a181..0a29a00feaf1 100644
@@ -2405,6 +2405,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);
        }
 
        if (!audit_signals || audit_dummy_context())

security/selinux/nlmsgtab.c +1 -1

index 9cec81209617..682fe7397762 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     },
--