[PATCH] Audit filter rule operators (2/2)
Dustin Kirkland
dustin.kirkland at us.ibm.com
Wed Oct 26 22:09:46 UTC 2005
On 10/24/05, Steve Grubb <sgrubb at redhat.com> wrote:
> We need to go ahead and take the next 2 upper bits in the same patch and save
> those for future use. For now, if those bits are set, the kernel should
> reject the rule. To support this, we also need some code added to
> audit_add_rule to check that the operators is something the kernel
> understands.
On 10/24/05, Steve Grubb <sgrubb at redhat.com> wrote:
> So that future kernels can detect that the rule is trying to use an operator
> that it doesn't understand and send an error back to the user. For example,
> someone my be using kernel 2.6.25 with audit 3.0.
>
> I really don't care what they are used for, but we need ways for older kernels
> to reject commands from newer user space tools.
>
> I really think that we should be looking at all the undefined bits and
> checking that they are 0.
On 10/25/05, Amy Griffis <amy.griffis at hp.com> wrote:
> I think this may have been alluded to in another email, but we need to
> do comaptibility checking on rule insert, not during rule evaluation.
> So if AUDIT_NEGATE is set, clear it and set AUDIT_NOT_EQUAL. If no
> bits are set, set AUDIT_EQUAL. Doing so eliminates these two branches
> in fast-path code.
Ok, as per our subsequent discussions via email and IRC, I've actually
created a bitmask called AUDIT_UNUSED_BITS which is currently in sync
with the constants we now have defined. If we start #defining audit
message types > 2048, or start extending the operator bits further
right, this simply needs to be updated.
Then in the audit_add_rule() function, there's a call that checks to
make sure that each field value in the new rule is not using unsupported
bits. I think this should give the future compatibility check that
Steve has been asking for. Which means that the default: return
-EINVAL; in the audit_comparator() switch should be dead code, assuming
that AUDIT_UNUSED_BITS is in good working order.
Also near this rule insertion code, there's a new check for the
AUDIT_NEGATE bit being set. If it is, I unset it and make the operator
AUDIT_NOT_EQUAL. And if all of the the operator bits are 0's, then we
assume that it's an AUDIT_EQUAL operation. These five lines will be
needed for legacy support of older audit userspace that utilized
AUDIT_NEGATE and flagged AUDIT_EQUAL in such a manner.
On 10/24/05, Steve Grubb <sgrubb at redhat.com> wrote:
> Does this make sense? What does !< mean? I think AUDIT_NEGATE only makes sense
> in relation to AUDIT_EQUAL. It should be moved to that case if not eliminated
> outright.
You're absolutely right, Steve.
On 10/25/05, Amy Griffis <amy.griffis at hp.com> wrote:
> Here's another way to specify this that may be more readable and
> common in the kernel:
>
> #define AUDIT_LESS_THAN_OR_EQUAL AUDIT_EQUAL|AUDIT_LESS_THAN
Phenomenal suggestion, Amy. I've reworked them to look much nicer, I
think you'll see.
On 10/25/05, Amy Griffis <amy.griffis at hp.com> wrote:
> AUDIT_RANGE isn't needed at all, as you already have an implementation
> that can express that.
This is totally true, as well. I've been testing with some rather
complicated sets of rules that have in fact proven to operate as
expected in range fashion. I meant to remove this part of the patch but
I forgot as I was cleansing it before submission.
On 10/25/05, Amy Griffis <amy.griffis at hp.com> wrote:
> Many lines in this function are way over 80 chars -- see
> Documentation/CodingStyle Chapter 2.
I fixed all of these. Please check to make sure I've indented them in
an acceptable manner.
On 10/25/05, Amy Griffis <amy.griffis at hp.com> wrote:
> Or how about this?
>
> int rc = 0;
>
> if ((operator & AUDIT_EQUAL) && (left == right))
> rc = 1;
> else if ((operator & AUDIT_LESS_THAN) && (left < right))
> rc = 1;
> else if ((operator & AUDIT_GREATER_THAN) && (left > right))
> rc = 1;
Ok, so there's been a side thread of discussion among myself, Amy, and
Steve where we've created a test program that tests both algorithms and
we're seeing almost precisely identical performance of both algorithms
across several architectures. It seems that there's a very slight
advantage of my switch-based approach on ix86, x86_64, and ppc, though
the different is possibly attributable to other performance noise.
Amy's if-based algorithm seems to have an advantage on ia64 though we
don't yet have an explanation as to why, though her last email seemed to
indicated that with compiler optimization at -O2, the results were again
nearly identical. Thus, I think that the switch-based approach is
easier on the eyes and I'm putting it forward again as my suggestion.
Feel free to add your two cents here, and perhaps we should publish the
tests as well...
--
A couple of other changes...
- I've incorporated the use of audit_comparator() in
audit_filter_user_rules() as well.
- I've also left a bit of explanation in the comment just above the
defines of the AUDIT_* stuff--let me know if this is ridiculously
elementary for the audience, as I can gladly remove it.
Patch follows.
:-Dustin
diff -urpNbB linux-2.6.14-rc4/include/linux/audit.h
linux-2.6.14-rc4-audit_ops/include/linux/audit.h
--- linux-2.6.14-rc4/include/linux/audit.h 2005-10-19 09:40:27.000000000 -0500
+++ linux-2.6.14-rc4-audit_ops/include/linux/audit.h 2005-10-26 16:12:42.000000000 -0500
@@ -98,6 +98,13 @@
#define AUDIT_WORD(nr) ((__u32)((nr)/32))
#define AUDIT_BIT(nr) (1 << ((nr) - AUDIT_WORD(nr)*32))
+/* This bitmask is used to validate user input. It represents all bits that
+ * are currently used in an audit field constant understood by the kernel.
+ * If you are adding a new #define AUDIT_<whatever>, please ensure that
+ * AUDIT_UNUSED_BITS is updated if need be. */
+#define AUDIT_UNUSED_BITS 0x0FFFFC00
+
+
/* Rule fields */
/* These are useful when checking the
* task structure at task creation time
@@ -130,6 +137,26 @@
#define AUDIT_NEGATE 0x80000000
+/* These are the supported operators.
+ * 4 2 1
+ * = > <
+ * -------
+ * 0 0 0 0 nonsense
+ * 0 0 1 1 <
+ * 0 1 0 2 >
+ * 0 1 1 3 !=
+ * 1 0 0 4 =
+ * 1 0 1 5 <=
+ * 1 1 0 6 >=
+ * 1 1 1 7 all operators
+ */
+#define AUDIT_LESS_THAN 0x10000000
+#define AUDIT_GREATER_THAN 0x20000000
+#define AUDIT_NOT_EQUAL 0x30000000
+#define AUDIT_EQUAL 0x40000000
+#define AUDIT_LESS_THAN_OR_EQUAL (AUDIT_LESS_THAN|AUDIT_EQUAL)
+#define AUDIT_GREATER_THAN_OR_EQUAL (AUDIT_GREATER_THAN|AUDIT_EQUAL)
+#define AUDIT_OPERATORS (AUDIT_EQUAL|AUDIT_NOT_EQUAL)
/* Status symbols */
/* Mask values */
diff -urpNbB linux-2.6.14-rc4/kernel/auditsc.c linux-2.6.14-rc4-audit_ops/kernel/auditsc.c
--- linux-2.6.14-rc4/kernel/auditsc.c 2005-10-19 09:40:29.000000000 -0500
+++ linux-2.6.14-rc4-audit_ops/kernel/auditsc.c 2005-10-26 16:11:10.000000000 -0500
@@ -2,6 +2,7 @@
* Handles all system-call specific auditing features.
*
* Copyright 2003-2004 Red Hat Inc., Durham, North Carolina.
+ * Copyright (C) 2005 IBM Corporation
* All Rights Reserved.
*
* This program is free software; you can redistribute it and/or modify
@@ -27,6 +28,9 @@
* this file -- see entry.S) is based on a GPL'd patch written by
* okir at suse.de and Copyright 2003 SuSE Linux AG.
*
+ * The support of additional filter rules compares (>, <, >=, <=) was
+ * added by Dustin Kirkland <dustin.kirkland at us.ibm.com>, 2005.
+ *
*/
#include <linux/init.h>
@@ -252,6 +256,7 @@ static inline int audit_add_rule(struct
struct list_head *list)
{
struct audit_entry *entry;
+ int i;
/* Do not use the _rcu iterator here, since this is the only
* addition routine. */
@@ -261,6 +266,16 @@ static inline int audit_add_rule(struct
}
}
+ 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)) {
@@ -385,6 +400,27 @@ int audit_receive_filter(int type, int p
return err;
}
+static int audit_comparator(const u32 left, const u32 op, const u32 right)
+{
+ printk(KERN_ERR "DUSTIN: peforming (%d %x %d)\n", left, op, right);
+ switch (op) {
+ case AUDIT_EQUAL:
+ return (left == right);
+ case AUDIT_NOT_EQUAL:
+ return (left != right);
+ case AUDIT_LESS_THAN:
+ return (left < right);
+ case AUDIT_LESS_THAN_OR_EQUAL:
+ return (left <= right);
+ case AUDIT_GREATER_THAN:
+ return (left > right);
+ case AUDIT_GREATER_THAN_OR_EQUAL:
+ return (left >= right);
+ default:
+ return -EINVAL;
+ }
+}
+
/* Compare a task_struct with an audit_rule. Return 1 on match, 0
* otherwise. */
static int audit_filter_rules(struct task_struct *tsk,
@@ -395,62 +431,71 @@ static int audit_filter_rules(struct tas
int i, j;
for (i = 0; i < rule->field_count; i++) {
- u32 field = rule->fields[i] & ~AUDIT_NEGATE;
+ u32 field = rule->fields[i] & ~AUDIT_OPERATORS;
+ u32 op = rule->fields[i] & AUDIT_OPERATORS;
u32 value = rule->values[i];
int result = 0;
switch (field) {
case AUDIT_PID:
- result = (tsk->pid == value);
+ result = audit_comparator(tsk->pid, op, value);
break;
case AUDIT_UID:
- result = (tsk->uid == value);
+ result = audit_comparator(tsk->uid, op, value);
break;
case AUDIT_EUID:
- result = (tsk->euid == value);
+ result = audit_comparator(tsk->euid, op, value);
break;
case AUDIT_SUID:
- result = (tsk->suid == value);
+ result = audit_comparator(tsk->suid, op, value);
break;
case AUDIT_FSUID:
- result = (tsk->fsuid == value);
+ result = audit_comparator(tsk->fsuid, op, value);
break;
case AUDIT_GID:
- result = (tsk->gid == value);
+ result = audit_comparator(tsk->gid, op, value);
break;
case AUDIT_EGID:
- result = (tsk->egid == value);
+ result = audit_comparator(tsk->egid, op, value);
break;
case AUDIT_SGID:
- result = (tsk->sgid == value);
+ result = audit_comparator(tsk->sgid, op, value);
break;
case AUDIT_FSGID:
- result = (tsk->fsgid == value);
+ result = audit_comparator(tsk->fsgid, op, value);
break;
case AUDIT_PERS:
- result = (tsk->personality == value);
+ result = audit_comparator(tsk->personality, op, value);
break;
case AUDIT_ARCH:
if (ctx)
- result = (ctx->arch == value);
+ result = audit_comparator(ctx->arch, op, value);
break;
case AUDIT_EXIT:
if (ctx && ctx->return_valid)
- result = (ctx->return_code == value);
+ result = audit_comparator(ctx->return_code,
+ op, value);
break;
case AUDIT_SUCCESS:
if (ctx && ctx->return_valid) {
if (value)
- result = (ctx->return_valid == AUDITSC_SUCCESS);
+ result = audit_comparator(
+ ctx->return_valid,
+ op, AUDITSC_SUCCESS);
else
- result = (ctx->return_valid == AUDITSC_FAILURE);
+ result = audit_comparator(
+ ctx->return_valid, op,
+ AUDITSC_FAILURE);
}
break;
case AUDIT_DEVMAJOR:
if (ctx) {
for (j = 0; j < ctx->name_count; j++) {
- if (MAJOR(ctx->names[j].dev)==value) {
+ if ( audit_comparator(
+ MAJOR(ctx->names[j].dev),
+ op, value)
+ ) {
++result;
break;
}
@@ -460,7 +505,10 @@ static int audit_filter_rules(struct tas
case AUDIT_DEVMINOR:
if (ctx) {
for (j = 0; j < ctx->name_count; j++) {
- if (MINOR(ctx->names[j].dev)==value) {
+ if ( audit_comparator(
+ MINOR(ctx->names[j].dev),
+ op, value)
+ ) {
++result;
break;
}
@@ -470,7 +518,9 @@ static int audit_filter_rules(struct tas
case AUDIT_INODE:
if (ctx) {
for (j = 0; j < ctx->name_count; j++) {
- if (ctx->names[j].ino == value) {
+ if ( audit_comparator(
+ ctx->names[j].ino, op, value)
+ ) {
++result;
break;
}
@@ -480,19 +530,20 @@ static int audit_filter_rules(struct tas
case AUDIT_LOGINUID:
result = 0;
if (ctx)
- result = (ctx->loginuid == value);
+ result = audit_comparator(ctx->loginuid, op,
+ value);
break;
case AUDIT_ARG0:
case AUDIT_ARG1:
case AUDIT_ARG2:
case AUDIT_ARG3:
if (ctx)
- result = (ctx->argv[field-AUDIT_ARG0]==value);
+ result = audit_comparator(
+ ctx->argv[field-AUDIT_ARG0],
+ op, value);
break;
}
- if (rule->fields[i] & AUDIT_NEGATE)
- result = !result;
if (!result)
return 0;
}
@@ -563,27 +614,26 @@ static int audit_filter_user_rules(struc
int i;
for (i = 0; i < rule->field_count; i++) {
- u32 field = rule->fields[i] & ~AUDIT_NEGATE;
+ u32 field = rule->fields[i] & ~AUDIT_OPERATORS;
+ u32 op = rule->fields[i] & AUDIT_OPERATORS;
u32 value = rule->values[i];
int result = 0;
switch (field) {
case AUDIT_PID:
- result = (cb->creds.pid == value);
+ result = audit_comparator(cb->creds.pid, op, value);
break;
case AUDIT_UID:
- result = (cb->creds.uid == value);
+ result = audit_comparator(cb->creds.uid, op, value);
break;
case AUDIT_GID:
- result = (cb->creds.gid == value);
+ result = audit_comparator(cb->creds.gid, op, value);
break;
case AUDIT_LOGINUID:
- result = (cb->loginuid == value);
+ result = audit_comparator(cb->loginuid, op, value);
break;
}
- if (rule->fields[i] & AUDIT_NEGATE)
- result = !result;
if (!result)
return 0;
}
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 189 bytes
Desc: This is a digitally signed message part
URL: <http://listman.redhat.com/archives/linux-audit/attachments/20051026/ba53febb/attachment.sig>
More information about the Linux-audit
mailing list