[PATCH 1/1] audit: add missing fields to AUDIT_CONFIG_CHANGE event

[PATCH 1/1] audit: add missing fields to AUDIT_CONFIG_CHANGE event

[Steve Grubb]

There are very important fields necessary to understand who is adding
audit rules and a little more context about the environment in which
its happening. This adds pid, uid, tty, subj, comm, and exe
information to the event. These are required fields.

Signed-off-by: sgrubb <sgrubb@redhat.com>
---
 kernel/audit_watch.c | 23 +++++++++++++++++++----
 kernel/auditfilter.c | 18 +++++++++++++++---
 2 files changed, 34 insertions(+), 7 deletions(-)

diff --git a/kernel/audit_watch.c b/kernel/audit_watch.c
index 9eb8b3511636..63abc2ba1372 100644
--- a/kernel/audit_watch.c
+++ b/kernel/audit_watch.c
@@ -239,14 +239,29 @@ 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 tty_struct *tty;
+		const struct cred *cred;
 		struct audit_buffer *ab;
+		char comm[sizeof(current->comm)];
+
 		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(&init_user_ns, audit_get_loginuid(current)),
-				 audit_get_sessionid(current), op);
-		audit_log_format(ab, " path=");
+
+		tty = audit_get_tty(current);
+		cred = current_cred();
+		audit_log_format(ab, "pid=%d uid=%u auid=%u tty=%s ses=%u",
+				task_tgid_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_log_task_context(ab);
+		audit_log_format(ab, " comm=");
+		audit_log_untrustedstring(ab, get_task_comm(comm, current));
+		audit_log_d_path_exe(ab, current->mm);
+		audit_log_format(ab, "op=%s path=", op);
 		audit_log_untrustedstring(ab, w->path);
 		audit_log_key(ab, r->filterkey);
 		audit_log_format(ab, " list=%d res=1", r->listnr);
diff --git a/kernel/auditfilter.c b/kernel/auditfilter.c
index 0b0aa5854dac..5e2a953da29a 100644
--- a/kernel/auditfilter.c
+++ b/kernel/auditfilter.c
@@ -1065,17 +1065,29 @@ static void audit_list_rules(int seq, struct sk_buff_head *q)
 static void audit_log_rule_change(char *action, struct audit_krule *rule, int res)
 {
 	struct audit_buffer *ab;
-	uid_t loginuid = from_kuid(&init_user_ns, audit_get_loginuid(current));
-	unsigned int sessionid = audit_get_sessionid(current);
+	struct tty_struct *tty;
+	const struct cred *cred;
+	char comm[sizeof(current->comm)];
 
 	if (!audit_enabled)
 		return;
 
+	tty = audit_get_tty(current);
+	cred = current_cred();
+
 	ab = audit_log_start(NULL, GFP_KERNEL, AUDIT_CONFIG_CHANGE);
 	if (!ab)
 		return;
-	audit_log_format(ab, "auid=%u ses=%u" ,loginuid, sessionid);
+	audit_log_format(ab, "pid=%d uid=%u auid=%u tty=%s ses=%u",
+			task_tgid_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_log_task_context(ab);
+	audit_log_format(ab, " comm=");
+	audit_log_untrustedstring(ab, get_task_comm(comm, current));
+	audit_log_d_path_exe(ab, current->mm);
 	audit_log_format(ab, " op=%s", action);
 	audit_log_key(ab, rule->filterkey);
 	audit_log_format(ab, " list=%d res=%d", rule->listnr, res);
-- 
2.13.6

Re: [PATCH 1/1] audit: add missing fields to AUDIT_CONFIG_CHANGE event

[Paul Moore]

On Thu, Oct 12, 2017 at 3:57 PM, Steve Grubb <sgrubb@redhat.com> wrote:
> There are very important fields necessary to understand who is adding
> audit rules and a little more context about the environment in which
> its happening. This adds pid, uid, tty, subj, comm, and exe
> information to the event. These are required fields.
>
> Signed-off-by: sgrubb <sgrubb@redhat.com>
> ---
>  kernel/audit_watch.c | 23 +++++++++++++++++++----
>  kernel/auditfilter.c | 18 +++++++++++++++---
>  2 files changed, 34 insertions(+), 7 deletions(-)
>
> diff --git a/kernel/audit_watch.c b/kernel/audit_watch.c
> index 9eb8b3511636..63abc2ba1372 100644
> --- a/kernel/audit_watch.c
> +++ b/kernel/audit_watch.c
> @@ -239,14 +239,29 @@ 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 tty_struct *tty;
> +               const struct cred *cred;
>                 struct audit_buffer *ab;
> +               char comm[sizeof(current->comm)];
> +
>                 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(&init_user_ns, audit_get_loginuid(current)),
> -                                audit_get_sessionid(current), op);
> -               audit_log_format(ab, " path=");
> +
> +               tty = audit_get_tty(current);
> +               cred = current_cred();
> +               audit_log_format(ab, "pid=%d uid=%u auid=%u tty=%s ses=%u",
> +                               task_tgid_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));

Another reminder that in general I'm not going to accept patches that
shuffle the fields or insert fields in the middle of a record; if you
want to add new fields to a record, add them at the end.  I see no
reason to break with the rule for this patch.

> +               audit_log_task_context(ab);
> +               audit_log_format(ab, " comm=");
> +               audit_log_untrustedstring(ab, get_task_comm(comm, current));
> +               audit_log_d_path_exe(ab, current->mm);
> +               audit_log_format(ab, "op=%s path=", op);
>                 audit_log_untrustedstring(ab, w->path);
>                 audit_log_key(ab, r->filterkey);
>                 audit_log_format(ab, " list=%d res=1", r->listnr);
> diff --git a/kernel/auditfilter.c b/kernel/auditfilter.c
> index 0b0aa5854dac..5e2a953da29a 100644
> --- a/kernel/auditfilter.c
> +++ b/kernel/auditfilter.c
> @@ -1065,17 +1065,29 @@ static void audit_list_rules(int seq, struct sk_buff_head *q)
>  static void audit_log_rule_change(char *action, struct audit_krule *rule, int res)
>  {
>         struct audit_buffer *ab;
> -       uid_t loginuid = from_kuid(&init_user_ns, audit_get_loginuid(current));
> -       unsigned int sessionid = audit_get_sessionid(current);
> +       struct tty_struct *tty;
> +       const struct cred *cred;
> +       char comm[sizeof(current->comm)];
>
>         if (!audit_enabled)
>                 return;
>
> +       tty = audit_get_tty(current);
> +       cred = current_cred();
> +
>         ab = audit_log_start(NULL, GFP_KERNEL, AUDIT_CONFIG_CHANGE);
>         if (!ab)
>                 return;
> -       audit_log_format(ab, "auid=%u ses=%u" ,loginuid, sessionid);
> +       audit_log_format(ab, "pid=%d uid=%u auid=%u tty=%s ses=%u",
> +                       task_tgid_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_log_task_context(ab);
> +       audit_log_format(ab, " comm=");
> +       audit_log_untrustedstring(ab, get_task_comm(comm, current));
> +       audit_log_d_path_exe(ab, current->mm);
>         audit_log_format(ab, " op=%s", action);
>         audit_log_key(ab, rule->filterkey);
>         audit_log_format(ab, " list=%d res=%d", rule->listnr, res);
> --
> 2.13.6
>
>
> --
> Linux-audit mailing list
> Linux-audit@redhat.com
> https://www.redhat.com/mailman/listinfo/linux-audit



-- 
paul moore
www.paul-moore.com

Re: [PATCH 1/1] audit: add missing fields to AUDIT_CONFIG_CHANGE event

[Steve Grubb]

On Thursday, October 12, 2017 5:04:41 PM EDT Paul Moore wrote:
> On Thu, Oct 12, 2017 at 3:57 PM, Steve Grubb <sgrubb@redhat.com> wrote:
> > There are very important fields necessary to understand who is adding
> > audit rules and a little more context about the environment in which
> > its happening. This adds pid, uid, tty, subj, comm, and exe
> > information to the event. These are required fields.
> > 
> > Signed-off-by: sgrubb <sgrubb@redhat.com>
> > ---
> > 
> >  kernel/audit_watch.c | 23 +++++++++++++++++++----
> >  kernel/auditfilter.c | 18 +++++++++++++++---
> >  2 files changed, 34 insertions(+), 7 deletions(-)
> > 
> > diff --git a/kernel/audit_watch.c b/kernel/audit_watch.c
> > index 9eb8b3511636..63abc2ba1372 100644
> > --- a/kernel/audit_watch.c
> > +++ b/kernel/audit_watch.c
> > @@ -239,14 +239,29 @@ 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 tty_struct *tty;
> > +               const struct cred *cred;
> > 
> >                 struct audit_buffer *ab;
> > 
> > +               char comm[sizeof(current->comm)];
> > +
> > 
> >                 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(&init_user_ns,
> > audit_get_loginuid(current)), -                               
> > audit_get_sessionid(current), op); -               audit_log_format(ab, "
> > path=");
> > +
> > +               tty = audit_get_tty(current);
> > +               cred = current_cred();
> > +               audit_log_format(ab, "pid=%d uid=%u auid=%u tty=%s
> > ses=%u",
> > +                               task_tgid_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));
> 
> Another reminder that in general I'm not going to accept patches that
> shuffle the fields or insert fields in the middle of a record; if you
> want to add new fields to a record, add them at the end.  I see no
> reason to break with the rule for this patch.

This has never been a rule. There are times I've suggested adding things at 
the end because I looked at the parsers and saw that was the best solution. 
But that is an informed decision based on looking at the code. Besides, there 
are 9 places where AUDIT_CONFIG_CHANGE is logged and we'll have to parse it 9 
different ways if we simply add things at the end. That said, I did some 
testing. Here's a sample event:

type=CONFIG_CHANGE msg=audit(1507836246.134:98): pid=576 uid=0 auid=4294967295 
tty=(none) ses=4294967295 subj=system_u:system_r:unconfined_service_t:s0 
comm="auditctl" exe="/usr/sbin/auditctl" op=add_rule key="modules" list=4 
res=1

and a current event:

type=CONFIG_CHANGE msg=audit(1507827262.547:6): audit_enabled=1 old=1 
auid=4294967295 ses=4294967295 subj=system_u:system_r:unconfined_service_t:s0 
op=add_rule key="sched" list=4 res=1

So, the old event has auid, session, and subj. Testing ausearch with those 
fields on the new event yields this:

[root@f26-audit ~]# ausearch --start boot -m config_change --loginuid 
4294967295 >/dev/null
[root@f26-audit ~]# echo $?
0
[root@f26-audit ~]# ausearch --start boot -m config_change --session 
4294967295 >/dev/null
[root@f26-audit ~]# echo $?
0
[root@f26-audit ~]# ausearch --start boot -m config_change --subject service_t 
>/dev/null
[root@f26-audit ~]# echo $?
0

So, ausearch still finds all the fields its supposed to. Does it find anything 
it doesn't know about?

[root@f26-audit ~]# ausearch --start boot -m config_change -ui 0 >/dev/null
<no matches>

So, a current or older ausearch is not harmed by any of these changes. It 
maintains the exact same behavior. The only time we have a problem is when 
there are changes introduced that are not coordinated or tested. This has been 
tested. This patch closes the last big hole that the auparse_normalizer sees 
on boot.

-Steve


> > +               audit_log_task_context(ab);
> > +               audit_log_format(ab, " comm=");
> > +               audit_log_untrustedstring(ab, get_task_comm(comm,
> > current)); +               audit_log_d_path_exe(ab, current->mm);
> > +               audit_log_format(ab, "op=%s path=", op);
> > 
> >                 audit_log_untrustedstring(ab, w->path);
> >                 audit_log_key(ab, r->filterkey);
> >                 audit_log_format(ab, " list=%d res=1", r->listnr);
> > 
> > diff --git a/kernel/auditfilter.c b/kernel/auditfilter.c
> > index 0b0aa5854dac..5e2a953da29a 100644
> > --- a/kernel/auditfilter.c
> > +++ b/kernel/auditfilter.c
> > @@ -1065,17 +1065,29 @@ static void audit_list_rules(int seq, struct
> > sk_buff_head *q)> 
> >  static void audit_log_rule_change(char *action, struct audit_krule *rule,
> >  int res) {
> >  
> >         struct audit_buffer *ab;
> > 
> > -       uid_t loginuid = from_kuid(&init_user_ns,
> > audit_get_loginuid(current)); -       unsigned int sessionid =
> > audit_get_sessionid(current);
> > +       struct tty_struct *tty;
> > +       const struct cred *cred;
> > +       char comm[sizeof(current->comm)];
> > 
> >         if (!audit_enabled)
> >         
> >                 return;
> > 
> > +       tty = audit_get_tty(current);
> > +       cred = current_cred();
> > +
> > 
> >         ab = audit_log_start(NULL, GFP_KERNEL, AUDIT_CONFIG_CHANGE);
> >         if (!ab)
> >         
> >                 return;
> > 
> > -       audit_log_format(ab, "auid=%u ses=%u" ,loginuid, sessionid);
> > +       audit_log_format(ab, "pid=%d uid=%u auid=%u tty=%s ses=%u",
> > +                       task_tgid_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_log_task_context(ab);
> > 
> > +       audit_log_format(ab, " comm=");
> > +       audit_log_untrustedstring(ab, get_task_comm(comm, current));
> > +       audit_log_d_path_exe(ab, current->mm);
> > 
> >         audit_log_format(ab, " op=%s", action);
> >         audit_log_key(ab, rule->filterkey);
> >         audit_log_format(ab, " list=%d res=%d", rule->listnr, res);
> > 
> > --
> > 2.13.6
> > 
> > 
> > --
> > Linux-audit mailing list
> > Linux-audit@redhat.com
> > https://www.redhat.com/mailman/listinfo/linux-audit

Re: [PATCH 1/1] audit: add missing fields to AUDIT_CONFIG_CHANGE event

[Paul Moore]

On Thu, Oct 12, 2017 at 6:13 PM, Steve Grubb <sgrubb@redhat.com> wrote:
> On Thursday, October 12, 2017 5:04:41 PM EDT Paul Moore wrote:
>> Another reminder that in general I'm not going to accept patches that
>> shuffle the fields or insert fields in the middle of a record; if you
>> want to add new fields to a record, add them at the end.  I see no
>> reason to break with the rule for this patch.
>
> This has never been a rule ...

Yes it has, one I've mentioned to you several times both on and off
the list.  You may disagree with it, but that doesn't mean you are
exempt.

-- 
paul moore
www.paul-moore.com

Re: [PATCH 1/1] audit: add missing fields to AUDIT_CONFIG_CHANGE event

[Steve Grubb]

On Thursday, October 12, 2017 6:51:19 PM EDT Paul Moore wrote:
> On Thu, Oct 12, 2017 at 6:13 PM, Steve Grubb <sgrubb@redhat.com> wrote:
> > On Thursday, October 12, 2017 5:04:41 PM EDT Paul Moore wrote:
> >> Another reminder that in general I'm not going to accept patches that
> >> shuffle the fields or insert fields in the middle of a record; if you
> >> want to add new fields to a record, add them at the end.  I see no
> >> reason to break with the rule for this patch.
> > 
> > This has never been a rule ...
> 
> Yes it has, one I've mentioned to you several times both on and off
> the list.  You may disagree with it, but that doesn't mean you are
> exempt.

I'm speaking on behalf of everyone that has to deal with the events. If we 
need to have 9 parsers for the same event, its really doing a disservice to 
everyone that works on audit events. Besides, since you don't write any user 
space code or do things that have to make sense of it, why would you block 
something that is fixing problems? Did you run any tests with the events I 
supplied to verify things for yourself?

Look at this mess:

1)	audit_log_format(ab, "%s=%u old=%u", function_name, new, old);
	audit_log_session_info(ab);
	rc = audit_log_task_context(ab);

2) 	audit_log_common_recv_msg(&ab, AUDIT_CONFIG_CHANGE);
	audit_log_format(ab, " audit_enabled=%d res=0", audit_enabled);

3)	audit_log_common_recv_msg(&ab, AUDIT_CONFIG_CHANGE);
	audit_log_format(ab, " op=trim res=1");

4)  audit_log_common_recv_msg(&ab, AUDIT_CONFIG_CHANGE);
	audit_log_format(ab, " op=make_equiv old=");
	audit_log_untrustedstring(ab, old);
	audit_log_format(ab, " new=");
	audit_log_untrustedstring(ab, new);
	audit_log_format(ab, " res=%d", !err);

