Discussion:
[PATCH 1/2] SELinux Context Label based audit filtering
(too old to reply)
Dustin Kirkland
2006-02-02 19:41:13 UTC
Permalink
Kernel audit component

This patch is against David Woodhouse's last update of his audit-2.6 git
tree, plus a patch submitted by Amy Griffis on 2006-01-17 that adds
support for strings in audit rules. This patch can be found here:
https://www.redhat.com/archives/linux-audit/2006-January/msg00064.html

Let me highlight and explain a few key points:

I've added another function similar to audit_comparator():
audit_str_comparator(). This function takes a left string, an operator,
and a right string and returns true or false based on the comparison.
NULL is considered absolute zero, such that any valid string is greater
than NULL. Additionally, two NULL strings are considered equal. These
assumptions are made such that ugly NULL pointer dereferences are easily
avoided.

Please double check my memory management. I need to kmalloc() space to
memcpy() the string from the netlink message into a kernel rule
structure. Obviously, this needs to be cleaned up properly upon rule
destruction, which I think I'm doing properly.

I also split the function audit_log_task_context() into two parts. The
first part allocates the memory necessary and returns the a context
(aka, label) as a string pointer--this new function is called
audit_get_task_label(). In turn, audit_log_task_context() simply calls
audit_get_task_label() and inserts that string into the audit record.
And audit_get_task_label() is called just before the filter code such
that it has a label to potentially filter upon. Again, proper kfree()'s
are required as audit_get_task_label() does the memory allocation and
the consumer must free.

