[PATCH] Add SELinux context support to AUDIT target

[PATCH] Add SELinux context support to AUDIT target

[Mr Dash Four]

Add SELinux context support for AUDIT target. Typical (raw auditd) output after applying this patch would be:

type=NETFILTER_PKT msg=audit(1305852240.082:31012): action=0 hook=3 len=52 inif=? outif=eth0 subj=system_u:object_r:sshd_packet_t:s0 saddr=10.1.1.7 daddr=10.1.2.1 ipid=16312 proto=6 sport=56150 dport=22


Signed-off-by: Mr Dash Four <mr.dash.four@googlemail.com>
---
 net/netfilter/xt_AUDIT.c |   13 +++++++++++++
 1 files changed, 13 insertions(+), 0 deletions(-)

diff --git a/net/netfilter/xt_AUDIT.c b/net/netfilter/xt_AUDIT.c
index 363a99e..438f82a 100644
--- a/net/netfilter/xt_AUDIT.c
+++ b/net/netfilter/xt_AUDIT.c
@@ -20,6 +20,7 @@
 #include <linux/netfilter/x_tables.h>
 #include <linux/netfilter/xt_AUDIT.h>
 #include <linux/netfilter_bridge/ebtables.h>
+#include <linux/security.h>
 #include <net/ipv6.h>
 #include <net/ip.h>
 
@@ -122,6 +123,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)
@@ -135,6 +140,14 @@ audit_tg(struct sk_buff *skb, const struct xt_action_param *par)
 	if (skb->mark)
 		audit_log_format(ab, " mark=%#x", skb->mark);
 
+#ifdef CONFIG_NF_CONNTRACK_SECMARK
+	if (skb->secmark)
+	  	if (!security_secid_to_secctx(skb->secmark, &secctx, &len)) {
+			audit_log_format(ab, " subj=%s", secctx);
+			security_release_secctx(secctx, len);
+		}
+#endif
+
 	if (skb->dev && skb->dev->type == ARPHRD_ETHER) {
 		audit_log_format(ab, " smac=%pM dmac=%pM macproto=0x%04x",
 				 eth_hdr(skb)->h_source, eth_hdr(skb)->h_dest,
-- 
1.7.3.4

Re: [PATCH] Add SELinux context support to AUDIT target

[Pablo Neira Ayuso]

On 20/05/11 03:09, Mr Dash Four wrote:
> Add SELinux context support for AUDIT target. Typical (raw auditd) output after applying this patch would be:
> 
> type=NETFILTER_PKT msg=audit(1305852240.082:31012): action=0 hook=3 len=52 inif=? outif=eth0 subj=system_u:object_r:sshd_packet_t:s0 saddr=10.1.1.7 daddr=10.1.2.1 ipid=16312 proto=6 sport=56150 dport=22

I think this new information should be added at the end of the string.

Re: [PATCH] Add SELinux context support to AUDIT target

[Mr Dash Four]

> I think this new information should be added at the end of the string.
>   
In other words:

type=NETFILTER_PKT msg=audit(1305852240.082:31012): action=0 hook=3 
len=52 inif=? outif=eth0 saddr=10.1.1.7 daddr=10.1.2.1 ipid=16312 
proto=6 sport=56150 dport=22 subj=system_u:object_r:sshd_packet_t:s0

As I am currently discussing this very issue (adding SELinux context to 
AUDIT) on the audit mail list, it was pointed out that "subj" should 
actually be "obj" as this is an object (i.e. a packet) on which this is 
applied, so that would ultimately mean:

type=NETFILTER_PKT msg=audit(1305852240.082:31012): action=0 hook=3 
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:sshd_packet_t:s0

I also need to check as I think the order is also important, otherwise 
ausearch/aureport may skip this due to "misconfiguration".

Re: [PATCH] Add SELinux context support to AUDIT target

[Pablo Neira Ayuso]

On 26/05/11 19:03, Mr Dash Four wrote:
>> I think this new information should be added at the end of the string.
>>   
> In other words:
> 
> type=NETFILTER_PKT msg=audit(1305852240.082:31012): action=0 hook=3
> len=52 inif=? outif=eth0 saddr=10.1.1.7 daddr=10.1.2.1 ipid=16312
> proto=6 sport=56150 dport=22 subj=system_u:object_r:sshd_packet_t:s0
> 
> As I am currently discussing this very issue (adding SELinux context to
> AUDIT) on the audit mail list, it was pointed out that "subj" should
> actually be "obj" as this is an object (i.e. a packet) on which this is
> applied, so that would ultimately mean:
> 
> type=NETFILTER_PKT msg=audit(1305852240.082:31012): action=0 hook=3
> 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:sshd_packet_t:s0

OK, that's fine.

> I also need to check as I think the order is also important, otherwise
> ausearch/aureport may skip this due to "misconfiguration".

I was spotting this because we don't want to break any existing FOSS
application that parses the output. Adding things at the end seems to me
like the better way to avoid this?

So, please, make sure that we don't break anything.

[PATCH 2nd revision] Add SELinux context support to AUDIT target

[Mr Dash Four]

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@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

Re: [PATCH 2nd revision] Add SELinux context support to AUDIT target

[Pablo Neira Ayuso]

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@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);
> +		}

}

