From mboxrd@z Thu Jan 1 00:00:00 1970 From: Lisa Smith Subject: Re: [PATCH] Updated audit failure query functionality Date: Tue, 27 Jun 2006 09:32:09 -0400 Message-ID: <44A13359.7090405@hp.com> References: <44A0341C.2040105@hp.com> <1151362530.12102.45.camel@localhost.localdomain> Mime-Version: 1.0 Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <1151362530.12102.45.camel@localhost.localdomain> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: linux-audit-bounces@redhat.com Errors-To: linux-audit-bounces@redhat.com To: "Timothy R. Chavez" Cc: LSPP , Audit Mailing List List-Id: linux-audit@redhat.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. >> > > >> 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 */ > } > /* */ > 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; >> > > >