Note that this code actually only provides enough functionality to
filter on _task_ labels. I'm looking for input or acknowledgment from
the SELinux guys (cc'd) on the validity of the approach herein.
Additionally, I'm open to suggestions on how I might similarly collect
object and user labels for the same filtering mechanism (if required).
I hope to easily extend this patch to handle those as well, though I
wanted to put this much forth immediately to incorporate suggestions.

Comments appreciated...

:-Dustin

---

diff --git a/include/linux/audit.h b/include/linux/audit.h
--- a/include/linux/audit.h
+++ b/include/linux/audit.h
@@ -140,6 +140,11 @@
#define AUDIT_PERS 10
#define AUDIT_ARCH 11
#define AUDIT_MSGTYPE 12
+#define AUDIT_SE_USER 13 /* security label user */
+#define AUDIT_SE_ROLE 14 /* security label role */
+#define AUDIT_SE_TYPE 15 /* security label type */
+#define AUDIT_SE_CAT 16 /* security label category */
+#define AUDIT_SE_SENS 17 /* security label sensitivity */

/* These are ONLY useful when checking
* at syscall exit time (AUDIT_AT_EXIT). */
diff --git a/kernel/audit.c b/kernel/audit.c
diff --git a/kernel/audit.h b/kernel/audit.h
--- a/kernel/audit.h
+++ b/kernel/audit.h
@@ -56,6 +56,7 @@ struct audit_field {
u32 type;
u32 val;
u32 op;
+ char *buf;
};

struct audit_krule {
@@ -78,6 +79,7 @@ struct audit_entry {

extern int audit_pid;
extern int audit_comparator(const u32 left, const u32 op, const u32 right);
+extern int audit_str_comparator(const char *left, const u32 op, const char *right);

extern void audit_send_reply(int pid, int seq, int type,
int done, int multi,
diff --git a/kernel/auditfilter.c b/kernel/auditfilter.c
--- a/kernel/auditfilter.c
+++ b/kernel/auditfilter.c
@@ -160,15 +160,14 @@ static struct audit_entry *audit_data_to
{
int err = 0;
struct audit_entry *entry;
- void *bufp;
/* size_t remain = datasz - sizeof(struct audit_rule_data); */
int i;
+ int offset = 0;

entry = audit_to_entry_common((struct audit_rule *)data);
if (IS_ERR(entry))
goto exit_nofree;

- bufp = data->buf;
entry->rule.vers_ops = 2;
for (i = 0; i < data->field_count; i++) {
struct audit_field *f = &entry->rule.fields[i];
@@ -180,10 +179,26 @@ static struct audit_entry *audit_data_to

f->op = data->fieldflags[i] & AUDIT_OPERATORS;
f->type = data->fields[i];
+ f->val = data->values[i];
switch(f->type) {
/* call type-specific conversion routines here */
+ case AUDIT_SE_USER:
+ case AUDIT_SE_ROLE:
+ case AUDIT_SE_TYPE:
+ case AUDIT_SE_CAT:
+ case AUDIT_SE_SENS:
+ f->buf = kmalloc(f->val + 1, GFP_KERNEL);
+ if (!f->buf) {
+ err = -ENOMEM;
+ goto exit_free;
+ }
+ memcpy(f->buf, data->buf + offset, f->val);
+ f->buf[f->val] = 0;
+ offset += f->val;
+ break;
default:
- f->val = data->values[i];
+ f->buf = NULL;
+ break;
}
}

@@ -326,7 +341,13 @@ static inline int audit_add_rule(struct

static inline void audit_free_rule(struct rcu_head *head)
{
+ int i;
struct audit_entry *e = container_of(head, struct audit_entry, rcu);
+ for (i=0; i<e->rule.field_count; i++) {
+ struct audit_field *f = &e->rule.fields[i];
+ if (f->buf)
+ kfree(f->buf);
+ }
kfree(e);
}

@@ -521,6 +542,39 @@ int audit_comparator(const u32 left, con
}
}

+int audit_str_comparator(const char *left, const u32 op, const char *right)
+{
+ u32 i = 0;
+
+ if ( (left == NULL) || (right == NULL) ) {
+ /* first handle cases where left or right are NULL */
+ if ( (left == NULL) && (right == NULL) )
+ i = 0;
+ else if ( (left == NULL) && (right != NULL) )
+ i = -1;
+ else if ( (left != NULL) && (right == NULL))
+ i = 1;
+ } else
+ /* ok, not NULL, so compare strings */
+ i = strcmp(left, right);
+
+ switch(op) {
+ case AUDIT_EQUAL:
+ return (i == 0);
+ case AUDIT_NOT_EQUAL:
+ return (i != 0);
+ case AUDIT_LESS_THAN:
+ return (i < 0);
+ case AUDIT_LESS_THAN_OR_EQUAL:
+ return (i <= 0);
+ case AUDIT_GREATER_THAN:
+ return (i > 0);
+ case AUDIT_GREATER_THAN_OR_EQUAL:
+ return (i >= 0);
+ default:
+ return -EINVAL;
+ }
+}


static int audit_filter_user_rules(struct netlink_skb_parms *cb,
diff --git a/kernel/auditsc.c b/kernel/auditsc.c
--- a/kernel/auditsc.c
+++ b/kernel/auditsc.c
@@ -157,15 +157,23 @@ struct audit_context {
#endif
};

+static char *audit_get_task_label(void);

/* 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_krule *rule,
struct audit_context *ctx,
- enum audit_state *state)
+ enum audit_state *state,
+ char *label)
{
int i, j;
+ char *user, *role, *type, *cat, *sens;
+ user = strsep(&label, ":");
+ role = strsep(&label, ":");
+ type = strsep(&label, ":");
+ cat = strsep(&label, ":");
+ sens = strsep(&label, ":");

for (i = 0; i < rule->field_count; i++) {
struct audit_field *f = &rule->fields[i];
@@ -206,6 +214,21 @@ static int audit_filter_rules(struct tas
if (ctx)
result = audit_comparator(ctx->arch, f->op, f->val);
break;
+ case AUDIT_SE_USER:
+ result = audit_str_comparator(user, f->op, f->buf);
+ break;
+ case AUDIT_SE_ROLE:
+ result = audit_str_comparator(role, f->op, f->buf);
+ break;
+ case AUDIT_SE_TYPE:
+ result = audit_str_comparator(type, f->op, f->buf);
+ break;
+ case AUDIT_SE_CAT:
+ result = audit_str_comparator(cat, f->op, f->buf);
+ break;
+ case AUDIT_SE_SENS:
+ result = audit_str_comparator(sens, f->op, f->buf);
+ break;

case AUDIT_EXIT:
if (ctx && ctx->return_valid)
@@ -283,15 +306,18 @@ static enum audit_state audit_filter_tas
{
struct audit_entry *e;
enum audit_state state;
+ char *label = audit_get_task_label();

rcu_read_lock();
list_for_each_entry_rcu(e, &audit_filter_list[AUDIT_FILTER_TASK], list) {
- if (audit_filter_rules(tsk, &e->rule, NULL, &state)) {
+ if (audit_filter_rules(tsk, &e->rule, NULL, &state, label)) {
rcu_read_unlock();
return state;
}
}
rcu_read_unlock();
+ if (label)
+ kfree(label);
return AUDIT_BUILD_CONTEXT;
}

@@ -306,6 +332,7 @@ static enum audit_state audit_filter_sys
{
struct audit_entry *e;
enum audit_state state;
+ char *label = audit_get_task_label();

if (audit_pid && tsk->tgid == audit_pid)
return AUDIT_DISABLED;
@@ -317,13 +344,15 @@ static enum audit_state audit_filter_sys

list_for_each_entry_rcu(e, list, list) {
if ((e->rule.mask[word] & bit) == bit
- && audit_filter_rules(tsk, &e->rule, ctx, &state)) {
+ && audit_filter_rules(tsk, &e->rule, ctx, &state, label)) {
rcu_read_unlock();
return state;
}
}
}
rcu_read_unlock();
+ if (label)
+ kfree(label);
return AUDIT_BUILD_CONTEXT;
}

@@ -503,32 +532,44 @@ static inline void audit_free_context(st

static void audit_log_task_context(struct audit_buffer *ab)
{
- char *ctx = NULL;
+ char *label = audit_get_task_label();
+
+ if (label == NULL)
+ audit_panic("error in audit_log_task_context");
+ else {
+ audit_log_format(ab, " subj=%s", label);
+ kfree(label);
+ }
+ return;
+}
+
+static char *audit_get_task_label(void)
+{
+ char *label = NULL;
ssize_t len = 0;

len = security_getprocattr(current, "current", NULL, 0);
if (len < 0) {
if (len != -EINVAL)
goto error_path;
- return;
+ return NULL;
}

- ctx = kmalloc(len, GFP_KERNEL);
- if (!ctx)
+ label = kmalloc(len, GFP_KERNEL);
+ if (!label)
goto error_path;

- len = security_getprocattr(current, "current", ctx, len);
+ len = security_getprocattr(current, "current", label, len);
if (len < 0 )
goto error_path;

- audit_log_format(ab, " subj=%s", ctx);
- return;
+ return label;

error_path:
- if (ctx)
- kfree(ctx);
- audit_panic("error in audit_log_task_context");
- return;
+ if (label)
+ kfree(label);
+ audit_panic("error in audit_get_task_label");
+ return NULL;
}

static void audit_log_task_info(struct audit_buffer *ab)
Dustin Kirkland
2006-02-02 21:47:48 UTC
Permalink
Post by Dustin Kirkland
Kernel audit component
This patch is against David Woodhouse's last update of his audit-2.6 git
tree, plus a patch submitted by Amy Griffis on 2006-01-17 that adds
https://www.redhat.com/archives/linux-audit/2006-January/msg00064.html
Patch was encoded by your mailer.
That it was. Apparently Evolution automatically encodes messages when
signed with a GPG key. Which means a difficult choice between making
the patch readable and establishing authenticity ;)
Post by Dustin Kirkland
Note that this code actually only provides enough functionality to
filter on _task_ labels. I'm looking for input or acknowledgment from
the SELinux guys (cc'd) on the validity of the approach herein.
Additionally, I'm open to suggestions on how I might similarly collect
object and user labels for the same filtering mechanism (if required).
I hope to easily extend this patch to handle those as well, though I
wanted to put this much forth immediately to incorporate suggestions.
Object security contexts are already being harvested along the way, e.g.
audit_inode() -> audit_inode_context(), so you already have them
available at the point of filter checking. Other (less expensive
option) for both the object contexts and the task context would be to
instead only harvest the SIDs (via new interfaces) and to provide
interfaces for getting specific fields and compare them rather than
having to allocate memory and generate full contexts each time, as we've
discussed before on the list. That does require changes to the SELinux
module and new interfaces from it, of course.
Let the efficiency games begin...

I'll gladly pursue the less expensive option, though I will require some
guidance from you in implementing these new SELinux exported API's.

In this case, I guess I would like to have SELinux export something like
the following hypothetical function, which takes as input a sid and the
field position, and SELinux returns a char pointer to the requested
string:
char *security_get_field_from_sid(u32 sid, u32 field);

As it seems similar in functionality, should it live somewhere near
security_sid_to_context() in security/selinux/ss/services.c? That
function expects a preallocated string... Should it be possible to
return a const char* from the hypothetical security_get_field_from_sid()
to simplify the caller's mem management? Where's the const string
located in SELinux located that could be so sliced up? I'm really
looking for some pointers here.

Thus, the audit code would first need to call something like:
u32 security_get_sid(???);

What arguments would be required to such a function? Could it be
general purpose enough for inode/ipc/etc objects, as well as tasks?
Post by Dustin Kirkland
Comments appreciated...
diff --git a/include/linux/audit.h b/include/linux/audit.h
--- a/include/linux/audit.h
+++ b/include/linux/audit.h
@@ -140,6 +140,11 @@
#define AUDIT_PERS 10
#define AUDIT_ARCH 11
#define AUDIT_MSGTYPE 12
+#define AUDIT_SE_USER 13 /* security label user */
+#define AUDIT_SE_ROLE 14 /* security label role */
+#define AUDIT_SE_TYPE 15 /* security label type */
+#define AUDIT_SE_CAT 16 /* security label category */
+#define AUDIT_SE_SENS 17 /* security label sensitivity */
There can be two levels in the MLS field (a low and a high), so you have
potentially two sensitivities and two category sets, plus you may want
to match on a particular category in a category set, not the entire
thing. Also, the above doesn't seem very extensible to cope with
potential future extension of the SELinux security context.
Well, the audit rule filter structure needs some unique way to identify
each field, which we're doing with a unique integer per field. Looking
at the lines preceding these new #define's, you'll see the rest of the
fields that we're able to filter upon.

I can easily throw in:
AUDIT_SE_CAT_L
AUDIT_SE_CAT_H
AUDIT_SE_SENS_L
AUDIT_SE_SENS_H

And add those to the switch statement. That can continue on basically
indefinitely. I don't know how much growth you expect in the context
label, but the audit system would have to stay in sync with your
changes.

But if I read you correctly, you'd like to see an entirely different
approach.

I suppose we could set the type of the field to a single AUDIT_SE_LABEL,
and elsewhere in the audit_rule_data structure store the offset (the
element number) of the SELinux label to match. Unfortunately, I'm not
seeing a clean place to drop that offset integer into the structure.
Perhaps in audit_rule_data->fieldflags[i], but I don't really think
that's what that structure member was intended for.

Other suggestions?
Post by Dustin Kirkland
diff --git a/kernel/auditsc.c b/kernel/auditsc.c
--- a/kernel/auditsc.c
+++ b/kernel/auditsc.c
@@ -157,15 +157,23 @@ struct audit_context {
#endif
};
+static char *audit_get_task_label(void);
/* 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_krule *rule,
struct audit_context *ctx,
- enum audit_state *state)
+ enum audit_state *state,
+ char *label)
{
int i, j;
+ char *user, *role, *type, *cat, *sens;
+ user = strsep(&label, ":");
+ role = strsep(&label, ":");
+ type = strsep(&label, ":");
+ cat = strsep(&label, ":");
+ sens = strsep(&label, ":");
Audit code should not be directly parsing SELinux contexts.
You want the SELinux module to perform the splitting, and preferably to
even just give you the field directly as a const char * from SID so that
you never have to allocate and generate the entire context string.
Ok. I think the comments/questions I posted above should address
this...


:-Dustin
Steve Grubb
2006-02-02 22:18:16 UTC
Permalink
Other (less expensive option) for both the object contexts and the task
context would be to instead only harvest the SIDs (via new interfaces) and
to provide interfaces for getting specific fields and compare them rather
than having to allocate memory and generate full contexts each time, as
we've discussed before on the list.  That does require changes to the
SELinux module and new interfaces from it, of course.
Stephen, It might be better if you make an interface for Dustin and send the
patch. We can then apply it locally for testing.
Post by Dustin Kirkland
diff --git a/include/linux/audit.h b/include/linux/audit.h
--- a/include/linux/audit.h
+++ b/include/linux/audit.h
@@ -140,6 +140,11 @@
 #define AUDIT_PERS 10
 #define AUDIT_ARCH 11
 #define AUDIT_MSGTYPE      12
+#define AUDIT_SE_USER      13      /* security label user */
+#define AUDIT_SE_ROLE      14      /* security label role */
+#define AUDIT_SE_TYPE      15      /* security label type */
+#define AUDIT_SE_CAT       16      /* security label category */
+#define AUDIT_SE_SENS      17      /* security label sensitivity */
There can be two levels in the MLS field (a low and a high), so you have
potentially two sensitivities and two category sets, plus you may want
to match on a particular category in a category set, not the entire
thing.
I think we are covered. I mentioned to Dustin that those fields need to be
handled as integers for comparison. We should be able to specify a range for
matching like:

-F "se_sensitivity>=2" -F "se_sensitivity<=9"
Also, the above doesn't seem very extensible to cope with
potential future extension of the SELinux security context.
Is there a convention for context parsing? If not, we should probably decide
what it will be or at least how to identify the end of what we know so that
if they get out of sync in the future, it would do the wrong thing.

-Steve
Steve Grubb
2006-02-02 22:22:57 UTC
Permalink
Post by Steve Grubb
I think we are covered. I mentioned to Dustin that those fields need to be
handled as integers for comparison. We should be able to specify a range
-F "se_sensitivity>=2" -F "se_sensitivity<=9"
Oops sb

-F "se_sensitivity>=s2" -F "se_sensitivity<=s9"

Or whatever the local customizations are:

-F "se_sensitivity>=confidential" -F "se_sensitivity<=top-secret"

-Steve
Stephen Smalley
2006-02-03 14:17:16 UTC
Permalink
Post by Steve Grubb
I think we are covered. I mentioned to Dustin that those fields need to be
handled as integers for comparison. We should be able to specify a range for
-F "se_sensitivity>=2" -F "se_sensitivity<=9"
This requires that SELinux perform the filter interpretation, as the
context structures and dominance relation are purely internal to it, and
the audit system should not be directly tied to them.
Post by Steve Grubb
Is there a convention for context parsing? If not, we should probably decide
what it will be or at least how to identify the end of what we know so that
if they get out of sync in the future, it would do the wrong thing.
The "convention" is that only the SELinux module and the core SELinux
libraries parse them. Everything else has to use an API provided by the
SELinux module (for in-kernel users) or the core SELinux libraries (for
userland).
--
Stephen Smalley
National Security Agency
Steve Grubb
2006-02-03 14:27:39 UTC
Permalink
Post by Stephen Smalley
Post by Steve Grubb
-F "se_sensitivity>=2" -F "se_sensitivity<=9"
This requires that SELinux perform the filter interpretation, as the
context structures and dominance relation are purely internal to it, and
the audit system should not be directly tied to them.
The plan was to call SE linux libraries to interpret custom text (public) to
sensitivity and send the raw sensitivity (s0). As for dominance
calculations...we aren't granting access. If someone say they want s2 and
above, we should be able to see that s3 is greater than s2 and generate an
audit record. If you have an api for comparing s3 and s2, let us know and
we'll use it.
Post by Stephen Smalley
The "convention" is that only the SELinux module and the core SELinux
libraries parse them. Everything else has to use an API provided by the
SELinux module (for in-kernel users) or the core SELinux libraries (for
userland).
I suppose you are right. I should have mentioned that we have no interest in
parsing the full context. User space was going to take the context parsed by
humans as separate fields. This way it is extensible. If a new extension is
added in the future, we add a new field.

In kernel, Dustin was going to use your api to take sid to individual
components. For string we only need = and !=. For levels and sensitivity, we
were going to need to do a comparison since people could desire auditing
secret and above, but let everything else go.

-Steve
Stephen Smalley
2006-02-03 14:46:14 UTC
Permalink
Post by Steve Grubb
Post by Stephen Smalley
Post by Steve Grubb
-F "se_sensitivity>=2" -F "se_sensitivity<=9"
This requires that SELinux perform the filter interpretation, as the
context structures and dominance relation are purely internal to it, and
the audit system should not be directly tied to them.
The plan was to call SE linux libraries to interpret custom text (public) to
sensitivity and send the raw sensitivity (s0).
Right, libsetrans. But that still leaves you with a string that has no
inherent meaning or ordering.
Post by Steve Grubb
As for dominance
calculations...we aren't granting access. If someone say they want s2 and
above, we should be able to see that s3 is greater than s2 and generate an
audit record. If you have an api for comparing s3 and s2, let us know and
we'll use it.
Nothing guarantees that s3 dominates s2; s2 _could_ dominate s3
depending on the policy specification. In the SELinux security server,
the relevant functions are mls_level_eq (equivalent), mls_level_dom
(dominates), and mls_level_incomp (incomparable), but they act on the
internal structure representation (integer sensitivities and extensible
bitmap category sets), not the string format. audit system needs to
call a SELinux API if it wants to compare two MLS levels.
Post by Steve Grubb
I suppose you are right. I should have mentioned that we have no interest in
parsing the full context. User space was going to take the context parsed by
humans as separate fields. This way it is extensible. If a new extension is
added in the future, we add a new field.
That's fine.
Post by Steve Grubb
In kernel, Dustin was going to use your api to take sid to individual
components. For string we only need = and !=. For levels and sensitivity, we
were going to need to do a comparison since people could desire auditing
secret and above, but let everything else go.
Ok, so this means that SELinux needs to provide an API for such
comparisons, and likely for precomputing the internal context structure
for a given MLS range provided in an audit rule so that we don't have to
re-do that on each filter evaluation. Then you pass a handle to that
precomputed context and the SID to the comparison API, and it returns
the relation (equivalent, dominates, dominatedby, incomparable).
--
Stephen Smalley
National Security Agency
Steve Grubb
2006-02-03 14:47:01 UTC
Permalink
Post by Stephen Smalley
Ok, so this means that SELinux needs to provide an API for such
comparisons, and likely for precomputing the internal context structure
for a given MLS range provided in an audit rule so that we don't have to
re-do that on each filter evaluation.
What if the filter rule was:

auditctl -a exit,always -S open -F "se_sensitivity>=confidential"

And that is all you have to work with? Are we still OK?

-Steve
Stephen Smalley
2006-02-03 15:12:33 UTC
Permalink
Post by Steve Grubb
Post by Stephen Smalley
Ok, so this means that SELinux needs to provide an API for such
comparisons, and likely for precomputing the internal context structure
for a given MLS range provided in an audit rule so that we don't have to
re-do that on each filter evaluation.
auditctl -a exit,always -S open -F "se_sensitivity>=confidential"
And that is all you have to work with? Are we still OK?
First, you'd need a way to indicate whether you mean the low (also
referred to as current) level or the high (also referred to as clearance
or max) level in the above specification, as each security context has
two levels associated with it (they show up abbreviated as a single
level if they happen to be the same), a low and a high.

Given that distinction, the way I see this as working is as follows:
1) the above auditctl command is invoked by the user,
2) auditctl calls libsetrans to map the string "confidential" to the
string "s1",
3) auditctl constructs a filter rule specifying the above filter,
including the "s1" still as a string, and passes it to the kernel audit
system,
4) the kernel audit system passes the "s1" string to a new in-kernel API
for SELinux that returns a void* handle to a precomputed mls level
structure (likely augmented with policy serial number to detect
invalidation due to policy reload) dynamically allocated at the time the
filter rule is inserted,
5) the kernel audit system saves this handle in its internal form of the
filter rule for later use upon filter evaluation/checking,
6) the kernel audit system harvests SIDs as needed during syscall
processing for the current task and objects referenced during the
syscall and saves them off in audit context,
7) upon filter evaluation/checking, the kernel audit system passes the
void* handle saved in the rule and the desired SID to a new in-kernel
API for SELinux that applies the MLS dominance computation and returns
one of (equivalent, dominates, dominatedby, or incomparable).
8) the audit system checks whether the returned relation matches the one
specified in the filter (here, >= is the same as dominates).
9) If so, the audit system then asks for contexts for the relevant SIDs
and adds them to the audit message. This might be an issue btw as we
might run into an allocation failure at this point.

