date:20180418

Re: [RFC PATCH V1 01/12] audit: add container id

2018-04-18 Thread Stefan Berger


On 04/18/2018 03:23 PM, Richard Guy Briggs wrote:

On 2018-04-18 14:45, Stefan Berger wrote:

On 03/15/2018 11:58 PM, Richard Guy Briggs wrote:

On 2018-03-15 16:27, Stefan Berger wrote:

On 03/01/2018 02:41 PM, Richard Guy Briggs wrote:

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

This is a write from the container orchestrator task to a proc entry of
the form /proc/PID/containerid where PID is the process ID of the newly
created task that is to become the first task in a container, or an
additional task added to a container.

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

This will produce a record such as this:
type=UNKNOWN[1333] msg=audit(1519903238.968:261): op=set pid=596 uid=0 
subj=unconfined_u:unconfined_r:unconfined_t:s0-s0:c0.c1023 auid=0 tty=pts0 
ses=1 opid=596 old-contid=18446744073709551615 contid=123455 res=0

The "op" field indicates an initial set.  The "pid" to "ses" fields are
the orchestrator while the "opid" field is the object's PID, the process
being "contained".  Old and new container ID values are given in the
"contid" fields, while res indicates its success.

It is not permitted to self-set, unset or re-set the container ID.  A
child inherits its parent's container ID, but then can be set only once
after.

See: https://github.com/linux-audit/audit-kernel/issues/32


/* audit_rule_data supports filter rules with both integer and string
 * fields.  It corresponds with AUDIT_ADD_RULE, AUDIT_DEL_RULE and
diff --git a/kernel/auditsc.c b/kernel/auditsc.c
index 4e0a4ac..0ee1e59 100644
--- a/kernel/auditsc.c
+++ b/kernel/auditsc.c
@@ -2073,6 +2073,92 @@ int audit_set_loginuid(kuid_t loginuid)
return rc;
}

+static int audit_set_containerid_perm(struct task_struct *task, u64 
containerid)
+{
+   struct task_struct *parent;
+   u64 pcontainerid, ccontainerid;
+   pid_t ppid;
+
+   /* Don't allow to set our own containerid */
+   if (current == task)
+   return -EPERM;
+   /* Don't allow the containerid to be unset */
+   if (!cid_valid(containerid))
+   return -EINVAL;
+   /* if we don't have caps, reject */
+   if (!capable(CAP_AUDIT_CONTROL))
+   return -EPERM;
+   /* if containerid is unset, allow */
+   if (!audit_containerid_set(task))
+   return 0;

I am wondering whether there should be a check for the target process that
will receive the containerid to not have CAP_SYS_ADMIN that would otherwise
allow it to arbitrarily unshare()/clone() and leave the set of namespaces
that may make up the container whose containerid we assign here?

This is a reasonable question.  This has been debated and I understood
the conclusion was that without a clear definition of a "container", the
task still remains in that container that just now has more
sub-namespaces (in the case of hierarchical namespaces), we don't want
to restrict it in such a way and that allows it to create nested
containers.  I see setns being more problematic if it could switch to
another existing namespace that was set up by the orchestrator for a
different container.  The coming v2 patchset acknowledges this situation
with the network namespace being potentially shared by multiple
containers.

Are you going to post v2 soon? We would like to build on top of it for IMA
namespacing and auditing inside of IMA namespaces.

I don't know if it addresses your specific needs, but V2 was posted on
March 16th along with userspace patches:
https://www.redhat.com/archives/linux-audit/2018-March/msg00110.html
https://www.redhat.com/archives/linux-audit/2018-March/msg00124.html

V3 is pending.
Thanks. I hadn't actually looked at primarily due to the ghak and ghau 
in the title. Whatever these may mean.


Does V2 or will V3 prevent a privileged process to setns() to a whole 
different set of namespaces and still be audited with that initial 
container id ?


--
Linux-audit mailing list
Linux-audit@redhat.com
https://www.redhat.com/mailman/listinfo/linux-audit

Re: [RFC PATCH V1 01/12] audit: add container id

2018-04-18 Thread Stefan Berger


On 03/15/2018 11:58 PM, Richard Guy Briggs wrote:

On 2018-03-15 16:27, Stefan Berger wrote:

On 03/01/2018 02:41 PM, Richard Guy Briggs wrote:

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

This is a write from the container orchestrator task to a proc entry of
the form /proc/PID/containerid where PID is the process ID of the newly
created task that is to become the first task in a container, or an
additional task added to a container.

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

This will produce a record such as this:
type=UNKNOWN[1333] msg=audit(1519903238.968:261): op=set pid=596 uid=0 
subj=unconfined_u:unconfined_r:unconfined_t:s0-s0:c0.c1023 auid=0 tty=pts0 
ses=1 opid=596 old-contid=18446744073709551615 contid=123455 res=0

The "op" field indicates an initial set.  The "pid" to "ses" fields are
the orchestrator while the "opid" field is the object's PID, the process
being "contained".  Old and new container ID values are given in the
"contid" fields, while res indicates its success.

It is not permitted to self-set, unset or re-set the container ID.  A
child inherits its parent's container ID, but then can be set only once
after.

See: https://github.com/linux-audit/audit-kernel/issues/32


   /* audit_rule_data supports filter rules with both integer and string
* fields.  It corresponds with AUDIT_ADD_RULE, AUDIT_DEL_RULE and
diff --git a/kernel/auditsc.c b/kernel/auditsc.c
index 4e0a4ac..0ee1e59 100644
--- a/kernel/auditsc.c
+++ b/kernel/auditsc.c
@@ -2073,6 +2073,92 @@ int audit_set_loginuid(kuid_t loginuid)
return rc;
   }

+static int audit_set_containerid_perm(struct task_struct *task, u64 
containerid)
+{
+   struct task_struct *parent;
+   u64 pcontainerid, ccontainerid;
+   pid_t ppid;
+
+   /* Don't allow to set our own containerid */
+   if (current == task)
+   return -EPERM;
+   /* Don't allow the containerid to be unset */
+   if (!cid_valid(containerid))
+   return -EINVAL;
+   /* if we don't have caps, reject */
+   if (!capable(CAP_AUDIT_CONTROL))
+   return -EPERM;
+   /* if containerid is unset, allow */
+   if (!audit_containerid_set(task))
+   return 0;

I am wondering whether there should be a check for the target process that
will receive the containerid to not have CAP_SYS_ADMIN that would otherwise
allow it to arbitrarily unshare()/clone() and leave the set of namespaces
that may make up the container whose containerid we assign here?

This is a reasonable question.  This has been debated and I understood
the conclusion was that without a clear definition of a "container", the
task still remains in that container that just now has more
sub-namespaces (in the case of hierarchical namespaces), we don't want
to restrict it in such a way and that allows it to create nested
containers.  I see setns being more problematic if it could switch to
another existing namespace that was set up by the orchestrator for a
different container.  The coming v2 patchset acknowledges this situation
with the network namespace being potentially shared by multiple
containers.


Are you going to post v2 soon? We would like to build on top of it for 
IMA namespacing and auditing inside of IMA namespaces.


   Stefan

