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; >