Re: [PATCH ghak90 V9 06/13] audit: add contid support for signalling the audit daemon

Re: [PATCH ghak90 V9 06/13] audit: add contid support for signalling the audit daemon

[Richard Guy Briggs]

On 2020-08-21 14:48, Paul Moore wrote:
> On Wed, Jul 29, 2020 at 3:00 PM Richard Guy Briggs <rgb@redhat.com> wrote:
> > On 2020-07-05 11:10, Paul Moore wrote:
> > > On Sat, Jun 27, 2020 at 9:22 AM Richard Guy Briggs <rgb@redhat.com> wrote:
> > > >
> > > > Add audit container identifier support to the action of signalling the
> > > > audit daemon.
> > > >
> > > > Since this would need to add an element to the audit_sig_info struct,
> > > > a new record type AUDIT_SIGNAL_INFO2 was created with a new
> > > > audit_sig_info2 struct.  Corresponding support is required in the
> > > > userspace code to reflect the new record request and reply type.
> > > > An older userspace won't break since it won't know to request this
> > > > record type.
> > > >
> > > > Signed-off-by: Richard Guy Briggs <rgb@redhat.com>
> > > > ---
> > > >  include/linux/audit.h       |  8 ++++
> > > >  include/uapi/linux/audit.h  |  1 +
> > > >  kernel/audit.c              | 95 ++++++++++++++++++++++++++++++++++++++++++++-
> > > >  security/selinux/nlmsgtab.c |  1 +
> > > >  4 files changed, 104 insertions(+), 1 deletion(-)
> > > >
> > > > diff --git a/include/linux/audit.h b/include/linux/audit.h
> > > > index 5eeba0efffc2..89cf7c66abe6 100644
> > > > --- a/include/linux/audit.h
> > > > +++ b/include/linux/audit.h
> > > > @@ -22,6 +22,13 @@ struct audit_sig_info {
> > > >         char            ctx[];
> > > >  };
> > > >
> > > > +struct audit_sig_info2 {
> > > > +       uid_t           uid;
> > > > +       pid_t           pid;
> > > > +       u32             cid_len;
> > > > +       char            data[];
> > > > +};
> > > > +
> > > >  struct audit_buffer;
> > > >  struct audit_context;
> > > >  struct inode;
> > > > @@ -105,6 +112,7 @@ struct audit_contobj {
> > > >         u64                     id;
> > > >         struct task_struct      *owner;
> > > >         refcount_t              refcount;
> > > > +       refcount_t              sigflag;
> > > >         struct rcu_head         rcu;
> > > >  };
> > >
> > > It seems like we need some protection in audit_set_contid() so that we
> > > don't allow reuse of an audit container ID when "refcount == 0 &&
> > > sigflag != 0", yes?
> >
> > We have it, see -ESHUTDOWN below.
> 
> That check in audit_set_contid() is checking ->refcount and not
> ->sigflag; ->sigflag is more important in this context, yes?

That contobj isn't findable until it is in the list with a positive
refcount.  If that contobj still exists and the refcount is zero, refuse
to increment it since it is dead.  The only reason it still exists is
the sigflag must be non-zero due to the signal not having been collected
yet or rcu hasn't released it yet after auditd exitted.

> > > > diff --git a/include/uapi/linux/audit.h b/include/uapi/linux/audit.h
> > > > index fd98460c983f..a56ad77069b9 100644
> > > > --- a/include/uapi/linux/audit.h
> > > > +++ b/include/uapi/linux/audit.h
> > > > @@ -72,6 +72,7 @@
> > > >  #define AUDIT_SET_FEATURE      1018    /* Turn an audit feature on or off */
> > > >  #define AUDIT_GET_FEATURE      1019    /* Get which features are enabled */
> > > >  #define AUDIT_CONTAINER_OP     1020    /* Define the container id and info */
> > > > +#define AUDIT_SIGNAL_INFO2     1021    /* Get info auditd signal sender */
> > > >
> > > >  #define AUDIT_FIRST_USER_MSG   1100    /* Userspace messages mostly uninteresting to kernel */
> > > >  #define AUDIT_USER_AVC         1107    /* We filter this differently */
> > > > diff --git a/kernel/audit.c b/kernel/audit.c
> > > > index a09f8f661234..54dd2cb69402 100644
> > > > --- a/kernel/audit.c
> > > > +++ b/kernel/audit.c
> > > > @@ -126,6 +126,8 @@ struct auditd_connection {
> > > >  kuid_t         audit_sig_uid = INVALID_UID;
> > > >  pid_t          audit_sig_pid = -1;
> > > >  u32            audit_sig_sid = 0;
> > > > +static struct audit_contobj *audit_sig_cid;
> > > > +static struct task_struct *audit_sig_atsk;
> > >
> > > This looks like a typo, or did you mean "atsk" for some reason?
> >
> > No, I meant atsk to refer specifically to the audit daemon task and not
> > any other random one that is doing the signalling.  I can change it is
> > there is a strong objection.
> 
> Esh, yeah, "atsk" looks too much like a typo ;)  At the very leask add
> a 'd' in there, e.g. "adtsk", but something better than that would be
> welcome.

Done.  I don't have a strong opinion.

