Re: [RFC PATCH ghak32 V2 01/13] audit: add container id

[Steve Grubb]

On Fri, 18 May 2018 11:21:06 -0400
Richard Guy Briggs  wrote:

> On 2018-05-18 09:56, Steve Grubb wrote:
> > On Thu, 17 May 2018 17:56:00 -0400
> > Richard Guy Briggs  wrote:
> >   
> > > > During syscall events, the path info is returned in a a record
> > > > simply called AUDIT_PATH, cwd info is returned in AUDIT_CWD. So,
> > > > rather than calling the record that gets attached to everything
> > > > AUDIT_CONTAINER_INFO, how about simply AUDIT_CONTAINER.
> > > 
> > > Considering the container initiation record is different than the
> > > record to document the container involved in an otherwise normal
> > > syscall, we need two names.  I don't have a strong opinion what
> > > they are.
> > > 
> > > I'd prefer AUDIT_CONTAIN and AUDIT_CONTAINER_INFO so that the two
> > > are different enough to be visually distinct while leaving
> > > AUDIT_CONTAINERID for the field type in patch 4 ("audit: add
> > > containerid filtering")  
> 
> (Sorry, I had intended AUDIT_CONTAINER for the first in that paragraph
> above.)
> 
> > How about AUDIT_CONTAINER for the auxiliary record? The one that
> > starts the container, I don't have a strong opinion on. Could be
> > AUDIT_CONTAINER_INIT, AUDIT_CONTAINER_START, AUDIT_CONTAINERID,
> > AUDIT_CONTAINER_ID, or something else. The API call that sets the ID
> > for filtering could be AUDIT_CID or AUDIT_CONTID if that helps
> > decide what the initial event might be. Normally, it should match
> > the field being filtered.  
> 
> Ok, I had shortened the record field name to "contid=" to be unique
> enough while not using too much netlink bandwidth.  I could have used
> "cid=" but that could be unobvious or ambiguous.  I didn't want to use
> the full "containerid=" due to that.  I suppose I could change the
> field name macro to AUDIT_CONTID.
> 
> For the one that starts the container, I'd prefer to leave the name a
> bit more general than "_INIT", "_START", so maybe I'll swap them
> around and use AUDIT_CONTAINER_INFO for the startup record, and use
> AUDIT_CONTAINER for the syscall auxiliary record.
> 
> Does that work?

I'll go along with that. Thanks. But making that swap frees up
AUDIT_CONTAINER_ID which could be the first event. But
AUDIT_CONTAINER_INFO is also fine with me.

Best Regards,
-Steve

Re: [RFC PATCH ghak32 V2 01/13] audit: add container id

[Steve Grubb]

On Fri, 18 May 2018 11:21:06 -0400
Richard Guy Briggs  wrote:

> On 2018-05-18 09:56, Steve Grubb wrote:
> > On Thu, 17 May 2018 17:56:00 -0400
> > Richard Guy Briggs  wrote:
> >   
> > > > During syscall events, the path info is returned in a a record
> > > > simply called AUDIT_PATH, cwd info is returned in AUDIT_CWD. So,
> > > > rather than calling the record that gets attached to everything
> > > > AUDIT_CONTAINER_INFO, how about simply AUDIT_CONTAINER.
> > > 
> > > Considering the container initiation record is different than the
> > > record to document the container involved in an otherwise normal
> > > syscall, we need two names.  I don't have a strong opinion what
> > > they are.
> > > 
> > > I'd prefer AUDIT_CONTAIN and AUDIT_CONTAINER_INFO so that the two
> > > are different enough to be visually distinct while leaving
> > > AUDIT_CONTAINERID for the field type in patch 4 ("audit: add
> > > containerid filtering")  
> 
> (Sorry, I had intended AUDIT_CONTAINER for the first in that paragraph
> above.)
> 
> > How about AUDIT_CONTAINER for the auxiliary record? The one that
> > starts the container, I don't have a strong opinion on. Could be
> > AUDIT_CONTAINER_INIT, AUDIT_CONTAINER_START, AUDIT_CONTAINERID,
> > AUDIT_CONTAINER_ID, or something else. The API call that sets the ID
> > for filtering could be AUDIT_CID or AUDIT_CONTID if that helps
> > decide what the initial event might be. Normally, it should match
> > the field being filtered.  
> 
> Ok, I had shortened the record field name to "contid=" to be unique
> enough while not using too much netlink bandwidth.  I could have used
> "cid=" but that could be unobvious or ambiguous.  I didn't want to use
> the full "containerid=" due to that.  I suppose I could change the
> field name macro to AUDIT_CONTID.
> 
> For the one that starts the container, I'd prefer to leave the name a
> bit more general than "_INIT", "_START", so maybe I'll swap them
> around and use AUDIT_CONTAINER_INFO for the startup record, and use
> AUDIT_CONTAINER for the syscall auxiliary record.
> 
> Does that work?

I'll go along with that. Thanks. But making that swap frees up
AUDIT_CONTAINER_ID which could be the first event. But
AUDIT_CONTAINER_INFO is also fine with me.

Best Regards,
-Steve

Re: [RFC PATCH ghak32 V2 01/13] audit: add container id

[Richard Guy Briggs]

On 2018-05-18 09:56, Steve Grubb wrote:
> On Thu, 17 May 2018 17:56:00 -0400
> Richard Guy Briggs  wrote:
> 
> > > During syscall events, the path info is returned in a a record
> > > simply called AUDIT_PATH, cwd info is returned in AUDIT_CWD. So,
> > > rather than calling the record that gets attached to everything
> > > AUDIT_CONTAINER_INFO, how about simply AUDIT_CONTAINER.  
> > 
> > Considering the container initiation record is different than the
> > record to document the container involved in an otherwise normal
> > syscall, we need two names.  I don't have a strong opinion what they
> > are.
> > 
> > I'd prefer AUDIT_CONTAIN and AUDIT_CONTAINER_INFO so that the two
> > are different enough to be visually distinct while leaving
> > AUDIT_CONTAINERID for the field type in patch 4 ("audit: add
> > containerid filtering")

(Sorry, I had intended AUDIT_CONTAINER for the first in that paragraph
above.)

> How about AUDIT_CONTAINER for the auxiliary record? The one that starts
> the container, I don't have a strong opinion on. Could be
> AUDIT_CONTAINER_INIT, AUDIT_CONTAINER_START, AUDIT_CONTAINERID,
> AUDIT_CONTAINER_ID, or something else. The API call that sets the ID
> for filtering could be AUDIT_CID or AUDIT_CONTID if that helps decide
> what the initial event might be. Normally, it should match the field
> being filtered.

Ok, I had shortened the record field name to "contid=" to be unique
enough while not using too much netlink bandwidth.  I could have used
"cid=" but that could be unobvious or ambiguous.  I didn't want to use
the full "containerid=" due to that.  I suppose I could change the
field name macro to AUDIT_CONTID.

For the one that starts the container, I'd prefer to leave the name a
bit more general than "_INIT", "_START", so maybe I'll swap them around
and use AUDIT_CONTAINER_INFO for the startup record, and use
AUDIT_CONTAINER for the syscall auxiliary record.

Does that work?

> -Steve

- RGB

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

Re: [RFC PATCH ghak32 V2 01/13] audit: add container id

[Richard Guy Briggs]

On 2018-05-18 09:56, Steve Grubb wrote:
> On Thu, 17 May 2018 17:56:00 -0400
> Richard Guy Briggs  wrote:
> 
> > > During syscall events, the path info is returned in a a record
> > > simply called AUDIT_PATH, cwd info is returned in AUDIT_CWD. So,
> > > rather than calling the record that gets attached to everything
> > > AUDIT_CONTAINER_INFO, how about simply AUDIT_CONTAINER.  
> > 
> > Considering the container initiation record is different than the
> > record to document the container involved in an otherwise normal
> > syscall, we need two names.  I don't have a strong opinion what they
> > are.
> > 
> > I'd prefer AUDIT_CONTAIN and AUDIT_CONTAINER_INFO so that the two
> > are different enough to be visually distinct while leaving
> > AUDIT_CONTAINERID for the field type in patch 4 ("audit: add
> > containerid filtering")

(Sorry, I had intended AUDIT_CONTAINER for the first in that paragraph
above.)

> How about AUDIT_CONTAINER for the auxiliary record? The one that starts
> the container, I don't have a strong opinion on. Could be
> AUDIT_CONTAINER_INIT, AUDIT_CONTAINER_START, AUDIT_CONTAINERID,
> AUDIT_CONTAINER_ID, or something else. The API call that sets the ID
> for filtering could be AUDIT_CID or AUDIT_CONTID if that helps decide
> what the initial event might be. Normally, it should match the field
> being filtered.

Ok, I had shortened the record field name to "contid=" to be unique
enough while not using too much netlink bandwidth.  I could have used
"cid=" but that could be unobvious or ambiguous.  I didn't want to use
the full "containerid=" due to that.  I suppose I could change the
field name macro to AUDIT_CONTID.

For the one that starts the container, I'd prefer to leave the name a
bit more general than "_INIT", "_START", so maybe I'll swap them around
and use AUDIT_CONTAINER_INFO for the startup record, and use
AUDIT_CONTAINER for the syscall auxiliary record.

Does that work?

> -Steve

- RGB

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

Re: [RFC PATCH ghak32 V2 01/13] audit: add container id

[Steve Grubb]

On Thu, 17 May 2018 17:56:00 -0400
Richard Guy Briggs  wrote:

> > During syscall events, the path info is returned in a a record
> > simply called AUDIT_PATH, cwd info is returned in AUDIT_CWD. So,
> > rather than calling the record that gets attached to everything
> > AUDIT_CONTAINER_INFO, how about simply AUDIT_CONTAINER.  
> 
> Considering the container initiation record is different than the
> record to document the container involved in an otherwise normal
> syscall, we need two names.  I don't have a strong opinion what they
> are.
> 
> I'd prefer AUDIT_CONTAIN and AUDIT_CONTAINER_INFO so that the two
> are different enough to be visually distinct while leaving
> AUDIT_CONTAINERID for the field type in patch 4 ("audit: add
> containerid filtering")

How about AUDIT_CONTAINER for the auxiliary record? The one that starts
the container, I don't have a strong opinion on. Could be
AUDIT_CONTAINER_INIT, AUDIT_CONTAINER_START, AUDIT_CONTAINERID,
AUDIT_CONTAINER_ID, or something else. The API call that sets the ID
for filtering could be AUDIT_CID or AUDIT_CONTID if that helps decide
what the initial event might be. Normally, it should match the field
being filtered.

Best Regards,
-Steve

Re: [RFC PATCH ghak32 V2 01/13] audit: add container id

[Steve Grubb]

On Thu, 17 May 2018 17:56:00 -0400
Richard Guy Briggs  wrote:

> > During syscall events, the path info is returned in a a record
> > simply called AUDIT_PATH, cwd info is returned in AUDIT_CWD. So,
> > rather than calling the record that gets attached to everything
> > AUDIT_CONTAINER_INFO, how about simply AUDIT_CONTAINER.  
> 
> Considering the container initiation record is different than the
> record to document the container involved in an otherwise normal
> syscall, we need two names.  I don't have a strong opinion what they
> are.
> 
> I'd prefer AUDIT_CONTAIN and AUDIT_CONTAINER_INFO so that the two
> are different enough to be visually distinct while leaving
> AUDIT_CONTAINERID for the field type in patch 4 ("audit: add
> containerid filtering")

How about AUDIT_CONTAINER for the auxiliary record? The one that starts
the container, I don't have a strong opinion on. Could be
AUDIT_CONTAINER_INIT, AUDIT_CONTAINER_START, AUDIT_CONTAINERID,
AUDIT_CONTAINER_ID, or something else. The API call that sets the ID
for filtering could be AUDIT_CID or AUDIT_CONTID if that helps decide
what the initial event might be. Normally, it should match the field
being filtered.

Best Regards,
-Steve

Re: [RFC PATCH ghak32 V2 01/13] audit: add container id

[Richard Guy Briggs]

On 2018-05-17 17:00, Steve Grubb wrote:
> On Fri, 16 Mar 2018 05:00:28 -0400
> Richard Guy Briggs  wrote:
> 
> > Implement the proc fs write to set the audit container ID of a
> > process, emitting an AUDIT_CONTAINER record to document the event.
> > 
> > This is a write from the container orchestrator task to a proc entry
> > of the form /proc/PID/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).
> > 
> > This will produce a record such as this:
> > type=CONTAINER msg=audit(1519903238.968:261): op=set pid=596 uid=0
> > subj=unconfined_u:unconfined_r:unconfined_t:s0-s0:c0.c1023 auid=0
> > tty=pts0 ses=1 opid=596 old-contid=18446744073709551615 contid=123455
> > res=0
> 
> The was one thing I was wondering about. Currently when we set the
> loginuid, the record is AUDIT_LOGINUID. The corollary is that when we
> set the container id, the event should be AUDIT_CONTAINERID or
> AUDIT_CONTAINER_ID.

The record type is actually AUDIT_LOGIN.  The field type is
AUDIT_LOGINUID.  Given that correction, I think we're fine and could
potentially violently agree.  The existing naming is consistent.

> During syscall events, the path info is returned in a a record simply
> called AUDIT_PATH, cwd info is returned in AUDIT_CWD. So, rather than
> calling the record that gets attached to everything
> AUDIT_CONTAINER_INFO, how about simply AUDIT_CONTAINER.

Considering the container initiation record is different than the record
to document the container involved in an otherwise normal syscall, we
need two names.  I don't have a strong opinion what they are.

I'd prefer AUDIT_CONTAINER and AUDIT_CONTAINER_INFO so that the two are
different enough to be visually distinct while leaving
AUDIT_CONTAINERID for the field type in patch 4 ("audit: add containerid
filtering")

> > The "op" field indicates an initial set.  The "pid" to "ses" fields
> > are the orchestrator while the "opid" field is the object's PID, the
> > process being "contained".  Old and new container ID values are given
> > in the "contid" fields, while res indicates its success.
> > 
> > It is not permitted to self-set, unset or re-set the container ID.  A
> > child inherits its parent's container ID, but then can be set only
> > once after.
> > 
> > See: https://github.com/linux-audit/audit-kernel/issues/32
> > 
> > Signed-off-by: Richard Guy Briggs 
> > ---
> >  fs/proc/base.c | 37 
> >  include/linux/audit.h  | 16 +
> >  include/linux/init_task.h  |  4 ++-
> >  include/linux/sched.h  |  1 +
> >  include/uapi/linux/audit.h |  2 ++
> >  kernel/auditsc.c   | 84
> > ++ 6 files changed, 143
> > insertions(+), 1 deletion(-)
> > 
> > diff --git a/fs/proc/base.c b/fs/proc/base.c
> > index 60316b5..6ce4fbe 100644
> > --- a/fs/proc/base.c
> > +++ b/fs/proc/base.c
> > @@ -1299,6 +1299,41 @@ static ssize_t proc_sessionid_read(struct file
> > * file, char __user * buf, .read= proc_sessionid_read,
> > .llseek = generic_file_llseek,
> >  };
> > +
> > +static ssize_t proc_containerid_write(struct file *file, const char
> > __user *buf,
> > +  size_t count, loff_t *ppos)
> > +{
> > +   struct inode *inode = file_inode(file);
> > +   u64 containerid;
> > +   int rv;
> > +   struct task_struct *task = get_proc_task(inode);
> > +
> > +   if (!task)
> > +   return -ESRCH;
> > +   if (*ppos != 0) {
> > +   /* No partial writes. */
> > +   put_task_struct(task);
> > +   return -EINVAL;
> > +   }
> > +
> > +   rv = kstrtou64_from_user(buf, count, 10, );
> > +   if (rv < 0) {
> > +   put_task_struct(task);
> > +   return rv;
> > +   }
> > +
> > +   rv = audit_set_containerid(task, containerid);
> > +   put_task_struct(task);
> > +   if (rv < 0)
> > +   return rv;
> > +   return count;
> > +}
> > +
> > +static const struct file_operations proc_containerid_operations = {
> > +   .write  = proc_containerid_write,
> > +   .llseek = generic_file_llseek,
> > +};
> > +
> >  #endif
> >  
> >  #ifdef CONFIG_FAULT_INJECTION
> > @@ -2961,6 +2996,7 @@ static int proc_pid_patch_state(struct seq_file
> > *m, struct pid_namespace *ns, #ifdef CONFIG_AUDITSYSCALL
> > REG("loginuid",   S_IWUSR|S_IRUGO, proc_loginuid_operations),
> > REG("sessionid",  S_IRUGO, proc_sessionid_operations),
> > +   REG("containerid", S_IWUSR, proc_containerid_operations),
> >  #endif
> >  #ifdef CONFIG_FAULT_INJECTION
> > REG("make-it-fail", S_IRUGO|S_IWUSR,
> > proc_fault_inject_operations), @@ -3355,6 +3391,7 @@ static int
> > proc_tid_comm_permission(struct inode *inode, int mask) #ifdef
> > CONFIG_AUDITSYSCALL REG("loginuid",  S_IWUSR|S_IRUGO,
> > 

Re: [RFC PATCH ghak32 V2 01/13] audit: add container id

[Richard Guy Briggs]

On 2018-05-17 17:00, Steve Grubb wrote:
> On Fri, 16 Mar 2018 05:00:28 -0400
> Richard Guy Briggs  wrote:
> 
> > Implement the proc fs write to set the audit container ID of a
> > process, emitting an AUDIT_CONTAINER record to document the event.
> > 
> > This is a write from the container orchestrator task to a proc entry
> > of the form /proc/PID/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).
> > 
> > This will produce a record such as this:
> > type=CONTAINER msg=audit(1519903238.968:261): op=set pid=596 uid=0
> > subj=unconfined_u:unconfined_r:unconfined_t:s0-s0:c0.c1023 auid=0
> > tty=pts0 ses=1 opid=596 old-contid=18446744073709551615 contid=123455
> > res=0
> 
> The was one thing I was wondering about. Currently when we set the
> loginuid, the record is AUDIT_LOGINUID. The corollary is that when we
> set the container id, the event should be AUDIT_CONTAINERID or
> AUDIT_CONTAINER_ID.

The record type is actually AUDIT_LOGIN.  The field type is
AUDIT_LOGINUID.  Given that correction, I think we're fine and could
potentially violently agree.  The existing naming is consistent.

> During syscall events, the path info is returned in a a record simply
> called AUDIT_PATH, cwd info is returned in AUDIT_CWD. So, rather than
> calling the record that gets attached to everything
> AUDIT_CONTAINER_INFO, how about simply AUDIT_CONTAINER.

Considering the container initiation record is different than the record
to document the container involved in an otherwise normal syscall, we
need two names.  I don't have a strong opinion what they are.

I'd prefer AUDIT_CONTAINER and AUDIT_CONTAINER_INFO so that the two are
different enough to be visually distinct while leaving
AUDIT_CONTAINERID for the field type in patch 4 ("audit: add containerid
filtering")

> > The "op" field indicates an initial set.  The "pid" to "ses" fields
> > are the orchestrator while the "opid" field is the object's PID, the
> > process being "contained".  Old and new container ID values are given
> > in the "contid" fields, while res indicates its success.
> > 
> > It is not permitted to self-set, unset or re-set the container ID.  A
> > child inherits its parent's container ID, but then can be set only
> > once after.
> > 
> > See: https://github.com/linux-audit/audit-kernel/issues/32
> > 
> > Signed-off-by: Richard Guy Briggs 
> > ---
> >  fs/proc/base.c | 37 
> >  include/linux/audit.h  | 16 +
> >  include/linux/init_task.h  |  4 ++-
> >  include/linux/sched.h  |  1 +
> >  include/uapi/linux/audit.h |  2 ++
> >  kernel/auditsc.c   | 84
> > ++ 6 files changed, 143
> > insertions(+), 1 deletion(-)
> > 
> > diff --git a/fs/proc/base.c b/fs/proc/base.c
> > index 60316b5..6ce4fbe 100644
> > --- a/fs/proc/base.c
> > +++ b/fs/proc/base.c
> > @@ -1299,6 +1299,41 @@ static ssize_t proc_sessionid_read(struct file
> > * file, char __user * buf, .read= proc_sessionid_read,
> > .llseek = generic_file_llseek,
> >  };
> > +
> > +static ssize_t proc_containerid_write(struct file *file, const char
> > __user *buf,
> > +  size_t count, loff_t *ppos)
> > +{
> > +   struct inode *inode = file_inode(file);
> > +   u64 containerid;
> > +   int rv;
> > +   struct task_struct *task = get_proc_task(inode);
> > +
> > +   if (!task)
> > +   return -ESRCH;
> > +   if (*ppos != 0) {
> > +   /* No partial writes. */
> > +   put_task_struct(task);
> > +   return -EINVAL;
> > +   }
> > +
> > +   rv = kstrtou64_from_user(buf, count, 10, );
> > +   if (rv < 0) {
> > +   put_task_struct(task);
> > +   return rv;
> > +   }
> > +
> > +   rv = audit_set_containerid(task, containerid);
> > +   put_task_struct(task);
> > +   if (rv < 0)
> > +   return rv;
> > +   return count;
> > +}
> > +
> > +static const struct file_operations proc_containerid_operations = {
> > +   .write  = proc_containerid_write,
> > +   .llseek = generic_file_llseek,
> > +};
> > +
> >  #endif
> >  
> >  #ifdef CONFIG_FAULT_INJECTION
> > @@ -2961,6 +2996,7 @@ static int proc_pid_patch_state(struct seq_file
> > *m, struct pid_namespace *ns, #ifdef CONFIG_AUDITSYSCALL
> > REG("loginuid",   S_IWUSR|S_IRUGO, proc_loginuid_operations),
> > REG("sessionid",  S_IRUGO, proc_sessionid_operations),
> > +   REG("containerid", S_IWUSR, proc_containerid_operations),
> >  #endif
> >  #ifdef CONFIG_FAULT_INJECTION
> > REG("make-it-fail", S_IRUGO|S_IWUSR,
> > proc_fault_inject_operations), @@ -3355,6 +3391,7 @@ static int
> > proc_tid_comm_permission(struct inode *inode, int mask) #ifdef
> > CONFIG_AUDITSYSCALL REG("loginuid",  S_IWUSR|S_IRUGO,
> > proc_loginuid_operations), REG("sessionid",  

Re: [RFC PATCH ghak32 V2 01/13] audit: add container id

[Steve Grubb]

On Fri, 16 Mar 2018 05:00:28 -0400
Richard Guy Briggs  wrote:

> Implement the proc fs write to set the audit container ID of a
> process, emitting an AUDIT_CONTAINER record to document the event.
> 
> This is a write from the container orchestrator task to a proc entry
> of the form /proc/PID/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).
> 
> This will produce a record such as this:
> type=CONTAINER msg=audit(1519903238.968:261): op=set pid=596 uid=0
> subj=unconfined_u:unconfined_r:unconfined_t:s0-s0:c0.c1023 auid=0
> tty=pts0 ses=1 opid=596 old-contid=18446744073709551615 contid=123455
> res=0

The was one thing I was wondering about. Currently when we set the
loginuid, the record is AUDIT_LOGINUID. The corollary is that when we
set the container id, the event should be AUDIT_CONTAINERID or
AUDIT_CONTAINER_ID.

During syscall events, the path info is returned in a a record simply
called AUDIT_PATH, cwd info is returned in AUDIT_CWD. So, rather than
calling the record that gets attached to everything
AUDIT_CONTAINER_INFO, how about simply AUDIT_CONTAINER.


> The "op" field indicates an initial set.  The "pid" to "ses" fields
> are the orchestrator while the "opid" field is the object's PID, the
> process being "contained".  Old and new container ID values are given
> in the "contid" fields, while res indicates its success.
> 
> It is not permitted to self-set, unset or re-set the container ID.  A
> child inherits its parent's container ID, but then can be set only
> once after.
> 
> See: https://github.com/linux-audit/audit-kernel/issues/32
> 
> Signed-off-by: Richard Guy Briggs 
> ---
>  fs/proc/base.c | 37 
>  include/linux/audit.h  | 16 +
>  include/linux/init_task.h  |  4 ++-
>  include/linux/sched.h  |  1 +
>  include/uapi/linux/audit.h |  2 ++
>  kernel/auditsc.c   | 84
> ++ 6 files changed, 143
> insertions(+), 1 deletion(-)
> 
> diff --git a/fs/proc/base.c b/fs/proc/base.c
> index 60316b5..6ce4fbe 100644
> --- a/fs/proc/base.c
> +++ b/fs/proc/base.c
> @@ -1299,6 +1299,41 @@ static ssize_t proc_sessionid_read(struct file
> * file, char __user * buf, .read  = proc_sessionid_read,
>   .llseek = generic_file_llseek,
>  };
> +
> +static ssize_t proc_containerid_write(struct file *file, const char
> __user *buf,
> +size_t count, loff_t *ppos)
> +{
> + struct inode *inode = file_inode(file);
> + u64 containerid;
> + int rv;
> + struct task_struct *task = get_proc_task(inode);
> +
> + if (!task)
> + return -ESRCH;
> + if (*ppos != 0) {
> + /* No partial writes. */
> + put_task_struct(task);
> + return -EINVAL;
> + }
> +
> + rv = kstrtou64_from_user(buf, count, 10, );
> + if (rv < 0) {
> + put_task_struct(task);
> + return rv;
> + }
> +
> + rv = audit_set_containerid(task, containerid);
> + put_task_struct(task);
> + if (rv < 0)
> + return rv;
> + return count;
> +}
> +
> +static const struct file_operations proc_containerid_operations = {
> + .write  = proc_containerid_write,
> + .llseek = generic_file_llseek,
> +};
> +
>  #endif
>  
>  #ifdef CONFIG_FAULT_INJECTION
> @@ -2961,6 +2996,7 @@ static int proc_pid_patch_state(struct seq_file
> *m, struct pid_namespace *ns, #ifdef CONFIG_AUDITSYSCALL
>   REG("loginuid",   S_IWUSR|S_IRUGO, proc_loginuid_operations),
>   REG("sessionid",  S_IRUGO, proc_sessionid_operations),
> + REG("containerid", S_IWUSR, proc_containerid_operations),
>  #endif
>  #ifdef CONFIG_FAULT_INJECTION
>   REG("make-it-fail", S_IRUGO|S_IWUSR,
> proc_fault_inject_operations), @@ -3355,6 +3391,7 @@ static int
> proc_tid_comm_permission(struct inode *inode, int mask) #ifdef
> CONFIG_AUDITSYSCALL REG("loginuid",  S_IWUSR|S_IRUGO,
> proc_loginuid_operations), REG("sessionid",  S_IRUGO,
> proc_sessionid_operations),
> + REG("containerid", S_IWUSR, proc_containerid_operations),
>  #endif
>  #ifdef CONFIG_FAULT_INJECTION
>   REG("make-it-fail", S_IRUGO|S_IWUSR,
> proc_fault_inject_operations), diff --git a/include/linux/audit.h
> b/include/linux/audit.h index af410d9..fe4ba3f 100644
> --- a/include/linux/audit.h
> +++ b/include/linux/audit.h
> @@ -29,6 +29,7 @@
>  
>  #define AUDIT_INO_UNSET ((unsigned long)-1)
>  #define AUDIT_DEV_UNSET ((dev_t)-1)
> +#define INVALID_CID AUDIT_CID_UNSET
>  
>  struct audit_sig_info {
>   uid_t   uid;
> @@ -321,6 +322,7 @@ static inline void audit_ptrace(struct
> task_struct *t) extern int auditsc_get_stamp(struct audit_context
> *ctx, struct timespec64 *t, unsigned int *serial);
>  extern 

Re: [RFC PATCH ghak32 V2 01/13] audit: add container id

[Steve Grubb]

On Fri, 16 Mar 2018 05:00:28 -0400
Richard Guy Briggs  wrote:

> Implement the proc fs write to set the audit container ID of a
> process, emitting an AUDIT_CONTAINER record to document the event.
> 
> This is a write from the container orchestrator task to a proc entry
> of the form /proc/PID/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).
> 
> This will produce a record such as this:
> type=CONTAINER msg=audit(1519903238.968:261): op=set pid=596 uid=0
> subj=unconfined_u:unconfined_r:unconfined_t:s0-s0:c0.c1023 auid=0
> tty=pts0 ses=1 opid=596 old-contid=18446744073709551615 contid=123455
> res=0

The was one thing I was wondering about. Currently when we set the
loginuid, the record is AUDIT_LOGINUID. The corollary is that when we
set the container id, the event should be AUDIT_CONTAINERID or
AUDIT_CONTAINER_ID.

During syscall events, the path info is returned in a a record simply
called AUDIT_PATH, cwd info is returned in AUDIT_CWD. So, rather than
calling the record that gets attached to everything
AUDIT_CONTAINER_INFO, how about simply AUDIT_CONTAINER.


> The "op" field indicates an initial set.  The "pid" to "ses" fields
> are the orchestrator while the "opid" field is the object's PID, the
> process being "contained".  Old and new container ID values are given
> in the "contid" fields, while res indicates its success.
> 
> It is not permitted to self-set, unset or re-set the container ID.  A
> child inherits its parent's container ID, but then can be set only
> once after.
> 
> See: https://github.com/linux-audit/audit-kernel/issues/32
> 
> Signed-off-by: Richard Guy Briggs 
> ---
>  fs/proc/base.c | 37 
>  include/linux/audit.h  | 16 +
>  include/linux/init_task.h  |  4 ++-
>  include/linux/sched.h  |  1 +
>  include/uapi/linux/audit.h |  2 ++
>  kernel/auditsc.c   | 84
> ++ 6 files changed, 143
> insertions(+), 1 deletion(-)
> 
> diff --git a/fs/proc/base.c b/fs/proc/base.c
> index 60316b5..6ce4fbe 100644
> --- a/fs/proc/base.c
> +++ b/fs/proc/base.c
> @@ -1299,6 +1299,41 @@ static ssize_t proc_sessionid_read(struct file
> * file, char __user * buf, .read  = proc_sessionid_read,
>   .llseek = generic_file_llseek,
>  };
> +
> +static ssize_t proc_containerid_write(struct file *file, const char
> __user *buf,
> +size_t count, loff_t *ppos)
> +{
> + struct inode *inode = file_inode(file);
> + u64 containerid;
> + int rv;
> + struct task_struct *task = get_proc_task(inode);
> +
> + if (!task)
> + return -ESRCH;
> + if (*ppos != 0) {
> + /* No partial writes. */
> + put_task_struct(task);
> + return -EINVAL;
> + }
> +
> + rv = kstrtou64_from_user(buf, count, 10, );
> + if (rv < 0) {
> + put_task_struct(task);
> + return rv;
> + }
> +
> + rv = audit_set_containerid(task, containerid);
> + put_task_struct(task);
> + if (rv < 0)
> + return rv;
> + return count;
> +}
> +
> +static const struct file_operations proc_containerid_operations = {
> + .write  = proc_containerid_write,
> + .llseek = generic_file_llseek,
> +};
> +
>  #endif
>  
>  #ifdef CONFIG_FAULT_INJECTION
> @@ -2961,6 +2996,7 @@ static int proc_pid_patch_state(struct seq_file
> *m, struct pid_namespace *ns, #ifdef CONFIG_AUDITSYSCALL
>   REG("loginuid",   S_IWUSR|S_IRUGO, proc_loginuid_operations),
>   REG("sessionid",  S_IRUGO, proc_sessionid_operations),
> + REG("containerid", S_IWUSR, proc_containerid_operations),
>  #endif
>  #ifdef CONFIG_FAULT_INJECTION
>   REG("make-it-fail", S_IRUGO|S_IWUSR,
> proc_fault_inject_operations), @@ -3355,6 +3391,7 @@ static int
> proc_tid_comm_permission(struct inode *inode, int mask) #ifdef
> CONFIG_AUDITSYSCALL REG("loginuid",  S_IWUSR|S_IRUGO,
> proc_loginuid_operations), REG("sessionid",  S_IRUGO,
> proc_sessionid_operations),
> + REG("containerid", S_IWUSR, proc_containerid_operations),
>  #endif
>  #ifdef CONFIG_FAULT_INJECTION
>   REG("make-it-fail", S_IRUGO|S_IWUSR,
> proc_fault_inject_operations), diff --git a/include/linux/audit.h
> b/include/linux/audit.h index af410d9..fe4ba3f 100644
> --- a/include/linux/audit.h
> +++ b/include/linux/audit.h
> @@ -29,6 +29,7 @@
>  
>  #define AUDIT_INO_UNSET ((unsigned long)-1)
>  #define AUDIT_DEV_UNSET ((dev_t)-1)
> +#define INVALID_CID AUDIT_CID_UNSET
>  
>  struct audit_sig_info {
>   uid_t   uid;
> @@ -321,6 +322,7 @@ static inline void audit_ptrace(struct
> task_struct *t) extern int auditsc_get_stamp(struct audit_context
> *ctx, struct timespec64 *t, unsigned int *serial);
>  extern int audit_set_loginuid(kuid_t 

Re: [RFC PATCH ghak32 V2 01/13] audit: add container id

[Richard Guy Briggs]

On 2018-04-18 19:47, Paul Moore wrote:
> On Fri, Mar 16, 2018 at 5:00 AM, Richard Guy Briggs  wrote:
> > Implement the proc fs write to set the audit container ID of a process,
> > emitting an AUDIT_CONTAINER record to document the event.
> >
> > This is a write from the container orchestrator task to a proc entry of
> > the form /proc/PID/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).
> >
> > This will produce a record such as this:
> > type=CONTAINER msg=audit(1519903238.968:261): op=set pid=596 uid=0 
> > subj=unconfined_u:unconfined_r:unconfined_t:s0-s0:c0.c1023 auid=0 tty=pts0 
> > ses=1 opid=596 old-contid=18446744073709551615 contid=123455 res=0
> >
> > The "op" field indicates an initial set.  The "pid" to "ses" fields are
> > the orchestrator while the "opid" field is the object's PID, the process
> > being "contained".  Old and new container ID values are given in the
> > "contid" fields, while res indicates its success.
> >
> > It is not permitted to self-set, unset or re-set the container ID.  A
> > child inherits its parent's container ID, but then can be set only once
> > after.
> >
> > See: https://github.com/linux-audit/audit-kernel/issues/32
> >
> > Signed-off-by: Richard Guy Briggs 
> > ---
> >  fs/proc/base.c | 37 
> >  include/linux/audit.h  | 16 +
> >  include/linux/init_task.h  |  4 ++-
> >  include/linux/sched.h  |  1 +
> >  include/uapi/linux/audit.h |  2 ++
> >  kernel/auditsc.c   | 84 
> > ++
> >  6 files changed, 143 insertions(+), 1 deletion(-)
> >
> > diff --git a/fs/proc/base.c b/fs/proc/base.c
> > index 60316b5..6ce4fbe 100644
> > --- a/fs/proc/base.c
> > +++ b/fs/proc/base.c
> > @@ -1299,6 +1299,41 @@ static ssize_t proc_sessionid_read(struct file * 
> > file, char __user * buf,
> > .read   = proc_sessionid_read,
> > .llseek = generic_file_llseek,
> >  };
> > +
> > +static ssize_t proc_containerid_write(struct file *file, const char __user 
> > *buf,
> > +  size_t count, loff_t *ppos)
> > +{
> > +   struct inode *inode = file_inode(file);
> > +   u64 containerid;
> > +   int rv;
> > +   struct task_struct *task = get_proc_task(inode);
> > +
> > +   if (!task)
> > +   return -ESRCH;
> > +   if (*ppos != 0) {
> > +   /* No partial writes. */
> > +   put_task_struct(task);
> > +   return -EINVAL;
> > +   }
> > +
> > +   rv = kstrtou64_from_user(buf, count, 10, );
> > +   if (rv < 0) {
> > +   put_task_struct(task);
> > +   return rv;
> > +   }
> > +
> > +   rv = audit_set_containerid(task, containerid);
> > +   put_task_struct(task);
> > +   if (rv < 0)
> > +   return rv;
> > +   return count;
> > +}
> > +
> > +static const struct file_operations proc_containerid_operations = {
> > +   .write  = proc_containerid_write,
> > +   .llseek = generic_file_llseek,
> > +};
> > +
> >  #endif
> >
> >  #ifdef CONFIG_FAULT_INJECTION
> > @@ -2961,6 +2996,7 @@ static int proc_pid_patch_state(struct seq_file *m, 
> > struct pid_namespace *ns,
> >  #ifdef CONFIG_AUDITSYSCALL
> > REG("loginuid",   S_IWUSR|S_IRUGO, proc_loginuid_operations),
> > REG("sessionid",  S_IRUGO, proc_sessionid_operations),
> > +   REG("containerid", S_IWUSR, proc_containerid_operations),
> >  #endif
> >  #ifdef CONFIG_FAULT_INJECTION
> > REG("make-it-fail", S_IRUGO|S_IWUSR, proc_fault_inject_operations),
> > @@ -3355,6 +3391,7 @@ static int proc_tid_comm_permission(struct inode 
> > *inode, int mask)
> >  #ifdef CONFIG_AUDITSYSCALL
> > REG("loginuid",  S_IWUSR|S_IRUGO, proc_loginuid_operations),
> > REG("sessionid",  S_IRUGO, proc_sessionid_operations),
> > +   REG("containerid", S_IWUSR, proc_containerid_operations),
> >  #endif
> >  #ifdef CONFIG_FAULT_INJECTION
> > REG("make-it-fail", S_IRUGO|S_IWUSR, proc_fault_inject_operations),

...

> > diff --git a/include/linux/sched.h b/include/linux/sched.h
> > index d258826..1b82191 100644
> > --- a/include/linux/sched.h
> > +++ b/include/linux/sched.h
> > @@ -796,6 +796,7 @@ struct task_struct {
> >  #ifdef CONFIG_AUDITSYSCALL
> > kuid_t  loginuid;
> > unsigned intsessionid;
> > +   u64 containerid;
> 
> This one line addition to the task_struct scares me the most of
> anything in this patchset.  Why?  It's a field named "containerid" in
> a perhaps one of the most widely used core kernel structures; the
> possibilities for abuse are endless, and it's foolish to think we
> would 

Re: [RFC PATCH ghak32 V2 01/13] audit: add container id

[Richard Guy Briggs]

On 2018-04-18 19:47, Paul Moore wrote:
> On Fri, Mar 16, 2018 at 5:00 AM, Richard Guy Briggs  wrote:
> > Implement the proc fs write to set the audit container ID of a process,
> > emitting an AUDIT_CONTAINER record to document the event.
> >
> > This is a write from the container orchestrator task to a proc entry of
> > the form /proc/PID/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).
> >
> > This will produce a record such as this:
> > type=CONTAINER msg=audit(1519903238.968:261): op=set pid=596 uid=0 
> > subj=unconfined_u:unconfined_r:unconfined_t:s0-s0:c0.c1023 auid=0 tty=pts0 
> > ses=1 opid=596 old-contid=18446744073709551615 contid=123455 res=0
> >
> > The "op" field indicates an initial set.  The "pid" to "ses" fields are
> > the orchestrator while the "opid" field is the object's PID, the process
> > being "contained".  Old and new container ID values are given in the
> > "contid" fields, while res indicates its success.
> >
> > It is not permitted to self-set, unset or re-set the container ID.  A
> > child inherits its parent's container ID, but then can be set only once
> > after.
> >
> > See: https://github.com/linux-audit/audit-kernel/issues/32
> >
> > Signed-off-by: Richard Guy Briggs 
> > ---
> >  fs/proc/base.c | 37 
> >  include/linux/audit.h  | 16 +
> >  include/linux/init_task.h  |  4 ++-
> >  include/linux/sched.h  |  1 +
> >  include/uapi/linux/audit.h |  2 ++
> >  kernel/auditsc.c   | 84 
> > ++
> >  6 files changed, 143 insertions(+), 1 deletion(-)
> >
> > diff --git a/fs/proc/base.c b/fs/proc/base.c
> > index 60316b5..6ce4fbe 100644
> > --- a/fs/proc/base.c
> > +++ b/fs/proc/base.c
> > @@ -1299,6 +1299,41 @@ static ssize_t proc_sessionid_read(struct file * 
> > file, char __user * buf,
> > .read   = proc_sessionid_read,
> > .llseek = generic_file_llseek,
> >  };
> > +
> > +static ssize_t proc_containerid_write(struct file *file, const char __user 
> > *buf,
> > +  size_t count, loff_t *ppos)
> > +{
> > +   struct inode *inode = file_inode(file);
> > +   u64 containerid;
> > +   int rv;
> > +   struct task_struct *task = get_proc_task(inode);
> > +
> > +   if (!task)
> > +   return -ESRCH;
> > +   if (*ppos != 0) {
> > +   /* No partial writes. */
> > +   put_task_struct(task);
> > +   return -EINVAL;
> > +   }
> > +
> > +   rv = kstrtou64_from_user(buf, count, 10, );
> > +   if (rv < 0) {
> > +   put_task_struct(task);
> > +   return rv;
> > +   }
> > +
> > +   rv = audit_set_containerid(task, containerid);
> > +   put_task_struct(task);
> > +   if (rv < 0)
> > +   return rv;
> > +   return count;
> > +}
> > +
> > +static const struct file_operations proc_containerid_operations = {
> > +   .write  = proc_containerid_write,
> > +   .llseek = generic_file_llseek,
> > +};
> > +
> >  #endif
> >
> >  #ifdef CONFIG_FAULT_INJECTION
> > @@ -2961,6 +2996,7 @@ static int proc_pid_patch_state(struct seq_file *m, 
> > struct pid_namespace *ns,
> >  #ifdef CONFIG_AUDITSYSCALL
> > REG("loginuid",   S_IWUSR|S_IRUGO, proc_loginuid_operations),
> > REG("sessionid",  S_IRUGO, proc_sessionid_operations),
> > +   REG("containerid", S_IWUSR, proc_containerid_operations),
> >  #endif
> >  #ifdef CONFIG_FAULT_INJECTION
> > REG("make-it-fail", S_IRUGO|S_IWUSR, proc_fault_inject_operations),
> > @@ -3355,6 +3391,7 @@ static int proc_tid_comm_permission(struct inode 
> > *inode, int mask)
> >  #ifdef CONFIG_AUDITSYSCALL
> > REG("loginuid",  S_IWUSR|S_IRUGO, proc_loginuid_operations),
> > REG("sessionid",  S_IRUGO, proc_sessionid_operations),
> > +   REG("containerid", S_IWUSR, proc_containerid_operations),
> >  #endif
> >  #ifdef CONFIG_FAULT_INJECTION
> > REG("make-it-fail", S_IRUGO|S_IWUSR, proc_fault_inject_operations),

...

> > diff --git a/include/linux/sched.h b/include/linux/sched.h
> > index d258826..1b82191 100644
> > --- a/include/linux/sched.h
> > +++ b/include/linux/sched.h
> > @@ -796,6 +796,7 @@ struct task_struct {
> >  #ifdef CONFIG_AUDITSYSCALL
> > kuid_t  loginuid;
> > unsigned intsessionid;
> > +   u64 containerid;
> 
> This one line addition to the task_struct scares me the most of
> anything in this patchset.  Why?  It's a field named "containerid" in
> a perhaps one of the most widely used core kernel structures; the
> possibilities for abuse are endless, and it's foolish to think we
> would ever be able to adequately police 

Re: [RFC PATCH ghak32 V2 01/13] audit: add container id

[Paul Moore]

On Tue, Apr 24, 2018 at 8:40 PM, Richard Guy Briggs  wrote:
> On 2018-04-24 15:01, Paul Moore wrote:
>> On Mon, Apr 23, 2018 at 10:02 PM, Richard Guy Briggs  wrote:
>> > On 2018-04-23 19:15, Paul Moore wrote:
>> >> On Sat, Apr 21, 2018 at 10:34 AM, Richard Guy Briggs  
>> >> wrote:
>> >> > On 2018-04-18 19:47, Paul Moore wrote:
>> >> >> On Fri, Mar 16, 2018 at 5:00 AM, Richard Guy Briggs  
>> >> >> wrote:
>> >> >> > Implement the proc fs write to set the audit container ID of a 
>> >> >> > process,
>> >> >> > emitting an AUDIT_CONTAINER record to document the event.
>> >> >> >
>> >> >> > This is a write from the container orchestrator task to a proc entry 
>> >> >> > of
>> >> >> > the form /proc/PID/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).
>> >> >> >
>> >> >> > This will produce a record such as this:
>> >> >> > type=CONTAINER msg=audit(1519903238.968:261): op=set pid=596 uid=0 
>> >> >> > subj=unconfined_u:unconfined_r:unconfined_t:s0-s0:c0.c1023 auid=0 
>> >> >> > tty=pts0 ses=1 opid=596 old-contid=18446744073709551615 
>> >> >> > contid=123455 res=0
>> >> >> >
>> >> >> > The "op" field indicates an initial set.  The "pid" to "ses" fields 
>> >> >> > are
>> >> >> > the orchestrator while the "opid" field is the object's PID, the 
>> >> >> > process
>> >> >> > being "contained".  Old and new container ID values are given in the
>> >> >> > "contid" fields, while res indicates its success.
>> >> >> >
>> >> >> > It is not permitted to self-set, unset or re-set the container ID.  A
>> >> >> > child inherits its parent's container ID, but then can be set only 
>> >> >> > once
>> >> >> > after.
>> >> >> >
>> >> >> > See: https://github.com/linux-audit/audit-kernel/issues/32
>> >> >> >
>> >> >> > Signed-off-by: Richard Guy Briggs 
>> >> >> > ---
>> >> >> >  fs/proc/base.c | 37 
>> >> >> >  include/linux/audit.h  | 16 +
>> >> >> >  include/linux/init_task.h  |  4 ++-
>> >> >> >  include/linux/sched.h  |  1 +
>> >> >> >  include/uapi/linux/audit.h |  2 ++
>> >> >> >  kernel/auditsc.c   | 84 
>> >> >> > ++
>> >> >> >  6 files changed, 143 insertions(+), 1 deletion(-)
>>
>> ...
>>
>> >> >> >  /* audit_rule_data supports filter rules with both integer and 
>> >> >> > string
>> >> >> >   * fields.  It corresponds with AUDIT_ADD_RULE, AUDIT_DEL_RULE and
>> >> >> > diff --git a/kernel/auditsc.c b/kernel/auditsc.c
>> >> >> > index 4e0a4ac..29c8482 100644
>> >> >> > --- a/kernel/auditsc.c
>> >> >> > +++ b/kernel/auditsc.c
>> >> >> > @@ -2073,6 +2073,90 @@ int audit_set_loginuid(kuid_t loginuid)
>> >> >> > return rc;
>> >> >> >  }
>> >> >> >
>> >> >> > +static int audit_set_containerid_perm(struct task_struct *task, u64 
>> >> >> > containerid)
>> >> >> > +{
>> >> >> > +   struct task_struct *parent;
>> >> >> > +   u64 pcontainerid, ccontainerid;
>> >> >> > +
>> >> >> > +   /* Don't allow to set our own containerid */
>> >> >> > +   if (current == task)
>> >> >> > +   return -EPERM;
>> >> >>
>> >> >> Why not?  Is there some obvious security concern that I missing?
>> >> >
>> >> > We then lose the distinction in the AUDIT_CONTAINER record between the
>> >> > initiating PID and the target PID.  This was outlined in the proposal.
>> >>
>> >> I just went back and reread the v3 proposal and I still don't see a
>> >> good explanation of this.  Why is this bad?  What's the security
>> >> concern?
>> >
>> > I don't remember, specifically.  Maybe this has been addressed by the
>> > check for children/threads or identical parent container ID.  So, I'm
>> > reluctantly willing to remove that check for now.
>>
>> Okay.  For the record, if someone can explain to me why this
>> restriction saves us from some terrible situation I'm all for leaving
>> it.  I'm just opposed to restrictions without solid reasoning behind
>> them.
>>
>> >> > Having said that, I'm still not sure we have protected sufficiently from
>> >> > a child turning around and setting it's parent's as yet unset or
>> >> > inherited audit container ID.
>> >>
>> >> Yes, I believe we only want to let a task set the audit container for
>> >> it's children (or itself/threads if we decide to allow that, see
>> >> above).  There *has* to be a function to check to see if a task if a
>> >> child of a given task ... right? ... although this is likely to be a
>> >> pointer traversal and locking nightmare ... hmmm.
>> >
>> > Isn't that just (struct task_struct)parent == (struct
>> > task_struct)child->parent (or ->real_parent)?
>> >
>> > And now that I say that, it is covered by the following patch's child
>> > check, so as long 

Re: [RFC PATCH ghak32 V2 01/13] audit: add container id

[Paul Moore]

On Tue, Apr 24, 2018 at 8:40 PM, Richard Guy Briggs  wrote:
> On 2018-04-24 15:01, Paul Moore wrote:
>> On Mon, Apr 23, 2018 at 10:02 PM, Richard Guy Briggs  wrote:
>> > On 2018-04-23 19:15, Paul Moore wrote:
>> >> On Sat, Apr 21, 2018 at 10:34 AM, Richard Guy Briggs  
>> >> wrote:
>> >> > On 2018-04-18 19:47, Paul Moore wrote:
>> >> >> On Fri, Mar 16, 2018 at 5:00 AM, Richard Guy Briggs  
>> >> >> wrote:
>> >> >> > Implement the proc fs write to set the audit container ID of a 
>> >> >> > process,
>> >> >> > emitting an AUDIT_CONTAINER record to document the event.
>> >> >> >
>> >> >> > This is a write from the container orchestrator task to a proc entry 
>> >> >> > of
>> >> >> > the form /proc/PID/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).
>> >> >> >
>> >> >> > This will produce a record such as this:
>> >> >> > type=CONTAINER msg=audit(1519903238.968:261): op=set pid=596 uid=0 
>> >> >> > subj=unconfined_u:unconfined_r:unconfined_t:s0-s0:c0.c1023 auid=0 
>> >> >> > tty=pts0 ses=1 opid=596 old-contid=18446744073709551615 
>> >> >> > contid=123455 res=0
>> >> >> >
>> >> >> > The "op" field indicates an initial set.  The "pid" to "ses" fields 
>> >> >> > are
>> >> >> > the orchestrator while the "opid" field is the object's PID, the 
>> >> >> > process
>> >> >> > being "contained".  Old and new container ID values are given in the
>> >> >> > "contid" fields, while res indicates its success.
>> >> >> >
>> >> >> > It is not permitted to self-set, unset or re-set the container ID.  A
>> >> >> > child inherits its parent's container ID, but then can be set only 
>> >> >> > once
>> >> >> > after.
>> >> >> >
>> >> >> > See: https://github.com/linux-audit/audit-kernel/issues/32
>> >> >> >
>> >> >> > Signed-off-by: Richard Guy Briggs 
>> >> >> > ---
>> >> >> >  fs/proc/base.c | 37 
>> >> >> >  include/linux/audit.h  | 16 +
>> >> >> >  include/linux/init_task.h  |  4 ++-
>> >> >> >  include/linux/sched.h  |  1 +
>> >> >> >  include/uapi/linux/audit.h |  2 ++
>> >> >> >  kernel/auditsc.c   | 84 
>> >> >> > ++
>> >> >> >  6 files changed, 143 insertions(+), 1 deletion(-)
>>
>> ...
>>
>> >> >> >  /* audit_rule_data supports filter rules with both integer and 
>> >> >> > string
>> >> >> >   * fields.  It corresponds with AUDIT_ADD_RULE, AUDIT_DEL_RULE and
>> >> >> > diff --git a/kernel/auditsc.c b/kernel/auditsc.c
>> >> >> > index 4e0a4ac..29c8482 100644
>> >> >> > --- a/kernel/auditsc.c
>> >> >> > +++ b/kernel/auditsc.c
>> >> >> > @@ -2073,6 +2073,90 @@ int audit_set_loginuid(kuid_t loginuid)
>> >> >> > return rc;
>> >> >> >  }
>> >> >> >
>> >> >> > +static int audit_set_containerid_perm(struct task_struct *task, u64 
>> >> >> > containerid)
>> >> >> > +{
>> >> >> > +   struct task_struct *parent;
>> >> >> > +   u64 pcontainerid, ccontainerid;
>> >> >> > +
>> >> >> > +   /* Don't allow to set our own containerid */
>> >> >> > +   if (current == task)
>> >> >> > +   return -EPERM;
>> >> >>
>> >> >> Why not?  Is there some obvious security concern that I missing?
>> >> >
>> >> > We then lose the distinction in the AUDIT_CONTAINER record between the
>> >> > initiating PID and the target PID.  This was outlined in the proposal.
>> >>
>> >> I just went back and reread the v3 proposal and I still don't see a
>> >> good explanation of this.  Why is this bad?  What's the security
>> >> concern?
>> >
>> > I don't remember, specifically.  Maybe this has been addressed by the
>> > check for children/threads or identical parent container ID.  So, I'm
>> > reluctantly willing to remove that check for now.
>>
>> Okay.  For the record, if someone can explain to me why this
>> restriction saves us from some terrible situation I'm all for leaving
>> it.  I'm just opposed to restrictions without solid reasoning behind
>> them.
>>
>> >> > Having said that, I'm still not sure we have protected sufficiently from
>> >> > a child turning around and setting it's parent's as yet unset or
>> >> > inherited audit container ID.
>> >>
>> >> Yes, I believe we only want to let a task set the audit container for
>> >> it's children (or itself/threads if we decide to allow that, see
>> >> above).  There *has* to be a function to check to see if a task if a
>> >> child of a given task ... right? ... although this is likely to be a
>> >> pointer traversal and locking nightmare ... hmmm.
>> >
>> > Isn't that just (struct task_struct)parent == (struct
>> > task_struct)child->parent (or ->real_parent)?
>> >
>> > And now that I say that, it is covered by the following patch's child
>> > check, so as long as we keep that, we should be fine.
>>
>> I was thinking of checking not just 

Re: [RFC PATCH ghak32 V2 01/13] audit: add container id

[Richard Guy Briggs]

On 2018-04-24 15:01, Paul Moore wrote:
> On Mon, Apr 23, 2018 at 10:02 PM, Richard Guy Briggs  wrote:
> > On 2018-04-23 19:15, Paul Moore wrote:
> >> On Sat, Apr 21, 2018 at 10:34 AM, Richard Guy Briggs  
> >> wrote:
> >> > On 2018-04-18 19:47, Paul Moore wrote:
> >> >> On Fri, Mar 16, 2018 at 5:00 AM, Richard Guy Briggs  
> >> >> wrote:
> >> >> > Implement the proc fs write to set the audit container ID of a 
> >> >> > process,
> >> >> > emitting an AUDIT_CONTAINER record to document the event.
> >> >> >
> >> >> > This is a write from the container orchestrator task to a proc entry 
> >> >> > of
> >> >> > the form /proc/PID/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).
> >> >> >
> >> >> > This will produce a record such as this:
> >> >> > type=CONTAINER msg=audit(1519903238.968:261): op=set pid=596 uid=0 
> >> >> > subj=unconfined_u:unconfined_r:unconfined_t:s0-s0:c0.c1023 auid=0 
> >> >> > tty=pts0 ses=1 opid=596 old-contid=18446744073709551615 contid=123455 
> >> >> > res=0
> >> >> >
> >> >> > The "op" field indicates an initial set.  The "pid" to "ses" fields 
> >> >> > are
> >> >> > the orchestrator while the "opid" field is the object's PID, the 
> >> >> > process
> >> >> > being "contained".  Old and new container ID values are given in the
> >> >> > "contid" fields, while res indicates its success.
> >> >> >
> >> >> > It is not permitted to self-set, unset or re-set the container ID.  A
> >> >> > child inherits its parent's container ID, but then can be set only 
> >> >> > once
> >> >> > after.
> >> >> >
> >> >> > See: https://github.com/linux-audit/audit-kernel/issues/32
> >> >> >
> >> >> > Signed-off-by: Richard Guy Briggs 
> >> >> > ---
> >> >> >  fs/proc/base.c | 37 
> >> >> >  include/linux/audit.h  | 16 +
> >> >> >  include/linux/init_task.h  |  4 ++-
> >> >> >  include/linux/sched.h  |  1 +
> >> >> >  include/uapi/linux/audit.h |  2 ++
> >> >> >  kernel/auditsc.c   | 84 
> >> >> > ++
> >> >> >  6 files changed, 143 insertions(+), 1 deletion(-)
> 
> ...
> 
> >> >> >  /* audit_rule_data supports filter rules with both integer and string
> >> >> >   * fields.  It corresponds with AUDIT_ADD_RULE, AUDIT_DEL_RULE and
> >> >> > diff --git a/kernel/auditsc.c b/kernel/auditsc.c
> >> >> > index 4e0a4ac..29c8482 100644
> >> >> > --- a/kernel/auditsc.c
> >> >> > +++ b/kernel/auditsc.c
> >> >> > @@ -2073,6 +2073,90 @@ int audit_set_loginuid(kuid_t loginuid)
> >> >> > return rc;
> >> >> >  }
> >> >> >
> >> >> > +static int audit_set_containerid_perm(struct task_struct *task, u64 
> >> >> > containerid)
> >> >> > +{
> >> >> > +   struct task_struct *parent;
> >> >> > +   u64 pcontainerid, ccontainerid;
> >> >> > +
> >> >> > +   /* Don't allow to set our own containerid */
> >> >> > +   if (current == task)
> >> >> > +   return -EPERM;
> >> >>
> >> >> Why not?  Is there some obvious security concern that I missing?
> >> >
> >> > We then lose the distinction in the AUDIT_CONTAINER record between the
> >> > initiating PID and the target PID.  This was outlined in the proposal.
> >>
> >> I just went back and reread the v3 proposal and I still don't see a
> >> good explanation of this.  Why is this bad?  What's the security
> >> concern?
> >
> > I don't remember, specifically.  Maybe this has been addressed by the
> > check for children/threads or identical parent container ID.  So, I'm
> > reluctantly willing to remove that check for now.
> 
> Okay.  For the record, if someone can explain to me why this
> restriction saves us from some terrible situation I'm all for leaving
> it.  I'm just opposed to restrictions without solid reasoning behind
> them.
> 
> >> > Having said that, I'm still not sure we have protected sufficiently from
> >> > a child turning around and setting it's parent's as yet unset or
> >> > inherited audit container ID.
> >>
> >> Yes, I believe we only want to let a task set the audit container for
> >> it's children (or itself/threads if we decide to allow that, see
> >> above).  There *has* to be a function to check to see if a task if a
> >> child of a given task ... right? ... although this is likely to be a
> >> pointer traversal and locking nightmare ... hmmm.
> >
> > Isn't that just (struct task_struct)parent == (struct
> > task_struct)child->parent (or ->real_parent)?
> >
> > And now that I say that, it is covered by the following patch's child
> > check, so as long as we keep that, we should be fine.
> 
> I was thinking of checking not just current's immediate children, but
> any of it's descendants as I believe that is what we want to limit,
> yes?  I 

Re: [RFC PATCH ghak32 V2 01/13] audit: add container id

[Richard Guy Briggs]

On 2018-04-24 15:01, Paul Moore wrote:
> On Mon, Apr 23, 2018 at 10:02 PM, Richard Guy Briggs  wrote:
> > On 2018-04-23 19:15, Paul Moore wrote:
> >> On Sat, Apr 21, 2018 at 10:34 AM, Richard Guy Briggs  
> >> wrote:
> >> > On 2018-04-18 19:47, Paul Moore wrote:
> >> >> On Fri, Mar 16, 2018 at 5:00 AM, Richard Guy Briggs  
> >> >> wrote:
> >> >> > Implement the proc fs write to set the audit container ID of a 
> >> >> > process,
> >> >> > emitting an AUDIT_CONTAINER record to document the event.
> >> >> >
> >> >> > This is a write from the container orchestrator task to a proc entry 
> >> >> > of
> >> >> > the form /proc/PID/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).
> >> >> >
> >> >> > This will produce a record such as this:
> >> >> > type=CONTAINER msg=audit(1519903238.968:261): op=set pid=596 uid=0 
> >> >> > subj=unconfined_u:unconfined_r:unconfined_t:s0-s0:c0.c1023 auid=0 
> >> >> > tty=pts0 ses=1 opid=596 old-contid=18446744073709551615 contid=123455 
> >> >> > res=0
> >> >> >
> >> >> > The "op" field indicates an initial set.  The "pid" to "ses" fields 
> >> >> > are
> >> >> > the orchestrator while the "opid" field is the object's PID, the 
> >> >> > process
> >> >> > being "contained".  Old and new container ID values are given in the
> >> >> > "contid" fields, while res indicates its success.
> >> >> >
> >> >> > It is not permitted to self-set, unset or re-set the container ID.  A
> >> >> > child inherits its parent's container ID, but then can be set only 
> >> >> > once
> >> >> > after.
> >> >> >
> >> >> > See: https://github.com/linux-audit/audit-kernel/issues/32
> >> >> >
> >> >> > Signed-off-by: Richard Guy Briggs 
> >> >> > ---
> >> >> >  fs/proc/base.c | 37 
> >> >> >  include/linux/audit.h  | 16 +
> >> >> >  include/linux/init_task.h  |  4 ++-
> >> >> >  include/linux/sched.h  |  1 +
> >> >> >  include/uapi/linux/audit.h |  2 ++
> >> >> >  kernel/auditsc.c   | 84 
> >> >> > ++
> >> >> >  6 files changed, 143 insertions(+), 1 deletion(-)
> 
> ...
> 
> >> >> >  /* audit_rule_data supports filter rules with both integer and string
> >> >> >   * fields.  It corresponds with AUDIT_ADD_RULE, AUDIT_DEL_RULE and
> >> >> > diff --git a/kernel/auditsc.c b/kernel/auditsc.c
> >> >> > index 4e0a4ac..29c8482 100644
> >> >> > --- a/kernel/auditsc.c
> >> >> > +++ b/kernel/auditsc.c
> >> >> > @@ -2073,6 +2073,90 @@ int audit_set_loginuid(kuid_t loginuid)
> >> >> > return rc;
> >> >> >  }
> >> >> >
> >> >> > +static int audit_set_containerid_perm(struct task_struct *task, u64 
> >> >> > containerid)
> >> >> > +{
> >> >> > +   struct task_struct *parent;
> >> >> > +   u64 pcontainerid, ccontainerid;
> >> >> > +
> >> >> > +   /* Don't allow to set our own containerid */
> >> >> > +   if (current == task)
> >> >> > +   return -EPERM;
> >> >>
> >> >> Why not?  Is there some obvious security concern that I missing?
> >> >
> >> > We then lose the distinction in the AUDIT_CONTAINER record between the
> >> > initiating PID and the target PID.  This was outlined in the proposal.
> >>
> >> I just went back and reread the v3 proposal and I still don't see a
> >> good explanation of this.  Why is this bad?  What's the security
> >> concern?
> >
> > I don't remember, specifically.  Maybe this has been addressed by the
> > check for children/threads or identical parent container ID.  So, I'm
> > reluctantly willing to remove that check for now.
> 
> Okay.  For the record, if someone can explain to me why this
> restriction saves us from some terrible situation I'm all for leaving
> it.  I'm just opposed to restrictions without solid reasoning behind
> them.
> 
> >> > Having said that, I'm still not sure we have protected sufficiently from
> >> > a child turning around and setting it's parent's as yet unset or
> >> > inherited audit container ID.
> >>
> >> Yes, I believe we only want to let a task set the audit container for
> >> it's children (or itself/threads if we decide to allow that, see
> >> above).  There *has* to be a function to check to see if a task if a
> >> child of a given task ... right? ... although this is likely to be a
> >> pointer traversal and locking nightmare ... hmmm.
> >
> > Isn't that just (struct task_struct)parent == (struct
> > task_struct)child->parent (or ->real_parent)?
> >
> > And now that I say that, it is covered by the following patch's child
> > check, so as long as we keep that, we should be fine.
> 
> I was thinking of checking not just current's immediate children, but
> any of it's descendants as I believe that is what we want to limit,
> yes?  I just worry that it isn't really practical to perform that
> check.

Re: [RFC PATCH ghak32 V2 01/13] audit: add container id

[Paul Moore]

On Mon, Apr 23, 2018 at 10:02 PM, Richard Guy Briggs  wrote:
> On 2018-04-23 19:15, Paul Moore wrote:
>> On Sat, Apr 21, 2018 at 10:34 AM, Richard Guy Briggs  wrote:
>> > On 2018-04-18 19:47, Paul Moore wrote:
>> >> On Fri, Mar 16, 2018 at 5:00 AM, Richard Guy Briggs  
>> >> wrote:
>> >> > Implement the proc fs write to set the audit container ID of a process,
>> >> > emitting an AUDIT_CONTAINER record to document the event.
>> >> >
>> >> > This is a write from the container orchestrator task to a proc entry of
>> >> > the form /proc/PID/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).
>> >> >
>> >> > This will produce a record such as this:
>> >> > type=CONTAINER msg=audit(1519903238.968:261): op=set pid=596 uid=0 
>> >> > subj=unconfined_u:unconfined_r:unconfined_t:s0-s0:c0.c1023 auid=0 
>> >> > tty=pts0 ses=1 opid=596 old-contid=18446744073709551615 contid=123455 
>> >> > res=0
>> >> >
>> >> > The "op" field indicates an initial set.  The "pid" to "ses" fields are
>> >> > the orchestrator while the "opid" field is the object's PID, the process
>> >> > being "contained".  Old and new container ID values are given in the
>> >> > "contid" fields, while res indicates its success.
>> >> >
>> >> > It is not permitted to self-set, unset or re-set the container ID.  A
>> >> > child inherits its parent's container ID, but then can be set only once
>> >> > after.
>> >> >
>> >> > See: https://github.com/linux-audit/audit-kernel/issues/32
>> >> >
>> >> > Signed-off-by: Richard Guy Briggs 
>> >> > ---
>> >> >  fs/proc/base.c | 37 
>> >> >  include/linux/audit.h  | 16 +
>> >> >  include/linux/init_task.h  |  4 ++-
>> >> >  include/linux/sched.h  |  1 +
>> >> >  include/uapi/linux/audit.h |  2 ++
>> >> >  kernel/auditsc.c   | 84 
>> >> > ++
>> >> >  6 files changed, 143 insertions(+), 1 deletion(-)

...

>> >> >  /* audit_rule_data supports filter rules with both integer and string
>> >> >   * fields.  It corresponds with AUDIT_ADD_RULE, AUDIT_DEL_RULE and
>> >> > diff --git a/kernel/auditsc.c b/kernel/auditsc.c
>> >> > index 4e0a4ac..29c8482 100644
>> >> > --- a/kernel/auditsc.c
>> >> > +++ b/kernel/auditsc.c
>> >> > @@ -2073,6 +2073,90 @@ int audit_set_loginuid(kuid_t loginuid)
>> >> > return rc;
>> >> >  }
>> >> >
>> >> > +static int audit_set_containerid_perm(struct task_struct *task, u64 
>> >> > containerid)
>> >> > +{
>> >> > +   struct task_struct *parent;
>> >> > +   u64 pcontainerid, ccontainerid;
>> >> > +
>> >> > +   /* Don't allow to set our own containerid */
>> >> > +   if (current == task)
>> >> > +   return -EPERM;
>> >>
>> >> Why not?  Is there some obvious security concern that I missing?
>> >
>> > We then lose the distinction in the AUDIT_CONTAINER record between the
>> > initiating PID and the target PID.  This was outlined in the proposal.
>>
>> I just went back and reread the v3 proposal and I still don't see a
>> good explanation of this.  Why is this bad?  What's the security
>> concern?
>
> I don't remember, specifically.  Maybe this has been addressed by the
> check for children/threads or identical parent container ID.  So, I'm
> reluctantly willing to remove that check for now.

Okay.  For the record, if someone can explain to me why this
restriction saves us from some terrible situation I'm all for leaving
it.  I'm just opposed to restrictions without solid reasoning behind
them.

>> > Having said that, I'm still not sure we have protected sufficiently from
>> > a child turning around and setting it's parent's as yet unset or
>> > inherited audit container ID.
>>
>> Yes, I believe we only want to let a task set the audit container for
>> it's children (or itself/threads if we decide to allow that, see
>> above).  There *has* to be a function to check to see if a task if a
>> child of a given task ... right? ... although this is likely to be a
>> pointer traversal and locking nightmare ... hmmm.
>
> Isn't that just (struct task_struct)parent == (struct
> task_struct)child->parent (or ->real_parent)?
>
> And now that I say that, it is covered by the following patch's child
> check, so as long as we keep that, we should be fine.

