[RFC: PATCH] Audit Failure Query Functionality

[RFC: PATCH] Audit Failure Query Functionality

[Lisa Smith]

This is this initial patch for the audit failure query functionality.

Currently, each service and application is responsible for determining
the action to take when the audit subsystem is unavailable (stop the
action or service, continue on, log a message).  Different
customers may want different actions taken, and this flexibility is
not possible if the action is hard-coded into the application.

This patch introduces a new query function,
audit_failure_action(), which can be called to determine what
action should be taken by the service after audit_open() fails.
The function will read the tunable "auditfailure" from the
new /etc/libaudit.conf file to determine what action should be
taken.  audit_failure_action() can also be passed a filename
so that an application-specific action can be defined within its
own file.

The audit_open() man page will be updated to introduce
application developers to the new query function that should
be used when audit_open() returns failure.  A new man page
will also be created for the audit_failure_action() function.

This patch is backed against audit version 1.2.3.

Lisa

----

 libaudit.c |  163 +++++++++++++++++++++++++++++++++++++++++++++++++++++
 libaudit.h |   25 ++++++++
 2 files changed, 188 insertions(+)

diff -burN orig/libaudit.c src/libaudit.c
--- orig/libaudit.c     2006-05-25 17:39:52.000000000 -0400
+++ src/libaudit.c      2006-06-13 14:46:33.000000000 -0400
@@ -68,6 +68,169 @@
        return rc;
 }

+/*
+ * This function will retrieve the audit failure tunable value
+ * from the filename passed in.  If no file is specified, the default
+ * /etc/libaudit.conf file will be used.
+ */
+auditfail_t audit_failure_action(char *file)
+{
+       int ret;
+       struct nv_pair nv;
+
+       if (file == NULL)
+               file = AUDIT_FAIL_CONFIG;
+
+       /* Find the audit failure action tunable in the config file */
+       ret = search_audituser_conf(file, AUDIT_FAIL_KEYWORD, &nv);
+
+       if (ret == -1) {
+               audit_msg(LOG_WARNING, "Error in %s", file);
+               return ERR;
+       } else if (ret == 1) {
+               /* Keyword not found, so do the default action */
+               return IGNORE;
+       }
+
+       /* Translate tunable string to valid enum */
+       if (strncmp(nv.value, AUDIT_FAIL_IGNORE,
+               strlen(AUDIT_FAIL_IGNORE)) == 0) {
+               free (nv.name);
+               free (nv.value);
+               return IGNORE;
+       }
+       else if (strncmp(nv.value, AUDIT_FAIL_LOG,
+               strlen(AUDIT_FAIL_LOG)) == 0) {
+               free (nv.name);
+               free (nv.value);
+               return LOG;
+        }
+       else if (strncmp(nv.value, AUDIT_FAIL_TERM,
+               strlen(AUDIT_FAIL_TERM)) == 0) {
+               free (nv.name);
+               free (nv.value);
+               return TERM;
+        }
+       else {
+               /* If we get this far, the failure action was not valid */
+               audit_msg(LOG_ERR, "Invalid %s keyword value in %s",
+                       AUDIT_FAIL_KEYWORD, file);
+               free (nv.name);
+               free (nv.value);
+               return ERR;
+       }
+}
+
+/*
+ * This function searches for a keyword pair in the passed in filename.
+ * If the keyword pair is found, it is saved in the nv structure and zero
+ * is returned. If the file can not be opened, -1 is returned.
+ * If the keyword is not found in the file, 1 is returned.
+ *
+ * nv->name and nv->value must be freed if an error will be returned from this
+ * function after nv_split() is called. If this function returns success,
+ * the caller must free nv->name and nv->value when finished using the
+ * values.
+ */
+int search_audituser_conf(char *file, char *keyword, struct nv_pair *nv)
+{
+       int rc, lineno = 1;
+       size_t len = 0;
+       ssize_t bytesread;
+       FILE *fp;
+       char *buf = NULL;
+
+       /* Open the file for line by line reading*/
+       fp = fopen(file, "r");
+       if (fp == NULL) {
+               audit_msg(LOG_ERR, "Error - fdopen failed for %s (%s)",
+                       file, strerror(errno));
+               return -1;
+        }
+
+       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);
+               if (rc == 1) {
+                       audit_msg(LOG_ERR, "Error on line %d in %s", lineno,
+                               file);
+                       lineno++;
+                       continue;
+                }
+
+               /* Find the name-value pair */
+               if (strcmp(nv->name, keyword) == 0)
+               {
+                       fclose(fp);
+                       if (buf)
+                               free (buf);
+                       return 0;
+               }
+
+               lineno++;
+       }
+
+       /* If we get here, the keyword was not found in the file */
+       audit_msg(LOG_ERR, "Keyword %s not found in %s", keyword, file);
+       fclose(fp);
+       if (buf)
+               free (buf);
+       if (nv->name)
+               free (nv->name);
+       if (nv->value)
+               free (nv->value);
+       return 1;
+}
+
+/*
+ *  This function parses a line looking for a keyword = value pair
+ *  and if found, returns it in the nv structure. If the function
+ *  returns success, the calling function is expected to free
+ *  nv->name and nv->value.
+ */
+int nv_split(char *buffer, struct nv_pair *nv)
+{
+       /* Get the name part */
+       char *saveptr, *ptr = NULL;
+       char *buf = strdup(buffer);
+
+       /* 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);
+       if (ptr == NULL) {
+               free (nv->name);
+               return 1;
+       }
+       nv->value = strdup(ptr);
+
+       /* Make sure there's nothing else on the line */
+       ptr = strtok_r(NULL, " ", &saveptr);
+       if (ptr) {
+               free (nv->name);
+               free (nv->value);
+               return 1;
+       }
+
+       /* Everything is OK */
+       return 0;
+}
+
+
+
 int audit_set_enabled(int fd, uint32_t enabled)
 {
        int rc;
diff -burN orig/libaudit.h src/libaudit.h
--- orig/libaudit.h     2006-05-25 17:38:21.000000000 -0400
+++ src/libaudit.h      2006-06-13 13:01:30.000000000 -0400
@@ -248,6 +248,28 @@
         MACH_ALPHA
 } machine_t;