--
Linux-audit mailing list
Linux-audit@redhat.com
https://www.redhat.com/mailman/listinfo/linux-audit

Re: [RFC PATCH V1 01/12] audit: add container id

2018-04-18 Thread Richard Guy Briggs

On 2018-04-18 15:39, Stefan Berger wrote:
> On 04/18/2018 03:23 PM, Richard Guy Briggs wrote:
> > On 2018-04-18 14:45, Stefan Berger wrote:
> > > On 03/15/2018 11:58 PM, Richard Guy Briggs wrote:
> > > > On 2018-03-15 16:27, Stefan Berger wrote:
> > > > > On 03/01/2018 02:41 PM, Richard Guy Briggs wrote:
> > > > > > Implement the proc fs write to set the audit container ID of a 
> > > > > > process,
> > > > > > emitting an AUDIT_CONTAINER record to document the event.
> > > > > > 
> > > > > > This is a write from the container orchestrator task to a proc 
> > > > > > entry of
> > > > > > the form /proc/PID/containerid where PID is the process ID of the 
> > > > > > newly
> > > > > > created task that is to become the first task in a container, or an
> > > > > > additional task added to a container.
> > > > > > 
> > > > > > The write expects up to a u64 value (unset: 18446744073709551615).
> > > > > > 
> > > > > > This will produce a record such as this:
> > > > > > type=UNKNOWN[1333] msg=audit(1519903238.968:261): op=set pid=596 
> > > > > > uid=0 subj=unconfined_u:unconfined_r:unconfined_t:s0-s0:c0.c1023 
> > > > > > auid=0 tty=pts0 ses=1 opid=596 old-contid=18446744073709551615 
> > > > > > contid=123455 res=0
> > > > > > 
> > > > > > The "op" field indicates an initial set.  The "pid" to "ses" fields 
> > > > > > are
> > > > > > the orchestrator while the "opid" field is the object's PID, the 
> > > > > > process
> > > > > > being "contained".  Old and new container ID values are given in the
> > > > > > "contid" fields, while res indicates its success.
> > > > > > 
> > > > > > It is not permitted to self-set, unset or re-set the container ID.  
> > > > > > A
> > > > > > child inherits its parent's container ID, but then can be set only 
> > > > > > once
> > > > > > after.
> > > > > > 
> > > > > > See: https://github.com/linux-audit/audit-kernel/issues/32
> > > > > > 
> > > > > > 
> > > > > > /* audit_rule_data supports filter rules with both integer and 
> > > > > > string
> > > > > >  * fields.  It corresponds with AUDIT_ADD_RULE, AUDIT_DEL_RULE 
> > > > > > and
> > > > > > diff --git a/kernel/auditsc.c b/kernel/auditsc.c
> > > > > > index 4e0a4ac..0ee1e59 100644
> > > > > > --- a/kernel/auditsc.c
> > > > > > +++ b/kernel/auditsc.c
> > > > > > @@ -2073,6 +2073,92 @@ int audit_set_loginuid(kuid_t loginuid)
> > > > > > return rc;
> > > > > > }
> > > > > > 
> > > > > > +static int audit_set_containerid_perm(struct task_struct *task, 
> > > > > > u64 containerid)
> > > > > > +{
> > > > > > +   struct task_struct *parent;
> > > > > > +   u64 pcontainerid, ccontainerid;
> > > > > > +   pid_t ppid;
> > > > > > +
> > > > > > +   /* Don't allow to set our own containerid */
> > > > > > +   if (current == task)
> > > > > > +   return -EPERM;
> > > > > > +   /* Don't allow the containerid to be unset */
> > > > > > +   if (!cid_valid(containerid))
> > > > > > +   return -EINVAL;
> > > > > > +   /* if we don't have caps, reject */
> > > > > > +   if (!capable(CAP_AUDIT_CONTROL))
> > > > > > +   return -EPERM;
> > > > > > +   /* if containerid is unset, allow */
> > > > > > +   if (!audit_containerid_set(task))
> > > > > > +   return 0;
> > > > > I am wondering whether there should be a check for the target process 
> > > > > that
> > > > > will receive the containerid to not have CAP_SYS_ADMIN that would 
> > > > > otherwise
> > > > > allow it to arbitrarily unshare()/clone() and leave the set of 
> > > > > namespaces
> > > > > that may make up the container whose containerid we assign here?
> > > > This is a reasonable question.  This has been debated and I understood
> > > > the conclusion was that without a clear definition of a "container", the
> > > > task still remains in that container that just now has more
> > > > sub-namespaces (in the case of hierarchical namespaces), we don't want
> > > > to restrict it in such a way and that allows it to create nested
> > > > containers.  I see setns being more problematic if it could switch to
> > > > another existing namespace that was set up by the orchestrator for a
> > > > different container.  The coming v2 patchset acknowledges this situation
> > > > with the network namespace being potentially shared by multiple
> > > > containers.
> > > Are you going to post v2 soon? We would like to build on top of it for IMA
> > > namespacing and auditing inside of IMA namespaces.
> > I don't know if it addresses your specific needs, but V2 was posted on
> > March 16th along with userspace patches:
> > https://www.redhat.com/archives/linux-audit/2018-March/msg00110.html
> > https://www.redhat.com/archives/linux-audit/2018-March/msg00124.html
> > 
> > V3 is pending.
> Thanks. I hadn't actually looked at primarily due to the ghak and ghau in
> the title. Whatever these may mean.

They are Github issue numbers:
GHAK: GitHub Audit Kernel
GHAU: GitHub Audit Userspace
GHAD: GitHub Audit Documentation
GHAT: GitHub

Re: [RFC PATCH V1 01/12] audit: add container id

2018-04-18 Thread Richard Guy Briggs