I was thinking of checking not just current's immediate children, but
any of it's descendants as I believe that is what we want to limit,
yes?  I just worry that it isn't really practical to perform that
check.

>> >> I ask because I suppose it might be possible for some container
>> >> runtime to do a fork, setup some of the environment and them exec the
>> >> container (before you answer the obvious "namespaces!" please remember
>> >> we're not trying to 

Re: [RFC PATCH ghak32 V2 01/13] audit: add container id

[Paul Moore]

On Mon, Apr 23, 2018 at 10:02 PM, Richard Guy Briggs  wrote:
> On 2018-04-23 19:15, Paul Moore wrote:
>> On Sat, Apr 21, 2018 at 10:34 AM, Richard Guy Briggs  wrote:
>> > On 2018-04-18 19:47, Paul Moore wrote:
>> >> On Fri, Mar 16, 2018 at 5:00 AM, Richard Guy Briggs  
>> >> wrote:
>> >> > Implement the proc fs write to set the audit container ID of a process,
>> >> > emitting an AUDIT_CONTAINER record to document the event.
>> >> >
>> >> > This is a write from the container orchestrator task to a proc entry of
>> >> > the form /proc/PID/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).
>> >> >
>> >> > This will produce a record such as this:
>> >> > type=CONTAINER msg=audit(1519903238.968:261): op=set pid=596 uid=0 
>> >> > subj=unconfined_u:unconfined_r:unconfined_t:s0-s0:c0.c1023 auid=0 
>> >> > tty=pts0 ses=1 opid=596 old-contid=18446744073709551615 contid=123455 
>> >> > res=0
>> >> >
>> >> > The "op" field indicates an initial set.  The "pid" to "ses" fields are
>> >> > the orchestrator while the "opid" field is the object's PID, the process
>> >> > being "contained".  Old and new container ID values are given in the
>> >> > "contid" fields, while res indicates its success.
>> >> >
>> >> > It is not permitted to self-set, unset or re-set the container ID.  A
>> >> > child inherits its parent's container ID, but then can be set only once
>> >> > after.
>> >> >
>> >> > See: https://github.com/linux-audit/audit-kernel/issues/32
>> >> >
>> >> > Signed-off-by: Richard Guy Briggs 
>> >> > ---
>> >> >  fs/proc/base.c | 37 
>> >> >  include/linux/audit.h  | 16 +
>> >> >  include/linux/init_task.h  |  4 ++-
>> >> >  include/linux/sched.h  |  1 +
>> >> >  include/uapi/linux/audit.h |  2 ++
>> >> >  kernel/auditsc.c   | 84 
>> >> > ++
>> >> >  6 files changed, 143 insertions(+), 1 deletion(-)