We would also need a callback registered by the audit system to be
called by SELinux upon policy reload to re-compute all of the
precomputed MLS level structures saved in audit filters against the new
policy.
--
Stephen Smalley
National Security Agency
Stephen Smalley
2006-02-03 15:14:09 UTC
Permalink
Post by Stephen Smalley
Post by Steve Grubb
Post by Stephen Smalley
Ok, so this means that SELinux needs to provide an API for such
comparisons, and likely for precomputing the internal context structure
for a given MLS range provided in an audit rule so that we don't have to
re-do that on each filter evaluation.
auditctl -a exit,always -S open -F "se_sensitivity>=confidential"
And that is all you have to work with? Are we still OK?
First, you'd need a way to indicate whether you mean the low (also
referred to as current) level or the high (also referred to as clearance
or max) level in the above specification, as each security context has
two levels associated with it (they show up abbreviated as a single
level if they happen to be the same), a low and a high.
The other missing distinction above is whether you are referring to the
process sensitivity or the sensitivity of one of the objects accessed
during the syscall, e.g. the file context.
--
Stephen Smalley
National Security Agency
Stephen Smalley
2006-02-03 15:20:49 UTC
Permalink
Post by Stephen Smalley
Post by Stephen Smalley
Post by Steve Grubb
Post by Stephen Smalley
Ok, so this means that SELinux needs to provide an API for such
comparisons, and likely for precomputing the internal context structure
for a given MLS range provided in an audit rule so that we don't have to
re-do that on each filter evaluation.
auditctl -a exit,always -S open -F "se_sensitivity>=confidential"
And that is all you have to work with? Are we still OK?
First, you'd need a way to indicate whether you mean the low (also
referred to as current) level or the high (also referred to as clearance
or max) level in the above specification, as each security context has
two levels associated with it (they show up abbreviated as a single
level if they happen to be the same), a low and a high.
The other missing distinction above is whether you are referring to the
process sensitivity or the sensitivity of one of the objects accessed
during the syscall, e.g. the file context.
And where it becomes even more fun is when multiple objects are accessed
during the syscall, e.g. during path lookup you'll harvest one object
context or SID per path component. So is the above filter supposed to
be applied to just the terminal component or all of them?
--
Stephen Smalley
National Security Agency
Steve Grubb
2006-02-03 15:20:14 UTC
Permalink
So is the above filter supposed to be applied to just the terminal
component or all of them?
I would expect it to be the object that is actually opened rather than any
intermediate path components.

