audit: implement container identifier

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

The "op" field indicates an initial set. 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.

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

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

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

kernel/audit.c +24 -0

index 397f8fb4836a..2d7707426b7d 100644
@@ -2356,6 +2358,62 @@ int audit_signal_info(int sig, struct task_struct *t)
        return audit_signal_info_syscall(t);
 }
 
+/*
+ * audit_set_contid - set current task's audit contid
+ * @task: target task
+ * @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;
+
+       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 */

kernel/audit.c +6 -0

+       read_unlock(&tasklist_lock);
+       if (!rc)
+               task->audit->contid = contid;
+       task_unlock(task);
+
+       if (!audit_enabled)

kernel/audit.c +3 -0

+       return rc;
+}
+
 /**
  * audit_log_end - end one audit record
  * @ab: the audit_buffer

Quoted 46 lines · Richard Guy Briggs

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

The "op" field indicates an initial set. 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.

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

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

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

...

Quoted 32 lines

kernel/audit.c +24 -0

index 397f8fb4836a..2d7707426b7d 100644
@@ -2356,6 +2358,62 @@ int audit_signal_info(int sig, struct task_struct *t)
        return audit_signal_info_syscall(t);
 }
 
+/*
+ * audit_set_contid - set current task's audit contid
+ * @task: target task
+ * @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;
+
+       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 */

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

Quoted 10 lines

kernel/audit.c +10 -0

+       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;

[NOTE: there is a bigger issue below which I think is going to require a respin/fixup of this patch so I'm going to take the opportunity to do a bit more bikeshedding ;)]

It seems like we could combine both the thread/children checks under a single -EBUSY return value. In both cases the caller should be able to determine if the target process is multi-threaded for has spawned children, yes? FWIW, my motivation for this question is that -EALREADY seems like a poor choice here.

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

Quoted 3 lines

kernel/audit.c +3 -0

+       /* if contid is already set, deny */
+       else if (audit_contid_set(task))
+               rc = -ECHILD;

Does -EEXIST make more sense here?

Quoted 6 lines

kernel/audit.c +6 -0

+       read_unlock(&tasklist_lock);
+       if (!rc)
+               task->audit->contid = contid;
+       task_unlock(task);
+
+       if (!audit_enabled)

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

Quoted 10 lines

kernel/audit.c +10 -0

+               return rc;
+
+       ab = audit_log_start(audit_context(), GFP_KERNEL, AUDIT_CONTAINER_OP);
+       if (!ab)
+               return rc;
+
+       audit_log_format(ab,
+                        "op=set opid=%d contid=%llu old-contid=%llu",
+                        task_tgid_nr(task), contid, oldcontid);
+       audit_log_end(ab);

Assuming audit is enabled we always emit the record above, even if we were not actually able to set the Audit Container ID (ACID); this seems wrong to me. I think the proper behavior would be to either add a "res=" field to indicate success/failure or only emit the record when we actually change a task's ACID. Considering the impact that the ACID value will potentially have on the audit stream, it seems like always logging the record and including a "res=" field may be the safer choice.

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

Quoted 6 lines

kernel/audit.c +3 -0

+       return rc;
+}
+
 /**
  * audit_log_end - end one audit record
  * @ab: the audit_buffer

--
paul moore
www.paul-moore.com

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

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

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

include/linux/audit.h +11 -3

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

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

kernel/audit.c +0 -0

index 2d7707426b7d..4bab20f5f781 100644

kernel/audit.c +12 -0

+/* audit_contobj_list_lock must be held by caller */
+static void _audit_contobj_put(struct audit_contobj *cont)
+{
+       if (!cont)
+               return;
+       if (refcount_dec_and_test(&cont->refcount)) {
+               put_task_struct(cont->owner);
+               list_del_rcu(&cont->list);
+               kfree_rcu(cont, rcu);
+       }
+}
+
 /**
  * audit_alloc - allocate an audit info block for a task
  * @tsk: task

kernel/audit.c +3 -0

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

@@ -2365,6 +2406,9 @@ int audit_signal_info(int sig, struct task_struct *t)
  *
  * Returns 0 on success, -EPERM on permission failure.
  *
+ * If the original container owner goes away, no task injection is
+ * possible to an existing container.
+ *
  * Called (set) from fs/proc/base.c::proc_contid_write().
  */
 int audit_set_contid(struct task_struct *task, u64 contid)

kernel/audit.c +0 -2

        /* if we don't have caps, reject */
        else if (!capable(CAP_AUDIT_CONTROL))
                rc = -EPERM;

@@ -2396,8 +2443,49 @@ int audit_set_contid(struct task_struct *task, u64 contid)
        else if (audit_contid_set(task))
                rc = -ECHILD;
        read_unlock(&tasklist_lock);
-       if (!rc)
-               task->audit->contid = contid;

kernel/audit.c +4 -0

+                                       newcont = cont;
+                               } else {
+                                       rc = -ENOTUNIQ;
+                                       goto conterror;

kernel/audit.c +3 -0

+conterror:
+               rcu_read_unlock();
+       }
        task_unlock(task);
 
        if (!audit_enabled)

Quoted 15 lines · Richard Guy Briggs

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

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

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

...

Quoted 44 lines

include/linux/audit.h +11 -3

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

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

kernel/audit.c +0 -0

index 2d7707426b7d..4bab20f5f781 100644
@@ -212,6 +218,31 @@ void __init audit_task_init(void)
                                             0, SLAB_PANIC, NULL);
 }
 

kernel/audit.c void __init audit_task_init(void) · line 225

Quoted 5 lines

kernel/audit.c +5 -0

+static struct audit_contobj *_audit_contobj(struct task_struct *tsk)
+{
+       if (!tsk->audit)
+               return NULL;
+       return tsk->audit->cont;

It seems like it would be safer to grab a reference here (e.g. _audit_contobj_hold(...)), yes? Or are you confident we will never have tsk go away while the caller is holding on to the returned audit container ID object?

Quoted 5 lines

kernel/audit.c +5 -0

+}
+
+/* audit_contobj_list_lock must be held by caller unless new */
+static void _audit_contobj_hold(struct audit_contobj *cont)
+{

kernel/audit.c void __init audit_task_init(void) · line 232

Quoted 2 lines

kernel/audit.c +2 -0

+       refcount_inc(&cont->refcount);
+}

If we are going to call the matching decrement function "_put" it seems like we might want to call the function about "_get". Further, we can also have it return an audit_contobj pointer in case the caller needs to do an assignment as well (which seems typical if you need to bump the refcount):

Quoted 16 lines

kernel/audit.c +12 -0

+/* audit_contobj_list_lock must be held by caller */
+static void _audit_contobj_put(struct audit_contobj *cont)
+{
+       if (!cont)
+               return;
+       if (refcount_dec_and_test(&cont->refcount)) {
+               put_task_struct(cont->owner);
+               list_del_rcu(&cont->list);
+               kfree_rcu(cont, rcu);
+       }
+}
+
 /**
  * audit_alloc - allocate an audit info block for a task
  * @tsk: task

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

Quoted 9 lines

kernel/audit.c +5 -1

@@ -232,7 +263,11 @@ 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);
+       spin_lock(&audit_contobj_list_lock);
+       info->cont = _audit_contobj(current);
+       if (info->cont)
+               _audit_contobj_hold(info->cont);
+       spin_unlock(&audit_contobj_list_lock);

If we are taking a spinlock in order to bump the refcount, does it really need to be a refcount_t or can we just use a normal integer? In RCU protected lists a spinlock is usually used to protect adds/removes to the list, not the content of individual list items.

My guess is you probably want to use the spinlock as described above (list add/remove protection) and manipulate the refcount_t inside a RCU read lock protected region.

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

Quoted 10 lines

kernel/audit.c +3 -0

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

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

This is another case of refcount_t vs normal integer in a spinlock protected region.

Quoted 18 lines

kernel/audit.c +3 -1

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

@@ -2365,6 +2406,9 @@ int audit_signal_info(int sig, struct task_struct *t)
  *
  * Returns 0 on success, -EPERM on permission failure.
  *
+ * If the original container owner goes away, no task injection is
+ * possible to an existing container.
+ *
  * Called (set) from fs/proc/base.c::proc_contid_write().
  */
 int audit_set_contid(struct task_struct *task, u64 contid)

@@ -2381,9 +2425,12 @@ int audit_set_contid(struct task_struct *task, u64 contid)
        }
        oldcontid = audit_get_contid(task);
        read_lock(&tasklist_lock);
-       /* Don't allow the audit containerid to be unset */

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

Quoted 6 lines

kernel/audit.c +4 -0

+       /* Don't allow the contid to be unset */
        if (!audit_contid_valid(contid))
                rc = -EINVAL;