On 2018-04-18 14:45, Stefan Berger wrote:
> On 03/15/2018 11:58 PM, Richard Guy Briggs wrote:
> > On 2018-03-15 16:27, Stefan Berger wrote:
> > > On 03/01/2018 02:41 PM, Richard Guy Briggs wrote:
> > > > Implement the proc fs write to set the audit container ID of a process,
> > > > emitting an AUDIT_CONTAINER record to document the event.
> > > > 
> > > > This is a write from the container orchestrator task to a proc entry of
> > > > the form /proc/PID/containerid where PID is the process ID of the newly
> > > > created task that is to become the first task in a container, or an
> > > > additional task added to a container.
> > > > 
> > > > The write expects up to a u64 value (unset: 18446744073709551615).
> > > > 
> > > > This will produce a record such as this:
> > > > type=UNKNOWN[1333] msg=audit(1519903238.968:261): op=set pid=596 uid=0 
> > > > subj=unconfined_u:unconfined_r:unconfined_t:s0-s0:c0.c1023 auid=0 
> > > > tty=pts0 ses=1 opid=596 old-contid=18446744073709551615 contid=123455 
> > > > res=0
> > > > 
> > > > The "op" field indicates an initial set.  The "pid" to "ses" fields are
> > > > the orchestrator while the "opid" field is the object's PID, the process
> > > > being "contained".  Old and new container ID values are given in the
> > > > "contid" fields, while res indicates its success.
> > > > 
> > > > It is not permitted to self-set, unset or re-set the container ID.  A
> > > > child inherits its parent's container ID, but then can be set only once
> > > > after.
> > > > 
> > > > See: https://github.com/linux-audit/audit-kernel/issues/32
> > > > 
> > > > 
> > > >/* audit_rule_data supports filter rules with both integer and string
> > > > * fields.  It corresponds with AUDIT_ADD_RULE, AUDIT_DEL_RULE and
> > > > diff --git a/kernel/auditsc.c b/kernel/auditsc.c
> > > > index 4e0a4ac..0ee1e59 100644
> > > > --- a/kernel/auditsc.c
> > > > +++ b/kernel/auditsc.c
> > > > @@ -2073,6 +2073,92 @@ int audit_set_loginuid(kuid_t loginuid)
> > > > return rc;
> > > >}
> > > > 
> > > > +static int audit_set_containerid_perm(struct task_struct *task, u64 
> > > > containerid)
> > > > +{
> > > > +   struct task_struct *parent;
> > > > +   u64 pcontainerid, ccontainerid;
> > > > +   pid_t ppid;
> > > > +
> > > > +   /* Don't allow to set our own containerid */
> > > > +   if (current == task)
> > > > +   return -EPERM;
> > > > +   /* Don't allow the containerid to be unset */
> > > > +   if (!cid_valid(containerid))
> > > > +   return -EINVAL;
> > > > +   /* if we don't have caps, reject */
> > > > +   if (!capable(CAP_AUDIT_CONTROL))
> > > > +   return -EPERM;
> > > > +   /* if containerid is unset, allow */
> > > > +   if (!audit_containerid_set(task))
> > > > +   return 0;
> > > I am wondering whether there should be a check for the target process that
> > > will receive the containerid to not have CAP_SYS_ADMIN that would 
> > > otherwise
> > > allow it to arbitrarily unshare()/clone() and leave the set of namespaces
> > > that may make up the container whose containerid we assign here?
> > This is a reasonable question.  This has been debated and I understood
> > the conclusion was that without a clear definition of a "container", the
> > task still remains in that container that just now has more
> > sub-namespaces (in the case of hierarchical namespaces), we don't want
> > to restrict it in such a way and that allows it to create nested
> > containers.  I see setns being more problematic if it could switch to
> > another existing namespace that was set up by the orchestrator for a
> > different container.  The coming v2 patchset acknowledges this situation
> > with the network namespace being potentially shared by multiple
> > containers.
> 
> Are you going to post v2 soon? We would like to build on top of it for IMA
> namespacing and auditing inside of IMA namespaces.

I don't know if it addresses your specific needs, but V2 was posted on
March 16th along with userspace patches:
https://www.redhat.com/archives/linux-audit/2018-March/msg00110.html
https://www.redhat.com/archives/linux-audit/2018-March/msg00124.html

V3 is pending.

>Stefan

- RGB

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

--
Linux-audit mailing list
Linux-audit@redhat.com
https://www.redhat.com/mailman/listinfo/linux-audit

Re: [RFC PATCH ghak32 V2 02/13] audit: check children and threading before allowing containerid

2018-04-18 Thread Paul Moore

On Fri, Mar 16, 2018 at 5:00 AM, Richard Guy Briggs  wrote:
> Check if a task has existing children or co-threads and refuse to set
> the container ID if either are present.  Failure to check this could
> permit games where a child scratches its parent's back to work around
> inheritance and double-setting policy.
>
> Signed-off-by: Richard Guy Briggs 
> ---
>  kernel/auditsc.c | 4 
>  1 file changed, 4 insertions(+)

I would just include this in patch 1/2 as I can't think of world where
we wouldn't this check.

> diff --git a/kernel/auditsc.c b/kernel/auditsc.c
> index 29c8482..a6b0a52 100644
> --- a/kernel/auditsc.c
> +++ b/kernel/auditsc.c
> @@ -2087,6 +2087,10 @@ static int audit_set_containerid_perm(struct 
> task_struct *task, u64 containerid)
> /* if we don't have caps, reject */
> if (!capable(CAP_AUDIT_CONTROL))
> return -EPERM;
> +   /* if task has children or is not single-threaded, deny */
> +   if (!list_empty(>children) ||
> +   !(thread_group_leader(task) && thread_group_empty(task)))
> +   return -EPERM;
> /* if containerid is unset, allow */
> if (!audit_containerid_set(task))
> return 0;
> --
> 1.8.3.1

-- 
paul moore
www.paul-moore.com

--
Linux-audit mailing list
Linux-audit@redhat.com
https://www.redhat.com/mailman/listinfo/linux-audit

Re: [RFC PATCH ghak32 V2 04/13] audit: add containerid filtering

2018-04-18 Thread Paul Moore

On Fri, Mar 16, 2018 at 5:00 AM, Richard Guy Briggs  wrote:
> Implement container ID filtering using the AUDIT_CONTAINERID field name
> to send an 8-character string representing a u64 since the value field
> is only u32.
>
> Sending it as two u32 was considered, but gathering and comparing two
> fields was more complex.

My only worry here is that you aren't really sending a string in the
ASCII sense, you are sending an 8 byte buffer (that better be NUL
terminated) that happens to be an unsigned 64-bit integer.  To be
clear, I'm okay with that (it's protected by AUDIT_CONTAINERID), and
the code is okay with that, I just want us to pause for a minute and
make sure that is an okay thing to do long term.