...

>> >> >  /* audit_rule_data supports filter rules with both integer and string
>> >> >   * fields.  It corresponds with AUDIT_ADD_RULE, AUDIT_DEL_RULE and
>> >> > diff --git a/kernel/auditsc.c b/kernel/auditsc.c
>> >> > index 4e0a4ac..29c8482 100644
>> >> > --- a/kernel/auditsc.c
>> >> > +++ b/kernel/auditsc.c
>> >> > @@ -2073,6 +2073,90 @@ int audit_set_loginuid(kuid_t loginuid)
>> >> > return rc;
>> >> >  }
>> >> >
>> >> > +static int audit_set_containerid_perm(struct task_struct *task, u64 
>> >> > containerid)
>> >> > +{
>> >> > +   struct task_struct *parent;
>> >> > +   u64 pcontainerid, ccontainerid;
>> >> > +
>> >> > +   /* Don't allow to set our own containerid */
>> >> > +   if (current == task)
>> >> > +   return -EPERM;
>> >>
>> >> Why not?  Is there some obvious security concern that I missing?
>> >
>> > We then lose the distinction in the AUDIT_CONTAINER record between the
>> > initiating PID and the target PID.  This was outlined in the proposal.
>>
>> I just went back and reread the v3 proposal and I still don't see a
>> good explanation of this.  Why is this bad?  What's the security
>> concern?
>
> I don't remember, specifically.  Maybe this has been addressed by the
> check for children/threads or identical parent container ID.  So, I'm
> reluctantly willing to remove that check for now.

