make it match explicitly when use option '-a', '-A' and '-d' to specify "list,action"
PATCH 1/1 make it match explicitly when use option '-a', '-A' and '-d' to specify "list,action"
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.
--- src/auditctl.c | 25 ++++++++++++++++--------- 1 files changed, 16 insertions(+), 9 deletions(-)
src/auditctl.c +16 -9
@@ -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 ; }
Hello,
Yu Zhiguo píše v Pá 18. 07. 2008 v 14:54 +0800:
Quoted 1 line
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.
src/auditctl.c static void usage(void) · line 175
Quoted 13 lines
src/auditctl.c +5
-0@@ -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.
src/auditctl.c static void usage(void) · line 177
Quoted 3 lines
src/auditctl.c +1
-1-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
Hello Miloslav,
CC Steve,
Thanks for you comment,
Quoted 5 lines · Miloslav Trmač
Quoted 1 line
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)====================
Quoted 3 lines
I am a little surprised that the "-a always,exit" doesn't cause an
Quoted 2 lines
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.
Quoted 4 lines
p = strchr(opt, ','); if (p == NULL || strchr(p + 1, ',') != NULL) return 2;would be simpler.
I think so, thanks..
Quoted 7 lines
Review
Quoted 4 lines
(quoted diff) +1
-1- 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.
Quoted 3 lines
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(-)
src/auditctl.c +17 -9
@@ -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 ; }
Hello,
Yu Zhiguo píše v Pá 18. 07. 2008 v 19:52 +0800:
Quoted 9 lines
Quoted 5 lines · Miloslav Trmač
Quoted 1 line
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.
Quoted 8 lines
Quoted 6 lines
src/auditctl.c static void usage(void) · line 178
Quoted 3 lines
src/auditctl.c +1
-1-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
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.
--- src/auditctl.c | 28 +++++++++++++++++++--------- 1 files changed, 19 insertions(+), 9 deletions(-)
src/auditctl.c +19 -9
@@ -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 ; }
Hello, Steve,
Quoted 2 lines · Yu Zhiguo
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.
--- src/auditctl.c | 28 +++++++++++++++++++--------- 1 files changed, 19 insertions(+), 9 deletions(-)
src/auditctl.c +19 -9
@@ -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 ; }
Quoted 5 lines · 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 1/1 the usage of strchr is wrong
Hello Steve,
Quoted 1 line · Steve Grubb
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.
--- src/auditctl.c | 2 +- 1 files changed, 1 insertions(+), 1 deletions(-)
src/auditctl.c +1 -1
@@ -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
Hello Steve,
Quoted 2 lines · Yu Zhiguo
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
Quoted 22 lines
strchr returns a pointer, this pointer should be given to 'p' directly.
Signed-off-by Yu Zhiguo<[email protected]>--- src/auditctl.c | 2 +- 1 files changed, 1 insertions(+), 1 deletions(-)src/auditctl.c +1
-1@@ -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
In reply to Yu ZhiguoQuoted 1 line · 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