> The feature indicator is AUDIT_FEATURE_BITMAP_CONTAINERID_FILTER.
>
> This requires support from userspace to be useful.
> See: https://github.com/linux-audit/audit-userspace/issues/40
> Signed-off-by: Richard Guy Briggs 
> ---
>  include/linux/audit.h  |  1 +
>  include/uapi/linux/audit.h |  5 -
>  kernel/audit.h |  1 +
>  kernel/auditfilter.c   | 47 
> ++
>  kernel/auditsc.c   |  3 +++
>  5 files changed, 56 insertions(+), 1 deletion(-)
>
> diff --git a/include/linux/audit.h b/include/linux/audit.h
> index 3acbe9d..f10ca1b 100644
> --- a/include/linux/audit.h
> +++ b/include/linux/audit.h
> @@ -76,6 +76,7 @@ struct audit_field {
> u32 type;
> union {
> u32 val;
> +   u64 val64;
> kuid_t  uid;
> kgid_t  gid;
> struct {
> diff --git a/include/uapi/linux/audit.h b/include/uapi/linux/audit.h
> index e83ccbd..8443a8f 100644
> --- a/include/uapi/linux/audit.h
> +++ b/include/uapi/linux/audit.h
> @@ -262,6 +262,7 @@
>  #define AUDIT_LOGINUID_SET 24
>  #define AUDIT_SESSIONID25  /* Session ID */
>  #define AUDIT_FSTYPE   26  /* FileSystem Type */
> +#define AUDIT_CONTAINERID  27  /* Container ID */
>
> /* These are ONLY useful when checking
>  * at syscall exit time (AUDIT_AT_EXIT). */
> @@ -342,6 +343,7 @@ enum {
>  #define AUDIT_FEATURE_BITMAP_SESSIONID_FILTER  0x0010
>  #define AUDIT_FEATURE_BITMAP_LOST_RESET0x0020
>  #define AUDIT_FEATURE_BITMAP_FILTER_FS 0x0040
> +#define AUDIT_FEATURE_BITMAP_CONTAINERID_FILTER0x0080
>
>  #define AUDIT_FEATURE_BITMAP_ALL (AUDIT_FEATURE_BITMAP_BACKLOG_LIMIT | \
>   AUDIT_FEATURE_BITMAP_BACKLOG_WAIT_TIME | \
> @@ -349,7 +351,8 @@ enum {
>   AUDIT_FEATURE_BITMAP_EXCLUDE_EXTEND | \
>   AUDIT_FEATURE_BITMAP_SESSIONID_FILTER | \
>   AUDIT_FEATURE_BITMAP_LOST_RESET | \
> - AUDIT_FEATURE_BITMAP_FILTER_FS)
> + AUDIT_FEATURE_BITMAP_FILTER_FS | \
> + AUDIT_FEATURE_BITMAP_CONTAINERID_FILTER)
>
>  /* deprecated: AUDIT_VERSION_* */
>  #define AUDIT_VERSION_LATEST   AUDIT_FEATURE_BITMAP_ALL
> diff --git a/kernel/audit.h b/kernel/audit.h
> index 214e149..aaa651a 100644
> --- a/kernel/audit.h
> +++ b/kernel/audit.h
> @@ -234,6 +234,7 @@ static inline int audit_hash_ino(u32 ino)
>
>  extern int audit_match_class(int class, unsigned syscall);
>  extern int audit_comparator(const u32 left, const u32 op, const u32 right);
> +extern int audit_comparator64(const u64 left, const u32 op, const u64 right);
>  extern int audit_uid_comparator(kuid_t left, u32 op, kuid_t right);
>  extern int audit_gid_comparator(kgid_t left, u32 op, kgid_t right);
>  extern int parent_len(const char *path);
> diff --git a/kernel/auditfilter.c b/kernel/auditfilter.c
> index d7a807e..c4c8746 100644
> --- a/kernel/auditfilter.c
> +++ b/kernel/auditfilter.c
> @@ -410,6 +410,7 @@ static int audit_field_valid(struct audit_entry *entry, 
> struct audit_field *f)
> /* FALL THROUGH */
> case AUDIT_ARCH:
> case AUDIT_FSTYPE:
> +   case AUDIT_CONTAINERID:
> if (f->op != Audit_not_equal && f->op != Audit_equal)
> return -EINVAL;
> break;
> @@ -584,6 +585,14 @@ static struct audit_entry *audit_data_to_entry(struct 
> audit_rule_data *data,
> }
> entry->rule.exe = audit_mark;
> break;
> +   case AUDIT_CONTAINERID:
> +   if (f->val != sizeof(u64))
> +   goto exit_free;
> +   str = audit_unpack_string(, , f->val);
> +   if (IS_ERR(str))
> +   goto exit_free;
> +   f->val64

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

2018-04-18 Thread Paul Moore

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

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

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

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

Re: [RFC PATCH ghak32 V2 05/13] audit: add containerid support for ptrace and signals

2018-04-18 Thread Paul Moore

On Fri, Mar 16, 2018 at 5:00 AM, Richard Guy Briggs  wrote:
> Add container ID support to ptrace and signals.  In particular, the "op"
> field provides a way to label the auxiliary record to which it is
> associated.
>
> Signed-off-by: Richard Guy Briggs 
> ---
>  include/linux/audit.h | 16 +++-
>  kernel/audit.c| 12 
>  kernel/audit.h|  2 ++
>  kernel/auditsc.c  | 19 +++
>  4 files changed, 36 insertions(+), 13 deletions(-)

...

> diff --git a/kernel/audit.c b/kernel/audit.c
> index a12f21f..b238be5 100644
> --- a/kernel/audit.c
> +++ b/kernel/audit.c
> @@ -142,6 +142,7 @@ struct audit_net {
>  kuid_t audit_sig_uid = INVALID_UID;
>  pid_t  audit_sig_pid = -1;
>  u32audit_sig_sid = 0;
> +u64audit_sig_cid = INVALID_CID;
>
>  /* Records can be lost in several ways:
> 0) [suppressed in audit_alloc]
> @@ -1438,6 +1439,7 @@ static int audit_receive_msg(struct sk_buff *skb, 
> struct nlmsghdr *nlh)
> memcpy(sig_data->ctx, ctx, len);
> security_release_secctx(ctx, len);
> }
> +   sig_data->cid = audit_sig_cid;
> audit_send_reply(skb, seq, AUDIT_SIGNAL_INFO, 0, 0,
>  sig_data, sizeof(*sig_data) + len);
> kfree(sig_data);
> @@ -2051,20 +2053,22 @@ void audit_log_session_info(struct audit_buffer *ab)
>
>  /*
>   * audit_log_container_info - report container info
> - * @tsk: task to be recorded
>   * @context: task or local context for record
> + * @op: containerid string description
> + * @containerid: container ID to report
>   */
> -int audit_log_container_info(struct task_struct *tsk, struct audit_context 
> *context)
> +int audit_log_container_info(struct audit_context *context,
> + char *op, u64 containerid)
>  {
> struct audit_buffer *ab;
>
> -   if (!audit_containerid_set(tsk))
> +   if (!cid_valid(containerid))
> return 0;
> /* Generate AUDIT_CONTAINER_INFO with container ID */
> ab = audit_log_start(context, GFP_KERNEL, AUDIT_CONTAINER_INFO);
> if (!ab)
> return -ENOMEM;
> -   audit_log_format(ab, "contid=%llu", audit_get_containerid(tsk));
> +   audit_log_format(ab, "op=%s contid=%llu", op, containerid);
> audit_log_end(ab);
> return 0;
>  }

Let's get these changes into the first patch where
audit_log_container_info() is defined.  Why?  This inserts a new field
into the record which is a no-no.  Yes, it is one single patchset, but
they are still separate patches and who knows which patches a given
distribution and/or tree may decide to backport.

> diff --git a/kernel/auditsc.c b/kernel/auditsc.c
> index 2bba324..2932ef1 100644
> --- a/kernel/auditsc.c
> +++ b/kernel/auditsc.c
> @@ -113,6 +113,7 @@ struct audit_aux_data_pids {
> kuid_t  target_uid[AUDIT_AUX_PIDS];
> unsigned inttarget_sessionid[AUDIT_AUX_PIDS];
> u32 target_sid[AUDIT_AUX_PIDS];
> +   u64 target_cid[AUDIT_AUX_PIDS];
> chartarget_comm[AUDIT_AUX_PIDS][TASK_COMM_LEN];
> int pid_count;
>  };
> @@ -1422,21 +1423,27 @@ static void audit_log_exit(struct audit_context 
> *context, struct task_struct *ts
> 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++) {
> +   char axsn[sizeof("aux0xN ")];
> +
> +   sprintf(axsn, "aux0x%x", i);
> if (audit_log_pid_context(context, axs->target_pid[i],
>   axs->target_auid[i],
>   axs->target_uid[i],
>   axs->target_sessionid[i],
>   axs->target_sid[i],
> - axs->target_comm[i]))
> + axs->target_comm[i])
> +   && audit_log_container_info(context, axsn, 
> axs->target_cid[i]))

Shouldn't this be an OR instead of an AND?

> call_panic = 1;
> +   }
> }
>
> if (context->target_pid &&
> audit_log_pid_context(context, context->target_pid,
>   context->target_auid, context->target_uid,
>   context->target_sessionid,
> - context->target_sid, context->target_comm))
> + context->target_sid, context->target_comm)
> +   &&