Okay.  For the record, if someone can explain to me why this
restriction saves us from some terrible situation I'm all for leaving
it.  I'm just opposed to restrictions without solid reasoning behind
them.

>> > Having said that, I'm still not sure we have protected sufficiently from
>> > a child turning around and setting it's parent's as yet unset or
>> > inherited audit container ID.
>>
>> Yes, I believe we only want to let a task set the audit container for
>> it's children (or itself/threads if we decide to allow that, see
>> above).  There *has* to be a function to check to see if a task if a
>> child of a given task ... right? ... although this is likely to be a
>> pointer traversal and locking nightmare ... hmmm.
>
> Isn't that just (struct task_struct)parent == (struct
> task_struct)child->parent (or ->real_parent)?
>
> And now that I say that, it is covered by the following patch's child
> check, so as long as we keep that, we should be fine.

I was thinking of checking not just current's immediate children, but
any of it's descendants as I believe that is what we want to limit,
yes?  I just worry that it isn't really practical to perform that
check.

>> >> I ask because I suppose it might be possible for some container
>> >> runtime to do a fork, setup some of the environment and them exec the
>> >> container (before you answer the obvious "namespaces!" please remember
>> >> we're not trying to define containers).
>> >
>> > I don't think namespaces have any bearing 

Re: [RFC PATCH ghak32 V2 01/13] audit: add container id

[Richard Guy Briggs]

On 2018-04-23 19:15, Paul Moore wrote:
> On Sat, Apr 21, 2018 at 10:34 AM, Richard Guy Briggs  wrote:
> > On 2018-04-18 19:47, Paul Moore wrote:
> >> On Fri, Mar 16, 2018 at 5:00 AM, Richard Guy Briggs  
> >> wrote:
> >> > Implement the proc fs write to set the audit container ID of a process,
> >> > emitting an AUDIT_CONTAINER record to document the event.
> >> >
> >> > This is a write from the container orchestrator task to a proc entry of
> >> > the form /proc/PID/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).
> >> >
> >> > This will produce a record such as this:
> >> > type=CONTAINER msg=audit(1519903238.968:261): op=set pid=596 uid=0 
> >> > subj=unconfined_u:unconfined_r:unconfined_t:s0-s0:c0.c1023 auid=0 
> >> > tty=pts0 ses=1 opid=596 old-contid=18446744073709551615 contid=123455 
> >> > res=0
> >> >
> >> > The "op" field indicates an initial set.  The "pid" to "ses" fields are
> >> > the orchestrator while the "opid" field is the object's PID, the process
> >> > being "contained".  Old and new container ID values are given in the
> >> > "contid" fields, while res indicates its success.
> >> >
> >> > It is not permitted to self-set, unset or re-set the container ID.  A
> >> > child inherits its parent's container ID, but then can be set only once
> >> > after.
> >> >
> >> > See: https://github.com/linux-audit/audit-kernel/issues/32
> >> >
> >> > Signed-off-by: Richard Guy Briggs 
> >> > ---
> >> >  fs/proc/base.c | 37 
> >> >  include/linux/audit.h  | 16 +
> >> >  include/linux/init_task.h  |  4 ++-
> >> >  include/linux/sched.h  |  1 +
> >> >  include/uapi/linux/audit.h |  2 ++
> >> >  kernel/auditsc.c   | 84 
> >> > ++
> >> >  6 files changed, 143 insertions(+), 1 deletion(-)
> 
> ...
> 
> >> > diff --git a/include/linux/sched.h b/include/linux/sched.h
> >> > index d258826..1b82191 100644
> >> > --- a/include/linux/sched.h
> >> > +++ b/include/linux/sched.h
> >> > @@ -796,6 +796,7 @@ struct task_struct {
> >> >  #ifdef CONFIG_AUDITSYSCALL
> >> > kuid_t  loginuid;
> >> > unsigned intsessionid;
> >> > +   u64 containerid;
> >>
> >> This one line addition to the task_struct scares me the most of
> >> anything in this patchset.  Why?  It's a field named "containerid" in
> >> a perhaps one of the most widely used core kernel structures; the
> >> possibilities for abuse are endless, and it's foolish to think we
> >> would ever be able to adequately police this.
> >
> > Fair enough.
> >
> >> Unfortunately, we can't add the field to audit_context as things
> >> currently stand because we don't always allocate an audit_context,
> >> it's dependent on the system's configuration, and we need to track the
> >> audit container ID for a given process, regardless of the audit
> >> configuration.  Pretty much the same reason why loginuid and sessionid
> >> are located directly in task_struct now.  As I stressed during the
> >> design phase, I really want to keep this as an *audit* container ID
> >> and not a general purpose kernel wide container ID.  If the kernel
> >> ever grows a general purpose container ID token, I'll be the first in
> >> line to convert the audit code, but I don't want audit to be that
> >> general purpose mechanism ... audit is hated enough as-is ;)
> >
> > When would we need an audit container ID when audit is not enabled
> > enough to have an audit_context?
> 
> I'm thinking of the audit_alloc() case where audit_filter_task()
> returns AUDIT_DISABLED.