Re: [PATCH 2nd revision] Add SELinux context support to AUDIT target

[Mr Dash Four]

> Minor nitpick. This 'if' needs one {
>   
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.

Re: [PATCH 2nd revision] Add SELinux context support to AUDIT target

[Steve Grubb]

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@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:

Re: [PATCH 2nd revision] Add SELinux context support to AUDIT target

[Mr Dash Four]

> 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.

Re: [PATCH 2nd revision] Add SELinux context support to AUDIT target

[Steve Grubb]

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

Re: [PATCH 2nd revision] Add SELinux context support to AUDIT target

[Mr Dash Four]

> 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.".

Re: [PATCH 2nd revision] Add SELinux context support to AUDIT target

[Steve Grubb]

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

Re: [PATCH 2nd revision] Add SELinux context support to AUDIT target

[Mr Dash Four]

> 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.

Re: [PATCH 2nd revision] Add SELinux context support to AUDIT target

[Pablo Neira Ayuso]

On 06/06/11 14:02, Mr Dash Four wrote:
> 
>> Minor nitpick. This 'if' needs one {
>>   
> 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.

Re: [PATCH 2nd revision] Add SELinux context support to AUDIT target

[Pablo Neira Ayuso]

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.

Re: [PATCH 2nd revision] Add SELinux context support to AUDIT target

[Steve Grubb]

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

Re: [PATCH 2nd revision] Add SELinux context support to AUDIT target

[Casey Schaufler]

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@redhat.com
> https://www.redhat.com/mailman/listinfo/linux-audit
>

Re: [PATCH 2nd revision] Add SELinux context support to AUDIT target

[Mr Dash Four]

>> 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?

Re: [PATCH 2nd revision] Add SELinux context support to AUDIT target

[Pablo Neira Ayuso]

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.

[PATCH 3rd revision] Add SELinux context support to AUDIT target

[Mr Dash Four]

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@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

Re: [PATCH 3rd revision] Add SELinux context support to AUDIT target

[Steve Grubb]

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:

Re: [PATCH 3rd revision] Add SELinux context support to AUDIT target

[Mr Dash Four]

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?

Re: [PATCH 3rd revision] Add SELinux context support to AUDIT target

[Steve Grubb]

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

Re: [PATCH 3rd revision] Add SELinux context support to AUDIT target

[Mr Dash Four]

> 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.

Re: [PATCH 3rd revision] Add SELinux context support to AUDIT target

[Casey Schaufler]

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@redhat.com
> https://www.redhat.com/mailman/listinfo/linux-audit
>

Re: [PATCH 3rd revision] Add SELinux context support to AUDIT target

[Eric Paris]

On Wed, Jun 8, 2011 at 2:13 PM, Casey Schaufler <casey@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
--
To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 3rd revision] Add SELinux context support to AUDIT target

[Steve Grubb]

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

Re: [PATCH 3rd revision] Add SELinux context support to AUDIT target

[Mr Dash Four]

> 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.

Re: [PATCH 3rd revision] Add SELinux context support to AUDIT target

[Mr Dash Four]

> 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.

Re: [PATCH 3rd revision] Add SELinux context support to AUDIT target

[Eric Paris]

On Wed, Jun 8, 2011 at 3:00 PM, Mr Dash Four
<mr.dash.four@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
--
To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 3rd revision] Add SELinux context support to AUDIT target

[Mr Dash Four]

> 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.

Re: [PATCH 3rd revision] Add SELinux context support to AUDIT target

[Steve Grubb]

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@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

Re: [PATCH 3rd revision] Add SELinux context support to AUDIT target

[Eric Paris]

On Wed, Jun 8, 2011 at 3:28 PM, Steve Grubb <sgrubb@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@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
--
To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 3rd revision] Add SELinux context support to AUDIT target