5)	audit_log_common_recv_msg(&ab, AUDIT_CONFIG_CHANGE);
	audit_log_format(ab, " op=tty_set old-enabled=%d new-enabled=%d"
		" old-log_passwd=%d new-log_passwd=%d res=%d",
		old.enabled, s.enabled, old.log_passwd,
		s.log_passwd, !err);

6)	audit_log_format(ab, "auid=%u ses=%u" ,loginuid, sessionid);
	audit_log_task_context(ab);
	audit_log_format(ab, " op=%s", action);
	audit_log_key(ab, rule->filterkey);
	audit_log_format(ab, " list=%d res=%d", rule->listnr, res);

7)	audit_log_format(ab, "auid=%u ses=%u op=%s",
		from_kuid(&init_user_ns, audit_get_loginuid(current)),
		audit_get_sessionid(current), op);
	audit_log_format(ab, " path=");
	audit_log_untrustedstring(ab, audit_mark->path);
	audit_log_key(ab, rule->filterkey);
	audit_log_format(ab, " list=%d res=1", rule->listnr);

8)	audit_log_format(ab, "op=remove_rule");
	audit_log_format(ab, " dir=");
	audit_log_untrustedstring(ab, rule->tree->pathname);
	audit_log_key(ab, rule->filterkey);
	audit_log_format(ab, " list=%d res=1", rule->listnr);