Ok, so a task could be marked as filtered but its children would still
be auditable and inheriting its parent containerid (as well at its
loginuid and sessionid)...

> I believe this is the same reason why loginuid and sessionid live
> directly in the task_struct and not in the audit_context; they need to
> persist for the lifetime of the task.

Yes, probably.

> > If it is only used for audit, and audit is the only consumer, and audit
> > can only use it when it is enabled, then we can just return success to
> > any write to the proc filehandle, or not even present it.  Nothing will
> > be able to know that value wasn't used.
> >
> > When are loginuid and sessionid used now when audit is not enabled (or
> > should I say, explicitly disabled)?
> 
> See above.  I think that should answer these questions.

Ok.

> >> > diff --git a/include/uapi/linux/audit.h b/include/uapi/linux/audit.h
> >> > index 4e61a9e..921a71f 100644
> >> > --- a/include/uapi/linux/audit.h
> >> > +++ b/include/uapi/linux/audit.h
> >> > @@ -71,6 +71,7 @@
> >> >  #define AUDIT_TTY_SET  1017/* Set TTY auditing status 

Re: [RFC PATCH ghak32 V2 01/13] audit: add container id

[Richard Guy Briggs]

On 2018-04-23 19:15, Paul Moore wrote:
> On Sat, Apr 21, 2018 at 10:34 AM, Richard Guy Briggs  wrote:
> > On 2018-04-18 19:47, Paul Moore wrote:
> >> On Fri, Mar 16, 2018 at 5:00 AM, Richard Guy Briggs  
> >> wrote:
> >> > Implement the proc fs write to set the audit container ID of a process,
> >> > emitting an AUDIT_CONTAINER record to document the event.
> >> >
> >> > This is a write from the container orchestrator task to a proc entry of
> >> > the form /proc/PID/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).
> >> >
> >> > This will produce a record such as this:
> >> > type=CONTAINER msg=audit(1519903238.968:261): op=set pid=596 uid=0 
> >> > subj=unconfined_u:unconfined_r:unconfined_t:s0-s0:c0.c1023 auid=0 
> >> > tty=pts0 ses=1 opid=596 old-contid=18446744073709551615 contid=123455 
> >> > res=0
> >> >
> >> > The "op" field indicates an initial set.  The "pid" to "ses" fields are
> >> > the orchestrator while the "opid" field is the object's PID, the process
> >> > being "contained".  Old and new container ID values are given in the
> >> > "contid" fields, while res indicates its success.
> >> >
> >> > It is not permitted to self-set, unset or re-set the container ID.  A
> >> > child inherits its parent's container ID, but then can be set only once
> >> > after.
> >> >
> >> > See: https://github.com/linux-audit/audit-kernel/issues/32
> >> >
> >> > Signed-off-by: Richard Guy Briggs 
> >> > ---
> >> >  fs/proc/base.c | 37 
> >> >  include/linux/audit.h  | 16 +
> >> >  include/linux/init_task.h  |  4 ++-
> >> >  include/linux/sched.h  |  1 +
> >> >  include/uapi/linux/audit.h |  2 ++
> >> >  kernel/auditsc.c   | 84 
> >> > ++
> >> >  6 files changed, 143 insertions(+), 1 deletion(-)
> 
> ...
> 
> >> > diff --git a/include/linux/sched.h b/include/linux/sched.h
> >> > index d258826..1b82191 100644
> >> > --- a/include/linux/sched.h
> >> > +++ b/include/linux/sched.h
> >> > @@ -796,6 +796,7 @@ struct task_struct {
> >> >  #ifdef CONFIG_AUDITSYSCALL
> >> > kuid_t  loginuid;
> >> > unsigned intsessionid;
> >> > +   u64 containerid;
> >>
> >> This one line addition to the task_struct scares me the most of
> >> anything in this patchset.  Why?  It's a field named "containerid" in
> >> a perhaps one of the most widely used core kernel structures; the
> >> possibilities for abuse are endless, and it's foolish to think we
> >> would ever be able to adequately police this.
> >
> > Fair enough.
> >
> >> Unfortunately, we can't add the field to audit_context as things
> >> currently stand because we don't always allocate an audit_context,
> >> it's dependent on the system's configuration, and we need to track the
> >> audit container ID for a given process, regardless of the audit
> >> configuration.  Pretty much the same reason why loginuid and sessionid
> >> are located directly in task_struct now.  As I stressed during the
> >> design phase, I really want to keep this as an *audit* container ID
> >> and not a general purpose kernel wide container ID.  If the kernel
> >> ever grows a general purpose container ID token, I'll be the first in
> >> line to convert the audit code, but I don't want audit to be that
> >> general purpose mechanism ... audit is hated enough as-is ;)
> >
> > When would we need an audit container ID when audit is not enabled
> > enough to have an audit_context?
> 
> I'm thinking of the audit_alloc() case where audit_filter_task()
> returns AUDIT_DISABLED.

