Re: [RFC: PATCH] Audit Failure Query Functionality

["Timothy R. Chavez" <tinytim@us.ibm.com>]

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;