[PATCH 1/1] audit: log binding and unbinding to netlink multicast

[PATCH 1/1] audit: log binding and unbinding to netlink multicast

[Steve Grubb]

Log information about programs connecting and disconnecting to the audit
netlink multicast socket. This is needed so that during investigations a
security officer can tell who or what had access to the audit trail. This
helps to meet the FAU_SAR.2 requirement for Common Criteria. Sample
event:

type=UNKNOWN[1332] msg=audit(1507924331.540:3): pid=1 uid=0
auid=4294967295 tty=(none) ses=4294967295 subj=kernel comm="systemd"
exe="/usr/lib/systemd/systemd" nlnk-grp=1 op=connect res=1

Signed-off-by: sgrubb <sgrubb@redhat.com>
---
 include/uapi/linux/audit.h |  1 +
 kernel/audit.c             | 48 ++++++++++++++++++++++++++++++++++++++++++----
 2 files changed, 45 insertions(+), 4 deletions(-)

diff --git a/include/uapi/linux/audit.h b/include/uapi/linux/audit.h
index 0714a66f0e0c..892e63d9f2c1 100644
--- a/include/uapi/linux/audit.h
+++ b/include/uapi/linux/audit.h
@@ -112,6 +112,7 @@
 #define AUDIT_FEATURE_CHANGE	1328	/* audit log listing feature changes */
 #define AUDIT_REPLACE		1329	/* Replace auditd if this packet unanswerd */
 #define AUDIT_KERN_MODULE	1330	/* Kernel Module events */
+#define AUDIT_EVENT_LISTENER	1332	/* Task joined multicast read socket */
 
 #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 1baabc9539b4..3d2461be83a8 100644
--- a/kernel/audit.c
+++ b/kernel/audit.c
@@ -1476,22 +1476,61 @@ static void audit_receive(struct sk_buff  *skb)
 	mutex_unlock(&audit_cmd_mutex);
 }
 
+/* Log information about who is connecting to the audit multicast socket */
+static void audit_log_multicast_bind(int group, const char *op, int err)
+{
+	const struct cred *cred;
+	struct tty_struct *tty;
+	char comm[sizeof(current->comm)];
+	struct audit_buffer *ab;
+
+	ab = audit_log_start(NULL, GFP_KERNEL, AUDIT_EVENT_LISTENER);
+	if (!ab)
+		return;
+
+	cred = current_cred();
+	tty = audit_get_tty(current);
+
+	audit_log_format(ab, "pid=%u uid=%u auid=%u tty=%s ses=%u",
+			task_pid_nr(current),
+			from_kuid(&init_user_ns, cred->uid),
+			from_kuid(&init_user_ns, audit_get_loginuid(current)),
+			tty ? tty_name(tty) : "(none)",
+			audit_get_sessionid(current));
+	audit_put_tty(tty);
+	audit_log_task_context(ab); /* subj= */
+	audit_log_format(ab, " comm=");
+	audit_log_untrustedstring(ab, get_task_comm(comm, current));
+	audit_log_d_path_exe(ab, current->mm); /* exe= */
+	audit_log_format(ab, " nlnk-grp=%d op=%s res=%d", group, op, !err);
+	audit_log_end(ab);
+}
+
 /* Run custom bind function on netlink socket group connect or bind requests. */