Re: [RFC PATCH ghak32 V2 06/13] audit: add support for non-syscall auxiliary records

2018-04-18 Thread Paul Moore

On Fri, Mar 16, 2018 at 5:00 AM, Richard Guy Briggs  wrote:
> Standalone audit records have the timestamp and serial number generated
> on the fly and as such are unique, making them standalone.  This new
> function audit_alloc_local() generates a local audit context that will
> be used only for a standalone record and its auxiliary record(s).  The
> context is discarded immediately after the local associated records are
> produced.
>
> Signed-off-by: Richard Guy Briggs 
> ---
>  include/linux/audit.h |  8 
>  kernel/auditsc.c  | 20 +++-
>  2 files changed, 27 insertions(+), 1 deletion(-)
>
> diff --git a/include/linux/audit.h b/include/linux/audit.h
> index ed16bb6..c0b83cb 100644
> --- a/include/linux/audit.h
> +++ b/include/linux/audit.h
> @@ -227,7 +227,9 @@ static inline int audit_log_container_info(struct 
> audit_context *context,
>  /* These are defined in auditsc.c */
> /* Public API */
>  extern int  audit_alloc(struct task_struct *task);
> +extern struct audit_context *audit_alloc_local(void);
>  extern void __audit_free(struct task_struct *task);
> +extern void audit_free_context(struct audit_context *context);
>  extern void __audit_syscall_entry(int major, unsigned long a0, unsigned long 
> a1,
>   unsigned long a2, unsigned long a3);
>  extern void __audit_syscall_exit(int ret_success, long ret_value);
> @@ -472,6 +474,12 @@ static inline int audit_alloc(struct task_struct *task)
>  {
> return 0;
>  }
> +static inline struct audit_context *audit_alloc_local(void)
> +{
> +   return NULL;
> +}
> +static inline void audit_free_context(struct audit_context *context)
> +{ }
>  static inline void audit_free(struct task_struct *task)
>  { }
>  static inline void audit_syscall_entry(int major, unsigned long a0,
> diff --git a/kernel/auditsc.c b/kernel/auditsc.c
> index 2932ef1..7103d23 100644
> --- a/kernel/auditsc.c
> +++ b/kernel/auditsc.c
> @@ -959,8 +959,26 @@ int audit_alloc(struct task_struct *tsk)
> return 0;
>  }
>
> -static inline void audit_free_context(struct audit_context *context)
> +struct audit_context *audit_alloc_local(void)
>  {
> +   struct audit_context *context;
> +
> +   if (!audit_ever_enabled)
> +   return NULL; /* Return if not auditing. */
> +
> +   context = audit_alloc_context(AUDIT_RECORD_CONTEXT);
> +   if (!context)
> +   return NULL;
> +   context->serial = audit_serial();
> +   context->ctime = current_kernel_time64();
> +   context->in_syscall = 1;
> +   return context;
> +}
> +
> +inline void audit_free_context(struct audit_context *context)
> +{
> +   if (!context)
> +   return;
> audit_free_names(context);
> unroll_tree_refs(context, NULL, 0);
> free_tree_refs(context);

I'm reserving the option to comment on this idea further as I make my
way through the patchset, but audit_free_context() definitely
shouldn't be declared as an inline function.

-- 
paul moore
www.paul-moore.com

--
Linux-audit mailing list
Linux-audit@redhat.com
https://www.redhat.com/mailman/listinfo/linux-audit

Re: [RFC PATCH ghak32 V2 07/13] audit: add container aux record to watch/tree/mark

2018-04-18 Thread Paul Moore

On Fri, Mar 16, 2018 at 5:00 AM, Richard Guy Briggs  wrote:
> Add container ID auxiliary record to mark, watch and tree rule
> configuration standalone records.
>
> Signed-off-by: Richard Guy Briggs 
> ---
>  kernel/audit_fsnotify.c |  5 -
>  kernel/audit_tree.c |  5 -
>  kernel/audit_watch.c| 33 +++--
>  3 files changed, 27 insertions(+), 16 deletions(-)
>
> diff --git a/kernel/audit_fsnotify.c b/kernel/audit_fsnotify.c
> index 52f368b..18c110d 100644
> --- a/kernel/audit_fsnotify.c
> +++ b/kernel/audit_fsnotify.c
> @@ -124,10 +124,11 @@ static void audit_mark_log_rule_change(struct 
> audit_fsnotify_mark *audit_mark, c
>  {
> struct audit_buffer *ab;
> struct audit_krule *rule = audit_mark->rule;
> +   struct audit_context *context = audit_alloc_local();
>
> if (!audit_enabled)
> return;

Move the audit_alloc_local() after the audit_enabled check.

> -   ab = audit_log_start(NULL, GFP_NOFS, AUDIT_CONFIG_CHANGE);
> +   ab = audit_log_start(context, GFP_NOFS, AUDIT_CONFIG_CHANGE);
> if (unlikely(!ab))
> return;
> audit_log_format(ab, "auid=%u ses=%u op=%s",
> @@ -138,6 +139,8 @@ static void audit_mark_log_rule_change(struct 
> audit_fsnotify_mark *audit_mark, c
> audit_log_key(ab, rule->filterkey);
> audit_log_format(ab, " list=%d res=1", rule->listnr);
> audit_log_end(ab);
> +   audit_log_container_info(context, "config", 
> audit_get_containerid(current));
> +   audit_free_context(context);
>  }
>
>  void audit_remove_mark(struct audit_fsnotify_mark *audit_mark)
> diff --git a/kernel/audit_tree.c b/kernel/audit_tree.c
> index 67e6956..7c085be 100644
> --- a/kernel/audit_tree.c
> +++ b/kernel/audit_tree.c
> @@ -496,8 +496,9 @@ static int tag_chunk(struct inode *inode, struct 
> audit_tree *tree)
>  static void audit_tree_log_remove_rule(struct audit_krule *rule)
>  {
> struct audit_buffer *ab;
> +   struct audit_context *context = audit_alloc_local();

Sort of independent of the audit container ID work, but shouldn't we
have an audit_enabled check here?

> -   ab = audit_log_start(NULL, GFP_KERNEL, AUDIT_CONFIG_CHANGE);
> +   ab = audit_log_start(context, GFP_KERNEL, AUDIT_CONFIG_CHANGE);
> if (unlikely(!ab))
> return;
> audit_log_format(ab, "op=remove_rule");
> @@ -506,6 +507,8 @@ static void audit_tree_log_remove_rule(struct audit_krule 
> *rule)
> audit_log_key(ab, rule->filterkey);
> audit_log_format(ab, " list=%d res=1", rule->listnr);
> audit_log_end(ab);
> +   audit_log_container_info(context, "config", 
> audit_get_containerid(current));
> +   audit_free_context(context);
>  }
>
>  static void kill_rules(struct audit_tree *tree)
> diff --git a/kernel/audit_watch.c b/kernel/audit_watch.c
> index 9eb8b35..60d75a2 100644
> --- a/kernel/audit_watch.c
> +++ b/kernel/audit_watch.c
> @@ -238,20 +238,25 @@ static struct audit_watch *audit_dupe_watch(struct 
> audit_watch *old)
>
>  static void audit_watch_log_rule_change(struct audit_krule *r, struct 
> audit_watch *w, char *op)
>  {
> -   if (audit_enabled) {
> -   struct audit_buffer *ab;
> -   ab = audit_log_start(NULL, GFP_NOFS, AUDIT_CONFIG_CHANGE);
> -   if (unlikely(!ab))
> -   return;
> -   audit_log_format(ab, "auid=%u ses=%u op=%s",
> -from_kuid(_user_ns, 
> audit_get_loginuid(current)),
> -audit_get_sessionid(current), op);
> -   audit_log_format(ab, " path=");
> -   audit_log_untrustedstring(ab, w->path);
> -   audit_log_key(ab, r->filterkey);
> -   audit_log_format(ab, " list=%d res=1", r->listnr);
> -   audit_log_end(ab);
> -   }
> +   struct audit_buffer *ab;
> +   struct audit_context *context = audit_alloc_local();
> +
> +   if (!audit_enabled)
> +   return;

Same as above, do the allocation after the audit_enabled check.

> +   ab = audit_log_start(context, GFP_NOFS, AUDIT_CONFIG_CHANGE);
> +   if (unlikely(!ab))
> +   return;
> +   audit_log_format(ab, "auid=%u ses=%u op=%s",
> +from_kuid(_user_ns, 
> audit_get_loginuid(current)),
> +audit_get_sessionid(current), op);
> +   audit_log_format(ab, " path=");
> +   audit_log_untrustedstring(ab, w->path);
> +   audit_log_key(ab, r->filterkey);
> +   audit_log_format(ab, " list=%d res=1", r->listnr);
> +   audit_log_end(ab);
> +   audit_log_container_info(context, "config", 
> audit_get_containerid(current));
> +   audit_free_context(context);
>  }

-- 
paul moore
www.paul-moore.com

--
Linux-audit mailing list
Linux-audit@redhat.com
https://www.redhat.com/mailman/listinfo/linux-audit

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

2018-04-18 Thread Paul Moore

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

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

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

-- 
paul moore
www.paul-moore.com

--
Linux-audit mailing list
Linux-audit@redhat.com
https://www.redhat.com/mailman/listinfo/linux-audit

Re: [RFC PATCH ghak32 V2 10/13] audit: add containerid support for seccomp and anom_abend records

2018-04-18 Thread Paul Moore

On Fri, Mar 16, 2018 at 5:00 AM, Richard Guy Briggs  wrote:
> Add container ID auxiliary records to secure computing and abnormal end
> standalone records.
>
> Signed-off-by: Richard Guy Briggs 
> ---
>  kernel/auditsc.c | 10 --
>  1 file changed, 8 insertions(+), 2 deletions(-)
>
> diff --git a/kernel/auditsc.c b/kernel/auditsc.c
> index 7103d23..2f02ed9 100644
> --- a/kernel/auditsc.c
> +++ b/kernel/auditsc.c
> @@ -2571,6 +2571,7 @@ static void audit_log_task(struct audit_buffer *ab)
>  void audit_core_dumps(long signr)
>  {
> struct audit_buffer *ab;
> +   struct audit_context *context = audit_alloc_local();

Looking quickly at do_coredump() I *believe* we can use current here.

> if (!audit_enabled)
> return;
> @@ -2578,19 +2579,22 @@ void audit_core_dumps(long signr)
> if (signr == SIGQUIT)   /* don't care for those */
> return;
>
> -   ab = audit_log_start(NULL, GFP_KERNEL, AUDIT_ANOM_ABEND);
> +   ab = audit_log_start(context, GFP_KERNEL, AUDIT_ANOM_ABEND);
> if (unlikely(!ab))
> return;
> audit_log_task(ab);
> audit_log_format(ab, " sig=%ld res=1", signr);
> audit_log_end(ab);
> +   audit_log_container_info(context, "abend", 
> audit_get_containerid(current));
> +   audit_free_context(context);
>  }
>
>  void __audit_seccomp(unsigned long syscall, long signr, int code)
>  {
> struct audit_buffer *ab;
> +   struct audit_context *context = audit_alloc_local();

We can definitely use current here.

> -   ab = audit_log_start(NULL, GFP_KERNEL, AUDIT_SECCOMP);
> +   ab = audit_log_start(context, GFP_KERNEL, AUDIT_SECCOMP);
> if (unlikely(!ab))
> return;
> audit_log_task(ab);
> @@ -2598,6 +2602,8 @@ void __audit_seccomp(unsigned long syscall, long signr, 
> int code)
>  signr, syscall_get_arch(), syscall,
>  in_compat_syscall(), KSTK_EIP(current), code);
> audit_log_end(ab);
> +   audit_log_container_info(context, "seccomp", 
> audit_get_containerid(current));
> +   audit_free_context(context);
>  }
>
>  struct list_head *audit_killed_trees(void)

-- 
paul moore
www.paul-moore.com

--
Linux-audit mailing list
Linux-audit@redhat.com
https://www.redhat.com/mailman/listinfo/linux-audit

Re: [RFC PATCH ghak32 V2 11/13] audit: add support for containerid to network namespaces

2018-04-18 Thread Paul Moore

On Fri, Mar 16, 2018 at 5:00 AM, Richard Guy Briggs  wrote:
> Audit events could happen in a network namespace outside of a task
> context due to packets received from the net that trigger an auditing
> rule prior to being associated with a running task.  The network
> namespace could in use by multiple containers by association to the
> tasks in that network namespace.  We still want a way to attribute
> these events to any potential containers.  Keep a list per network
> namespace to track these container identifiiers.
>
> Add/increment the container identifier on:
> - initial setting of the container id via /proc
> - clone/fork call that inherits a container identifier
> - unshare call that inherits a container identifier
> - setns call that inherits a container identifier
> Delete/decrement the container identifier on:
> - an inherited container id dropped when child set
> - process exit
> - unshare call that drops a net namespace
> - setns call that drops a net namespace
>
> See: https://github.com/linux-audit/audit-kernel/issues/32
> See: https://github.com/linux-audit/audit-testsuite/issues/64
> Signed-off-by: Richard Guy Briggs 
> ---
>  include/linux/audit.h   |  7 +++
>  include/net/net_namespace.h | 12 
>  kernel/auditsc.c|  9 ++---
>  kernel/nsproxy.c|  6 ++
>  net/core/net_namespace.c| 45 
> +
>  5 files changed, 76 insertions(+), 3 deletions(-)

...

> diff --git a/include/net/net_namespace.h b/include/net/net_namespace.h
> index 0490084..343a428 100644
> --- a/include/net/net_namespace.h
> +++ b/include/net/net_namespace.h
> @@ -33,6 +33,7 @@
>  #include 
>  #include 
>  #include 
> +#include 
>
>  struct user_namespace;
>  struct proc_dir_entry;
> @@ -150,6 +151,7 @@ struct net {
>  #endif
> struct sock *diag_nlsk;
> atomic_tfnhe_genid;
> +   struct list_headaudit_containerid;
>  } __randomize_layout;

We talked about this briefly off-list, you should be using audit_net
and the net_generic mechanism instead of this.

>  #include 
> @@ -301,6 +303,16 @@ static inline struct net *read_pnet(const possible_net_t 
> *pnet)
>  #define __net_initconst__initconst
>  #endif
>
> +#ifdef CONFIG_NET_NS
> +void net_add_audit_containerid(struct net *net, u64 containerid);
> +void net_del_audit_containerid(struct net *net, u64 containerid);
> +#else
> +static inline void net_add_audit_containerid(struct net *, u64)
> +{ }
> +static inline void net_del_audit_containerid(struct net *, u64)
> +{ }
> +#endif
> +
>  int peernet2id_alloc(struct net *net, struct net *peer);
>  int peernet2id(struct net *net, struct net *peer);
>  bool peernet_has_id(struct net *net, struct net *peer);
> diff --git a/kernel/auditsc.c b/kernel/auditsc.c
> index 2f02ed9..208da962 100644
> --- a/kernel/auditsc.c
> +++ b/kernel/auditsc.c
> @@ -75,6 +75,7 @@
>  #include 
>  #include 
>  #include 
> +#include 
>
>  #include "audit.h"
>
> @@ -2175,16 +2176,18 @@ static void audit_log_set_containerid(struct 
> task_struct *task, u64 oldcontainer
>   */
>  int audit_set_containerid(struct task_struct *task, u64 containerid)
>  {
> -   u64 oldcontainerid;
> +   u64 oldcontainerid = audit_get_containerid(task);
> int rc;
> -
> -   oldcontainerid = audit_get_containerid(task);
> +   struct net *net = task->nsproxy->net_ns;
>
> rc = audit_set_containerid_perm(task, containerid);
> if (!rc) {
> +   if (cid_valid(oldcontainerid))
> +   net_del_audit_containerid(net, oldcontainerid);

Using audit_net we can handle this internal to audit, which is a Good Thing.

> task_lock(task);
> task->containerid = containerid;
> task_unlock(task);
> +   net_add_audit_containerid(net, containerid);

Same.

> }
>
> audit_log_set_containerid(task, oldcontainerid, containerid, rc);
> diff --git a/kernel/nsproxy.c b/kernel/nsproxy.c
> index f6c5d33..d9f1090 100644
> --- a/kernel/nsproxy.c
> +++ b/kernel/nsproxy.c
> @@ -140,6 +140,7 @@ int copy_namespaces(unsigned long flags, struct 
> task_struct *tsk)
> struct nsproxy *old_ns = tsk->nsproxy;
> struct user_namespace *user_ns = task_cred_xxx(tsk, user_ns);
> struct nsproxy *new_ns;
> +   u64 containerid = audit_get_containerid(tsk);
>
> if (likely(!(flags & (CLONE_NEWNS | CLONE_NEWUTS | CLONE_NEWIPC |
>   CLONE_NEWPID | CLONE_NEWNET |
> @@ -167,6 +168,7 @@ int copy_namespaces(unsigned long flags, struct 
> task_struct *tsk)
> return  PTR_ERR(new_ns);
>
> tsk->nsproxy = new_ns;
> +   net_add_audit_containerid(new_ns->net_ns, containerid);
> return 0;
>  }

Hopefully we can handle this in audit_net_init(), we just need to
figure out where we can get the correct task_struct for

Re: [RFC PATCH ghak32 V2 12/13] audit: NETFILTER_PKT: record each container ID associated with a netNS

2018-04-18 Thread Paul Moore

On Fri, Mar 16, 2018 at 5:00 AM, Richard Guy Briggs  wrote:
> Add container ID auxiliary record(s) to NETFILTER_PKT event standalone
> records.  Iterate through all potential container IDs associated with a
> network namespace.
>
> Signed-off-by: Richard Guy Briggs 
> ---
>  kernel/audit.c   |  1 +
>  kernel/auditsc.c |  2 ++
>  net/netfilter/xt_AUDIT.c | 15 ++-
>  3 files changed, 17 insertions(+), 1 deletion(-)
>
> diff --git a/kernel/audit.c b/kernel/audit.c
> index 08662b4..3c77e47 100644
> --- a/kernel/audit.c
> +++ b/kernel/audit.c
> @@ -2102,6 +2102,7 @@ int audit_log_container_info(struct audit_context 
> *context,
> audit_log_end(ab);
> return 0;
>  }
> +EXPORT_SYMBOL(audit_log_container_info);
>
>  void audit_log_key(struct audit_buffer *ab, char *key)
>  {
> diff --git a/kernel/auditsc.c b/kernel/auditsc.c
> index 208da962..af68d01 100644
> --- a/kernel/auditsc.c
> +++ b/kernel/auditsc.c
> @@ -975,6 +975,7 @@ struct audit_context *audit_alloc_local(void)
> context->in_syscall = 1;
> return context;
>  }
> +EXPORT_SYMBOL(audit_alloc_local);
>
>  inline void audit_free_context(struct audit_context *context)
>  {
> @@ -989,6 +990,7 @@ inline void audit_free_context(struct audit_context 
> *context)
> audit_proctitle_free(context);
> kfree(context);
>  }
> +EXPORT_SYMBOL(audit_free_context);
>
>  static int audit_log_pid_context(struct audit_context *context, pid_t pid,
>  kuid_t auid, kuid_t uid, unsigned int 
> sessionid,
> diff --git a/net/netfilter/xt_AUDIT.c b/net/netfilter/xt_AUDIT.c
> index c502419..edaa456 100644
> --- a/net/netfilter/xt_AUDIT.c
> +++ b/net/netfilter/xt_AUDIT.c
> @@ -71,10 +71,14 @@ static bool audit_ip6(struct audit_buffer *ab, struct 
> sk_buff *skb)
>  {
> struct audit_buffer *ab;
> int fam = -1;
> +   struct audit_context *context = audit_alloc_local();
> +   struct audit_containerid *cont;
> +   int i = 0;
> +   struct net *net;
>
> if (audit_enabled == 0)
> goto errout;

Do I need to say it?  I probably should ... the allocation should
happen after the audit_enabled check.

> -   ab = audit_log_start(NULL, GFP_ATOMIC, AUDIT_NETFILTER_PKT);
> +   ab = audit_log_start(context, GFP_ATOMIC, AUDIT_NETFILTER_PKT);
> if (ab == NULL)
> goto errout;
>
> @@ -104,7 +108,16 @@ static bool audit_ip6(struct audit_buffer *ab, struct 
> sk_buff *skb)
>
> audit_log_end(ab);
>
> +   net = sock_net(NETLINK_CB(skb).sk);
> +   list_for_each_entry(cont, >audit_containerid, list) {
> +   char buf[14];
> +
> +   sprintf(buf, "net%u", i++);
> +   audit_log_container_info(context, buf, cont->id);
> +   }

It seems like this could (should?) be hidden inside an audit function,
e.g. audit_log_net_containers() or something like that.

>  errout:
> +   audit_free_context(context);
> return XT_CONTINUE;
>  }

-- 
paul moore
www.paul-moore.com

--
Linux-audit mailing list
Linux-audit@redhat.com
https://www.redhat.com/mailman/listinfo/linux-audit

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

2018-04-18 Thread Casey Schaufler

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

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


--
Linux-audit mailing list
Linux-audit@redhat.com
https://www.redhat.com/mailman/listinfo/linux-audit

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

2018-04-18 Thread Casey Schaufler

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

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

Fair enough.

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

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

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

I have to keep the skills sharpened somehow!

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

--
Linux-audit mailing list
Linux-audit@redhat.com
https://www.redhat.com/mailman/listinfo/linux-audit

Re: [RFC PATCH ghak32 V2 09/13] audit: add containerid support for config/feature/user records

2018-04-18 Thread Paul Moore

On Fri, Mar 16, 2018 at 5:00 AM, Richard Guy Briggs  wrote:
> Add container ID auxiliary records to configuration change, feature set change
> and user generated standalone records.
>
> Signed-off-by: Richard Guy Briggs 
> ---
>  kernel/audit.c   | 50 --
>  kernel/auditfilter.c |  5 -
>  2 files changed, 44 insertions(+), 11 deletions(-)
>
> diff --git a/kernel/audit.c b/kernel/audit.c
> index b238be5..08662b4 100644
> --- a/kernel/audit.c
> +++ b/kernel/audit.c
> @@ -400,8 +400,9 @@ static int audit_log_config_change(char *function_name, 
> u32 new, u32 old,
>  {
> struct audit_buffer *ab;
> int rc = 0;
> +   struct audit_context *context = audit_alloc_local();

We should be able to use current->audit_context here right?  If we
can't for every caller, perhaps we pass an audit_context as an
argument and only allocate a local context when the passed
audit_context is NULL.

Also, if you're not comfortable always using current, just pass the
audit_context as you do with audit_log_common_recv_msg().

> -   ab = audit_log_start(NULL, GFP_KERNEL, AUDIT_CONFIG_CHANGE);
> +   ab = audit_log_start(context, GFP_KERNEL, AUDIT_CONFIG_CHANGE);
> if (unlikely(!ab))
> return rc;
> audit_log_format(ab, "%s=%u old=%u", function_name, new, old);
> @@ -411,6 +412,8 @@ static int audit_log_config_change(char *function_name, 
> u32 new, u32 old,
> allow_changes = 0; /* Something weird, deny request */
> audit_log_format(ab, " res=%d", allow_changes);
> audit_log_end(ab);
> +   audit_log_container_info(context, "config", 
> audit_get_containerid(current));
> +   audit_free_context(context);
> return rc;
>  }
>
> @@ -1058,7 +1061,8 @@ static int audit_netlink_ok(struct sk_buff *skb, u16 
> msg_type)
> return err;
>  }
>
> -static void audit_log_common_recv_msg(struct audit_buffer **ab, u16 msg_type)
> +static void audit_log_common_recv_msg(struct audit_context *context,
> + struct audit_buffer **ab, u16 msg_type)
>  {
> uid_t uid = from_kuid(_user_ns, current_uid());
> pid_t pid = task_tgid_nr(current);
> @@ -1068,7 +1072,7 @@ static void audit_log_common_recv_msg(struct 
> audit_buffer **ab, u16 msg_type)
> return;
> }
>
> -   *ab = audit_log_start(NULL, GFP_KERNEL, msg_type);
> +   *ab = audit_log_start(context, GFP_KERNEL, msg_type);
> if (unlikely(!*ab))
> return;
> audit_log_format(*ab, "pid=%d uid=%u", pid, uid);
> @@ -1097,11 +1101,12 @@ static void audit_log_feature_change(int which, u32 
> old_feature, u32 new_feature
>  u32 old_lock, u32 new_lock, int res)
>  {
> struct audit_buffer *ab;
> +   struct audit_context *context = audit_alloc_local();

So I know based on the other patch we are currently discussing that we
can use current here ...

> if (audit_enabled == AUDIT_OFF)
> return;
>
> -   ab = audit_log_start(NULL, GFP_KERNEL, AUDIT_FEATURE_CHANGE);
> +   ab = audit_log_start(context, GFP_KERNEL, AUDIT_FEATURE_CHANGE);
> if (!ab)
> return;
> audit_log_task_info(ab, current);
> @@ -1109,6 +1114,8 @@ static void audit_log_feature_change(int which, u32 
> old_feature, u32 new_feature
>  audit_feature_names[which], !!old_feature, 
> !!new_feature,
>  !!old_lock, !!new_lock, res);
> audit_log_end(ab);
> +   audit_log_container_info(context, "feature", 
> audit_get_containerid(current));
> +   audit_free_context(context);
>  }
>
>  static int audit_set_feature(struct sk_buff *skb)
> @@ -1337,13 +1344,15 @@ static int audit_receive_msg(struct sk_buff *skb, 
> struct nlmsghdr *nlh)
>
> err = audit_filter(msg_type, AUDIT_FILTER_USER);
> if (err == 1) { /* match or error */
> +   struct audit_context *context = audit_alloc_local();

I'm pretty sure we can use current here.

> err = 0;
> if (msg_type == AUDIT_USER_TTY) {
> err = tty_audit_push();
> if (err)
> break;
> }
> -   audit_log_common_recv_msg(, msg_type);
> +   audit_log_common_recv_msg(context, , msg_type);
> if (msg_type != AUDIT_USER_TTY)
> audit_log_format(ab, " msg='%.*s'",
>  AUDIT_MESSAGE_TEXT_MAX,
> @@ -1359,6 +1368,9 @@ static int audit_receive_msg(struct sk_buff *skb, 
> struct nlmsghdr *nlh)
> audit_log_n_untrustedstring(ab, data, size);
> }
>

Re: [RFC PATCH V1 01/12] audit: add container id

Re: [RFC PATCH V1 01/12] audit: add container id

Re: [RFC PATCH V1 01/12] audit: add container id

Re: [RFC PATCH V1 01/12] audit: add container id

Re: [RFC PATCH ghak32 V2 02/13] audit: check children and threading before allowing containerid

Re: [RFC PATCH ghak32 V2 04/13] audit: add containerid filtering

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

Re: [RFC PATCH ghak32 V2 05/13] audit: add containerid support for ptrace and signals

Re: [RFC PATCH ghak32 V2 06/13] audit: add support for non-syscall auxiliary records

Re: [RFC PATCH ghak32 V2 07/13] audit: add container aux record to watch/tree/mark

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

Re: [RFC PATCH ghak32 V2 10/13] audit: add containerid support for seccomp and anom_abend records

Re: [RFC PATCH ghak32 V2 11/13] audit: add support for containerid to network namespaces

Re: [RFC PATCH ghak32 V2 12/13] audit: NETFILTER_PKT: record each container ID associated with a netNS

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

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

Re: [RFC PATCH ghak32 V2 09/13] audit: add containerid support for config/feature/user records

17 matches

Site Navigation

Mail list logo

Footer information