* [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