[RFC,ghak90,(was,ghak32),V3,02/10] audit: log container info of syscalls

Submitted by Richard Guy Briggs on June 6, 2018, 4:58 p.m.

Details

Message ID b839dc7fb29aae226b2a4602e7ef3c2a406ee90a.1528304203.git.rgb@redhat.com
State New
Series "audit: implement container identifier"
Headers show

Commit Message

Richard Guy Briggs June 6, 2018, 4:58 p.m.
Create a new audit record AUDIT_CONTAINER to document the audit
container identifier of a process if it is present.

Called from audit_log_exit(), syscalls are covered.

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

See: https://github.com/linux-audit/audit-kernel/issues/90
See: https://github.com/linux-audit/audit-userspace/issues/51
See: https://github.com/linux-audit/audit-testsuite/issues/64
See: https://github.com/linux-audit/audit-kernel/wiki/RFE-Audit-Container-ID
Signed-off-by: Richard Guy Briggs <rgb@redhat.com>
---
 include/linux/audit.h      |  7 +++++++
 include/uapi/linux/audit.h |  1 +
 kernel/audit.c             | 23 +++++++++++++++++++++++
 kernel/auditsc.c           |  3 +++
 4 files changed, 34 insertions(+)

Patch hide | download patch | download mbox

diff --git a/include/linux/audit.h b/include/linux/audit.h
index 497cd81..4e1e34e 100644
--- a/include/linux/audit.h
+++ b/include/linux/audit.h
@@ -152,6 +152,9 @@  extern void		    audit_log_key(struct audit_buffer *ab,
 extern int audit_log_task_context(struct audit_buffer *ab);
 extern void audit_log_task_info(struct audit_buffer *ab,
 				struct task_struct *tsk);
+extern int audit_log_contid(struct task_struct *tsk,
+				    struct audit_context *context,
+				    char *op);
 
 extern int		    audit_update_lsm_rules(void);
 
@@ -202,6 +205,10 @@  static inline int audit_log_task_context(struct audit_buffer *ab)
 static inline void audit_log_task_info(struct audit_buffer *ab,
 				       struct task_struct *tsk)
 { }
+static inline int audit_log_contid(struct task_struct *tsk,
+					   struct audit_context *context,
+					   char *op)
+{ }
 #define audit_enabled 0
 #endif /* CONFIG_AUDIT */
 
diff --git a/include/uapi/linux/audit.h b/include/uapi/linux/audit.h
index c3b1aca..469ab25 100644
--- a/include/uapi/linux/audit.h
+++ b/include/uapi/linux/audit.h
@@ -115,6 +115,7 @@ 
 #define AUDIT_REPLACE		1329	/* Replace auditd if this packet unanswerd */
 #define AUDIT_KERN_MODULE	1330	/* Kernel Module events */
 #define AUDIT_FANOTIFY		1331	/* Fanotify access decision */
+#define AUDIT_CONTAINER		1332	/* Container ID */
 
 #define AUDIT_AVC		1400	/* SE Linux avc denial or grant */
 #define AUDIT_SELINUX_ERR	1401	/* Internal SE Linux Errors */
diff --git a/kernel/audit.c b/kernel/audit.c
index e7478cb..5e150c6 100644
--- a/kernel/audit.c
+++ b/kernel/audit.c
@@ -2048,6 +2048,29 @@  void audit_log_session_info(struct audit_buffer *ab)
 	audit_log_format(ab, " auid=%u ses=%u", auid, sessionid);
 }
 