[Patrick McHardy]

On 08.06.2011 21:39, Eric Paris wrote:
> On Wed, Jun 8, 2011 at 3:28 PM, Steve Grubb <sgrubb@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@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.

Just to make sure, so the conclusion is that the patch is fine as
it is and anything related to unconvertible secids will be handled
by SELinux internally?

Re: [PATCH 3rd revision] Add SELinux context support to AUDIT target

[Eric Paris]

On Thu, Jun 9, 2011 at 8:28 AM, Patrick McHardy <kaber@trash.net> wrote:
> On 08.06.2011 21:39, Eric Paris wrote:
>> On Wed, Jun 8, 2011 at 3:28 PM, Steve Grubb <sgrubb@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@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.
>
> Just to make sure, so the conclusion is that the patch is fine as
> it is and anything related to unconvertible secids will be handled
> by SELinux internally?
>

No.  This patch does not get my ACK.  Steve is right that silently
dropping information is a big big no no for the audit system and
that's what this patch does.  This cannot be wholly handled properly
inside the LSM either.  I don't see any patch meeting everyone's
requirements outside of a new one that includes the audit helper I
suggested.

-Eric
--
To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 3rd revision] Add SELinux context support to AUDIT target

[Patrick McHardy]

On 09.06.2011 14:52, Eric Paris wrote:
> On Thu, Jun 9, 2011 at 8:28 AM, Patrick McHardy <kaber@trash.net> wrote:
>> Just to make sure, so the conclusion is that the patch is fine as
>> it is and anything related to unconvertible secids will be handled
>> by SELinux internally?
>>
> 
> No.  This patch does not get my ACK.  Steve is right that silently
> dropping information is a big big no no for the audit system and
> that's what this patch does.  This cannot be wholly handled properly
> inside the LSM either.  I don't see any patch meeting everyone's
> requirements outside of a new one that includes the audit helper I
> suggested.

OK, I see, thanks.

Re: [PATCH 3rd revision] Add SELinux context support to AUDIT target

[Mr Dash Four]

>> Just to make sure, so the conclusion is that the patch is fine as
>> it is and anything related to unconvertible secids will be handled
>> by SELinux internally?
>>
>>     
>
> No.  This patch does not get my ACK.  Steve is right that silently
> dropping information is a big big no no for the audit system and
> that's what this patch does.  This cannot be wholly handled properly
> inside the LSM either.  I don't see any patch meeting everyone's
> requirements outside of a new one that includes the audit helper I
> suggested.
>   
Right, so the function you suggested yesterday (audit_log_secctx) should 
be added in audit.c in its entirety, and xt_AUDIT.c should just have 
something like:

#ifdef CONFIG_NF_CONNTRACK_SECMARK
    if (skb->secmark)
                audit_log_secctx(ab,skb->secmark);
#endif

Thus, discarding the result (rc), unless we are interested in the error 
code, which I don't think is the case here. Would everyone be happy with 
this?

Re: [PATCH 3rd revision] Add SELinux context support to AUDIT target

[Eric Paris]

On Thu, Jun 9, 2011 at 10:08 AM, Mr Dash Four
<mr.dash.four@googlemail.com> wrote:
>
>>> Just to make sure, so the conclusion is that the patch is fine as
>>> it is and anything related to unconvertible secids will be handled
>>> by SELinux internally?
>>>
>>>
>>
>> No.  This patch does not get my ACK.  Steve is right that silently
>> dropping information is a big big no no for the audit system and
>> that's what this patch does.  This cannot be wholly handled properly
>> inside the LSM either.  I don't see any patch meeting everyone's
>> requirements outside of a new one that includes the audit helper I
>> suggested.
>>
>
> Right, so the function you suggested yesterday (audit_log_secctx) should be
> added in audit.c in its entirety, and xt_AUDIT.c should just have something
> like:
>
> #ifdef CONFIG_NF_CONNTRACK_SECMARK
>   if (skb->secmark)
>               audit_log_secctx(ab,skb->secmark);
> #endif
>
> Thus, discarding the result (rc), unless we are interested in the error
> code, which I don't think is the case here. Would everyone be happy with
> this?

Actually just make it a void function as I don't think anyone
would/could/should make use of the return value.

-Eric
--
To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 3rd revision] Add SELinux context support to AUDIT target

[Mr Dash Four]