Ok, so a task could be marked as filtered but its children would still
be auditable and inheriting its parent containerid (as well at its
loginuid and sessionid)...

> I believe this is the same reason why loginuid and sessionid live
> directly in the task_struct and not in the audit_context; they need to
> persist for the lifetime of the task.

Yes, probably.

> > If it is only used for audit, and audit is the only consumer, and audit
> > can only use it when it is enabled, then we can just return success to
> > any write to the proc filehandle, or not even present it.  Nothing will
> > be able to know that value wasn't used.
> >
> > When are loginuid and sessionid used now when audit is not enabled (or
> > should I say, explicitly disabled)?
> 
> See above.  I think that should answer these questions.

Ok.

> >> > diff --git a/include/uapi/linux/audit.h b/include/uapi/linux/audit.h
> >> > index 4e61a9e..921a71f 100644
> >> > --- a/include/uapi/linux/audit.h
> >> > +++ b/include/uapi/linux/audit.h
> >> > @@ -71,6 +71,7 @@
> >> >  #define AUDIT_TTY_SET  1017/* Set TTY auditing status */
> >> >  #define AUDIT_SET_FEATURE  1018

Re: [RFC PATCH ghak32 V2 01/13] audit: add container id

[Paul Moore]

On Sat, Apr 21, 2018 at 10:34 AM, Richard Guy Briggs  wrote:
> On 2018-04-18 19:47, Paul Moore wrote:
>> On Fri, Mar 16, 2018 at 5:00 AM, Richard Guy Briggs  wrote:
>> > Implement the proc fs write to set the audit container ID of a process,
>> > emitting an AUDIT_CONTAINER record to document the event.
>> >
>> > This is a write from the container orchestrator task to a proc entry of
>> > the form /proc/PID/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).
>> >
>> > This will produce a record such as this:
>> > type=CONTAINER msg=audit(1519903238.968:261): op=set pid=596 uid=0 
>> > subj=unconfined_u:unconfined_r:unconfined_t:s0-s0:c0.c1023 auid=0 tty=pts0 
>> > ses=1 opid=596 old-contid=18446744073709551615 contid=123455 res=0
>> >
>> > The "op" field indicates an initial set.  The "pid" to "ses" fields are
>> > the orchestrator while the "opid" field is the object's PID, the process
>> > being "contained".  Old and new container ID values are given in the
>> > "contid" fields, while res indicates its success.
>> >
>> > It is not permitted to self-set, unset or re-set the container ID.  A
>> > child inherits its parent's container ID, but then can be set only once
>> > after.
>> >
>> > See: https://github.com/linux-audit/audit-kernel/issues/32
>> >
>> > Signed-off-by: Richard Guy Briggs 
>> > ---
>> >  fs/proc/base.c | 37 
>> >  include/linux/audit.h  | 16 +
>> >  include/linux/init_task.h  |  4 ++-
>> >  include/linux/sched.h  |  1 +
>> >  include/uapi/linux/audit.h |  2 ++
>> >  kernel/auditsc.c   | 84 
>> > ++
>> >  6 files changed, 143 insertions(+), 1 deletion(-)

...

>> > diff --git a/include/linux/sched.h b/include/linux/sched.h
>> > index d258826..1b82191 100644
>> > --- a/include/linux/sched.h
>> > +++ b/include/linux/sched.h
>> > @@ -796,6 +796,7 @@ struct task_struct {
>> >  #ifdef CONFIG_AUDITSYSCALL
>> > kuid_t  loginuid;
>> > unsigned intsessionid;
>> > +   u64 containerid;
>>
>> This one line addition to the task_struct scares me the most of
>> anything in this patchset.  Why?  It's a field named "containerid" in
>> a perhaps one of the most widely used core kernel structures; the
>> possibilities for abuse are endless, and it's foolish to think we
>> would ever be able to adequately police this.
>
> Fair enough.
>
>> Unfortunately, we can't add the field to audit_context as things
>> currently stand because we don't always allocate an audit_context,
>> it's dependent on the system's configuration, and we need to track the
>> audit container ID for a given process, regardless of the audit
>> configuration.  Pretty much the same reason why loginuid and sessionid
>> are located directly in task_struct now.  As I stressed during the
>> design phase, I really want to keep this as an *audit* container ID
>> and not a general purpose kernel wide container ID.  If the kernel
>> ever grows a general purpose container ID token, I'll be the first in
>> line to convert the audit code, but I don't want audit to be that
>> general purpose mechanism ... audit is hated enough as-is ;)
>
> When would we need an audit container ID when audit is not enabled
> enough to have an audit_context?

I'm thinking of the audit_alloc() case where audit_filter_task()
returns AUDIT_DISABLED.

I believe this is the same reason why loginuid and sessionid live
directly in the task_struct and not in the audit_context; they need to
persist for the lifetime of the task.

> If it is only used for audit, and audit is the only consumer, and audit
> can only use it when it is enabled, then we can just return success to
> any write to the proc filehandle, or not even present it.  Nothing will
> be able to know that value wasn't used.
>
> When are loginuid and sessionid used now when audit is not enabled (or
> should I say, explicitly disabled)?

See above.  I think that should answer these questions.

>> > diff --git a/include/uapi/linux/audit.h b/include/uapi/linux/audit.h
>> > index 4e61a9e..921a71f 100644
>> > --- a/include/uapi/linux/audit.h
>> > +++ b/include/uapi/linux/audit.h
>> > @@ -71,6 +71,7 @@
>> >  #define AUDIT_TTY_SET  1017/* Set TTY auditing status */
>> >  #define AUDIT_SET_FEATURE  1018/* Turn an audit feature on or off 
>> > */
>> >  #define AUDIT_GET_FEATURE  1019/* Get which features are enabled 
>> > */
>> > +#define AUDIT_CONTAINER1020/* Define the container id 
>> > and information */
>> >
>> >  #define AUDIT_FIRST_USER_MSG   1100/* Userspace messages mostly 
>> > uninteresting to kernel */
>> >  #define AUDIT_USER_AVC  

Re: [RFC PATCH ghak32 V2 01/13] audit: add container id

[Paul Moore]

On Sat, Apr 21, 2018 at 10:34 AM, Richard Guy Briggs  wrote:
> On 2018-04-18 19:47, Paul Moore wrote:
>> On Fri, Mar 16, 2018 at 5:00 AM, Richard Guy Briggs  wrote:
>> > Implement the proc fs write to set the audit container ID of a process,
>> > emitting an AUDIT_CONTAINER record to document the event.
>> >
>> > This is a write from the container orchestrator task to a proc entry of
>> > the form /proc/PID/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).
>> >
>> > This will produce a record such as this:
>> > type=CONTAINER msg=audit(1519903238.968:261): op=set pid=596 uid=0 
>> > subj=unconfined_u:unconfined_r:unconfined_t:s0-s0:c0.c1023 auid=0 tty=pts0 
>> > ses=1 opid=596 old-contid=18446744073709551615 contid=123455 res=0
>> >
>> > The "op" field indicates an initial set.  The "pid" to "ses" fields are
>> > the orchestrator while the "opid" field is the object's PID, the process
>> > being "contained".  Old and new container ID values are given in the
>> > "contid" fields, while res indicates its success.
>> >
>> > It is not permitted to self-set, unset or re-set the container ID.  A
>> > child inherits its parent's container ID, but then can be set only once
>> > after.
>> >
>> > See: https://github.com/linux-audit/audit-kernel/issues/32
>> >
>> > Signed-off-by: Richard Guy Briggs 
>> > ---
>> >  fs/proc/base.c | 37 
>> >  include/linux/audit.h  | 16 +
>> >  include/linux/init_task.h  |  4 ++-
>> >  include/linux/sched.h  |  1 +
>> >  include/uapi/linux/audit.h |  2 ++
>> >  kernel/auditsc.c   | 84 
>> > ++
>> >  6 files changed, 143 insertions(+), 1 deletion(-)

...

>> > diff --git a/include/linux/sched.h b/include/linux/sched.h
>> > index d258826..1b82191 100644
>> > --- a/include/linux/sched.h
>> > +++ b/include/linux/sched.h
>> > @@ -796,6 +796,7 @@ struct task_struct {
>> >  #ifdef CONFIG_AUDITSYSCALL
>> > kuid_t  loginuid;
>> > unsigned intsessionid;
>> > +   u64 containerid;
>>
>> This one line addition to the task_struct scares me the most of
>> anything in this patchset.  Why?  It's a field named "containerid" in
>> a perhaps one of the most widely used core kernel structures; the
>> possibilities for abuse are endless, and it's foolish to think we
>> would ever be able to adequately police this.
>
> Fair enough.
>
>> Unfortunately, we can't add the field to audit_context as things
>> currently stand because we don't always allocate an audit_context,
>> it's dependent on the system's configuration, and we need to track the
>> audit container ID for a given process, regardless of the audit
>> configuration.  Pretty much the same reason why loginuid and sessionid
>> are located directly in task_struct now.  As I stressed during the
>> design phase, I really want to keep this as an *audit* container ID
>> and not a general purpose kernel wide container ID.  If the kernel
>> ever grows a general purpose container ID token, I'll be the first in
>> line to convert the audit code, but I don't want audit to be that
>> general purpose mechanism ... audit is hated enough as-is ;)
>
> When would we need an audit container ID when audit is not enabled
> enough to have an audit_context?

I'm thinking of the audit_alloc() case where audit_filter_task()
returns AUDIT_DISABLED.

I believe this is the same reason why loginuid and sessionid live
directly in the task_struct and not in the audit_context; they need to
persist for the lifetime of the task.

> If it is only used for audit, and audit is the only consumer, and audit
> can only use it when it is enabled, then we can just return success to
> any write to the proc filehandle, or not even present it.  Nothing will
> be able to know that value wasn't used.
>
> When are loginuid and sessionid used now when audit is not enabled (or
> should I say, explicitly disabled)?

See above.  I think that should answer these questions.

>> > diff --git a/include/uapi/linux/audit.h b/include/uapi/linux/audit.h
>> > index 4e61a9e..921a71f 100644
>> > --- a/include/uapi/linux/audit.h
>> > +++ b/include/uapi/linux/audit.h
>> > @@ -71,6 +71,7 @@
>> >  #define AUDIT_TTY_SET  1017/* Set TTY auditing status */
>> >  #define AUDIT_SET_FEATURE  1018/* Turn an audit feature on or off 
>> > */
>> >  #define AUDIT_GET_FEATURE  1019/* Get which features are enabled 
>> > */
>> > +#define AUDIT_CONTAINER1020/* Define the container id 
>> > and information */
>> >
>> >  #define AUDIT_FIRST_USER_MSG   1100/* Userspace messages mostly 
>> > uninteresting to kernel */
>> >  #define AUDIT_USER_AVC 1107/* We filter this differently */
>> > 

Re: [RFC PATCH ghak32 V2 01/13] audit: add container id

[Richard Guy Briggs]