+/*
+ * audit_log_contid - report container info
+ * @tsk: task to be recorded
+ * @context: task or local context for record
+ * @op: contid string description
+ */
+int audit_log_contid(struct task_struct *tsk,
+			     struct audit_context *context, char *op)
+{
+	struct audit_buffer *ab;
+
+	if (!audit_contid_set(tsk))
+		return 0;
+	/* Generate AUDIT_CONTAINER record with container ID */
+	ab = audit_log_start(context, GFP_KERNEL, AUDIT_CONTAINER);
+	if (!ab)
+		return -ENOMEM;
+	audit_log_format(ab, "op=%s contid=%llu",
+			 op, audit_get_contid(tsk));
+	audit_log_end(ab);
+	return 0;
+}
+
 void audit_log_key(struct audit_buffer *ab, char *key)
 {
 	audit_log_format(ab, " key=");
diff --git a/kernel/auditsc.c b/kernel/auditsc.c
index 611e926..a3c946c 100644
--- a/kernel/auditsc.c
+++ b/kernel/auditsc.c
@@ -1490,10 +1490,13 @@  static void audit_log_exit(struct audit_context *context, struct task_struct *ts
 
 	audit_log_proctitle(tsk, context);
 
+	audit_log_contid(tsk, context, "task");
+
 	/* Send end of event record to help user space know we are finished */
 	ab = audit_log_start(context, GFP_KERNEL, AUDIT_EOE);
 	if (ab)
 		audit_log_end(ab);
+
 	if (call_panic)
 		audit_panic("error converting sid to string");
 }

Comments

Steve Grubb June 6, 2018, 5:58 p.m.
On Wednesday, June 6, 2018 12:58:29 PM EDT Richard Guy Briggs wrote:
> Create a new audit record AUDIT_CONTAINER to document the audit
> container identifier of a process if it is present.
> 
> Called from audit_log_exit(), syscalls are covered.
> 
> A sample raw event:
> type=SYSCALL msg=audit(1519924845.499:257): arch=c000003e syscall=257
> success=yes exit=3 a0=ffffff9c a1=56374e1cef30 a2=241 a3=1b6 items=2
> ppid=606 pid=635 auid=0 uid=0 gid=0 euid=0 suid=0 fsuid=0 egid=0 sgid=0
> fsgid=0 tty=pts0 ses=3 comm="bash" exe="/usr/bin/bash"
> subj=unconfined_u:unconfined_r:unconfined_t:s0-s0:c0.c1023
> key="tmpcontainerid" type=CWD msg=audit(1519924845.499:257): cwd="/root"
> type=PATH msg=audit(1519924845.499:257): item=0 name="/tmp/" inode=13863
> dev=00:27 mode=041777 ouid=0 ogid=0 rdev=00:00
> obj=system_u:object_r:tmp_t:s0 nametype= PARENT cap_fp=0000000000000000
> cap_fi=0000000000000000 cap_fe=0 cap_fver=0 type=PATH
> msg=audit(1519924845.499:257): item=1 name="/tmp/tmpcontainerid"
> inode=17729 dev=00:27 mode=0100644 ouid=0 ogid=0 rdev=00:00
> obj=unconfined_u:object_r:user_tmp_t:s0 nametype=CREATE
> cap_fp=0000000000000000 cap_fi=0000000000000000 cap_fe=0 cap_fver=0
> type=PROCTITLE msg=audit(1519924845.499:257):
> proctitle=62617368002D6300736C65657020313B206563686F2074657374203E202F746D
> 702F746D70636F6E7461696E65726964 type=CONTAINER
> msg=audit(1519924845.499:257): op=task contid=123458

Ack for the audit record names.

-Steve

> See: https://github.com/linux-audit/audit-kernel/issues/90
> See: https://github.com/linux-audit/audit-userspace/issues/51
> See: https://github.com/linux-audit/audit-testsuite/issues/64
> See:
> https://github.com/linux-audit/audit-kernel/wiki/RFE-Audit-Container-ID
> Signed-off-by: Richard Guy Briggs <rgb@redhat.com>
> ---
>  include/linux/audit.h      |  7 +++++++
>  include/uapi/linux/audit.h |  1 +
>  kernel/audit.c             | 23 +++++++++++++++++++++++
>  kernel/auditsc.c           |  3 +++
>  4 files changed, 34 insertions(+)
> 
> diff --git a/include/linux/audit.h b/include/linux/audit.h
> index 497cd81..4e1e34e 100644
> --- a/include/linux/audit.h
> +++ b/include/linux/audit.h
> @@ -152,6 +152,9 @@ extern void		    audit_log_key(struct audit_buffer 
*ab,
> extern int audit_log_task_context(struct audit_buffer *ab);
>  extern void audit_log_task_info(struct audit_buffer *ab,
>  				struct task_struct *tsk);
> +extern int audit_log_contid(struct task_struct *tsk,
> +				    struct audit_context *context,
> +				    char *op);
> 
>  extern int		    audit_update_lsm_rules(void);
> 
> @@ -202,6 +205,10 @@ static inline int audit_log_task_context(struct
> audit_buffer *ab) static inline void audit_log_task_info(struct
> audit_buffer *ab,
>  				       struct task_struct *tsk)
>  { }
> +static inline int audit_log_contid(struct task_struct *tsk,
> +					   struct audit_context *context,
> +					   char *op)
> +{ }
>  #define audit_enabled 0
>  #endif /* CONFIG_AUDIT */
> 
> diff --git a/include/uapi/linux/audit.h b/include/uapi/linux/audit.h
> index c3b1aca..469ab25 100644
> --- a/include/uapi/linux/audit.h
> +++ b/include/uapi/linux/audit.h
> @@ -115,6 +115,7 @@
>  #define AUDIT_REPLACE		1329	/* Replace auditd if this packet unanswerd 
*/
>  #define AUDIT_KERN_MODULE	1330	/* Kernel Module events */
>  #define AUDIT_FANOTIFY		1331	/* Fanotify access decision */
> +#define AUDIT_CONTAINER		1332	/* Container ID */
> 
>  #define AUDIT_AVC		1400	/* SE Linux avc denial or grant */
>  #define AUDIT_SELINUX_ERR	1401	/* Internal SE Linux Errors */
> diff --git a/kernel/audit.c b/kernel/audit.c
> index e7478cb..5e150c6 100644
> --- a/kernel/audit.c
> +++ b/kernel/audit.c
> @@ -2048,6 +2048,29 @@ void audit_log_session_info(struct audit_buffer *ab)
> audit_log_format(ab, " auid=%u ses=%u", auid, sessionid);
>  }
> 
> +/*
> + * audit_log_contid - report container info
> + * @tsk: task to be recorded
> + * @context: task or local context for record
> + * @op: contid string description
> + */
> +int audit_log_contid(struct task_struct *tsk,
> +			     struct audit_context *context, char *op)
> +{
> +	struct audit_buffer *ab;
> +
> +	if (!audit_contid_set(tsk))
> +		return 0;
> +	/* Generate AUDIT_CONTAINER record with container ID */
> +	ab = audit_log_start(context, GFP_KERNEL, AUDIT_CONTAINER);
> +	if (!ab)
> +		return -ENOMEM;
> +	audit_log_format(ab, "op=%s contid=%llu",
> +			 op, audit_get_contid(tsk));
> +	audit_log_end(ab);
> +	return 0;
> +}
> +
>  void audit_log_key(struct audit_buffer *ab, char *key)
>  {
>  	audit_log_format(ab, " key=");
> diff --git a/kernel/auditsc.c b/kernel/auditsc.c
> index 611e926..a3c946c 100644
> --- a/kernel/auditsc.c
> +++ b/kernel/auditsc.c
> @@ -1490,10 +1490,13 @@ static void audit_log_exit(struct audit_context
> *context, struct task_struct *ts
> 
>  	audit_log_proctitle(tsk, context);
> 
> +	audit_log_contid(tsk, context, "task");
> +
>  	/* Send end of event record to help user space know we are finished */
>  	ab = audit_log_start(context, GFP_KERNEL, AUDIT_EOE);
>  	if (ab)
>  		audit_log_end(ab);
> +
>  	if (call_panic)
>  		audit_panic("error converting sid to string");
>  }
Paul Moore July 20, 2018, 10:13 p.m.
On Wed, Jun 6, 2018 at 1:00 PM Richard Guy Briggs <rgb@redhat.com> wrote:
> Create a new audit record AUDIT_CONTAINER to document the audit
> container identifier of a process if it is present.
>
> Called from audit_log_exit(), syscalls are covered.
>
> A sample raw event:
> type=SYSCALL msg=audit(1519924845.499:257): arch=c000003e syscall=257 success=yes exit=3 a0=ffffff9c a1=56374e1cef30 a2=241 a3=1b6 items=2 ppid=606 pid=635 auid=0 uid=0 gid=0 euid=0 suid=0 fsuid=0 egid=0 sgid=0 fsgid=0 tty=pts0 ses=3 comm="bash" exe="/usr/bin/bash" subj=unconfined_u:unconfined_r:unconfined_t:s0-s0:c0.c1023 key="tmpcontainerid"
> type=CWD msg=audit(1519924845.499:257): cwd="/root"
> type=PATH msg=audit(1519924845.499:257): item=0 name="/tmp/" inode=13863 dev=00:27 mode=041777 ouid=0 ogid=0 rdev=00:00 obj=system_u:object_r:tmp_t:s0 nametype= PARENT cap_fp=0000000000000000 cap_fi=0000000000000000 cap_fe=0 cap_fver=0
> type=PATH msg=audit(1519924845.499:257): item=1 name="/tmp/tmpcontainerid" inode=17729 dev=00:27 mode=0100644 ouid=0 ogid=0 rdev=00:00 obj=unconfined_u:object_r:user_tmp_t:s0 nametype=CREATE cap_fp=0000000000000000 cap_fi=0000000000000000 cap_fe=0 cap_fver=0
> type=PROCTITLE msg=audit(1519924845.499:257): proctitle=62617368002D6300736C65657020313B206563686F2074657374203E202F746D702F746D70636F6E7461696E65726964
> type=CONTAINER msg=audit(1519924845.499:257): op=task contid=123458
>
> See: https://github.com/linux-audit/audit-kernel/issues/90
> See: https://github.com/linux-audit/audit-userspace/issues/51
> See: https://github.com/linux-audit/audit-testsuite/issues/64
> See: https://github.com/linux-audit/audit-kernel/wiki/RFE-Audit-Container-ID
> Signed-off-by: Richard Guy Briggs <rgb@redhat.com>
> ---
>  include/linux/audit.h      |  7 +++++++
>  include/uapi/linux/audit.h |  1 +
>  kernel/audit.c             | 23 +++++++++++++++++++++++
>  kernel/auditsc.c           |  3 +++
>  4 files changed, 34 insertions(+)

...

> --- a/include/uapi/linux/audit.h
> +++ b/include/uapi/linux/audit.h
> @@ -115,6 +115,7 @@
>  #define AUDIT_REPLACE          1329    /* Replace auditd if this packet unanswerd */
>  #define AUDIT_KERN_MODULE      1330    /* Kernel Module events */
>  #define AUDIT_FANOTIFY         1331    /* Fanotify access decision */
> +#define AUDIT_CONTAINER                1332    /* Container ID */

I'm not sure I'm completely sold on the AUDIT_CONTAINER_ID and
AUDIT_CONTAINER record type names.  From what I can tell
AUDIT_CONTAINER_ID seems to be used for audit container ID management
operations, e.g. setting the ID, whereas the AUDIT_CONTAINER is used
to tag events with the corresponding audit container ID.  Assuming
that is correct, it seems like AUDIT_CONTAINER might be better served
if it was named AUDIT_CONTAINER_ID and if we could change
AUDIT_CONTAINER_ID to AUDIT_CONTAINER_OP/MGMT/etc.  Thoughts?

>  #define AUDIT_AVC              1400    /* SE Linux avc denial or grant */
>  #define AUDIT_SELINUX_ERR      1401    /* Internal SE Linux Errors */
> diff --git a/kernel/audit.c b/kernel/audit.c
> index e7478cb..5e150c6 100644
> --- a/kernel/audit.c
> +++ b/kernel/audit.c
> @@ -2048,6 +2048,29 @@ void audit_log_session_info(struct audit_buffer *ab)
>         audit_log_format(ab, " auid=%u ses=%u", auid, sessionid);
>  }
>
> +/*
> + * audit_log_contid - report container info
> + * @tsk: task to be recorded
> + * @context: task or local context for record
> + * @op: contid string description
> + */
> +int audit_log_contid(struct task_struct *tsk,
> +                            struct audit_context *context, char *op)
> +{
> +       struct audit_buffer *ab;
> +
> +       if (!audit_contid_set(tsk))
> +               return 0;
> +       /* Generate AUDIT_CONTAINER record with container ID */
> +       ab = audit_log_start(context, GFP_KERNEL, AUDIT_CONTAINER);
> +       if (!ab)
> +               return -ENOMEM;
> +       audit_log_format(ab, "op=%s contid=%llu",
> +                        op, audit_get_contid(tsk));

Can you explain your reason for including an "op" field in this record
type?  I've been looking at the rest of the patches in this patchset
and it seems to be used more as an indicator of the record's
generating context rather than any sort of audit container ID
operation.

> +       audit_log_end(ab);
> +       return 0;
> +}

--
paul moore
www.paul-moore.com
Richard Guy Briggs July 21, 2018, 8:29 p.m.
On 2018-07-20 18:13, Paul Moore wrote:
> On Wed, Jun 6, 2018 at 1:00 PM Richard Guy Briggs <rgb@redhat.com> wrote:
> > Create a new audit record AUDIT_CONTAINER to document the audit
> > container identifier of a process if it is present.
> >
> > Called from audit_log_exit(), syscalls are covered.
> >
> > A sample raw event:
> > type=SYSCALL msg=audit(1519924845.499:257): arch=c000003e syscall=257 success=yes exit=3 a0=ffffff9c a1=56374e1cef30 a2=241 a3=1b6 items=2 ppid=606 pid=635 auid=0 uid=0 gid=0 euid=0 suid=0 fsuid=0 egid=0 sgid=0 fsgid=0 tty=pts0 ses=3 comm="bash" exe="/usr/bin/bash" subj=unconfined_u:unconfined_r:unconfined_t:s0-s0:c0.c1023 key="tmpcontainerid"
> > type=CWD msg=audit(1519924845.499:257): cwd="/root"
> > type=PATH msg=audit(1519924845.499:257): item=0 name="/tmp/" inode=13863 dev=00:27 mode=041777 ouid=0 ogid=0 rdev=00:00 obj=system_u:object_r:tmp_t:s0 nametype= PARENT cap_fp=0000000000000000 cap_fi=0000000000000000 cap_fe=0 cap_fver=0
> > type=PATH msg=audit(1519924845.499:257): item=1 name="/tmp/tmpcontainerid" inode=17729 dev=00:27 mode=0100644 ouid=0 ogid=0 rdev=00:00 obj=unconfined_u:object_r:user_tmp_t:s0 nametype=CREATE cap_fp=0000000000000000 cap_fi=0000000000000000 cap_fe=0 cap_fver=0
> > type=PROCTITLE msg=audit(1519924845.499:257): proctitle=62617368002D6300736C65657020313B206563686F2074657374203E202F746D702F746D70636F6E7461696E65726964
> > type=CONTAINER msg=audit(1519924845.499:257): op=task contid=123458
> >
> > See: https://github.com/linux-audit/audit-kernel/issues/90
> > See: https://github.com/linux-audit/audit-userspace/issues/51
> > See: https://github.com/linux-audit/audit-testsuite/issues/64
> > See: https://github.com/linux-audit/audit-kernel/wiki/RFE-Audit-Container-ID
> > Signed-off-by: Richard Guy Briggs <rgb@redhat.com>
> > ---
> >  include/linux/audit.h      |  7 +++++++
> >  include/uapi/linux/audit.h |  1 +
> >  kernel/audit.c             | 23 +++++++++++++++++++++++
> >  kernel/auditsc.c           |  3 +++
> >  4 files changed, 34 insertions(+)
> 
> ...
> 
> > --- a/include/uapi/linux/audit.h
> > +++ b/include/uapi/linux/audit.h
> > @@ -115,6 +115,7 @@
> >  #define AUDIT_REPLACE          1329    /* Replace auditd if this packet unanswerd */
> >  #define AUDIT_KERN_MODULE      1330    /* Kernel Module events */
> >  #define AUDIT_FANOTIFY         1331    /* Fanotify access decision */
> > +#define AUDIT_CONTAINER                1332    /* Container ID */
> 
> I'm not sure I'm completely sold on the AUDIT_CONTAINER_ID and
> AUDIT_CONTAINER record type names.  From what I can tell
> AUDIT_CONTAINER_ID seems to be used for audit container ID management
> operations, e.g. setting the ID, whereas the AUDIT_CONTAINER is used
> to tag events with the corresponding audit container ID.  Assuming
> that is correct, it seems like AUDIT_CONTAINER might be better served
> if it was named AUDIT_CONTAINER_ID and if we could change
> AUDIT_CONTAINER_ID to AUDIT_CONTAINER_OP/MGMT/etc.  Thoughts?

Please see discussion at:
	https://www.redhat.com/archives/linux-audit/2018-May/msg00101.html

I'm fine with changing AUDIT_CONTAINER_ID to AUDIT_CONTAINER_OP/MGMT/etc.

> >  #define AUDIT_AVC              1400    /* SE Linux avc denial or grant */
> >  #define AUDIT_SELINUX_ERR      1401    /* Internal SE Linux Errors */
> > diff --git a/kernel/audit.c b/kernel/audit.c
> > index e7478cb..5e150c6 100644
> > --- a/kernel/audit.c
> > +++ b/kernel/audit.c
> > @@ -2048,6 +2048,29 @@ void audit_log_session_info(struct audit_buffer *ab)
> >         audit_log_format(ab, " auid=%u ses=%u", auid, sessionid);
> >  }
> >
> > +/*
> > + * audit_log_contid - report container info
> > + * @tsk: task to be recorded
> > + * @context: task or local context for record
> > + * @op: contid string description
> > + */
> > +int audit_log_contid(struct task_struct *tsk,
> > +                            struct audit_context *context, char *op)
> > +{
> > +       struct audit_buffer *ab;
> > +
> > +       if (!audit_contid_set(tsk))
> > +               return 0;
> > +       /* Generate AUDIT_CONTAINER record with container ID */
> > +       ab = audit_log_start(context, GFP_KERNEL, AUDIT_CONTAINER);
> > +       if (!ab)
> > +               return -ENOMEM;
> > +       audit_log_format(ab, "op=%s contid=%llu",
> > +                        op, audit_get_contid(tsk));
> 
> Can you explain your reason for including an "op" field in this record
> type?  I've been looking at the rest of the patches in this patchset
> and it seems to be used more as an indicator of the record's
> generating context rather than any sort of audit container ID
> operation.

"action" might work, but that's netfilter and numeric... "kind"?
Nothing else really seems to fit from a field name, type or lack of
searchability perspective.

Steve, do you have an opinion?

> > +       audit_log_end(ab);
> > +       return 0;
> > +}
> 
> --
> paul moore
> www.paul-moore.com