>> Right, so the function you suggested yesterday (audit_log_secctx) should be
>> added in audit.c in its entirety, and xt_AUDIT.c should just have something
>> like:
>>
>> #ifdef CONFIG_NF_CONNTRACK_SECMARK
>>   if (skb->secmark)
>>               audit_log_secctx(ab,skb->secmark);
>> #endif
>>
>> Thus, discarding the result (rc), unless we are interested in the error
>> code, which I don't think is the case here. Would everyone be happy with
>> this?
>>     
>
> Actually just make it a void function as I don't think anyone
> would/could/should make use of the return value.
>   
In other words (audit.c) - N.B. the change from "subj" to "obj" as per 
Steve's suggestion a while ago:

void audit_log_secctx(struct auditbuffer *ab, u32 secid)
{
    int len;
    char *ctx;

    if (security_secid_to_secctx(sid, &ctx, &len)) {
        audit_panic("Cannot convert secid to context");
    } else {
            audit_log_format(ab, " obj=%s", ctx);
            security_release_secctx(ctx, len);
    }
}

And xt_AUDIT.c stays as per my suggestion above. Should I assume that 
gets the "go" from everyone concerned?

Re: [PATCH 3rd revision] Add SELinux context support to AUDIT target

[Mr Dash Four]

>>> #ifdef CONFIG_NF_CONNTRACK_SECMARK
>>>   if (skb->secmark)
>>>               audit_log_secctx(ab,skb->secmark);
>>> #endif
>>>
>>> Thus, discarding the result (rc), unless we are interested in the error
>>> code, which I don't think is the case here. Would everyone be happy 
>>> with
>>> this?
>>>     
>>
>> Actually just make it a void function as I don't think anyone
>> would/could/should make use of the return value.
>>   
> In other words (audit.c) - N.B. the change from "subj" to "obj" as per 
> Steve's suggestion a while ago:
>
> void audit_log_secctx(struct auditbuffer *ab, u32 secid)
> {
>    int len;
>    char *ctx;
>
>    if (security_secid_to_secctx(sid, &ctx, &len)) {
>        audit_panic("Cannot convert secid to context");
>    } else {
>            audit_log_format(ab, " obj=%s", ctx);
>            security_release_secctx(ctx, len);
>    }
> }
>
> And xt_AUDIT.c stays as per my suggestion above. Should I assume that 
> gets the "go" from everyone concerned?
If there are no objections, I'll resubmit the patch at the weekend with 
the above functionality implemented.

[PATCH 4th revision] Add SELinux context support to AUDIT target

[Mr Dash Four]

In this revision the conversion of secid to SELinux context and adding it to the audit log is moved from xt_AUDIT.c to audit.c with the aid of a separate helper function - audit_log_secctx - which does both the conversion and logging of SELinux context, thus also preventing internal secid number being leaked to userspace. If conversion is not successful an error is raised. 

With the introduction of this helper function the work done in xt_AUDIT.c is much more simplified. It also opens the possibility of this helper function being used by other modules (including auditd itself), if desired. With this addition, typical (raw auditd) output after applying the 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_client_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=22 dport=3561 obj=system_u:object_r:ssh_server_packet_t:s0


Signed-off-by: Mr Dash Four <mr.dash.four@googlemail.com>
---
 include/linux/audit.h    |    7 +++++++
 kernel/audit.c           |   29 +++++++++++++++++++++++++++++
 net/netfilter/xt_AUDIT.c |    5 +++++
 3 files changed, 41 insertions(+), 0 deletions(-)

diff --git a/include/linux/audit.h b/include/linux/audit.h
index 9d339eb..3e47019 100644
--- a/include/linux/audit.h
+++ b/include/linux/audit.h
@@ -613,6 +613,12 @@ extern void		    audit_log_d_path(struct audit_buffer *ab,
 extern void		    audit_log_key(struct audit_buffer *ab,
 					  char *key);
 extern void		    audit_log_lost(const char *message);
+#ifdef CONFIG_SECURITY
+extern void 		    audit_log_secctx(struct audit_buffer *ab, u32 secid);
+#else
+#define audit_log_secctx(b,s) do { ; } while (0)
+#endif
+
 extern int		    audit_update_lsm_rules(void);
 
 				/* Private API (for audit.c only) */
@@ -635,6 +641,7 @@ extern int audit_enabled;
 #define audit_log_untrustedstring(a,s) do { ; } while (0)
 #define audit_log_d_path(b, p, d) do { ; } while (0)
 #define audit_log_key(b, k) do { ; } while (0)
+#define audit_log_secctx(b,s) do { ; } while (0)
 #define audit_enabled 0
 #endif
 #endif
diff --git a/kernel/audit.c b/kernel/audit.c
index 9395003..4e0685e 100644
--- a/kernel/audit.c
+++ b/kernel/audit.c
@@ -55,6 +55,9 @@
 #include <net/sock.h>
 #include <net/netlink.h>
 #include <linux/skbuff.h>
+#ifdef CONFIG_SECURITY
+#include <linux/security.h>
+#endif
 #include <linux/netlink.h>
 #include <linux/freezer.h>
 #include <linux/tty.h>
@@ -1502,6 +1505,32 @@ void audit_log(struct audit_context *ctx, gfp_t gfp_mask, int type,
 	}
 }
 