On 2018-04-18 19:47, Paul Moore wrote:
> On Fri, Mar 16, 2018 at 5:00 AM, Richard Guy Briggs  wrote:
> > Implement the proc fs write to set the audit container ID of a process,
> > emitting an AUDIT_CONTAINER record to document the event.
> >
> > This is a write from the container orchestrator task to a proc entry of
> > the form /proc/PID/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).
> >
> > This will produce a record such as this:
> > type=CONTAINER msg=audit(1519903238.968:261): op=set pid=596 uid=0 
> > subj=unconfined_u:unconfined_r:unconfined_t:s0-s0:c0.c1023 auid=0 tty=pts0 
> > ses=1 opid=596 old-contid=18446744073709551615 contid=123455 res=0
> >
> > The "op" field indicates an initial set.  The "pid" to "ses" fields are
> > the orchestrator while the "opid" field is the object's PID, the process
> > being "contained".  Old and new container ID values are given in the
> > "contid" fields, while res indicates its success.
> >
> > It is not permitted to self-set, unset or re-set the container ID.  A
> > child inherits its parent's container ID, but then can be set only once
> > after.
> >
> > See: https://github.com/linux-audit/audit-kernel/issues/32
> >
> > Signed-off-by: Richard Guy Briggs 
> > ---
> >  fs/proc/base.c | 37 
> >  include/linux/audit.h  | 16 +
> >  include/linux/init_task.h  |  4 ++-
> >  include/linux/sched.h  |  1 +
> >  include/uapi/linux/audit.h |  2 ++
> >  kernel/auditsc.c   | 84 
> > ++
> >  6 files changed, 143 insertions(+), 1 deletion(-)
> >
> > diff --git a/fs/proc/base.c b/fs/proc/base.c
> > index 60316b5..6ce4fbe 100644
> > --- a/fs/proc/base.c
> > +++ b/fs/proc/base.c
> > @@ -1299,6 +1299,41 @@ static ssize_t proc_sessionid_read(struct file * 
> > file, char __user * buf,
> > .read   = proc_sessionid_read,
> > .llseek = generic_file_llseek,
> >  };
> > +
> > +static ssize_t proc_containerid_write(struct file *file, const char __user 
> > *buf,
> > +  size_t count, loff_t *ppos)
> > +{
> > +   struct inode *inode = file_inode(file);
> > +   u64 containerid;
> > +   int rv;
> > +   struct task_struct *task = get_proc_task(inode);
> > +
> > +   if (!task)
> > +   return -ESRCH;
> > +   if (*ppos != 0) {
> > +   /* No partial writes. */
> > +   put_task_struct(task);
> > +   return -EINVAL;
> > +   }
> > +
> > +   rv = kstrtou64_from_user(buf, count, 10, );
> > +   if (rv < 0) {
> > +   put_task_struct(task);
> > +   return rv;
> > +   }
> > +
> > +   rv = audit_set_containerid(task, containerid);
> > +   put_task_struct(task);
> > +   if (rv < 0)
> > +   return rv;
> > +   return count;
> > +}
> > +
> > +static const struct file_operations proc_containerid_operations = {
> > +   .write  = proc_containerid_write,
> > +   .llseek = generic_file_llseek,
> > +};
> > +
> >  #endif
> >
> >  #ifdef CONFIG_FAULT_INJECTION
> > @@ -2961,6 +2996,7 @@ static int proc_pid_patch_state(struct seq_file *m, 
> > struct pid_namespace *ns,
> >  #ifdef CONFIG_AUDITSYSCALL
> > REG("loginuid",   S_IWUSR|S_IRUGO, proc_loginuid_operations),
> > REG("sessionid",  S_IRUGO, proc_sessionid_operations),
> > +   REG("containerid", S_IWUSR, proc_containerid_operations),
> >  #endif
> >  #ifdef CONFIG_FAULT_INJECTION
> > REG("make-it-fail", S_IRUGO|S_IWUSR, proc_fault_inject_operations),
> > @@ -3355,6 +3391,7 @@ static int proc_tid_comm_permission(struct inode 
> > *inode, int mask)
> >  #ifdef CONFIG_AUDITSYSCALL
> > REG("loginuid",  S_IWUSR|S_IRUGO, proc_loginuid_operations),
> > REG("sessionid",  S_IRUGO, proc_sessionid_operations),
> > +   REG("containerid", S_IWUSR, proc_containerid_operations),
> >  #endif
> >  #ifdef CONFIG_FAULT_INJECTION
> > REG("make-it-fail", S_IRUGO|S_IWUSR, proc_fault_inject_operations),
> > diff --git a/include/linux/audit.h b/include/linux/audit.h
> > index af410d9..fe4ba3f 100644
> > --- a/include/linux/audit.h
> > +++ b/include/linux/audit.h
> > @@ -29,6 +29,7 @@
> >
> >  #define AUDIT_INO_UNSET ((unsigned long)-1)
> >  #define AUDIT_DEV_UNSET ((dev_t)-1)
> > +#define INVALID_CID AUDIT_CID_UNSET
> 
> Why can't we just use AUDIT_CID_UNSET?  Is there an important
> distinction?  If so, they shouldn't they have different values?

One was intended as user-facing and the other was intended for kernel
internal.  As you point out, this does not appear to be necessary since
they are both the same type.  This was to mirror loginuid due to UID
namespace practice to seperate 

Re: [RFC PATCH ghak32 V2 01/13] audit: add container id

[Richard Guy Briggs]

On 2018-04-18 19:47, Paul Moore wrote:
> On Fri, Mar 16, 2018 at 5:00 AM, Richard Guy Briggs  wrote:
> > Implement the proc fs write to set the audit container ID of a process,
> > emitting an AUDIT_CONTAINER record to document the event.
> >
> > This is a write from the container orchestrator task to a proc entry of
> > the form /proc/PID/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).
> >
> > This will produce a record such as this:
> > type=CONTAINER msg=audit(1519903238.968:261): op=set pid=596 uid=0 
> > subj=unconfined_u:unconfined_r:unconfined_t:s0-s0:c0.c1023 auid=0 tty=pts0 
> > ses=1 opid=596 old-contid=18446744073709551615 contid=123455 res=0
> >
> > The "op" field indicates an initial set.  The "pid" to "ses" fields are
> > the orchestrator while the "opid" field is the object's PID, the process
> > being "contained".  Old and new container ID values are given in the
> > "contid" fields, while res indicates its success.
> >
> > It is not permitted to self-set, unset or re-set the container ID.  A
> > child inherits its parent's container ID, but then can be set only once
> > after.
> >
> > See: https://github.com/linux-audit/audit-kernel/issues/32
> >
> > Signed-off-by: Richard Guy Briggs 
> > ---
> >  fs/proc/base.c | 37 
> >  include/linux/audit.h  | 16 +
> >  include/linux/init_task.h  |  4 ++-
> >  include/linux/sched.h  |  1 +
> >  include/uapi/linux/audit.h |  2 ++
> >  kernel/auditsc.c   | 84 
> > ++
> >  6 files changed, 143 insertions(+), 1 deletion(-)
> >
> > diff --git a/fs/proc/base.c b/fs/proc/base.c
> > index 60316b5..6ce4fbe 100644
> > --- a/fs/proc/base.c
> > +++ b/fs/proc/base.c
> > @@ -1299,6 +1299,41 @@ static ssize_t proc_sessionid_read(struct file * 
> > file, char __user * buf,
> > .read   = proc_sessionid_read,
> > .llseek = generic_file_llseek,
> >  };
> > +
> > +static ssize_t proc_containerid_write(struct file *file, const char __user 
> > *buf,
> > +  size_t count, loff_t *ppos)
> > +{
> > +   struct inode *inode = file_inode(file);
> > +   u64 containerid;
> > +   int rv;
> > +   struct task_struct *task = get_proc_task(inode);
> > +
> > +   if (!task)
> > +   return -ESRCH;
> > +   if (*ppos != 0) {
> > +   /* No partial writes. */
> > +   put_task_struct(task);
> > +   return -EINVAL;
> > +   }
> > +
> > +   rv = kstrtou64_from_user(buf, count, 10, );
> > +   if (rv < 0) {
> > +   put_task_struct(task);
> > +   return rv;
> > +   }
> > +
> > +   rv = audit_set_containerid(task, containerid);
> > +   put_task_struct(task);
> > +   if (rv < 0)
> > +   return rv;
> > +   return count;
> > +}
> > +
> > +static const struct file_operations proc_containerid_operations = {
> > +   .write  = proc_containerid_write,
> > +   .llseek = generic_file_llseek,
> > +};
> > +
> >  #endif
> >
> >  #ifdef CONFIG_FAULT_INJECTION
> > @@ -2961,6 +2996,7 @@ static int proc_pid_patch_state(struct seq_file *m, 
> > struct pid_namespace *ns,
> >  #ifdef CONFIG_AUDITSYSCALL
> > REG("loginuid",   S_IWUSR|S_IRUGO, proc_loginuid_operations),
> > REG("sessionid",  S_IRUGO, proc_sessionid_operations),
> > +   REG("containerid", S_IWUSR, proc_containerid_operations),
> >  #endif
> >  #ifdef CONFIG_FAULT_INJECTION
> > REG("make-it-fail", S_IRUGO|S_IWUSR, proc_fault_inject_operations),
> > @@ -3355,6 +3391,7 @@ static int proc_tid_comm_permission(struct inode 
> > *inode, int mask)
> >  #ifdef CONFIG_AUDITSYSCALL
> > REG("loginuid",  S_IWUSR|S_IRUGO, proc_loginuid_operations),
> > REG("sessionid",  S_IRUGO, proc_sessionid_operations),
> > +   REG("containerid", S_IWUSR, proc_containerid_operations),
> >  #endif
> >  #ifdef CONFIG_FAULT_INJECTION
> > REG("make-it-fail", S_IRUGO|S_IWUSR, proc_fault_inject_operations),
> > diff --git a/include/linux/audit.h b/include/linux/audit.h
> > index af410d9..fe4ba3f 100644
> > --- a/include/linux/audit.h
> > +++ b/include/linux/audit.h
> > @@ -29,6 +29,7 @@
> >
> >  #define AUDIT_INO_UNSET ((unsigned long)-1)
> >  #define AUDIT_DEV_UNSET ((dev_t)-1)
> > +#define INVALID_CID AUDIT_CID_UNSET
> 
> Why can't we just use AUDIT_CID_UNSET?  Is there an important
> distinction?  If so, they shouldn't they have different values?

One was intended as user-facing and the other was intended for kernel
internal.  As you point out, this does not appear to be necessary since
they are both the same type.  This was to mirror loginuid due to UID
namespace practice to seperate the two to make things very clear 

Re: [RFC PATCH ghak32 V2 01/13] audit: add container id

[Casey Schaufler]

On 4/18/2018 5:46 PM, Paul Moore wrote:
> On Wed, Apr 18, 2018 at 8:41 PM, Casey Schaufler  
> wrote:
>> On 4/18/2018 4:47 PM, Paul Moore wrote:
>>> On Fri, Mar 16, 2018 at 5:00 AM, Richard Guy Briggs  wrote:
 Implement the proc fs write to set the audit container ID of a process,
 emitting an AUDIT_CONTAINER record to document the event.
 ...

 diff --git a/include/linux/sched.h b/include/linux/sched.h
 index d258826..1b82191 100644
 --- a/include/linux/sched.h
 +++ b/include/linux/sched.h
 @@ -796,6 +796,7 @@ struct task_struct {
  #ifdef CONFIG_AUDITSYSCALL
 kuid_t  loginuid;
 unsigned intsessionid;
 +   u64 containerid;
>>> This one line addition to the task_struct scares me the most of
>>> anything in this patchset.  Why?  It's a field named "containerid" in
>>> a perhaps one of the most widely used core kernel structures; the
>>> possibilities for abuse are endless, and it's foolish to think we
>>> would ever be able to adequately police this.
>> If we can get the LSM infrastructure managed task blobs from
>> module stacking in ahead of this we could create a trivial security
>> module to manage this. It's not as if there aren't all sorts of
>> interactions between security modules and the audit system already.
> While yes, there are plenty of interactions between the two, it is
> possible to use audit without the LSMs and I would like to preserve
> that.  

Fair enough.

> Further, I don't want to entangle two very complicated code
> changes or make the audit container ID effort dependent on LSM
> stacking.

Also fair, although the use case for container audit IDs is
already pulling in audit, namespaces (yeah, I know it's not
necessary for a container to use namespaces) security modules
(stacked and/or namespaced), cgroups and who knows what else.

> You're a good salesman Casey, but you're not that good ;)

I have to keep the skills sharpened somehow!

OK, I'll grant that this isn't a great fit.

Re: [RFC PATCH ghak32 V2 01/13] audit: add container id

[Casey Schaufler]

On 4/18/2018 5:46 PM, Paul Moore wrote:
> On Wed, Apr 18, 2018 at 8:41 PM, Casey Schaufler  
> wrote:
>> On 4/18/2018 4:47 PM, Paul Moore wrote:
>>> On Fri, Mar 16, 2018 at 5:00 AM, Richard Guy Briggs  wrote:
 Implement the proc fs write to set the audit container ID of a process,
 emitting an AUDIT_CONTAINER record to document the event.
 ...

 diff --git a/include/linux/sched.h b/include/linux/sched.h
 index d258826..1b82191 100644
 --- a/include/linux/sched.h
 +++ b/include/linux/sched.h
 @@ -796,6 +796,7 @@ struct task_struct {
  #ifdef CONFIG_AUDITSYSCALL
 kuid_t  loginuid;
 unsigned intsessionid;
 +   u64 containerid;
>>> This one line addition to the task_struct scares me the most of
>>> anything in this patchset.  Why?  It's a field named "containerid" in
>>> a perhaps one of the most widely used core kernel structures; the
>>> possibilities for abuse are endless, and it's foolish to think we
>>> would ever be able to adequately police this.
>> If we can get the LSM infrastructure managed task blobs from
>> module stacking in ahead of this we could create a trivial security
>> module to manage this. It's not as if there aren't all sorts of
>> interactions between security modules and the audit system already.
> While yes, there are plenty of interactions between the two, it is
> possible to use audit without the LSMs and I would like to preserve
> that.  

Fair enough.

> Further, I don't want to entangle two very complicated code
> changes or make the audit container ID effort dependent on LSM
> stacking.

Also fair, although the use case for container audit IDs is
already pulling in audit, namespaces (yeah, I know it's not
necessary for a container to use namespaces) security modules
(stacked and/or namespaced), cgroups and who knows what else.

> You're a good salesman Casey, but you're not that good ;)

I have to keep the skills sharpened somehow!

OK, I'll grant that this isn't a great fit.

Re: [RFC PATCH ghak32 V2 01/13] audit: add container id

[Paul Moore]

On Wed, Apr 18, 2018 at 8:41 PM, Casey Schaufler  wrote:
> On 4/18/2018 4:47 PM, Paul Moore wrote:
>> On Fri, Mar 16, 2018 at 5:00 AM, Richard Guy Briggs  wrote:
>>> Implement the proc fs write to set the audit container ID of a process,
>>> emitting an AUDIT_CONTAINER record to document the event.
>>> ...
>>>
>>> diff --git a/include/linux/sched.h b/include/linux/sched.h
>>> index d258826..1b82191 100644
>>> --- a/include/linux/sched.h
>>> +++ b/include/linux/sched.h
>>> @@ -796,6 +796,7 @@ struct task_struct {
>>>  #ifdef CONFIG_AUDITSYSCALL
>>> kuid_t  loginuid;
>>> unsigned intsessionid;
>>> +   u64 containerid;
>> This one line addition to the task_struct scares me the most of
>> anything in this patchset.  Why?  It's a field named "containerid" in
>> a perhaps one of the most widely used core kernel structures; the
>> possibilities for abuse are endless, and it's foolish to think we
>> would ever be able to adequately police this.
>
> If we can get the LSM infrastructure managed task blobs from
> module stacking in ahead of this we could create a trivial security
> module to manage this. It's not as if there aren't all sorts of
> interactions between security modules and the audit system already.

While yes, there are plenty of interactions between the two, it is
possible to use audit without the LSMs and I would like to preserve
that.  Further, I don't want to entangle two very complicated code
changes or make the audit container ID effort dependent on LSM
stacking.

You're a good salesman Casey, but you're not that good ;)

-- 
paul moore
www.paul-moore.com

Re: [RFC PATCH ghak32 V2 01/13] audit: add container id

[Paul Moore]

On Wed, Apr 18, 2018 at 8:41 PM, Casey Schaufler  wrote:
> On 4/18/2018 4:47 PM, Paul Moore wrote:
>> On Fri, Mar 16, 2018 at 5:00 AM, Richard Guy Briggs  wrote:
>>> Implement the proc fs write to set the audit container ID of a process,
>>> emitting an AUDIT_CONTAINER record to document the event.
>>> ...
>>>
>>> diff --git a/include/linux/sched.h b/include/linux/sched.h
>>> index d258826..1b82191 100644
>>> --- a/include/linux/sched.h
>>> +++ b/include/linux/sched.h
>>> @@ -796,6 +796,7 @@ struct task_struct {
>>>  #ifdef CONFIG_AUDITSYSCALL
>>> kuid_t  loginuid;
>>> unsigned intsessionid;
>>> +   u64 containerid;
>> This one line addition to the task_struct scares me the most of
>> anything in this patchset.  Why?  It's a field named "containerid" in
>> a perhaps one of the most widely used core kernel structures; the
>> possibilities for abuse are endless, and it's foolish to think we
>> would ever be able to adequately police this.
>
> If we can get the LSM infrastructure managed task blobs from
> module stacking in ahead of this we could create a trivial security
> module to manage this. It's not as if there aren't all sorts of
> interactions between security modules and the audit system already.

While yes, there are plenty of interactions between the two, it is
possible to use audit without the LSMs and I would like to preserve
that.  Further, I don't want to entangle two very complicated code
changes or make the audit container ID effort dependent on LSM
stacking.

You're a good salesman Casey, but you're not that good ;)

-- 
paul moore
www.paul-moore.com

Re: [RFC PATCH ghak32 V2 01/13] audit: add container id

[Casey Schaufler]

On 4/18/2018 4:47 PM, Paul Moore wrote:
> On Fri, Mar 16, 2018 at 5:00 AM, Richard Guy Briggs  wrote:
>> Implement the proc fs write to set the audit container ID of a process,
>> emitting an AUDIT_CONTAINER record to document the event.
>> ...
>>
>> diff --git a/include/linux/sched.h b/include/linux/sched.h
>> index d258826..1b82191 100644
>> --- a/include/linux/sched.h
>> +++ b/include/linux/sched.h
>> @@ -796,6 +796,7 @@ struct task_struct {
>>  #ifdef CONFIG_AUDITSYSCALL
>> kuid_t  loginuid;
>> unsigned intsessionid;
>> +   u64 containerid;
> This one line addition to the task_struct scares me the most of
> anything in this patchset.  Why?  It's a field named "containerid" in
> a perhaps one of the most widely used core kernel structures; the
> possibilities for abuse are endless, and it's foolish to think we
> would ever be able to adequately police this.

If we can get the LSM infrastructure managed task blobs from 
module stacking in ahead of this we could create a trivial security
module to manage this. It's not as if there aren't all sorts of
interactions between security modules and the audit system already.

Re: [RFC PATCH ghak32 V2 01/13] audit: add container id

[Casey Schaufler]

On 4/18/2018 4:47 PM, Paul Moore wrote:
> On Fri, Mar 16, 2018 at 5:00 AM, Richard Guy Briggs  wrote:
>> Implement the proc fs write to set the audit container ID of a process,
>> emitting an AUDIT_CONTAINER record to document the event.
>> ...
>>
>> diff --git a/include/linux/sched.h b/include/linux/sched.h
>> index d258826..1b82191 100644
>> --- a/include/linux/sched.h
>> +++ b/include/linux/sched.h
>> @@ -796,6 +796,7 @@ struct task_struct {
>>  #ifdef CONFIG_AUDITSYSCALL
>> kuid_t  loginuid;
>> unsigned intsessionid;
>> +   u64 containerid;
> This one line addition to the task_struct scares me the most of
> anything in this patchset.  Why?  It's a field named "containerid" in
> a perhaps one of the most widely used core kernel structures; the
> possibilities for abuse are endless, and it's foolish to think we
> would ever be able to adequately police this.

If we can get the LSM infrastructure managed task blobs from 
module stacking in ahead of this we could create a trivial security
module to manage this. It's not as if there aren't all sorts of
interactions between security modules and the audit system already.

Re: [RFC PATCH ghak32 V2 01/13] audit: add container id

[Paul Moore]

On Fri, Mar 16, 2018 at 5:00 AM, Richard Guy Briggs  wrote:
> Implement the proc fs write to set the audit container ID of a process,
> emitting an AUDIT_CONTAINER record to document the event.
>
> This is a write from the container orchestrator task to a proc entry of
> the form /proc/PID/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).
>
> This will produce a record such as this:
> type=CONTAINER msg=audit(1519903238.968:261): op=set pid=596 uid=0 
> subj=unconfined_u:unconfined_r:unconfined_t:s0-s0:c0.c1023 auid=0 tty=pts0 
> ses=1 opid=596 old-contid=18446744073709551615 contid=123455 res=0
>
> The "op" field indicates an initial set.  The "pid" to "ses" fields are
> the orchestrator while the "opid" field is the object's PID, the process
> being "contained".  Old and new container ID values are given in the
> "contid" fields, while res indicates its success.
>
> It is not permitted to self-set, unset or re-set the container ID.  A
> child inherits its parent's container ID, but then can be set only once
> after.
>
> See: https://github.com/linux-audit/audit-kernel/issues/32
>
> Signed-off-by: Richard Guy Briggs 
> ---
>  fs/proc/base.c | 37 
>  include/linux/audit.h  | 16 +
>  include/linux/init_task.h  |  4 ++-
>  include/linux/sched.h  |  1 +
>  include/uapi/linux/audit.h |  2 ++
>  kernel/auditsc.c   | 84 
> ++
>  6 files changed, 143 insertions(+), 1 deletion(-)
>
> diff --git a/fs/proc/base.c b/fs/proc/base.c
> index 60316b5..6ce4fbe 100644
> --- a/fs/proc/base.c
> +++ b/fs/proc/base.c
> @@ -1299,6 +1299,41 @@ static ssize_t proc_sessionid_read(struct file * file, 
> char __user * buf,
> .read   = proc_sessionid_read,
> .llseek = generic_file_llseek,
>  };
> +
> +static ssize_t proc_containerid_write(struct file *file, const char __user 
> *buf,
> +  size_t count, loff_t *ppos)
> +{
> +   struct inode *inode = file_inode(file);
> +   u64 containerid;
> +   int rv;
> +   struct task_struct *task = get_proc_task(inode);
> +
> +   if (!task)
> +   return -ESRCH;
> +   if (*ppos != 0) {
> +   /* No partial writes. */
> +   put_task_struct(task);
> +   return -EINVAL;
> +   }
> +
> +   rv = kstrtou64_from_user(buf, count, 10, );
> +   if (rv < 0) {
> +   put_task_struct(task);
> +   return rv;
> +   }
> +
> +   rv = audit_set_containerid(task, containerid);
> +   put_task_struct(task);
> +   if (rv < 0)
> +   return rv;
> +   return count;
> +}
> +
> +static const struct file_operations proc_containerid_operations = {
> +   .write  = proc_containerid_write,
> +   .llseek = generic_file_llseek,
> +};
> +
>  #endif
>
>  #ifdef CONFIG_FAULT_INJECTION
> @@ -2961,6 +2996,7 @@ static int proc_pid_patch_state(struct seq_file *m, 
> struct pid_namespace *ns,
>  #ifdef CONFIG_AUDITSYSCALL
> REG("loginuid",   S_IWUSR|S_IRUGO, proc_loginuid_operations),
> REG("sessionid",  S_IRUGO, proc_sessionid_operations),
> +   REG("containerid", S_IWUSR, proc_containerid_operations),
>  #endif
>  #ifdef CONFIG_FAULT_INJECTION
> REG("make-it-fail", S_IRUGO|S_IWUSR, proc_fault_inject_operations),
> @@ -3355,6 +3391,7 @@ static int proc_tid_comm_permission(struct inode 
> *inode, int mask)
>  #ifdef CONFIG_AUDITSYSCALL
> REG("loginuid",  S_IWUSR|S_IRUGO, proc_loginuid_operations),
> REG("sessionid",  S_IRUGO, proc_sessionid_operations),
> +   REG("containerid", S_IWUSR, proc_containerid_operations),
>  #endif
>  #ifdef CONFIG_FAULT_INJECTION
> REG("make-it-fail", S_IRUGO|S_IWUSR, proc_fault_inject_operations),
> diff --git a/include/linux/audit.h b/include/linux/audit.h
> index af410d9..fe4ba3f 100644
> --- a/include/linux/audit.h
> +++ b/include/linux/audit.h
> @@ -29,6 +29,7 @@
>
>  #define AUDIT_INO_UNSET ((unsigned long)-1)
>  #define AUDIT_DEV_UNSET ((dev_t)-1)
> +#define INVALID_CID AUDIT_CID_UNSET

Why can't we just use AUDIT_CID_UNSET?  Is there an important
distinction?  If so, they shouldn't they have different values?

If we do need to keep INVALID_CID, let's rename it to
AUDIT_CID_INVALID so we have some consistency to the naming patterns
and we stress that it is an *audit* container ID.

> diff --git a/include/linux/sched.h b/include/linux/sched.h
> index d258826..1b82191 100644
> --- a/include/linux/sched.h
> +++ b/include/linux/sched.h
> @@ -796,6 +796,7 @@ struct task_struct {
>  #ifdef CONFIG_AUDITSYSCALL
> kuid_t  loginuid;
> unsigned intsessionid;
> +   

Re: [RFC PATCH ghak32 V2 01/13] audit: add container id

[Paul Moore]

On Fri, Mar 16, 2018 at 5:00 AM, Richard Guy Briggs  wrote:
> Implement the proc fs write to set the audit container ID of a process,
> emitting an AUDIT_CONTAINER record to document the event.
>
> This is a write from the container orchestrator task to a proc entry of
> the form /proc/PID/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).
>
> This will produce a record such as this:
> type=CONTAINER msg=audit(1519903238.968:261): op=set pid=596 uid=0 
> subj=unconfined_u:unconfined_r:unconfined_t:s0-s0:c0.c1023 auid=0 tty=pts0 
> ses=1 opid=596 old-contid=18446744073709551615 contid=123455 res=0
>
> The "op" field indicates an initial set.  The "pid" to "ses" fields are
> the orchestrator while the "opid" field is the object's PID, the process
> being "contained".  Old and new container ID values are given in the
> "contid" fields, while res indicates its success.
>
> It is not permitted to self-set, unset or re-set the container ID.  A
> child inherits its parent's container ID, but then can be set only once
> after.
>
> See: https://github.com/linux-audit/audit-kernel/issues/32
>
> Signed-off-by: Richard Guy Briggs 
> ---
>  fs/proc/base.c | 37 
>  include/linux/audit.h  | 16 +
>  include/linux/init_task.h  |  4 ++-
>  include/linux/sched.h  |  1 +
>  include/uapi/linux/audit.h |  2 ++
>  kernel/auditsc.c   | 84 
> ++
>  6 files changed, 143 insertions(+), 1 deletion(-)
>
> diff --git a/fs/proc/base.c b/fs/proc/base.c
> index 60316b5..6ce4fbe 100644
> --- a/fs/proc/base.c
> +++ b/fs/proc/base.c
> @@ -1299,6 +1299,41 @@ static ssize_t proc_sessionid_read(struct file * file, 
> char __user * buf,
> .read   = proc_sessionid_read,
> .llseek = generic_file_llseek,
>  };
> +
> +static ssize_t proc_containerid_write(struct file *file, const char __user 
> *buf,
> +  size_t count, loff_t *ppos)
> +{
> +   struct inode *inode = file_inode(file);
> +   u64 containerid;
> +   int rv;
> +   struct task_struct *task = get_proc_task(inode);
> +
> +   if (!task)
> +   return -ESRCH;
> +   if (*ppos != 0) {
> +   /* No partial writes. */
> +   put_task_struct(task);
> +   return -EINVAL;
> +   }
> +
> +   rv = kstrtou64_from_user(buf, count, 10, );
> +   if (rv < 0) {
> +   put_task_struct(task);
> +   return rv;
> +   }
> +
> +   rv = audit_set_containerid(task, containerid);
> +   put_task_struct(task);
> +   if (rv < 0)
> +   return rv;
> +   return count;
> +}
> +
> +static const struct file_operations proc_containerid_operations = {
> +   .write  = proc_containerid_write,
> +   .llseek = generic_file_llseek,
> +};
> +
>  #endif
>
>  #ifdef CONFIG_FAULT_INJECTION
> @@ -2961,6 +2996,7 @@ static int proc_pid_patch_state(struct seq_file *m, 
> struct pid_namespace *ns,
>  #ifdef CONFIG_AUDITSYSCALL
> REG("loginuid",   S_IWUSR|S_IRUGO, proc_loginuid_operations),
> REG("sessionid",  S_IRUGO, proc_sessionid_operations),
> +   REG("containerid", S_IWUSR, proc_containerid_operations),
>  #endif
>  #ifdef CONFIG_FAULT_INJECTION
> REG("make-it-fail", S_IRUGO|S_IWUSR, proc_fault_inject_operations),
> @@ -3355,6 +3391,7 @@ static int proc_tid_comm_permission(struct inode 
> *inode, int mask)
>  #ifdef CONFIG_AUDITSYSCALL
> REG("loginuid",  S_IWUSR|S_IRUGO, proc_loginuid_operations),
> REG("sessionid",  S_IRUGO, proc_sessionid_operations),
> +   REG("containerid", S_IWUSR, proc_containerid_operations),
>  #endif
>  #ifdef CONFIG_FAULT_INJECTION
> REG("make-it-fail", S_IRUGO|S_IWUSR, proc_fault_inject_operations),
> diff --git a/include/linux/audit.h b/include/linux/audit.h
> index af410d9..fe4ba3f 100644
> --- a/include/linux/audit.h
> +++ b/include/linux/audit.h
> @@ -29,6 +29,7 @@
>
>  #define AUDIT_INO_UNSET ((unsigned long)-1)
>  #define AUDIT_DEV_UNSET ((dev_t)-1)
> +#define INVALID_CID AUDIT_CID_UNSET

Why can't we just use AUDIT_CID_UNSET?  Is there an important
distinction?  If so, they shouldn't they have different values?

If we do need to keep INVALID_CID, let's rename it to
AUDIT_CID_INVALID so we have some consistency to the naming patterns
and we stress that it is an *audit* container ID.

> diff --git a/include/linux/sched.h b/include/linux/sched.h
> index d258826..1b82191 100644
> --- a/include/linux/sched.h
> +++ b/include/linux/sched.h
> @@ -796,6 +796,7 @@ struct task_struct {
>  #ifdef CONFIG_AUDITSYSCALL
> kuid_t  loginuid;
> unsigned intsessionid;
> +   u64 

Re: [RFC PATCH ghak32 V2 01/13] audit: add container id

[Richard Guy Briggs]

On 2018-03-29 07:03, Jonathan Corbet wrote:
> On Thu, 29 Mar 2018 05:01:32 -0400
> Richard Guy Briggs  wrote:
> 
> > > A little detail, but still...  
> > 
> > I am understanding that you would prefer more context (as opposed to
> > operational detail) in the description, laying out the use case for this
> > patch(set)?
> 
> No, sorry, "a little detail" was referring to my comment.  The use case,
> I believe, has been well described.

Ah!  "A minor nit".  :-)

> jon

- RGB

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

Re: [RFC PATCH ghak32 V2 01/13] audit: add container id

[Richard Guy Briggs]

On 2018-03-29 07:03, Jonathan Corbet wrote:
> On Thu, 29 Mar 2018 05:01:32 -0400
> Richard Guy Briggs  wrote:
> 
> > > A little detail, but still...  
> > 
> > I am understanding that you would prefer more context (as opposed to
> > operational detail) in the description, laying out the use case for this
> > patch(set)?
> 
> No, sorry, "a little detail" was referring to my comment.  The use case,
> I believe, has been well described.

Ah!  "A minor nit".  :-)

> jon

- RGB

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

Re: [RFC PATCH ghak32 V2 01/13] audit: add container id

[Jonathan Corbet]

On Thu, 29 Mar 2018 05:01:32 -0400
Richard Guy Briggs  wrote:

> > A little detail, but still...  
> 
> I am understanding that you would prefer more context (as opposed to
> operational detail) in the description, laying out the use case for this
> patch(set)?

No, sorry, "a little detail" was referring to my comment.  The use case,
I believe, has been well described.

Thanks,

jon

Re: [RFC PATCH ghak32 V2 01/13] audit: add container id

[Jonathan Corbet]

On Thu, 29 Mar 2018 05:01:32 -0400
Richard Guy Briggs  wrote:

> > A little detail, but still...  
> 
> I am understanding that you would prefer more context (as opposed to
> operational detail) in the description, laying out the use case for this
> patch(set)?

No, sorry, "a little detail" was referring to my comment.  The use case,
I believe, has been well described.

Thanks,

jon

Re: [RFC PATCH ghak32 V2 01/13] audit: add container id

[Richard Guy Briggs]

On 2018-03-28 12:39, Jonathan Corbet wrote:
> On Fri, 16 Mar 2018 05:00:28 -0400
> Richard Guy Briggs  wrote:
> > Implement the proc fs write to set the audit container ID of a process,
> > emitting an AUDIT_CONTAINER record to document the event.
> 
> A little detail, but still...

I am understanding that you would prefer more context (as opposed to
operational detail) in the description, laying out the use case for this
patch(set)?

> > +static int audit_set_containerid_perm(struct task_struct *task, u64 
> > containerid)
> > +{
> > +   struct task_struct *parent;
> > +   u64 pcontainerid, ccontainerid;
> > +
> > +   /* Don't allow to set our own containerid */
> > +   if (current == task)
> > +   return -EPERM;
> > +   /* Don't allow the containerid to be unset */
> > +   if (!cid_valid(containerid))
> > +   return -EINVAL;
> 
> I went looking for cid_valid(), but it turns out you don't add it until
> patch 5.  That, I expect, will not be good for bisectability (or patch
> review).

Nice catch, thanks Jon.  That is very likely another victim of a git
rebase to re-order afterthoughts in the right place.  I'll need to be
more careful of that class of bug, rethink my workflow, or script builds
to verify each commit is compilable.

> Thanks,
> 
> jon

- RGB

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

Re: [RFC PATCH ghak32 V2 01/13] audit: add container id

[Richard Guy Briggs]

On 2018-03-28 12:39, Jonathan Corbet wrote:
> On Fri, 16 Mar 2018 05:00:28 -0400
> Richard Guy Briggs  wrote:
> > Implement the proc fs write to set the audit container ID of a process,
> > emitting an AUDIT_CONTAINER record to document the event.
> 
> A little detail, but still...

I am understanding that you would prefer more context (as opposed to
operational detail) in the description, laying out the use case for this
patch(set)?

> > +static int audit_set_containerid_perm(struct task_struct *task, u64 
> > containerid)
> > +{
> > +   struct task_struct *parent;
> > +   u64 pcontainerid, ccontainerid;
> > +
> > +   /* Don't allow to set our own containerid */
> > +   if (current == task)
> > +   return -EPERM;
> > +   /* Don't allow the containerid to be unset */
> > +   if (!cid_valid(containerid))
> > +   return -EINVAL;
> 
> I went looking for cid_valid(), but it turns out you don't add it until
> patch 5.  That, I expect, will not be good for bisectability (or patch
> review).

Nice catch, thanks Jon.  That is very likely another victim of a git
rebase to re-order afterthoughts in the right place.  I'll need to be
more careful of that class of bug, rethink my workflow, or script builds
to verify each commit is compilable.

> Thanks,
> 
> jon

- RGB

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

Re: [RFC PATCH ghak32 V2 01/13] audit: add container id

[Jonathan Corbet]

On Fri, 16 Mar 2018 05:00:28 -0400
Richard Guy Briggs  wrote:

> Implement the proc fs write to set the audit container ID of a process,
> emitting an AUDIT_CONTAINER record to document the event.

A little detail, but still...

> +static int audit_set_containerid_perm(struct task_struct *task, u64 
> containerid)
> +{
> + struct task_struct *parent;
> + u64 pcontainerid, ccontainerid;
> +
> + /* Don't allow to set our own containerid */
> + if (current == task)
> + return -EPERM;
> + /* Don't allow the containerid to be unset */
> + if (!cid_valid(containerid))
> + return -EINVAL;

I went looking for cid_valid(), but it turns out you don't add it until
patch 5.  That, I expect, will not be good for bisectability (or patch
review).

Thanks,

jon

Re: [RFC PATCH ghak32 V2 01/13] audit: add container id

[Jonathan Corbet]

On Fri, 16 Mar 2018 05:00:28 -0400
Richard Guy Briggs  wrote:

> Implement the proc fs write to set the audit container ID of a process,
> emitting an AUDIT_CONTAINER record to document the event.

A little detail, but still...

> +static int audit_set_containerid_perm(struct task_struct *task, u64 
> containerid)
> +{
> + struct task_struct *parent;
> + u64 pcontainerid, ccontainerid;
> +
> + /* Don't allow to set our own containerid */
> + if (current == task)
> + return -EPERM;
> + /* Don't allow the containerid to be unset */
> + if (!cid_valid(containerid))
> + return -EINVAL;

I went looking for cid_valid(), but it turns out you don't add it until
patch 5.  That, I expect, will not be good for bisectability (or patch
review).

Thanks,

jon