> > > > @@ -2532,6 +2620,11 @@ int audit_set_contid(struct task_struct *task, u64 contid)
> > > >                         if (cont->id == contid) {
> > > >                                 /* task injection to existing container */
> > > >                                 if (current == cont->owner) {
> > > > +                                       if (!refcount_read(&cont->refcount)) {
> > > > +                                               rc = -ESHUTDOWN;
> > >
> > > Reuse -ENOTUNIQ; I'm not overly excited about providing a lot of
> > > detail here as these are global system objects.  If you must have a
> > > different errno (and I would prefer you didn't), use something like
> > > -EBUSY.
> >
> > I don't understand the issue of "global system objects" since the only
> > time this error would be issued is if its own contid were being reused
> > but it hadn't cleaned up its own references yet by either issuing an
> > AUDIT_SIGNAL_INFO* request or the targetted audit daemon hadn't cleaned
> > up yet.  EBUSY could be confused with already having spawned threads or
> > children, and ENOTUNIQ could indicate that another orchestrator/engine
> > had stolen its desired contid after we released it and wanted to reuse
> > it.
> 
> All the more reason for ENOTUNIQ.  The point is that the audit
> container ID is not available for use, and since the IDs are shared
> across the entire system I think we are better off having some
> ambiquity here with errnos.

Done.

> > This gets me thinking about making reservations for preferred
> > contids that are otherwise unavailable and making callbacks to indicate
> > when they become available, but that seems undesirably complex right
> > now.
> 
> That is definitely beyond the scope of this work, or rather *should*
> be beyond the scope of this work.

Good.

> paul moore

- RGB

--
Richard Guy Briggs <rgb@redhat.com>
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

_______________________________________________
Containers mailing list
Containers@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/containers

Re: [PATCH ghak90 V9 05/13] audit: log container info of syscalls

[Richard Guy Briggs]

On 2020-08-21 15:15, Paul Moore wrote:
> On Wed, Jul 29, 2020 at 3:41 PM Richard Guy Briggs <rgb@redhat.com> wrote:
> > On 2020-07-05 11:10, Paul Moore wrote:
> > > On Sat, Jun 27, 2020 at 9:22 AM Richard Guy Briggs <rgb@redhat.com> wrote:
> 
> ...
> 
> > > > diff --git a/kernel/auditsc.c b/kernel/auditsc.c
> > > > index f03d3eb0752c..9e79645e5c0e 100644
> > > > --- a/kernel/auditsc.c
> > > > +++ b/kernel/auditsc.c
> > > > @@ -1458,6 +1466,7 @@ static void audit_log_exit(void)
> > > >         struct audit_buffer *ab;
> > > >         struct audit_aux_data *aux;
> > > >         struct audit_names *n;
> > > > +       struct audit_contobj *cont;
> > > >
> > > >         context->personality = current->personality;
> > > >
> > > > @@ -1541,7 +1550,7 @@ static void audit_log_exit(void)
> > > >         for (aux = context->aux_pids; aux; aux = aux->next) {
> > > >                 struct audit_aux_data_pids *axs = (void *)aux;
> > > >
> > > > -               for (i = 0; i < axs->pid_count; i++)
> > > > +               for (i = 0; i < axs->pid_count; i++) {
> > > >                         if (audit_log_pid_context(context, axs->target_pid[i],
> > > >                                                   axs->target_auid[i],
> > > >                                                   axs->target_uid[i],
> > > > @@ -1549,14 +1558,20 @@ static void audit_log_exit(void)
> > > >                                                   axs->target_sid[i],
> > > >                                                   axs->target_comm[i]))
> > > >                                 call_panic = 1;
> > > > +                       audit_log_container_id(context, axs->target_cid[i]);
> > > > +               }
> > >
> > > It might be nice to see an audit event example including the
> > > ptrace/signal information.  I'm concerned there may be some confusion
> > > about associating the different audit container IDs with the correct
> > > information in the event.
> >
> > This is the subject of ghat81, which is a test for ptrace and signal
> > records.
> >
> > This was the reason I had advocated for an op= field since there is a
> > possibility of multiple contid records per event.
> 
> I think an "op=" field is the wrong way to link audit container ID to
> a particular record.  It may be convenient, but I fear that it would
> be overloading the field too much.

Ok, after looking at the field dictionary how about item, rel, ref or rec?
Item perhaps could be added to the OBJ_PID records, but that might be
overloading a field that is already used in PATH records.  "rel" for
relates-to, "ref" for reference to, "rec" for record...  Perhaps pid=
would be enough to tie this record to the OBJ_PID record or the SYSCALL
record, but in the case of network events, the pid may refer to a kernel
thread.

> Like I said above, I think it would be good to see an audit event
> example including the ptrace/signal information.  This way we can talk
> about it on-list and hash out the various solutions if it proves to be
> a problem.

See the list posting from 2020-09-29 "auditing signals" pointing to
ghat81 test case about testing ptrace and signals from 18 months ago.

I think I have a way to generate a signal to multiple targets in one
syscall...  The added challenge is to also give those targets different
audit container identifiers.

> > > > @@ -1575,6 +1590,14 @@ static void audit_log_exit(void)
> > > >
> > > >         audit_log_proctitle();
> > > >
> > > > +       rcu_read_lock();
> > > > +       cont = _audit_contobj_get(current);
> > > > +       rcu_read_unlock();
> > > > +       audit_log_container_id(context, cont);
> > > > +       rcu_read_lock();
> > > > +       _audit_contobj_put(cont);
> > > > +       rcu_read_unlock();
> > >
> > > Do we need to grab an additional reference for the audit container
> > > object here?  We don't create any additional references here that
> > > persist beyond the lifetime of this function, right?
> >
> > Why do we need another reference?  There's one for each pointer pointing
> > to it and so far we have just one from this task.  Or are you thinking
> > of the contid hash list, which is only added to when a task points to it
> > and gets removed from that list when the last task stops pointing to it.
> > Later that gets more complicated with network namespaces and nested
> > container objects.  For now we just needed it while generating the
> > record, then it gets freed.
> 
> I don't think we need to grab an additional reference here, that is
> why I asked the question.  The code above grabs a reference for the
> audit container ID object associated with the current task and then
> drops it before returning; if the current task, and it's associated
> audit container ID object, disappears in the middle of the function
> we've got much bigger worries :)

I misunderstood your question previously thinking you wanted yet another
reference taken in this case, when in fact it was the opposite and you
thought the one taken here was superfluous.

I don't *need* to grab the additional references here, but those are the
accessor functions that exist, so I either create sub-accessor functions
without the refcount manipulations that called from the primary accessor
functions or open code a reduncancy...  The locking has been updated to
protect the _put by a spin-lock.  Now that I look at it, the 4th to 7th
lines could be bypassed by a cont == NULL check.

It is somewhat hidden now since this sequence of 7 commands has been
abstracted into another function that is called from a second location.

> paul moore

- RGB

--
Richard Guy Briggs <rgb@redhat.com>
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

_______________________________________________
Containers mailing list
Containers@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/containers

Re: [PATCH ghak90 V9 05/13] audit: log container info of syscalls

[Richard Guy Briggs]

On 2020-10-02 15:52, Richard Guy Briggs wrote:
> On 2020-08-21 15:15, Paul Moore wrote:
> > On Wed, Jul 29, 2020 at 3:41 PM Richard Guy Briggs <rgb@redhat.com> wrote:
> > > On 2020-07-05 11:10, Paul Moore wrote:
> > > > On Sat, Jun 27, 2020 at 9:22 AM Richard Guy Briggs <rgb@redhat.com> wrote:
> > 
> > ...
> > 
> > > > > diff --git a/kernel/auditsc.c b/kernel/auditsc.c
> > > > > index f03d3eb0752c..9e79645e5c0e 100644
> > > > > --- a/kernel/auditsc.c
> > > > > +++ b/kernel/auditsc.c
> > > > > @@ -1458,6 +1466,7 @@ static void audit_log_exit(void)
> > > > >         struct audit_buffer *ab;
> > > > >         struct audit_aux_data *aux;
> > > > >         struct audit_names *n;
> > > > > +       struct audit_contobj *cont;
> > > > >
> > > > >         context->personality = current->personality;
> > > > >
> > > > > @@ -1541,7 +1550,7 @@ static void audit_log_exit(void)
> > > > >         for (aux = context->aux_pids; aux; aux = aux->next) {
> > > > >                 struct audit_aux_data_pids *axs = (void *)aux;
> > > > >
> > > > > -               for (i = 0; i < axs->pid_count; i++)
> > > > > +               for (i = 0; i < axs->pid_count; i++) {
> > > > >                         if (audit_log_pid_context(context, axs->target_pid[i],
> > > > >                                                   axs->target_auid[i],
> > > > >                                                   axs->target_uid[i],
> > > > > @@ -1549,14 +1558,20 @@ static void audit_log_exit(void)
> > > > >                                                   axs->target_sid[i],
> > > > >                                                   axs->target_comm[i]))
> > > > >                                 call_panic = 1;
> > > > > +                       audit_log_container_id(context, axs->target_cid[i]);
> > > > > +               }
> > > >
> > > > It might be nice to see an audit event example including the
> > > > ptrace/signal information.  I'm concerned there may be some confusion
> > > > about associating the different audit container IDs with the correct
> > > > information in the event.
> > >
> > > This is the subject of ghat81, which is a test for ptrace and signal
> > > records.
> > >
> > > This was the reason I had advocated for an op= field since there is a
> > > possibility of multiple contid records per event.
> > 
> > I think an "op=" field is the wrong way to link audit container ID to
> > a particular record.  It may be convenient, but I fear that it would
> > be overloading the field too much.
> 
> Ok, after looking at the field dictionary how about item, rel, ref or rec?
> Item perhaps could be added to the OBJ_PID records, but that might be
> overloading a field that is already used in PATH records.  "rel" for
> relates-to, "ref" for reference to, "rec" for record...  Perhaps pid=
> would be enough to tie this record to the OBJ_PID record or the SYSCALL
> record, but in the case of network events, the pid may refer to a kernel
> thread.
> 
> > Like I said above, I think it would be good to see an audit event
> > example including the ptrace/signal information.  This way we can talk
> > about it on-list and hash out the various solutions if it proves to be
> > a problem.
> 
> See the list posting from 2020-09-29 "auditing signals" pointing to
> ghat81 test case about testing ptrace and signals from 18 months ago.
> 
> I think I have a way to generate a signal to multiple targets in one
> syscall...  The added challenge is to also give those targets different
> audit container identifiers.

Here is an exmple I was able to generate after updating the testsuite
script to include a signalling example of a nested audit container
identifier:

----
type=PROCTITLE msg=audit(2020-10-21 10:31:16.655:6731) : proctitle=/usr/bin/perl -w containerid/test
type=CONTAINER_ID msg=audit(2020-10-21 10:31:16.655:6731) : contid=7129731255799087104^3333941723245477888
type=OBJ_PID msg=audit(2020-10-21 10:31:16.655:6731) : opid=115583 oauid=root ouid=root oses=1 obj=unconfined_u:unconfined_r:unconfined_t:s0-s0:c0.c1023 ocomm=perl
type=CONTAINER_ID msg=audit(2020-10-21 10:31:16.655:6731) : contid=3333941723245477888
type=OBJ_PID msg=audit(2020-10-21 10:31:16.655:6731) : opid=115580 oauid=root ouid=root oses=1 obj=unconfined_u:unconfined_r:unconfined_t:s0-s0:c0.c1023 ocomm=perl
type=CONTAINER_ID msg=audit(2020-10-21 10:31:16.655:6731) : contid=8098399240850112512^3333941723245477888
type=OBJ_PID msg=audit(2020-10-21 10:31:16.655:6731) : opid=115582 oauid=root ouid=root oses=1 obj=unconfined_u:unconfined_r:unconfined_t:s0-s0:c0.c1023 ocomm=perl
type=SYSCALL msg=audit(2020-10-21 10:31:16.655:6731) : arch=x86_64 syscall=kill success=yes exit=0 a0=0xfffe3c84 a1=SIGTERM a2=0x4d524554 a3=0x0 items=0 ppid=115564 pid=115567 auid=root uid=root gid=root euid=root suid=root fsuid=root egid=root sgid=root fsgid=root tty=ttyS0 ses=1 comm=perl exe=/usr/bin/perl subj=unconfined_u:unconfined_r:unconfined_t:s0-s0:c0.c1023 key=testsuite-1603290671-AcLtUulY                     
----

There are three CONTAINER_ID records which need some way of associating with OBJ_PID records.  An additional CONTAINER_ID record would be present if the killing process itself had an audit container identifier.  I think the most obvious way to connect them is with a pid= field in the CONTAINER_ID record.

> > > > > @@ -1575,6 +1590,14 @@ static void audit_log_exit(void)
> > > > >
> > > > >         audit_log_proctitle();
> > > > >
> > > > > +       rcu_read_lock();
> > > > > +       cont = _audit_contobj_get(current);
> > > > > +       rcu_read_unlock();
> > > > > +       audit_log_container_id(context, cont);
> > > > > +       rcu_read_lock();
> > > > > +       _audit_contobj_put(cont);
> > > > > +       rcu_read_unlock();
> > > >
> > > > Do we need to grab an additional reference for the audit container
> > > > object here?  We don't create any additional references here that
> > > > persist beyond the lifetime of this function, right?
> > >
> > > Why do we need another reference?  There's one for each pointer pointing
> > > to it and so far we have just one from this task.  Or are you thinking
> > > of the contid hash list, which is only added to when a task points to it
> > > and gets removed from that list when the last task stops pointing to it.
> > > Later that gets more complicated with network namespaces and nested
> > > container objects.  For now we just needed it while generating the
> > > record, then it gets freed.
> > 
> > I don't think we need to grab an additional reference here, that is
> > why I asked the question.  The code above grabs a reference for the
> > audit container ID object associated with the current task and then
> > drops it before returning; if the current task, and it's associated
> > audit container ID object, disappears in the middle of the function
> > we've got much bigger worries :)
> 
> I misunderstood your question previously thinking you wanted yet another
> reference taken in this case, when in fact it was the opposite and you
> thought the one taken here was superfluous.
> 
> I don't *need* to grab the additional references here, but those are the
> accessor functions that exist, so I either create sub-accessor functions
> without the refcount manipulations that called from the primary accessor
> functions or open code a reduncancy...  The locking has been updated to
> protect the _put by a spin-lock.  Now that I look at it, the 4th to 7th
> lines could be bypassed by a cont == NULL check.
> 
> It is somewhat hidden now since this sequence of 7 commands has been
> abstracted into another function that is called from a second location.
> 
> > paul moore
> 
> - RGB

- RGB

--
Richard Guy Briggs <rgb@redhat.com>
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

_______________________________________________
Containers mailing list
Containers@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/containers

Re: [PATCH ghak90 V9 05/13] audit: log container info of syscalls

[Steve Grubb]

On Wednesday, October 21, 2020 12:39:26 PM EDT Richard Guy Briggs wrote:
> > I think I have a way to generate a signal to multiple targets in one
> > syscall...  The added challenge is to also give those targets different
> > audit container identifiers.
> 
> Here is an exmple I was able to generate after updating the testsuite
> script to include a signalling example of a nested audit container
> identifier:
> 
> ----
> type=PROCTITLE msg=audit(2020-10-21 10:31:16.655:6731) :
> proctitle=/usr/bin/perl -w containerid/test type=CONTAINER_ID
> msg=audit(2020-10-21 10:31:16.655:6731) :
> contid=7129731255799087104^3333941723245477888 type=OBJ_PID
> msg=audit(2020-10-21 10:31:16.655:6731) : opid=115583 oauid=root ouid=root
> oses=1 obj=unconfined_u:unconfined_r:unconfined_t:s0-s0:c0.c1023
> ocomm=perl type=CONTAINER_ID msg=audit(2020-10-21 10:31:16.655:6731) :
> contid=3333941723245477888 type=OBJ_PID msg=audit(2020-10-21
> 10:31:16.655:6731) : opid=115580 oauid=root ouid=root oses=1
> obj=unconfined_u:unconfined_r:unconfined_t:s0-s0:c0.c1023 ocomm=perl
> type=CONTAINER_ID msg=audit(2020-10-21 10:31:16.655:6731) :
> contid=8098399240850112512^3333941723245477888 type=OBJ_PID
> msg=audit(2020-10-21 10:31:16.655:6731) : opid=115582 oauid=root ouid=root
> oses=1 obj=unconfined_u:unconfined_r:unconfined_t:s0-s0:c0.c1023
> ocomm=perl type=SYSCALL msg=audit(2020-10-21 10:31:16.655:6731) :
> arch=x86_64 syscall=kill success=yes exit=0 a0=0xfffe3c84 a1=SIGTERM
> a2=0x4d524554 a3=0x0 items=0 ppid=115564 pid=115567 auid=root uid=root
> gid=root euid=root suid=root fsuid=root egid=root sgid=root fsgid=root
> tty=ttyS0 ses=1 comm=perl exe=/usr/bin/perl
> subj=unconfined_u:unconfined_r:unconfined_t:s0-s0:c0.c1023
> key=testsuite-1603290671-AcLtUulY ----
> 
> There are three CONTAINER_ID records which need some way of associating
> with OBJ_PID records.  An additional CONTAINER_ID record would be present
> if the killing process itself had an audit container identifier.  I think
> the most obvious way to connect them is with a pid= field in the
> CONTAINER_ID record.

pid is the process sending the signal, opid is the process receiving the 
signal. I think you mean opid?

-Steve


_______________________________________________
Containers mailing list
Containers@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/containers

Re: [PATCH ghak90 V9 05/13] audit: log container info of syscalls

[Richard Guy Briggs]

On 2020-10-21 12:49, Steve Grubb wrote:
> On Wednesday, October 21, 2020 12:39:26 PM EDT Richard Guy Briggs wrote:
> > > I think I have a way to generate a signal to multiple targets in one
> > > syscall...  The added challenge is to also give those targets different
> > > audit container identifiers.
> > 
> > Here is an exmple I was able to generate after updating the testsuite
> > script to include a signalling example of a nested audit container
> > identifier:
> > 
> > ----
> > type=PROCTITLE msg=audit(2020-10-21 10:31:16.655:6731) :
> > proctitle=/usr/bin/perl -w containerid/test type=CONTAINER_ID
> > msg=audit(2020-10-21 10:31:16.655:6731) :
> > contid=7129731255799087104^3333941723245477888 type=OBJ_PID
> > msg=audit(2020-10-21 10:31:16.655:6731) : opid=115583 oauid=root ouid=root
> > oses=1 obj=unconfined_u:unconfined_r:unconfined_t:s0-s0:c0.c1023
> > ocomm=perl type=CONTAINER_ID msg=audit(2020-10-21 10:31:16.655:6731) :
> > contid=3333941723245477888 type=OBJ_PID msg=audit(2020-10-21
> > 10:31:16.655:6731) : opid=115580 oauid=root ouid=root oses=1
> > obj=unconfined_u:unconfined_r:unconfined_t:s0-s0:c0.c1023 ocomm=perl
> > type=CONTAINER_ID msg=audit(2020-10-21 10:31:16.655:6731) :
> > contid=8098399240850112512^3333941723245477888 type=OBJ_PID
> > msg=audit(2020-10-21 10:31:16.655:6731) : opid=115582 oauid=root ouid=root
> > oses=1 obj=unconfined_u:unconfined_r:unconfined_t:s0-s0:c0.c1023
> > ocomm=perl type=SYSCALL msg=audit(2020-10-21 10:31:16.655:6731) :
> > arch=x86_64 syscall=kill success=yes exit=0 a0=0xfffe3c84 a1=SIGTERM
> > a2=0x4d524554 a3=0x0 items=0 ppid=115564 pid=115567 auid=root uid=root
> > gid=root euid=root suid=root fsuid=root egid=root sgid=root fsgid=root
> > tty=ttyS0 ses=1 comm=perl exe=/usr/bin/perl
> > subj=unconfined_u:unconfined_r:unconfined_t:s0-s0:c0.c1023
> > key=testsuite-1603290671-AcLtUulY ----
> > 
> > There are three CONTAINER_ID records which need some way of associating
> > with OBJ_PID records.  An additional CONTAINER_ID record would be present
> > if the killing process itself had an audit container identifier.  I think
> > the most obvious way to connect them is with a pid= field in the
> > CONTAINER_ID record.
> 
> pid is the process sending the signal, opid is the process receiving the 
> signal. I think you mean opid?

If the process sending the signal (it has a pid= field) has an audit
container identifier, it will generate a CONTAINER_ID record.  Each
process being signalled (each has an opid= field) that has an audit
container identifier will also generate a CONTAINER_ID record.  The
former will be much more common.  Which do we use in the CONTAINER_ID
record?  Having swinging fields, pid vs opid does not seem like a
reasonable solution.  Do we go back to "ref=pid=..." vs "ref=opid=..."?

> -Steve

- RGB

--
Richard Guy Briggs <rgb@redhat.com>
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

_______________________________________________
Containers mailing list
Containers@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/containers

Re: [PATCH ghak90 V9 05/13] audit: log container info of syscalls

[Paul Moore]

On Wed, Oct 21, 2020 at 12:39 PM Richard Guy Briggs <rgb@redhat.com> wrote:
> Here is an exmple I was able to generate after updating the testsuite
> script to include a signalling example of a nested audit container
> identifier:
>
> ----
> type=PROCTITLE msg=audit(2020-10-21 10:31:16.655:6731) : proctitle=/usr/bin/perl -w containerid/test
> type=CONTAINER_ID msg=audit(2020-10-21 10:31:16.655:6731) : contid=7129731255799087104^3333941723245477888
> type=OBJ_PID msg=audit(2020-10-21 10:31:16.655:6731) : opid=115583 oauid=root ouid=root oses=1 obj=unconfined_u:unconfined_r:unconfined_t:s0-s0:c0.c1023 ocomm=perl
> type=CONTAINER_ID msg=audit(2020-10-21 10:31:16.655:6731) : contid=3333941723245477888
> type=OBJ_PID msg=audit(2020-10-21 10:31:16.655:6731) : opid=115580 oauid=root ouid=root oses=1 obj=unconfined_u:unconfined_r:unconfined_t:s0-s0:c0.c1023 ocomm=perl
> type=CONTAINER_ID msg=audit(2020-10-21 10:31:16.655:6731) : contid=8098399240850112512^3333941723245477888
> type=OBJ_PID msg=audit(2020-10-21 10:31:16.655:6731) : opid=115582 oauid=root ouid=root oses=1 obj=unconfined_u:unconfined_r:unconfined_t:s0-s0:c0.c1023 ocomm=perl
> type=SYSCALL msg=audit(2020-10-21 10:31:16.655:6731) : arch=x86_64 syscall=kill success=yes exit=0 a0=0xfffe3c84 a1=SIGTERM a2=0x4d524554 a3=0x0 items=0 ppid=115564 pid=115567 auid=root uid=root gid=root euid=root suid=root fsuid=root egid=root sgid=root fsgid=root tty=ttyS0 ses=1 comm=perl exe=/usr/bin/perl subj=unconfined_u:unconfined_r:unconfined_t:s0-s0:c0.c1023 key=testsuite-1603290671-AcLtUulY
> ----
>
> There are three CONTAINER_ID records which need some way of associating with OBJ_PID records.  An additional CONTAINER_ID record would be present if the killing process itself had an audit container identifier.  I think the most obvious way to connect them is with a pid= field in the CONTAINER_ID record.

Using a "pid=" field as a way to link CONTAINER_ID records to other
records raises a few questions.  What happens if/when we need to
represent those PIDs in the context of a namespace?  Are we ever going
to need to link to records which don't have a "pid=" field?  I haven't
done the homework to know if either of these are a concern right now,
but I worry that this might become a problem in the future.

The idea of using something like "item=" is interesting.  As you
mention, the "item=" field does present some overlap problems with the
PATH record, but perhaps we can do something similar.  What if we
added a "record=" (or similar, I'm not worried about names at this
point) to each record, reset to 0/1 at the start of each event, and
when we needed to link records somehow we could add a "related=1,..,N"
field.  This would potentially be useful beyond just the audit
container ID work.

-- 
paul moore
www.paul-moore.com
_______________________________________________
Containers mailing list
Containers@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/containers

Re: [PATCH ghak90 V9 05/13] audit: log container info of syscalls

[Richard Guy Briggs]

On 2020-10-22 21:21, Paul Moore wrote:
> On Wed, Oct 21, 2020 at 12:39 PM Richard Guy Briggs <rgb@redhat.com> wrote:
> > Here is an exmple I was able to generate after updating the testsuite
> > script to include a signalling example of a nested audit container
> > identifier:
> >
> > ----
> > type=PROCTITLE msg=audit(2020-10-21 10:31:16.655:6731) : proctitle=/usr/bin/perl -w containerid/test
> > type=CONTAINER_ID msg=audit(2020-10-21 10:31:16.655:6731) : contid=7129731255799087104^3333941723245477888
> > type=OBJ_PID msg=audit(2020-10-21 10:31:16.655:6731) : opid=115583 oauid=root ouid=root oses=1 obj=unconfined_u:unconfined_r:unconfined_t:s0-s0:c0.c1023 ocomm=perl
> > type=CONTAINER_ID msg=audit(2020-10-21 10:31:16.655:6731) : contid=3333941723245477888
> > type=OBJ_PID msg=audit(2020-10-21 10:31:16.655:6731) : opid=115580 oauid=root ouid=root oses=1 obj=unconfined_u:unconfined_r:unconfined_t:s0-s0:c0.c1023 ocomm=perl
> > type=CONTAINER_ID msg=audit(2020-10-21 10:31:16.655:6731) : contid=8098399240850112512^3333941723245477888
> > type=OBJ_PID msg=audit(2020-10-21 10:31:16.655:6731) : opid=115582 oauid=root ouid=root oses=1 obj=unconfined_u:unconfined_r:unconfined_t:s0-s0:c0.c1023 ocomm=perl
> > type=SYSCALL msg=audit(2020-10-21 10:31:16.655:6731) : arch=x86_64 syscall=kill success=yes exit=0 a0=0xfffe3c84 a1=SIGTERM a2=0x4d524554 a3=0x0 items=0 ppid=115564 pid=115567 auid=root uid=root gid=root euid=root suid=root fsuid=root egid=root sgid=root fsgid=root tty=ttyS0 ses=1 comm=perl exe=/usr/bin/perl subj=unconfined_u:unconfined_r:unconfined_t:s0-s0:c0.c1023 key=testsuite-1603290671-AcLtUulY
> > ----
> >
> > There are three CONTAINER_ID records which need some way of associating with OBJ_PID records.  An additional CONTAINER_ID record would be present if the killing process itself had an audit container identifier.  I think the most obvious way to connect them is with a pid= field in the CONTAINER_ID record.
> 
> Using a "pid=" field as a way to link CONTAINER_ID records to other
> records raises a few questions.  What happens if/when we need to
> represent those PIDs in the context of a namespace?  Are we ever going
> to need to link to records which don't have a "pid=" field?  I haven't
> done the homework to know if either of these are a concern right now,
> but I worry that this might become a problem in the future.

Good point about PID namespaces in the future but those accompanying
records will already have to be conditioned for the PID namespace
context that is requesting it, so I don't see this as a showstopper.

I've forgotten about an important one we already hit, which is a network
event that only has a NETFILTER_PKT record, but in that case, there is
no ambiguity since there are no other records associated with that
event.  So the second is already an issue now.  Using
task_tgid_nr(current), in the contid testsuite script network event it
attributed it to ping which caused the event, but we cannot use this
since it wasn't triggered by a syscall and doesn't accurately reflect
the kernel thread that received it.  It could just be set to zero for
network events.

> The idea of using something like "item=" is interesting.  As you
> mention, the "item=" field does present some overlap problems with the
> PATH record, but perhaps we can do something similar.  What if we
> added a "record=" (or similar, I'm not worried about names at this
> point) to each record, reset to 0/1 at the start of each event, and
> when we needed to link records somehow we could add a "related=1,..,N"
> field.  This would potentially be useful beyond just the audit
> container ID work.

Does it make any sense to use the same keyword in each type of record
such as record/records as in PATH/SYSCALL: item/items ?

(I prefer 0-indexed like item=...)

> paul moore

- RGB

--
Richard Guy Briggs <rgb@redhat.com>
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

_______________________________________________
Containers mailing list
Containers@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/containers

Re: [PATCH ghak90 V9 05/13] audit: log container info of syscalls

[Paul Moore]

On Fri, Oct 23, 2020 at 4:40 PM Richard Guy Briggs <rgb@redhat.com> wrote:
> On 2020-10-22 21:21, Paul Moore wrote:
> > On Wed, Oct 21, 2020 at 12:39 PM Richard Guy Briggs <rgb@redhat.com> wrote:
> > > Here is an exmple I was able to generate after updating the testsuite
> > > script to include a signalling example of a nested audit container
> > > identifier:
> > >
> > > ----
> > > type=PROCTITLE msg=audit(2020-10-21 10:31:16.655:6731) : proctitle=/usr/bin/perl -w containerid/test
> > > type=CONTAINER_ID msg=audit(2020-10-21 10:31:16.655:6731) : contid=7129731255799087104^3333941723245477888
> > > type=OBJ_PID msg=audit(2020-10-21 10:31:16.655:6731) : opid=115583 oauid=root ouid=root oses=1 obj=unconfined_u:unconfined_r:unconfined_t:s0-s0:c0.c1023 ocomm=perl
> > > type=CONTAINER_ID msg=audit(2020-10-21 10:31:16.655:6731) : contid=3333941723245477888
> > > type=OBJ_PID msg=audit(2020-10-21 10:31:16.655:6731) : opid=115580 oauid=root ouid=root oses=1 obj=unconfined_u:unconfined_r:unconfined_t:s0-s0:c0.c1023 ocomm=perl
> > > type=CONTAINER_ID msg=audit(2020-10-21 10:31:16.655:6731) : contid=8098399240850112512^3333941723245477888
> > > type=OBJ_PID msg=audit(2020-10-21 10:31:16.655:6731) : opid=115582 oauid=root ouid=root oses=1 obj=unconfined_u:unconfined_r:unconfined_t:s0-s0:c0.c1023 ocomm=perl
> > > type=SYSCALL msg=audit(2020-10-21 10:31:16.655:6731) : arch=x86_64 syscall=kill success=yes exit=0 a0=0xfffe3c84 a1=SIGTERM a2=0x4d524554 a3=0x0 items=0 ppid=115564 pid=115567 auid=root uid=root gid=root euid=root suid=root fsuid=root egid=root sgid=root fsgid=root tty=ttyS0 ses=1 comm=perl exe=/usr/bin/perl subj=unconfined_u:unconfined_r:unconfined_t:s0-s0:c0.c1023 key=testsuite-1603290671-AcLtUulY
> > > ----
> > >
> > > There are three CONTAINER_ID records which need some way of associating with OBJ_PID records.  An additional CONTAINER_ID record would be present if the killing process itself had an audit container identifier.  I think the most obvious way to connect them is with a pid= field in the CONTAINER_ID record.
> >
> > Using a "pid=" field as a way to link CONTAINER_ID records to other
> > records raises a few questions.  What happens if/when we need to
> > represent those PIDs in the context of a namespace?  Are we ever going
> > to need to link to records which don't have a "pid=" field?  I haven't
> > done the homework to know if either of these are a concern right now,
> > but I worry that this might become a problem in the future.
>
> Good point about PID namespaces in the future but those accompanying
> records will already have to be conditioned for the PID namespace
> context that is requesting it, so I don't see this as a showstopper.

Possibly, it just gets very messy.  Doubly so when you start looking
at potentially adjusting for multiple audit daemons.  Thankfully it
doesn't look like using the PID is a good idea for other reasons.

> I've forgotten about an important one we already hit, which is a network
> event that only has a NETFILTER_PKT record, but in that case, there is
> no ambiguity since there are no other records associated with that
> event.  So the second is already an issue now.  Using
> task_tgid_nr(current), in the contid testsuite script network event it
> attributed it to ping which caused the event, but we cannot use this
> since it wasn't triggered by a syscall and doesn't accurately reflect
> the kernel thread that received it.  It could just be set to zero for
> network events.

Possibly.  It just seems like too much hackery to start; that's the
stuff you do once it has been in a kernel release for years and need
to find a workaround that doesn't break everything.  I think we should
aim a bit higher right now.

> > The idea of using something like "item=" is interesting.  As you
> > mention, the "item=" field does present some overlap problems with the
> > PATH record, but perhaps we can do something similar.  What if we
> > added a "record=" (or similar, I'm not worried about names at this
> > point) to each record, reset to 0/1 at the start of each event, and
> > when we needed to link records somehow we could add a "related=1,..,N"
> > field.  This would potentially be useful beyond just the audit
> > container ID work.
>
> Does it make any sense to use the same keyword in each type of record
> such as record/records as in PATH/SYSCALL: item/items ?

That was mentioned above, if you can assure yourself and the rest of
us that it can be safely reused I think that might be okay, but I'm
not convinced that is safe at the moment.  Although I will admit those
are fears are not based on an exhaustive search through the code (or a
determined "think").

> (I prefer 0-indexed like item=...)

I have no preference on where we start the index, but it makes sense
to keep the same index starting point as the PATH records.

-- 
paul moore
www.paul-moore.com
_______________________________________________
Containers mailing list
Containers@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/containers

Re: [PATCH ghak90 V9 11/13] audit: contid check descendancy and nesting

[Richard Guy Briggs]

On 2020-08-21 16:13, Paul Moore wrote:
> On Fri, Aug 7, 2020 at 1:10 PM Richard Guy Briggs <rgb@redhat.com> wrote:
> > On 2020-07-05 11:11, Paul Moore wrote:
> > > On Sat, Jun 27, 2020 at 9:23 AM Richard Guy Briggs <rgb@redhat.com> wrote:
> > > > Require the target task to be a descendant of the container
> > > > orchestrator/engine.
> 
> If you want to get formal about this, you need to define "target" in
> the sentence above.  Target of what?

The target is the task having its audit container identifier modified by
the orchestrator current task.

> FWIW, I read the above to basically mean that a task can only set the
> audit container ID of processes which are beneath it in the "process
> tree" where the "process tree" is defined as the relationship between
> a parent and children processes such that the children processes are
> branches below the parent process.

Yes.

> I have no problem with that, with the understanding that nesting
> complicates it somewhat.  For example, this isn't true when one of the
> children is a nested orchestrator, is it?

It should still be true if that child is a nested orchestrator that has
not yet spawned any children or threads (or they have all died off).

It does get more complicated when we consider the scenario outlined
below about perceived layer violations...

> > > > You would only change the audit container ID from one set or inherited
> > > > value to another if you were nesting containers.
> 
> I thought we decided we were going to allow an orchestrator to move a
> process between audit container IDs, yes?  no?

We did?  I don't remember anything about that.  Has this been requested?
This seems to violate the rule that we can't change the audit container
identifier once it has been set (other than nesting).  Can you suggest a
use case?

> > > > If changing the contid, the container orchestrator/engine must be a
> > > > descendant and not same orchestrator as the one that set it so it is not
> > > > possible to change the contid of another orchestrator's container.
> 
> Try rephrasing the above please, it isn't clear to me what you are
> trying to say.

This is harder than I expected to rephrase...  It also makes it clear
that there are some scenarios that have not been considered that may
need to be restricted.

Orchestrator A spawned task B which is itself an orchestrator without
chidren yet.  Orchestrator A sets the audit container identifier of B.
Neither A, nor B, nor any other child of A (or any of their
descendants), nor any orchestrator outside the tree of A (uncles, aunts
and cousins are outside), can change the audit container identifier of
B.

Orchestrator B spawns task C.  Here's where it gets tricky.  It seems
like a layer violation for B to spawn a child C and have A reach over B
to set the audit container identifier of C, especially if B is also an
orchestrator.  This all will be especially hard to police if we don't
limit the ability of an orchestrator task to set an audit container
identifier to that orchestrator's immediate children, only once.

> > Are we able to agree on the premises above?  Is anything asserted that
> > should not be and is there anything missing?
> 
> See above.
> 
> If you want to go back to the definitions/assumptions stage, it
> probably isn't worth worrying about the other comments until we get
> the above sorted.

I don't want to.  I'm trying to confirm that we are on the same page.

> paul moore

- RGB

--
Richard Guy Briggs <rgb@redhat.com>
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

_______________________________________________
Containers mailing list
Containers@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/containers