From mboxrd@z Thu Jan 1 00:00:00 1970 From: "Timothy R. Chavez" Subject: Re: [RFC: PATCH] Audit Failure Query Functionality Date: Tue, 13 Jun 2006 14:57:09 -0500 Message-ID: <1150228629.26983.35.camel@localhost.localdomain> References: <448F096E.8060703@hp.com> Mime-Version: 1.0 Content-Type: text/plain Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <448F096E.8060703@hp.com> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: redhat-lspp-bounces@redhat.com Errors-To: redhat-lspp-bounces@redhat.com To: Lisa Smith Cc: LSPP , linux-audit@redhat.com List-Id: linux-audit@redhat.com On Tue, 2006-06-13 at 14:52 -0400, Lisa Smith wrote: > This is this initial patch for the audit failure query functionality. > > > 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;