-Steve
Stephen Smalley
2006-02-03 15:49:47 UTC
Permalink
Post by Steve Grubb
So is the above filter supposed to be applied to just the terminal
component or all of them?
I would expect it to be the object that is actually opened rather than any
intermediate path components.
Hmm..well, audit system harvests the information for the inodes as the
lookup proceeds, so it ends up with the information for all of them.
And the last one might not even be the terminal component of the
specified path; it may just be the last one before it hit some error
(like a search denial on a directory component).
also, you still need to distinguish between filters on process context
and filters on object context at the least.
--
Stephen Smalley
National Security Agency
Stephen Smalley
2006-02-03 15:32:54 UTC
Permalink
Post by Steve Grubb
So is the above filter supposed to be applied to just the terminal
component or all of them?
I would expect it to be the object that is actually opened rather than any
intermediate path components.
Hmm..well, audit system harvests the information for the inodes as the
lookup proceeds, so it ends up with the information for all of them.
And the last one might not even be the terminal component of the
specified path; it may just be the last one before it hit some error
(like a search denial on a directory component).
--
Stephen Smalley
National Security Agency
Amy Griffis
2006-02-06 22:49:55 UTC
Permalink
Stephen Smalley wrote: [Fri Feb 03 2006, 10:32:54AM EST]
Post by Steve Grubb
So is the above filter supposed to be applied to just the terminal
component or all of them?
I would expect it to be the object that is actually opened rather than any
intermediate path components.
Hmm..well, audit system harvests the information for the inodes as the
lookup proceeds, so it ends up with the information for all of them.
That may be how the audit system used to work, but it doesn't work
quite like that anymore.