+       /* Don't allow the contid to be set to the same value again */
+       else if (contid == oldcontid) {
+               rc = -EADDRINUSE;

First, is that brace a typo? It looks like it. Did this compile?

Second, can you explain why (re)setting the audit container ID to the same value is something we need to prohibit? I'm guessing it has something to do with explicitly set vs inherited, but I don't want to assume too much about your thinking behind this.

If it is "set vs inherited", would allowing an orchestrator to explicitly "set" an inherited audit container ID provide some level or protection against moving the task?

Quoted 16 lines

kernel/audit.c +7 -2

        /* if we don't have caps, reject */
        else if (!capable(CAP_AUDIT_CONTROL))
                rc = -EPERM;

@@ -2396,8 +2443,49 @@ int audit_set_contid(struct task_struct *task, u64 contid)
        else if (audit_contid_set(task))
                rc = -ECHILD;
        read_unlock(&tasklist_lock);
-       if (!rc)
-               task->audit->contid = contid;
+       if (!rc) {
+               struct audit_contobj *oldcont = _audit_contobj(task);
+               struct audit_contobj *cont = NULL, *newcont = NULL;
+               int h = audit_hash_contid(contid);
+
+               rcu_read_lock();
+               list_for_each_entry_rcu(cont, &audit_contid_hash[h], list)

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

Quoted 3 lines

kernel/audit.c +3 -0

+                       if (cont->id == contid) {
+                               /* task injection to existing container */
+                               if (current == cont->owner) {

Do we have any protection against the task pointed to by cont->owner going away and a new task with the same current pointer value (no longer the legitimate audit container ID owner) manipulating the target task's audit container ID?

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

Quoted 3 lines

kernel/audit.c +3 -0

+                                       spin_lock(&audit_contobj_list_lock);
+                                       _audit_contobj_hold(cont);
+                                       spin_unlock(&audit_contobj_list_lock);

More of the recount_t vs integer/spinlock question.

Quoted 4 lines

kernel/audit.c +4 -0

+                                       newcont = cont;
+                               } else {
+                                       rc = -ENOTUNIQ;
+                                       goto conterror;

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

Quoted 10 lines

kernel/audit.c +10 -0

+                               }
+                               break;
+                       }
+               if (!newcont) {
+                       newcont = kmalloc(sizeof(*newcont), GFP_ATOMIC);
+                       if (newcont) {
+                               INIT_LIST_HEAD(&newcont->list);
+                               newcont->id = contid;
+                               get_task_struct(current);
+                               newcont->owner = current;

newcont->owner = get_task_struct(current);

(This is what I was talking about above with returning the struct pointer in the _get/_hold function)

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

Quoted 4 lines

kernel/audit.c +4 -0

+                               refcount_set(&newcont->refcount, 1);
+                               spin_lock(&audit_contobj_list_lock);
+                               list_add_rcu(&newcont->list, &audit_contid_hash[h]);
+                               spin_unlock(&audit_contobj_list_lock);

I think we might have a problem where multiple tasks could race adding the same audit container ID and since there is no check inside the spinlock protected region we could end up adding multiple instances of the same audit container ID, yes?

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

Quoted 9 lines

kernel/audit.c +9 -0

+                       } else {
+                               rc = -ENOMEM;
+                               goto conterror;
+                       }
+               }
+               task->audit->cont = newcont;
+               spin_lock(&audit_contobj_list_lock);
+               _audit_contobj_put(oldcont);
+               spin_unlock(&audit_contobj_list_lock);

More of the refcount_t/integer/spinlock question.

Quoted 5 lines

kernel/audit.c +3 -0

+conterror:
+               rcu_read_unlock();
+       }
        task_unlock(task);
 

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

Quoted 1 line

kernel/audit.c +0 -0

        if (!audit_enabled)

--
paul moore
www.paul-moore.com

Quoted 16 lines · Paul Moore

Quoted 15 lines · Richard Guy Briggs

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

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

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

Quoted 16 lines

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

Quoted 10 lines

kernel/audit.c +5 -1

@@ -232,7 +263,11 @@ 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);
+       spin_lock(&audit_contobj_list_lock);
+       info->cont = _audit_contobj(current);
+       if (info->cont)
+               _audit_contobj_hold(info->cont);
+       spin_unlock(&audit_contobj_list_lock);

If we are taking a spinlock in order to bump the refcount, does it really need to be a refcount_t or can we just use a normal integer? In RCU protected lists a spinlock is usually used to protect adds/removes to the list, not the content of individual list items.

Quoted 3 lines

My guess is you probably want to use the spinlock as described above (list add/remove protection) and manipulate the refcount_t inside a RCU read lock protected region.

Ok, I guess it could be an integer if it were protected by the spinlock, but I think you've guessed my intent, so let us keep it as a refcount and tighten the spinlock scope and use rcu read locking to protect _get and _put in _alloc, _free, and later on when protecting the network namespace contobj lists. This should reduce potential contention for the spinlock to one location over fewer lines of code in that place while speeding up updates and slightly simplifying code in the others.

Quoted 5 lines

Quoted 5 lines

kernel/audit.c +0 -1

@@ -2381,9 +2425,12 @@ int audit_set_contid(struct task_struct *task, u64 contid)
        }
        oldcontid = audit_get_contid(task);
        read_lock(&tasklist_lock);
-       /* Don't allow the audit containerid to be unset */

Quoted 8 lines

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

Quoted 6 lines

kernel/audit.c +4 -0

+       /* Don't allow the contid to be unset */
        if (!audit_contid_valid(contid))
                rc = -EINVAL;