- 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
Steve Grubb July 22, 2018, 1:32 p.m.
On Saturday, July 21, 2018 4:29:30 PM EDT Richard Guy Briggs wrote:
> > > + * audit_log_contid - report container info
> > > + * @tsk: task to be recorded
> > > + * @context: task or local context for record
> > > + * @op: contid string description
> > > + */
> > > +int audit_log_contid(struct task_struct *tsk,
> > > +                            struct audit_context *context, char *op)
> > > +{
> > > +       struct audit_buffer *ab;
> > > +
> > > +       if (!audit_contid_set(tsk))
> > > +               return 0;
> > > +       /* Generate AUDIT_CONTAINER record with container ID */
> > > +       ab = audit_log_start(context, GFP_KERNEL, AUDIT_CONTAINER);
> > > +       if (!ab)
> > > +               return -ENOMEM;
> > > +       audit_log_format(ab, "op=%s contid=%llu",
> > > +                        op, audit_get_contid(tsk));
> > 
> > Can you explain your reason for including an "op" field in this record
> > type?  I've been looking at the rest of the patches in this patchset
> > and it seems to be used more as an indicator of the record's
> > generating context rather than any sort of audit container ID
> > operation.
> 
> "action" might work, but that's netfilter and numeric... "kind"?
> Nothing else really seems to fit from a field name, type or lack of
> searchability perspective.
> 
> Steve, do you have an opinion?

We only have 1 sample event where we have op=task. What are the other 
possible values?

-Steve
Richard Guy Briggs July 22, 2018, 8:55 p.m.
On 2018-07-22 09:32, Steve Grubb wrote:
> On Saturday, July 21, 2018 4:29:30 PM EDT Richard Guy Briggs wrote:
> > > > + * audit_log_contid - report container info
> > > > + * @tsk: task to be recorded
> > > > + * @context: task or local context for record
> > > > + * @op: contid string description
> > > > + */
> > > > +int audit_log_contid(struct task_struct *tsk,
> > > > +                            struct audit_context *context, char *op)
> > > > +{
> > > > +       struct audit_buffer *ab;
> > > > +
> > > > +       if (!audit_contid_set(tsk))
> > > > +               return 0;
> > > > +       /* Generate AUDIT_CONTAINER record with container ID */
> > > > +       ab = audit_log_start(context, GFP_KERNEL, AUDIT_CONTAINER);
> > > > +       if (!ab)
> > > > +               return -ENOMEM;
> > > > +       audit_log_format(ab, "op=%s contid=%llu",
> > > > +                        op, audit_get_contid(tsk));
> > > 
> > > Can you explain your reason for including an "op" field in this record
> > > type?  I've been looking at the rest of the patches in this patchset
> > > and it seems to be used more as an indicator of the record's
> > > generating context rather than any sort of audit container ID
> > > operation.
> > 
> > "action" might work, but that's netfilter and numeric... "kind"?
> > Nothing else really seems to fit from a field name, type or lack of
> > searchability perspective.
> > 
> > Steve, do you have an opinion?
> 
> We only have 1 sample event where we have op=task. What are the other 
> possible values?

For the AUDIT_CONTAINER record we have op= "task", "target" (from the
ptrace and signals patch), "tty".

For the AUDIT_CONTAINER_ID record we have "op=set".

> -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
Richard Guy Briggs July 22, 2018, 9:03 p.m.
On 2018-07-22 16:55, Richard Guy Briggs wrote:
> On 2018-07-22 09:32, Steve Grubb wrote:
> > On Saturday, July 21, 2018 4:29:30 PM EDT Richard Guy Briggs wrote:
> > > > > + * audit_log_contid - report container info
> > > > > + * @tsk: task to be recorded
> > > > > + * @context: task or local context for record
> > > > > + * @op: contid string description
> > > > > + */
> > > > > +int audit_log_contid(struct task_struct *tsk,
> > > > > +                            struct audit_context *context, char *op)
> > > > > +{
> > > > > +       struct audit_buffer *ab;
> > > > > +
> > > > > +       if (!audit_contid_set(tsk))
> > > > > +               return 0;
> > > > > +       /* Generate AUDIT_CONTAINER record with container ID */
> > > > > +       ab = audit_log_start(context, GFP_KERNEL, AUDIT_CONTAINER);
> > > > > +       if (!ab)
> > > > > +               return -ENOMEM;
> > > > > +       audit_log_format(ab, "op=%s contid=%llu",
> > > > > +                        op, audit_get_contid(tsk));
> > > > 
> > > > Can you explain your reason for including an "op" field in this record
> > > > type?  I've been looking at the rest of the patches in this patchset
> > > > and it seems to be used more as an indicator of the record's
> > > > generating context rather than any sort of audit container ID
> > > > operation.
> > > 
> > > "action" might work, but that's netfilter and numeric... "kind"?
> > > Nothing else really seems to fit from a field name, type or lack of
> > > searchability perspective.
> > > 
> > > Steve, do you have an opinion?
> > 
> > We only have 1 sample event where we have op=task. What are the other 
> > possible values?
> 
> For the AUDIT_CONTAINER record we have op= "task", "target" (from the
> ptrace and signals patch), "tty".

Sorry, pressed "send" too quickly.  Also "aux0x%x" (also from the
ptrace/signals patch), "net%u" (from the AUDIT_NETFILTER_PKT patch).

> For the AUDIT_CONTAINER_ID record we have "op=set".
> 
> > -Steve
> 
> - 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
Paul Moore July 23, 2018, 1:16 p.m.
On Sat, Jul 21, 2018 at 4:32 PM Richard Guy Briggs <rgb@redhat.com> wrote:
> On 2018-07-20 18:13, Paul Moore wrote:
> > On Wed, Jun 6, 2018 at 1:00 PM Richard Guy Briggs <rgb@redhat.com> wrote:
> > > Create a new audit record AUDIT_CONTAINER to document the audit
> > > container identifier of a process if it is present.
> > >
> > > Called from audit_log_exit(), syscalls are covered.
> > >
> > > A sample raw event:
> > > type=SYSCALL msg=audit(1519924845.499:257): arch=c000003e syscall=257 success=yes exit=3 a0=ffffff9c a1=56374e1cef30 a2=241 a3=1b6 items=2 ppid=606 pid=635 auid=0 uid=0 gid=0 euid=0 suid=0 fsuid=0 egid=0 sgid=0 fsgid=0 tty=pts0 ses=3 comm="bash" exe="/usr/bin/bash" subj=unconfined_u:unconfined_r:unconfined_t:s0-s0:c0.c1023 key="tmpcontainerid"
> > > type=CWD msg=audit(1519924845.499:257): cwd="/root"
> > > type=PATH msg=audit(1519924845.499:257): item=0 name="/tmp/" inode=13863 dev=00:27 mode=041777 ouid=0 ogid=0 rdev=00:00 obj=system_u:object_r:tmp_t:s0 nametype= PARENT cap_fp=0000000000000000 cap_fi=0000000000000000 cap_fe=0 cap_fver=0
> > > type=PATH msg=audit(1519924845.499:257): item=1 name="/tmp/tmpcontainerid" inode=17729 dev=00:27 mode=0100644 ouid=0 ogid=0 rdev=00:00 obj=unconfined_u:object_r:user_tmp_t:s0 nametype=CREATE cap_fp=0000000000000000 cap_fi=0000000000000000 cap_fe=0 cap_fver=0
> > > type=PROCTITLE msg=audit(1519924845.499:257): proctitle=62617368002D6300736C65657020313B206563686F2074657374203E202F746D702F746D70636F6E7461696E65726964
> > > type=CONTAINER msg=audit(1519924845.499:257): op=task contid=123458
> > >
> > > See: https://github.com/linux-audit/audit-kernel/issues/90
> > > See: https://github.com/linux-audit/audit-userspace/issues/51
> > > See: https://github.com/linux-audit/audit-testsuite/issues/64
> > > See: https://github.com/linux-audit/audit-kernel/wiki/RFE-Audit-Container-ID
> > > Signed-off-by: Richard Guy Briggs <rgb@redhat.com>
> > > ---
> > >  include/linux/audit.h      |  7 +++++++
> > >  include/uapi/linux/audit.h |  1 +
> > >  kernel/audit.c             | 23 +++++++++++++++++++++++
> > >  kernel/auditsc.c           |  3 +++
> > >  4 files changed, 34 insertions(+)
> >
> > ...
> >
> > > --- a/include/uapi/linux/audit.h
> > > +++ b/include/uapi/linux/audit.h
> > > @@ -115,6 +115,7 @@
> > >  #define AUDIT_REPLACE          1329    /* Replace auditd if this packet unanswerd */
> > >  #define AUDIT_KERN_MODULE      1330    /* Kernel Module events */
> > >  #define AUDIT_FANOTIFY         1331    /* Fanotify access decision */
> > > +#define AUDIT_CONTAINER                1332    /* Container ID */
> >
> > I'm not sure I'm completely sold on the AUDIT_CONTAINER_ID and
> > AUDIT_CONTAINER record type names.  From what I can tell
> > AUDIT_CONTAINER_ID seems to be used for audit container ID management
> > operations, e.g. setting the ID, whereas the AUDIT_CONTAINER is used
> > to tag events with the corresponding audit container ID.  Assuming
> > that is correct, it seems like AUDIT_CONTAINER might be better served
> > if it was named AUDIT_CONTAINER_ID and if we could change
> > AUDIT_CONTAINER_ID to AUDIT_CONTAINER_OP/MGMT/etc.  Thoughts?
>
> Please see discussion at:
>         https://www.redhat.com/archives/linux-audit/2018-May/msg00101.html
>
> I'm fine with changing AUDIT_CONTAINER_ID to AUDIT_CONTAINER_OP/MGMT/etc.

Noted, and while I'm generally a big fan of consistency for things
like this, I think these things are different enough (the loginuid is
recorded as a field, the audit container ID is recorded in a dedicated
record) that we don't need to be bound by LOGINUID's naming
convention.

> > >  #define AUDIT_AVC              1400    /* SE Linux avc denial or grant */
> > >  #define AUDIT_SELINUX_ERR      1401    /* Internal SE Linux Errors */
> > > diff --git a/kernel/audit.c b/kernel/audit.c
> > > index e7478cb..5e150c6 100644
> > > --- a/kernel/audit.c
> > > +++ b/kernel/audit.c
> > > @@ -2048,6 +2048,29 @@ void audit_log_session_info(struct audit_buffer *ab)
> > >         audit_log_format(ab, " auid=%u ses=%u", auid, sessionid);
> > >  }
> > >
> > > +/*
> > > + * audit_log_contid - report container info
> > > + * @tsk: task to be recorded
> > > + * @context: task or local context for record
> > > + * @op: contid string description
> > > + */
> > > +int audit_log_contid(struct task_struct *tsk,
> > > +                            struct audit_context *context, char *op)
> > > +{
> > > +       struct audit_buffer *ab;
> > > +
> > > +       if (!audit_contid_set(tsk))
> > > +               return 0;
> > > +       /* Generate AUDIT_CONTAINER record with container ID */
> > > +       ab = audit_log_start(context, GFP_KERNEL, AUDIT_CONTAINER);
> > > +       if (!ab)
> > > +               return -ENOMEM;
> > > +       audit_log_format(ab, "op=%s contid=%llu",
> > > +                        op, audit_get_contid(tsk));
> >
> > Can you explain your reason for including an "op" field in this record
> > type?  I've been looking at the rest of the patches in this patchset
> > and it seems to be used more as an indicator of the record's
> > generating context rather than any sort of audit container ID
> > operation.
>
> "action" might work, but that's netfilter and numeric... "kind"?
> Nothing else really seems to fit from a field name, type or lack of
> searchability perspective.

My concern isn't so much the name of the "op" field, although that
does seem wrong, but rather the existence of the field in the first
place.  This audit container ID record (whatever we end up calling it)
exists to attach an audit container ID to an audit event, that's it;
an audit event should have other records which provide the context
(granted, the exact number of records depends on the event and the
system's configuration).  If we are relying on this record to provide
critical information about the audit event other than the audit
container ID, I believe this is a strong indicator that the existing
audit records are lacking and should be augmented.
Steve Grubb July 23, 2018, 1:19 p.m.
On Sunday, July 22, 2018 4:55:10 PM EDT Richard Guy Briggs wrote:
> On 2018-07-22 09:32, Steve Grubb wrote:
> > On Saturday, July 21, 2018 4:29:30 PM EDT Richard Guy Briggs wrote:
> > > > > + * audit_log_contid - report container info
> > > > > + * @tsk: task to be recorded
> > > > > + * @context: task or local context for record
> > > > > + * @op: contid string description
> > > > > + */
> > > > > +int audit_log_contid(struct task_struct *tsk,
> > > > > +                            struct audit_context *context, char
> > > > > *op)
> > > > > +{
> > > > > +       struct audit_buffer *ab;
> > > > > +
> > > > > +       if (!audit_contid_set(tsk))
> > > > > +               return 0;
> > > > > +       /* Generate AUDIT_CONTAINER record with container ID */
> > > > > +       ab = audit_log_start(context, GFP_KERNEL, AUDIT_CONTAINER);
> > > > > +       if (!ab)
> > > > > +               return -ENOMEM;
> > > > > +       audit_log_format(ab, "op=%s contid=%llu",
> > > > > +                        op, audit_get_contid(tsk));
> > > > 
> > > > Can you explain your reason for including an "op" field in this
> > > > record
> > > > type?  I've been looking at the rest of the patches in this patchset
> > > > and it seems to be used more as an indicator of the record's
> > > > generating context rather than any sort of audit container ID
> > > > operation.
> > > 
> > > "action" might work, but that's netfilter and numeric... "kind"?
> > > Nothing else really seems to fit from a field name, type or lack of
> > > searchability perspective.
> > > 
> > > Steve, do you have an opinion?
> > 
> > We only have 1 sample event where we have op=task. What are the other
> > possible values?
> 
> For the AUDIT_CONTAINER record we have op= "task", "target" (from the
> ptrace and signals patch), "tty".
> 
> For the AUDIT_CONTAINER_ID record we have "op=set".

Since the purpose of this record is to log the container id, I think that is 
all that is needed. We can get the context from the other records in the 
event. I'd suggest dropping the "op" field.

-Steve
Richard Guy Briggs July 23, 2018, 3:11 p.m.
On 2018-07-23 09:19, Steve Grubb wrote:
> On Sunday, July 22, 2018 4:55:10 PM EDT Richard Guy Briggs wrote:
> > On 2018-07-22 09:32, Steve Grubb wrote:
> > > On Saturday, July 21, 2018 4:29:30 PM EDT Richard Guy Briggs wrote:
> > > > > > + * audit_log_contid - report container info
> > > > > > + * @tsk: task to be recorded
> > > > > > + * @context: task or local context for record
> > > > > > + * @op: contid string description
> > > > > > + */
> > > > > > +int audit_log_contid(struct task_struct *tsk,
> > > > > > +                            struct audit_context *context, char
> > > > > > *op)
> > > > > > +{
> > > > > > +       struct audit_buffer *ab;
> > > > > > +
> > > > > > +       if (!audit_contid_set(tsk))
> > > > > > +               return 0;
> > > > > > +       /* Generate AUDIT_CONTAINER record with container ID */
> > > > > > +       ab = audit_log_start(context, GFP_KERNEL, AUDIT_CONTAINER);
> > > > > > +       if (!ab)
> > > > > > +               return -ENOMEM;
> > > > > > +       audit_log_format(ab, "op=%s contid=%llu",
> > > > > > +                        op, audit_get_contid(tsk));
> > > > > 
> > > > > Can you explain your reason for including an "op" field in this
> > > > > record
> > > > > type?  I've been looking at the rest of the patches in this patchset
> > > > > and it seems to be used more as an indicator of the record's
> > > > > generating context rather than any sort of audit container ID
> > > > > operation.
> > > > 
> > > > "action" might work, but that's netfilter and numeric... "kind"?
> > > > Nothing else really seems to fit from a field name, type or lack of
> > > > searchability perspective.
> > > > 
> > > > Steve, do you have an opinion?
> > > 
> > > We only have 1 sample event where we have op=task. What are the other
> > > possible values?
> > 
> > For the AUDIT_CONTAINER record we have op= "task", "target" (from the
> > ptrace and signals patch), "tty".
> > 
> > For the AUDIT_CONTAINER_ID record we have "op=set".
> 
> Since the purpose of this record is to log the container id, I think that is 
> all that is needed. We can get the context from the other records in the 
> event. I'd suggest dropping the "op" field.

Ok, the information above it for two different audit container
identifier records.  Which one should drop the "op=" field?  Both?  Or
just the AUDIT_CONTAINER record?  The AUDIT_CONTAINER_ID record (which
might be renamed) could use it to distinguish a "set" record from a
dropped audit container identifier that is no longer registered by any
task or namespace.

> -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
Steve Grubb July 23, 2018, 4:48 p.m.
On Monday, July 23, 2018 11:11:48 AM EDT Richard Guy Briggs wrote:
> On 2018-07-23 09:19, Steve Grubb wrote:
> > On Sunday, July 22, 2018 4:55:10 PM EDT Richard Guy Briggs wrote:
> > > On 2018-07-22 09:32, Steve Grubb wrote:
> > > > On Saturday, July 21, 2018 4:29:30 PM EDT Richard Guy Briggs wrote:
> > > > > > > + * audit_log_contid - report container info
> > > > > > > + * @tsk: task to be recorded
> > > > > > > + * @context: task or local context for record
> > > > > > > + * @op: contid string description
> > > > > > > + */
> > > > > > > +int audit_log_contid(struct task_struct *tsk,
> > > > > > > +                            struct audit_context *context,
> > > > > > > char
> > > > > > > *op)
> > > > > > > +{
> > > > > > > +       struct audit_buffer *ab;
> > > > > > > +
> > > > > > > +       if (!audit_contid_set(tsk))
> > > > > > > +               return 0;
> > > > > > > +       /* Generate AUDIT_CONTAINER record with container ID */
> > > > > > > +       ab = audit_log_start(context, GFP_KERNEL,
> > > > > > > AUDIT_CONTAINER);
> > > > > > > +       if (!ab)
> > > > > > > +               return -ENOMEM;
> > > > > > > +       audit_log_format(ab, "op=%s contid=%llu",
> > > > > > > +                        op, audit_get_contid(tsk));
> > > > > > 
> > > > > > Can you explain your reason for including an "op" field in this
> > > > > > record
> > > > > > type?  I've been looking at the rest of the patches in this
> > > > > > patchset
> > > > > > and it seems to be used more as an indicator of the record's
> > > > > > generating context rather than any sort of audit container ID
> > > > > > operation.
> > > > > 
> > > > > "action" might work, but that's netfilter and numeric... "kind"?
> > > > > Nothing else really seems to fit from a field name, type or lack of
> > > > > searchability perspective.
> > > > > 
> > > > > Steve, do you have an opinion?
> > > > 
> > > > We only have 1 sample event where we have op=task. What are the other
> > > > possible values?
> > > 
> > > For the AUDIT_CONTAINER record we have op= "task", "target" (from the
> > > ptrace and signals patch), "tty".
> > > 
> > > For the AUDIT_CONTAINER_ID record we have "op=set".
> > 
> > Since the purpose of this record is to log the container id, I think that
> > is all that is needed. We can get the context from the other records in
> > the event. I'd suggest dropping the "op" field.
> 
> Ok, the information above it for two different audit container
> identifier records.  Which one should drop the "op=" field?  Both?  Or
> just the AUDIT_CONTAINER record?  The AUDIT_CONTAINER_ID record (which
> might be renamed) could use it to distinguish a "set" record from a
> dropped audit container identifier that is no longer registered by any
> task or namespace.

Neither of them need it. All they need to do is state the container that is 
being acted upon.

-Steve
Paul Moore July 23, 2018, 6:31 p.m.
On Mon, Jul 23, 2018 at 12:48 PM Steve Grubb <sgrubb@redhat.com> wrote:
> On Monday, July 23, 2018 11:11:48 AM EDT Richard Guy Briggs wrote:
> > On 2018-07-23 09:19, Steve Grubb wrote:
> > > On Sunday, July 22, 2018 4:55:10 PM EDT Richard Guy Briggs wrote:
> > > > On 2018-07-22 09:32, Steve Grubb wrote:
> > > > > On Saturday, July 21, 2018 4:29:30 PM EDT Richard Guy Briggs wrote:
> > > > > > > > + * audit_log_contid - report container info
> > > > > > > > + * @tsk: task to be recorded
> > > > > > > > + * @context: task or local context for record
> > > > > > > > + * @op: contid string description
> > > > > > > > + */
> > > > > > > > +int audit_log_contid(struct task_struct *tsk,
> > > > > > > > +                            struct audit_context *context,
> > > > > > > > char
> > > > > > > > *op)
> > > > > > > > +{
> > > > > > > > +       struct audit_buffer *ab;
> > > > > > > > +
> > > > > > > > +       if (!audit_contid_set(tsk))
> > > > > > > > +               return 0;
> > > > > > > > +       /* Generate AUDIT_CONTAINER record with container ID */
> > > > > > > > +       ab = audit_log_start(context, GFP_KERNEL,
> > > > > > > > AUDIT_CONTAINER);
> > > > > > > > +       if (!ab)
> > > > > > > > +               return -ENOMEM;
> > > > > > > > +       audit_log_format(ab, "op=%s contid=%llu",
> > > > > > > > +                        op, audit_get_contid(tsk));
> > > > > > >
> > > > > > > Can you explain your reason for including an "op" field in this
> > > > > > > record
> > > > > > > type?  I've been looking at the rest of the patches in this
> > > > > > > patchset
> > > > > > > and it seems to be used more as an indicator of the record's
> > > > > > > generating context rather than any sort of audit container ID
> > > > > > > operation.
> > > > > >
> > > > > > "action" might work, but that's netfilter and numeric... "kind"?
> > > > > > Nothing else really seems to fit from a field name, type or lack of
> > > > > > searchability perspective.
> > > > > >
> > > > > > Steve, do you have an opinion?
> > > > >
> > > > > We only have 1 sample event where we have op=task. What are the other
> > > > > possible values?
> > > >
> > > > For the AUDIT_CONTAINER record we have op= "task", "target" (from the
> > > > ptrace and signals patch), "tty".
> > > >
> > > > For the AUDIT_CONTAINER_ID record we have "op=set".
> > >
> > > Since the purpose of this record is to log the container id, I think that
> > > is all that is needed. We can get the context from the other records in
> > > the event. I'd suggest dropping the "op" field.
> >
> > Ok, the information above it for two different audit container
> > identifier records.  Which one should drop the "op=" field?  Both?  Or
> > just the AUDIT_CONTAINER record?  The AUDIT_CONTAINER_ID record (which
> > might be renamed) could use it to distinguish a "set" record from a
> > dropped audit container identifier that is no longer registered by any
> > task or namespace.
>
> Neither of them need it. All they need to do is state the container that is
> being acted upon.

I think we should keep the "op" field for audit container ID
management operations, even though we really only have a "set"
operation at the moment, but the others should drop the "op" field
(see my previous emails in this thread).
Richard Guy Briggs July 26, 2018, 12:51 a.m.
On 2018-07-23 14:31, Paul Moore wrote:
> On Mon, Jul 23, 2018 at 12:48 PM Steve Grubb <sgrubb@redhat.com> wrote:
> > On Monday, July 23, 2018 11:11:48 AM EDT Richard Guy Briggs wrote:
> > > On 2018-07-23 09:19, Steve Grubb wrote:
> > > > On Sunday, July 22, 2018 4:55:10 PM EDT Richard Guy Briggs wrote:
> > > > > On 2018-07-22 09:32, Steve Grubb wrote:
> > > > > > On Saturday, July 21, 2018 4:29:30 PM EDT Richard Guy Briggs wrote:
> > > > > > > > > + * audit_log_contid - report container info
> > > > > > > > > + * @tsk: task to be recorded
> > > > > > > > > + * @context: task or local context for record
> > > > > > > > > + * @op: contid string description
> > > > > > > > > + */
> > > > > > > > > +int audit_log_contid(struct task_struct *tsk,
> > > > > > > > > +                            struct audit_context *context,
> > > > > > > > > char
> > > > > > > > > *op)
> > > > > > > > > +{
> > > > > > > > > +       struct audit_buffer *ab;
> > > > > > > > > +
> > > > > > > > > +       if (!audit_contid_set(tsk))
> > > > > > > > > +               return 0;
> > > > > > > > > +       /* Generate AUDIT_CONTAINER record with container ID */
> > > > > > > > > +       ab = audit_log_start(context, GFP_KERNEL,
> > > > > > > > > AUDIT_CONTAINER);
> > > > > > > > > +       if (!ab)
> > > > > > > > > +               return -ENOMEM;
> > > > > > > > > +       audit_log_format(ab, "op=%s contid=%llu",
> > > > > > > > > +                        op, audit_get_contid(tsk));
> > > > > > > >
> > > > > > > > Can you explain your reason for including an "op" field in this
> > > > > > > > record
> > > > > > > > type?  I've been looking at the rest of the patches in this
> > > > > > > > patchset
> > > > > > > > and it seems to be used more as an indicator of the record's
> > > > > > > > generating context rather than any sort of audit container ID
> > > > > > > > operation.
> > > > > > >
> > > > > > > "action" might work, but that's netfilter and numeric... "kind"?
> > > > > > > Nothing else really seems to fit from a field name, type or lack of
> > > > > > > searchability perspective.
> > > > > > >
> > > > > > > Steve, do you have an opinion?
> > > > > >
> > > > > > We only have 1 sample event where we have op=task. What are the other
> > > > > > possible values?
> > > > >
> > > > > For the AUDIT_CONTAINER record we have op= "task", "target" (from the
> > > > > ptrace and signals patch), "tty".
> > > > >
> > > > > For the AUDIT_CONTAINER_ID record we have "op=set".
> > > >
> > > > Since the purpose of this record is to log the container id, I think that
> > > > is all that is needed. We can get the context from the other records in
> > > > the event. I'd suggest dropping the "op" field.
> > >
> > > Ok, the information above it for two different audit container
> > > identifier records.  Which one should drop the "op=" field?  Both?  Or
> > > just the AUDIT_CONTAINER record?  The AUDIT_CONTAINER_ID record (which
> > > might be renamed) could use it to distinguish a "set" record from a
> > > dropped audit container identifier that is no longer registered by any
> > > task or namespace.
> >
> > Neither of them need it. All they need to do is state the container that is
> > being acted upon.
> 
> I think we should keep the "op" field for audit container ID
> management operations, even though we really only have a "set"
> operation at the moment, but the others should drop the "op" field
> (see my previous emails in this thread).

In fact, I'd like to question the wisdom of dropping this field and
perhaps fine a better or new name for it, since these contid records
could be multiple and different from the primary task that is generating
this record.  In particular, there are extra contid records generated by
the ptrace/signals patch that could be from other processes in other
containers that I mentioned in a branch thread that got dropped
including the auc_pids data and the target_pid also attached to the
primary task's audit context.

> paul moore
> www.paul-moore.com

- 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
Richard Guy Briggs July 31, 2018, 8:07 p.m.
On 2018-07-25 20:51, Richard Guy Briggs wrote:
> On 2018-07-23 14:31, Paul Moore wrote:
> > On Mon, Jul 23, 2018 at 12:48 PM Steve Grubb <sgrubb@redhat.com> wrote:
> > > On Monday, July 23, 2018 11:11:48 AM EDT Richard Guy Briggs wrote:
> > > > On 2018-07-23 09:19, Steve Grubb wrote:
> > > > > On Sunday, July 22, 2018 4:55:10 PM EDT Richard Guy Briggs wrote:
> > > > > > On 2018-07-22 09:32, Steve Grubb wrote:
> > > > > > > On Saturday, July 21, 2018 4:29:30 PM EDT Richard Guy Briggs wrote:
> > > > > > > > > > + * audit_log_contid - report container info
> > > > > > > > > > + * @tsk: task to be recorded
> > > > > > > > > > + * @context: task or local context for record
> > > > > > > > > > + * @op: contid string description
> > > > > > > > > > + */
> > > > > > > > > > +int audit_log_contid(struct task_struct *tsk,
> > > > > > > > > > +                            struct audit_context *context,
> > > > > > > > > > char
> > > > > > > > > > *op)
> > > > > > > > > > +{
> > > > > > > > > > +       struct audit_buffer *ab;
> > > > > > > > > > +
> > > > > > > > > > +       if (!audit_contid_set(tsk))
> > > > > > > > > > +               return 0;
> > > > > > > > > > +       /* Generate AUDIT_CONTAINER record with container ID */
> > > > > > > > > > +       ab = audit_log_start(context, GFP_KERNEL,
> > > > > > > > > > AUDIT_CONTAINER);
> > > > > > > > > > +       if (!ab)
> > > > > > > > > > +               return -ENOMEM;
> > > > > > > > > > +       audit_log_format(ab, "op=%s contid=%llu",
> > > > > > > > > > +                        op, audit_get_contid(tsk));
> > > > > > > > >
> > > > > > > > > Can you explain your reason for including an "op" field in this
> > > > > > > > > record
> > > > > > > > > type?  I've been looking at the rest of the patches in this
> > > > > > > > > patchset
> > > > > > > > > and it seems to be used more as an indicator of the record's
> > > > > > > > > generating context rather than any sort of audit container ID
> > > > > > > > > operation.
> > > > > > > >
> > > > > > > > "action" might work, but that's netfilter and numeric... "kind"?
> > > > > > > > Nothing else really seems to fit from a field name, type or lack of
> > > > > > > > searchability perspective.
> > > > > > > >
> > > > > > > > Steve, do you have an opinion?
> > > > > > >
> > > > > > > We only have 1 sample event where we have op=task. What are the other
> > > > > > > possible values?
> > > > > >
> > > > > > For the AUDIT_CONTAINER record we have op= "task", "target" (from the
> > > > > > ptrace and signals patch), "tty".
> > > > > >
> > > > > > For the AUDIT_CONTAINER_ID record we have "op=set".
> > > > >
> > > > > Since the purpose of this record is to log the container id, I think that
> > > > > is all that is needed. We can get the context from the other records in
> > > > > the event. I'd suggest dropping the "op" field.
> > > >
> > > > Ok, the information above it for two different audit container
> > > > identifier records.  Which one should drop the "op=" field?  Both?  Or
> > > > just the AUDIT_CONTAINER record?  The AUDIT_CONTAINER_ID record (which
> > > > might be renamed) could use it to distinguish a "set" record from a
> > > > dropped audit container identifier that is no longer registered by any
> > > > task or namespace.
> > >
> > > Neither of them need it. All they need to do is state the container that is
> > > being acted upon.
> > 
> > I think we should keep the "op" field for audit container ID
> > management operations, even though we really only have a "set"
> > operation at the moment, but the others should drop the "op" field
> > (see my previous emails in this thread).
> 
> In fact, I'd like to question the wisdom of dropping this field and
> perhaps fine a better or new name for it, since these contid records
> could be multiple and different from the primary task that is generating
> this record.  In particular, there are extra contid records generated by
> the ptrace/signals patch that could be from other processes in other
> containers that I mentioned in a branch thread that got dropped
> including the auc_pids data and the target_pid also attached to the
> primary task's audit context.

Ok, not seeing any further follow-up on this item in almost a week, I'll
not delay any more and post rev 4 of the patchset.

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