[redhat-lspp] [RFC: PATCH] Audit Failure Query Functionality
Timothy R. Chavez
tinytim at us.ibm.com
Tue Jun 13 19:57:09 UTC 2006
On Tue, 2006-06-13 at 14:52 -0400, Lisa Smith wrote:
> This is this initial patch for the audit failure query functionality.
>
<snip>
>
> 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;
More information about the redhat-lspp
mailing list