+       /* Don't allow the contid to be set to the same value again */
+       else if (contid == oldcontid) {
+               rc = -EADDRINUSE;

First, is that brace a typo? It looks like it. Did this compile?

Yes, it was fixed in the later patch that restructured the if statements.

Quoted 4 lines

Second, can you explain why (re)setting the audit container ID to the same value is something we need to prohibit? I'm guessing it has something to do with explicitly set vs inherited, but I don't want to assume too much about your thinking behind this.

It made the refcounting more complicated later, and besides, the prohibition on setting the contid again if it is already set would catch this case, so I'll remove it in this patch and ensure this action doesn't cause a problem in later patches.

Quoted 3 lines

If it is "set vs inherited", would allowing an orchestrator to explicitly "set" an inherited audit container ID provide some level or protection against moving the task?

I can't see it helping prevent this since later descendancy checks will stop this move anyways.

Quoted 13 lines

Quoted 13 lines

kernel/audit.c +7 -2

@@ -2396,8 +2443,49 @@ int audit_set_contid(struct task_struct *task, u64 contid)
        else if (audit_contid_set(task))
                rc = -ECHILD;
        read_unlock(&tasklist_lock);
-       if (!rc)
-               task->audit->contid = contid;
+       if (!rc) {
+               struct audit_contobj *oldcont = _audit_contobj(task);
+               struct audit_contobj *cont = NULL, *newcont = NULL;
+               int h = audit_hash_contid(contid);
+
+               rcu_read_lock();
+               list_for_each_entry_rcu(cont, &audit_contid_hash[h], list)

Quoted 8 lines

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

Quoted 3 lines

kernel/audit.c +3 -0

+                       if (cont->id == contid) {
+                               /* task injection to existing container */
+                               if (current == cont->owner) {

Do we have any protection against the task pointed to by cont->owner going away and a new task with the same current pointer value (no longer the legitimate audit container ID owner) manipulating the target task's audit container ID?

Yes, the get_task_struct() call below.

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

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

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

kernel/audit.c +6 -0

index 4bab20f5f781..fa8f1aa3a605 100644
@@ -2502,6 +2502,23 @@ int audit_set_contid(struct task_struct *task, u64 contid)
        return rc;
 }
 
+void audit_log_container_drop(void)
+{
+       struct audit_buffer *ab;
+
+       if (!current->audit || !current->audit->cont ||
+           refcount_read(&current->audit->cont->refcount) > 1)

kernel/audit.c +0 -0

 /**
  * audit_log_end - end one audit record
  * @ab: the audit_buffer

kernel/audit.h +2 -0

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

kernel/auditsc.c +2 -1

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

1.8.3.1

Quoted 33 lines · Richard Guy Briggs

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

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

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

kernel/audit.c +12 -0

index 4bab20f5f781..fa8f1aa3a605 100644
@@ -2502,6 +2502,23 @@ int audit_set_contid(struct task_struct *task, u64 contid)
        return rc;
 }
 
+void audit_log_container_drop(void)
+{
+       struct audit_buffer *ab;
+
+       if (!current->audit || !current->audit->cont ||
+           refcount_read(&current->audit->cont->refcount) > 1)
+               return;
+       ab = audit_log_start(audit_context(), GFP_KERNEL, AUDIT_CONTAINER_OP);
+       if (!ab)
+               return;
+
+       audit_log_format(ab, "op=drop opid=%d contid=%llu old-contid=%llu",

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

Quoted 4 lines

kernel/audit.c +4 -0

+                        task_tgid_nr(current), audit_get_contid(current),
+                        audit_get_contid(current));
+       audit_log_end(ab);
+}

Assumine we are careful about where we call it in audit_free(...), you are confident we can't do this as part of _audit_contobj_put(...), yes?

Quoted 24 lines

kernel/audit.c +0 -0

 /**
  * audit_log_end - end one audit record
  * @ab: the audit_buffer

kernel/audit.h +2 -0

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

kernel/auditsc.c +0 -0

index 0e2d50533959..bd855794ad26 100644
@@ -1568,6 +1568,8 @@ static void audit_log_exit(void)
 
        audit_log_proctitle();
 

Quoted 8 lines

kernel/auditsc.c +2 -1

+       audit_log_container_drop();
+
        /* Send end of event record to help user space know we are finished */
        ab = audit_log_start(context, GFP_KERNEL, AUDIT_EOE);
        if (ab)
--

1.8.3.1

--
paul moore
www.paul-moore.com

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              | 35 +++++++++++++++++++++++++++++++++++
 kernel/audit.h              |  1 +
 security/selinux/nlmsgtab.c |  1 +
 5 files changed, 45 insertions(+)

kernel/audit.c +0 -0

index 0871c3e5d6df..51159c94041c 100644
@@ -126,6 +126,14 @@ struct auditd_connection {
 kuid_t         audit_sig_uid = INVALID_UID;

Quoted 19 lines · Richard Guy Briggs

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              | 35 +++++++++++++++++++++++++++++++++++
 kernel/audit.h              |  1 +
 security/selinux/nlmsgtab.c |  1 +
 5 files changed, 45 insertions(+)

...

Quoted 6 lines

kernel/audit.c +0 -0

index 0871c3e5d6df..51159c94041c 100644
@@ -126,6 +126,14 @@ struct auditd_connection {
 kuid_t         audit_sig_uid = INVALID_UID;

kernel/audit.c struct auditd_connection { · line 136

Quoted 10 lines

kernel/audit.c +8 -0

 pid_t          audit_sig_pid = -1;
 u32            audit_sig_sid = 0;
+/* Since the signal information is stored in the record buffer at the
+ * time of the signal, but not retrieved until later, there is a chance
+ * that the last process in the container could terminate before the
+ * signal record is delivered.  In this circumstance, there is a chance
+ * the orchestrator could reuse the audit container identifier, causing
+ * an overlap of audit records that refer to the same audit container
+ * identifier, but a different container instance.  */
+u64            audit_sig_cid = AUDIT_CID_UNSET;

I believe we could prevent the case mentioned above by taking an additional reference to the audit container ID object when the signal information is collected, dropping it only after the signal information is collected by userspace or another process signals the audit daemon. Yes, it would block that audit container ID from being reused immediately, but since we are talking about one number out of 2^64 that seems like a reasonable tradeoff.

paul moore

Quoted 40 lines · Paul Moore

Quoted 19 lines · Richard Guy Briggs

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              | 35 +++++++++++++++++++++++++++++++++++
 kernel/audit.h              |  1 +
 security/selinux/nlmsgtab.c |  1 +
 5 files changed, 45 insertions(+)

...

Quoted 6 lines

kernel/audit.c +0 -0

index 0871c3e5d6df..51159c94041c 100644
@@ -126,6 +126,14 @@ struct auditd_connection {
 kuid_t         audit_sig_uid = INVALID_UID;

kernel/audit.c struct auditd_connection { · line 136

Quoted 10 lines

kernel/audit.c +8 -0

 pid_t          audit_sig_pid = -1;
 u32            audit_sig_sid = 0;
+/* Since the signal information is stored in the record buffer at the
+ * time of the signal, but not retrieved until later, there is a chance
+ * that the last process in the container could terminate before the
+ * signal record is delivered.  In this circumstance, there is a chance
+ * the orchestrator could reuse the audit container identifier, causing
+ * an overlap of audit records that refer to the same audit container
+ * identifier, but a different container instance.  */
+u64            audit_sig_cid = AUDIT_CID_UNSET;

Quoted 7 lines

I believe we could prevent the case mentioned above by taking an additional reference to the audit container ID object when the signal information is collected, dropping it only after the signal information is collected by userspace or another process signals the audit daemon. Yes, it would block that audit container ID from being reused immediately, but since we are talking about one number out of 2^64 that seems like a reasonable tradeoff.

I had thought that through and should have been more explicit about that situation when I documented it. We could do that, but then the syscall records would be connected with the call from auditd on shutdown to request that signal information, rather than the exit of that last process that was using that container. This strikes me as misleading. Is that really what we want?

Quoted 49 lines · Richard Guy Briggs

Quoted 40 lines · Paul Moore

Quoted 19 lines · Richard Guy Briggs

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              | 35 +++++++++++++++++++++++++++++++++++
 kernel/audit.h              |  1 +
 security/selinux/nlmsgtab.c |  1 +
 5 files changed, 45 insertions(+)

...

Quoted 6 lines

kernel/audit.c +0 -0

index 0871c3e5d6df..51159c94041c 100644
@@ -126,6 +126,14 @@ struct auditd_connection {
 kuid_t         audit_sig_uid = INVALID_UID;

kernel/audit.c struct auditd_connection { · line 136

Quoted 10 lines

kernel/audit.c +8 -0

 pid_t          audit_sig_pid = -1;
 u32            audit_sig_sid = 0;
+/* Since the signal information is stored in the record buffer at the
+ * time of the signal, but not retrieved until later, there is a chance
+ * that the last process in the container could terminate before the
+ * signal record is delivered.  In this circumstance, there is a chance
+ * the orchestrator could reuse the audit container identifier, causing
+ * an overlap of audit records that refer to the same audit container
+ * identifier, but a different container instance.  */
+u64            audit_sig_cid = AUDIT_CID_UNSET;

Quoted 7 lines

I believe we could prevent the case mentioned above by taking an additional reference to the audit container ID object when the signal information is collected, dropping it only after the signal information is collected by userspace or another process signals the audit daemon. Yes, it would block that audit container ID from being reused immediately, but since we are talking about one number out of 2^64 that seems like a reasonable tradeoff.

Quoted 6 lines

I had thought that through and should have been more explicit about that situation when I documented it. We could do that, but then the syscall records would be connected with the call from auditd on shutdown to request that signal information, rather than the exit of that last process that was using that container. This strikes me as misleading. Is that really what we want?

???

I think one of us is not understanding the other; maybe it's me, maybe it's you, maybe it's both of us.

Anyway, here is what I was trying to convey with my original comment ... When we record the audit container ID in audit_signal_info() we take an extra reference to the audit container ID object so that it will not disappear (and get reused) until after we respond with an AUDIT_SIGNAL_INFO2. In audit_receive_msg() when we do the AUDIT_SIGNAL_INFO2 processing we drop the extra reference we took in audit_signal_info(). Unless I'm missing some other change you made, this *shouldn't* affect the syscall records, all it does is preserve the audit container ID object in the kernel's ACID store so it doesn't get reused.

(We do need to do some extra housekeeping in audit_signal_info() to deal with the case where nobody asks for AUDIT_SIGNAL_INFO2 - basically if audit_sig_cid is not NULL we should drop a reference before assigning it a new object pointer, and of course we would need to set audit_sig_cid to NULL in audit_receive_msg() after sending it up to userspace and dropping the extra ref.)

paul moore

Quoted 62 lines · Paul Moore

Quoted 49 lines · Richard Guy Briggs

Quoted 40 lines · Paul Moore

Quoted 19 lines · Richard Guy Briggs

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              | 35 +++++++++++++++++++++++++++++++++++
 kernel/audit.h              |  1 +
 security/selinux/nlmsgtab.c |  1 +
 5 files changed, 45 insertions(+)

...

Quoted 6 lines

kernel/audit.c +0 -0

index 0871c3e5d6df..51159c94041c 100644
@@ -126,6 +126,14 @@ struct auditd_connection {
 kuid_t         audit_sig_uid = INVALID_UID;

kernel/audit.c struct auditd_connection { · line 136

Quoted 10 lines

kernel/audit.c +8 -0

 pid_t          audit_sig_pid = -1;
 u32            audit_sig_sid = 0;
+/* Since the signal information is stored in the record buffer at the
+ * time of the signal, but not retrieved until later, there is a chance
+ * that the last process in the container could terminate before the
+ * signal record is delivered.  In this circumstance, there is a chance
+ * the orchestrator could reuse the audit container identifier, causing
+ * an overlap of audit records that refer to the same audit container
+ * identifier, but a different container instance.  */
+u64            audit_sig_cid = AUDIT_CID_UNSET;

Quoted 7 lines

I believe we could prevent the case mentioned above by taking an additional reference to the audit container ID object when the signal information is collected, dropping it only after the signal information is collected by userspace or another process signals the audit daemon. Yes, it would block that audit container ID from being reused immediately, but since we are talking about one number out of 2^64 that seems like a reasonable tradeoff.

Quoted 6 lines

I had thought that through and should have been more explicit about that situation when I documented it. We could do that, but then the syscall records would be connected with the call from auditd on shutdown to request that signal information, rather than the exit of that last process that was using that container. This strikes me as misleading. Is that really what we want?

???

I think one of us is not understanding the other; maybe it's me, maybe it's you, maybe it's both of us.

Quoted 10 lines

Anyway, here is what I was trying to convey with my original comment ... When we record the audit container ID in audit_signal_info() we take an extra reference to the audit container ID object so that it will not disappear (and get reused) until after we respond with an AUDIT_SIGNAL_INFO2. In audit_receive_msg() when we do the AUDIT_SIGNAL_INFO2 processing we drop the extra reference we took in audit_signal_info(). Unless I'm missing some other change you made, this *shouldn't* affect the syscall records, all it does is preserve the audit container ID object in the kernel's ACID store so it doesn't get reused.

This is exactly what I had understood. I hadn't considered the extra details below in detail due to my original syscall concern, but they make sense.

The syscall I refer to is the one connected with the drop of the audit container identifier by the last process that was in that container in patch 5/16. The production of this record is contingent on the last ref in a contobj being dropped. So if it is due to that ref being maintained by audit_signal_info() until the AUDIT_SIGNAL_INFO2 record it fetched, then it will appear that the fetch action closed the container rather than the last process in the container to exit.

Does this make sense?

Quoted 6 lines

(We do need to do some extra housekeeping in audit_signal_info() to deal with the case where nobody asks for AUDIT_SIGNAL_INFO2 - basically if audit_sig_cid is not NULL we should drop a reference before assigning it a new object pointer, and of course we would need to set audit_sig_cid to NULL in audit_receive_msg() after sending it up to userspace and dropping the extra ref.)

Quoted 86 lines · Richard Guy Briggs

Quoted 62 lines · Paul Moore

Quoted 49 lines · Richard Guy Briggs

Quoted 40 lines · Paul Moore

Quoted 19 lines · Richard Guy Briggs

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              | 35 +++++++++++++++++++++++++++++++++++
 kernel/audit.h              |  1 +
 security/selinux/nlmsgtab.c |  1 +
 5 files changed, 45 insertions(+)

...

Quoted 6 lines

kernel/audit.c +0 -0

index 0871c3e5d6df..51159c94041c 100644
@@ -126,6 +126,14 @@ struct auditd_connection {
 kuid_t         audit_sig_uid = INVALID_UID;

kernel/audit.c struct auditd_connection { · line 136

Quoted 10 lines

kernel/audit.c +8 -0

 pid_t          audit_sig_pid = -1;
 u32            audit_sig_sid = 0;
+/* Since the signal information is stored in the record buffer at the
+ * time of the signal, but not retrieved until later, there is a chance
+ * that the last process in the container could terminate before the
+ * signal record is delivered.  In this circumstance, there is a chance
+ * the orchestrator could reuse the audit container identifier, causing
+ * an overlap of audit records that refer to the same audit container
+ * identifier, but a different container instance.  */
+u64            audit_sig_cid = AUDIT_CID_UNSET;

Quoted 7 lines

I believe we could prevent the case mentioned above by taking an additional reference to the audit container ID object when the signal information is collected, dropping it only after the signal information is collected by userspace or another process signals the audit daemon. Yes, it would block that audit container ID from being reused immediately, but since we are talking about one number out of 2^64 that seems like a reasonable tradeoff.

Quoted 6 lines

I had thought that through and should have been more explicit about that situation when I documented it. We could do that, but then the syscall records would be connected with the call from auditd on shutdown to request that signal information, rather than the exit of that last process that was using that container. This strikes me as misleading. Is that really what we want?

???

I think one of us is not understanding the other; maybe it's me, maybe it's you, maybe it's both of us.

Quoted 10 lines

Anyway, here is what I was trying to convey with my original comment ... When we record the audit container ID in audit_signal_info() we take an extra reference to the audit container ID object so that it will not disappear (and get reused) until after we respond with an AUDIT_SIGNAL_INFO2. In audit_receive_msg() when we do the AUDIT_SIGNAL_INFO2 processing we drop the extra reference we took in audit_signal_info(). Unless I'm missing some other change you made, this *shouldn't* affect the syscall records, all it does is preserve the audit container ID object in the kernel's ACID store so it doesn't get reused.

This is exactly what I had understood. I hadn't considered the extra details below in detail due to my original syscall concern, but they make sense.

The syscall I refer to is the one connected with the drop of the audit container identifier by the last process that was in that container in patch 5/16. The production of this record is contingent on the last ref in a contobj being dropped. So if it is due to that ref being maintained by audit_signal_info() until the AUDIT_SIGNAL_INFO2 record it fetched, then it will appear that the fetch action closed the container rather than the last process in the container to exit.

Quoted 1 line

Does this make sense?

More so than your original reply, at least to me anyway.

It makes sense that the audit container ID wouldn't be marked as "dead" since it would still be very much alive and available for use by the orchestrator, the question is if that is desirable or not. I think the answer to this comes down the preserving the correctness of the audit log.

If the audit container ID reported by AUDIT_SIGNAL_INFO2 has been reused then I think there is a legitimate concern that the audit log is not correct, and could be misleading. If we solve that by grabbing an extra reference, then there could also be some confusion as userspace considers a container to be "dead" while the audit container ID still exists in the kernel, and the kernel generated audit container ID death record will not be generated until much later (and possibly be associated with a different event, but that could be solved by unassociating the container death record).

Of the two
approaches, I think the latter is safer in that it preserves the correctness of the audit log, even though it could result in a delay of the container death record.

Neither way is perfect, so if you have any other ideas I'm all ears.

Quoted 6 lines

Quoted 6 lines

(We do need to do some extra housekeeping in audit_signal_info() to deal with the case where nobody asks for AUDIT_SIGNAL_INFO2 - basically if audit_sig_cid is not NULL we should drop a reference before assigning it a new object pointer, and of course we would need to set audit_sig_cid to NULL in audit_receive_msg() after sending it up to userspace and dropping the extra ref.)

Signature · 2 lines

paul moore
www.paul-moore.com

Quoted 97 lines · Paul Moore

Quoted 86 lines · Richard Guy Briggs

Quoted 62 lines · Paul Moore

Quoted 49 lines · Richard Guy Briggs

Quoted 40 lines · Paul Moore

Quoted 19 lines · Richard Guy Briggs

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              | 35 +++++++++++++++++++++++++++++++++++
 kernel/audit.h              |  1 +
 security/selinux/nlmsgtab.c |  1 +
 5 files changed, 45 insertions(+)

...

Quoted 6 lines

kernel/audit.c +0 -0

index 0871c3e5d6df..51159c94041c 100644
@@ -126,6 +126,14 @@ struct auditd_connection {
 kuid_t         audit_sig_uid = INVALID_UID;

kernel/audit.c struct auditd_connection { · line 136

Quoted 10 lines

kernel/audit.c +8 -0

 pid_t          audit_sig_pid = -1;
 u32            audit_sig_sid = 0;
+/* Since the signal information is stored in the record buffer at the
+ * time of the signal, but not retrieved until later, there is a chance
+ * that the last process in the container could terminate before the
+ * signal record is delivered.  In this circumstance, there is a chance
+ * the orchestrator could reuse the audit container identifier, causing
+ * an overlap of audit records that refer to the same audit container
+ * identifier, but a different container instance.  */
+u64            audit_sig_cid = AUDIT_CID_UNSET;

Quoted 7 lines

I believe we could prevent the case mentioned above by taking an additional reference to the audit container ID object when the signal information is collected, dropping it only after the signal information is collected by userspace or another process signals the audit daemon. Yes, it would block that audit container ID from being reused immediately, but since we are talking about one number out of 2^64 that seems like a reasonable tradeoff.

Quoted 6 lines

I had thought that through and should have been more explicit about that situation when I documented it. We could do that, but then the syscall records would be connected with the call from auditd on shutdown to request that signal information, rather than the exit of that last process that was using that container. This strikes me as misleading. Is that really what we want?

???

I think one of us is not understanding the other; maybe it's me, maybe it's you, maybe it's both of us.

Quoted 10 lines

Anyway, here is what I was trying to convey with my original comment ... When we record the audit container ID in audit_signal_info() we take an extra reference to the audit container ID object so that it will not disappear (and get reused) until after we respond with an AUDIT_SIGNAL_INFO2. In audit_receive_msg() when we do the AUDIT_SIGNAL_INFO2 processing we drop the extra reference we took in audit_signal_info(). Unless I'm missing some other change you made, this *shouldn't* affect the syscall records, all it does is preserve the audit container ID object in the kernel's ACID store so it doesn't get reused.

This is exactly what I had understood. I hadn't considered the extra details below in detail due to my original syscall concern, but they make sense.

The syscall I refer to is the one connected with the drop of the audit container identifier by the last process that was in that container in patch 5/16. The production of this record is contingent on the last ref in a contobj being dropped. So if it is due to that ref being maintained by audit_signal_info() until the AUDIT_SIGNAL_INFO2 record it fetched, then it will appear that the fetch action closed the container rather than the last process in the container to exit.

Quoted 1 line

Does this make sense?

More so than your original reply, at least to me anyway.

It makes sense that the audit container ID wouldn't be marked as "dead" since it would still be very much alive and available for use by the orchestrator, the question is if that is desirable or not. I think the answer to this comes down the preserving the correctness of the audit log.

Quoted 9 lines

If the audit container ID reported by AUDIT_SIGNAL_INFO2 has been reused then I think there is a legitimate concern that the audit log is not correct, and could be misleading. If we solve that by grabbing an extra reference, then there could also be some confusion as userspace considers a container to be "dead" while the audit container ID still exists in the kernel, and the kernel generated audit container ID death record will not be generated until much later (and possibly be associated with a different event, but that could be solved by unassociating the container death record).

How does syscall association of the death record with AUDIT_SIGNAL_INFO2 possibly get associated with another event? Or is the syscall association with the fetch for the AUDIT_SIGNAL_INFO2 the other event?

Another idea might be to bump the refcount in audit_signal_info() but mark tht contid as dead so it can't be reused if we are concerned that the dead contid be reused?

There is still the problem later that the reported contid is incomplete compared to the rest of the contid reporting cycle wrt nesting since AUDIT_SIGNAL_INFO2 will need to be more complex w/2 variable length fields to accommodate a nested contid list.

Quoted 4 lines

Of the two
approaches, I think the latter is safer in that it preserves the correctness of the audit log, even though it could result in a delay of the container death record.

I prefer the former since it strongly indicates last task in the container. The AUDIT_SIGNAL_INFO2 msg has the pid and other subject attributes and the contid to strongly link the responsible party.

Quoted 48 lines

Quoted 38 lines

Quoted 28 lines

Quoted 5 lines

... When we record the audit container ID in audit_signal_info() we take an extra reference to the audit container ID object so that it will not disappear (and get reused) until after we respond with an AUDIT_SIGNAL_INFO2. In audit_receive_msg() when we do the AUDIT_SIGNAL_INFO2 processing we drop the extra reference we took

Quoted 8 lines

in
audit_signal_info(). Unless I'm missing some other change you made,
this *shouldn't* affect the syscall records, all it does is preserve
the audit container ID object in the kernel's ACID store so it doesn't
get reused.

This is exactly what I had understood. I hadn't considered the extra details below in detail due to my original syscall concern, but they make sense.

The syscall I refer to is the one connected with the drop of the audit container identifier by the last process that was in that container in patch 5/16. The production of this record is contingent on
the last ref in a contobj being dropped. So if it is due to that ref being maintained by audit_signal_info() until the AUDIT_SIGNAL_INFO2 record it fetched, then it will appear that the fetch action closed the
container rather than the last process in the container to exit.

Quoted 1 line

Does this make sense?

More so than your original reply, at least to me anyway.

It makes sense that the audit container ID wouldn't be marked as "dead" since it would still be very much alive and available for use by the orchestrator, the question is if that is desirable or not. I think the answer to this comes down the preserving the correctness of the audit log.

Quoted 9 lines

If the audit container ID reported by AUDIT_SIGNAL_INFO2 has been reused then I think there is a legitimate concern that the audit log is not correct, and could be misleading. If we solve that by grabbing an extra reference, then there could also be some confusion as userspace considers a container to be "dead" while the audit container ID still exists in the kernel, and the kernel generated audit container ID death record will not be generated until much later (and possibly be associated with a different event, but that could be solved by unassociating the container death record).

Quoted 3 lines

How does syscall association of the death record with AUDIT_SIGNAL_INFO2 possibly get associated with another event? Or is the syscall association with the fetch for the AUDIT_SIGNAL_INFO2 the other event?

The issue is when does the audit container ID "die". If it is when the last task in the container exits, then the death record will be associated when the task's exit. If the audit container ID lives on until the last reference of it in the audit logs, including the SIGNAL_INFO2 message, the death record will be associated with the related SIGNAL_INFO2 syscalls, or perhaps unassociated depending on the details of the syscalls/netlink.

Quoted 3 lines

Another idea might be to bump the refcount in audit_signal_info() but mark tht contid as dead so it can't be reused if we are concerned that the dead contid be reused?

Ooof. Yes, maybe, but that would be ugly.

Quoted 4 lines

There is still the problem later that the reported contid is incomplete compared to the rest of the contid reporting cycle wrt nesting since AUDIT_SIGNAL_INFO2 will need to be more complex w/2 variable length fields to accommodate a nested contid list.

Do we really care about the full nested audit container ID list in the SIGNAL_INFO2 record?

Quoted 5 lines

Quoted 4 lines

Of the two
approaches, I think the latter is safer in that it preserves the correctness of the audit log, even though it could result in a delay of the container death record.

Quoted 3 lines

I prefer the former since it strongly indicates last task in the container. The AUDIT_SIGNAL_INFO2 msg has the pid and other subject attributes and the contid to strongly link the responsible party.

Steve is the only one who really tracks the security certifications that are relevant to audit, see what the certification requirements have to say and we can revisit this.

Quoted 79 lines · EST Paul Moore

Quoted 48 lines

Quoted 38 lines

Quoted 28 lines

Quoted 5 lines

... When we record the audit container ID in audit_signal_info() we take an extra reference to the audit container ID object so that it will not disappear (and get reused) until after we respond with an AUDIT_SIGNAL_INFO2. In audit_receive_msg() when we do the AUDIT_SIGNAL_INFO2 processing we drop the extra reference we took

Quoted 8 lines

in
audit_signal_info(). Unless I'm missing some other change you made,
this *shouldn't* affect the syscall records, all it does is preserve
the audit container ID object in the kernel's ACID store so it doesn't
get reused.

This is exactly what I had understood. I hadn't considered the extra details below in detail due to my original syscall concern, but they make sense.

The syscall I refer to is the one connected with the drop of the audit container identifier by the last process that was in that container in patch 5/16. The production of this record is contingent on
the last ref in a contobj being dropped. So if it is due to that ref being maintained by audit_signal_info() until the AUDIT_SIGNAL_INFO2 record it fetched, then it will appear that the fetch action closed the
container rather than the last process in the container to exit.

Quoted 1 line

Does this make sense?

More so than your original reply, at least to me anyway.

It makes sense that the audit container ID wouldn't be marked as "dead" since it would still be very much alive and available for use by the orchestrator, the question is if that is desirable or not. I think the answer to this comes down the preserving the correctness of the audit log.

Quoted 9 lines

If the audit container ID reported by AUDIT_SIGNAL_INFO2 has been reused then I think there is a legitimate concern that the audit log is not correct, and could be misleading. If we solve that by grabbing an extra reference, then there could also be some confusion as userspace considers a container to be "dead" while the audit container ID still exists in the kernel, and the kernel generated audit container ID death record will not be generated until much later (and possibly be associated with a different event, but that could be solved by unassociating the container death record).

Quoted 3 lines

How does syscall association of the death record with AUDIT_SIGNAL_INFO2 possibly get associated with another event? Or is the syscall association with the fetch for the AUDIT_SIGNAL_INFO2 the other event?

The issue is when does the audit container ID "die". If it is when the last task in the container exits, then the death record will be associated when the task's exit. If the audit container ID lives on until the last reference of it in the audit logs, including the SIGNAL_INFO2 message, the death record will be associated with the related SIGNAL_INFO2 syscalls, or perhaps unassociated depending on the details of the syscalls/netlink.

Quoted 3 lines

Another idea might be to bump the refcount in audit_signal_info() but mark tht contid as dead so it can't be reused if we are concerned that the dead contid be reused?

Ooof. Yes, maybe, but that would be ugly.

Quoted 4 lines

There is still the problem later that the reported contid is incomplete compared to the rest of the contid reporting cycle wrt nesting since AUDIT_SIGNAL_INFO2 will need to be more complex w/2 variable length fields to accommodate a nested contid list.

Do we really care about the full nested audit container ID list in the SIGNAL_INFO2 record?

Quoted 5 lines

Quoted 4 lines

Of the two
approaches, I think the latter is safer in that it preserves the correctness of the audit log, even though it could result in a delay of the container death record.

Quoted 7 lines

Quoted 3 lines

I prefer the former since it strongly indicates last task in the container. The AUDIT_SIGNAL_INFO2 msg has the pid and other subject attributes and the contid to strongly link the responsible party.

Steve is the only one who really tracks the security certifications that are relevant to audit, see what the certification requirements have to say and we can revisit this.

Sever Virtualization Protection Profile is the closest applicable standard

https://www.niap-ccevs.org/Profile/Info.cfm?PPID=408&id=408

It is silent on audit requirements for the lifecycle of a VM. I assume that all that is needed is what the orchestrator says its doing at the high level. So, if an orchestrator wants to shutdown a container, the orchestrator must log that intent and its results. In a similar fashion, systemd logs that it's killing a service and we don't actually hook the exit syscall of the service to record that.

Now, if a container was being used as a VPS, and it had a fully functioning userspace, it's own services, and its very own audit daemon, then in this case it would care who sent a signal to its auditd. The tenant of that container may have to comply with PCI-DSS or something else. It would log the audit service is being terminated and systemd would record that its tearing down the environment. The OS doesn't need to do anything.

This is a bit of a thread-hijack, and for that I apologize, but another thought crossed my mind while thinking about this issue further ... Once we support multiple auditd instances, including the necessary record routing and duplication/multiple-sends (the host always sees *everything*), we will likely need to find a way to "trim" the audit container ID (ACID) lists we send in the records. The auditd instance running on the host/initns will always see everything, so it will want the full container ACID list; however an auditd instance running inside a container really should only see the ACIDs of any child containers.

For example, imagine a system where the host has containers 1 and 2, each running an auditd instance. Inside container 1 there are containers A and B. Inside container 2 there are containers Y and Z. If an audit event is generated in container Z, I would expect the host's auditd to see a ACID list of "1,Z" but container 1's auditd should only see an ACID list of "Z". The auditd running in container 2 should not see the record at all (that will be relatively straightforward). Does that make sense? Do we have the record formats properly designed to handle this without too much problem (I'm not entirely sure we do)?

paul moore

Quoted 10 lines · Paul Moore

This is a bit of a thread-hijack, and for that I apologize, but another thought crossed my mind while thinking about this issue further ... Once we support multiple auditd instances, including the necessary record routing and duplication/multiple-sends (the host always sees *everything*), we will likely need to find a way to "trim" the audit container ID (ACID) lists we send in the records. The auditd instance running on the host/initns will always see everything, so it will want the full container ACID list; however an auditd instance running inside a container really should only see the ACIDs of any child containers.

Agreed. This should be easy to check and limit, preventing an auditd from seeing any contid that is a parent of its own contid.

Quoted 10 lines

For example, imagine a system where the host has containers 1 and 2, each running an auditd instance. Inside container 1 there are containers A and B. Inside container 2 there are containers Y and Z. If an audit event is generated in container Z, I would expect the host's auditd to see a ACID list of "1,Z" but container 1's auditd should only see an ACID list of "Z". The auditd running in container 2 should not see the record at all (that will be relatively straightforward). Does that make sense? Do we have the record formats properly designed to handle this without too much problem (I'm not entirely sure we do)?

I completely agree and I believe we have record formats that are able to handle this already.

Quoted 26 lines · Richard Guy Briggs

Quoted 10 lines · Paul Moore

This is a bit of a thread-hijack, and for that I apologize, but another thought crossed my mind while thinking about this issue further ... Once we support multiple auditd instances, including the necessary record routing and duplication/multiple-sends (the host always sees *everything*), we will likely need to find a way to "trim" the audit container ID (ACID) lists we send in the records. The auditd instance running on the host/initns will always see everything, so it will want the full container ACID list; however an auditd instance running inside a container really should only see the ACIDs of any child containers.

Agreed. This should be easy to check and limit, preventing an auditd from seeing any contid that is a parent of its own contid.

Quoted 10 lines

For example, imagine a system where the host has containers 1 and 2, each running an auditd instance. Inside container 1 there are containers A and B. Inside container 2 there are containers Y and Z. If an audit event is generated in container Z, I would expect the host's auditd to see a ACID list of "1,Z" but container 1's auditd should only see an ACID list of "Z". The auditd running in container 2 should not see the record at all (that will be relatively straightforward). Does that make sense? Do we have the record formats properly designed to handle this without too much problem (I'm not entirely sure we do)?

Quoted 2 lines

I completely agree and I believe we have record formats that are able to handle this already.

I'm not convinced we do. What about the cases where we have a field with a list of audit container IDs? How do we handle that?

paul moore

Quoted 27 lines · Paul Moore

Quoted 26 lines · Richard Guy Briggs

Quoted 10 lines · Paul Moore

This is a bit of a thread-hijack, and for that I apologize, but another thought crossed my mind while thinking about this issue further ... Once we support multiple auditd instances, including the necessary record routing and duplication/multiple-sends (the host always sees *everything*), we will likely need to find a way to "trim" the audit container ID (ACID) lists we send in the records. The auditd instance running on the host/initns will always see everything, so it will want the full container ACID list; however an auditd instance running inside a container really should only see the ACIDs of any child containers.

Agreed. This should be easy to check and limit, preventing an auditd from seeing any contid that is a parent of its own contid.

Quoted 10 lines

For example, imagine a system where the host has containers 1 and 2, each running an auditd instance. Inside container 1 there are containers A and B. Inside container 2 there are containers Y and Z. If an audit event is generated in container Z, I would expect the host's auditd to see a ACID list of "1,Z" but container 1's auditd should only see an ACID list of "Z". The auditd running in container 2 should not see the record at all (that will be relatively straightforward). Does that make sense? Do we have the record formats properly designed to handle this without too much problem (I'm not entirely sure we do)?

Quoted 5 lines

Quoted 2 lines

I completely agree and I believe we have record formats that are able to handle this already.

I'm not convinced we do. What about the cases where we have a field with a list of audit container IDs? How do we handle that?

I don't understand the problem. (I think you crossed your 1/2 vs A/B/Y/Z in your example.) ...

... Clarifying the example above, if as you
suggest an event happens in container Z, the hosts's auditd would report

        Z,^2
and the auditd in container 2 would report
        Z,^2

but if there were another auditd running in container Z it would report
Z
while the auditd in container 1 or A/B would see nothing.

Quoted 28 lines · Richard Guy Briggs

Quoted 27 lines · Paul Moore

Quoted 26 lines · Richard Guy Briggs

Quoted 10 lines · Paul Moore

This is a bit of a thread-hijack, and for that I apologize, but another thought crossed my mind while thinking about this issue further ... Once we support multiple auditd instances, including the necessary record routing and duplication/multiple-sends (the host always sees *everything*), we will likely need to find a way to "trim" the audit container ID (ACID) lists we send in the records. The auditd instance running on the host/initns will always see everything, so it will want the full container ACID list; however an auditd instance running inside a container really should only see the ACIDs of any child containers.

Agreed. This should be easy to check and limit, preventing an auditd from seeing any contid that is a parent of its own contid.

Quoted 10 lines

For example, imagine a system where the host has containers 1 and 2, each running an auditd instance. Inside container 1 there are containers A and B. Inside container 2 there are containers Y and Z. If an audit event is generated in container Z, I would expect the host's auditd to see a ACID list of "1,Z" but container 1's auditd should only see an ACID list of "Z". The auditd running in container 2 should not see the record at all (that will be relatively straightforward). Does that make sense? Do we have the record formats properly designed to handle this without too much problem (I'm not entirely sure we do)?

Quoted 8 lines

Quoted 5 lines

Quoted 2 lines

I completely agree and I believe we have record formats that are able to handle this already.

I'm not convinced we do. What about the cases where we have a field with a list of audit container IDs? How do we handle that?

I don't understand the problem. (I think you crossed your 1/2 vs A/B/Y/Z in your example.) ...

It looks like I did, sorry about that.

Quoted 8 lines

... Clarifying the example above, if as you
suggest an event happens in container Z, the hosts's auditd would report

        Z,^2
and the auditd in container 2 would report
        Z,^2

but if there were another auditd running in container Z it would report
Z
while the auditd in container 1 or A/B would see nothing.

Yes. My concern is how do we handle this to minimize duplicating and rewriting the records? It isn't so much about the format, although the format is a side effect.

paul moore

Quoted 49 lines · Paul Moore

Quoted 28 lines · Richard Guy Briggs

Quoted 27 lines · Paul Moore

Quoted 26 lines · Richard Guy Briggs

Quoted 10 lines · Paul Moore

This is a bit of a thread-hijack, and for that I apologize, but another thought crossed my mind while thinking about this issue further ... Once we support multiple auditd instances, including the necessary record routing and duplication/multiple-sends (the host always sees *everything*), we will likely need to find a way to "trim" the audit container ID (ACID) lists we send in the records. The auditd instance running on the host/initns will always see everything, so it will want the full container ACID list; however an auditd instance running inside a container really should only see the ACIDs of any child containers.

Agreed. This should be easy to check and limit, preventing an auditd from seeing any contid that is a parent of its own contid.

Quoted 10 lines

For example, imagine a system where the host has containers 1 and 2, each running an auditd instance. Inside container 1 there are containers A and B. Inside container 2 there are containers Y and Z. If an audit event is generated in container Z, I would expect the host's auditd to see a ACID list of "1,Z" but container 1's auditd should only see an ACID list of "Z". The auditd running in container 2 should not see the record at all (that will be relatively straightforward). Does that make sense? Do we have the record formats properly designed to handle this without too much problem (I'm not entirely sure we do)?

Quoted 8 lines

Quoted 5 lines

Quoted 2 lines

I completely agree and I believe we have record formats that are able to handle this already.

I'm not convinced we do. What about the cases where we have a field with a list of audit container IDs? How do we handle that?

I don't understand the problem. (I think you crossed your 1/2 vs A/B/Y/Z in your example.) ...

It looks like I did, sorry about that.

Quoted 8 lines

... Clarifying the example above, if as you
suggest an event happens in container Z, the hosts's auditd would report

        Z,^2
and the auditd in container 2 would report
        Z,^2

but if there were another auditd running in container Z it would report
Z
while the auditd in container 1 or A/B would see nothing.

Quoted 3 lines

Yes. My concern is how do we handle this to minimize duplicating and rewriting the records? It isn't so much about the format, although the format is a side effect.

Are you talking about caching, or about divulging more information than necessary or even information leaks? Or even noticing that records that need to be generated to two audit daemons share the same contid field values and should be generated at the same time or information shared between them? I'd see any of these as optimizations that don't affect the api.

Quoted 54 lines · Richard Guy Briggs

Quoted 49 lines · Paul Moore

Quoted 28 lines · Richard Guy Briggs

Quoted 27 lines · Paul Moore

Quoted 26 lines · Richard Guy Briggs

Quoted 10 lines · Paul Moore

This is a bit of a thread-hijack, and for that I apologize, but another thought crossed my mind while thinking about this issue further ... Once we support multiple auditd instances, including the necessary record routing and duplication/multiple-sends (the host always sees *everything*), we will likely need to find a way to "trim" the audit container ID (ACID) lists we send in the records. The auditd instance running on the host/initns will always see everything, so it will want the full container ACID list; however an auditd instance running inside a container really should only see the ACIDs of any child containers.

Agreed. This should be easy to check and limit, preventing an auditd from seeing any contid that is a parent of its own contid.

Quoted 10 lines

For example, imagine a system where the host has containers 1 and 2, each running an auditd instance. Inside container 1 there are containers A and B. Inside container 2 there are containers Y and Z. If an audit event is generated in container Z, I would expect the host's auditd to see a ACID list of "1,Z" but container 1's auditd should only see an ACID list of "Z". The auditd running in container 2 should not see the record at all (that will be relatively straightforward). Does that make sense? Do we have the record formats properly designed to handle this without too much problem (I'm not entirely sure we do)?

Quoted 8 lines

Quoted 5 lines

Quoted 2 lines

I completely agree and I believe we have record formats that are able to handle this already.

I'm not convinced we do. What about the cases where we have a field with a list of audit container IDs? How do we handle that?

I don't understand the problem. (I think you crossed your 1/2 vs A/B/Y/Z in your example.) ...

It looks like I did, sorry about that.

Quoted 8 lines

... Clarifying the example above, if as you
suggest an event happens in container Z, the hosts's auditd would report

        Z,^2
and the auditd in container 2 would report
        Z,^2

but if there were another auditd running in container Z it would report
Z
while the auditd in container 1 or A/B would see nothing.

Quoted 3 lines

Yes. My concern is how do we handle this to minimize duplicating and rewriting the records? It isn't so much about the format, although the format is a side effect.

Quoted 6 lines

Are you talking about caching, or about divulging more information than necessary or even information leaks? Or even noticing that records that need to be generated to two audit daemons share the same contid field values and should be generated at the same time or information shared between them? I'd see any of these as optimizations that don't affect the api.

Imagine a record is generated in a container which has more than one auditd in it's ancestry that should receive this record, how do we handle that without completely killing performance? That's my concern. If you've already thought up a plan for this - excellent, please share :)

paul moore

Quoted 62 lines · Paul Moore

Quoted 54 lines · Richard Guy Briggs

Quoted 49 lines · Paul Moore

Quoted 28 lines · Richard Guy Briggs

Quoted 27 lines · Paul Moore

Quoted 26 lines · Richard Guy Briggs

Quoted 10 lines · Paul Moore

This is a bit of a thread-hijack, and for that I apologize, but another thought crossed my mind while thinking about this issue further ... Once we support multiple auditd instances, including the necessary record routing and duplication/multiple-sends (the host always sees *everything*), we will likely need to find a way to "trim" the audit container ID (ACID) lists we send in the records. The auditd instance running on the host/initns will always see everything, so it will want the full container ACID list; however an auditd instance running inside a container really should only see the ACIDs of any child containers.

Agreed. This should be easy to check and limit, preventing an auditd from seeing any contid that is a parent of its own contid.

Quoted 10 lines

For example, imagine a system where the host has containers 1 and 2, each running an auditd instance. Inside container 1 there are containers A and B. Inside container 2 there are containers Y and Z. If an audit event is generated in container Z, I would expect the host's auditd to see a ACID list of "1,Z" but container 1's auditd should only see an ACID list of "Z". The auditd running in container 2 should not see the record at all (that will be relatively straightforward). Does that make sense? Do we have the record formats properly designed to handle this without too much problem (I'm not entirely sure we do)?

Quoted 8 lines

Quoted 5 lines

Quoted 2 lines

I completely agree and I believe we have record formats that are able to handle this already.

I'm not convinced we do. What about the cases where we have a field with a list of audit container IDs? How do we handle that?

I don't understand the problem. (I think you crossed your 1/2 vs A/B/Y/Z in your example.) ...

It looks like I did, sorry about that.

Quoted 8 lines

... Clarifying the example above, if as you
suggest an event happens in container Z, the hosts's auditd would report

        Z,^2
and the auditd in container 2 would report
        Z,^2

but if there were another auditd running in container Z it would report
Z
while the auditd in container 1 or A/B would see nothing.

Quoted 3 lines

Yes. My concern is how do we handle this to minimize duplicating and rewriting the records? It isn't so much about the format, although the format is a side effect.

Quoted 6 lines

Are you talking about caching, or about divulging more information than necessary or even information leaks? Or even noticing that records that need to be generated to two audit daemons share the same contid field values and should be generated at the same time or information shared between them? I'd see any of these as optimizations that don't affect the api.

Quoted 5 lines

Imagine a record is generated in a container which has more than one auditd in it's ancestry that should receive this record, how do we handle that without completely killing performance? That's my concern. If you've already thought up a plan for this - excellent, please share :)

No, I haven't given that much thought other than the correctness and security issues of making sure that each audit daemon is sufficiently isolated to do its job but not jeopardize another audit domain. Audit already kills performance, according to some...

We currently won't have that problem since there can only be one so far. Fixing and optimizing this is part of the next phase of the challenge of adding a second audit daemon.

Let's work on correctness and reasonable efficiency for this phase and not focus on a problem we don't yet have. I wouldn't consider this incurring technical debt at this point.

I could see cacheing a contid string from one starting point, but it may be more work to search that cached string to truncate it or add to it when another audit daemon requests a copy of a similar string. I suppose every full contid string could be generated the first time it is used and parts of it used (start/finish) as needed but that search/indexing may not be worth it.

Quoted 74 lines · Richard Guy Briggs

Quoted 62 lines · Paul Moore

Quoted 54 lines · Richard Guy Briggs

Quoted 49 lines · Paul Moore

Quoted 28 lines · Richard Guy Briggs

Quoted 27 lines · Paul Moore

Quoted 26 lines · Richard Guy Briggs

Quoted 10 lines · Paul Moore

This is a bit of a thread-hijack, and for that I apologize, but another thought crossed my mind while thinking about this issue further ... Once we support multiple auditd instances, including the necessary record routing and duplication/multiple-sends (the host always sees *everything*), we will likely need to find a way to "trim" the audit container ID (ACID) lists we send in the records. The auditd instance running on the host/initns will always see everything, so it will want the full container ACID list; however an auditd instance running inside a container really should only see the ACIDs of any child containers.

Agreed. This should be easy to check and limit, preventing an auditd from seeing any contid that is a parent of its own contid.

Quoted 10 lines

For example, imagine a system where the host has containers 1 and 2, each running an auditd instance. Inside container 1 there are containers A and B. Inside container 2 there are containers Y and Z. If an audit event is generated in container Z, I would expect the host's auditd to see a ACID list of "1,Z" but container 1's auditd should only see an ACID list of "Z". The auditd running in container 2 should not see the record at all (that will be relatively straightforward). Does that make sense? Do we have the record formats properly designed to handle this without too much problem (I'm not entirely sure we do)?

Quoted 8 lines

Quoted 5 lines

Quoted 2 lines

I completely agree and I believe we have record formats that are able to handle this already.

I'm not convinced we do. What about the cases where we have a field with a list of audit container IDs? How do we handle that?

I don't understand the problem. (I think you crossed your 1/2 vs A/B/Y/Z in your example.) ...

It looks like I did, sorry about that.

Quoted 8 lines

... Clarifying the example above, if as you
suggest an event happens in container Z, the hosts's auditd would report

        Z,^2
and the auditd in container 2 would report
        Z,^2

but if there were another auditd running in container Z it would report
Z
while the auditd in container 1 or A/B would see nothing.

Quoted 3 lines

Yes. My concern is how do we handle this to minimize duplicating and rewriting the records? It isn't so much about the format, although the format is a side effect.

Quoted 6 lines

Are you talking about caching, or about divulging more information than necessary or even information leaks? Or even noticing that records that need to be generated to two audit daemons share the same contid field values and should be generated at the same time or information shared between them? I'd see any of these as optimizations that don't affect the api.

Quoted 5 lines

Imagine a record is generated in a container which has more than one auditd in it's ancestry that should receive this record, how do we handle that without completely killing performance? That's my concern. If you've already thought up a plan for this - excellent, please share :)

No, I haven't given that much thought other than the correctness and security issues of making sure that each audit daemon is sufficiently isolated to do its job but not jeopardize another audit domain. Audit already kills performance, according to some...

Quoted 7 lines

We currently won't have that problem since there can only be one so far. Fixing and optimizing this is part of the next phase of the challenge of adding a second audit daemon.

Let's work on correctness and reasonable efficiency for this phase and not focus on a problem we don't yet have. I wouldn't consider this incurring technical debt at this point.

I agree, one stage at a time, but the choice we make here is going to have a significant impact on what we can do later. We need to get this as "right" as possible; this isn't something we should dismiss with a hand-wave as a problem for the next stage. We don't need an implementation, but I would like to see a rough design of how we would address this problem.

Quoted 6 lines

I could see cacheing a contid string from one starting point, but it may be more work to search that cached string to truncate it or add to it when another audit daemon requests a copy of a similar string. I suppose every full contid string could be generated the first time it is used and parts of it used (start/finish) as needed but that search/indexing may not be worth it.

I hope we can do better than string manipulations in the kernel. I'd much rather defer generating the ACID list (if possible), than generating a list only to keep copying and editing it as the record is sent.

paul moore

On 2020-03-18 18:06, Paul Moore wrote:

Quoted 4 lines

I hope we can do better than string manipulations in the kernel. I'd much rather defer generating the ACID list (if possible), than generating a list only to keep copying and editing it as the record is sent.

At the moment we are stuck with a string-only format.

Quoted 1 line · Richard Guy Briggs

On 2020-03-18 18:06, Paul Moore wrote:

...

Quoted 6 lines

Quoted 4 lines

I hope we can do better than string manipulations in the kernel. I'd much rather defer generating the ACID list (if possible), than generating a list only to keep copying and editing it as the record is sent.

At the moment we are stuck with a string-only format.

Yes, we are. That is another topic, and another set of changes I've been deferring so as to not disrupt the audit container ID work.

I was thinking of what we do inside the kernel between when the record triggering event happens and when we actually emit the record to userspace. Perhaps we collect the ACID information while the event is occurring, but we defer generating the record until later when we have a better understanding of what should be included in the ACID list. It is somewhat similar (but obviously different) to what we do for PATH records (we collect the pathname info when the path is being resolved).

paul moore

Quoted 15 lines · Paul Moore

Quoted 1 line · Richard Guy Briggs

On 2020-03-18 18:06, Paul Moore wrote:

...

Quoted 6 lines

Quoted 4 lines

I hope we can do better than string manipulations in the kernel. I'd much rather defer generating the ACID list (if possible), than generating a list only to keep copying and editing it as the record is sent.

At the moment we are stuck with a string-only format.

Yes, we are. That is another topic, and another set of changes I've been deferring so as to not disrupt the audit container ID work.

Quoted 8 lines

I was thinking of what we do inside the kernel between when the record triggering event happens and when we actually emit the record to userspace. Perhaps we collect the ACID information while the event is occurring, but we defer generating the record until later when we have a better understanding of what should be included in the ACID list. It is somewhat similar (but obviously different) to what we do for PATH records (we collect the pathname info when the path is being resolved).

Ok, now I understand your concern.

In the case of NETFILTER_PKT records, the CONTAINER_ID record is the only other possible record and they are generated at the same time with a local context.

In the case of any event involving a syscall, that CONTAINER_ID record is generated at the time of the rest of the event record generation at syscall exit.

The others are only generated when needed, such as the sig2 reply.

We generally just store the contobj pointer until we actually generate the CONTAINER_ID (or CONTAINER_OP) record.

Quoted 37 lines · Richard Guy Briggs

Quoted 15 lines · Paul Moore

Quoted 1 line · Richard Guy Briggs

On 2020-03-18 18:06, Paul Moore wrote:

...

Quoted 6 lines

Quoted 4 lines

I hope we can do better than string manipulations in the kernel. I'd much rather defer generating the ACID list (if possible), than generating a list only to keep copying and editing it as the record is sent.

At the moment we are stuck with a string-only format.

Yes, we are. That is another topic, and another set of changes I've been deferring so as to not disrupt the audit container ID work.

Quoted 8 lines

I was thinking of what we do inside the kernel between when the record triggering event happens and when we actually emit the record to userspace. Perhaps we collect the ACID information while the event is occurring, but we defer generating the record until later when we have a better understanding of what should be included in the ACID list. It is somewhat similar (but obviously different) to what we do for PATH records (we collect the pathname info when the path is being resolved).

Ok, now I understand your concern.

In the case of NETFILTER_PKT records, the CONTAINER_ID record is the only other possible record and they are generated at the same time with a local context.

In the case of any event involving a syscall, that CONTAINER_ID record is generated at the time of the rest of the event record generation at syscall exit.

The others are only generated when needed, such as the sig2 reply.

Quoted 2 lines

We generally just store the contobj pointer until we actually generate the CONTAINER_ID (or CONTAINER_OP) record.

Perhaps I'm remembering your latest spin of these patches incorrectly, but there is still a big gap between when the record is generated and when it is sent up to the audit daemon. Most importantly in that gap is the whole big queue/multicast/unicast mess.

You don't need to show me code, but I would like to see some sort of plan for dealing with multiple nested audit daemons. Basically I just want to make sure we aren't painting ourselves into a corner with this approach; and if for some horrible reason we are, I at least want us to be aware of what we are getting ourselves into.

paul moore

Quoted 38 lines · Paul Moore

Quoted 37 lines · Richard Guy Briggs

Quoted 15 lines · Paul Moore

Quoted 1 line · Richard Guy Briggs

On 2020-03-18 18:06, Paul Moore wrote:

...

Quoted 6 lines

Quoted 4 lines

I hope we can do better than string manipulations in the kernel. I'd much rather defer generating the ACID list (if possible), than generating a list only to keep copying and editing it as the record is sent.

At the moment we are stuck with a string-only format.

Yes, we are. That is another topic, and another set of changes I've been deferring so as to not disrupt the audit container ID work.

Quoted 8 lines

I was thinking of what we do inside the kernel between when the record triggering event happens and when we actually emit the record to userspace. Perhaps we collect the ACID information while the event is occurring, but we defer generating the record until later when we have a better understanding of what should be included in the ACID list. It is somewhat similar (but obviously different) to what we do for PATH records (we collect the pathname info when the path is being resolved).

Ok, now I understand your concern.

In the case of NETFILTER_PKT records, the CONTAINER_ID record is the only other possible record and they are generated at the same time with a local context.

In the case of any event involving a syscall, that CONTAINER_ID record is generated at the time of the rest of the event record generation at syscall exit.

The others are only generated when needed, such as the sig2 reply.

Quoted 7 lines

Quoted 2 lines

We generally just store the contobj pointer until we actually generate the CONTAINER_ID (or CONTAINER_OP) record.

Perhaps I'm remembering your latest spin of these patches incorrectly, but there is still a big gap between when the record is generated and when it is sent up to the audit daemon. Most importantly in that gap is the whole big queue/multicast/unicast mess.

So you suggest generating that record on the fly once it reaches the end of the audit_queue just before being sent? That sounds... disruptive. Each audit daemon is going to have its own queues, so by the time it ends up in a particular queue, we'll already know its scope and would have the right list of contids to print in that record.

Quoted 47 lines · Richard Guy Briggs

Quoted 38 lines · Paul Moore

Quoted 37 lines · Richard Guy Briggs

Quoted 15 lines · Paul Moore

Quoted 1 line · Richard Guy Briggs

On 2020-03-18 18:06, Paul Moore wrote:

...

Quoted 6 lines

Quoted 4 lines

I hope we can do better than string manipulations in the kernel. I'd much rather defer generating the ACID list (if possible), than generating a list only to keep copying and editing it as the record is sent.

At the moment we are stuck with a string-only format.

Yes, we are. That is another topic, and another set of changes I've been deferring so as to not disrupt the audit container ID work.

Quoted 8 lines

I was thinking of what we do inside the kernel between when the record triggering event happens and when we actually emit the record to userspace. Perhaps we collect the ACID information while the event is occurring, but we defer generating the record until later when we have a better understanding of what should be included in the ACID list. It is somewhat similar (but obviously different) to what we do for PATH records (we collect the pathname info when the path is being resolved).

Ok, now I understand your concern.

In the case of NETFILTER_PKT records, the CONTAINER_ID record is the only other possible record and they are generated at the same time with a local context.

In the case of any event involving a syscall, that CONTAINER_ID record is generated at the time of the rest of the event record generation at syscall exit.

The others are only generated when needed, such as the sig2 reply.

Quoted 7 lines

Quoted 2 lines

We generally just store the contobj pointer until we actually generate the CONTAINER_ID (or CONTAINER_OP) record.

Perhaps I'm remembering your latest spin of these patches incorrectly, but there is still a big gap between when the record is generated and when it is sent up to the audit daemon. Most importantly in that gap is the whole big queue/multicast/unicast mess.

Quoted 5 lines

So you suggest generating that record on the fly once it reaches the end of the audit_queue just before being sent? That sounds... disruptive. Each audit daemon is going to have its own queues, so by the time it ends up in a particular queue, we'll already know its scope and would have the right list of contids to print in that record.

I'm not suggesting any particular solution, I'm just pointing out a potential problem. It isn't clear to me that you've thought about how we generate a multiple records, each with the correct ACID list intended for a specific audit daemon, based on a single audit event. Explain to me how you intend that to work and we are good. Be specific because I'm not convinced we are talking on the same plane here.

paul moore

Quoted 54 lines · Paul Moore

Quoted 47 lines · Richard Guy Briggs

Quoted 38 lines · Paul Moore

Quoted 37 lines · Richard Guy Briggs

Quoted 15 lines · Paul Moore

Quoted 1 line · Richard Guy Briggs

On 2020-03-18 18:06, Paul Moore wrote:

...

Quoted 6 lines

Quoted 4 lines

I hope we can do better than string manipulations in the kernel. I'd much rather defer generating the ACID list (if possible), than generating a list only to keep copying and editing it as the record is sent.

At the moment we are stuck with a string-only format.

Yes, we are. That is another topic, and another set of changes I've been deferring so as to not disrupt the audit container ID work.

Quoted 8 lines

I was thinking of what we do inside the kernel between when the record triggering event happens and when we actually emit the record to userspace. Perhaps we collect the ACID information while the event is occurring, but we defer generating the record until later when we have a better understanding of what should be included in the ACID list. It is somewhat similar (but obviously different) to what we do for PATH records (we collect the pathname info when the path is being resolved).

Ok, now I understand your concern.

In the case of NETFILTER_PKT records, the CONTAINER_ID record is the only other possible record and they are generated at the same time with a local context.

In the case of any event involving a syscall, that CONTAINER_ID record is generated at the time of the rest of the event record generation at syscall exit.

The others are only generated when needed, such as the sig2 reply.

Quoted 7 lines

Quoted 2 lines

We generally just store the contobj pointer until we actually generate the CONTAINER_ID (or CONTAINER_OP) record.

Perhaps I'm remembering your latest spin of these patches incorrectly, but there is still a big gap between when the record is generated and when it is sent up to the audit daemon. Most importantly in that gap is the whole big queue/multicast/unicast mess.

Quoted 5 lines

So you suggest generating that record on the fly once it reaches the end of the audit_queue just before being sent? That sounds... disruptive. Each audit daemon is going to have its own queues, so by the time it ends up in a particular queue, we'll already know its scope and would have the right list of contids to print in that record.

Quoted 7 lines

I'm not suggesting any particular solution, I'm just pointing out a potential problem. It isn't clear to me that you've thought about how we generate a multiple records, each with the correct ACID list intended for a specific audit daemon, based on a single audit event. Explain to me how you intend that to work and we are good. Be specific because I'm not convinced we are talking on the same plane here.

Well, every time a record gets generated, *any* record gets generated, we'll need to check for which audit daemons this record is in scope and generate a different one for each depending on the content and whether or not the content is influenced by the scope.

Quoted 63 lines · Richard Guy Briggs

Quoted 61 lines · Paul Moore

Quoted 47 lines · Richard Guy Briggs

Quoted 38 lines · Paul Moore

Quoted 37 lines · Richard Guy Briggs

Quoted 15 lines · Paul Moore

Quoted 1 line · Richard Guy Briggs

On 2020-03-18 18:06, Paul Moore wrote:

...

Quoted 6 lines

Quoted 4 lines

I hope we can do better than string manipulations in the kernel. I'd much rather defer generating the ACID list (if possible), than generating a list only to keep copying and editing it as the record is sent.

At the moment we are stuck with a string-only format.

Yes, we are. That is another topic, and another set of changes I've been deferring so as to not disrupt the audit container ID work.

Quoted 8 lines

I was thinking of what we do inside the kernel between when the record triggering event happens and when we actually emit the record to userspace. Perhaps we collect the ACID information while the event is occurring, but we defer generating the record until later when we have a better understanding of what should be included in the ACID list. It is somewhat similar (but obviously different) to what we do for PATH records (we collect the pathname info when the path is being resolved).

Ok, now I understand your concern.

In the case of NETFILTER_PKT records, the CONTAINER_ID record is the only other possible record and they are generated at the same time with a local context.

In the case of any event involving a syscall, that CONTAINER_ID record is generated at the time of the rest of the event record generation at syscall exit.

The others are only generated when needed, such as the sig2 reply.

Quoted 7 lines

Quoted 2 lines

We generally just store the contobj pointer until we actually generate the CONTAINER_ID (or CONTAINER_OP) record.

Perhaps I'm remembering your latest spin of these patches incorrectly, but there is still a big gap between when the record is generated and when it is sent up to the audit daemon. Most importantly in that gap is the whole big queue/multicast/unicast mess.

Quoted 5 lines

So you suggest generating that record on the fly once it reaches the end of the audit_queue just before being sent? That sounds... disruptive. Each audit daemon is going to have its own queues, so by the time it ends up in a particular queue, we'll already know its scope and would have the right list of contids to print in that record.

I'm not suggesting any particular solution, I'm just pointing out a potential problem. It isn't clear to me that you've thought about how we generate a multiple records, each with the correct ACID list intended for a specific audit daemon, based on a single audit event. Explain to me how you intend that to work and we are good. Be specific because I'm not convinced we are talking on the same plane here.

Quoted 4 lines

Well, every time a record gets generated, *any* record gets generated, we'll need to check for which audit daemons this record is in scope and generate a different one for each depending on the content and whether or not the content is influenced by the scope.

That's the problem right there - we don't want to have to generate a unique record for *each* auditd on *every* record. That is a recipe for disaster.

Solving this for all of the known audit records is not something we need to worry about in depth at the moment (although giving it some casual thought is not a bad thing), but solving this for the audit container ID information *is* something we need to worry about right now.

paul moore

Quoted 8 lines · Paul Moore

Quoted 7 lines · Richard Guy Briggs

Quoted 6 lines · Paul Moore

Quoted 5 lines · Richard Guy Briggs

Quoted 4 lines · Paul Moore

Quoted 3 lines · Richard Guy Briggs

Quoted 2 lines · Paul Moore

Quoted 1 line · Richard Guy Briggs

On 2020-03-18 18:06, Paul Moore wrote:

Quoted 5 lines

Quoted 4 lines

Well, every time a record gets generated, *any* record gets generated, we'll need to check for which audit daemons this record is in scope and generate a different one for each depending on the content and whether or not the content is influenced by the scope.

Quoted 3 lines

That's the problem right there - we don't want to have to generate a unique record for *each* auditd on *every* record. That is a recipe for disaster.

I don't see how we can get around this.

We will already have that problem for PIDs in different PID namespaces.

We already need to use a different serial number in each auditd/queue, or else we serialize *all* audit events on the machine and either leak information to the nested daemons that there are other events happenning on the machine, or confuse the host daemon because it now thinks that we are losing events due to serial numbers missing because some nested daemon issued an event that was not relevant to the host daemon, consuming a globally serial audit message sequence number.

Quoted 5 lines

Solving this for all of the known audit records is not something we need to worry about in depth at the moment (although giving it some casual thought is not a bad thing), but solving this for the audit container ID information *is* something we need to worry about right now.

If you think that a different nested contid value string per daemon is not acceptable, then we are back to issuing a record that has only *one* contid listed without any nesting information. This brings us back to the original problem of keeping *all* audit log history since the boot of the machine to be able to track the nesting of any particular contid.

What am I missing? What do you suggest?

Quoted 9 lines · Richard Guy Briggs

Quoted 8 lines · Paul Moore

Quoted 7 lines · Richard Guy Briggs

Quoted 6 lines · Paul Moore

Quoted 5 lines · Richard Guy Briggs

Quoted 4 lines · Paul Moore

Quoted 3 lines · Richard Guy Briggs

Quoted 2 lines · Paul Moore

Quoted 1 line · Richard Guy Briggs

On 2020-03-18 18:06, Paul Moore wrote:

...

Quoted 15 lines

Quoted 14 lines

Quoted 4 lines

Well, every time a record gets generated, *any* record gets generated, we'll need to check for which audit daemons this record is in scope and generate a different one for each depending on the content and whether or not the content is influenced by the scope.

That's the problem right there - we don't want to have to generate a unique record for *each* auditd on *every* record. That is a recipe for disaster.

Solving this for all of the known audit records is not something we need to worry about in depth at the moment (although giving it some casual thought is not a bad thing), but solving this for the audit container ID information *is* something we need to worry about right now.

Quoted 5 lines

If you think that a different nested contid value string per daemon is not acceptable, then we are back to issuing a record that has only *one* contid listed without any nesting information. This brings us back to the original problem of keeping *all* audit log history since the boot of the machine to be able to track the nesting of any particular contid.

I'm not ruling anything out, except for the "let's just completely regenerate every record for each auditd instance".

Quoted 28 lines · Paul Moore

Quoted 9 lines · Richard Guy Briggs

Quoted 8 lines · Paul Moore

Quoted 7 lines · Richard Guy Briggs

Quoted 6 lines · Paul Moore

Quoted 5 lines · Richard Guy Briggs

Quoted 4 lines · Paul Moore

Quoted 3 lines · Richard Guy Briggs

Quoted 2 lines · Paul Moore

Quoted 1 line · Richard Guy Briggs

On 2020-03-18 18:06, Paul Moore wrote:

...

Quoted 15 lines

Quoted 14 lines

Quoted 4 lines

Well, every time a record gets generated, *any* record gets generated, we'll need to check for which audit daemons this record is in scope and generate a different one for each depending on the content and whether or not the content is influenced by the scope.

That's the problem right there - we don't want to have to generate a unique record for *each* auditd on *every* record. That is a recipe for disaster.

Solving this for all of the known audit records is not something we need to worry about in depth at the moment (although giving it some casual thought is not a bad thing), but solving this for the audit container ID information *is* something we need to worry about right now.

Quoted 8 lines

Quoted 5 lines

If you think that a different nested contid value string per daemon is not acceptable, then we are back to issuing a record that has only *one* contid listed without any nesting information. This brings us back to the original problem of keeping *all* audit log history since the boot of the machine to be able to track the nesting of any particular contid.

I'm not ruling anything out, except for the "let's just completely regenerate every record for each auditd instance".