Re: [PATCH] Updated audit failure query functionality

[Lisa Smith <lisa.m.smith@hp.com>]

Tim,

Thanks for your comments. 

Other than the missing brackets in get_auditfail_action (good catch,
thanks), all the other functions were pulled directly from
auditd-config.c.  It was requested that the new audit failure query code
use the functions from auditd for ease of code maintainability.  As you
pointed out, the parsing only works with a very specific format, which
is why I rewrote it in the original patch.  This is the parsing
mechanism used for auditd.conf.

Steve and others, I assume you'd like to leave the functions as they
are, so they match what is in auditd-config.c.  If not, please comment
on how to handle each of the concerns Tim points out below.

Thanks,
Lisa

Timothy R. Chavez wrote:
> On Mon, 2006-06-26 at 15:23 -0400, Lisa Smith wrote:
>   
>> This is an updated patch for the audit failure query 
>> functionality.  The patch has been completely rewritten
>> to match the programming style and algorithms used in
>> auditd-config.c.
>>     
> <snip>
>   
>> Lisa
>>     
>
> Hi Lisa,
>
> I had some comments below.  Thanks!
>
> -tim
>
>   
>> -------------------
>>
>>  libaudit.c |  246 +++++++++++++++++++++++++++++++++++++++++++++++++++++
>>  libaudit.h |   11 ++
>>  2 files changed, 257 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-26 15:00:56.000000000 -0400
>> @@ -42,13 +42,56 @@
>>  #include "libaudit.h"
>>  #include "private.h"
>>
>> +/* Local prototypes */
>> +struct nv_pair
>> +{
>> +        const char *name;
>> +        const char *value;
>> +};
>> +
>> +struct kw_pair
>> +{
>> +        const char *name;
>> +        int (*parser)(const char *, int);
>> +};
>> +
>> +struct nv_list
>> +{
>> +        const char *name;
>> +        int option;
>> +};
>> +
>> +struct libaudit_conf
>> +{
>> +        auditfail_t failure_action;
>> +};
>>
>>  int audit_archadded = 0;
>>  int audit_syscalladded = 0;
>>  unsigned int audit_elf = 0U;
>> +static struct libaudit_conf config;
>> +static int load_libaudit_config();
>> +static char *get_line(FILE *f, char *buf);
>> +static int nv_split(char *buf, struct nv_pair *nv);
>> +static const struct kw_pair *kw_lookup(const char *val);
>> +static int audit_failure_parser(const char *val, int line);
>>  static int name_to_uid(const char *name, uid_t *uid);
>>  static int name_to_gid(const char *name, gid_t *gid);
>>
>> +static const struct kw_pair keywords[] =
>> +{
>> +  {"failure_action",           audit_failure_parser },
>> +  { NULL,                      NULL }
>> +};
>> +
>> +static const struct nv_list failure_actions[] =
>> +{
>> +  {"ignore",                   FA_IGNORE },
>> +  {"log",                      FA_LOG },
>> +  {"terminate",                FA_TERMINATE },
>> +  { NULL,                      0 }
>> +};
>> +
>>
>>  int audit_request_status(int fd)
>>  {
>> @@ -68,6 +111,209 @@
>>         return rc;
>>  }
>>
>> +/*
>> + * Set everything to its default value
>> +*/
>> +static void clear_config()
>> +{
>> +        config.failure_action = FA_IGNORE;
>> +}
>> +
>> +static int load_libaudit_config()
>> +{
>> +        int fd, rc, lineno = 1;
>> +        struct stat st;
>> +        FILE *f;
>> +        char buf[128];
>> +
>> +        clear_config();
>> +
>> +        /* open the file */
>> +        rc = open(CONFIG_FILE, O_NOFOLLOW|O_RDONLY);
>> +        if (rc < 0) {
>> +                if (errno != ENOENT) {
>> +                        audit_msg(LOG_ERR, "Error opening config file (%s)",
>> +                                strerror(errno));
>> +                        return 1;
>> +                }
>> +                audit_msg(LOG_WARNING,
>> +                        "Config file %s doesn't exist, skipping", CONFIG_FILE);
>> +                return 0;
>> +        }
>> +        fd = rc;
>> +
>> +        /* check the file's permissions: owned by root, not world writable,
>> +         * not symlink.
>> +         */
>> +        audit_msg(LOG_DEBUG, "Config file %s opened for parsing",
>> +                        CONFIG_FILE);
>> +        if (fstat(fd, &st) < 0) {
>> +                audit_msg(LOG_ERR, "Error fstat'ing config file (%s)",
>> +                        strerror(errno));
>> +                return 1;
>> +        }
>> +        if (st.st_uid != 0) {
>> +                audit_msg(LOG_ERR, "Error - %s isn't owned by root",
>> +                        CONFIG_FILE);
>> +                return 1;
>> +        }
>>     
>
> Hm, you really want to hard-code in a root check?  What if you're acting
> as another type of administrator with access to audit?  Done know, I
> could be out of line here.
>
> [..]
>   
>> +        if ((st.st_mode & S_IWOTH) == S_IWOTH) {
>> +                audit_msg(LOG_ERR, "Error - %s is world writable",
>> +                        CONFIG_FILE);
>> +                return 1;
>> +        }
>> +        if (!S_ISREG(st.st_mode)) {
>> +                audit_msg(LOG_ERR, "Error - %s is not a regular file",
>> +                        CONFIG_FILE);
>> +                return 1;
>> +        }
>> +
>> +        /* it's ok, read line by line */
>> +        f = fdopen(fd, "r");
>> +        if (f == NULL) {
>> +                audit_msg(LOG_ERR, "Error - fdopen failed (%s)",
>> +                        strerror(errno));
>> +                return 1;
>> +        }
>> +
>> +        while (get_line(f, buf)) {
>> +                // convert line into name-value pair
>> +                const struct kw_pair *kw;
>> +                struct nv_pair nv;
>> +                rc = nv_split(buf, &nv);
>> +                switch (rc) {
>> +                        case 0: // fine
>> +                                break;
>> +                        case 1: // not the right number of tokens.
>> +                                audit_msg(LOG_ERR,
>> +                                "Wrong number of arguments for line %d in %s",
>> +                                        lineno, CONFIG_FILE);
>> +                                break;
>> +                        case 2: // no '=' sign
>> +                                audit_msg(LOG_ERR,
>> +                                        "Missing equal sign for line %d in %s",
>> +                                        lineno, CONFIG_FILE);
>> +                                break;
>> +                        default: // something else went wrong...
>> +                                audit_msg(LOG_ERR,
>> +                                        "Unknown error for line %d in %s",
>> +                                        lineno, CONFIG_FILE);
>> +                                break;
>> +                }
>> +                if (nv.name == NULL) {
>> +                        lineno++;
>> +                        continue;
>> +                }
>> +
>> +                /* identify keyword or error */
>> +                kw = kw_lookup(nv.name);
>> +                if (kw->name == NULL) {
>> +                        audit_msg(LOG_ERR,
>> +                                "Unknown keyword \"%s\" in line %d of %s",
>> +                                nv.name, lineno, CONFIG_FILE);
>> +                        fclose(f);
>> +                        return 1;
>> +                }
>> +
>> +                /* dispatch to keyword's local parser */
>> +                rc = kw->parser(nv.value, lineno);
>> +                if (rc != 0) {
>> +                        fclose(f);
>> +                        return 1; // local parser puts message out
>> +                }
>> +
>> +                lineno++;
>> +        }
>> +
>> +        fclose(f);
>> +        return 0;
>> +}
>> +
>> +int get_auditfail_action(auditfail_t *failmode)
>> +{
>> +       if (load_libaudit_config())
>> +               *failmode = config.failure_action;
>> +               return 1;
>>     
>
> Missing brackets?
>
> [..]
>   
>> +
>> +       *failmode = config.failure_action;
>> +       return 0;
>> +}
>> +
>> +
>> +static char *get_line(FILE *f, char *buf)
>> +{
>> +        if (fgets_unlocked(buf, 128, f)) {
>> +                /* remove newline */
>> +                char *ptr = strchr(buf, 0x0a);
>> +                if (ptr)
>> +                        *ptr = 0;
>> +                return buf;
>> +        }
>> +        return NULL;
>> +}
>> +
>> +static int nv_split(char *buf, struct nv_pair *nv)
>> +{
>> +        /* Get the name part */
>> +        char *ptr;
>> +
>> +        nv->name = NULL;
>> +        nv->value = NULL;
>> +        ptr = strtok(buf, " ");
>> +        if (ptr == NULL)
>> +                return 0; /* If there's nothing, go to next line */
>> +        if (ptr[0] == '#')
>> +                return 0; /* If there's a comment, go to next line */
>>     
>
> I still think this will break if ptr[0] happens to be a space followed
> by a '#'.  I suppose it could break by design if the first character of
> the line is not '#', but I wouldn't like that much as a user myself.
>
> [..]
>   
>> +        nv->name = ptr;
>> +
>> +        /* Check for a '=' */
>> +        ptr = strtok(NULL, " ");
>> +        if (ptr == NULL)
>> +                return 1;
>> +        if (strcmp(ptr, "=") != 0)
>>     
>
> No real need to do a strcmp here.
>
> if (ptr && *ptr == '=')
>
> [..]
>   
>> +                return 2;
>> +
>> +        /* get the value */
>> +        ptr = strtok(NULL, " ");
>> +        if (ptr == NULL)
>> +                return 1;
>> +        nv->value = ptr;
>>     
>
> I'll be the first to admit that I relatively little experience with
> strtok, but is this pointer assignment even safe?  You're pointing into
> a buffer that could potentially be overwritten by another app, right?
>
> Here's my off-the-top-of-the-head, uncompiled, and untested example
> using strtok_r... 
>
> char *buf;
> char *ptr;
>
> buf = (char *)malloc(128);
> if (!buf) {
> 	/* handle memory error */
> }
> ptr = strtok_r(line, "=", buf);
> if (!ptr || *buf == '\0') {
> 	/* not "token=value", free memory */
> }
> /* <do comment check here> */
> nv->name = (char*)malloc(strlen(ptr)+1);
> if (!nv->name) {
> 	/* handle memory error */
> }
> strcpy(nv->name, ptr);
> nv->value = (char*)malloc(strlen(buf)+1);
> if (!nv->value) {
> 	/* handle memory error */
> }
> strcpy(nv->value, buf);
>
> /* clean up memory */
> return 0;
>
> You may not care about whether or not you're thread-safe or being able
> to have multiple word token values... but at least by providing your own
> buffer, you can keep track of your position in the parsed string in a
> lovely way.  
>
> [..]
>   
>> +
>> +        /* Make sure there's nothing else */
>> +        ptr = strtok(NULL, " ");
>> +        if (ptr)
>> +                return 1;
>> +
>> +        /* Everything is OK */
>> +        return 0;
>> +}
>> +
>> +static const struct kw_pair *kw_lookup(const char *val)
>> +{
>> +        int i = 0;
>> +        while (keywords[i].name != NULL) {
>> +                if (strcasecmp(keywords[i].name, val) == 0)
>> +                        break;
>> +                i++;
>> +        }
>> +        return &keywords[i];
>>     
>
> So I think this would look better a for-loop as you're in fact iterating
> anyway :) *oh my, looks down at next function*
>
> [..]
>   
>> +}
>> +
>> +static int audit_failure_parser(const char *val, int line)
>> +{
>> +        int i;
>> +
>> +        audit_msg(LOG_DEBUG, "audit_failure_parser called with: %s", val);
>> +        for (i=0; failure_actions[i].name != NULL; i++) {
>> +                if (strcasecmp(val, failure_actions[i].name) == 0) {
>> +                        config.failure_action = failure_actions[i].option;
>> +                        return 0;
>> +                }
>> +        }
>>     
>
> :)
>
> [..]
>   
>> +        audit_msg(LOG_ERR, "Option %s not found - line %d", val, line);
>> +        return 1;
>> +}
>> +
>>  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-26 14:58:53.000000000 -0400
>> @@ -248,6 +248,16 @@
>>          MACH_ALPHA
>>  } machine_t;
>>
>> +/* These are the valid audit failure tunable enum values */
>> +typedef enum {
>> +       FA_IGNORE=0,
>> +       FA_LOG,
>> +       FA_TERMINATE
>> +} auditfail_t;
>> +
>> +/* #defines for the audit failure query  */
>> +#define CONFIG_FILE "/etc/libaudit.conf"
>> +
>>  /*
>>   * audit_rule_data supports filter rules with both integer and string
>>   * fields. It corresponds with AUDIT_ADD_RULE, AUDIT_DEL_RULE and
>> @@ -362,6 +372,7 @@
>>  /* AUDIT_GET */
>>  extern int audit_request_status(int fd);
>>  extern int audit_is_enabled(int fd);
>> +extern int get_auditfail_action(auditfail_t *failmode);
>>
>>  /* AUDIT_SET */
>>  typedef enum { WAIT_NO, WAIT_YES } rep_wait_t;
>>     
>
>
>