Audit typically collects information about the inode which is the
terminal component of the specified path. If the operation involves
adding or removing objects from a directory, information about the
relevant dentry parent inodes is also collected.

At most, audit would collect information about:
1) source inode
2) source inode's parent
3) target inode
4) target inode's parent
And the last one might not even be the terminal component of the
specified path; it may just be the last one before it hit some error
(like a search denial on a directory component).
In the unsuccessful case, the source or target inode may not be
collected, and the parent inode may be indicated as the last path
component accessed, as you said.

Hope this helps,
Amy
Dustin Kirkland
2006-02-03 17:26:39 UTC
Permalink
Post by Stephen Smalley
9) If so, the audit system then asks for contexts for the relevant SIDs
and adds them to the audit message. This might be an issue btw as we
might run into an allocation failure at this point.
Actually, this "filtering" isn't determining whether or not the context
is added to the messsage... Rather, it's determining whether or not
audit constructs the audit message at all and passes it to the audit
daemon for publication. Maybe, you realized that already--just
clarifying though.

:-Dustin
Dustin Kirkland
2006-02-03 16:37:17 UTC
Permalink
Post by Stephen Smalley
Post by Steve Grubb
Post by Stephen Smalley
Post by Steve Grubb
-F "se_sensitivity>=2" -F "se_sensitivity<=9"
This requires that SELinux perform the filter interpretation, as the
context structures and dominance relation are purely internal to it, and
the audit system should not be directly tied to them.
The plan was to call SE linux libraries to interpret custom text (public) to
sensitivity and send the raw sensitivity (s0).
Right, libsetrans. But that still leaves you with a string that has no
inherent meaning or ordering.
This is begging for placement in a configuration file that allows custom
defined aliases:
"s0" = "non_confidential"
"s1" = "secret"
"s2" = "mostly_secret"
"s3" = "more_secret_than_that"
"s4" = "top_secret"
"s5" = "cheating_on_a_spouse_secret"


