* [PATCH] Updated audit failure query functionality
@ 2006-06-26 19:23 Lisa Smith
2006-06-26 22:55 ` Timothy R. Chavez
0 siblings, 1 reply; 4+ messages in thread
From: Lisa Smith @ 2006-06-26 19:23 UTC (permalink / raw)
To: LSPP, Audit Mailing List
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.
This patch introduces a new query function,
get_auditfail_action(enum auditfail_t *failmode), which
can be called to determine what action should be taken by
a service after audit_open() fails. The function will
read the tunable "failure_action" from the new
/etc/libaudit.conf file to determine what action should be
taken.
This patch provides easy expansion of the /etc/libaudit.conf
file to introduce new libaudit tunables if the need arises
in the future.
The audit_open() man page will be updated to reference this
query function, and a new man page will also be created for
get_auditfail_action().
Lisa
-------------------
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;
+ }
+ 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;
+
+ *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 */
+ nv->name = ptr;
+
+ /* Check for a '=' */
+ ptr = strtok(NULL, " ");
+ if (ptr == NULL)
+ return 1;
+ if (strcmp(ptr, "=") != 0)
+ return 2;
+
+ /* get the value */
+ ptr = strtok(NULL, " ");
+ if (ptr == NULL)
+ return 1;
+ nv->value = ptr;
+
+ /* 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];
+}
+
+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;
^ permalink raw reply [flat|nested] 4+ messages in thread* Re: [PATCH] Updated audit failure query functionality 2006-06-26 19:23 [PATCH] Updated audit failure query functionality Lisa Smith @ 2006-06-26 22:55 ` Timothy R. Chavez 2006-06-27 13:32 ` Lisa Smith 0 siblings, 1 reply; 4+ messages in thread From: Timothy R. Chavez @ 2006-06-26 22:55 UTC (permalink / raw) To: Lisa Smith; +Cc: LSPP, Audit Mailing List 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; ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] Updated audit failure query functionality 2006-06-26 22:55 ` Timothy R. Chavez @ 2006-06-27 13:32 ` Lisa Smith 2006-06-27 13:55 ` Steve Grubb 0 siblings, 1 reply; 4+ messages in thread From: Lisa Smith @ 2006-06-27 13:32 UTC (permalink / raw) To: Timothy R. Chavez; +Cc: LSPP, Audit Mailing List 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; >> > > > ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] Updated audit failure query functionality 2006-06-27 13:32 ` Lisa Smith @ 2006-06-27 13:55 ` Steve Grubb 0 siblings, 0 replies; 4+ messages in thread From: Steve Grubb @ 2006-06-27 13:55 UTC (permalink / raw) To: linux-audit; +Cc: LSPP On Tuesday 27 June 2006 09:32, Lisa Smith wrote: > 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. Lisa, Thanks for working on this. I will integrate it in the next release and take over maintenance of it. Thanks again... -Steve ^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2006-06-27 13:55 UTC | newest] Thread overview: 4+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2006-06-26 19:23 [PATCH] Updated audit failure query functionality Lisa Smith 2006-06-26 22:55 ` Timothy R. Chavez 2006-06-27 13:32 ` Lisa Smith 2006-06-27 13:55 ` Steve Grubb
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox