6:08 p.m.
Hello,
Follwing is an updated patch which provides an interface for specifing
audit rules with string fields. The code related to filesystem paths
has been removed, and the interface has been generalized to support
string fields for more than one field type.
To summarize the interface, a new struct audit_rule_xprt and
corresponding netlink message types have been added. The new struct
allows userspace to supply one or more string fields packed in a
variable length buffer. The kernel expects the buffer to be neither
null-delimited nor null-terminated.
The length of string data for a given field is provided as its value
element in the array. The kernel provides the buflen element for
convenience on rule listing, to allow userspace to allocate memory for
the buffer without walking the array to tabulate lengths. Buflen is
ignored coming from userspace. Instead, the kernel ensures that the
total of the lengths specified in the value elements do not exceed the
length of the message payload minus sizeof(struct audit_rule_xprt).
Several routines have been added to auditfilter.c to translate between
the kernel's rule representation and the two userspace rule
representations.
This patch resolves the following issues present in the previous
patch:
- splits string fields interface patch from filesystem audit patch.
- struct audit_krule uses a separate field for operator bits
- struct audit_krule tracks which type of operator bits were used,
and sends rule back to userspace as it was supplied
- contents of struct audit_rule_xprt reviewed and solidified
Several of us talked on IRC and decided to split the field flags
away from the field type in the new structure. We also decided
against adding empty fields and versioning the structure.
- documentation related to modified routines has been updated
Signed-off-by: Amy Griffis <amy.griffis(a)hp.com>
diff --git a/include/linux/audit.h b/include/linux/audit.h
index 79d8271..b5da475 100644
--- a/include/linux/audit.h
+++ b/include/linux/audit.h
@@ -50,15 +50,18 @@
*/
#define AUDIT_GET 1000 /* Get status */
#define AUDIT_SET 1001 /* Set status (enable/disable/auditd) */
-#define AUDIT_LIST 1002 /* List syscall filtering rules */
-#define AUDIT_ADD 1003 /* Add syscall filtering rule */
-#define AUDIT_DEL 1004 /* Delete syscall filtering rule */
+#define AUDIT_LIST 1002 /* List syscall rules -- deprecated */
+#define AUDIT_ADD 1003 /* Add syscall rule -- deprecated */
+#define AUDIT_DEL 1004 /* Delete syscall rule -- deprecated */
#define AUDIT_USER 1005 /* Message from userspace -- deprecated */
#define AUDIT_LOGIN 1006 /* Define the login id and information */
#define AUDIT_WATCH_INS 1007 /* Insert file/dir watch entry */
#define AUDIT_WATCH_REM 1008 /* Remove file/dir watch entry */
#define AUDIT_WATCH_LIST 1009 /* List all file/dir watches */
#define AUDIT_SIGNAL_INFO 1010 /* Get info about sender of signal to auditd */
+#define AUDIT_ADD_RULE 1011 /* Add syscall filtering rule */
+#define AUDIT_DEL_RULE 1012 /* Delete syscall filtering rule */
+#define AUDIT_LIST_RULES 1013 /* List syscall filtering rules */
#define AUDIT_FIRST_USER_MSG 1100 /* Userspace messages mostly uninteresting to kernel
*/
#define AUDIT_USER_AVC 1107 /* We filter this differently */
@@ -226,6 +229,26 @@ struct audit_status {
__u32 backlog; /* messages waiting in queue */
};
+/* audit_rule_xprt supports filter rules with both integer and string
+ * fields. It corresponds with AUDIT_ADD_RULE, AUDIT_DEL_RULE and
+ * AUDIT_LIST_RULES requests.
+ */
+struct audit_rule_xprt {
+ __u32 flags; /* AUDIT_PER_{TASK,CALL}, AUDIT_PREPEND */
+ __u32 action; /* AUDIT_NEVER, AUDIT_POSSIBLE, AUDIT_ALWAYS */
+ __u32 field_count;
+ __u32 mask[AUDIT_BITMASK_SIZE];
+ __u32 fields[AUDIT_MAX_FIELDS];
+ __u32 values[AUDIT_MAX_FIELDS];
+ __u32 fieldflags[AUDIT_MAX_FIELDS];
+ __u32 buflen; /* total length of string fields */
+ char buf[0]; /* string fields buffer */
+};
+
+/* audit_rule is supported to maintain backward compatibility with
+ * userspace. It supports integer fields only and corresponds to
+ * AUDIT_ADD, AUDIT_DEL and AUDIT_LIST requests.
+ */
struct audit_rule { /* for AUDIT_LIST, AUDIT_ADD, and AUDIT_DEL */
__u32 flags; /* AUDIT_PER_{TASK,CALL}, AUDIT_PREPEND */
__u32 action; /* AUDIT_NEVER, AUDIT_POSSIBLE, AUDIT_ALWAYS */
@@ -337,7 +360,7 @@ extern void audit_log_d_path(struct
extern int audit_filter_user(struct netlink_skb_parms *cb, int type);
extern int audit_filter_type(int type);
extern int audit_receive_filter(int type, int pid, int uid, int seq,
- void *data, uid_t loginuid);
+ void *data, int dlen, uid_t loginuid);
#else
#define audit_log(c,g,t,f,...) do { ; } while (0)
#define audit_log_start(c,g,t) ({ NULL; })
diff --git a/kernel/audit.c b/kernel/audit.c
index d95efd6..d55e8c5 100644
--- a/kernel/audit.c
+++ b/kernel/audit.c
@@ -52,6 +52,7 @@
#include <linux/audit.h>
#include <net/sock.h>
+#include <net/netlink.h>
#include <linux/skbuff.h>
#include <linux/netlink.h>
@@ -361,9 +362,12 @@ static int audit_netlink_ok(kernel_cap_t
switch (msg_type) {
case AUDIT_GET:
case AUDIT_LIST:
+ case AUDIT_LIST_RULES:
case AUDIT_SET:
case AUDIT_ADD:
+ case AUDIT_ADD_RULE:
case AUDIT_DEL:
+ case AUDIT_DEL_RULE:
case AUDIT_SIGNAL_INFO:
if (!cap_raised(eff_cap, CAP_AUDIT_CONTROL))
err = -EPERM;
@@ -470,12 +474,23 @@ static int audit_receive_msg(struct sk_b
break;
case AUDIT_ADD:
case AUDIT_DEL:
- if (nlh->nlmsg_len < sizeof(struct audit_rule))
+ if (nlmsg_len(nlh) < sizeof(struct audit_rule))
return -EINVAL;
/* fallthrough */
case AUDIT_LIST:
err = audit_receive_filter(nlh->nlmsg_type, NETLINK_CB(skb).pid,
- uid, seq, data, loginuid);
+ uid, seq, data, nlmsg_len(nlh),
+ loginuid);
+ break;
+ case AUDIT_ADD_RULE:
+ case AUDIT_DEL_RULE:
+ if (nlmsg_len(nlh) < sizeof(struct audit_rule_xprt))
+ return -EINVAL;
+ /* fallthrough */
+ case AUDIT_LIST_RULES:
+ err = audit_receive_filter(nlh->nlmsg_type, NETLINK_CB(skb).pid,
+ uid, seq, data, nlmsg_len(nlh),
+ loginuid);
break;
case AUDIT_SIGNAL_INFO:
sig_data.uid = audit_sig_uid;
diff --git a/kernel/audit.h b/kernel/audit.h
index 7643e46..3e69ab3 100644
--- a/kernel/audit.h
+++ b/kernel/audit.h
@@ -52,12 +52,28 @@ enum audit_state {
};
/* Rule lists */
-struct audit_entry {
- struct list_head list;
- struct rcu_head rcu;
- struct audit_rule rule;
+struct audit_field {
+ u32 type;
+ u32 val;
+ u32 op;
+};
+
+struct audit_krule {
+ int vers_ops;
+ u32 flags;
+ u32 listnr;
+ u32 action;
+ u32 mask[AUDIT_BITMASK_SIZE];
+ u32 buflen; /* for data alloc on list rules */
+ u32 field_count;
+ struct audit_field fields[AUDIT_MAX_FIELDS];
};
+struct audit_entry {
+ struct list_head list;
+ struct rcu_head rcu;
+ struct audit_krule rule;
+};
extern int audit_pid;
extern int audit_comparator(const u32 left, const u32 op, const u32 right);
diff --git a/kernel/auditfilter.c b/kernel/auditfilter.c
index a3a3275..1fd4b2a 100644
--- a/kernel/auditfilter.c
+++ b/kernel/auditfilter.c
@@ -40,52 +40,253 @@ struct list_head audit_filter_list[AUDIT
#endif
};
-/* Copy rule from user-space to kernel-space. Called from
- * audit_add_rule during AUDIT_ADD. */
-static inline int audit_copy_rule(struct audit_rule *d, struct audit_rule *s)
+/* Unpack a filter field's string representation from user-space
+ * buffer. */
+static inline char *audit_unpack_string(void **bufp, int *remaining, int len)
{
+ char *str;
+
+ if (!*bufp || (len > *remaining))
+ return ERR_PTR(-EINVAL);
+
+ str = kmalloc(len + 1, GFP_KERNEL);
+ if (!str)
+ return ERR_PTR(-ENOMEM);
+
+ memcpy(str, *bufp, len);
+ str[len] = 0;
+ *bufp += len;
+ *remaining -= len;
+
+ return str;
+}
+
+/* Common user-space to kernel rule translation. */
+static struct audit_entry *audit_to_entry_common(struct audit_rule *rule)
+{
+ unsigned listnr;
+ struct audit_entry *entry;
+ int i, err;
+
+ err = -EINVAL;
+ listnr = rule->flags & ~AUDIT_FILTER_PREPEND;
+ switch(listnr) {
+ default:
+ goto exit_err;
+ case AUDIT_FILTER_USER:
+ case AUDIT_FILTER_TYPE:
+#ifdef CONFIG_AUDITSYSCALL
+ case AUDIT_FILTER_ENTRY:
+ case AUDIT_FILTER_EXIT:
+ case AUDIT_FILTER_TASK:
+#endif
+ ;
+ }
+ if (rule->action != AUDIT_NEVER && rule->action != AUDIT_POSSIBLE
&&
+ rule->action != AUDIT_ALWAYS)
+ goto exit_err;
+ if (rule->field_count < 0 || rule->field_count > AUDIT_MAX_FIELDS)
+ goto exit_err;
+
+ err = -ENOMEM;
+ entry = kmalloc(sizeof(*entry), GFP_KERNEL);
+ if (!entry)
+ goto exit_err;
+ memset(&entry->rule, 0, sizeof(struct audit_krule));
+
+ entry->rule.flags = rule->flags & AUDIT_FILTER_PREPEND;
+ entry->rule.listnr = listnr;
+ entry->rule.action = rule->action;
+ entry->rule.field_count = rule->field_count;
+
+ for (i = 0; i < AUDIT_BITMASK_SIZE; i++)
+ entry->rule.mask[i] = rule->mask[i];
+
+ return entry;
+
+exit_err:
+ return ERR_PTR(err);
+}
+
+/* Translate struct audit_rule to kernel's rule respresentation.
+ * Exists for backward compatibility with userspace. */
+static struct audit_entry *audit_rule_to_entry(struct audit_rule *rule)
+{
+ struct audit_entry *entry;
+ int err = 0;
int i;
- if (s->action != AUDIT_NEVER
- && s->action != AUDIT_POSSIBLE
- && s->action != AUDIT_ALWAYS)
- return -1;
- if (s->field_count < 0 || s->field_count > AUDIT_MAX_FIELDS)
- return -1;
- if ((s->flags & ~AUDIT_FILTER_PREPEND) >= AUDIT_NR_FILTERS)
- return -1;
-
- d->flags = s->flags;
- d->action = s->action;
- d->field_count = s->field_count;
- for (i = 0; i < d->field_count; i++) {
- d->fields[i] = s->fields[i];
- d->values[i] = s->values[i];
+ entry = audit_to_entry_common(rule);
+ if (IS_ERR(entry))
+ goto exit_nofree;
+
+ for (i = 0; i < rule->field_count; i++) {
+ struct audit_field *f = &entry->rule.fields[i];
+
+ if (rule->fields[i] & AUDIT_UNUSED_BITS) {
+ err = -EINVAL;
+ goto exit_free;
+ }
+
+ f->op = rule->fields[i] & (AUDIT_NEGATE|AUDIT_OPERATORS);
+ f->type = rule->fields[i] & (~AUDIT_NEGATE|AUDIT_OPERATORS);
+ f->val = rule->values[i];
+
+ entry->rule.vers_ops = (f->op & AUDIT_OPERATORS) ? 2 : 1;
+ if (f->op & AUDIT_NEGATE)
+ f->op |= AUDIT_NOT_EQUAL;
+ else if (!(f->op & AUDIT_OPERATORS))
+ f->op |= AUDIT_EQUAL;
+ f->op &= ~AUDIT_NEGATE;
}
- for (i = 0; i < AUDIT_BITMASK_SIZE; i++) d->mask[i] = s->mask[i];
- return 0;
+
+exit_nofree:
+ return entry;
+
+exit_free:
+ kfree(entry);
+ return ERR_PTR(err);
}
-/* Check to see if two rules are identical. It is called from
- * audit_add_rule during AUDIT_ADD and
- * audit_del_rule during AUDIT_DEL. */
-static int audit_compare_rule(struct audit_rule *a, struct audit_rule *b)
+/* Translate struct audit_rule_xprt to kernel's rule respresentation. */
+static struct audit_entry *audit_xprt_to_entry(struct audit_rule_xprt *xprt,
+ int dlen)
{
+ int err = 0;
+ struct audit_entry *entry;
+ void *bufp;
+ /* int remaining = dlen - sizeof(struct audit_rule_xprt); */
int i;
- if (a->flags != b->flags)
- return 1;
+ entry = audit_to_entry_common((struct audit_rule *)xprt);
+ if (IS_ERR(entry))
+ goto exit_nofree;
+
+ bufp = xprt->buf;
+ entry->rule.vers_ops = 2;
+ for (i = 0; i < xprt->field_count; i++) {
+ struct audit_field *f = &entry->rule.fields[i];
+
+ if (!(xprt->fieldflags[i] & AUDIT_OPERATORS) ||
+ xprt->fieldflags[i] & ~AUDIT_OPERATORS) {
+ err = -EINVAL;
+ goto exit_free;
+ }
- if (a->action != b->action)
- return 1;
+ f->op = xprt->fieldflags[i] & AUDIT_OPERATORS;
+ f->type = xprt->fields[i];
+ switch(f->type) {
+ /* call type-specific conversion routines here */
+ default:
+ f->val = xprt->values[i];
+ }
+ }
+
+exit_nofree:
+ return entry;
+
+exit_free:
+ kfree(entry);
+ return ERR_PTR(err);
+}
+
+/* Pack a filter field's string representation into data block. */
+static inline int audit_pack_string(void **bufp, char *str)
+{
+ int len = strlen(str);
+
+ memcpy(*bufp, str, len);
+ *bufp += len;
+
+ return len;
+}
+
+/* Translate kernel rule respresentation to struct audit_rule.
+ * Exists for backward compatibility with userspace. */
+static struct audit_rule *audit_krule_to_rule(struct audit_krule *krule)
+{
+ struct audit_rule *rule;
+ int i;
+
+ rule = kmalloc(sizeof(*rule), GFP_KERNEL);
+ if (!rule)
+ return ERR_PTR(-ENOMEM);
+ memset(rule, 0, sizeof(*rule));
+
+ rule->flags = krule->flags | krule->listnr;
+ rule->action = krule->action;
+ rule->field_count = krule->field_count;
+ for (i = 0; i < rule->field_count; i++) {
+ rule->values[i] = krule->fields[i].val;
+ rule->fields[i] = krule->fields[i].type;
+
+ if (krule->vers_ops == 1) {
+ if (krule->fields[i].op & AUDIT_NOT_EQUAL)
+ rule->fields[i] |= AUDIT_NEGATE;
+ } else {
+ rule->fields[i] |= krule->fields[i].op;
+ }
+ }
+ for (i = 0; i < AUDIT_BITMASK_SIZE; i++) rule->mask[i] = krule->mask[i];
+
+ return rule;
+}
+
+/* Translate kernel rule respresentation to struct audit_rule_xprt. */
+static struct audit_rule_xprt *audit_krule_to_xprt(struct audit_krule *krule)
+{
+ struct audit_rule_xprt *xprt;
+ void *bufp;
+ int i;
+
+ xprt = kmalloc(sizeof(*xprt) + krule->buflen, GFP_KERNEL);
+ if (!xprt)
+ return ERR_PTR(-ENOMEM);
+ memset(xprt, 0, sizeof(*xprt));
+
+ xprt->flags = krule->flags | krule->listnr;
+ xprt->action = krule->action;
+ xprt->field_count = krule->field_count;
+ bufp = xprt->buf;
+ for (i = 0; i < xprt->field_count; i++) {
+ struct audit_field *f = &krule->fields[i];
+
+ xprt->fields[i] = f->type;
+ xprt->fieldflags[i] = f->op;
+ switch(f->type) {
+ /* call type-specific conversion routines here */
+ default:
+ xprt->values[i] = f->val;
+ }
+ }
+ for (i = 0; i < AUDIT_BITMASK_SIZE; i++) xprt->mask[i] = krule->mask[i];
+
+ return xprt;
+}
- if (a->field_count != b->field_count)
+/* Compare two rules in kernel format. Considered success if rules
+ * don't match. */
+static int audit_compare_rule(struct audit_krule *a, struct audit_krule *b)
+{
+ int i;
+
+ if (a->flags != b->flags ||
+ a->listnr != b->listnr ||
+ a->action != b->action ||
+ a->field_count != b->field_count)
return 1;
for (i = 0; i < a->field_count; i++) {
- if (a->fields[i] != b->fields[i]
- || a->values[i] != b->values[i])
+ if (a->fields[i].type != b->fields[i].type ||
+ a->fields[i].op != b->fields[i].op)
return 1;
+
+ switch(a->fields[i].type) {
+ /* call type-specific comparison routines here */
+ default:
+ if (a->fields[i].val != b->fields[i].val)
+ return 1;
+ }
}
for (i = 0; i < AUDIT_BITMASK_SIZE; i++)
@@ -95,41 +296,21 @@ static int audit_compare_rule(struct aud
return 0;
}
-/* Note that audit_add_rule and audit_del_rule are called via
- * audit_receive() in audit.c, and are protected by
+/* Add rule to given filterlist if not a duplicate. Protected by
* audit_netlink_sem. */
-static inline int audit_add_rule(struct audit_rule *rule,
- struct list_head *list)
+static inline int audit_add_rule(struct audit_entry *entry,
+ struct list_head *list)
{
- struct audit_entry *entry;
- int i;
+ struct audit_entry *e;
/* Do not use the _rcu iterator here, since this is the only
* addition routine. */
- list_for_each_entry(entry, list, list) {
- if (!audit_compare_rule(rule, &entry->rule))
+ list_for_each_entry(e, list, list) {
+ if (!audit_compare_rule(&entry->rule, &e->rule))
return -EEXIST;
}
- for (i = 0; i < rule->field_count; i++) {
- if (rule->fields[i] & AUDIT_UNUSED_BITS)
- return -EINVAL;
- if ( rule->fields[i] & AUDIT_NEGATE)
- rule->fields[i] |= AUDIT_NOT_EQUAL;
- else if ( (rule->fields[i] & AUDIT_OPERATORS) == 0 )
- rule->fields[i] |= AUDIT_EQUAL;
- rule->fields[i] &= ~AUDIT_NEGATE;
- }
-
- if (!(entry = kmalloc(sizeof(*entry), GFP_KERNEL)))
- return -ENOMEM;
- if (audit_copy_rule(&entry->rule, rule)) {
- kfree(entry);
- return -EINVAL;
- }
-
if (entry->rule.flags & AUDIT_FILTER_PREPEND) {
- entry->rule.flags &= ~AUDIT_FILTER_PREPEND;
list_add_rcu(&entry->list, list);
} else {
list_add_tail_rcu(&entry->list, list);
@@ -144,10 +325,9 @@ static inline void audit_free_rule(struc
kfree(e);
}
-/* Note that audit_add_rule and audit_del_rule are called via
- * audit_receive() in audit.c, and are protected by
+/* Remove an existing rule from filterlist. Protected by
* audit_netlink_sem. */
-static inline int audit_del_rule(struct audit_rule *rule,
+static inline int audit_del_rule(struct audit_entry *entry,
struct list_head *list)
{
struct audit_entry *e;
@@ -155,7 +335,7 @@ static inline int audit_del_rule(struct
/* Do not use the _rcu iterator here, since this is the only
* deletion routine. */
list_for_each_entry(e, list, list) {
- if (!audit_compare_rule(rule, &e->rule)) {
+ if (!audit_compare_rule(&entry->rule, &e->rule)) {
list_del_rcu(&e->list);
call_rcu(&e->rcu, audit_free_rule);
return 0;
@@ -164,7 +344,9 @@ static inline int audit_del_rule(struct
return -ENOENT; /* No matching rule */
}
-static int audit_list_rules(void *_dest)
+/* List rules using struct audit_rule. Exists for backward
+ * compatibility with userspace. */
+static int audit_list(void *_dest)
{
int pid, seq;
int *dest = _dest;
@@ -180,9 +362,16 @@ static int audit_list_rules(void *_dest)
/* The *_rcu iterators not needed here because we are
always called with audit_netlink_sem held. */
for (i=0; i<AUDIT_NR_FILTERS; i++) {
- list_for_each_entry(entry, &audit_filter_list[i], list)
+ list_for_each_entry(entry, &audit_filter_list[i], list) {
+ struct audit_rule *rule;
+
+ rule = audit_krule_to_rule(&entry->rule);
+ if (!rule)
+ break;
audit_send_reply(pid, seq, AUDIT_LIST, 0, 1,
- &entry->rule, sizeof(entry->rule));
+ rule, sizeof(*rule));
+ kfree(rule);
+ }
}
audit_send_reply(pid, seq, AUDIT_LIST, 1, 1, NULL, 0);
@@ -190,6 +379,40 @@ static int audit_list_rules(void *_dest)
return 0;
}
+/* List rules using struct audit_rule_xprt. */
+static int audit_list_rules(void *_dest)
+{
+ int pid, seq;
+ int *dest = _dest;
+ struct audit_entry *e;
+ int i;
+
+ pid = dest[0];
+ seq = dest[1];
+ kfree(dest);
+
+ down(&audit_netlink_sem);
+
+ /* The *_rcu iterators not needed here because we are
+ always called with audit_netlink_sem held. */
+ for (i=0; i<AUDIT_NR_FILTERS; i++) {
+ list_for_each_entry(e, &audit_filter_list[i], list) {
+ struct audit_rule_xprt *xprt;
+
+ xprt = audit_krule_to_xprt(&e->rule);
+ if (!xprt)
+ break;
+ audit_send_reply(pid, seq, AUDIT_LIST_RULES, 0, 1,
+ xprt, sizeof(*xprt));
+ kfree(xprt);
+ }
+ }
+ audit_send_reply(pid, seq, AUDIT_LIST_RULES, 1, 1, NULL, 0);
+
+ up(&audit_netlink_sem);
+ return 0;
+}
+
/**
* audit_receive_filter - apply all rules to the specified message type
* @type: audit message type
@@ -197,18 +420,20 @@ static int audit_list_rules(void *_dest)
* @uid: target uid for netlink audit messages
* @seq: netlink audit message sequence (serial) number
* @data: payload data
+ * @dlen: length of payload data
* @loginuid: loginuid of sender
*/
int audit_receive_filter(int type, int pid, int uid, int seq, void *data,
- uid_t loginuid)
+ int dlen, uid_t loginuid)
{
struct task_struct *tsk;
int *dest;
- int err = 0;
- unsigned listnr;
+ int err = 0;
+ struct audit_entry *entry;
switch (type) {
case AUDIT_LIST:
+ case AUDIT_LIST_RULES:
/* We can't just spew out the rules here because we might fill
* the available socket buffer space and deadlock waiting for
* auditctl to read from it... which isn't ever going to
@@ -221,41 +446,48 @@ int audit_receive_filter(int type, int p
dest[0] = pid;
dest[1] = seq;
- tsk = kthread_run(audit_list_rules, dest, "audit_list_rules");
+ if (type == AUDIT_LIST)
+ tsk = kthread_run(audit_list, dest, "audit_list");
+ else
+ tsk = kthread_run(audit_list_rules, dest,
+ "audit_list_rules");
if (IS_ERR(tsk)) {
kfree(dest);
err = PTR_ERR(tsk);
}
break;
case AUDIT_ADD:
- listnr = ((struct audit_rule *)data)->flags & ~AUDIT_FILTER_PREPEND;
- switch(listnr) {
- default:
- return -EINVAL;
+ case AUDIT_ADD_RULE:
+ if (type == AUDIT_ADD)
+ entry = audit_rule_to_entry(data);
+ else
+ entry = audit_xprt_to_entry(data, dlen);
+ if (IS_ERR(entry))
+ return PTR_ERR(entry);
- case AUDIT_FILTER_USER:
- case AUDIT_FILTER_TYPE:
-#ifdef CONFIG_AUDITSYSCALL
- case AUDIT_FILTER_ENTRY:
- case AUDIT_FILTER_EXIT:
- case AUDIT_FILTER_TASK:
-#endif
- ;
- }
- err = audit_add_rule(data, &audit_filter_list[listnr]);
+ err = audit_add_rule(entry,
+ &audit_filter_list[entry->rule.listnr]);
if (!err)
audit_log(NULL, GFP_KERNEL, AUDIT_CONFIG_CHANGE,
"auid=%u added an audit rule\n", loginuid);
+ else
+ kfree(entry);
break;
case AUDIT_DEL:
- listnr =((struct audit_rule *)data)->flags & ~AUDIT_FILTER_PREPEND;
- if (listnr >= AUDIT_NR_FILTERS)
- return -EINVAL;
+ case AUDIT_DEL_RULE:
+ if (type == AUDIT_DEL)
+ entry = audit_rule_to_entry(data);
+ else
+ entry = audit_xprt_to_entry(data, dlen);
+ if (IS_ERR(entry))
+ return PTR_ERR(entry);
- err = audit_del_rule(data, &audit_filter_list[listnr]);
+ err = audit_del_rule(entry,
+ &audit_filter_list[entry->rule.listnr]);
if (!err)
audit_log(NULL, GFP_KERNEL, AUDIT_CONFIG_CHANGE,
"auid=%u removed an audit rule\n", loginuid);
+ kfree(entry);
break;
default:
return -EINVAL;
@@ -287,29 +519,27 @@ int audit_comparator(const u32 left, con
static int audit_filter_user_rules(struct netlink_skb_parms *cb,
- struct audit_rule *rule,
+ struct audit_krule *rule,
enum audit_state *state)
{
int i;
for (i = 0; i < rule->field_count; i++) {
- u32 field = rule->fields[i] & ~AUDIT_OPERATORS;
- u32 op = rule->fields[i] & AUDIT_OPERATORS;
- u32 value = rule->values[i];
+ struct audit_field *f = &rule->fields[i];
int result = 0;
- switch (field) {
+ switch (f->type) {
case AUDIT_PID:
- result = audit_comparator(cb->creds.pid, op, value);
+ result = audit_comparator(cb->creds.pid, f->op, f->val);
break;
case AUDIT_UID:
- result = audit_comparator(cb->creds.uid, op, value);
+ result = audit_comparator(cb->creds.uid, f->op, f->val);
break;
case AUDIT_GID:
- result = audit_comparator(cb->creds.gid, op, value);
+ result = audit_comparator(cb->creds.gid, f->op, f->val);
break;
case AUDIT_LOGINUID:
- result = audit_comparator(cb->loginuid, op, value);
+ result = audit_comparator(cb->loginuid, f->op, f->val);
break;
}
@@ -354,14 +584,11 @@ int audit_filter_type(int type)
list_for_each_entry_rcu(e, &audit_filter_list[AUDIT_FILTER_TYPE],
list) {
- struct audit_rule *rule = &e->rule;
int i;
- for (i = 0; i < rule->field_count; i++) {
- u32 field = rule->fields[i] & ~AUDIT_OPERATORS;
- u32 op = rule->fields[i] & AUDIT_OPERATORS;
- u32 value = rule->values[i];
- if ( field == AUDIT_MSGTYPE ) {
- result = audit_comparator(type, op, value);
+ for (i = 0; i < e->rule.field_count; i++) {
+ struct audit_field *f = &e->rule.fields[i];
+ if (f->type == AUDIT_MSGTYPE) {
+ result = audit_comparator(type, f->op, f->val);
if (!result)
break;
}
diff --git a/kernel/auditsc.c b/kernel/auditsc.c
index 8f0a61c..e4f7096 100644
--- a/kernel/auditsc.c
+++ b/kernel/auditsc.c
@@ -161,70 +161,68 @@ struct audit_context {
/* Compare a task_struct with an audit_rule. Return 1 on match, 0
* otherwise. */
static int audit_filter_rules(struct task_struct *tsk,
- struct audit_rule *rule,
+ struct audit_krule *rule,
struct audit_context *ctx,
enum audit_state *state)
{
int i, j;
for (i = 0; i < rule->field_count; i++) {
- u32 field = rule->fields[i] & ~AUDIT_OPERATORS;
- u32 op = rule->fields[i] & AUDIT_OPERATORS;
- u32 value = rule->values[i];
+ struct audit_field *f = &rule->fields[i];
int result = 0;
- switch (field) {
+ switch (f->type) {
case AUDIT_PID:
- result = audit_comparator(tsk->pid, op, value);
+ result = audit_comparator(tsk->pid, f->op, f->val);
break;
case AUDIT_UID:
- result = audit_comparator(tsk->uid, op, value);
+ result = audit_comparator(tsk->uid, f->op, f->val);
break;
case AUDIT_EUID:
- result = audit_comparator(tsk->euid, op, value);
+ result = audit_comparator(tsk->euid, f->op, f->val);
break;
case AUDIT_SUID:
- result = audit_comparator(tsk->suid, op, value);
+ result = audit_comparator(tsk->suid, f->op, f->val);
break;
case AUDIT_FSUID:
- result = audit_comparator(tsk->fsuid, op, value);
+ result = audit_comparator(tsk->fsuid, f->op, f->val);
break;
case AUDIT_GID:
- result = audit_comparator(tsk->gid, op, value);
+ result = audit_comparator(tsk->gid, f->op, f->val);
break;
case AUDIT_EGID:
- result = audit_comparator(tsk->egid, op, value);
+ result = audit_comparator(tsk->egid, f->op, f->val);
break;
case AUDIT_SGID:
- result = audit_comparator(tsk->sgid, op, value);
+ result = audit_comparator(tsk->sgid, f->op, f->val);
break;
case AUDIT_FSGID:
- result = audit_comparator(tsk->fsgid, op, value);
+ result = audit_comparator(tsk->fsgid, f->op, f->val);
break;
case AUDIT_PERS:
- result = audit_comparator(tsk->personality, op, value);
+ result = audit_comparator(tsk->personality, f->op, f->val);
break;
case AUDIT_ARCH:
if (ctx)
- result = audit_comparator(ctx->arch, op, value);
+ result = audit_comparator(ctx->arch, f->op, f->val);
break;
case AUDIT_EXIT:
if (ctx && ctx->return_valid)
- result = audit_comparator(ctx->return_code, op, value);
+ result = audit_comparator(ctx->return_code, f->op, f->val);
break;
case AUDIT_SUCCESS:
if (ctx && ctx->return_valid) {
- if (value)
- result = audit_comparator(ctx->return_valid, op, AUDITSC_SUCCESS);
+ if (f->val)
+ result = audit_comparator(ctx->return_valid, f->op, AUDITSC_SUCCESS);
else
- result = audit_comparator(ctx->return_valid, op, AUDITSC_FAILURE);
+ result = audit_comparator(ctx->return_valid, f->op, AUDITSC_FAILURE);
}
break;
case AUDIT_DEVMAJOR:
if (ctx) {
for (j = 0; j < ctx->name_count; j++) {
- if (audit_comparator(MAJOR(ctx->names[j].dev), op, value)) {
+ if (audit_comparator(MAJOR(ctx->names[j].dev), f->op, f->val)) {
++result;
break;
}
@@ -234,7 +232,7 @@ static int audit_filter_rules(struct tas
case AUDIT_DEVMINOR:
if (ctx) {
for (j = 0; j < ctx->name_count; j++) {
- if (audit_comparator(MINOR(ctx->names[j].dev), op, value)) {
+ if (audit_comparator(MINOR(ctx->names[j].dev), f->op, f->val)) {
++result;
break;
}
@@ -244,8 +242,8 @@ static int audit_filter_rules(struct tas
case AUDIT_INODE:
if (ctx) {
for (j = 0; j < ctx->name_count; j++) {
- if (audit_comparator(ctx->names[j].ino, op, value) ||
- audit_comparator(ctx->names[j].pino, op, value)) {
+ if (audit_comparator(ctx->names[j].ino, f->op, f->val) ||
+ audit_comparator(ctx->names[j].pino, f->op, f->val)) {
++result;
break;
}
@@ -255,14 +253,14 @@ static int audit_filter_rules(struct tas
case AUDIT_LOGINUID:
result = 0;
if (ctx)
- result = audit_comparator(ctx->loginuid, op, value);
+ result = audit_comparator(ctx->loginuid, f->op, f->val);
break;
case AUDIT_ARG0:
case AUDIT_ARG1:
case AUDIT_ARG2:
case AUDIT_ARG3:
if (ctx)
- result = audit_comparator(ctx->argv[field-AUDIT_ARG0], op, value);
+ result = audit_comparator(ctx->argv[f->type-AUDIT_ARG0], f->op, f->val);
break;
}
2:46 p.m.
On Wednesday 21 December 2005 19:08, Amy Griffis wrote:
Thanks Amy for sending this. We at least have the beginnings of the new file
system audit patch to chew on. :) I only have a couple issues listed below.
They basically revolve around length checking of the netlink packet.
case AUDIT_ADD:
case AUDIT_DEL:
- if (nlh->nlmsg_len < sizeof(struct audit_rule))
+ if (nlmsg_len(nlh) < sizeof(struct audit_rule))
This checks the lower size bound, is there a check for the upper bound? Or
that the strings after buf[0] are in fact correctly sized? Does all the
lengths + struct add up to be equal to the payload length?
return -EINVAL;
/* fallthrough */
case AUDIT_LIST:
err = audit_receive_filter(nlh->nlmsg_type,
NETLINK_CB(skb).pid, - uid, seq,
data, loginuid); + uid, seq, data,
nlmsg_len(nlh), + loginuid);
+ break;
+ case AUDIT_ADD_RULE:
+ case AUDIT_DEL_RULE:
+ if (nlmsg_len(nlh) < sizeof(struct audit_rule_xprt))
Same.
+ return -EINVAL;
+ /* fallthrough */
+ case AUDIT_LIST_RULES:
+ err = audit_receive_filter(nlh->nlmsg_type,
NETLINK_CB(skb).pid, + uid, seq,
data, nlmsg_len(nlh), + loginuid);
break;
case AUDIT_SIGNAL_INFO:
sig_data.uid = audit_sig_uid;
right); diff --git a/kernel/auditfilter.c b/kernel/auditfilter.c
index a3a3275..1fd4b2a 100644
--- a/kernel/auditfilter.c
+++ b/kernel/auditfilter.c
@@ -40,52 +40,253 @@ struct list_head audit_filter_list[AUDIT
#endif
};
-/* Copy rule from user-space to kernel-space. Called from
- * audit_add_rule during AUDIT_ADD. */
-static inline int audit_copy_rule(struct audit_rule *d, struct audit_rule
*s) +/* Unpack a filter field's string representation from user-space
+ * buffer. */
+static inline char *audit_unpack_string(void **bufp, int *remaining, int
len) {
I couldn't find callers for this. Is there any chance that len or remaining
could be negative? If not, lets make them unsigned.
+ char *str;
+
+ if (!*bufp || (len > *remaining))
+ return ERR_PTR(-EINVAL);
len should always be <= PATH_MAX*2. That must be checked for as well or there
could be wrapping.
+/* Common user-space to kernel rule translation. */
+static struct audit_entry *audit_to_entry_common(struct audit_rule *rule)
+{
+ unsigned listnr;
+ struct audit_entry *entry;
+ int i, err;
+
+ err = -EINVAL;
+ listnr = rule->flags & ~AUDIT_FILTER_PREPEND;
+ switch(listnr) {
+ default:
+ goto exit_err;
+ case AUDIT_FILTER_USER:
+ case AUDIT_FILTER_TYPE:
+#ifdef CONFIG_AUDITSYSCALL
+ case AUDIT_FILTER_ENTRY:
+ case AUDIT_FILTER_EXIT:
+ case AUDIT_FILTER_TASK:
+#endif
+ ;
+ }
+ if (rule->action != AUDIT_NEVER && rule->action != AUDIT_POSSIBLE
&& + rule->action != AUDIT_ALWAYS)
+ goto exit_err;
+ if (rule->field_count < 0 || rule->field_count > AUDIT_MAX_FIELDS)
+ goto exit_err;
field_count is u32 & cannot be less than 0. It should be checked to make sure
its not 0, though.
+
+ err = -ENOMEM;
+ entry = kmalloc(sizeof(*entry), GFP_KERNEL);
+ if (!entry)
+ goto exit_err;
+ memset(&entry->rule, 0, sizeof(struct audit_krule));
+
+ entry->rule.flags = rule->flags & AUDIT_FILTER_PREPEND;
+ entry->rule.listnr = listnr;
+ entry->rule.action = rule->action;
+ entry->rule.field_count = rule->field_count;
+
+ for (i = 0; i < AUDIT_BITMASK_SIZE; i++)
+ entry->rule.mask[i] = rule->mask[i];
+
+ return entry;
+
+exit_err:
+ return ERR_PTR(err);
+}
+
+/* Translate struct audit_rule to kernel's rule respresentation.
+ * Exists for backward compatibility with userspace. */
+static struct audit_entry *audit_rule_to_entry(struct audit_rule *rule)
+{
+ struct audit_entry *entry;
+ int err = 0;
int i;
- if (s->action != AUDIT_NEVER
- && s->action != AUDIT_POSSIBLE
- && s->action != AUDIT_ALWAYS)
- return -1;
- if (s->field_count < 0 || s->field_count > AUDIT_MAX_FIELDS)
- return -1;
Same as above. sb == 0 rather than < 0.
- if ((s->flags & ~AUDIT_FILTER_PREPEND) >=
AUDIT_NR_FILTERS)
- return -1;
-
- d->flags = s->flags;
- d->action = s->action;
- d->field_count = s->field_count;
- for (i = 0; i < d->field_count; i++) {
- d->fields[i] = s->fields[i];
- d->values[i] = s->values[i];
+ entry = audit_to_entry_common(rule);
+ if (IS_ERR(entry))
+ goto exit_nofree;
+
+ for (i = 0; i < rule->field_count; i++) {
+ struct audit_field *f = &entry->rule.fields[i];
+
+ if (rule->fields[i] & AUDIT_UNUSED_BITS) {
+ err = -EINVAL;
+ goto exit_free;
+ }
+
+ f->op = rule->fields[i] & (AUDIT_NEGATE|AUDIT_OPERATORS);
+ f->type = rule->fields[i] &
(~AUDIT_NEGATE|AUDIT_OPERATORS); + f->val = rule->values[i];
+
+ entry->rule.vers_ops = (f->op & AUDIT_OPERATORS) ? 2 : 1;
+ if (f->op & AUDIT_NEGATE)
+ f->op |= AUDIT_NOT_EQUAL;
+ else if (!(f->op & AUDIT_OPERATORS))
+ f->op |= AUDIT_EQUAL;
+ f->op &= ~AUDIT_NEGATE;
}
- for (i = 0; i < AUDIT_BITMASK_SIZE; i++) d->mask[i] = s->mask[i];
- return 0;
+
+exit_nofree:
+ return entry;
+
+exit_free:
+ kfree(entry);
+ return ERR_PTR(err);
}
-/* Check to see if two rules are identical. It is called from
- * audit_add_rule during AUDIT_ADD and
- * audit_del_rule during AUDIT_DEL. */
-static int audit_compare_rule(struct audit_rule *a, struct audit_rule *b)
+/* Translate struct audit_rule_xprt to kernel's rule respresentation. */
+static struct audit_entry *audit_xprt_to_entry(struct audit_rule_xprt
*xprt, + int dlen)
{
+ int err = 0;
+ struct audit_entry *entry;
+ void *bufp;
+ /* int remaining = dlen - sizeof(struct audit_rule_xprt); */
int i;
- if (a->flags != b->flags)
- return 1;
+ entry = audit_to_entry_common((struct audit_rule *)xprt);
+ if (IS_ERR(entry))
+ goto exit_nofree;
+
+ bufp = xprt->buf;
What uses bufp?
+/* Pack a filter field's string representation into data block.
*/
+static inline int audit_pack_string(void **bufp, char *str)
What calls this?
+{
+ int len = strlen(str);
Why use int? size_t is natural size.
+ memcpy(*bufp, str, len);
+ *bufp += len;
+
+ return len;
+}
+
-Steve
5:05 p.m.
Hi Steve,
Thanks for reviewing.
On Tue, Jan 03, 2006 at 03:46:42PM -0500, Steve Grubb wrote:
On Wednesday 21 December 2005 19:08, Amy Griffis wrote:
Thanks Amy for sending this. We at least have the beginnings of the new file
system audit patch to chew on. :) I only have a couple issues listed below.
They basically revolve around length checking of the netlink packet.
> ????????case AUDIT_ADD:
> ????????case AUDIT_DEL:
> -???????????????if (nlh->nlmsg_len < sizeof(struct audit_rule))
> +???????????????if (nlmsg_len(nlh) < sizeof(struct audit_rule))
This checks the lower size bound, is there a check for the upper bound? Or
that the strings after buf[0] are in fact correctly sized? Does all the
lengths + struct add up to be equal to the payload length?
These message types use struct audit_rule, which doesn't support an
additional buffer.
> ????????????????????????return -EINVAL;
> ????????????????/* fallthrough */
> ????????case AUDIT_LIST:
> ????????????????err = audit_receive_filter(nlh->nlmsg_type,
> NETLINK_CB(skb).pid, -??????????????????????????????????????? ? uid, seq,
> data, loginuid); +??????????????????????????????????????? ? uid, seq, data,
> nlmsg_len(nlh), +??????????????????????????????????????? ? loginuid);
> +???????????????break;
> +???????case AUDIT_ADD_RULE:
> +???????case AUDIT_DEL_RULE:
> +???????????????if (nlmsg_len(nlh) < sizeof(struct audit_rule_xprt))
Same.
The netlink packet length is passed along through
audit_receive_filter() to audit_xprt_to_entry(), where it can be
referenced while processing the buffer. audit_xprt_to_entry() keeps
track of the bytes in the buffer yet to be consumed and provides this
value to audit_unpack_string(), which both checks that sufficient
space remains, and updates the 'remaining' value accordingly. I see
audit_unpack_string() as being called by this patch's consumers in the
per-field-type switch statement in audit_xprt_to_entry().
> +???????????????????????return -EINVAL;
> +???????????????/* fallthrough */
> +???????case AUDIT_LIST_RULES:
> +???????????????err = audit_receive_filter(nlh->nlmsg_type,
> NETLINK_CB(skb).pid, +??????????????????????????????????????? ? uid, seq,
> data, nlmsg_len(nlh), +??????????????????????????????????????? ? loginuid);
> ????????????????break;
> ????????case AUDIT_SIGNAL_INFO:
> ????????????????sig_data.uid = audit_sig_uid;
> right); diff --git a/kernel/auditfilter.c b/kernel/auditfilter.c
> index a3a3275..1fd4b2a 100644
> --- a/kernel/auditfilter.c
> +++ b/kernel/auditfilter.c
> @@ -40,52 +40,253 @@ struct list_head audit_filter_list[AUDIT
> ?#endif
> ?};
> ?
> -/* Copy rule from user-space to kernel-space. ?Called from
> - * audit_add_rule during AUDIT_ADD. */
> -static inline int audit_copy_rule(struct audit_rule *d, struct audit_rule
> *s) +/* Unpack a filter field's string representation from user-space
> + * buffer. */
> +static inline char *audit_unpack_string(void **bufp, int *remaining, int
> len) {
I couldn't find callers for this.
Correct. I intended for this to be called by consumers from the
switch in audit_xprt_to_entry(). I clearly didn't provide enough
comments. :-/
Is there any chance that len or remaining could be negative? If not,
lets make them unsigned.
I'll do that, thanks.
> +???????char *str;
> +
> +???????if (!*bufp || (len > *remaining))
> +???????????????return ERR_PTR(-EINVAL);
len should always be <= PATH_MAX*2. That must be checked for as well or there
could be wrapping.
I'm confused about the significance of the value PATH_MAX*2 and how
that relates here. Could you clarify?
> +/* Common user-space to kernel rule translation. */
> +static struct audit_entry *audit_to_entry_common(struct audit_rule *rule)
> +{
> +???????unsigned listnr;
> +???????struct audit_entry *entry;
> +???????int i, err;
> +
> +???????err = -EINVAL;
> +???????listnr = rule->flags & ~AUDIT_FILTER_PREPEND;
> +???????switch(listnr) {
> +???????default:
> +???????????????goto exit_err;
> +???????case AUDIT_FILTER_USER:
> +???????case AUDIT_FILTER_TYPE:
> +#ifdef CONFIG_AUDITSYSCALL
> +???????case AUDIT_FILTER_ENTRY:
> +???????case AUDIT_FILTER_EXIT:
> +???????case AUDIT_FILTER_TASK:
> +#endif
> +???????????????;
> +???????}
> +???????if (rule->action != AUDIT_NEVER && rule->action !=
AUDIT_POSSIBLE
> && +??????? ? ?rule->action != AUDIT_ALWAYS)
> +???????????????goto exit_err;
> +???????if (rule->field_count < 0 || rule->field_count >
AUDIT_MAX_FIELDS)
> +???????????????goto exit_err;
field_count is u32 & cannot be less than 0. It should be checked to make sure
its not 0, though.
I thought it was okay to specify a rule with no fields, i.e.
auditctl -a entry,always -S swapon
> +
> +???????err = -ENOMEM;
> +???????entry = kmalloc(sizeof(*entry), GFP_KERNEL);
> +???????if (!entry)
> +???????????????goto exit_err;
> +???????memset(&entry->rule, 0, sizeof(struct audit_krule));
> +
> +???????entry->rule.flags = rule->flags & AUDIT_FILTER_PREPEND;
> +???????entry->rule.listnr = listnr;
> +???????entry->rule.action = rule->action;
> +???????entry->rule.field_count = rule->field_count;
> +
> +???????for (i = 0; i < AUDIT_BITMASK_SIZE; i++)
> +???????????????entry->rule.mask[i] = rule->mask[i];
> +
> +???????return entry;
> +
> +exit_err:
> +???????return ERR_PTR(err);
> +}
> +
> +/* Translate struct audit_rule to kernel's rule respresentation.
> + * Exists for backward compatibility with userspace. */
> +static struct audit_entry *audit_rule_to_entry(struct audit_rule *rule)
> +{
> +???????struct audit_entry *entry;
> +???????int err = 0;
> ????????int i;
> ?
> -???????if (s->action != AUDIT_NEVER
> -??????? ? ?&& s->action != AUDIT_POSSIBLE
> -??????? ? ?&& s->action != AUDIT_ALWAYS)
> -???????????????return -1;
> -???????if (s->field_count < 0 || s->field_count > AUDIT_MAX_FIELDS)
> -???????????????return -1;
Same as above. sb == 0 rather than < 0.
I'm not sure what you mean. I don't see a variable 'sb' and this is
code that was removed.
> -???????if ((s->flags & ~AUDIT_FILTER_PREPEND) >= AUDIT_NR_FILTERS)
> -???????????????return -1;
> -
> -???????d->flags????????= s->flags;
> -???????d->action???????= s->action;
> -???????d->field_count??= s->field_count;
> -???????for (i = 0; i < d->field_count; i++) {
> -???????????????d->fields[i] = s->fields[i];
> -???????????????d->values[i] = s->values[i];
> +???????entry = audit_to_entry_common(rule);
> +???????if (IS_ERR(entry))
> +???????????????goto exit_nofree;
> +
> +???????for (i = 0; i < rule->field_count; i++) {
> +???????????????struct audit_field *f = &entry->rule.fields[i];
> +
> +???????????????if (rule->fields[i] & AUDIT_UNUSED_BITS) {
> +???????????????????????err = -EINVAL;
> +???????????????????????goto exit_free;
> +???????????????}
> +
> +???????????????f->op = rule->fields[i] & (AUDIT_NEGATE|AUDIT_OPERATORS);
> +???????????????f->type = rule->fields[i] &
> (~AUDIT_NEGATE|AUDIT_OPERATORS); +???????????????f->val = rule->values[i];
> +
> +???????????????entry->rule.vers_ops = (f->op & AUDIT_OPERATORS) ? 2 : 1;
> +???????????????if (f->op & AUDIT_NEGATE)
> +???????????????????????f->op |= AUDIT_NOT_EQUAL;
> +???????????????else if (!(f->op & AUDIT_OPERATORS))
> +???????????????????????f->op |= AUDIT_EQUAL;
> +???????????????f->op &= ~AUDIT_NEGATE;
> ????????}
> -???????for (i = 0; i < AUDIT_BITMASK_SIZE; i++) d->mask[i] = s->mask[i];
> -???????return 0;
> +
> +exit_nofree:
> +???????return entry;
> +
> +exit_free:
> +???????kfree(entry);
> +???????return ERR_PTR(err);
> ?}
> ?
> -/* Check to see if two rules are identical. ?It is called from
> - * audit_add_rule during AUDIT_ADD and
> - * audit_del_rule during AUDIT_DEL. */
> -static int audit_compare_rule(struct audit_rule *a, struct audit_rule *b)
> +/* Translate struct audit_rule_xprt to kernel's rule respresentation. */
> +static struct audit_entry *audit_xprt_to_entry(struct audit_rule_xprt
> *xprt, +??????????????????????????????????????? ? ? ? int dlen)
> ?{
> +???????int err = 0;
> +???????struct audit_entry *entry;
> +???????void *bufp;
> +???????/* int remaining = dlen - sizeof(struct audit_rule_xprt); */
> ????????int i;
> ?
> -???????if (a->flags != b->flags)
> -???????????????return 1;
> +???????entry = audit_to_entry_common((struct audit_rule *)xprt);
> +???????if (IS_ERR(entry))
> +???????????????goto exit_nofree;
> +
> +???????bufp = xprt->buf;
What uses bufp?
Nothing right now, but it should be used by a consumer adding code to
the switch statement.
> +/* Pack a filter field's string representation into data block. */
> +static inline int audit_pack_string(void **bufp, char *str)
What calls this?
This should be called by a consumer from the switch in
audit_krule_to_xprt().
> +{
> +???????int len = strlen(str);
Why use int? size_t is natural size.
Sure, will fix.
> +???????memcpy(*bufp, str, len);
> +???????*bufp += len;
> +
> +???????return len;
> +}
> +
-Steve
5:23 p.m.
On Tuesday 03 January 2006 18:05, Amy Griffis wrote:
These message types use struct audit_rule, which doesn't support
an
additional buffer.
Ah, Ok.
> > ????????????????????????return -EINVAL;
> > ????????????????/* fallthrough */
> > ????????case AUDIT_LIST:
> > ????????????????err = audit_receive_filter(nlh->nlmsg_type,
> > NETLINK_CB(skb).pid, -??????????????????????????????????????? ? uid,
> > seq, data, loginuid); +??????????????????????????????????????? ? uid,
> > seq, data, nlmsg_len(nlh), +??????????????????????????????????????? ?
> > loginuid); +???????????????break;
> > +???????case AUDIT_ADD_RULE:
> > +???????case AUDIT_DEL_RULE:
> > +???????????????if (nlmsg_len(nlh) < sizeof(struct audit_rule_xprt))
>
> Same.
The netlink packet length is passed along through
audit_receive_filter() to audit_xprt_to_entry(), where it can be
referenced while processing the buffer.
In the last incarnation of file system audit code, we tried hard to validate
the length before ever allocating any kernel memory of any kind. I was trying
to trace through to see where the length is really checked to ensure the
payload is legit.
> > +???????char *str;
> > +
> > +???????if (!*bufp || (len > *remaining))
> > +???????????????return ERR_PTR(-EINVAL);
>
> len should always be <= PATH_MAX*2. That must be checked for as well or
> there could be wrapping.
I'm confused about the significance of the value PATH_MAX*2 and how
that relates here. Could you clarify?
In order to make sure people aren't playing games by abusing the audit system,
I was thinking that you could do a check that the length isn't too big also.
If you have a u32 for len, and someone passes -1, then you have a large
positive number. To detect these problems, you have to check for a legal
upper bounds. In this case, the longest string should be PATH_MAX. I guess
the "*2" only applies to actual audit records rather than the file path for
rules.
> > +/* Common user-space to kernel rule translation. */
> > +static struct audit_entry *audit_to_entry_common(struct audit_rule
> > *rule) +{
> > +???????unsigned listnr;
> > +???????struct audit_entry *entry;
> > +???????int i, err;
> > +
> > +???????if (rule->field_count < 0 || rule->field_count >
> > AUDIT_MAX_FIELDS) +???????????????goto exit_err;
>
> field_count is u32 & cannot be less than 0. It should be checked to make
> sure its not 0, though.
I thought it was okay to specify a rule with no fields, i.e.
auditctl -a entry,always -S swapon
OK...right. So the < 0 part can be deleted.
> > -/* Check to see if two rules are identical. ?It is called
from
> > - * audit_add_rule during AUDIT_ADD and
> > - * audit_del_rule during AUDIT_DEL. */
> > -static int audit_compare_rule(struct audit_rule *a, struct audit_rule
> > *b) +/* Translate struct audit_rule_xprt to kernel's rule
> > respresentation. */ +static struct audit_entry
> > *audit_xprt_to_entry(struct audit_rule_xprt *xprt,
> > +??????????????????????????????????????? ? ? ? int dlen) ?{
> > +???????int err = 0;
> > +???????struct audit_entry *entry;
> > +???????void *bufp;
> > +???????/* int remaining = dlen - sizeof(struct audit_rule_xprt); */
> > ????????int i;
> > ?
> > -???????if (a->flags != b->flags)
> > -???????????????return 1;
> > +???????entry = audit_to_entry_common((struct audit_rule *)xprt);
> > +???????if (IS_ERR(entry))
> > +???????????????goto exit_nofree;
> > +
> > +???????bufp = xprt->buf;
>
> What uses bufp?
Nothing right now, but it should be used by a consumer adding code to
the switch statement.
OK, I thought the patch was complete. Nevermind.
> > +/* Pack a filter field's string representation into data block. */
> > +static inline int audit_pack_string(void **bufp, char *str)
>
> What calls this?
This should be called by a consumer from the switch in
audit_krule_to_xprt().
I really need to see the consumer to finish evaluating the use of the
interface. I just want to make sure there's no vulnerabilities that will give
hackers a chance.
-Steve
9:36 p.m.
On Tue, Jan 03, 2006 at 06:23:48PM -0500, Steve Grubb wrote:
> > > +???????char *str;
> > > +
> > > +???????if (!*bufp || (len > *remaining))
> > > +???????????????return ERR_PTR(-EINVAL);
> >
> > len should always be <= PATH_MAX*2. That must be checked for as well or
> > there could be wrapping.
>
> I'm confused about the significance of the value PATH_MAX*2 and how
> that relates here. Could you clarify?
In order to make sure people aren't playing games by abusing the audit system,
I was thinking that you could do a check that the length isn't too big also.
If you have a u32 for len, and someone passes -1, then you have a large
positive number. To detect these problems, you have to check for a legal
upper bounds. In this case, the longest string should be PATH_MAX. I guess
the "*2" only applies to actual audit records rather than the file path for
rules.
I understand the situation you're trying to address, but PATH_MAX may
not make sense as a bound for other string fields. Wouldn't checking
the specified string field length against the actual size of the
provided buffer suffice?
> > > -/* Check to see if two rules are identical. ?It is
called from
> > > - * audit_add_rule during AUDIT_ADD and
> > > - * audit_del_rule during AUDIT_DEL. */
> > > -static int audit_compare_rule(struct audit_rule *a, struct audit_rule
> > > *b) +/* Translate struct audit_rule_xprt to kernel's rule
> > > respresentation. */ +static struct audit_entry
> > > *audit_xprt_to_entry(struct audit_rule_xprt *xprt,
> > > +??????????????????????????????????????? ? ? ? int dlen) ?{
> > > +???????int err = 0;
> > > +???????struct audit_entry *entry;
> > > +???????void *bufp;
> > > +???????/* int remaining = dlen - sizeof(struct audit_rule_xprt); */
> > > ????????int i;
> > > ?
> > > -???????if (a->flags != b->flags)
> > > -???????????????return 1;
> > > +???????entry = audit_to_entry_common((struct audit_rule *)xprt);
> > > +???????if (IS_ERR(entry))
> > > +???????????????goto exit_nofree;
> > > +
> > > +???????bufp = xprt->buf;
> >
> > What uses bufp?
>
> Nothing right now, but it should be used by a consumer adding code to
> the switch statement.
OK, I thought the patch was complete. Nevermind.
Well, it is intended to be complete as an interface.
>
> > > +/* Pack a filter field's string representation into data block. */
> > > +static inline int audit_pack_string(void **bufp, char *str)
> >
> > What calls this?
>
> This should be called by a consumer from the switch in
> audit_krule_to_xprt().
I really need to see the consumer to finish evaluating the use of the
interface.
Makes sense. I'll post a consumer patch with the next iteration.
5:27 a.m.
On Wednesday 04 January 2006 22:36, Amy Griffis wrote:
I understand the situation you're trying to address, but PATH_MAX
may
not make sense as a bound for other string fields.
Do you know of any that will be bigger? I can't think of any. The size can be
adjusted up if we ever need to.
Wouldn't checking the specified string field length against the
actual size
of the provided buffer suffice?
No. We could fall victim to some attack that overflows the variable and
appears to be correct.
> > > > +/* Pack a filter field's string
representation into data block. */
> > > > +static inline int audit_pack_string(void **bufp, char *str)
> > >
> > > What calls this?
> >
> > This should be called by a consumer from the switch in
> > audit_krule_to_xprt().
>
> I really need to see the consumer to finish evaluating the use of the
> interface.
Makes sense. I'll post a consumer patch with the next iteration.
I just need to see a little more. The unpacking I think is all I need to see
to make sure this interface can't be abused.
-Steve
10:43 a.m.
On 12/21/05, Amy Griffis <amy.griffis(a)hp.com> wrote:
diff --git a/include/linux/audit.h b/include/linux/audit.h
index 79d8271..b5da475 100644
--- a/include/linux/audit.h
+++ b/include/linux/audit.h
@@ -226,6 +229,26 @@ struct audit_status {
__u32 backlog; /* messages waiting in queue
*/
};
+/* audit_rule_xprt supports filter rules with both integer and string
+ * fields. It corresponds with AUDIT_ADD_RULE, AUDIT_DEL_RULE and
+ * AUDIT_LIST_RULES requests.
+ */
+struct audit_rule_xprt {
This is really just a nit, but I think we might be more clear with the
name of the structure. I thought 'xprt' was "expert" or
"export" until
Amy explained to me on IRC that it actually stands for "transport". Not
a big deal, but it might make it more readable to call it
audit_rule_transport or audit_rule_wbuf or audit_rule_varlen, or
something else.
:-Dustin
12:05 p.m.
On Thursday 05 January 2006 10:43, Dustin Kirkland wrote:
On 12/21/05, Amy Griffis <amy.griffis(a)hp.com> wrote:
> diff --git a/include/linux/audit.h b/include/linux/audit.h
> index 79d8271..b5da475 100644
> --- a/include/linux/audit.h
> +++ b/include/linux/audit.h
> @@ -226,6 +229,26 @@ struct audit_status {
> __u32 backlog; /* messages waiting in queue
*/
> };
>
> +/* audit_rule_xprt supports filter rules with both integer and string
> + * fields. It corresponds with AUDIT_ADD_RULE, AUDIT_DEL_RULE and
> + * AUDIT_LIST_RULES requests.
> + */
> +struct audit_rule_xprt {
This is really just a nit, but I think we might be more clear with the
name of the structure. I thought 'xprt' was "expert" or
"export" until
Amy explained to me on IRC that it actually stands for "transport". Not
a big deal, but it might make it more readable to call it
audit_rule_transport or audit_rule_wbuf or audit_rule_varlen, or
something else.
:-Dustin
I like audit_rule_transport, personally. It's descriptive and not too
verbose.
-tim
4:19 p.m.
On Thu, Jan 05, 2006 at 10:43:26AM -0600, Dustin Kirkland wrote:
On 12/21/05, Amy Griffis <amy.griffis(a)hp.com> wrote:
> diff --git a/include/linux/audit.h b/include/linux/audit.h
> index 79d8271..b5da475 100644
> --- a/include/linux/audit.h
> +++ b/include/linux/audit.h
> @@ -226,6 +229,26 @@ struct audit_status {
> __u32 backlog; /* messages waiting in queue
*/
> };
>
> +/* audit_rule_xprt supports filter rules with both integer and string
> + * fields. It corresponds with AUDIT_ADD_RULE, AUDIT_DEL_RULE and
> + * AUDIT_LIST_RULES requests.
> + */
> +struct audit_rule_xprt {
This is really just a nit, but I think we might be more clear with the
name of the structure. I thought 'xprt' was "expert" or
"export" until
Amy explained to me on IRC that it actually stands for "transport".
Not a big deal, but it might make it more readable to call it
audit_rule_transport or audit_rule_wbuf or audit_rule_varlen, or
something else.
Of the three, I prefer audit_rule_transport, but it seems
unnecessarily long to me. I suppose we could shorten it to
audit_rule_trans, but I'm not sure that's any more readable than
'xprt'.
What do you think about changing the comment to make it more
descriptive? i.e.,
/* audit_rule_xprt is used to transport audit filter rule content
* between kernel and userspace. It supports filter rules with both
* integer and string fields and corresponds with AUDIT_ADD_RULE,
* AUDIT_DEL_RULE and AUDIT_LIST_RULES requests.
*/
Amy
10:30 a.m.
Of the three, I prefer audit_rule_transport, but it seems
unnecessarily long to me. I suppose we could shorten it to
audit_rule_trans, but I'm not sure that's any more readable than
'xprt'.
What do you think about changing the comment to make it more
descriptive? i.e.,
/* audit_rule_xprt is used to transport audit filter rule content
* between kernel and userspace. It supports filter rules with both
* integer and string fields and corresponds with AUDIT_ADD_RULE,
* AUDIT_DEL_RULE and AUDIT_LIST_RULES requests.
*/
Amy
IMHO changing audit_rule_xprt to audit_rule_transport does make it more
readable.
But then does that also mean changing all occurrences of 'xprt' to
'transport' for consistency?
i.e.
xprt->buf becomes transport->buf
audit_krule_to_xprt() becomes audit_krule_to_transport()
Not sure that changing one and leaving the others 'xprt' really helps the
overall readability.
11:10 a.m.
On Friday 06 January 2006 10:30, Debora Velarde wrote:
>
> Of the three, I prefer audit_rule_transport, but it seems
> unnecessarily long to me. I suppose we could shorten it to
> audit_rule_trans, but I'm not sure that's any more readable than
> 'xprt'.
>
> What do you think about changing the comment to make it more
> descriptive? i.e.,
>
> /* audit_rule_xprt is used to transport audit filter rule content
> * between kernel and userspace. It supports filter rules with both
> * integer and string fields and corresponds with AUDIT_ADD_RULE,
> * AUDIT_DEL_RULE and AUDIT_LIST_RULES requests.
> */
>
> Amy
IMHO changing audit_rule_xprt to audit_rule_transport does make it more
readable.
But then does that also mean changing all occurrences of 'xprt' to
'transport' for consistency?
i.e.
xprt->buf becomes transport->buf
audit_krule_to_xprt() becomes audit_krule_to_transport()
Not sure that changing one and leaving the others 'xprt' really helps the
overall readability.
Good points Debbie. And just to add fuel to the fire, if we're concerned about
verbosity and readability, why not just:
audit_transport
-tim
1:58 p.m.
On Fri, 2006-01-06 at 11:10 -0600, Timothy R. Chavez wrote:
Good points Debbie. And just to add fuel to the fire, if we're
concerned about
verbosity and readability, why not just:
audit_transport
I think that unfortunately looses the fact that this is a data structure
representing an "audit rule".
Honestly, if everyone knows and understands that xprt=transport, then
this will work ok. There are certainly more offensive data types and
variable names else in the kernel.
:-Dustin
2:14 p.m.
On Fri, Jan 06, 2006 at 01:58:29PM -0600, Dustin Kirkland wrote:
On Fri, 2006-01-06 at 11:10 -0600, Timothy R. Chavez wrote:
> Good points Debbie. And just to add fuel to the fire, if we're
> concerned about verbosity and readability, why not just:
>
> audit_transport
I think that unfortunately looses the fact that this is a data structure
representing an "audit rule".
That's true.
The more I think about it, I don't really like xprt/transport.
Looking through the kernel sources, the only code that uses xprt
meaning transport is RPC code. I'd prefer to avoid any unwarranted
associations with RPC. I also don't think transport accurately
describes what's happening here.
How about audit_rule_data? I would personally prefer that to
audit_rule_xprt.
Amy
2:51 p.m.
On Fri, 2006-01-06 at 15:14 -0500, Amy Griffis wrote:
The more I think about it, I don't really like xprt/transport.
Looking through the kernel sources, the only code that uses xprt
meaning transport is RPC code. I'd prefer to avoid any unwarranted
associations with RPC. I also don't think transport accurately
describes what's happening here.
How about audit_rule_data? I would personally prefer that to
audit_rule_xprt.
I like audit_rule_data better than *_xprt.
Or even audit_rule_ext (for "extended").
:-Dustin
4:20 p.m.
On Wednesday 21 December 2005 19:08, Amy Griffis wrote:
+#define AUDIT_ADD_RULE 1011 /* Add syscall filtering rule
*/
+#define AUDIT_DEL_RULE 1012 /* Delete syscall filtering rule */
+#define AUDIT_LIST_RULES 1013 /* List syscall filtering rules */
You'll also need to patch security/selinux/nlmsg_tab.c sb
{ AUDIT_ADD_RULE, NETLINK_AUDIT_SOCKET__NLMSG_WRITE },
{ AUDIT_DEL_RULE, NETLINK_AUDIT_SOCKET__NLMSG_WRITE },
{ AUDIT_LIST_RULES, NETLINK_AUDIT_SOCKET__NLMSG_READPRIV },
Thanks,
-Steve
6924
days inactive
6939
days old
linux-audit@lists.linux-audit.osci.io
14 comments
5 participants
participants (5)
-
Amy Griffis -
Debora Velarde -
Dustin Kirkland -
Steve Grubb -
Timothy R. Chavez