Let those be set in either an SELinux config file, or in an Audit config
file. Let audit userspace interpret these human readable aliases to
SELinux's representation.

:-Dustin
Stephen Smalley
2006-02-03 14:13:35 UTC
Permalink
Post by Dustin Kirkland
Let the efficiency games begin...
I'll gladly pursue the less expensive option, though I will require some
guidance from you in implementing these new SELinux exported API's.
In this case, I guess I would like to have SELinux export something like
the following hypothetical function, which takes as input a sid and the
field position, and SELinux returns a char pointer to the requested
char *security_get_field_from_sid(u32 sid, u32 field);
The public interface could even be along the lines of:
selinux_get_role_from_sid
selinux_get_user_from_sid
selinux_get_type_from_sid
selinux_get_range_from_sid
That is ok; they can be internally implemented via common helper as
appropriate.

It should return const char *, and preferably return it via argument
pointer, returning int as the return value to indicate error status.
Post by Dustin Kirkland
As it seems similar in functionality, should it live somewhere near
security_sid_to_context() in security/selinux/ss/services.c?
Yes.
Post by Dustin Kirkland
That
function expects a preallocated string...
No, that function internally allocates the string buffer and sets
*scontext to refer to it. But for your purposes, there is no reason to
perform any dynamic allocation or copying; you just lookup the SID in
the sidtab, grab the desired field value, and index into the val_to_name
arrays to obtain the already existing string which you can return as a
const char *. Only complicating case is for the MLS range which is
multi-component.
Post by Dustin Kirkland
Should it be possible to
return a const char* from the hypothetical security_get_field_from_sid()
to simplify the caller's mem management? Where's the const string
located in SELinux located that could be so sliced up? I'm really
looking for some pointers here.
You would just return a const char * from the existing val_to_name
tables for user/role/type. They aren't internally const, but the caller
doesn't need to modify the returned string.
Post by Dustin Kirkland
u32 security_get_sid(???);
int return value, pass sid by argument pointer. You'd have
security_task_getsid, security_inode_getsid, etc.
Post by Dustin Kirkland
What arguments would be required to such a function? Could it be
general purpose enough for inode/ipc/etc objects, as well as tasks?
One per object type.
Post by Dustin Kirkland
But if I read you correctly, you'd like to see an entirely different
approach.
I suppose we could set the type of the field to a single AUDIT_SE_LABEL,
and elsewhere in the audit_rule_data structure store the offset (the
element number) of the SELinux label to match. Unfortunately, I'm not
seeing a clean place to drop that offset integer into the structure.
Perhaps in audit_rule_data->fieldflags[i], but I don't really think
that's what that structure member was intended for.
Other suggestions?
For the kernel-userland API, you want something extensible so that we
don't have to revise it if we later add another SELinux context field.
But for the in-kernel APIs, it is ok to be fieldname specific as long as
the audit system is not directly interpreting the context string format
itself.
--
Stephen Smalley
National Security Agency
Dustin Kirkland
2006-02-03 16:29:56 UTC
Permalink
Post by Stephen Smalley
selinux_get_role_from_sid
selinux_get_user_from_sid
selinux_get_type_from_sid
selinux_get_range_from_sid
That is ok; they can be internally implemented via common helper as
appropriate.
It should return const char *, and preferably return it via argument
pointer, returning int as the return value to indicate error status.
Stephen-

Is this something that you have time to hack together, or should I start
working on it?


:-Dustin
Dustin Kirkland
2006-02-17 06:08:11 UTC
Permalink
I've reworked this patch based on the work that Darrel Goedel has recently
submitted in which he has implemented an SELinux interface for the Audit
subsystem to call to match security context label components. This follows
the advice of Stephen Smalley, and Darrel based his interface at least in
part on work that James Morris had started.

These patches are against Al Viro's latest git tree plus the following two
patches:

1. Minor cleanup patch of audit_comparator(); no real dependency here, but might
cause some patch fuzz:
https://www.redhat.com/archives/linux-audit/2006-February/msg00102.html

2. Darrel's SELinux audit api patch:
https://www.redhat.com/archives/linux-audit/2006-February/msg00101.html


The audit system needs to store (and eventually free) pointers to
Darrel's new selinux_audit_rule structure. I implemented this as a
struct selinux_audit_rule *se_rule in kernel/audit.h:audit_field. I
considered making this a void *ext (something more generic), which would
have the advantage of being extensible if something like this ever came
up again, where an audit rule field interfaces with some other kernel
system. Of course, it would need to be cast properly when referenced.
Please speak up if you have strong feelings here.

Amy: In audit_data_to_entry(), you're using an effectively temporary
char *path. I, too, needed a temporary string pointer, so I declared
char *str and replaced your couple of instances of path with str. Let
me know if this is ok by you. I couldn't very well call my temp string
"path". And it didn't make much sense to me to declare another throwaway
pointer. There's a little code duplication with the audit_unpack_string too.

Darrel: I realize now that your selinux_audit_rule structure duplicates a
little data that it doesn't need to, such as au_op and au_field. I could
very easily pass those to you when I call selinux_audit_rule_match. I think
that would be a little cleaner. Just a suggestion.

Steve Grubb mentioned that there's a chance we might get this into the test
kernel by EOB Friday... That would be great. Of course, I've built and
tested the basic functionality and it seems to filter messages in/out
appropriately. I'll submit an updated userspace patch tomorrow.

One last point... This patch only covers matching on process context.
As Darrel and I have discussed, we'll move onto the other contexts once
this methodology is acceptable.


Thanks for your review.
:-Dustin



diff --git a/kernel/audit.h b/kernel/audit.h
--- a/kernel/audit.h
+++ b/kernel/audit.h
@@ -59,9 +59,10 @@ struct audit_watch {
};

struct audit_field {
- u32 type;
- u32 val;
- u32 op;
+ u32 type;
+ u32 val;
+ u32 op;
+ struct selinux_audit_rule *se_rule;
};

struct audit_krule {
diff --git a/kernel/auditfilter.c b/kernel/auditfilter.c
--- a/kernel/auditfilter.c
+++ b/kernel/auditfilter.c
@@ -25,6 +25,7 @@
#include <linux/fs.h>
#include <linux/namei.h>
#include <linux/netlink.h>
+#include <linux/selinux.h>
#include "audit.h"

/* There are three lists of rules -- one to search at task creation
@@ -50,6 +51,12 @@ static inline void audit_free_watch(stru

static inline void audit_free_rule(struct audit_entry *e)
{
+ int i;
+ for (i=0; i<e->rule.field_count; i++) {
+ struct audit_field *f = &e->rule.fields[i];
+ if (f->se_rule)
+ selinux_audit_rule_free(f->se_rule);
+ }
kfree(e->rule.fields);
kfree(e);
}
@@ -228,7 +235,7 @@ static struct audit_entry *audit_data_to
void *bufp;
size_t remain = datasz - sizeof(struct audit_rule_data);
int i;
- char *path;
+ char *str;

entry = audit_to_entry_common((struct audit_rule *)data);
if (IS_ERR(entry))
@@ -247,16 +254,33 @@ static struct audit_entry *audit_data_to
f->op = data->fieldflags[i] & AUDIT_OPERATORS;
f->type = data->fields[i];
f->val = data->values[i];
+ f->se_rule = NULL;
switch(f->type) {
+ case AUDIT_SE_USER:
+ case AUDIT_SE_ROLE:
+ case AUDIT_SE_TYPE:
+ case AUDIT_SE_SEN:
+ case AUDIT_SE_CLR:
+ str = audit_unpack_string(&bufp, &remain, f->val);
+ if (IS_ERR(str))
+ goto exit_free;
+ entry->rule.buflen += f->val;
+
+ err = selinux_audit_rule_init(f->type, f->op, str, &f->se_rule);
+ if (err) {
+ kfree(str);
+ goto exit_free;
+ }
+ break;
case AUDIT_WATCH:
- path = audit_unpack_string(&bufp, &remain, f->val);
- if (IS_ERR(path))
+ str = audit_unpack_string(&bufp, &remain, f->val);
+ if (IS_ERR(str))
goto exit_free;
entry->rule.buflen += f->val;

- err = audit_to_watch(path, &entry->rule, i);
+ err = audit_to_watch(str, &entry->rule, i);
if (err) {
- kfree(path);
+ kfree(str);
goto exit_free;
}
break;
@@ -644,6 +668,9 @@ int audit_comparator(const u32 left, con
case AUDIT_GREATER_THAN_OR_EQUAL:
return (left >= right);
}
+ /* should NEVER get here */
+ BUG();
+ return 0;
}


diff --git a/kernel/auditsc.c b/kernel/auditsc.c
--- a/kernel/auditsc.c
+++ b/kernel/auditsc.c
@@ -58,6 +58,7 @@
#include <linux/security.h>
#include <linux/list.h>
#include <linux/tty.h>
+#include <linux/selinux.h>

#include "audit.h"