+#ifdef CONFIG_SECURITY
+/**
+ * audit_log_secctx - Converts and logs SELinux context
+ * @ab: audit_buffer
+ * @secid: security number
+ *
+ * This is a helper function that calls security_secid_to_secctx to convert secid to secctx 
+ * and then adds the (converted) SELinux context to the audit log 
+ * by calling audit_log_format, thus also preventing leak of internal secid to userspace. 
+ * If secid cannot be converted audit_panic is called. 
+ */
+void audit_log_secctx(struct audit_buffer *ab, u32 secid)
+{
+	u32 len;
+	char *secctx;
+
+	if (security_secid_to_secctx(secid, &secctx, &len)) {
+		audit_panic("Cannot convert secid to context");
+	} else {
+		audit_log_format(ab, " obj=%s", secctx);
+		security_release_secctx(secctx, len);
+	}
+} 
+EXPORT_SYMBOL(audit_log_secctx);
+#endif
+
 EXPORT_SYMBOL(audit_log_start);
 EXPORT_SYMBOL(audit_log_end);
 EXPORT_SYMBOL(audit_log_format);
diff --git a/net/netfilter/xt_AUDIT.c b/net/netfilter/xt_AUDIT.c
index 363a99e..4bca15a 100644
--- a/net/netfilter/xt_AUDIT.c
+++ b/net/netfilter/xt_AUDIT.c
@@ -163,6 +163,11 @@ audit_tg(struct sk_buff *skb, const struct xt_action_param *par)
 		break;
 	}
 
+#ifdef CONFIG_NETWORK_SECMARK
+	if (skb->secmark)
+		audit_log_secctx(ab, skb->secmark);
+#endif
+
 	audit_log_end(ab);
 
 errout:
-- 
1.7.3.4

Re: [PATCH 4th revision] Add SELinux context support to AUDIT target

[Steve Grubb]

On Saturday, June 18, 2011 08:08:05 AM Mr Dash Four wrote:
> +#ifdef CONFIG_SECURITY
> +/**
> + * audit_log_secctx - Converts and logs SELinux context
> + * @ab: audit_buffer
> + * @secid: security number
> + *
> + * This is a helper function that calls security_secid_to_secctx to
> convert secid to secctx + * and then adds the (converted) SELinux context
> to the audit log + * by calling audit_log_format, thus also preventing
> leak of internal secid to userspace. + * If secid cannot be converted
> audit_panic is called.
> + */
> +void audit_log_secctx(struct audit_buffer *ab, u32 secid)
> +{
> +	u32 len;
> +	char *secctx;
> +
> +	if (security_secid_to_secctx(secid, &secctx, &len)) {
> +		audit_panic("Cannot convert secid to context");
> +	} else {
> +		audit_log_format(ab, " obj=%s", secctx);
> +		security_release_secctx(secctx, len);

Eric,

Do you think this should be hardcoded to be obj? Would we ever log the subj? Or should 
obj be part of the function name to make it clear which piece is getting logged?

-Steve

Re: [PATCH 4th revision] Add SELinux context support to AUDIT target

[Mr Dash Four]

> Do you think this should be hardcoded to be obj? Would we ever log the subj? Or should 
> obj be part of the function name to make it clear which piece is getting logged?
>   
I thought of that, though I don't know what variety of option names 
would be there to be used with that function.

If there is a need to use something other than "obj", like, "subj" or 
even "tcontext" or "scontext" for example, then I would favour passing 
the option name as function parameter - something like "void 
audit_log_secctx(struct audit_buffer *ab, char *secname, u32 secid)" or 
even "void audit_log_secctx(struct audit_buffer *ab, int secname, u32 
secid)" (secname here being one of 0, 1, 2 ... corresponding to "obj", 
"subj" etc).

Similar approach is already used in audit.c - in audit_log_config_change 
for example:

