[PATCH] make it match explicitly when use option '-a', '-A' and '-d' to specify "list,action"

[PATCH] make it match explicitly when use option '-a', '-A' and '-d' to specify "list,action"

[Yu Zhiguo]

Hello Steve,

I know "list" and "action" can be changed, this is convenient.
But wildcard match maybe make user confused, for example "auditctl -a noentry,noalways"
will add a rule same with "auditctl -a entry,always".

furthermore, comma must be used to seperate list and action according to manpage:
   "Please note the comma separating the two values. Omitting it will cause errors."
but now, "auditctl -a entryalways" will add the same rule.

So we'd better make it match explicitly. This is a patch for latest audit-1.7.4.

Signed-off-by: Yu Zhiguo<yuzg@cn.fujitsu.com>
---
  src/auditctl.c |   25 ++++++++++++++++---------
  1 files changed, 16 insertions(+), 9 deletions(-)

diff --git a/src/auditctl.c b/src/auditctl.c
index 2c136ea..1aba437 100644
--- a/src/auditctl.c
+++ b/src/auditctl.c
@@ -168,27 +168,34 @@ static void usage(void)
  /* Returns 0 ok, 1 deprecated action, 2 error */
  static int audit_rule_setup(const char *opt, int *flags, int *act)
  {
-	if (strstr(opt, "task"))
+	char *p;
+	if ((strchr(opt, ',') != strrchr(opt, ',')) || !strchr(opt, ','))
+		return 2;
+
+	p = strchr(opt, ',');
+	if (!strncmp(opt, "task,", p - opt + 1) || !strcmp(p, ",task"))
  		*flags = AUDIT_FILTER_TASK;
-	else if (strstr(opt, "entry"))
+	else if (!strncmp(opt, "entry,", p - opt + 1) || !strcmp(p, ",entry"))
  		*flags = AUDIT_FILTER_ENTRY;
-	else if (strstr(opt, "exit"))
+	else if (!strncmp(opt, "exit,", p - opt + 1) || !strcmp(p, ",exit"))
  		*flags = AUDIT_FILTER_EXIT;
-	else if (strstr(opt, "user"))
+	else if (!strncmp(opt, "user,", p - opt + 1) || !strcmp(p, ",user"))
  		*flags = AUDIT_FILTER_USER;
-	else if (strstr(opt, "exclude")) {
+	else if (!strncmp(opt, "exclude,", p - opt + 1) || !strcmp(p, ",exclude")) {
  		*flags = AUDIT_FILTER_EXCLUDE;
  		exclude = 1;
  	} else
  		return 2;
-	if (strstr(opt, "never"))
+
+	if (!strncmp(opt, "always,", p - opt + 1) || !strcmp(p, ",always"))
+		*act = AUDIT_ALWAYS;
+	else if (!strncmp(opt, "never,", p - opt + 1) || !strcmp(p, ",never"))
  		*act = AUDIT_NEVER;
-	else if (strstr(opt, "possible"))
+	else if (!strncmp(opt, "possible,", p - opt + 1) || !strcmp(p, ",possible"))
  		return 1;
-	else if (strstr(opt, "always"))
-		*act = AUDIT_ALWAYS;
  	else
  		return 2;
+
  	return 0;
  }

Re: [PATCH] make it match explicitly when use option '-a', '-A' and '-d' to specify "list,action"

[Miloslav Trmač]

Hello,
Yu Zhiguo píše v Pá 18. 07. 2008 v 14:54 +0800:
> I know "list" and "action" can be changed, this is convenient.
No, it is undocumented.  As an author of system-config-audit I'd much
prefer if audit rejected such options, replicating the exact code in
auditctl in order to handle all undocumented behavior the same way as
auditctl is rather impractical.

> diff --git a/src/auditctl.c b/src/auditctl.c
> index 2c136ea..1aba437 100644
> --- a/src/auditctl.c
> +++ b/src/auditctl.c
> @@ -168,27 +168,34 @@ static void usage(void)
>   /* Returns 0 ok, 1 deprecated action, 2 error */
>   static int audit_rule_setup(const char *opt, int *flags, int *act)
>   {
> +	char *p;
> +	if ((strchr(opt, ',') != strrchr(opt, ',')) || !strchr(opt, ','))
> +		return 2;
> +
> +	p = strchr(opt, ',');
I think
	p = strchr(opt, ',');
	if (p == NULL || strchr(p + 1, ',') != NULL)
		return 2;
would be simpler.

> -	if (strstr(opt, "task"))
> +	if (!strncmp(opt, "task,", p - opt + 1) || !strcmp(p, ",task"))
>   		*flags = AUDIT_FILTER_TASK;
Each string should be recognized only in the documented position IMHO.
The patch also replaces case-sensitive matching by case-insensitive,
which is not described above.

If such changes in the semantics of the parameter are accepted, at
minimum the auditctl.8 man page should be updated as well.
	Mirek

Re: [PATCH] make it match explicitly when use option '-a', '-A' and '-d' to specify "list,action"

[Yu Zhiguo]

Hello Miloslav,
CC Steve,

  Thanks for you comment,

Miloslav Trmač wrote:
>> I know "list" and "action" can be changed, this is convenient.
> No, it is undocumented.  As an author of system-config-audit I'd much
> prefer if audit rejected such options, replicating the exact code in
> auditctl in order to handle all undocumented behavior the same way as
> auditctl is rather impractical.

Indeed it is uncompatible with manpage, but it seems that Mr. Steve
like this convenient method:
=======refer to mail from Steve(2008/05/08)====================
  > I am a little surprised that the "-a always,exit" doesn't cause an
  > > error. I wonder if it works correctly - maybe auditctl code is smart
  > > enough to overcome syntactic dyslexia?  :)

  I was always mixing up the order when writing rules, so I fixed auditctl to
  take it in either order. Its been like this for 3-4 years.


> 	p = strchr(opt, ',');
> 	if (p == NULL || strchr(p + 1, ',') != NULL)
> 		return 2;
> would be simpler.
I think so, thanks..

> 
>> -	if (strstr(opt, "task"))
>> +	if (!strncmp(opt, "task,", p - opt + 1) || !strcmp(p, ",task"))
>>   		*flags = AUDIT_FILTER_TASK;
> Each string should be recognized only in the documented position IMHO.
> The patch also replaces case-sensitive matching by case-insensitive,
> which is not described above.

Both strstr and strcmp are case-sensitive.

> 
> If such changes in the semantics of the parameter are accepted, at
> minimum the auditctl.8 man page should be updated as well.
I think the manpage should introduce that the order of "list"
and "action" can be changed.
What's the opinion of Mr. Steve? If you think the manpage is right,
we should correct the code to compatible with it, and I'll make
another patch.


if order insensitive is right, the simpler patch now:
---
  src/auditctl.c |   26 +++++++++++++++++---------
  1 files changed, 17 insertions(+), 9 deletions(-)

diff --git a/src/auditctl.c b/src/auditctl.c
index 2c136ea..a268bcf 100644
--- a/src/auditctl.c
+++ b/src/auditctl.c
@@ -168,27 +168,35 @@ static void usage(void)
  /* Returns 0 ok, 1 deprecated action, 2 error */
  static int audit_rule_setup(const char *opt, int *flags, int *act)
  {
-	if (strstr(opt, "task"))
+	char *p;
+
+	p = strchr(opt, ',');
+	if (!p || strchr(p + 1, ','))
+		return 2;
+
+	if (!strncmp(opt, "task,", p - opt + 1) || !strcmp(p, ",task"))
  		*flags = AUDIT_FILTER_TASK;
-	else if (strstr(opt, "entry"))
+	else if (!strncmp(opt, "entry,", p - opt + 1) || !strcmp(p, ",entry"))
  		*flags = AUDIT_FILTER_ENTRY;
-	else if (strstr(opt, "exit"))
+	else if (!strncmp(opt, "exit,", p - opt + 1) || !strcmp(p, ",exit"))
  		*flags = AUDIT_FILTER_EXIT;
-	else if (strstr(opt, "user"))
+	else if (!strncmp(opt, "user,", p - opt + 1) || !strcmp(p, ",user"))
  		*flags = AUDIT_FILTER_USER;
-	else if (strstr(opt, "exclude")) {
+	else if (!strncmp(opt, "exclude,", p - opt + 1) || !strcmp(p, ",exclude")) {
  		*flags = AUDIT_FILTER_EXCLUDE;
  		exclude = 1;
  	} else
  		return 2;
-	if (strstr(opt, "never"))
+
+	if (!strncmp(opt, "always,", p - opt + 1) || !strcmp(p, ",always"))
+		*act = AUDIT_ALWAYS;
+	else if (!strncmp(opt, "never,", p - opt + 1) || !strcmp(p, ",never"))
  		*act = AUDIT_NEVER;
-	else if (strstr(opt, "possible"))
+	else if (!strncmp(opt, "possible,", p - opt + 1) || !strcmp(p, ",possible"))
  		return 1;
-	else if (strstr(opt, "always"))
-		*act = AUDIT_ALWAYS;
  	else
  		return 2;
+
  	return 0;
  }

Re: [PATCH] make it match explicitly when use option '-a', '-A' and '-d' to specify "list,action"

[Miloslav Trmač]

Hello,
Yu Zhiguo píše v Pá 18. 07. 2008 v 19:52 +0800:
> Miloslav Trmač wrote:
> >> I know "list" and "action" can be changed, this is convenient.
> > No, it is undocumented.  As an author of system-config-audit I'd much
> > prefer if audit rejected such options, replicating the exact code in
> > auditctl in order to handle all undocumented behavior the same way as
> > auditctl is rather impractical.
> 
> Indeed it is uncompatible with manpage, but it seems that Mr. Steve
> like this convenient method:
Oh.  OK.

> >> -	if (strstr(opt, "task"))
> >> +	if (!strncmp(opt, "task,", p - opt + 1) || !strcmp(p, ",task"))
> >>   		*flags = AUDIT_FILTER_TASK;
> > Each string should be recognized only in the documented position IMHO.
> > The patch also replaces case-sensitive matching by case-insensitive,
> > which is not described above.
> 
> Both strstr and strcmp are case-sensitive.
You're obviously right, I'm sorry.  I wasn't paying enough attention.
	Mirek

Re: [PATCH] make it match explicitly when use option '-a', '-A' and '-d' to specify "list,action"

[Yu Zhiguo]

Hello Steve,

  What your opinion about this patch?
  Perhaps you think we'd better be compatible with the manpage now.
So I made another patch according to the introduction of manpage.

  Whether there is a comma should be check because it is said in
the manpage:
   Please note the comma separating the two values. Omitting it
will cause errors.

  Then 'list' and 'action' will be obtained separately.

  Do you agree with me? This is the new patch for latest code in
audit SVN project.

Signed-off-by: Yu Zhiguo<yuzg@cn.fujitsu.com>
---
  src/auditctl.c |   28 +++++++++++++++++++---------
  1 files changed, 19 insertions(+), 9 deletions(-)

diff --git a/src/auditctl.c b/src/auditctl.c
index d740509..dbd086e 100644
--- a/src/auditctl.c
+++ b/src/auditctl.c
@@ -172,31 +172,41 @@ static void usage(void)
  static int audit_rule_setup(const char *opt, int *flags, int *act)
  {
  	static int multiple = 0;
+	char *p;

  	if (++multiple != 1)
  		return 3;

-	if (strstr(opt, "task"))
+	/* comma separating */
+	p = strchr(opt, ',');
+	if (!p || strchr(p + 1, ','))
+		return 2;
+
+	/* obtain list */
+	if (!strncmp(opt, "task", p - opt))
  		*flags = AUDIT_FILTER_TASK;
-	else if (strstr(opt, "entry"))
+	else if (!strncmp(opt, "entry", p - opt))
  		*flags = AUDIT_FILTER_ENTRY;
-	else if (strstr(opt, "exit"))
+	else if (!strncmp(opt, "exit", p - opt))
  		*flags = AUDIT_FILTER_EXIT;
-	else if (strstr(opt, "user"))
+	else if (!strncmp(opt, "user", p - opt))
  		*flags = AUDIT_FILTER_USER;
-	else if (strstr(opt, "exclude")) {
+	else if (!strncmp(opt, "exclude", p - opt)) {
  		*flags = AUDIT_FILTER_EXCLUDE;
  		exclude = 1;
  	} else
  		return 2;
-	if (strstr(opt, "never"))
+
+	/* obtain action */
+	if (!strcmp(p + 1, "always"))
+		*act = AUDIT_ALWAYS;
+	else if (!strcmp(p + 1, "never"))
  		*act = AUDIT_NEVER;
-	else if (strstr(opt, "possible"))
+	else if (!strcmp(p + 1, "possible"))
  		return 1;
-	else if (strstr(opt, "always"))
-		*act = AUDIT_ALWAYS;
  	else
  		return 2;
+
  	return 0;
  }

Re: [PATCH] make it match explicitly when use option '-a', '-A' and '-d' to specify "list,action"

[Yu Zhiguo]

Hello, Steve,


Yu Zhiguo wrote:
>  Perhaps you think we'd better be compatible with the manpage now.
> So I made another patch according to the introduction of manpage.

  I'm sorry I had made a mistake in this patch. Now I make a new patch
for the for latest code in audit SVN project.
  I know the method to correct this bug is not very beautiful, but I
think this is the most efficient and simplest method.
  Hope your indication.


Signed-off-by: Yu Zhiguo<yuzg@cn.fujitsu.com>
---
  src/auditctl.c |   28 +++++++++++++++++++---------
  1 files changed, 19 insertions(+), 9 deletions(-)

diff --git a/src/auditctl.c b/src/auditctl.c
index d740509..26028b9 100644
--- a/src/auditctl.c
+++ b/src/auditctl.c
@@ -172,31 +172,41 @@ static void usage(void)
  static int audit_rule_setup(const char *opt, int *flags, int *act)
  {
  	static int multiple = 0;
+	char *p;

  	if (++multiple != 1)
  		return 3;

-	if (strstr(opt, "task"))
+	/* comma separating */
+	p = strchr(opt, ',');
+	if (!p || strchr(p + 1, ','))
+		return 2;
+
+	/* obtain list */
+	if (!strncmp(opt, "task,", p - opt + 1))
  		*flags = AUDIT_FILTER_TASK;
-	else if (strstr(opt, "entry"))
+	else if (!strncmp(opt, "entry,", p - opt + 1))
  		*flags = AUDIT_FILTER_ENTRY;
-	else if (strstr(opt, "exit"))
+	else if (!strncmp(opt, "exit,", p - opt + 1))
  		*flags = AUDIT_FILTER_EXIT;
-	else if (strstr(opt, "user"))
+	else if (!strncmp(opt, "user,", p - opt + 1))
  		*flags = AUDIT_FILTER_USER;
-	else if (strstr(opt, "exclude")) {
+	else if (!strncmp(opt, "exclude,", p - opt + 1)) {
  		*flags = AUDIT_FILTER_EXCLUDE;
  		exclude = 1;
  	} else
  		return 2;
-	if (strstr(opt, "never"))
+
+	/* obtain action */
+	if (!strcmp(p + 1, "always"))
+		*act = AUDIT_ALWAYS;
+	else if (!strcmp(p + 1, "never"))
  		*act = AUDIT_NEVER;
-	else if (strstr(opt, "possible"))
+	else if (!strcmp(p + 1, "possible"))
  		return 1;
-	else if (strstr(opt, "always"))
-		*act = AUDIT_ALWAYS;
  	else
  		return 2;
+
  	return 0;
  }

Re: [PATCH] make it match explicitly when use option '-a', '-A' and '-d' to specify "list,action"

[Steve Grubb]

On Wednesday 30 July 2008 20:57:23 Yu Zhiguo wrote:
>   I'm sorry I had made a mistake in this patch. Now I make a new patch
> for the for latest code in audit SVN project.
>   I know the method to correct this bug is not very beautiful, but I
> think this is the most efficient and simplest method.
>   Hope your indication.

OK, I understand what you were trying to do with this patch. But I opted to 
make a more complicated patch that separates the comparisons with the logic 
of where it goes. SVN now has a commit that should fix the problem that you 
found.  Thanks for reporting this problem!

-Steve

[PATCH] the usage of strchr is wrong

[Yu Zhiguo]

Hello Steve,

Steve Grubb wrote:
> OK, I understand what you were trying to do with this patch. But I opted to 
> make a more complicated patch that separates the comparisons with the logic 
> of where it goes. SVN now has a commit that should fix the problem that you 
> found.  Thanks for reporting this problem!

Ok, but I tested this commit a moment ago. I think this is a bug about usage
of strchr().

strchr returns a pointer, this pointer should be given to 'p' directly.

Signed-off-by: Yu Zhiguo<yuzg@cn.fujitsu.com>
---
  src/auditctl.c |    2 +-
  1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/src/auditctl.c b/src/auditctl.c
index 1053638..868f770 100644
--- a/src/auditctl.c
+++ b/src/auditctl.c
@@ -209,7 +209,7 @@ static int audit_rule_setup(char *opt, int *filter, int *act)
  	if (++multiple != 1)
  		return 3;

-	*p = strchr(opt, ',');
+	p = strchr(opt, ',');
  	if (p == NULL || strchr(p+1, ','))
  		return 2;
  	*p = 0;

Re: [PATCH] the usage of strchr is wrong

[Yu Zhiguo]

Hello Steve,

Yu Zhiguo wrote:
> Ok, but I tested this commit a moment ago. I think this is a bug about 
> usage
> of strchr().

We'd better correct this bug ASAP. Otherwise no rule can be added/deleted
and the error message reported is very strange, e.g.

# ./auditctl -a entry,always
Append rule - bad keyword Wntry,always
# ./auditctl -d never,exit
Delete rule - bad keyword Yever,exit


> 
> strchr returns a pointer, this pointer should be given to 'p' directly.
> 
> Signed-off-by: Yu Zhiguo<yuzg@cn.fujitsu.com>
> ---
>  src/auditctl.c |    2 +-
>  1 files changed, 1 insertions(+), 1 deletions(-)
> 
> diff --git a/src/auditctl.c b/src/auditctl.c
> index 1053638..868f770 100644
> --- a/src/auditctl.c
> +++ b/src/auditctl.c
> @@ -209,7 +209,7 @@ static int audit_rule_setup(char *opt, int *filter, 
> int *act)
>      if (++multiple != 1)
>          return 3;
> 
> -    *p = strchr(opt, ',');
> +    p = strchr(opt, ',');
>      if (p == NULL || strchr(p+1, ','))
>          return 2;
>      *p = 0;

Re: [PATCH] the usage of strchr is wrong

[Steve Grubb]

On Monday 04 August 2008 22:14:15 Yu Zhiguo wrote:
> strchr returns a pointer, this pointer should be given to 'p' directly.

That's what I meant. :)   Fixed.

Thanks,
-Steve