@@ -165,7 +166,8 @@ struct audit_context {
static int audit_filter_rules(struct task_struct *tsk,
struct audit_krule *rule,
struct audit_context *ctx,
- enum audit_state *state)
+ enum audit_state *state,
+ u32 sid)
{
int i, j;

@@ -258,6 +260,13 @@ static int audit_filter_rules(struct tas
if (ctx)
result = audit_comparator(ctx->loginuid, f->op, f->val);
break;
+ case AUDIT_SE_USER:
+ case AUDIT_SE_ROLE:
+ case AUDIT_SE_TYPE:
+ case AUDIT_SE_SEN:
+ case AUDIT_SE_CLR:
+ result = selinux_audit_rule_match(sid, f->se_rule);
+ break;
case AUDIT_ARG0:
case AUDIT_ARG1:
case AUDIT_ARG2:
@@ -286,10 +295,13 @@ static enum audit_state audit_filter_tas
{
struct audit_entry *e;
enum audit_state state;
+ u32 sid;
+
+ selinux_task_ctxid(tsk, &sid);

rcu_read_lock();
list_for_each_entry_rcu(e, &audit_filter_list[AUDIT_FILTER_TASK], list) {
- if (audit_filter_rules(tsk, &e->rule, NULL, &state)) {
+ if (audit_filter_rules(tsk, &e->rule, NULL, &state, sid)) {
rcu_read_unlock();
return state;
}
@@ -309,6 +321,9 @@ static enum audit_state audit_filter_sys
{
struct audit_entry *e;
enum audit_state state;
+ u32 sid;
+
+ selinux_task_ctxid(tsk, &sid);

if (audit_pid && tsk->tgid == audit_pid)
return AUDIT_DISABLED;
@@ -320,7 +335,8 @@ static enum audit_state audit_filter_sys

list_for_each_entry_rcu(e, list, list) {
if ((e->rule.mask[word] & bit) == bit
- && audit_filter_rules(tsk, &e->rule, ctx, &state)) {
+ && audit_filter_rules(tsk, &e->rule,
+ ctx, &state, sid)) {
rcu_read_unlock();
return state;
}
Darrel Goeddel
2006-02-17 16:49:14 UTC
Permalink
Post by Dustin Kirkland
Darrel: I realize now that your selinux_audit_rule structure duplicates a
little data that it doesn't need to, such as au_op and au_field. I could
very easily pass those to you when I call selinux_audit_rule_match. I think
that would be a little cleaner. Just a suggestion.
Sure - I can't remember what I did earlier that made me want my own copy.
I have a few othre changes to make. I'll include that change.
Post by Dustin Kirkland
--- a/kernel/audit.h
+++ b/kernel/audit.h
@@ -59,9 +59,10 @@ struct audit_watch {
};
struct audit_field {
- u32 type;
- u32 val;
- u32 op;
+ u32 type;
+ u32 val;
+ u32 op;
+ struct selinux_audit_rule *se_rule;
};
struct audit_krule {
This will require that <linux/selinux.h> be included before "audit.h".
Maybe just include selinux.h from this file to get the structure
declaration.
Post by Dustin Kirkland
@@ -58,6 +58,7 @@
#include <linux/security.h>
#include <linux/list.h>
#include <linux/tty.h>
+#include <linux/selinux.h>
#include "audit.h"
@@ -165,7 +166,8 @@ struct audit_context {
static int audit_filter_rules(struct task_struct *tsk,
struct audit_krule *rule,
struct audit_context *ctx,
- enum audit_state *state)
+ enum audit_state *state,
+ u32 sid)
{
int i, j;
The sid can be obtained from tsk within this function to avoid modifying the
callers to get the sid and pass it in.
Post by Dustin Kirkland
@@ -258,6 +260,13 @@ static int audit_filter_rules(struct tas
if (ctx)
result = audit_comparator(ctx->loginuid, f->op, f->val);
break;
+ result = selinux_audit_rule_match(sid, f->se_rule);
+ break;
What about the error (result < 0) cases here? For this to error, something
pretty bad has happened (a sid couldn't be mapped or an atomic memory allocation
failed). That being said, do we want to go all out and audit_panic?
Post by Dustin Kirkland
@@ -286,10 +295,13 @@ static enum audit_state audit_filter_tas
{
struct audit_entry *e;
enum audit_state state;
+ u32 sid;
+
+ selinux_task_ctxid(tsk, &sid);
rcu_read_lock();
list_for_each_entry_rcu(e, &audit_filter_list[AUDIT_FILTER_TASK], list) {
- if (audit_filter_rules(tsk, &e->rule, NULL, &state)) {
+ if (audit_filter_rules(tsk, &e->rule, NULL, &state, sid)) {
rcu_read_unlock();
return state;
}
@@ -309,6 +321,9 @@ static enum audit_state audit_filter_sys
{
struct audit_entry *e;
enum audit_state state;
+ u32 sid;
+
+ selinux_task_ctxid(tsk, &sid);
if (audit_pid && tsk->tgid == audit_pid)
return AUDIT_DISABLED;
@@ -320,7 +335,8 @@ static enum audit_state audit_filter_sys
list_for_each_entry_rcu(e, list, list) {
if ((e->rule.mask[word] & bit) == bit
- && audit_filter_rules(tsk, &e->rule, ctx, &state)) {
+ && audit_filter_rules(tsk, &e->rule,
+ ctx, &state, sid)) {
rcu_read_unlock();
return state;
}
These are the changes that can go away if audit_filter_rules gets the sid.
--
Darrel
Amy Griffis
2006-02-17 17:43:25 UTC
Permalink
Post by Dustin Kirkland
Amy: In audit_data_to_entry(), you're using an effectively temporary
char *path. I, too, needed a temporary string pointer, so I declared
char *str and replaced your couple of instances of path with str. Let
me know if this is ok by you. I couldn't very well call my temp string
"path". And it didn't make much sense to me to declare another throwaway
pointer.
Looks fine.
Post by Dustin Kirkland
There's a little code duplication with the audit_unpack_string too.
In order to eliminate that, we'd need something that ties the
AUDIT_SE_* and AUDIT_WATCH fields together as string fields. Given
that it's only four lines and only duplicated once, it might not be
worth it at this point. But if it's an indication we would want
userspace to provide, then we need to add it now.
Post by Dustin Kirkland
One last point... This patch only covers matching on process context.
You'll also need to add helpers for the new AUDIT_SE_* fields to be
used for rule listing and rule comparison.

Regards,
Amy

Continue reading on narkive:
Loading...