static int audit_log_config_change(char *function_name, int new, int 
old, uid_t loginuid, u32 sessionid, u32 sid, int allow_changes)
{
    struct audit_buffer *ab;
    int rc = 0;

    ab = audit_log_start(NULL, GFP_KERNEL, AUDIT_CONFIG_CHANGE);
    audit_log_format(ab, "%s=%d old=%d auid=%u ses=%u", function_name, 
new, old, loginuid, sessionid);

Re: [PATCH 4th revision] Add SELinux context support to AUDIT target

[Eric Paris]

On Mon, Jun 20, 2011 at 10:21 AM, Mr Dash Four
<mr.dash.four@googlemail.com> wrote:
>
>> Do you think this should be hardcoded to be obj? Would we ever log the
>> subj? Or should obj be part of the function name to make it clear which
>> piece is getting logged?
>>
>
> I thought of that, though I don't know what variety of option names would be
> there to be used with that function.
>
> If there is a need to use something other than "obj", like, "subj" or even
> "tcontext" or "scontext" for example, then I would favour passing the option
> name as function parameter - something like "void audit_log_secctx(struct
> audit_buffer *ab, char *secname, u32 secid)" or even "void
> audit_log_secctx(struct audit_buffer *ab, int secname, u32 secid)" (secname
> here being one of 0, 1, 2 ... corresponding to "obj", "subj" etc).
>
> Similar approach is already used in audit.c - in audit_log_config_change for
> example:
>
> static int audit_log_config_change(char *function_name, int new, int old,
> uid_t loginuid, u32 sessionid, u32 sid, int allow_changes)
> {
>   struct audit_buffer *ab;
>   int rc = 0;
>
>   ab = audit_log_start(NULL, GFP_KERNEL, AUDIT_CONFIG_CHANGE);
>   audit_log_format(ab, "%s=%d old=%d auid=%u ses=%u", function_name, new,
> old, loginuid, sessionid);

Hard code for now.  %s in audit record building is the devil since
there is no enforcement of audit's rather 'special' string encoding
rules.  If we need another name later we'll cross that bridge when we
get there, possibly with another helper function and pushing the %s to
a static function inside audit.  I will not willing expose %s to code
outside of audit.c.

Acked-by: Eric Paris <eparis@redhat.com>
--
To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 4th revision] Add SELinux context support to AUDIT target

[Patrick McHardy]

Am 20.06.2011 16:27, schrieb Eric Paris:
> On Mon, Jun 20, 2011 at 10:21 AM, Mr Dash Four
> <mr.dash.four@googlemail.com> wrote:
>> >
>>> >> Do you think this should be hardcoded to be obj? Would we ever log the
>>> >> subj? Or should obj be part of the function name to make it clear which
>>> >> piece is getting logged?
>>> >>
>> >
>> > I thought of that, though I don't know what variety of option names would be
>> > there to be used with that function.
>> >
>> > If there is a need to use something other than "obj", like, "subj" or even
>> > "tcontext" or "scontext" for example, then I would favour passing the option
>> > name as function parameter - something like "void audit_log_secctx(struct
>> > audit_buffer *ab, char *secname, u32 secid)" or even "void
>> > audit_log_secctx(struct audit_buffer *ab, int secname, u32 secid)" (secname
>> > here being one of 0, 1, 2 ... corresponding to "obj", "subj" etc).
>> >
>> > Similar approach is already used in audit.c - in audit_log_config_change for
>> > example:
>> >
>> > static int audit_log_config_change(char *function_name, int new, int old,
>> > uid_t loginuid, u32 sessionid, u32 sid, int allow_changes)
>> > {
>> >   struct audit_buffer *ab;
>> >   int rc = 0;
>> >
>> >   ab = audit_log_start(NULL, GFP_KERNEL, AUDIT_CONFIG_CHANGE);
>> >   audit_log_format(ab, "%s=%d old=%d auid=%u ses=%u", function_name, new,
>> > old, loginuid, sessionid);
> Hard code for now.  %s in audit record building is the devil since
> there is no enforcement of audit's rather 'special' string encoding
> rules.  If we need another name later we'll cross that bridge when we
> get there, possibly with another helper function and pushing the %s to
> a static function inside audit.  I will not willing expose %s to code
> outside of audit.c.
> 
> Acked-by: Eric Paris <eparis@redhat.com>
> 

Applied, thanks.

I had to fix some overly long lines and whitespace errors in the
patch and the commit message to contain a subject and not have the
entire text contained in two lines.

Please be more careful of this next time.