+/* These are the valid audit failure tunable enum values */
+typedef enum {
+       ERR=-1,
+       IGNORE=0,
+       LOG,
+       TERM
+} auditfail_t;
+
+/* #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;
+};
+
 /*
  * 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);

 /* AUDIT_SET */
 typedef enum { WAIT_NO, WAIT_YES } rep_wait_t;

Re: [RFC: PATCH] Audit Failure Query Functionality

[Timothy R. Chavez]

On Tue, 2006-06-13 at 14:52 -0400, Lisa Smith wrote:
> This is this initial patch for the audit failure query functionality.
> 
<snip>
> 
> This patch is backed against audit version 1.2.3.
> 
> Lisa
> 

Hello Lisa,

I'm a little rusty... I just had a few things..

-tim

> ----
> 
>  libaudit.c |  163 +++++++++++++++++++++++++++++++++++++++++++++++++++++
>  libaudit.h |   25 ++++++++
>  2 files changed, 188 insertions(+)
> 
> diff -burN orig/libaudit.c src/libaudit.c
> --- orig/libaudit.c     2006-05-25 17:39:52.000000000 -0400
> +++ src/libaudit.c      2006-06-13 14:46:33.000000000 -0400
> @@ -68,6 +68,169 @@
>         return rc;
>  }
> 
> +/*
> + * This function will retrieve the audit failure tunable value
> + * from the filename passed in.  If no file is specified, the default
> + * /etc/libaudit.conf file will be used.
> + */
> +auditfail_t audit_failure_action(char *file)
> +{
> +       int ret;
> +       struct nv_pair nv;
> +
> +       if (file == NULL)
> +               file = AUDIT_FAIL_CONFIG;
> +
> +       /* Find the audit failure action tunable in the config file */
> +       ret = search_audituser_conf(file, AUDIT_FAIL_KEYWORD, &nv);
> +
> +       if (ret == -1) {
> +               audit_msg(LOG_WARNING, "Error in %s", file);

Would you not want to use LOG_ERR here?  Or if you really did intend a
LOG_WARNING using the word "Error" in your message might be confusing. 

[..]
> +               return ERR;

This is really awkward to me.  I think it makes more sense to make this
function simply return success or failure and update a parameter passed
to it by the user with the correct failure mode.  As it stands now, ERR
isn't really a failure mode.  May I also suggest making your enumeration
a little more descriptive?

[..]
> +       } else if (ret == 1) {
> +               /* Keyword not found, so do the default action */
> +               return IGNORE;
> +       }
> +
> +       /* Translate tunable string to valid enum */
> +       if (strncmp(nv.value, AUDIT_FAIL_IGNORE,
> +               strlen(AUDIT_FAIL_IGNORE)) == 0) {
> +               free (nv.name);
> +               free (nv.value);
> +               return IGNORE;
> +       }
> +       else if (strncmp(nv.value, AUDIT_FAIL_LOG,
> +               strlen(AUDIT_FAIL_LOG)) == 0) {
> +               free (nv.name);
> +               free (nv.value);
> +               return LOG;
> +        }
> +       else if (strncmp(nv.value, AUDIT_FAIL_TERM,
> +               strlen(AUDIT_FAIL_TERM)) == 0) {
> +               free (nv.name);
> +               free (nv.value);
> +               return TERM;
> +        }
> +       else {
> +               /* If we get this far, the failure action was not valid */
> +               audit_msg(LOG_ERR, "Invalid %s keyword value in %s",
> +                       AUDIT_FAIL_KEYWORD, file);
> +               free (nv.name);
> +               free (nv.value);
> +               return ERR;
> +       }

Looks like there's a lot of duplicate code here.  This can be better
organized.

[..]
> +}
> +
> +/*
> + * This function searches for a keyword pair in the passed in filename.
> + * If the keyword pair is found, it is saved in the nv structure and zero
> + * is returned. If the file can not be opened, -1 is returned.
> + * If the keyword is not found in the file, 1 is returned.
> + *
> + * nv->name and nv->value must be freed if an error will be returned from this
> + * function after nv_split() is called. If this function returns success,
> + * the caller must free nv->name and nv->value when finished using the
> + * values.
> + */
> +int search_audituser_conf(char *file, char *keyword, struct nv_pair *nv)
> +{
> +       int rc, lineno = 1;
> +       size_t len = 0;
> +       ssize_t bytesread;
> +       FILE *fp;
> +       char *buf = NULL;
> +
> +       /* Open the file for line by line reading*/
> +       fp = fopen(file, "r");
> +       if (fp == NULL) {
> +               audit_msg(LOG_ERR, "Error - fdopen failed for %s (%s)",
> +                       file, strerror(errno));
> +               return -1;
> +        }

Why return -1, why not return errno?

[..]
> +
> +       while ((bytesread = getline(&buf, &len, fp)) != -1) {
> +
> +               if (buf[0] == '#') {
> +                       lineno++;
> +                       continue;       // Ignore comments
> +               }

So will this blow up if buf[0] == ' ' ?

[..]
> +
> +               /* Convert line into name-value pair */
> +               rc = nv_split(buf, nv);
> +               if (rc == 1) {
> +                       audit_msg(LOG_ERR, "Error on line %d in %s", lineno,
> +                               file);
> +                       lineno++;
> +                       continue;
> +                }
> +
> +               /* Find the name-value pair */
> +               if (strcmp(nv->name, keyword) == 0)
> +               {
> +                       fclose(fp);
> +                       if (buf)

Superfluous conditional.

[..]
> +                               free (buf);
> +                       return 0;
> +               }
> +
> +               lineno++;
> +       }
> +
> +       /* If we get here, the keyword was not found in the file */
> +       audit_msg(LOG_ERR, "Keyword %s not found in %s", keyword, file);
> +       fclose(fp);
> +       if (buf)

Again.

[..]
> +               free (buf);
> +       if (nv->name)
> +               free (nv->name);
> +       if (nv->value)
> +               free (nv->value);

:)