9)	audit_log_format(ab, "auid=%u ses=%u op=%s",
		from_kuid(&init_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);

Of these 7 & 9 are the same. So that means following your suggestion, everyone 
has to write 8 parsers for the same event. Does that sound like good 
engineering practice? Also, it will cause subtle changes that break bigger 
rules than this - like all events everywhere end with the results. They are 
always the last item. Everywhere. This is a self evident rule that you are 
suggesting we break. We can't do that.

Meanwhile...there is a problem that needs to be fixed....

-Steve

Re: [PATCH 1/1] audit: add missing fields to AUDIT_CONFIG_CHANGE event

[Paul Moore]

On Thu, Oct 12, 2017 at 8:34 PM, Steve Grubb <sgrubb@redhat.com> wrote:
> On Thursday, October 12, 2017 6:51:19 PM EDT Paul Moore wrote:
>> On Thu, Oct 12, 2017 at 6:13 PM, Steve Grubb <sgrubb@redhat.com> wrote:
>> > On Thursday, October 12, 2017 5:04:41 PM EDT Paul Moore wrote:
>> >> Another reminder that in general I'm not going to accept patches that
>> >> shuffle the fields or insert fields in the middle of a record; if you
>> >> want to add new fields to a record, add them at the end.  I see no
>> >> reason to break with the rule for this patch.
>> >
>> > This has never been a rule ...
>>
>> Yes it has, one I've mentioned to you several times both on and off
>> the list.  You may disagree with it, but that doesn't mean you are
>> exempt.
>
> I'm speaking on behalf of everyone that has to deal with the events ...

I honestly don't know what to tell you anymore Steve; you seem to
block out all of our past conversations on this matter ... I've heard
all these arguments before, you're not saying anything new, and as a
result my stance remains the same.

> Of these 7 & 9 are the same. So that means following your suggestion, everyone
> has to write 8 parsers for the same event. Does that sound like good
> engineering practice?

Perhaps if the original audit design had used some more of this "good
engineering" we wouldn't be in this situation.  Unfortunately that's
not the case, so we need to hobble along with what we have until we
get an opportunity to rework it.

If you want to add new information to existing records, you have my
suggested guidance; I suggest you use it.  I'm growing very tired of
repeating this discussion.

-- 
paul moore
www.paul-moore.com

Re: [PATCH 1/1] audit: add missing fields to AUDIT_CONFIG_CHANGE event

[Richard Guy Briggs]

On 2017-10-12 19:57, Steve Grubb wrote:
> There are very important fields necessary to understand who is adding
> audit rules and a little more context about the environment in which
> its happening. This adds pid, uid, tty, subj, comm, and exe
> information to the event. These are required fields.

Could we add an issue number to the patch description?

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

> Signed-off-by: sgrubb <sgrubb@redhat.com>
> ---
>  kernel/audit_watch.c | 23 +++++++++++++++++++----
>  kernel/auditfilter.c | 18 +++++++++++++++---
>  2 files changed, 34 insertions(+), 7 deletions(-)
> 
> diff --git a/kernel/audit_watch.c b/kernel/audit_watch.c
> index 9eb8b3511636..63abc2ba1372 100644
> --- a/kernel/audit_watch.c
> +++ b/kernel/audit_watch.c
> @@ -239,14 +239,29 @@ 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 tty_struct *tty;
> +		const struct cred *cred;
>  		struct audit_buffer *ab;
> +		char comm[sizeof(current->comm)];
> +
>  		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(&init_user_ns, audit_get_loginuid(current)),
> -				 audit_get_sessionid(current), op);
> -		audit_log_format(ab, " path=");
> +
> +		tty = audit_get_tty(current);
> +		cred = current_cred();
> +		audit_log_format(ab, "pid=%d uid=%u auid=%u tty=%s ses=%u",
> +				task_tgid_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_log_task_context(ab);
> +		audit_log_format(ab, " comm=");
> +		audit_log_untrustedstring(ab, get_task_comm(comm, current));
> +		audit_log_d_path_exe(ab, current->mm);
> +		audit_log_format(ab, "op=%s path=", op);
>  		audit_log_untrustedstring(ab, w->path);
>  		audit_log_key(ab, r->filterkey);
>  		audit_log_format(ab, " list=%d res=1", r->listnr);

As noted, audit_put_tty(tty) is missing...

> diff --git a/kernel/auditfilter.c b/kernel/auditfilter.c
> index 0b0aa5854dac..5e2a953da29a 100644
> --- a/kernel/auditfilter.c
> +++ b/kernel/auditfilter.c
> @@ -1065,17 +1065,29 @@ static void audit_list_rules(int seq, struct sk_buff_head *q)
>  static void audit_log_rule_change(char *action, struct audit_krule *rule, int res)
>  {
>  	struct audit_buffer *ab;
> -	uid_t loginuid = from_kuid(&init_user_ns, audit_get_loginuid(current));
> -	unsigned int sessionid = audit_get_sessionid(current);
> +	struct tty_struct *tty;
> +	const struct cred *cred;
> +	char comm[sizeof(current->comm)];
>  
>  	if (!audit_enabled)
>  		return;
>  
> +	tty = audit_get_tty(current);
> +	cred = current_cred();
> +
>  	ab = audit_log_start(NULL, GFP_KERNEL, AUDIT_CONFIG_CHANGE);
>  	if (!ab)
>  		return;
> -	audit_log_format(ab, "auid=%u ses=%u" ,loginuid, sessionid);
> +	audit_log_format(ab, "pid=%d uid=%u auid=%u tty=%s ses=%u",
> +			task_tgid_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_log_task_context(ab);
> +	audit_log_format(ab, " comm=");
> +	audit_log_untrustedstring(ab, get_task_comm(comm, current));
> +	audit_log_d_path_exe(ab, current->mm);
>  	audit_log_format(ab, " op=%s", action);
>  	audit_log_key(ab, rule->filterkey);
>  	audit_log_format(ab, " list=%d res=%d", rule->listnr, res);

Again, missing audit_put_tty(tty).

Since these are already standalone records (since the context passed to
audit_log_start() is NULL) this info is necessary.

I'm fine with the field ordering.  If that is not acceptable, I'd
recommend a new record type (AUDIT_TASK) to act as an aux record to this
record that lists this information in a standard order that can be used
as an aux record for all the standalone records that are missing this
information.

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


- RGB

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

Re: [PATCH 1/1] audit: add missing fields to AUDIT_CONFIG_CHANGE event

[Paul Moore]

On Fri, Oct 13, 2017 at 3:54 PM, Richard Guy Briggs <rgb@redhat.com> wrote:
> Since these are already standalone records (since the context passed to
> audit_log_start() is NULL) this info is necessary.

For the record, I don't have a problem with converting standalone
records to syscall accompanied records if that makes sense (not all
audit events can be attributed to a syscall).

Looking purely at the additional information mentioned in this thread,
e.g. pid/uid/session/tty, it would make me believe that these records
*could* be accompanied by a syscall (what is the point of recording
that information if it isn't triggered by a syscall?).  However, I
can't say I've followed all the different fsnotify paths to know if
that is the case ... it may be a mix, and perhaps that would be an
argument for the logging this information in the accompanied SYSCALL
record (it's only recorded when it is valid).

> I'm fine with the field ordering.  If that is not acceptable, I'd
> recommend a new record type (AUDIT_TASK) to act as an aux record to this
> record that lists this information in a standard order that can be used
> as an aux record for all the standalone records that are missing this
> information.

As I just said in the GH issue, I'm not a big fan of the aux record at
the moment (it seems too much of a dup with the SYSCALL record), but
I'm not going to rule it out.

-- 
paul moore
www.paul-moore.com

Re: [PATCH 1/1] audit: add missing fields to AUDIT_CONFIG_CHANGE event

[Richard Guy Briggs]

On 2017-10-13 21:11, Paul Moore wrote:
> On Fri, Oct 13, 2017 at 3:54 PM, Richard Guy Briggs <rgb@redhat.com> wrote:
> > Since these are already standalone records (since the context passed to
> > audit_log_start() is NULL) this info is necessary.
> 
> For the record, I don't have a problem with converting standalone
> records to syscall accompanied records if that makes sense (not all
> audit events can be attributed to a syscall).

I don't either.  I think I've fixed a couple like that in the past when
I thought it made sense.

> Looking purely at the additional information mentioned in this thread,
> e.g. pid/uid/session/tty, it would make me believe that these records
> *could* be accompanied by a syscall (what is the point of recording
> that information if it isn't triggered by a syscall?).  However, I
> can't say I've followed all the different fsnotify paths to know if
> that is the case ... it may be a mix, and perhaps that would be an
> argument for the logging this information in the accompanied SYSCALL
> record (it's only recorded when it is valid).

Ok, fair enough.  There are some records generated by actions that seem
indirect for watches and trees, but I suppose they are all ultimately
triggered by a user action...

The issue I still get stuck with is how do we make sure we put in rules
to catch all the CONFIG_CHANGE instances without getting flooded by all
sorts of other stuff we don't want?

> > I'm fine with the field ordering.  If that is not acceptable, I'd
> > recommend a new record type (AUDIT_TASK) to act as an aux record to this
> > record that lists this information in a standard order that can be used
> > as an aux record for all the standalone records that are missing this
> > information.
> 
> As I just said in the GH issue, I'm not a big fan of the aux record at
> the moment (it seems too much of a dup with the SYSCALL record), but
> I'm not going to rule it out.
> 
> -- 
> paul moore
> www.paul-moore.com

- RGB

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

Re: [PATCH 1/1] audit: add missing fields to AUDIT_CONFIG_CHANGE event

[Paul Moore]

On Mon, Oct 16, 2017 at 11:27 AM, Richard Guy Briggs <rgb@redhat.com> wrote:
> On 2017-10-13 21:11, Paul Moore wrote:
>> On Fri, Oct 13, 2017 at 3:54 PM, Richard Guy Briggs <rgb@redhat.com> wrote:
>> > Since these are already standalone records (since the context passed to
>> > audit_log_start() is NULL) this info is necessary.
>>
>> For the record, I don't have a problem with converting standalone
>> records to syscall accompanied records if that makes sense (not all
>> audit events can be attributed to a syscall).
>
> I don't either.  I think I've fixed a couple like that in the past when
> I thought it made sense.
>
>> Looking purely at the additional information mentioned in this thread,
>> e.g. pid/uid/session/tty, it would make me believe that these records
>> *could* be accompanied by a syscall (what is the point of recording
>> that information if it isn't triggered by a syscall?).  However, I
>> can't say I've followed all the different fsnotify paths to know if
>> that is the case ... it may be a mix, and perhaps that would be an
>> argument for the logging this information in the accompanied SYSCALL
>> record (it's only recorded when it is valid).
>
> Ok, fair enough.  There are some records generated by actions that seem
> indirect for watches and trees, but I suppose they are all ultimately
> triggered by a user action...
>
> The issue I still get stuck with is how do we make sure we put in rules
> to catch all the CONFIG_CHANGE instances without getting flooded by all
> sorts of other stuff we don't want?

My opinion is that is a separate issue related to in-kernel filtering
of audit records and shouldn't affect what we do here.

>> > I'm fine with the field ordering.  If that is not acceptable, I'd
>> > recommend a new record type (AUDIT_TASK) to act as an aux record to this
>> > record that lists this information in a standard order that can be used
>> > as an aux record for all the standalone records that are missing this
>> > information.
>>
>> As I just said in the GH issue, I'm not a big fan of the aux record at
>> the moment (it seems too much of a dup with the SYSCALL record), but
>> I'm not going to rule it out.

-- 
paul moore
www.paul-moore.com

Re: [PATCH 1/1] audit: add missing fields to AUDIT_CONFIG_CHANGE event

[Steve Grubb]

On Monday, October 16, 2017 11:27:06 AM EDT Richard Guy Briggs wrote:
> On 2017-10-13 21:11, Paul Moore wrote:
> > On Fri, Oct 13, 2017 at 3:54 PM, Richard Guy Briggs <rgb@redhat.com> 
wrote:
> > > Since these are already standalone records (since the context passed to
> > > audit_log_start() is NULL) this info is necessary.
> > 
> > For the record, I don't have a problem with converting standalone
> > records to syscall accompanied records if that makes sense (not all
> > audit events can be attributed to a syscall).
> 
> I don't either.  I think I've fixed a couple like that in the past when
> I thought it made sense.

My main objection to this is on wasting disk space and hurting search 
performance. We don't need group information or extended uid information or 
the actual syscall because it will always be a write nor do we need the arch 
or arguments. It also eats more memory when parsing and we have lots of extra 
fields that now need strtok to iterate over. 

I'd almost rather add a task record because its more conservative, but syscall 
is the only kind of event that carries auxiliary records. We'd have to 
probably create something to do that if we did go that way. But it sounds like 
more trouble than just adding a couple required fields.


> > Looking purely at the additional information mentioned in this thread,
> > e.g. pid/uid/session/tty, it would make me believe that these records
> > *could* be accompanied by a syscall (what is the point of recording
> > that information if it isn't triggered by a syscall?).  However, I
> > can't say I've followed all the different fsnotify paths to know if
> > that is the case ... it may be a mix, and perhaps that would be an
> > argument for the logging this information in the accompanied SYSCALL
> > record (it's only recorded when it is valid).
> 
> Ok, fair enough.  There are some records generated by actions that seem
> indirect for watches and trees, but I suppose they are all ultimately
> triggered by a user action...

They all should be. Rules just don't disappear for no reason at all. Its 
because a directory tree got removed or the admin ran auditctl.


> The issue I still get stuck with is how do we make sure we put in rules
> to catch all the CONFIG_CHANGE instances without getting flooded by all
> sorts of other stuff we don't want?

I don't quite follow you here. Its all working fine. It just needs some extra 
fields.

> > > I'm fine with the field ordering.  If that is not acceptable, I'd
> > > recommend a new record type (AUDIT_TASK) to act as an aux record to this
> > > record that lists this information in a standard order that can be used
> > > as an aux record for all the standalone records that are missing this
> > > information.
> > 
> > As I just said in the GH issue, I'm not a big fan of the aux record at
> > the moment (it seems too much of a dup with the SYSCALL record), but
> > I'm not going to rule it out.

The advantage of it is its not as wasteful and incurs less performance hit for 
ausearch. Although parsing a second record with the memory allocations, strtok 
iterations, frees and moving past duplicate timestamp info will slow down 
processing.

-Steve

Re: [PATCH 1/1] audit: add missing fields to AUDIT_CONFIG_CHANGE event

[Paul Moore]

On Mon, Oct 16, 2017 at 5:17 PM, Steve Grubb <sgrubb@redhat.com> wrote:
> On Monday, October 16, 2017 11:27:06 AM EDT Richard Guy Briggs wrote:
>> On 2017-10-13 21:11, Paul Moore wrote:
>> > On Fri, Oct 13, 2017 at 3:54 PM, Richard Guy Briggs <rgb@redhat.com>
> wrote:
>> > > Since these are already standalone records (since the context passed to
>> > > audit_log_start() is NULL) this info is necessary.
>> >
>> > For the record, I don't have a problem with converting standalone
>> > records to syscall accompanied records if that makes sense (not all
>> > audit events can be attributed to a syscall).
>>
>> I don't either.  I think I've fixed a couple like that in the past when
>> I thought it made sense.
>
> My main objection to this is on wasting disk space and hurting search
> performance. We don't need group information or extended uid information or
> the actual syscall because it will always be a write nor do we need the arch
> or arguments. It also eats more memory when parsing and we have lots of extra
> fields that now need strtok to iterate over.
>
> I'd almost rather add a task record because its more conservative, but syscall
> is the only kind of event that carries auxiliary records. We'd have to
> probably create something to do that if we did go that way. But it sounds like
> more trouble than just adding a couple required fields.

See my comments from today in the multicast/bind patch thread; my
preference is to associate this with an existing audit event.  If that
isn't possible, or desirable from your perspective, then we should add
the new fields at the end.  As far as I'm concerned, at this
particular moment those are the only options that I would be willing
to merge into the kernel.

-- 
paul moore
www.paul-moore.com