-static int audit_bind(struct net *net, int group)
+static int audit_multicast_bind(struct net *net, int group)
 {
+	int err = 0;
+
 	if (!capable(CAP_AUDIT_READ))
-		return -EPERM;
+		err = -EPERM;
+	audit_log_multicast_bind(group, "connect", err);
 
-	return 0;
+	return err;
+}
+
+static void audit_multicast_unbind(struct net *net, int group)
+{
+	audit_log_multicast_bind(group, "disconnect", 0);
 }
 
 static int __net_init audit_net_init(struct net *net)
 {
 	struct netlink_kernel_cfg cfg = {
 		.input	= audit_receive,
-		.bind	= audit_bind,
+		.bind	= audit_multicast_bind,
 		.flags	= NL_CFG_F_NONROOT_RECV,
 		.groups	= AUDIT_NLGRP_MAX,
+		.unbind = audit_multicast_unbind,
 	};
 
 	struct audit_net *aunet = net_generic(net, audit_net_id);
-- 
2.13.6

Re: [PATCH 1/1] audit: log binding and unbinding to netlink multicast

[Richard Guy Briggs]

On 2017-10-13 19:58, Steve Grubb wrote:
> Log information about programs connecting and disconnecting to the audit
> netlink multicast socket. This is needed so that during investigations a
> security officer can tell who or what had access to the audit trail. This
> helps to meet the FAU_SAR.2 requirement for Common Criteria. Sample
> event:
> 
> type=UNKNOWN[1332] msg=audit(1507924331.540:3): pid=1 uid=0
> auid=4294967295 tty=(none) ses=4294967295 subj=kernel comm="systemd"
> exe="/usr/lib/systemd/systemd" nlnk-grp=1 op=connect res=1
> 
> Signed-off-by: sgrubb <sgrubb@redhat.com>

Reviewed-by: Richard Guy Briggs <rgb@redhat.com>

> ---
>  include/uapi/linux/audit.h |  1 +
>  kernel/audit.c             | 48 ++++++++++++++++++++++++++++++++++++++++++----
>  2 files changed, 45 insertions(+), 4 deletions(-)
> 
> diff --git a/include/uapi/linux/audit.h b/include/uapi/linux/audit.h
> index 0714a66f0e0c..892e63d9f2c1 100644
> --- a/include/uapi/linux/audit.h
> +++ b/include/uapi/linux/audit.h
> @@ -112,6 +112,7 @@
>  #define AUDIT_FEATURE_CHANGE	1328	/* audit log listing feature changes */
>  #define AUDIT_REPLACE		1329	/* Replace auditd if this packet unanswerd */
>  #define AUDIT_KERN_MODULE	1330	/* Kernel Module events */
> +#define AUDIT_EVENT_LISTENER	1332	/* Task joined multicast read socket */

I assume this is waiting on another patch to be reviewed and merged...
IIRC the maintainer prefers to deal with that merge conflict rather than
accidentally leave a gap if the dependent patch doesn't get merged.

>  #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 1baabc9539b4..3d2461be83a8 100644
> --- a/kernel/audit.c
> +++ b/kernel/audit.c
> @@ -1476,22 +1476,61 @@ static void audit_receive(struct sk_buff  *skb)
>  	mutex_unlock(&audit_cmd_mutex);
>  }
>  
> +/* Log information about who is connecting to the audit multicast socket */
> +static void audit_log_multicast_bind(int group, const char *op, int err)
> +{
> +	const struct cred *cred;
> +	struct tty_struct *tty;
> +	char comm[sizeof(current->comm)];
> +	struct audit_buffer *ab;
> +
> +	ab = audit_log_start(NULL, GFP_KERNEL, AUDIT_EVENT_LISTENER);
> +	if (!ab)
> +		return;
> +
> +	cred = current_cred();
> +	tty = audit_get_tty(current);
> +
> +	audit_log_format(ab, "pid=%u uid=%u auid=%u tty=%s ses=%u",
> +			task_pid_nr(current),
> +			from_kuid(&init_user_ns, cred->uid),
> +			from_kuid(&init_user_ns, audit_get_loginuid(current)),
> +			tty ? tty_name(tty) : "(none)",
> +			audit_get_sessionid(current));
> +	audit_put_tty(tty);
> +	audit_log_task_context(ab); /* subj= */
> +	audit_log_format(ab, " comm=");
> +	audit_log_untrustedstring(ab, get_task_comm(comm, current));
> +	audit_log_d_path_exe(ab, current->mm); /* exe= */
> +	audit_log_format(ab, " nlnk-grp=%d op=%s res=%d", group, op, !err);
> +	audit_log_end(ab);
> +}
> +
>  /* Run custom bind function on netlink socket group connect or bind requests. */
> -static int audit_bind(struct net *net, int group)
> +static int audit_multicast_bind(struct net *net, int group)
>  {
> +	int err = 0;
> +
>  	if (!capable(CAP_AUDIT_READ))
> -		return -EPERM;
> +		err = -EPERM;
> +	audit_log_multicast_bind(group, "connect", err);
>  
> -	return 0;
> +	return err;
> +}
> +
> +static void audit_multicast_unbind(struct net *net, int group)
> +{
> +	audit_log_multicast_bind(group, "disconnect", 0);
>  }
>  
>  static int __net_init audit_net_init(struct net *net)
>  {
>  	struct netlink_kernel_cfg cfg = {
>  		.input	= audit_receive,
> -		.bind	= audit_bind,
> +		.bind	= audit_multicast_bind,
>  		.flags	= NL_CFG_F_NONROOT_RECV,
>  		.groups	= AUDIT_NLGRP_MAX,
> +		.unbind = audit_multicast_unbind,
>  	};
>  
>  	struct audit_net *aunet = net_generic(net, audit_net_id);
> -- 
> 2.13.6
> 
> 
> --
> Linux-audit mailing list
> Linux-audit@redhat.com
> https://www.redhat.com/mailman/listinfo/linux-audit

- 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

Re: [PATCH 1/1] audit: log binding and unbinding to netlink multicast

[Richard Guy Briggs]

On 2017-10-16 18:56, Richard Guy Briggs wrote:
> On 2017-10-13 19:58, Steve Grubb wrote:
> > Log information about programs connecting and disconnecting to the audit
> > netlink multicast socket. This is needed so that during investigations a
> > security officer can tell who or what had access to the audit trail. This
> > helps to meet the FAU_SAR.2 requirement for Common Criteria. Sample
> > event:
> > 
> > type=UNKNOWN[1332] msg=audit(1507924331.540:3): pid=1 uid=0
> > auid=4294967295 tty=(none) ses=4294967295 subj=kernel comm="systemd"
> > exe="/usr/lib/systemd/systemd" nlnk-grp=1 op=connect res=1

Please add the upstream issue tracker link to the commit description body:

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

> > Signed-off-by: sgrubb <sgrubb@redhat.com>
> 
> Reviewed-by: Richard Guy Briggs <rgb@redhat.com>
> 
> > ---
> >  include/uapi/linux/audit.h |  1 +
> >  kernel/audit.c             | 48 ++++++++++++++++++++++++++++++++++++++++++----
> >  2 files changed, 45 insertions(+), 4 deletions(-)
> > 
> > diff --git a/include/uapi/linux/audit.h b/include/uapi/linux/audit.h
> > index 0714a66f0e0c..892e63d9f2c1 100644
> > --- a/include/uapi/linux/audit.h
> > +++ b/include/uapi/linux/audit.h
> > @@ -112,6 +112,7 @@
> >  #define AUDIT_FEATURE_CHANGE	1328	/* audit log listing feature changes */
> >  #define AUDIT_REPLACE		1329	/* Replace auditd if this packet unanswerd */
> >  #define AUDIT_KERN_MODULE	1330	/* Kernel Module events */
> > +#define AUDIT_EVENT_LISTENER	1332	/* Task joined multicast read socket */
> 
> I assume this is waiting on another patch to be reviewed and merged...
> IIRC the maintainer prefers to deal with that merge conflict rather than
> accidentally leave a gap if the dependent patch doesn't get merged.
> 
> >  #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 1baabc9539b4..3d2461be83a8 100644
> > --- a/kernel/audit.c
> > +++ b/kernel/audit.c
> > @@ -1476,22 +1476,61 @@ static void audit_receive(struct sk_buff  *skb)
> >  	mutex_unlock(&audit_cmd_mutex);
> >  }
> >  
> > +/* Log information about who is connecting to the audit multicast socket */
> > +static void audit_log_multicast_bind(int group, const char *op, int err)
> > +{
> > +	const struct cred *cred;
> > +	struct tty_struct *tty;
> > +	char comm[sizeof(current->comm)];
> > +	struct audit_buffer *ab;
> > +
> > +	ab = audit_log_start(NULL, GFP_KERNEL, AUDIT_EVENT_LISTENER);
> > +	if (!ab)
> > +		return;
> > +
> > +	cred = current_cred();
> > +	tty = audit_get_tty(current);
> > +
> > +	audit_log_format(ab, "pid=%u uid=%u auid=%u tty=%s ses=%u",
> > +			task_pid_nr(current),
> > +			from_kuid(&init_user_ns, cred->uid),
> > +			from_kuid(&init_user_ns, audit_get_loginuid(current)),
> > +			tty ? tty_name(tty) : "(none)",
> > +			audit_get_sessionid(current));
> > +	audit_put_tty(tty);
> > +	audit_log_task_context(ab); /* subj= */
> > +	audit_log_format(ab, " comm=");
> > +	audit_log_untrustedstring(ab, get_task_comm(comm, current));
> > +	audit_log_d_path_exe(ab, current->mm); /* exe= */
> > +	audit_log_format(ab, " nlnk-grp=%d op=%s res=%d", group, op, !err);
> > +	audit_log_end(ab);
> > +}
> > +
> >  /* Run custom bind function on netlink socket group connect or bind requests. */
> > -static int audit_bind(struct net *net, int group)
> > +static int audit_multicast_bind(struct net *net, int group)
> >  {
> > +	int err = 0;
> > +
> >  	if (!capable(CAP_AUDIT_READ))
> > -		return -EPERM;
> > +		err = -EPERM;
> > +	audit_log_multicast_bind(group, "connect", err);
> >  
> > -	return 0;
> > +	return err;
> > +}
> > +
> > +static void audit_multicast_unbind(struct net *net, int group)
> > +{
> > +	audit_log_multicast_bind(group, "disconnect", 0);
> >  }
> >  
> >  static int __net_init audit_net_init(struct net *net)
> >  {
> >  	struct netlink_kernel_cfg cfg = {
> >  		.input	= audit_receive,
> > -		.bind	= audit_bind,
> > +		.bind	= audit_multicast_bind,
> >  		.flags	= NL_CFG_F_NONROOT_RECV,
> >  		.groups	= AUDIT_NLGRP_MAX,
> > +		.unbind = audit_multicast_unbind,
> >  	};
> >  
> >  	struct audit_net *aunet = net_generic(net, audit_net_id);
> > -- 
> > 2.13.6
> > 
> > 
> > --
> > Linux-audit mailing list
> > Linux-audit@redhat.com
> > https://www.redhat.com/mailman/listinfo/linux-audit
> 
> - 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

- 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

Re: [PATCH 1/1] audit: log binding and unbinding to netlink multicast

[Paul Moore]

On Fri, Oct 13, 2017 at 3:58 PM, Steve Grubb <sgrubb@redhat.com> wrote:
> Log information about programs connecting and disconnecting to the audit
> netlink multicast socket. This is needed so that during investigations a
> security officer can tell who or what had access to the audit trail. This
> helps to meet the FAU_SAR.2 requirement for Common Criteria. Sample
> event:
>
> type=UNKNOWN[1332] msg=audit(1507924331.540:3): pid=1 uid=0
> auid=4294967295 tty=(none) ses=4294967295 subj=kernel comm="systemd"
> exe="/usr/lib/systemd/systemd" nlnk-grp=1 op=connect res=1
>
> Signed-off-by: sgrubb <sgrubb@redhat.com>
> ---
>  include/uapi/linux/audit.h |  1 +
>  kernel/audit.c             | 48 ++++++++++++++++++++++++++++++++++++++++++----
>  2 files changed, 45 insertions(+), 4 deletions(-)

Since I think this is going to involve a respin, I just want to
mention again "sgrubb" vs "Steve Grubb".  More comments inline ...

> diff --git a/include/uapi/linux/audit.h b/include/uapi/linux/audit.h
> index 0714a66f0e0c..892e63d9f2c1 100644
> --- a/include/uapi/linux/audit.h
> +++ b/include/uapi/linux/audit.h
> @@ -112,6 +112,7 @@
>  #define AUDIT_FEATURE_CHANGE   1328    /* audit log listing feature changes */
>  #define AUDIT_REPLACE          1329    /* Replace auditd if this packet unanswerd */
>  #define AUDIT_KERN_MODULE      1330    /* Kernel Module events */
> +#define AUDIT_EVENT_LISTENER   1332    /* Task joined multicast read socket */

What Richard said.  Basically AUDIT_EVENT_LISTENER should be 1331 or
have a *really* good explanation as to why it needs to be 1332.

> diff --git a/kernel/audit.c b/kernel/audit.c
> index 1baabc9539b4..3d2461be83a8 100644
> --- a/kernel/audit.c
> +++ b/kernel/audit.c
> @@ -1476,22 +1476,61 @@ static void audit_receive(struct sk_buff  *skb)
>         mutex_unlock(&audit_cmd_mutex);
>  }
>
> +/* Log information about who is connecting to the audit multicast socket */
> +static void audit_log_multicast_bind(int group, const char *op, int err)
> +{
> +       const struct cred *cred;
> +       struct tty_struct *tty;
> +       char comm[sizeof(current->comm)];
> +       struct audit_buffer *ab;
> +
> +       ab = audit_log_start(NULL, GFP_KERNEL, AUDIT_EVENT_LISTENER);

It really seems like this should be associated with the current task,
e.g. "audit_log_start(current->audit_context, ...)".  After all, the
whole point of this record is to capture information about the subject
who is binding to the multicast socket.

> +       if (!ab)
> +               return;
> +
> +       cred = current_cred();
> +       tty = audit_get_tty(current);
> +
> +       audit_log_format(ab, "pid=%u uid=%u auid=%u tty=%s ses=%u",
> +                       task_pid_nr(current),
> +                       from_kuid(&init_user_ns, cred->uid),
> +                       from_kuid(&init_user_ns, audit_get_loginuid(current)),
> +                       tty ? tty_name(tty) : "(none)",
> +                       audit_get_sessionid(current));
> +       audit_put_tty(tty);
> +       audit_log_task_context(ab); /* subj= */
> +       audit_log_format(ab, " comm=");
> +       audit_log_untrustedstring(ab, get_task_comm(comm, current));
> +       audit_log_d_path_exe(ab, current->mm); /* exe= */

We could drop a lot of the above if this was an associated record.

> +       audit_log_format(ab, " nlnk-grp=%d op=%s res=%d", group, op, !err);
> +       audit_log_end(ab);
> +}
> +
>  /* Run custom bind function on netlink socket group connect or bind requests. */
> -static int audit_bind(struct net *net, int group)
> +static int audit_multicast_bind(struct net *net, int group)
>  {
> +       int err = 0;
> +
>         if (!capable(CAP_AUDIT_READ))
> -               return -EPERM;
> +               err = -EPERM;
> +       audit_log_multicast_bind(group, "connect", err);
>
> -       return 0;
> +       return err;
> +}
> +
> +static void audit_multicast_unbind(struct net *net, int group)
> +{
> +       audit_log_multicast_bind(group, "disconnect", 0);
>  }
>
>  static int __net_init audit_net_init(struct net *net)
>  {
>         struct netlink_kernel_cfg cfg = {
>                 .input  = audit_receive,
> -               .bind   = audit_bind,
> +               .bind   = audit_multicast_bind,
>                 .flags  = NL_CFG_F_NONROOT_RECV,
>                 .groups = AUDIT_NLGRP_MAX,
> +               .unbind = audit_multicast_unbind,

Please group the unbind with the bind, it is much easier to read that way.

>         };
>
>         struct audit_net *aunet = net_generic(net, audit_net_id);

-- 
paul moore
www.paul-moore.com

Re: [PATCH 1/1] audit: log binding and unbinding to netlink multicast

[Steve Grubb]

On Monday, October 16, 2017 5:35:55 PM EDT Paul Moore wrote:
> On Fri, Oct 13, 2017 at 3:58 PM, Steve Grubb <sgrubb@redhat.com> wrote:
> > Log information about programs connecting and disconnecting to the audit
> > netlink multicast socket. This is needed so that during investigations a
> > security officer can tell who or what had access to the audit trail. This
> > helps to meet the FAU_SAR.2 requirement for Common Criteria. Sample
> > event:
> > 
> > type=UNKNOWN[1332] msg=audit(1507924331.540:3): pid=1 uid=0
> > auid=4294967295 tty=(none) ses=4294967295 subj=kernel comm="systemd"
> > exe="/usr/lib/systemd/systemd" nlnk-grp=1 op=connect res=1
> > 
> > Signed-off-by: sgrubb <sgrubb@redhat.com>
> > ---
> > 
> >  include/uapi/linux/audit.h |  1 +
> >  kernel/audit.c             | 48
> >  ++++++++++++++++++++++++++++++++++++++++++---- 2 files changed, 45
> >  insertions(+), 4 deletions(-)
> 
> Since I think this is going to involve a respin, I just want to
> mention again "sgrubb" vs "Steve Grubb".  More comments inline ...
> 
> > diff --git a/include/uapi/linux/audit.h b/include/uapi/linux/audit.h
> > index 0714a66f0e0c..892e63d9f2c1 100644
> > --- a/include/uapi/linux/audit.h
> > +++ b/include/uapi/linux/audit.h
> > @@ -112,6 +112,7 @@
> > 
> >  #define AUDIT_FEATURE_CHANGE   1328    /* audit log listing feature
> >  changes */ #define AUDIT_REPLACE          1329    /* Replace auditd if
> >  this packet unanswerd */ #define AUDIT_KERN_MODULE      1330    /*
> >  Kernel Module events */
> > 
> > +#define AUDIT_EVENT_LISTENER   1332    /* Task joined multicast read
> > socket */
>
> What Richard said.  Basically AUDIT_EVENT_LISTENER should be 1331 or
> have a *really* good explanation as to why it needs to be 1332.

Because 1331 is already assigned and in https://git.kernel.org/pub/scm/linux/
kernel/git/jack/linux-fs.git/log/?h=for_next as commit 
de8cd83e91bc3ee212b3e6ec6e4283af9e4ab269.

If you want me to assign 1331 which is already assigned to AUDIT_FANOTIFY in 
the user space piece, then it will make your testing...not look right. So, how 
do you want it?


> > diff --git a/kernel/audit.c b/kernel/audit.c
> > index 1baabc9539b4..3d2461be83a8 100644
> > --- a/kernel/audit.c
> > +++ b/kernel/audit.c
> > @@ -1476,22 +1476,61 @@ static void audit_receive(struct sk_buff  *skb)
> > 
> >         mutex_unlock(&audit_cmd_mutex);
> >  
> >  }
> > 
> > +/* Log information about who is connecting to the audit multicast socket
> > */ +static void audit_log_multicast_bind(int group, const char *op, int
> > err) +{
> > +       const struct cred *cred;
> > +       struct tty_struct *tty;
> > +       char comm[sizeof(current->comm)];
> > +       struct audit_buffer *ab;
> > +
> > +       ab = audit_log_start(NULL, GFP_KERNEL, AUDIT_EVENT_LISTENER);
> 
> It really seems like this should be associated with the current task,
> e.g. "audit_log_start(current->audit_context, ...)".  After all, the
> whole point of this record is to capture information about the subject
> who is binding to the multicast socket.

OK, easy enough.


> > +       if (!ab)
> > +               return;
> > +
> > +       cred = current_cred();
> > +       tty = audit_get_tty(current);
> > +
> > +       audit_log_format(ab, "pid=%u uid=%u auid=%u tty=%s ses=%u",
> > +                       task_pid_nr(current),
> > +                       from_kuid(&init_user_ns, cred->uid),
> > +                       from_kuid(&init_user_ns,
> > audit_get_loginuid(current)), +                       tty ? tty_name(tty)
> > : "(none)",
> > +                       audit_get_sessionid(current));
> > +       audit_put_tty(tty);
> > +       audit_log_task_context(ab); /* subj= */
> > +       audit_log_format(ab, " comm=");
> > +       audit_log_untrustedstring(ab, get_task_comm(comm, current));
> > +       audit_log_d_path_exe(ab, current->mm); /* exe= */
> 
> We could drop a lot of the above if this was an associated record.

Really do not want that to be the way we do things. We never have relied on 
syscall records for simple events.


> > +       audit_log_format(ab, " nlnk-grp=%d op=%s res=%d", group, op,
> > !err);
> > +       audit_log_end(ab);
> > +}
> > +
> > 
> >  /* Run custom bind function on netlink socket group connect or bind
> >  requests. */> 
> > -static int audit_bind(struct net *net, int group)
> > +static int audit_multicast_bind(struct net *net, int group)
> > 
> >  {
> > 
> > +       int err = 0;
> > +
> > 
> >         if (!capable(CAP_AUDIT_READ))
> > 
> > -               return -EPERM;
> > +               err = -EPERM;
> > +       audit_log_multicast_bind(group, "connect", err);
> > 
> > -       return 0;
> > +       return err;
> > +}
> > +
> > +static void audit_multicast_unbind(struct net *net, int group)
> > +{
> > +       audit_log_multicast_bind(group, "disconnect", 0);
> > 
> >  }
> >  
> >  static int __net_init audit_net_init(struct net *net)
> >  {
> >  
> >         struct netlink_kernel_cfg cfg = {
> >         
> >                 .input  = audit_receive,
> > 
> > -               .bind   = audit_bind,
> > +               .bind   = audit_multicast_bind,
> > 
> >                 .flags  = NL_CFG_F_NONROOT_RECV,
> >                 .groups = AUDIT_NLGRP_MAX,
> > 
> > +               .unbind = audit_multicast_unbind,
> 
> Please group the unbind with the bind, it is much easier to read that way.

No problem.

-Steve

> >         };
> >         
> >         struct audit_net *aunet = net_generic(net, audit_net_id);

Re: [PATCH 1/1] audit: log binding and unbinding to netlink multicast

[Steve Grubb]

On Monday, October 16, 2017 6:06:47 PM EDT Steve Grubb wrote:
> > > +/* Log information about who is connecting to the audit multicast
> > > socket
> > > */ +static void audit_log_multicast_bind(int group, const char *op, int
> > > err) +{
> > > +       const struct cred *cred;
> > > +       struct tty_struct *tty;
> > > +       char comm[sizeof(current->comm)];
> > > +       struct audit_buffer *ab;
> > > +
> > > +       ab = audit_log_start(NULL, GFP_KERNEL, AUDIT_EVENT_LISTENER);
> > 
> > It really seems like this should be associated with the current task,
> > e.g. "audit_log_start(current->audit_context, ...)".  After all, the
> > whole point of this record is to capture information about the subject
> > who is binding to the multicast socket.
> 
> OK, easy enough.

But wouldn't that make it an auxiliary record (if there happens to be a 
syscall record) instead of a standalone event? The intention is that this 
event is standalone just like AUDIT_SECCOMP or AUDIT_LOGIN. Associating with 
the current task is done by using current in formatting the message as seen 
below. (e.g.  task_pid_nr(current), audit_get_sessionid(current))

I think it's correct as is.

-Steve

> > +       if (!ab)
> > +               return;
> > +
> > +       cred = current_cred();
> > +       tty = audit_get_tty(current);
> > +
> > +       audit_log_format(ab, "pid=%u uid=%u auid=%u tty=%s ses=%u",
> > +                       task_pid_nr(current),
> > +                       from_kuid(&init_user_ns, cred->uid),
> > +                       from_kuid(&init_user_ns,
> > audit_get_loginuid(current)), +                       tty ? tty_name(tty)
> > : "(none)",
> > +                       audit_get_sessionid(current));
> > +       audit_put_tty(tty);
> > +       audit_log_task_context(ab); /* subj= */
> > +       audit_log_format(ab, " comm=");
> > +       audit_log_untrustedstring(ab, get_task_comm(comm, current));
> > +       audit_log_d_path_exe(ab, current->mm); /* exe= */

Re: [PATCH 1/1] audit: log binding and unbinding to netlink multicast

[Richard Guy Briggs]

On 2017-10-16 22:28, Steve Grubb wrote:
> On Monday, October 16, 2017 6:06:47 PM EDT Steve Grubb wrote:
> > > > +/* Log information about who is connecting to the audit multicast
> > > > socket
> > > > */ +static void audit_log_multicast_bind(int group, const char *op, int
> > > > err) +{
> > > > +       const struct cred *cred;
> > > > +       struct tty_struct *tty;
> > > > +       char comm[sizeof(current->comm)];
> > > > +       struct audit_buffer *ab;
> > > > +
> > > > +       ab = audit_log_start(NULL, GFP_KERNEL, AUDIT_EVENT_LISTENER);
> > > 
> > > It really seems like this should be associated with the current task,
> > > e.g. "audit_log_start(current->audit_context, ...)".  After all, the
> > > whole point of this record is to capture information about the subject
> > > who is binding to the multicast socket.
> > 
> > OK, easy enough.
> 
> But wouldn't that make it an auxiliary record (if there happens to be a 
> syscall record) instead of a standalone event? The intention is that this 
> event is standalone just like AUDIT_SECCOMP or AUDIT_LOGIN. Associating with 
> the current task is done by using current in formatting the message as seen 
> below. (e.g.  task_pid_nr(current), audit_get_sessionid(current))
> 
> I think it's correct as is.

Yes, this would associate this record with the tentatively generated
SYSCALL record for this task.

To use the AUDIT_TASK record idea, a local audit context would need to
be created and used for the AUDIT_CONFIG_CHANGE and AUDIT_TASK records
only.

> -Steve
> 
> > > +       if (!ab)
> > > +               return;
> > > +
> > > +       cred = current_cred();
> > > +       tty = audit_get_tty(current);
> > > +
> > > +       audit_log_format(ab, "pid=%u uid=%u auid=%u tty=%s ses=%u",
> > > +                       task_pid_nr(current),
> > > +                       from_kuid(&init_user_ns, cred->uid),
> > > +                       from_kuid(&init_user_ns,
> > > audit_get_loginuid(current)), +                       tty ? tty_name(tty)
> > > : "(none)",
> > > +                       audit_get_sessionid(current));
> > > +       audit_put_tty(tty);
> > > +       audit_log_task_context(ab); /* subj= */
> > > +       audit_log_format(ab, " comm=");
> > > +       audit_log_untrustedstring(ab, get_task_comm(comm, current));
> > > +       audit_log_d_path_exe(ab, current->mm); /* exe= */
> 
> --
> Linux-audit mailing list
> Linux-audit@redhat.com
> https://www.redhat.com/mailman/listinfo/linux-audit

- 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

Re: [PATCH 1/1] audit: log binding and unbinding to netlink multicast

[Steve Grubb]

On Monday, October 16, 2017 7:04:14 PM EDT Richard Guy Briggs wrote:
> On 2017-10-16 22:28, Steve Grubb wrote:
> > On Monday, October 16, 2017 6:06:47 PM EDT Steve Grubb wrote:
> > > > > +/* Log information about who is connecting to the audit multicast
> > > > > socket
> > > > > */ +static void audit_log_multicast_bind(int group, const char *op,
> > > > > int
> > > > > err) +{
> > > > > +       const struct cred *cred;
> > > > > +       struct tty_struct *tty;
> > > > > +       char comm[sizeof(current->comm)];
> > > > > +       struct audit_buffer *ab;
> > > > > +
> > > > > +       ab = audit_log_start(NULL, GFP_KERNEL,
> > > > > AUDIT_EVENT_LISTENER);
> > > > 
> > > > It really seems like this should be associated with the current task,
> > > > e.g. "audit_log_start(current->audit_context, ...)".  After all, the
> > > > whole point of this record is to capture information about the subject
> > > > who is binding to the multicast socket.
> > > 
> > > OK, easy enough.
> > 
> > But wouldn't that make it an auxiliary record (if there happens to be a
> > syscall record) instead of a standalone event? The intention is that this
> > event is standalone just like AUDIT_SECCOMP or AUDIT_LOGIN. Associating
> > with the current task is done by using current in formatting the message
> > as seen below. (e.g.  task_pid_nr(current), audit_get_sessionid(current))
> > 
> > I think it's correct as is.
> 
> Yes, this would associate this record with the tentatively generated
> SYSCALL record for this task.

OK, then my secondary assessment was right. The patch in this hunk is fine. 
This is a standalone event. I have a patch ready with the .bind/.unbind 
function pointers grouped together. I can add the ghak issue easily enough. I 
just need a ruling on what define number to use for AUDIT_EVENT_LISTENER. 
Should it be 1331 or 1332 (which is its actual number)?

-Steve

Re: [PATCH 1/1] audit: log binding and unbinding to netlink multicast

[Paul Moore]

On Mon, Oct 16, 2017 at 6:06 PM, Steve Grubb <sgrubb@redhat.com> wrote:
> On Monday, October 16, 2017 5:35:55 PM EDT Paul Moore wrote:
>> On Fri, Oct 13, 2017 at 3:58 PM, Steve Grubb <sgrubb@redhat.com> wrote:
>> > Log information about programs connecting and disconnecting to the audit
>> > netlink multicast socket. This is needed so that during investigations a
>> > security officer can tell who or what had access to the audit trail. This
>> > helps to meet the FAU_SAR.2 requirement for Common Criteria. Sample
>> > event:
>> >
>> > type=UNKNOWN[1332] msg=audit(1507924331.540:3): pid=1 uid=0
>> > auid=4294967295 tty=(none) ses=4294967295 subj=kernel comm="systemd"
>> > exe="/usr/lib/systemd/systemd" nlnk-grp=1 op=connect res=1
>> >
>> > Signed-off-by: sgrubb <sgrubb@redhat.com>
>> > ---
>> >
>> >  include/uapi/linux/audit.h |  1 +
>> >  kernel/audit.c             | 48
>> >  ++++++++++++++++++++++++++++++++++++++++++---- 2 files changed, 45
>> >  insertions(+), 4 deletions(-)
>>
>> Since I think this is going to involve a respin, I just want to
>> mention again "sgrubb" vs "Steve Grubb".  More comments inline ...
>>
>> > diff --git a/include/uapi/linux/audit.h b/include/uapi/linux/audit.h
>> > index 0714a66f0e0c..892e63d9f2c1 100644
>> > --- a/include/uapi/linux/audit.h
>> > +++ b/include/uapi/linux/audit.h
>> > @@ -112,6 +112,7 @@
>> >
>> >  #define AUDIT_FEATURE_CHANGE   1328    /* audit log listing feature
>> >  changes */ #define AUDIT_REPLACE          1329    /* Replace auditd if
>> >  this packet unanswerd */ #define AUDIT_KERN_MODULE      1330    /*
>> >  Kernel Module events */
>> >
>> > +#define AUDIT_EVENT_LISTENER   1332    /* Task joined multicast read
>> > socket */
>>
>> What Richard said.  Basically AUDIT_EVENT_LISTENER should be 1331 or
>> have a *really* good explanation as to why it needs to be 1332.
>
> Because 1331 is already assigned and in https://git.kernel.org/pub/scm/linux/
> kernel/git/jack/linux-fs.git/log/?h=for_next as commit
> de8cd83e91bc3ee212b3e6ec6e4283af9e4ab269.
>
> If you want me to assign 1331 which is already assigned to AUDIT_FANOTIFY in
> the user space piece, then it will make your testing...not look right. So, how
> do you want it?

As I said above, I wanted a *really* good explanation, which you
provided.  In the future it's helpful to add a note about things like
this, it saves us all from being annoyed.

I need to think about when this should get merged, but considering we
are are -rc5 right now and this is a new feature with no test (at
least not that I'm seeing on the list, or on GH) it is likely that
this patch will get held until after the upcoming merge window so the
merge conflict will not be a practical issue.

(HINT: in case you haven't been paying attention to audit kernel
development lately, you should work on a test for the audit-testsuite
which tests this new functionality.)

-- 
paul moore
www.paul-moore.com

Re: [PATCH 1/1] audit: log binding and unbinding to netlink multicast

[Paul Moore]

On Mon, Oct 16, 2017 at 7:04 PM, Richard Guy Briggs <rgb@redhat.com> wrote:
> To use the AUDIT_TASK record idea, a local audit context would need to
> be created and used for the AUDIT_CONFIG_CHANGE and AUDIT_TASK records
> only.

For the record, the more I think about the AUDIT_TASK record idea, the
more I'm souring on the idea.  Someday when we get around to an
attribute based format something like this will be handy (I'm thinking
we will want higher granularity when it comes to records), but right
now it doesn't seem very attractive to me.

-- 
paul moore
www.paul-moore.com

Re: [PATCH 1/1] audit: log binding and unbinding to netlink multicast

[Paul Moore]

On Mon, Oct 16, 2017 at 6:28 PM, Steve Grubb <sgrubb@redhat.com> wrote:
> On Monday, October 16, 2017 6:06:47 PM EDT Steve Grubb wrote:
>> > > +/* Log information about who is connecting to the audit multicast
>> > > socket
>> > > */ +static void audit_log_multicast_bind(int group, const char *op, int
>> > > err) +{
>> > > +       const struct cred *cred;
>> > > +       struct tty_struct *tty;
>> > > +       char comm[sizeof(current->comm)];
>> > > +       struct audit_buffer *ab;
>> > > +
>> > > +       ab = audit_log_start(NULL, GFP_KERNEL, AUDIT_EVENT_LISTENER);
>> >
>> > It really seems like this should be associated with the current task,
>> > e.g. "audit_log_start(current->audit_context, ...)".  After all, the
>> > whole point of this record is to capture information about the subject
>> > who is binding to the multicast socket.
>>
>> OK, easy enough.
>
> But wouldn't that make it an auxiliary record (if there happens to be a
> syscall record) instead of a standalone event?

I've always found the significance placed on auxiliary vs standalone
events amusing.  The only significant difference between the two is
that "auxiliary" records share a timestamp with the other records that
have been triggered by a given syscall instance whereas standlone
records always have a new timestamp generated.  I'm personally of the
opinion that if we are emitting records we should always try to group
related records into a single event to help reduce the confusion for
people looking through their audit logs; in other words, whenever
possible we should pass the audit_context to audit_log_start() and
friends.

If syscall auditing is disabled, these records (e.g.
AUDIT_EVENT_LISTENER) will still be emitted by the kernel as
"standalone" events.  If syscall auditing is enabled, these records
will be grouped with the triggering syscall, which just makes sense.
Look at the recent problem with init/systemd and the MAC_STATUS
records, that is a great example.

-- 
paul moore
www.paul-moore.com

Re: [PATCH 1/1] audit: log binding and unbinding to netlink multicast

[Steve Grubb]

On Tuesday, October 17, 2017 11:11:31 AM EDT Paul Moore wrote:
> On Mon, Oct 16, 2017 at 6:28 PM, Steve Grubb <sgrubb@redhat.com> wrote:
> > On Monday, October 16, 2017 6:06:47 PM EDT Steve Grubb wrote:
> >> > > +/* Log information about who is connecting to the audit multicast
> >> > > socket
> >> > > */ +static void audit_log_multicast_bind(int group, const char *op,
> >> > > int
> >> > > err) +{
> >> > > +       const struct cred *cred;
> >> > > +       struct tty_struct *tty;
> >> > > +       char comm[sizeof(current->comm)];
> >> > > +       struct audit_buffer *ab;
> >> > > +
> >> > > +       ab = audit_log_start(NULL, GFP_KERNEL, AUDIT_EVENT_LISTENER);
> >> > 
> >> > It really seems like this should be associated with the current task,
> >> > e.g. "audit_log_start(current->audit_context, ...)".  After all, the
> >> > whole point of this record is to capture information about the subject
> >> > who is binding to the multicast socket.
> >> 
> >> OK, easy enough.
> > 
> > But wouldn't that make it an auxiliary record (if there happens to be a
> > syscall record) instead of a standalone event?
> 
> I've always found the significance placed on auxiliary vs standalone
> events amusing.  The only significant difference between the two is
> that "auxiliary" records share a timestamp with the other records that
> have been triggered by a given syscall instance whereas standlone
> records always have a new timestamp generated.  I'm personally of the
> opinion that if we are emitting records we should always try to group
> related records into a single event to help reduce the confusion for
> people looking through their audit logs; in other words, whenever
> possible we should pass the audit_context to audit_log_start() and
> friends.

No, actually that introduces confusion. An event should have one specific 
meaning. This subject did something to that. Adding a syscall record takes a 
simple event and makes it incomprehensible to anyone that doesn't have 
familiarity with audit events. All necessary information is in the event that 
I created.

> If syscall auditing is disabled, these records (e.g.
> AUDIT_EVENT_LISTENER) will still be emitted by the kernel as
> "standalone" events.  If syscall auditing is enabled, these records
> will be grouped with the triggering syscall, which just makes sense.
> Look at the recent problem with init/systemd and the MAC_STATUS
> records, that is a great example.

Right. That was a mess because of piggy backing on top of a SYSCALL. I do not 
like events that are sometime simple and sometimes compound. I think that also 
messes up anyone doing json or xml formatting because they need a firm layout 
in their schema. The event is good as is. It has all required fields. Its easy 
to parse and keeps things compact.

-Steve

Re: [PATCH 1/1] audit: log binding and unbinding to netlink multicast

[Paul Moore]

On Tue, Oct 17, 2017 at 12:01 PM, Steve Grubb <sgrubb@redhat.com> wrote:
> On Tuesday, October 17, 2017 11:11:31 AM EDT Paul Moore wrote:
>> On Mon, Oct 16, 2017 at 6:28 PM, Steve Grubb <sgrubb@redhat.com> wrote:
>> > On Monday, October 16, 2017 6:06:47 PM EDT Steve Grubb wrote:
>> >> > > +/* Log information about who is connecting to the audit multicast
>> >> > > socket
>> >> > > */ +static void audit_log_multicast_bind(int group, const char *op,
>> >> > > int
>> >> > > err) +{
>> >> > > +       const struct cred *cred;
>> >> > > +       struct tty_struct *tty;
>> >> > > +       char comm[sizeof(current->comm)];
>> >> > > +       struct audit_buffer *ab;
>> >> > > +
>> >> > > +       ab = audit_log_start(NULL, GFP_KERNEL, AUDIT_EVENT_LISTENER);
>> >> >
>> >> > It really seems like this should be associated with the current task,
>> >> > e.g. "audit_log_start(current->audit_context, ...)".  After all, the
>> >> > whole point of this record is to capture information about the subject
>> >> > who is binding to the multicast socket.
>> >>
>> >> OK, easy enough.
>> >
>> > But wouldn't that make it an auxiliary record (if there happens to be a
>> > syscall record) instead of a standalone event?
>>
>> I've always found the significance placed on auxiliary vs standalone
>> events amusing.  The only significant difference between the two is
>> that "auxiliary" records share a timestamp with the other records that
>> have been triggered by a given syscall instance whereas standlone
>> records always have a new timestamp generated.  I'm personally of the
>> opinion that if we are emitting records we should always try to group
>> related records into a single event to help reduce the confusion for
>> people looking through their audit logs; in other words, whenever
>> possible we should pass the audit_context to audit_log_start() and
>> friends.
>
> No, actually that introduces confusion. An event should have one specific
> meaning. This subject did something to that. Adding a syscall record takes a
> simple event and makes it incomprehensible to anyone that doesn't have
> familiarity with audit events. All necessary information is in the event that
> I created.

What?  That makes zero sense.  The SYSCALL record and the
AUDIT_EVENT_LISTENER record are part of the same real world event: a
process executed a syscall and established themselves as an audit
event listener.

>> If syscall auditing is disabled, these records (e.g.
>> AUDIT_EVENT_LISTENER) will still be emitted by the kernel as
>> "standalone" events.  If syscall auditing is enabled, these records
>> will be grouped with the triggering syscall, which just makes sense.
>> Look at the recent problem with init/systemd and the MAC_STATUS
>> records, that is a great example.
>
> Right. That was a mess because of piggy backing on top of a SYSCALL.

Again ... what?  The SYSCALL record was the solution to that
particular problem, and demonstrated that an event could have multiple
records depending on the audit configuration (in this particular case
syscall auditing disabled/enabled).

> I do not
> like events that are sometime simple and sometimes compound.

That's dependent on your system's audit configuration.

> I think that also
> messes up anyone doing json or xml formatting because they need a firm layout
> in their schema.

I would think anyone consuming audit data for analysis at a higher
level would *really* want to see all related records grouped together
(i.e. same timestamp).  Any higher order analysis should be able to
handle event metadata that is optional/missing (we have that now).

> The event is good as is. It has all required fields ...

... until you decide that you need to add something; we've seen how
well that is going.

-- 
paul moore
www.paul-moore.com