[..]
> +       return 1;
> +}
> +
> +/*
> + *  This function parses a line looking for a keyword = value pair
> + *  and if found, returns it in the nv structure. If the function
> + *  returns success, the calling function is expected to free
> + *  nv->name and nv->value.
> + */
> +int nv_split(char *buffer, struct nv_pair *nv)
> +{
> +       /* Get the name part */
> +       char *saveptr, *ptr = NULL;
> +       char *buf = strdup(buffer);
> +
> +       /* Look for = in buf */
> +       nv->name = NULL;
> +       nv->value = NULL;
> +       ptr = strtok_r(buf, " =", &saveptr);
> +       if ((ptr == NULL) || !(strcmp(ptr,"\n"))) {

You can kill that strcmp, right?

if (!ptr || *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);
> +       if (ptr == NULL) {
> +               free (nv->name);
> +               return 1;
> +       }
> +       nv->value = strdup(ptr);
> +
> +       /* Make sure there's nothing else on the line */
> +       ptr = strtok_r(NULL, " ", &saveptr);
> +       if (ptr) {
> +               free (nv->name);
> +               free (nv->value);
> +               return 1;
> +       }
> +
> +       /* Everything is OK */
> +       return 0;
> +}
> +
> +
> +
>  int audit_set_enabled(int fd, uint32_t enabled)
>  {
>         int rc;
> diff -burN orig/libaudit.h src/libaudit.h
> --- orig/libaudit.h     2006-05-25 17:38:21.000000000 -0400
> +++ src/libaudit.h      2006-06-13 13:01:30.000000000 -0400
> @@ -248,6 +248,28 @@
>          MACH_ALPHA
>  } machine_t;
> 
> +/* These are the valid audit failure tunable enum values */
> +typedef enum {
> +       ERR=-1,
> +       IGNORE=0,
> +       LOG,
> +       TERM
> +} auditfail_t;
> +
> +/* #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;
> +};
> +

You should just write a little helper routine called something like:
	
	free_nv(struct nv_pair *nv);

It's obvious what it does and it consolidates some code and makes things
look cleaner. 

>  /*
>   * 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);
> 
>  /* AUDIT_SET */
>  typedef enum { WAIT_NO, WAIT_YES } rep_wait_t;

Re: [RFC: PATCH] Audit Failure Query Functionality

[Lisa Smith]

Tim,

>> +       /* Find the audit failure action tunable in the config file */
>> +       ret = search_audituser_conf(file, AUDIT_FAIL_KEYWORD, &nv);
>> +
>> +       if (ret == -1) {
>> +               audit_msg(LOG_WARNING, "Error in %s", file);
>>     
>
> Would you not want to use LOG_ERR here?  Or if you really did intend a
> LOG_WARNING using the word "Error" in your message might be confusing. 
>   

Good catch.  I've changed this to LOG_ERR.

>> +               return ERR;
>>     
> This is really awkward to me.  I think it makes more sense to make this
> function simply return success or failure and update a parameter passed
> to it by the user with the correct failure mode.  As it stands now, ERR
> isn't really a failure mode.  May I also suggest making your enumeration
> a little more descriptive?
>   

How about on any error IGNORE is simply returned, and the ERR mode is
eliminated?  There will still be an audit log message, so the error does
not vanish.  This means the caller will always have one of the three
failure modes to handle.

What did you have in mind to make the enumeration more descriptive?

>> +
>> +       /* Open the file for line by line reading*/
>> +       fp = fopen(file, "r");
>> +       if (fp == NULL) {
>> +               audit_msg(LOG_ERR, "Error - fdopen failed for %s (%s)",
>> +                       file, strerror(errno));
>> +               return -1;
>> +        }
>>     
>
> Why return -1, why not return errno?
>   

The caller only cares that the file could not be opened - it doesn't
matter why.  If we passes errno back, nothing would be done with that
value anyway.  The errno will be posted in the log, however, if an admin
has a need to debug a problem.

> [..]
>   
>> +
>> +       while ((bytesread = getline(&buf, &len, fp)) != -1) {
>> +
>> +               if (buf[0] == '#') {
>> +                       lineno++;
>> +                       continue;       // Ignore comments
>> +               }
>>     
>
> So will this blow up if buf[0] == ' ' ?
>   

No.  getline() will retrieve a new line from a file until there are no
more lines in the file.  After all lines have been read, getline() will
return -1 and drop out of the while loop.

> [..]
>   
>> +
>> +               /* Convert line into name-value pair */
>> +               rc = nv_split(buf, nv);
>> +               if (rc == 1) {
>> +                       audit_msg(LOG_ERR, "Error on line %d in %s", lineno,
>> +                               file);
>> +                       lineno++;
>> +                       continue;
>> +                }
>> +
>> +               /* Find the name-value pair */
>> +               if (strcmp(nv->name, keyword) == 0)
>> +               {
>> +                       fclose(fp);
>> +                       if (buf)
>>     
>
> Superfluous conditional.
>   

Fixed, thanks.

>> +       return 1;
>> +}
>> +
>> +/*
>> + *  This function parses a line looking for a keyword = value pair
>> + *  and if found, returns it in the nv structure. If the function
>> + *  returns success, the calling function is expected to free
>> + *  nv->name and nv->value.
>> + */
>> +int nv_split(char *buffer, struct nv_pair *nv)
>> +{
>> +       /* Get the name part */
>> +       char *saveptr, *ptr = NULL;
>> +       char *buf = strdup(buffer);
>> +
>> +       /* Look for = in buf */
>> +       nv->name = NULL;
>> +       nv->value = NULL;
>> +       ptr = strtok_r(buf, " =", &saveptr);
>> +       if ((ptr == NULL) || !(strcmp(ptr,"\n"))) {
>>     
>
> You can kill that strcmp, right?
>
> if (!ptr || *ptr == '\n') {
>   

Yup, corrected.

>
> You should just write a little helper routine called something like:
> 	
> 	free_nv(struct nv_pair *nv);
>
> It's obvious what it does and it consolidates some code and makes things
> look cleaner. 
>   

Great idea; will do.

Thanks for your comments.

Lisa

Re: [RFC: PATCH] Audit Failure Query Functionality

[James Antill]

[-- Attachment #1.1: Type: text/plain, Size: 7415 bytes --]

On Tue, 2006-06-13 at 14:52 -0400, Lisa Smith wrote:
> This is this initial patch for the audit failure query functionality.

[...]

>  libaudit.c |  163 +++++++++++++++++++++++++++++++++++++++++++++++++++++
>  libaudit.h |   25 ++++++++
>  2 files changed, 188 insertions(+)
> 
> diff -burN orig/libaudit.c src/libaudit.c
> --- orig/libaudit.c     2006-05-25 17:39:52.000000000 -0400
> +++ src/libaudit.c      2006-06-13 14:46:33.000000000 -0400
> @@ -68,6 +68,169 @@
>         return rc;
>  }
> 
> +/*
> + * This function will retrieve the audit failure tunable value
> + * from the filename passed in.  If no file is specified, the default
> + * /etc/libaudit.conf file will be used.
> + */
> +auditfail_t audit_failure_action(char *file)
> +{
> +       int ret;
> +       struct nv_pair nv;
> +
> +       if (file == NULL)
> +               file = AUDIT_FAIL_CONFIG;
> +
> +       /* Find the audit failure action tunable in the config file */
> +       ret = search_audituser_conf(file, AUDIT_FAIL_KEYWORD, &nv);
> +
> +       if (ret == -1) {
> +               audit_msg(LOG_WARNING, "Error in %s", file);
> +               return ERR;
> +       } else if (ret == 1) {
> +               /* Keyword not found, so do the default action */
> +               return IGNORE;
> +       }
> +
> +       /* 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".

> +               free (nv.name);
> +               free (nv.value);
> +               return IGNORE;
> +       }
[...]

> +/*
> + * This function searches for a keyword pair in the passed in filename.
> + * If the keyword pair is found, it is saved in the nv structure and zero
> + * is returned. If the file can not be opened, -1 is returned.
> + * If the keyword is not found in the file, 1 is returned.
> + *
> + * nv->name and nv->value must be freed if an error will be returned from this
> + * function after nv_split() is called. If this function returns success,
> + * the caller must free nv->name and nv->value when finished using the
> + * values.
> + */
> +int search_audituser_conf(char *file, char *keyword, struct nv_pair *nv)
> +{
> +       int rc, lineno = 1;
> +       size_t len = 0;
> +       ssize_t bytesread;
> +       FILE *fp;
> +       char *buf = NULL;
> +
> +       /* Open the file for line by line reading*/
> +       fp = fopen(file, "r");
> +       if (fp == NULL) {
> +               audit_msg(LOG_ERR, "Error - fdopen failed for %s (%s)",
> +                       file, strerror(errno));
> +               return -1;
> +        }
> +
> +       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.

> +               if (rc == 1) {
> +                       audit_msg(LOG_ERR, "Error on line %d in %s", lineno,
> +                               file);
> +                       lineno++;
> +                       continue;
> +                }
> +
> +               /* Find the name-value pair */
> +               if (strcmp(nv->name, keyword) == 0)
> +               {
> +                       fclose(fp);
> +                       if (buf)
> +                               free (buf);
> +                       return 0;
> +               }
> +
> +               lineno++;
> +       }
> +
> +       /* If we get here, the keyword was not found in the file */
> +       audit_msg(LOG_ERR, "Keyword %s not found in %s", keyword, file);
> +       fclose(fp);
> +       if (buf)
> +               free (buf);
> +       if (nv->name)
> +               free (nv->name);
> +       if (nv->value)
> +               free (nv->value);
> +       return 1;
> +}
> +
> +/*
> + *  This function parses a line looking for a keyword = value pair
> + *  and if found, returns it in the nv structure. If the function
> + *  returns success, the calling function is expected to free
> + *  nv->name and nv->value.
> + */
> +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.

> +
> +       /* 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.

> +       if (ptr == NULL) {
> +               free (nv->name);
> +               return 1;
> +       }
> +       nv->value = strdup(ptr);
> +
> +       /* Make sure there's nothing else on the line */
> +       ptr = strtok_r(NULL, " ", &saveptr);
> +       if (ptr) {
> +               free (nv->name);
> +               free (nv->value);
> +               return 1;
> +       }
> +
> +       /* Everything is OK */
> +       return 0;
> +}
> +
> +
> +
>  int audit_set_enabled(int fd, uint32_t enabled)
>  {
>         int rc;
> diff -burN orig/libaudit.h src/libaudit.h
> --- orig/libaudit.h     2006-05-25 17:38:21.000000000 -0400
> +++ src/libaudit.h      2006-06-13 13:01:30.000000000 -0400
> @@ -248,6 +248,28 @@
>          MACH_ALPHA
>  } machine_t;
> 
> +/* 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.

>  /*
>   * 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.

>  /* AUDIT_SET */
>  typedef enum { WAIT_NO, WAIT_YES } rep_wait_t;

 Could these be namespaced too?

-- 
James Antill <jantill@redhat.com>

[-- Attachment #1.2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 189 bytes --]

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

Re: [RFC: PATCH] Audit Failure Query Functionality

[Lisa Smith]

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