From mboxrd@z Thu Jan  1 00:00:00 1970
From: Lisa Smith <lisa.m.smith@hp.com>
Subject: Re: [RFC: PATCH] Audit Failure Query Functionality
Date: Wed, 14 Jun 2006 14:05:29 -0400
Message-ID: <44904FE9.4000407@hp.com>
References: <448F096E.8060703@hp.com> <1150234246.29837.29.camel@code.and.org>
Mime-Version: 1.0
Content-Type: text/plain; charset=ISO-8859-1
Content-Transfer-Encoding: 7bit
Return-path: <redhat-lspp-bounces@redhat.com>
In-Reply-To: <1150234246.29837.29.camel@code.and.org>
List-Unsubscribe: <https://www.redhat.com/mailman/listinfo/redhat-lspp>,
	<mailto:redhat-lspp-request@redhat.com?subject=unsubscribe>
List-Archive: <https://www.redhat.com/archives/redhat-lspp>
List-Post: <mailto:redhat-lspp@redhat.com>
List-Help: <mailto:redhat-lspp-request@redhat.com?subject=help>
List-Subscribe: <https://www.redhat.com/mailman/listinfo/redhat-lspp>,
	<mailto:redhat-lspp-request@redhat.com?subject=subscribe>
Sender: redhat-lspp-bounces@redhat.com
Errors-To: redhat-lspp-bounces@redhat.com
To: James Antill <jantill@redhat.com>
Cc: LSPP <redhat-lspp@redhat.com>, linux-audit@redhat.com
List-Id: linux-audit@redhat.com

James,

>> +       /* Translate tunable string to valid enum */
>> +       if (strncmp(nv.value, AUDIT_FAIL_IGNORE,
>> +               strlen(AUDIT_FAIL_IGNORE)) == 0) {
> 
>  This means that "ignores" will be valid, as will "logout".

Hmm... Good point.  I'll see what I can do about that.

>> +       while ((bytesread = getline(&buf, &len, fp)) != -1) {
>> +
>> +               if (buf[0] == '#') {
>> +                       lineno++;
>> +                       continue;       // Ignore comments
>> +               }
>> +
>> +               /* Convert line into name-value pair */
>> +               rc = nv_split(buf, nv);
> 
>  The values in nv are leaked when there isn't a match or an error.
>
>> +int nv_split(char *buffer, struct nv_pair *nv)
>> +{
>> +       /* Get the name part */
>> +       char *saveptr, *ptr = NULL;
>> +       char *buf = strdup(buffer);
> 
>  This is always leaked.

Good catches. I've fixed both these leaks.

>> +       /* Look for = in buf */
>> +       nv->name = NULL;
>> +       nv->value = NULL;
>> +       ptr = strtok_r(buf, " =", &saveptr);
>> +       if ((ptr == NULL) || !(strcmp(ptr,"\n"))) {
>> +               return 0;   // If there's nothing, go to next line
>> +       }
>> +       nv->name = strdup(ptr);
>> +
>> +        /* Get the keyword value */
>> +       ptr = strtok_r(NULL, " =", &saveptr);
> 
>  I appreciate this is somewhat easier given C's default string API, but
> it would be really nice to do the right thing if the user uses "x=y"
> instead of needing "x =y".
>  This also isn't how auditd parses the it's file.

Actually, this code will handle "x=y", "x =y", "x= y" and "x = y".

>> +/* These are the valid audit failure tunable enum values */
>> +typedef enum {
>> +       ERR=-1,
>> +       IGNORE=0,
>> +       LOG,
>> +       TERM
>> +} auditfail_t;
> 
>  These enum values should be namespaced esp. as they are very generic
> names.
> 
>> +
>> +/* #defines for the audit failure query  */
>> +#define AUDIT_FAIL_CONFIG      "/etc/libaudit.conf"
>> +#define AUDIT_FAIL_KEYWORD     "auditfailure"
>> +#define AUDIT_FAIL_IGNORE      "ignore"
>> +#define AUDIT_FAIL_LOG         "log"
>> +#define AUDIT_FAIL_TERM        "terminate"
>> +
>> +/* Name-value pair */
>> +struct nv_pair
>> +{
>> +       char *name;
>> +       char *value;
>> +};
>> +
> 
>  This should be namespaced.

Will do.

>>  /*
>>   * audit_rule_data supports filter rules with both integer and string
>>   * fields. It corresponds with AUDIT_ADD_RULE, AUDIT_DEL_RULE and
>> @@ -362,6 +384,9 @@
>>  /* AUDIT_GET */
>>  extern int audit_request_status(int fd);
>>  extern int audit_is_enabled(int fd);
>> +extern auditfail_t audit_failure_action(char *file);
>> +static int search_audituser_conf(char *file, char *keyword, struct nv_pair *nv);
>> +static int nv_split(char *buf, struct nv_pair *nv);
> 
>  These shouldn't be in the public .h file.

I'll move these declarations.

Thanks for the comments.

Lisa