10:12 a.m.
Add SELinux context support to AUDIT target (2nd revision). Typical (raw auditd) output
after applying this patch would be:
type=NETFILTER_PKT msg=audit(1305852240.082:31012): action=0 hook=1 len=52 inif=?
outif=eth0 saddr=10.1.1.7 daddr=10.1.2.1 ipid=16312 proto=6 sport=56150 dport=22
obj=system_u:object_r:ssh_packet_t:s0
type=NETFILTER_PKT msg=audit(1306772064.079:56): action=0 hook=3 len=48 inif=eth0 outif=?
smac=00:05:5d:7c:27:0b dmac=00:02:b3:0a:7f:81 macproto=0x0800 saddr=10.1.2.1
daddr=10.1.1.7 ipid=462 proto=6 sport=3561 dport=22 obj=system_u:object_r:ssh_packet_t:s0
Signed-off-by: Mr Dash Four <mr.dash.four(a)googlemail.com>
---
net/netfilter/xt_AUDIT.c | 15 +++++++++++++++
1 files changed, 15 insertions(+), 0 deletions(-)
diff --git a/net/netfilter/xt_AUDIT.c b/net/netfilter/xt_AUDIT.c
index 363a99e..616cadc 100644
--- a/net/netfilter/xt_AUDIT.c
+++ b/net/netfilter/xt_AUDIT.c
@@ -20,6 +20,9 @@
#include <linux/netfilter/x_tables.h>
#include <linux/netfilter/xt_AUDIT.h>
#include <linux/netfilter_bridge/ebtables.h>
+#ifdef CONFIG_NF_CONNTRACK_SECMARK
+#include <linux/security.h>
+#endif
#include <net/ipv6.h>
#include <net/ip.h>
@@ -122,6 +125,10 @@ audit_tg(struct sk_buff *skb, const struct xt_action_param *par)
{
const struct xt_audit_info *info = par->targinfo;
struct audit_buffer *ab;
+#ifdef CONFIG_NF_CONNTRACK_SECMARK
+ u32 len;
+ char *secctx;
+#endif
ab = audit_log_start(NULL, GFP_ATOMIC, AUDIT_NETFILTER_PKT);
if (ab == NULL)
@@ -163,6 +170,14 @@ audit_tg(struct sk_buff *skb, const struct xt_action_param *par)
break;
}
+#ifdef CONFIG_NF_CONNTRACK_SECMARK
+ if (skb->secmark)
+ if (!security_secid_to_secctx(skb->secmark, &secctx, &len)) {
+ audit_log_format(ab, " obj=%s", secctx);
+ security_release_secctx(secctx, len);
+ }
+#endif
+
audit_log_end(ab);
errout:
--
1.7.3.4
6:06 p.m.
On 04/06/11 17:12, Mr Dash Four wrote:
Add SELinux context support to AUDIT target (2nd revision). Typical
(raw auditd) output after applying this patch would be:
type=NETFILTER_PKT msg=audit(1305852240.082:31012): action=0 hook=1 len=52 inif=?
outif=eth0 saddr=10.1.1.7 daddr=10.1.2.1 ipid=16312 proto=6 sport=56150 dport=22
obj=system_u:object_r:ssh_packet_t:s0
type=NETFILTER_PKT msg=audit(1306772064.079:56): action=0 hook=3 len=48 inif=eth0 outif=?
smac=00:05:5d:7c:27:0b dmac=00:02:b3:0a:7f:81 macproto=0x0800 saddr=10.1.2.1
daddr=10.1.1.7 ipid=462 proto=6 sport=3561 dport=22 obj=system_u:object_r:ssh_packet_t:s0
Signed-off-by: Mr Dash Four <mr.dash.four(a)googlemail.com>
---
net/netfilter/xt_AUDIT.c | 15 +++++++++++++++
1 files changed, 15 insertions(+), 0 deletions(-)
diff --git a/net/netfilter/xt_AUDIT.c b/net/netfilter/xt_AUDIT.c
index 363a99e..616cadc 100644
--- a/net/netfilter/xt_AUDIT.c
+++ b/net/netfilter/xt_AUDIT.c
@@ -20,6 +20,9 @@
#include <linux/netfilter/x_tables.h>
#include <linux/netfilter/xt_AUDIT.h>
#include <linux/netfilter_bridge/ebtables.h>
+#ifdef CONFIG_NF_CONNTRACK_SECMARK
+#include <linux/security.h>
+#endif
#include <net/ipv6.h>
#include <net/ip.h>
@@ -122,6 +125,10 @@ audit_tg(struct sk_buff *skb, const struct xt_action_param *par)
{
const struct xt_audit_info *info = par->targinfo;
struct audit_buffer *ab;
+#ifdef CONFIG_NF_CONNTRACK_SECMARK
+ u32 len;
+ char *secctx;
+#endif
ab = audit_log_start(NULL, GFP_ATOMIC, AUDIT_NETFILTER_PKT);
if (ab == NULL)
@@ -163,6 +170,14 @@ audit_tg(struct sk_buff *skb, const struct xt_action_param *par)
break;
}
+#ifdef CONFIG_NF_CONNTRACK_SECMARK
+ if (skb->secmark)
Minor nitpick. This 'if' needs one {
+ if (!security_secid_to_secctx(skb->secmark, &secctx,
&len)) {
+ audit_log_format(ab, " obj=%s", secctx);
+ security_release_secctx(secctx, len);
+ }
}
3:18 a.m.
> Is this a style-type requirement I wasn't aware of? Because
from a
> syntax point of view the left/right braces aren't necessary.
>
Aware of it, it's just coding style.
Thanks for pointing it out (now I know)! Would you like me to resubmit
the patch with the 2 braces included or are you happy with what I have
provided?
4:12 a.m.
On 07/06/11 10:18, Mr Dash Four wrote:
>> Is this a style-type requirement I wasn't aware of? Because from a
>> syntax point of view the left/right braces aren't necessary.
>>
>
> Aware of it, it's just coding style.
>
Thanks for pointing it out (now I know)! Would you like me to resubmit
the patch with the 2 braces included or are you happy with what I have
provided?
resubmit please.
5:32 a.m.
New subject: [PATCH 3rd revision] Add SELinux context support to AUDIT target
Add SELinux context support to AUDIT target - 3rd revision (style-type changes made *only*
since 2nd revision of this patch). Typical (raw auditd) output after applying this patch
would be:
type=NETFILTER_PKT msg=audit(1305852240.082:31012): action=0 hook=1 len=52 inif=?
outif=eth0 saddr=10.1.1.7 daddr=10.1.2.1 ipid=16312 proto=6 sport=56150 dport=22
obj=system_u:object_r:ssh_packet_t:s0
type=NETFILTER_PKT msg=audit(1306772064.079:56): action=0 hook=3 len=48 inif=eth0 outif=?
smac=00:05:5d:7c:27:0b dmac=00:02:b3:0a:7f:81 macproto=0x0800 saddr=10.1.2.1
daddr=10.1.1.7 ipid=462 proto=6 sport=3561 dport=22 obj=system_u:object_r:ssh_packet_t:s0
Signed-off-by: Mr Dash Four <mr.dash.four(a)googlemail.com>
---
net/netfilter/xt_AUDIT.c | 16 ++++++++++++++++
1 files changed, 16 insertions(+), 0 deletions(-)
diff --git a/net/netfilter/xt_AUDIT.c b/net/netfilter/xt_AUDIT.c
index 363a99e..26933ef 100644
--- a/net/netfilter/xt_AUDIT.c
+++ b/net/netfilter/xt_AUDIT.c
@@ -20,6 +20,9 @@
#include <linux/netfilter/x_tables.h>
#include <linux/netfilter/xt_AUDIT.h>
#include <linux/netfilter_bridge/ebtables.h>
+#ifdef CONFIG_NF_CONNTRACK_SECMARK
+#include <linux/security.h>
+#endif
#include <net/ipv6.h>
#include <net/ip.h>
@@ -122,6 +125,10 @@ audit_tg(struct sk_buff *skb, const struct xt_action_param *par)
{
const struct xt_audit_info *info = par->targinfo;
struct audit_buffer *ab;
+#ifdef CONFIG_NF_CONNTRACK_SECMARK
+ u32 len;
+ char *secctx;
+#endif
ab = audit_log_start(NULL, GFP_ATOMIC, AUDIT_NETFILTER_PKT);
if (ab == NULL)
@@ -163,6 +170,15 @@ audit_tg(struct sk_buff *skb, const struct xt_action_param *par)
break;
}
+#ifdef CONFIG_NF_CONNTRACK_SECMARK
+ if (skb->secmark) {
+ if (!security_secid_to_secctx(skb->secmark, &secctx, &len)) {
+ audit_log_format(ab, " obj=%s", secctx);
+ security_release_secctx(secctx, len);
+ }
+ }
+#endif
+
audit_log_end(ab);
errout:
--
1.7.3.4
9:49 a.m.
New subject: [PATCH 3rd revision] Add SELinux context support to AUDIT target
On Tuesday, June 07, 2011 06:32:35 AM Mr Dash Four wrote:
Add SELinux context support to AUDIT target - 3rd revision
(style-type
changes made *only* since 2nd revision of this patch). Typical (raw
auditd) output after applying this patch would be:
<snip>
@@ -163,6 +170,15 @@ audit_tg(struct sk_buff *skb, const struct
xt_action_param *par) break;
}
+#ifdef CONFIG_NF_CONNTRACK_SECMARK
+ if (skb->secmark) {
+ if (!security_secid_to_secctx(skb->secmark, &secctx, &len)) {
+ audit_log_format(ab, " obj=%s", secctx);
+ security_release_secctx(secctx, len);
+ }
else
audit_log_format(ab, " osid=%u", skb->secmark);
_All_ audit code records the number on a failed conversion.
-Steve
+ }
+#endif
+
audit_log_end(ab);
errout:
11:12 a.m.
New subject: [PATCH 3rd revision] Add SELinux context support to AUDIT target
Mr Dash Four wrote:
Logging the internal numerical representation of secctx is, as I have
already stated about 3 times by now, exposing internal
(private-to-the-kernel-only) information to userspace. That cannot be
allowed.
Besides, this numerical representation isn't reliable - these numbers
are dynamic and can change - another reason why they should not be
allowed to be present in the audit log. What happens if I make changes
to my security policy and then run ausearch/aureport? I am either
going to see different (wrong!) context reported if ausearch/aureport
attempts to "convert" those numbers into SELinux context, or, I am
going to see meaningless numbers. Either way, useless or misleading
information is going to be reported and we don't want that, do we?
else
audit_log_format(ab, " osid=%u", skb->secmark);
_All_ audit code records the number on a failed conversion.
I am assuming you haven't read the above. Show me one good reason why I
should alter my patch to include that abomination of yours?
12:14 p.m.
New subject: [PATCH 3rd revision] Add SELinux context support to AUDIT target
On Wednesday, June 08, 2011 12:12:39 PM Mr Dash Four wrote:
Mr Dash Four wrote:
> Logging the internal numerical representation of secctx is, as I have
> already stated about 3 times by now, exposing internal
> (private-to-the-kernel-only) information to userspace. That cannot be
> allowed.
It doesn't matter if its private. If its important enough to log to the audit system,
we can't let something like this slide.
> Besides, this numerical representation isn't reliable -
these numbers
> are dynamic and can change - another reason why they should not be
> allowed to be present in the audit log.
Doesn't matter. Its the event that we want and all its attributes. If the label is not
correct, how else are we going to know? Do the LSMs generate an audit event saying
they couldn't lookup a label?
> What happens if I make changes to my security policy and then
run
> ausearch/aureport?
Nothing.
> I am either going to see different (wrong!) context reported if
ausearch/aureport
> attempts to "convert" those numbers into SELinux context, or, I am
> going to see meaningless numbers. Either way, useless or misleading
> information is going to be reported and we don't want that, do we?
Yes, we do.
> else
>
> audit_log_format(ab, " osid=%u", skb->secmark);
>
> _All_ audit code records the number on a failed conversion.
I am assuming you haven't read the above. Show me one good reason why I
should alter my patch to include that abomination of yours?
Because _all_ logging of object labels in the audit system do this. You are asking to
add information to the audit system. I am telling you how its done everywhere else so
that you patch is consistent.
-Steve
1:04 p.m.
New subject: [PATCH 3rd revision] Add SELinux context support to AUDIT target
It doesn't matter if its private. If its important enough to log
to the audit system,
we can't let something like this slide.
Oh, yes it does! For you may be it doesn't, but that doesn't necessarily
mean that applies to everyone else.
>> Besides, this numerical representation isn't reliable -
these numbers
>> are dynamic and can change - another reason why they should not be
>> allowed to be present in the audit log.
>>
Doesn't matter.
Oh, yes it does! If you are content or used to putting heaps of
useless
and meaningless "data" in the audit logs (and lets be frank, that is
what this number is to the admins or other people who will be looking at
those logs), then you may wish to submit another patch to the
netfilter-dev list for review with whatever takes your fancy in it, and
see how far does that take you!
I won't be doing that as I am not at all convinced that the number you
are asking me to add is anything more than a piece of useless junk!
Its the event that we want and all its attributes. If the label is
not
correct, how else are we going to know?
This isn't a label! It is, for all
intents and purposes, a random number
which may have been used to point to something.
How many times would you like me, or other people on this list, to
repeat that until there is a remote chance that you will finally get it?
>> What happens if I make changes to my security policy and then
run
>> ausearch/aureport?
>>
Nothing.
Absolute rubbish and what's worse is that you know it!
>> I am either going to see different (wrong!) context reported
if ausearch/aureport
>> attempts to "convert" those numbers into SELinux context, or, I am
>> going to see meaningless numbers. Either way, useless or misleading
>> information is going to be reported and we don't want that, do we?
>>
Yes, we do.
You do what?! Put a complete rubbish in the audit logs? May be you do,
but I don't.
Again, I am not contributing to something which places misleading and/or
useless information in the audit logs. It is not desired and unless I am
convinced otherwise, there is little chance of me altering my patch, so
it stays as I originally submitted it.
1:13 p.m.
New subject: [PATCH 3rd revision] Add SELinux context support to AUDIT target
On 6/8/2011 7:49 AM, Steve Grubb wrote:
On Tuesday, June 07, 2011 06:32:35 AM Mr Dash Four wrote:
> Add SELinux context support to AUDIT target - 3rd revision (style-type
> changes made *only* since 2nd revision of this patch). Typical (raw
> auditd) output after applying this patch would be:
<snip>
> @@ -163,6 +170,15 @@ audit_tg(struct sk_buff *skb, const struct
> xt_action_param *par) break;
> }
>
> +#ifdef CONFIG_NF_CONNTRACK_SECMARK
> + if (skb->secmark) {
> + if (!security_secid_to_secctx(skb->secmark, &secctx, &len)) {
> + audit_log_format(ab, " obj=%s", secctx);
> + security_release_secctx(secctx, len);
> + }
else
audit_log_format(ab, " osid=%u", skb->secmark);
_All_ audit code records the number on a failed conversion.
But it really shouldn't. An unconvertible secid is indicative
of a serious, unrecoverable failure within the LSM. It's every
bit as bad as an invalid pointer.
-Steve
> + }
> +#endif
> +
> audit_log_end(ab);
>
> errout:
--
Linux-audit mailing list
Linux-audit(a)redhat.com
https://www.redhat.com/mailman/listinfo/linux-audit
1:33 p.m.
New subject: [PATCH 3rd revision] Add SELinux context support to AUDIT target
On Wed, Jun 8, 2011 at 2:13 PM, Casey Schaufler <casey(a)schaufler-ca.com> wrote:
On 6/8/2011 7:49 AM, Steve Grubb wrote:
> On Tuesday, June 07, 2011 06:32:35 AM Mr Dash Four wrote:
>> Add SELinux context support to AUDIT target - 3rd revision (style-type
>> changes made *only* since 2nd revision of this patch). Typical (raw
>> auditd) output after applying this patch would be:
> <snip>
>
>> @@ -163,6 +170,15 @@ audit_tg(struct sk_buff *skb, const struct
>> xt_action_param *par) break;
>> }
>>
>> +#ifdef CONFIG_NF_CONNTRACK_SECMARK
>> + if (skb->secmark) {
>> + if (!security_secid_to_secctx(skb->secmark, &secctx,
&len)) {
>> + audit_log_format(ab, " obj=%s", secctx);
>> + security_release_secctx(secctx, len);
>> + }
> else
> audit_log_format(ab, " osid=%u", skb->secmark);
>
> _All_ audit code records the number on a failed conversion.
But it really shouldn't. An unconvertible secid is indicative
of a serious, unrecoverable failure within the LSM. It's every
bit as bad as an invalid pointer.
I'm with Casey and Mr Dash Four. The right way to do this is to make
a helper function in the audit code which takes care of the LSM calls
(rather than making LSM calls up here). The function should throw an
error of one sort or another if it cannot convert the context. My
suggestion:
int audit_log_secctx(struct auditbuffer *ab, u32 secid)
{
int len, rc;
char *ctx;
rc = security_secid_to_secctx(sid, &ctx, &len);
if (rc) {
audit_panic("Cannot convert secid to context");
} else {
audit_log_format(ab, " subj=%s", ctx);
security_release_secctx(ctx, len);
}
return rc;
}
Such a function could be used a couple of places in the audit code itself.
-Eric
2 p.m.
New subject: [PATCH 3rd revision] Add SELinux context support to AUDIT target
int audit_log_secctx(struct auditbuffer *ab, u32 secid)
{
int len, rc;
char *ctx;
rc = security_secid_to_secctx(sid, &ctx, &len);
if (rc) {
audit_panic("Cannot convert secid to context");
} else {
audit_log_format(ab, " subj=%s", ctx);
security_release_secctx(ctx, len);
}
return rc;
}
Such a function could be used a couple of places in the audit code itself.
My view on this is that LSM error-handling should be part of LSM.
I presume security_secid_to_secctx is going to be called from quite a
few places (well, I know of at least two now and they have nothing to do
with the LSM) and in my opinion it would be better if that error
handling, if adopted, is implemented within the function itself -
whether by calling another function, like the one you proposed above, or
as part of the secctx retrieval - this could be open to interpretation,
but the point I am trying to make is that whichever code
security_secid_to_secctx is invoked from shouldn't be involved in
reporting/handling (internal LSM) errors at all.
I think I made that point in my previous post, but just wanted to make
sure that is the case.
2:08 p.m.
New subject: [PATCH 3rd revision] Add SELinux context support to AUDIT target
On Wed, Jun 8, 2011 at 3:00 PM, Mr Dash Four
<mr.dash.four(a)googlemail.com> wrote:
> int audit_log_secctx(struct auditbuffer *ab, u32 secid)
> {
> int len, rc;
> char *ctx;
>
> rc = security_secid_to_secctx(sid, &ctx, &len);
> if (rc) {
> audit_panic("Cannot convert secid to context");
> } else {
> audit_log_format(ab, " subj=%s", ctx);
> security_release_secctx(ctx, len);
> }
> return rc;
> }
>
> Such a function could be used a couple of places in the audit code itself.
>
My view on this is that LSM error-handling should be part of LSM.
I presume security_secid_to_secctx is going to be called from quite a few
places (well, I know of at least two now and they have nothing to do with
the LSM) and in my opinion it would be better if that error handling, if
adopted, is implemented within the function itself - whether by calling
another function, like the one you proposed above, or as part of the secctx
retrieval - this could be open to interpretation, but the point I am trying
to make is that whichever code security_secid_to_secctx is invoked from
shouldn't be involved in reporting/handling (internal LSM) errors at all.
I think I made that point in my previous post, but just wanted to make sure
that is the case.
The LSM might report and error. It's up to the caller to figure out
how to deal with that error. In this case we want to use the audit
system so it's up to the audit system how to handle that error. This
helper function says the audit system should log it if it work and
should audit_panic() if it doesn't. audit_panic() will just call
printk for most people and can actually panic the box for nutters who
really care. In this way we always log the information and if we
don't it's up to audit how audit handles it's inability to log info.
It's not netfilter's job to handle the error. It's not the LSMs job
to know how it's caller wants to handle the error. Audit is who has
special requirements and the code to handle the error should be in
audit code. (Maybe it wasn't clear, but I think this function should
go in kernel/audit.c, not the netfilter code. The netfilter code
should call this helper function.
-Eric
2:14 p.m.
New subject: [PATCH 3rd revision] Add SELinux context support to AUDIT target
The LSM might report and error. It's up to the caller to figure
out
how to deal with that error. In this case we want to use the audit
system so it's up to the audit system how to handle that error. This
helper function says the audit system should log it if it work and
should audit_panic() if it doesn't. audit_panic() will just call
printk for most people and can actually panic the box for nutters who
really care. In this way we always log the information and if we
don't it's up to audit how audit handles it's inability to log info.
It's not netfilter's job to handle the error. It's not the LSMs job
to know how it's caller wants to handle the error. Audit is who has
special requirements and the code to handle the error should be in
audit code. (Maybe it wasn't clear, but I think this function should
go in kernel/audit.c, not the netfilter code. The netfilter code
should call this helper function.
Yeah, that's fair enough, though from what I remember
security_secid_to_secctx already returns a 'yes'/'no' result (I am
talking from the top of my head here as I am away at present and can't
check it out to be certain), indicating whether the conversion was
successful or not.
2:28 p.m.
New subject: [PATCH 3rd revision] Add SELinux context support to AUDIT target
On Wednesday, June 08, 2011 03:08:38 PM Eric Paris wrote:
On Wed, Jun 8, 2011 at 3:00 PM, Mr Dash Four
<mr.dash.four(a)googlemail.com> wrote:
>> int audit_log_secctx(struct auditbuffer *ab, u32 secid)
>> {
>> int len, rc;
>> char *ctx;
>>
>> rc = security_secid_to_secctx(sid, &ctx, &len);
>> if (rc) {
>> audit_panic("Cannot convert secid to context");
>> } else {
>> audit_log_format(ab, " subj=%s", ctx);
>> security_release_secctx(ctx, len);
>> }
>> return rc;
>> }
>>
>> Such a function could be used a couple of places in the audit code
>> itself.
>
> My view on this is that LSM error-handling should be part of LSM.
>
> I presume security_secid_to_secctx is going to be called from quite a few
> places (well, I know of at least two now and they have nothing to do with
> the LSM) and in my opinion it would be better if that error handling, if
> adopted, is implemented within the function itself - whether by calling
> another function, like the one you proposed above, or as part of the
> secctx retrieval - this could be open to interpretation, but the point I
> am trying to make is that whichever code security_secid_to_secctx is
> invoked from shouldn't be involved in reporting/handling (internal LSM)
> errors at all.
>
> I think I made that point in my previous post, but just wanted to make
> sure that is the case.
The LSM might report and error. It's up to the caller to figure out
how to deal with that error. In this case we want to use the audit
system so it's up to the audit system how to handle that error.
We are happy recording the failed number. Its the LSM people that say nuke the system.
So, I would put that in security_secid_to_secctx() so that everyone knows whose
requirements it was to do the nuclear option.
-Steve
2:39 p.m.
New subject: [PATCH 3rd revision] Add SELinux context support to AUDIT target
On Wed, Jun 8, 2011 at 3:28 PM, Steve Grubb <sgrubb(a)redhat.com> wrote:
On Wednesday, June 08, 2011 03:08:38 PM Eric Paris wrote:
> On Wed, Jun 8, 2011 at 3:00 PM, Mr Dash Four
>
> <mr.dash.four(a)googlemail.com> wrote:
> >> int audit_log_secctx(struct auditbuffer *ab, u32 secid)
> >> {
> >> int len, rc;
> >> char *ctx;
> >>
> >> rc = security_secid_to_secctx(sid, &ctx, &len);
> >> if (rc) {
> >> audit_panic("Cannot convert secid to context");
> >> } else {
> >> audit_log_format(ab, " subj=%s", ctx);
> >> security_release_secctx(ctx, len);
> >> }
> >> return rc;
> >> }
> >>
> >> Such a function could be used a couple of places in the audit code
> >> itself.
> >
> > My view on this is that LSM error-handling should be part of LSM.
> >
> > I presume security_secid_to_secctx is going to be called from quite a few
> > places (well, I know of at least two now and they have nothing to do with
> > the LSM) and in my opinion it would be better if that error handling, if
> > adopted, is implemented within the function itself - whether by calling
> > another function, like the one you proposed above, or as part of the
> > secctx retrieval - this could be open to interpretation, but the point I
> > am trying to make is that whichever code security_secid_to_secctx is
> > invoked from shouldn't be involved in reporting/handling (internal LSM)
> > errors at all.
> >
> > I think I made that point in my previous post, but just wanted to make
> > sure that is the case.
>
> The LSM might report and error. It's up to the caller to figure out
> how to deal with that error. In this case we want to use the audit
> system so it's up to the audit system how to handle that error.
We are happy recording the failed number. Its the LSM people that say nuke the system.
So, I would put that in security_secid_to_secctx() so that everyone knows whose
requirements it was to do the nuclear option.
If the number meets your requirements then the requirements are total
shit. The number has NO relation to the label on the object as
understood by the system. If audit has a requirement to always log
the label or call audit_panic(), its only option is to call
audit_panic().
Exposing secids and internal representations of information to
userspace is always wrong. Full stop.
I'd be willing to take a patch which caused security_secid_to_secctx()
to BUG() if it got an invalid secid. But on ENOMEM I'm going to just
push the error back up the stack. In that case audit has to decide
how to handle the situation. That secid is NOT the label associated
with the object and printing it to userspace is meaningless garbage.
Just because audit did it wrong yesterday doesn't mean I'm going to
ACK more patches that do it wrong tomorrow. I don't care what some
arbitrary and obviously poorly thought out requirement document says.
-Eric
1:36 p.m.
New subject: [PATCH 3rd revision] Add SELinux context support to AUDIT target
On Wednesday, June 08, 2011 02:13:18 PM Casey Schaufler wrote:
On 6/8/2011 7:49 AM, Steve Grubb wrote:
> On Tuesday, June 07, 2011 06:32:35 AM Mr Dash Four wrote:
>> Add SELinux context support to AUDIT target - 3rd revision (style-type
>> changes made *only* since 2nd revision of this patch). Typical (raw
>
>> auditd) output after applying this patch would be:
> <snip>
>
>> @@ -163,6 +170,15 @@ audit_tg(struct sk_buff *skb, const struct
>> xt_action_param *par) break;
>>
>> }
>>
>> +#ifdef CONFIG_NF_CONNTRACK_SECMARK
>> + if (skb->secmark) {
>> + if (!security_secid_to_secctx(skb->secmark, &secctx, &len)) {
>> + audit_log_format(ab, " obj=%s", secctx);
>> + security_release_secctx(secctx, len);
>> + }
>
> else
>
> audit_log_format(ab, " osid=%u", skb->secmark);
>
> _All_ audit code records the number on a failed conversion.
But it really shouldn't. An unconvertible secid is indicative
of a serious, unrecoverable failure within the LSM. It's every
bit as bad as an invalid pointer.
I agree with that point. But do the LSM's panic the kernel or send an audit event that
they could not convert something? Besides my patch to the patch, how is this error
preserved in the audit trail?
-Steve
1:45 p.m.
New subject: [PATCH 3rd revision] Add SELinux context support to AUDIT target
how is this error preserved in the audit trail?
Look at my patch again - if the secctx cannot be retrieved, either
because a) it does not exists; or b) because of internal error or
otherwise, then it is not logged in the audit log as part of the
NETFILTER_PKT message (the fact there is internal LSM error has
absolutely nothing to do with a netfilter packet!).
If, internally (upon calling security_secid_to_secctx) there is a
decision to handle that *internal* error in one way or another so be it,
but as far as my patch goes - there is no secctx if that function
returns nothing and I think that is the right think to do.
7:14 a.m.
On Saturday, June 04, 2011 11:12:04 AM Mr Dash Four wrote:
Add SELinux context support to AUDIT target (2nd revision). Typical
(raw
auditd) output after applying this patch would be:
type=NETFILTER_PKT msg=audit(1305852240.082:31012): action=0 hook=1 len=52
inif=? outif=eth0 saddr=10.1.1.7 daddr=10.1.2.1 ipid=16312 proto=6
sport=56150 dport=22 obj=system_u:object_r:ssh_packet_t:s0
type=NETFILTER_PKT msg=audit(1306772064.079:56): action=0 hook=3 len=48
inif=eth0 outif=? smac=00:05:5d:7c:27:0b dmac=00:02:b3:0a:7f:81
macproto=0x0800 saddr=10.1.2.1 daddr=10.1.1.7 ipid=462 proto=6 sport=3561
dport=22 obj=system_u:object_r:ssh_packet_t:s0
Signed-off-by: Mr Dash Four <mr.dash.four(a)googlemail.com>
---
net/netfilter/xt_AUDIT.c | 15 +++++++++++++++
1 files changed, 15 insertions(+), 0 deletions(-)
diff --git a/net/netfilter/xt_AUDIT.c b/net/netfilter/xt_AUDIT.c
index 363a99e..616cadc 100644
--- a/net/netfilter/xt_AUDIT.c
+++ b/net/netfilter/xt_AUDIT.c
@@ -20,6 +20,9 @@
#include <linux/netfilter/x_tables.h>
#include <linux/netfilter/xt_AUDIT.h>
#include <linux/netfilter_bridge/ebtables.h>
+#ifdef CONFIG_NF_CONNTRACK_SECMARK
+#include <linux/security.h>
+#endif
#include <net/ipv6.h>
#include <net/ip.h>
@@ -122,6 +125,10 @@ audit_tg(struct sk_buff *skb, const struct
xt_action_param *par) {
const struct xt_audit_info *info = par->targinfo;
struct audit_buffer *ab;
+#ifdef CONFIG_NF_CONNTRACK_SECMARK
+ u32 len;
+ char *secctx;
+#endif
ab = audit_log_start(NULL, GFP_ATOMIC, AUDIT_NETFILTER_PKT);
if (ab == NULL)
@@ -163,6 +170,14 @@ audit_tg(struct sk_buff *skb, const struct
xt_action_param *par) break;
}
+#ifdef CONFIG_NF_CONNTRACK_SECMARK
+ if (skb->secmark)
+ if (!security_secid_to_secctx(skb->secmark, &secctx, &len)) {
+ audit_log_format(ab, " obj=%s", secctx);
+ security_release_secctx(secctx, len);
+ }
Normally there would be an else here to do something like
audit_log_format(ab, " osid=%u", skb->secmark);
so that its recorded numerically if the context could not be looked up.
+#endif
+
audit_log_end(ab);
errout:
7:25 a.m.
Normally there would be an else here to do something like
audit_log_format(ab, " osid=%u", skb->secmark);
so that its recorded numerically if the context could not be looked up.
I disagree! That approach was dropped long ago when the secctx was first
introduced to prevent kernel information leaking into userspace (Eric
would know more about this as he designed that aspect of it a couple of
months ago). So the secctx is either present (and retrievable!) or not
present from the (xt_)audit point of view. For more information see
net/netfilter/nf_conntrack_standalone.c in the current nf-next tree.
In other words, no else is necessary.
7:30 a.m.
On Monday, June 06, 2011 08:25:56 AM Mr Dash Four wrote:
> Normally there would be an else here to do something like
> audit_log_format(ab, " osid=%u", skb->secmark);
> so that its recorded numerically if the context could not be looked up.
I disagree! That approach was dropped long ago when the secctx was first
introduced to prevent kernel information leaking into userspace (Eric
would know more about this as he designed that aspect of it a couple of
months ago).
This is not any more leak than leaking the context string to user space as this patch
attempts to do. The rest of the audit code does log the numeric representation when
text fails.
-Steve
7:42 a.m.
This is not any more leak than leaking the context string to user
space as this patch
attempts to do. The rest of the audit code does log the numeric representation when
text fails.
There is no "leak" when the secctx is recorded in the audit log - it
is
supposed to be there, if present (and retrievable). As for exposing the
(internal) numerical representation of the secctx - this was discussed
previously and the approach you are suggesting was dropped. To quote
Eric on this very issue "[It] exports the internal secid to userspace.
These are dynamic, can change on lsm changes, and have no meaning in
userspace. We should instead be sending lsm contexts to userspace instead.".
7:53 a.m.
On Monday, June 06, 2011 08:42:15 AM Mr Dash Four wrote:
> This is not any more leak than leaking the context string to
user space
> as this patch attempts to do. The rest of the audit code does log the
> numeric representation when text fails.
There is no "leak" when the secctx is recorded in the audit log - it is
supposed to be there, if present (and retrievable).
Exactly my point. There is no leak if its text or numeric.
As for exposing the (internal) numerical representation of the secctx
- this was
discussed previously and the approach you are suggesting was dropped. To quote
Eric on this very issue "[It] exports the internal secid to userspace.
These are dynamic, can change on lsm changes, and have no meaning in
userspace. We should instead be sending lsm contexts to userspace
instead.".
Doesn't matter. The requirements of the protection profiles say to log the
object's
label. It does not care if its text or numeric. It also does not say sometimes or only
when its convenient. :) Its either important enough to log even if text conversion
fails or its not important enough to log at all.
-Steve
8:10 a.m.
Exactly my point. There is no leak if its text or numeric.
No, there is no leak if it is a text, but there *is* a leak if it is a
numeric. I think I've made that quite clear.
> As for exposing the (internal) numerical representation of the
secctx - this was
> discussed previously and the approach you are suggesting was dropped. To quote
> Eric on this very issue "[It] exports the internal secid to userspace.
> These are dynamic, can change on lsm changes, and have no meaning in
> userspace. We should instead be sending lsm contexts to userspace
> instead.".
>
Doesn't matter. The requirements of the protection profiles say to log the
object's
label.
It does not care if its text or numeric. It also does not say sometimes or only
when its convenient. :)
Again, I disagree. Logging the internal numerical
representation of
secctx is, as I have already stated about 3 times by now, exposing
internal (private-to-the-kernel-only) information to userspace. That
cannot be allowed.
Besides, this numerical representation isn't reliable - these numbers
are dynamic and can change - another reason why they should not be
allowed to be present in the audit log. What happens if I make changes
to my security policy and then run ausearch/aureport? I am either going
to see different (wrong!) context reported if ausearch/aureport attempts
to "convert" those numbers into SELinux context, or, I am going to see
meaningless numbers. Either way, useless or misleading information is
going to be reported and we don't want that, do we?
Its either important enough to log even if text conversion
fails or its not important enough to log at all.
That is exactly what the current patch does - if secctx is present (and
retrievable) it is logged, if not, then it isn't. Quite simple really.
6:22 p.m.
On 06/06/11 15:10, Mr Dash Four wrote:
> Exactly my point. There is no leak if its text or numeric.
>
No, there is no leak if it is a text, but there *is* a leak if it is a
numeric. I think I've made that quite clear.
We don't use numeric secmark anymore in nf_conntrack. Not very familiar
with SELinux, but I remember that the convention was not to provide
internal numeric values.
7:59 p.m.
On Monday, June 06, 2011 07:22:43 PM Pablo Neira Ayuso wrote:
On 06/06/11 15:10, Mr Dash Four wrote:
>> Exactly my point. There is no leak if its text or numeric.
>
> No, there is no leak if it is a text, but there *is* a leak if it is a
> numeric. I think I've made that quite clear.
We don't use numeric secmark anymore in nf_conntrack. Not very familiar
with SELinux, but I remember that the convention was not to provide
internal numeric values.
All of the audit system records the numbers if conversion fails. We want it as
forensic evidence or troubleshooting information as the case may be.
-Steve
8:23 p.m.
On 6/6/2011 5:59 PM, Steve Grubb wrote:
On Monday, June 06, 2011 07:22:43 PM Pablo Neira Ayuso wrote:
> On 06/06/11 15:10, Mr Dash Four wrote:
>>> Exactly my point. There is no leak if its text or numeric.
>> No, there is no leak if it is a text, but there *is* a leak if it is a
>> numeric. I think I've made that quite clear.
> We don't use numeric secmark anymore in nf_conntrack. Not very familiar
> with SELinux, but I remember that the convention was not to provide
> internal numeric values.
All of the audit system records the numbers if conversion fails.
Consistency is important
We want it as
forensic evidence or troubleshooting information as the case may be.
It's completely pointless to have in the audit record. The code
ought to treat an untranslatable secmark with the same severity
as an invalid pointer. You could argue that it is oopsable. Certainly
worthy of a BUG invocation. Printing the numeric is sloppy error
handling.
-Steve
--
Linux-audit mailing list
Linux-audit(a)redhat.com
https://www.redhat.com/mailman/listinfo/linux-audit
4945
days inactive
4949
days old
linux-audit@lists.linux-audit.osci.io
28 comments
5 participants
participants (5)
-
Casey Schaufler -
Eric Paris -
Mr Dash Four -
Pablo Neira Ayuso -
Steve Grubb