[PATCH] Security context information added to netfilter_queue

[PATCH] Security context information added to netfilter_queue

[Roman Kubiak]

>From dccc2ca387d7b4dd16fff537ce2cab280517cab5 Mon Sep 17 00:00:00 2001
From: Roman Kubiak <r.kubiak@samsung.com>
Date: Wed, 22 Apr 2015 15:54:20 +0200
Subject: [PATCH] Security context information added to netfilter_queue

This patch adds an additional attribute when sending
packet information via netlink in netfilter_queue module.
It will send additional security context data, so that
userspace applications can verify this context against
their own security databases.

Signed-off-by: Roman Kubiak <r.kubiak@samsung.com>
---
 include/uapi/linux/netfilter/nfnetlink_queue.h |  4 ++-
 net/netfilter/nfnetlink_queue_core.c           | 46 ++++++++++++++++++++++++++
 2 files changed, 49 insertions(+), 1 deletion(-)

diff --git a/include/uapi/linux/netfilter/nfnetlink_queue.h b/include/uapi/linux/netfilter/nfnetlink_queue.h
index 8dd819e..313935a 100644
--- a/include/uapi/linux/netfilter/nfnetlink_queue.h
+++ b/include/uapi/linux/netfilter/nfnetlink_queue.h
@@ -49,6 +49,7 @@ enum nfqnl_attr_type {
 	NFQA_EXP,			/* nf_conntrack_netlink.h */
 	NFQA_UID,			/* __u32 sk uid */
 	NFQA_GID,			/* __u32 sk gid */
+	NFQA_SECCTX,			/* security context, NL_A_STRING */
 
 	__NFQA_MAX
 };
@@ -102,7 +103,8 @@ enum nfqnl_attr_config {
 #define NFQA_CFG_F_CONNTRACK			(1 << 1)
 #define NFQA_CFG_F_GSO				(1 << 2)
 #define NFQA_CFG_F_UID_GID			(1 << 3)
-#define NFQA_CFG_F_MAX				(1 << 4)
+#define NFQA_CFG_F_SECCTX			(1 << 4)
+#define NFQA_CFG_F_MAX				(1 << 5)
 
 /* flags for NFQA_SKB_INFO */
 /* packet appears to have wrong checksums, but they are ok */
diff --git a/net/netfilter/nfnetlink_queue_core.c b/net/netfilter/nfnetlink_queue_core.c
index 0b98c74..de3b97a 100644
--- a/net/netfilter/nfnetlink_queue_core.c
+++ b/net/netfilter/nfnetlink_queue_core.c
@@ -278,6 +278,27 @@ nla_put_failure:
 	return -1;
 }
 
+static int nfqnl_get_sk_secctx(struct sock *sk, char **secdata, u32 *seclen)
+{
+	u32 secid = 0;
+	int ret = -1;
+
+	if (!sk)
+		return ret;
+
+	if (!sk_fullsock(sk))
+		return ret;
+
+	read_lock_bh(&sk->sk_callback_lock);
+	security_sk_getsecid(sk, &secid);
+
+	if (secid)
+		ret = security_secid_to_secctx(secid, secdata, seclen);
+
+	read_unlock_bh(&sk->sk_callback_lock);
+	return ret;
+}
+
 static struct sk_buff *
 nfqnl_build_packet_message(struct net *net, struct nfqnl_instance *queue,
 			   struct nf_queue_entry *entry,
@@ -297,6 +318,9 @@ nfqnl_build_packet_message(struct net *net, struct nfqnl_instance *queue,
 	struct nf_conn *ct = NULL;
 	enum ip_conntrack_info uninitialized_var(ctinfo);
 	bool csum_verify;
+	char *secdata = NULL;
+	char *secdata_nterm = NULL;
+	u32 seclen = 0;
 
 	size =    nlmsg_total_size(sizeof(struct nfgenmsg))
 		+ nla_total_size(sizeof(struct nfqnl_msg_packet_hdr))
@@ -352,6 +376,13 @@ nfqnl_build_packet_message(struct net *net, struct nfqnl_instance *queue,
 			+ nla_total_size(sizeof(u_int32_t)));	/* gid */
 	}
 
+	if (queue->flags & NFQA_CFG_F_SECCTX) {
+		if (nfqnl_get_sk_secctx(entskb->sk, &secdata, &seclen) == 0) {
+			if (seclen > 0)
+				size += nla_total_size(seclen) + 1;
+		}
+	}
+
 	skb = nfnetlink_alloc_skb(net, size, queue->peer_portid,
 				  GFP_ATOMIC);
 	if (!skb) {
@@ -479,6 +510,20 @@ nfqnl_build_packet_message(struct net *net, struct nfqnl_instance *queue,
 	    nfqnl_put_sk_uidgid(skb, entskb->sk) < 0)
 		goto nla_put_failure;
 
+	if ((queue->flags & NFQA_CFG_F_SECCTX) && seclen > 0) {
+		secdata_nterm = kmalloc(seclen + 1, GFP_ATOMIC);
+
+		if (!secdata_nterm)
+			goto nla_put_failure;
+		memcpy(secdata_nterm, secdata, seclen);
+		secdata_nterm[seclen] = 0;
+
+		if (nla_put(skb, NFQA_SECCTX, seclen + 1, secdata_nterm))
+			goto nla_put_failure;
+
+		kfree(secdata_nterm);
+	}
+
 	if (ct && nfqnl_ct_put(skb, ct, ctinfo) < 0)
 		goto nla_put_failure;
 
@@ -509,6 +554,7 @@ nfqnl_build_packet_message(struct net *net, struct nfqnl_instance *queue,
 nla_put_failure:
 	skb_tx_error(entskb);
 	kfree_skb(skb);
+	kfree(secdata_nterm);
 	net_err_ratelimited("nf_queue: error creating packet message\n");
 	return NULL;
 }
-- 
1.9.1

Re: [PATCH] Security context information added to netfilter_queue

[Florian Westphal]

Roman Kubiak <r.kubiak@samsung.com> wrote:
> From dccc2ca387d7b4dd16fff537ce2cab280517cab5 Mon Sep 17 00:00:00 2001
> From: Roman Kubiak <r.kubiak@samsung.com>
> Date: Wed, 22 Apr 2015 15:54:20 +0200
> Subject: [PATCH] Security context information added to netfilter_queue
> 
> This patch adds an additional attribute when sending
> packet information via netlink in netfilter_queue module.
> It will send additional security context data, so that
> userspace applications can verify this context against
> their own security databases.
> 
> Signed-off-by: Roman Kubiak <r.kubiak@samsung.com>
> ---
>  include/uapi/linux/netfilter/nfnetlink_queue.h |  4 ++-
>  net/netfilter/nfnetlink_queue_core.c           | 46 ++++++++++++++++++++++++++
>  2 files changed, 49 insertions(+), 1 deletion(-)
> 
> diff --git a/include/uapi/linux/netfilter/nfnetlink_queue.h b/include/uapi/linux/netfilter/nfnetlink_queue.h
> index 8dd819e..313935a 100644
> --- a/include/uapi/linux/netfilter/nfnetlink_queue.h
> +++ b/include/uapi/linux/netfilter/nfnetlink_queue.h
> @@ -49,6 +49,7 @@ enum nfqnl_attr_type {
>  	NFQA_EXP,			/* nf_conntrack_netlink.h */
>  	NFQA_UID,			/* __u32 sk uid */
>  	NFQA_GID,			/* __u32 sk gid */
> +	NFQA_SECCTX,			/* security context, NL_A_STRING */
>  
>  	__NFQA_MAX
>  };
> @@ -102,7 +103,8 @@ enum nfqnl_attr_config {
>  #define NFQA_CFG_F_CONNTRACK			(1 << 1)
>  #define NFQA_CFG_F_GSO				(1 << 2)
>  #define NFQA_CFG_F_UID_GID			(1 << 3)
> -#define NFQA_CFG_F_MAX				(1 << 4)
> +#define NFQA_CFG_F_SECCTX			(1 << 4)
> +#define NFQA_CFG_F_MAX				(1 << 5)
>  
>  /* flags for NFQA_SKB_INFO */
>  /* packet appears to have wrong checksums, but they are ok */
> diff --git a/net/netfilter/nfnetlink_queue_core.c b/net/netfilter/nfnetlink_queue_core.c
> index 0b98c74..de3b97a 100644
> --- a/net/netfilter/nfnetlink_queue_core.c
> +++ b/net/netfilter/nfnetlink_queue_core.c
> @@ -278,6 +278,27 @@ nla_put_failure:
>  	return -1;
>  }
>  
> +static int nfqnl_get_sk_secctx(struct sock *sk, char **secdata, u32 *seclen)
> +{

static u32, and return seclen directly instead of *seclen.

>  	bool csum_verify;
> +	char *secdata = NULL;
> +	char *secdata_nterm = NULL;
> +	u32 seclen = 0;
>  
>  	size =    nlmsg_total_size(sizeof(struct nfgenmsg))
>  		+ nla_total_size(sizeof(struct nfqnl_msg_packet_hdr))
> @@ -352,6 +376,13 @@ nfqnl_build_packet_message(struct net *net, struct nfqnl_instance *queue,
>  			+ nla_total_size(sizeof(u_int32_t)));	/* gid */
>  	}
>  
> +	if (queue->flags & NFQA_CFG_F_SECCTX) {
> +		if (nfqnl_get_sk_secctx(entskb->sk, &secdata, &seclen) == 0) {
> +			if (seclen > 0)
> +				size += nla_total_size(seclen) + 1;
> +		}
> +	}

I'd suggest

if ((queue->flags & NFQA_CFG_F_SECCTX) && entskb->sk) {
	seclen = nfqnl_get_sk_secctx(entskb->sk, &secdata);
	if (seclen > 0)
		size += nla_total_size(seclen);
}

>  	if (!skb) {
> @@ -479,6 +510,20 @@ nfqnl_build_packet_message(struct net *net, struct nfqnl_instance *queue,
>  	    nfqnl_put_sk_uidgid(skb, entskb->sk) < 0)
>  		goto nla_put_failure;
>  
> +	if ((queue->flags & NFQA_CFG_F_SECCTX) && seclen > 0) {

if (seclen > 0) {

(no need to test ->flags again)

> +		secdata_nterm = kmalloc(seclen + 1, GFP_ATOMIC);
> +
> +		if (!secdata_nterm)
> +			goto nla_put_failure;
> +		memcpy(secdata_nterm, secdata, seclen);
> +		secdata_nterm[seclen] = 0;

Why this extra copy?

> +		if (nla_put(skb, NFQA_SECCTX, seclen + 1, secdata_nterm))
> +			goto nla_put_failure;
		if (nla_put(skb, NFQA_SECCTX, seclen, secdata))

No need for 0-terminated string.
If its absolutely needed, consider adding it via nla_append().

Re: [PATCH] Security context information added to netfilter_queue

[Pablo Neira Ayuso]

On Mon, May 25, 2015 at 12:16:01PM +0200, Roman Kubiak wrote:
> From dccc2ca387d7b4dd16fff537ce2cab280517cab5 Mon Sep 17 00:00:00 2001
> From: Roman Kubiak <r.kubiak@samsung.com>
> Date: Wed, 22 Apr 2015 15:54:20 +0200
> Subject: [PATCH] Security context information added to netfilter_queue
> 
> This patch adds an additional attribute when sending
> packet information via netlink in netfilter_queue module.
> It will send additional security context data, so that
> userspace applications can verify this context against
> their own security databases.

Please prepend the subsystem prefix to the patch title, ie.

        netfilter: nfnetlink_queue: Add security context information

A couple of minor comments on top of Florian's.

> Signed-off-by: Roman Kubiak <r.kubiak@samsung.com>
> ---
>  include/uapi/linux/netfilter/nfnetlink_queue.h |  4 ++-
>  net/netfilter/nfnetlink_queue_core.c           | 46 ++++++++++++++++++++++++++
>  2 files changed, 49 insertions(+), 1 deletion(-)
> 
> diff --git a/include/uapi/linux/netfilter/nfnetlink_queue.h b/include/uapi/linux/netfilter/nfnetlink_queue.h
> index 8dd819e..313935a 100644
> --- a/include/uapi/linux/netfilter/nfnetlink_queue.h
> +++ b/include/uapi/linux/netfilter/nfnetlink_queue.h
> @@ -49,6 +49,7 @@ enum nfqnl_attr_type {
>  	NFQA_EXP,			/* nf_conntrack_netlink.h */
>  	NFQA_UID,			/* __u32 sk uid */
>  	NFQA_GID,			/* __u32 sk gid */
> +	NFQA_SECCTX,			/* security context, NL_A_STRING */

I'd suggest for this comment.

/* security context string */

>  	__NFQA_MAX
>  };
> @@ -102,7 +103,8 @@ enum nfqnl_attr_config {
>  #define NFQA_CFG_F_CONNTRACK			(1 << 1)
>  #define NFQA_CFG_F_GSO				(1 << 2)
>  #define NFQA_CFG_F_UID_GID			(1 << 3)
> -#define NFQA_CFG_F_MAX				(1 << 4)
> +#define NFQA_CFG_F_SECCTX			(1 << 4)
> +#define NFQA_CFG_F_MAX				(1 << 5)
>  
>  /* flags for NFQA_SKB_INFO */
>  /* packet appears to have wrong checksums, but they are ok */
> diff --git a/net/netfilter/nfnetlink_queue_core.c b/net/netfilter/nfnetlink_queue_core.c
> index 0b98c74..de3b97a 100644
> --- a/net/netfilter/nfnetlink_queue_core.c
> +++ b/net/netfilter/nfnetlink_queue_core.c
> @@ -278,6 +278,27 @@ nla_put_failure:
>  	return -1;
>  }
>  
> +static int nfqnl_get_sk_secctx(struct sock *sk, char **secdata, u32 *seclen)
> +{
> +	u32 secid = 0;
> +	int ret = -1;
> +
> +	if (!sk)
> +		return ret;
> +
> +	if (!sk_fullsock(sk))
> +		return ret;

you can collapse these two if's:

        if (!sk || !sk_fullsock(sk))

[PATCH v2] nfnetlink_queue: add security context information

[Roman Kubiak]

[sidenote]
The additional NULL at the end of the security context is there because SMACK does not add this
to it's labels while SELinux does. So in order to avoid checking i just add it always.
This additional byte is also represented when calculating the size.
I did that because we are not transmitting the size of the context and there is no specified
max length so it has to be NULL terminated (at least it seemed like a valid solution)

best regards

This patch adds an additional attribute when sending
packet information via netlink in netfilter_queue module.
It will send additional security context data, so that
userspace applications can verify this context against
their own security databases.

Signed-off-by: Roman Kubiak <r.kubiak@samsung.com>
---
v2:
- nfqnl_get_sk_secctx returns seclen now, this changes
- updated size calculation
- changed NFQA_SECCTX comment
- removed duplicate testing of NFQA_CFG_F flags
---
 include/uapi/linux/netfilter/nfnetlink_queue.h |  4 ++-
 net/netfilter/nfnetlink_queue_core.c           | 45 ++++++++++++++++++++++++++
 2 files changed, 48 insertions(+), 1 deletion(-)

diff --git a/include/uapi/linux/netfilter/nfnetlink_queue.h b/include/uapi/linux/netfilter/nfnetlink_queue.h
index 8dd819e..b67a853 100644
--- a/include/uapi/linux/netfilter/nfnetlink_queue.h
+++ b/include/uapi/linux/netfilter/nfnetlink_queue.h
@@ -49,6 +49,7 @@ enum nfqnl_attr_type {
 	NFQA_EXP,			/* nf_conntrack_netlink.h */
 	NFQA_UID,			/* __u32 sk uid */
 	NFQA_GID,			/* __u32 sk gid */
+	NFQA_SECCTX,			/* security context string */
 
 	__NFQA_MAX
 };
@@ -102,7 +103,8 @@ enum nfqnl_attr_config {
 #define NFQA_CFG_F_CONNTRACK			(1 << 1)
 #define NFQA_CFG_F_GSO				(1 << 2)
 #define NFQA_CFG_F_UID_GID			(1 << 3)
-#define NFQA_CFG_F_MAX				(1 << 4)
+#define NFQA_CFG_F_SECCTX			(1 << 4)
+#define NFQA_CFG_F_MAX				(1 << 5)
 
 /* flags for NFQA_SKB_INFO */
 /* packet appears to have wrong checksums, but they are ok */
diff --git a/net/netfilter/nfnetlink_queue_core.c b/net/netfilter/nfnetlink_queue_core.c
index 0b98c74..60c70fb 100644
--- a/net/netfilter/nfnetlink_queue_core.c
+++ b/net/netfilter/nfnetlink_queue_core.c
@@ -278,6 +278,27 @@ nla_put_failure:
 	return -1;
 }
 
+static u32 nfqnl_get_sk_secctx(struct sock *sk, char **secdata)
+{
+	u32 secid = 0;
+	u32 seclen = 0;
+	int ret = -1;
+
+	if (!sk || !sk_fullsock(sk))
+		return ret;
+
+	read_lock_bh(&sk->sk_callback_lock);
+	security_sk_getsecid(sk, &secid);
+
+	if (secid)
+		ret = security_secid_to_secctx(secid, secdata, &seclen);
+	if (!ret)
+		seclen = 0;
+
+	read_unlock_bh(&sk->sk_callback_lock);
+	return seclen;
+}
+
 static struct sk_buff *
 nfqnl_build_packet_message(struct net *net, struct nfqnl_instance *queue,
 			   struct nf_queue_entry *entry,
@@ -297,6 +318,9 @@ nfqnl_build_packet_message(struct net *net, struct nfqnl_instance *queue,
 	struct nf_conn *ct = NULL;
 	enum ip_conntrack_info uninitialized_var(ctinfo);
 	bool csum_verify;
+	char *secdata = NULL;
+	char *secdata_nterm = NULL;
+	u32 seclen = 0;
 
 	size =    nlmsg_total_size(sizeof(struct nfgenmsg))
 		+ nla_total_size(sizeof(struct nfqnl_msg_packet_hdr))
@@ -352,6 +376,12 @@ nfqnl_build_packet_message(struct net *net, struct nfqnl_instance *queue,
 			+ nla_total_size(sizeof(u_int32_t)));	/* gid */
 	}
 
+	if ((queue->flags & NFQA_CFG_F_SECCTX) && entskb->sk) {
+		seclen = nfqnl_get_sk_secctx(entskb->sk, &secdata);
+			if (seclen > 0)
+				size += nla_total_size(seclen) + 1;
+	}
+
 	skb = nfnetlink_alloc_skb(net, size, queue->peer_portid,
 				  GFP_ATOMIC);
 	if (!skb) {
@@ -479,6 +509,20 @@ nfqnl_build_packet_message(struct net *net, struct nfqnl_instance *queue,
 	    nfqnl_put_sk_uidgid(skb, entskb->sk) < 0)
 		goto nla_put_failure;
 
+	if (seclen > 0) {
+		secdata_nterm = kmalloc(seclen + 1, GFP_ATOMIC);
+
+		if (!secdata_nterm)
+			goto nla_put_failure;
+		memcpy(secdata_nterm, secdata, seclen);
+		secdata_nterm[seclen] = 0;
+
+		if (nla_put(skb, NFQA_SECCTX, seclen + 1, secdata_nterm))
+			goto nla_put_failure;
+
+		kfree(secdata_nterm);
+	}
+
 	if (ct && nfqnl_ct_put(skb, ct, ctinfo) < 0)
 		goto nla_put_failure;
 
@@ -509,6 +553,7 @@ nfqnl_build_packet_message(struct net *net, struct nfqnl_instance *queue,
 nla_put_failure:
 	skb_tx_error(entskb);
 	kfree_skb(skb);
+	kfree(secdata_nterm);
 	net_err_ratelimited("nf_queue: error creating packet message\n");
 	return NULL;
 }
-- 
1.9.1


On 05/25/2015 03:13 PM, Pablo Neira Ayuso wrote:
> On Mon, May 25, 2015 at 12:16:01PM +0200, Roman Kubiak wrote:
>> From dccc2ca387d7b4dd16fff537ce2cab280517cab5 Mon Sep 17 00:00:00 2001
>> From: Roman Kubiak <r.kubiak@samsung.com>
>> Date: Wed, 22 Apr 2015 15:54:20 +0200
>> Subject: [PATCH] Security context information added to netfilter_queue
>>
>> This patch adds an additional attribute when sending
>> packet information via netlink in netfilter_queue module.
>> It will send additional security context data, so that
>> userspace applications can verify this context against
>> their own security databases.
> 
> Please prepend the subsystem prefix to the patch title, ie.
> 
>         netfilter: nfnetlink_queue: Add security context information
> 
> A couple of minor comments on top of Florian's.
> 
>> Signed-off-by: Roman Kubiak <r.kubiak@samsung.com>
>> ---
>>  include/uapi/linux/netfilter/nfnetlink_queue.h |  4 ++-
>>  net/netfilter/nfnetlink_queue_core.c           | 46 ++++++++++++++++++++++++++
>>  2 files changed, 49 insertions(+), 1 deletion(-)
>>
>> diff --git a/include/uapi/linux/netfilter/nfnetlink_queue.h b/include/uapi/linux/netfilter/nfnetlink_queue.h
>> index 8dd819e..313935a 100644
>> --- a/include/uapi/linux/netfilter/nfnetlink_queue.h
>> +++ b/include/uapi/linux/netfilter/nfnetlink_queue.h
>> @@ -49,6 +49,7 @@ enum nfqnl_attr_type {
>>  	NFQA_EXP,			/* nf_conntrack_netlink.h */
>>  	NFQA_UID,			/* __u32 sk uid */
>>  	NFQA_GID,			/* __u32 sk gid */
>> +	NFQA_SECCTX,			/* security context, NL_A_STRING */
> 
> I'd suggest for this comment.
> 
> /* security context string */
> 
>>  	__NFQA_MAX
>>  };
>> @@ -102,7 +103,8 @@ enum nfqnl_attr_config {
>>  #define NFQA_CFG_F_CONNTRACK			(1 << 1)
>>  #define NFQA_CFG_F_GSO				(1 << 2)
>>  #define NFQA_CFG_F_UID_GID			(1 << 3)
>> -#define NFQA_CFG_F_MAX				(1 << 4)
>> +#define NFQA_CFG_F_SECCTX			(1 << 4)
>> +#define NFQA_CFG_F_MAX				(1 << 5)
>>  
>>  /* flags for NFQA_SKB_INFO */
>>  /* packet appears to have wrong checksums, but they are ok */
>> diff --git a/net/netfilter/nfnetlink_queue_core.c b/net/netfilter/nfnetlink_queue_core.c
>> index 0b98c74..de3b97a 100644
>> --- a/net/netfilter/nfnetlink_queue_core.c
>> +++ b/net/netfilter/nfnetlink_queue_core.c
>> @@ -278,6 +278,27 @@ nla_put_failure:
>>  	return -1;
>>  }
>>  
>> +static int nfqnl_get_sk_secctx(struct sock *sk, char **secdata, u32 *seclen)
>> +{
>> +	u32 secid = 0;
>> +	int ret = -1;
>> +
>> +	if (!sk)
>> +		return ret;
>> +
>> +	if (!sk_fullsock(sk))
>> +		return ret;
> 
> you can collapse these two if's:
> 
>         if (!sk || !sk_fullsock(sk))
> 

-- 
--------------
 Roman Kubiak
--------------

Re: [PATCH v2] nfnetlink_queue: add security context information

[Florian Westphal]

Roman Kubiak <r.kubiak@samsung.com> wrote:
> [sidenote]
> The additional NULL at the end of the security context is there because SMACK does not add this
> to it's labels while SELinux does. So in order to avoid checking i just add it always.
> This additional byte is also represented when calculating the size.
> I did that because we are not transmitting the size of the context and there is no specified
> max length so it has to be NULL terminated (at least it seemed like a valid solution)

The netlink header contains the size of the attribute.
I'd prefer to not have the kernel deal with NULL termination.

> +static u32 nfqnl_get_sk_secctx(struct sock *sk, char **secdata)
> +{
> +	u32 secid = 0;
> +	u32 seclen = 0;
> +	int ret = -1;
> +
> +	if (!sk || !sk_fullsock(sk))
> +		return ret;

return 0/return seclen?

> +	if ((queue->flags & NFQA_CFG_F_SECCTX) && entskb->sk) {
> +		seclen = nfqnl_get_sk_secctx(entskb->sk, &secdata);
> +			if (seclen > 0)
> +				size += nla_total_size(seclen) + 1;

Wrong intent level for if (seclen > 0)

Other than this, it looks ok to me.

Re: [PATCH v2] nfnetlink_queue: add security context information

[Roman Kubiak]

I was wondering, assuming i remove the NULL termination and SMACK sends a piece of data that's not null terminated,
how, on the userland side, can i find out about that size ?

Please notice that i send a libnetfilter_queue patch:
[PATCH] libnetfitler_queue: receive security context info

it uses
*secdata = (unsigned char *)nfnl_get_pointer_to_data(nfad->data, NFQA_SECCTX, char);
to get the security context data, but there is no info about the size, where can i find that not to go over bounds and read beyond what i should ?

I already have the patch prepared with the NULL termination removed but i'd like to make sure it will be ok.

best regards
On 05/25/2015 10:52 PM, Florian Westphal wrote:
> Roman Kubiak <r.kubiak@samsung.com> wrote:
>> [sidenote]
>> The additional NULL at the end of the security context is there because SMACK does not add this
>> to it's labels while SELinux does. So in order to avoid checking i just add it always.
>> This additional byte is also represented when calculating the size.
>> I did that because we are not transmitting the size of the context and there is no specified
>> max length so it has to be NULL terminated (at least it seemed like a valid solution)
> 
> The netlink header contains the size of the attribute.
> I'd prefer to not have the kernel deal with NULL termination.
> 
>> +static u32 nfqnl_get_sk_secctx(struct sock *sk, char **secdata)
>> +{
>> +	u32 secid = 0;
>> +	u32 seclen = 0;
>> +	int ret = -1;
>> +
>> +	if (!sk || !sk_fullsock(sk))
>> +		return ret;
> 
> return 0/return seclen?
> 
>> +	if ((queue->flags & NFQA_CFG_F_SECCTX) && entskb->sk) {
>> +		seclen = nfqnl_get_sk_secctx(entskb->sk, &secdata);
>> +			if (seclen > 0)
>> +				size += nla_total_size(seclen) + 1;
> 
> Wrong intent level for if (seclen > 0)
> 
> Other than this, it looks ok to me.
> --
> 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
> 

-- 
--------------
 Roman Kubiak
--------------

Re: [PATCH v2] nfnetlink_queue: add security context information

[Florian Westphal]

Roman Kubiak <r.kubiak@samsung.com> wrote:
> I was wondering, assuming i remove the NULL termination and SMACK sends a piece of data that's not null terminated,
> how, on the userland side, can i find out about that size ?

The size of netlink attribute is stored in netlink header.
For old libnfnetlink based api, see nfq_get_payload() in
libnetfilter_queue.

> Please notice that i send a libnetfilter_queue patch:
> [PATCH] libnetfitler_queue: receive security context info
> 
> it uses
> *secdata = (unsigned char *)nfnl_get_pointer_to_data(nfad->data, NFQA_SECCTX, char);
> to get the security context data, but there is no info about the size, where can i find that not to go over bounds and read beyond what i should ?

NFQ_PAYLOAD(nfad->data[NFQA_SECCTX - 1])

For libmnl based api (preferred), you'd use

mnl_nlmsg_get_payload_len(attr[NFQA_SECCTX])

[PATCH v3] nfnetlink_queue: add security context information

[Roman Kubiak]

I removed the part where i was adding NULL, i also changed as per suggestions and slightly
streamlined the code.


This patch adds an additional attribute when sending
packet information via netlink in netfilter_queue module.
It will send additional security context data, so that
userspace applications can verify this context against
their own security databases.

Signed-off-by: Roman Kubiak <r.kubiak@samsung.com>
---
v2:
- nfqnl_get_sk_secctx returns seclen now, this changes
- updated size calculation
- changed NFQA_SECCTX comment
- removed duplicate testing of NFQA_CFG_F flags

v3:
- NULL is not added to the security context anymore
- return 0 when socket is invalid in nfqnl_get_sk_secctx
- small intent change
- removed ret variable in nfqnl_get_sk_secctx
---
 include/uapi/linux/netfilter/nfnetlink_queue.h |  4 +++-
 net/netfilter/nfnetlink_queue_core.c           | 31 ++++++++++++++++++++++++++
 2 files changed, 34 insertions(+), 1 deletion(-)

diff --git a/include/uapi/linux/netfilter/nfnetlink_queue.h b/include/uapi/linux/netfilter/nfnetlink_queue.h
index 8dd819e..b67a853 100644
--- a/include/uapi/linux/netfilter/nfnetlink_queue.h
+++ b/include/uapi/linux/netfilter/nfnetlink_queue.h
@@ -49,6 +49,7 @@ enum nfqnl_attr_type {
 	NFQA_EXP,			/* nf_conntrack_netlink.h */
 	NFQA_UID,			/* __u32 sk uid */
 	NFQA_GID,			/* __u32 sk gid */
+	NFQA_SECCTX,			/* security context string */
 
 	__NFQA_MAX
 };
@@ -102,7 +103,8 @@ enum nfqnl_attr_config {
 #define NFQA_CFG_F_CONNTRACK			(1 << 1)
 #define NFQA_CFG_F_GSO				(1 << 2)
 #define NFQA_CFG_F_UID_GID			(1 << 3)
-#define NFQA_CFG_F_MAX				(1 << 4)
+#define NFQA_CFG_F_SECCTX			(1 << 4)
+#define NFQA_CFG_F_MAX				(1 << 5)
 
 /* flags for NFQA_SKB_INFO */
 /* packet appears to have wrong checksums, but they are ok */
diff --git a/net/netfilter/nfnetlink_queue_core.c b/net/netfilter/nfnetlink_queue_core.c
index 0b98c74..ae4f520 100644
--- a/net/netfilter/nfnetlink_queue_core.c
+++ b/net/netfilter/nfnetlink_queue_core.c
@@ -278,6 +278,24 @@ nla_put_failure:
 	return -1;
 }
 
+static u32 nfqnl_get_sk_secctx(struct sock *sk, char **secdata)
+{
+	u32 secid = 0;
+	u32 seclen = 0;
+
+	if (!sk || !sk_fullsock(sk))
+		return 0;
+
+	read_lock_bh(&sk->sk_callback_lock);
+	security_sk_getsecid(sk, &secid);
+
+	if (secid && security_secid_to_secctx(secid, secdata, &seclen))
+		seclen = 0;
+
+	read_unlock_bh(&sk->sk_callback_lock);
+	return seclen;
+}
+
 static struct sk_buff *
 nfqnl_build_packet_message(struct net *net, struct nfqnl_instance *queue,
 			   struct nf_queue_entry *entry,
@@ -297,6 +315,8 @@ nfqnl_build_packet_message(struct net *net, struct nfqnl_instance *queue,
 	struct nf_conn *ct = NULL;
 	enum ip_conntrack_info uninitialized_var(ctinfo);
 	bool csum_verify;
+	char *secdata = NULL;
+	u32 seclen = 0;
 
 	size =    nlmsg_total_size(sizeof(struct nfgenmsg))
 		+ nla_total_size(sizeof(struct nfqnl_msg_packet_hdr))
@@ -352,6 +372,12 @@ nfqnl_build_packet_message(struct net *net, struct nfqnl_instance *queue,
 			+ nla_total_size(sizeof(u_int32_t)));	/* gid */
 	}
 
+	if ((queue->flags & NFQA_CFG_F_SECCTX) && entskb->sk) {
+		seclen = nfqnl_get_sk_secctx(entskb->sk, &secdata);
+		if (seclen)
+			size += nla_total_size(seclen);
+	}
+
 	skb = nfnetlink_alloc_skb(net, size, queue->peer_portid,
 				  GFP_ATOMIC);
 	if (!skb) {
@@ -479,6 +505,11 @@ nfqnl_build_packet_message(struct net *net, struct nfqnl_instance *queue,
 	    nfqnl_put_sk_uidgid(skb, entskb->sk) < 0)
 		goto nla_put_failure;
 
+	if (seclen) {
+		if (nla_put(skb, NFQA_SECCTX, seclen, secdata))
+			goto nla_put_failure;
+	}
+
 	if (ct && nfqnl_ct_put(skb, ct, ctinfo) < 0)
 		goto nla_put_failure;
 
-- 
1.9.1

Re: [PATCH v3] nfnetlink_queue: add security context information

[Roman Kubiak]

I think i forgot to mention one important thing the function:
security_sk_getsecid
is not in the kernel yet, i posted a patch to add it on the linux-security-module mailing list:
http://marc.info/?t=143254934900006&r=1&w=2

It actually does not do anything it's a definition only, the actual code belong to the currently
active LSM in the kernel.

On 05/27/2015 01:04 PM, Roman Kubiak wrote:
> I removed the part where i was adding NULL, i also changed as per suggestions and slightly
> streamlined the code.
> 
> 
> This patch adds an additional attribute when sending
> packet information via netlink in netfilter_queue module.
> It will send additional security context data, so that
> userspace applications can verify this context against
> their own security databases.
> 
> Signed-off-by: Roman Kubiak <r.kubiak@samsung.com>
> ---
> v2:
> - nfqnl_get_sk_secctx returns seclen now, this changes
> - updated size calculation
> - changed NFQA_SECCTX comment
> - removed duplicate testing of NFQA_CFG_F flags
> 
> v3:
> - NULL is not added to the security context anymore
> - return 0 when socket is invalid in nfqnl_get_sk_secctx
> - small intent change
> - removed ret variable in nfqnl_get_sk_secctx
> ---
>  include/uapi/linux/netfilter/nfnetlink_queue.h |  4 +++-
>  net/netfilter/nfnetlink_queue_core.c           | 31 ++++++++++++++++++++++++++
>  2 files changed, 34 insertions(+), 1 deletion(-)
> 
> diff --git a/include/uapi/linux/netfilter/nfnetlink_queue.h b/include/uapi/linux/netfilter/nfnetlink_queue.h
> index 8dd819e..b67a853 100644
> --- a/include/uapi/linux/netfilter/nfnetlink_queue.h
> +++ b/include/uapi/linux/netfilter/nfnetlink_queue.h
> @@ -49,6 +49,7 @@ enum nfqnl_attr_type {
>  	NFQA_EXP,			/* nf_conntrack_netlink.h */
>  	NFQA_UID,			/* __u32 sk uid */
>  	NFQA_GID,			/* __u32 sk gid */
> +	NFQA_SECCTX,			/* security context string */
>  
>  	__NFQA_MAX
>  };
> @@ -102,7 +103,8 @@ enum nfqnl_attr_config {
>  #define NFQA_CFG_F_CONNTRACK			(1 << 1)
>  #define NFQA_CFG_F_GSO				(1 << 2)
>  #define NFQA_CFG_F_UID_GID			(1 << 3)
> -#define NFQA_CFG_F_MAX				(1 << 4)
> +#define NFQA_CFG_F_SECCTX			(1 << 4)
> +#define NFQA_CFG_F_MAX				(1 << 5)
>  
>  /* flags for NFQA_SKB_INFO */
>  /* packet appears to have wrong checksums, but they are ok */
> diff --git a/net/netfilter/nfnetlink_queue_core.c b/net/netfilter/nfnetlink_queue_core.c
> index 0b98c74..ae4f520 100644
> --- a/net/netfilter/nfnetlink_queue_core.c
> +++ b/net/netfilter/nfnetlink_queue_core.c
> @@ -278,6 +278,24 @@ nla_put_failure:
>  	return -1;
>  }
>  
> +static u32 nfqnl_get_sk_secctx(struct sock *sk, char **secdata)
> +{
> +	u32 secid = 0;
> +	u32 seclen = 0;
> +
> +	if (!sk || !sk_fullsock(sk))
> +		return 0;
> +
> +	read_lock_bh(&sk->sk_callback_lock);
> +	security_sk_getsecid(sk, &secid);
> +
> +	if (secid && security_secid_to_secctx(secid, secdata, &seclen))
> +		seclen = 0;
> +
> +	read_unlock_bh(&sk->sk_callback_lock);
> +	return seclen;
> +}
> +
>  static struct sk_buff *
>  nfqnl_build_packet_message(struct net *net, struct nfqnl_instance *queue,
>  			   struct nf_queue_entry *entry,
> @@ -297,6 +315,8 @@ nfqnl_build_packet_message(struct net *net, struct nfqnl_instance *queue,
>  	struct nf_conn *ct = NULL;
>  	enum ip_conntrack_info uninitialized_var(ctinfo);
>  	bool csum_verify;
> +	char *secdata = NULL;
> +	u32 seclen = 0;
>  
>  	size =    nlmsg_total_size(sizeof(struct nfgenmsg))
>  		+ nla_total_size(sizeof(struct nfqnl_msg_packet_hdr))
> @@ -352,6 +372,12 @@ nfqnl_build_packet_message(struct net *net, struct nfqnl_instance *queue,
>  			+ nla_total_size(sizeof(u_int32_t)));	/* gid */
>  	}
>  
> +	if ((queue->flags & NFQA_CFG_F_SECCTX) && entskb->sk) {
> +		seclen = nfqnl_get_sk_secctx(entskb->sk, &secdata);
> +		if (seclen)
> +			size += nla_total_size(seclen);
> +	}
> +
>  	skb = nfnetlink_alloc_skb(net, size, queue->peer_portid,
>  				  GFP_ATOMIC);
>  	if (!skb) {
> @@ -479,6 +505,11 @@ nfqnl_build_packet_message(struct net *net, struct nfqnl_instance *queue,
>  	    nfqnl_put_sk_uidgid(skb, entskb->sk) < 0)
>  		goto nla_put_failure;
>  
> +	if (seclen) {
> +		if (nla_put(skb, NFQA_SECCTX, seclen, secdata))
> +			goto nla_put_failure;
> +	}
> +
>  	if (ct && nfqnl_ct_put(skb, ct, ctinfo) < 0)
>  		goto nla_put_failure;
>  
> 

-- 
--------------
 Roman Kubiak
--------------

Re: [PATCH v3] nfnetlink_queue: add security context information

[Pablo Neira Ayuso]

On Wed, May 27, 2015 at 01:12:42PM +0200, Roman Kubiak wrote:
> I think i forgot to mention one important thing the function:
> security_sk_getsecid is not in the kernel yet, i posted a patch to
> add it on the linux-security-module mailing list:
> http://marc.info/?t=143254934900006&r=1&w=2

You shouldn't split the patches between several lists, they are
interdependent and without that context it is normal that people don't
understand your intentions.

So please send the full patchset, Cc'ing the relevant lists so we can
get feedback from both the netfilter and the linux-security
communities.

BTW, another minor nitpick below:

> > diff --git a/net/netfilter/nfnetlink_queue_core.c b/net/netfilter/nfnetlink_queue_core.c
> > index 0b98c74..ae4f520 100644
> > --- a/net/netfilter/nfnetlink_queue_core.c
> > +++ b/net/netfilter/nfnetlink_queue_core.c
> > @@ -278,6 +278,24 @@ nla_put_failure:
> >  	return -1;
> >  }
> >  
> > +static u32 nfqnl_get_sk_secctx(struct sock *sk, char **secdata)
> > +{
> > +	u32 secid = 0;
> > +	u32 seclen = 0;

Merge these two variable declarations in one line.

Thanks.

Re: [PATCH v3] nfnetlink_queue: add security context information

[Roman Kubiak]

So after some discussion on the security mailing list i had to figure out a way to implement what i wanted
without added security functions and ops.

So the below v4 patch version does that, is uses the secmark field of the sk_buff structure to get the
secid and map that to the security context. I tested it and it works with SMACK and the option:
CONFIG_SECURITY_SMACK_NETFILTER

i don't know about SELinux but it should work assuming that they map secid to secctx.

-- cut here

This patch adds an additional attribute when sending
packet information via netlink in netfilter_queue module.
It will send additional security context data, so that
userspace applications can verify this context against
their own security databases.

Signed-off-by: Roman Kubiak <r.kubiak@samsung.com>
---
v2:
- nfqnl_get_sk_secctx returns seclen now, this changes
- updated size calculation
- changed NFQA_SECCTX comment
- removed duplicate testing of NFQA_CFG_F flags

v3:
- NULL is not added to the security context anymore
- return 0 when socket is invalid in nfqnl_get_sk_secctx
- small intent change
- removed ret variable in nfqnl_get_sk_secctx

v4:
- removed security dependency, this patch does not
  require any changes in other subsystems
- nfqnl_get_sk_secctx returns seclen
- added IFDEF when using secmark from the sk_buff
  structure
---
---
 include/uapi/linux/netfilter/nfnetlink_queue.h |  4 +++-
 net/netfilter/nfnetlink_queue_core.c           | 29 ++++++++++++++++++++++++++
 2 files changed, 32 insertions(+), 1 deletion(-)

diff --git a/include/uapi/linux/netfilter/nfnetlink_queue.h b/include/uapi/linux/netfilter/nfnetlink_queue.h
index 8dd819e..b67a853 100644
--- a/include/uapi/linux/netfilter/nfnetlink_queue.h
+++ b/include/uapi/linux/netfilter/nfnetlink_queue.h
@@ -49,6 +49,7 @@ enum nfqnl_attr_type {
 	NFQA_EXP,			/* nf_conntrack_netlink.h */
 	NFQA_UID,			/* __u32 sk uid */
 	NFQA_GID,			/* __u32 sk gid */
+	NFQA_SECCTX,			/* security context string */
 
 	__NFQA_MAX
 };
@@ -102,7 +103,8 @@ enum nfqnl_attr_config {
 #define NFQA_CFG_F_CONNTRACK			(1 << 1)
 #define NFQA_CFG_F_GSO				(1 << 2)
 #define NFQA_CFG_F_UID_GID			(1 << 3)
-#define NFQA_CFG_F_MAX				(1 << 4)
+#define NFQA_CFG_F_SECCTX			(1 << 4)
+#define NFQA_CFG_F_MAX				(1 << 5)
 
 /* flags for NFQA_SKB_INFO */
 /* packet appears to have wrong checksums, but they are ok */
diff --git a/net/netfilter/nfnetlink_queue_core.c b/net/netfilter/nfnetlink_queue_core.c
index 0b98c74..2c35112 100644
--- a/net/netfilter/nfnetlink_queue_core.c
+++ b/net/netfilter/nfnetlink_queue_core.c
@@ -278,6 +278,24 @@ nla_put_failure:
 	return -1;
 }
 
+static u32 nfqnl_get_sk_secctx(struct sk_buff *skb, char **secdata)
+{
+	u32 seclen = 0;
+
+	if (!skb || !sk_fullsock(skb->sk))
+		return 0;
+
+#if IS_ENABLED(CONFIG_NETWORK_SECMARK)
+	read_lock_bh(&skb->sk->sk_callback_lock);
+
+	if (skb->secmark)
+		security_secid_to_secctx(skb->secmark, secdata, &seclen);
+
+	read_unlock_bh(&skb->sk->sk_callback_lock);
+#endif
+	return seclen;
+}
+
 static struct sk_buff *
 nfqnl_build_packet_message(struct net *net, struct nfqnl_instance *queue,
 			   struct nf_queue_entry *entry,
@@ -297,6 +315,8 @@ nfqnl_build_packet_message(struct net *net, struct nfqnl_instance *queue,
 	struct nf_conn *ct = NULL;
 	enum ip_conntrack_info uninitialized_var(ctinfo);
 	bool csum_verify;
+	char *secdata = NULL;
+	u32 seclen = 0;
 
 	size =    nlmsg_total_size(sizeof(struct nfgenmsg))
 		+ nla_total_size(sizeof(struct nfqnl_msg_packet_hdr))
@@ -352,6 +372,12 @@ nfqnl_build_packet_message(struct net *net, struct nfqnl_instance *queue,
 			+ nla_total_size(sizeof(u_int32_t)));	/* gid */
 	}
 
+	if ((queue->flags & NFQA_CFG_F_SECCTX) && entskb->sk) {
+		seclen = nfqnl_get_sk_secctx(entskb, &secdata);
+		if (seclen)
+			size += nla_total_size(seclen);
+	}
+
 	skb = nfnetlink_alloc_skb(net, size, queue->peer_portid,
 				  GFP_ATOMIC);
 	if (!skb) {
@@ -479,6 +505,9 @@ nfqnl_build_packet_message(struct net *net, struct nfqnl_instance *queue,
 	    nfqnl_put_sk_uidgid(skb, entskb->sk) < 0)
 		goto nla_put_failure;
 
+	if (seclen && nla_put(skb, NFQA_SECCTX, seclen, secdata))
+		goto nla_put_failure;
+
 	if (ct && nfqnl_ct_put(skb, ct, ctinfo) < 0)
 		goto nla_put_failure;
 
-- 
1.9.1


On 05/27/2015 02:49 PM, Pablo Neira Ayuso wrote:
> On Wed, May 27, 2015 at 01:12:42PM +0200, Roman Kubiak wrote:
>> I think i forgot to mention one important thing the function:
>> security_sk_getsecid is not in the kernel yet, i posted a patch to
>> add it on the linux-security-module mailing list:
>> http://marc.info/?t=143254934900006&r=1&w=2
> 
> You shouldn't split the patches between several lists, they are
> interdependent and without that context it is normal that people don't
> understand your intentions.
> 
> So please send the full patchset, Cc'ing the relevant lists so we can
> get feedback from both the netfilter and the linux-security
> communities.
> 
> BTW, another minor nitpick below:
> 
>>> diff --git a/net/netfilter/nfnetlink_queue_core.c b/net/netfilter/nfnetlink_queue_core.c
>>> index 0b98c74..ae4f520 100644
>>> --- a/net/netfilter/nfnetlink_queue_core.c
>>> +++ b/net/netfilter/nfnetlink_queue_core.c
>>> @@ -278,6 +278,24 @@ nla_put_failure:
>>>  	return -1;
>>>  }
>>>  
>>> +static u32 nfqnl_get_sk_secctx(struct sock *sk, char **secdata)
>>> +{
>>> +	u32 secid = 0;
>>> +	u32 seclen = 0;
> 
> Merge these two variable declarations in one line.
> 
> Thanks.
> --
> 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
> 

-- 
--------------
 Roman Kubiak
--------------

Re: [PATCH v3] nfnetlink_queue: add security context information

[Florian Westphal]

Roman Kubiak <r.kubiak@samsung.com> wrote:
> diff --git a/include/uapi/linux/netfilter/nfnetlink_queue.h b/include/uapi/linux/netfilter/nfnetlink_queue.h
> index 8dd819e..b67a853 100644
> --- a/include/uapi/linux/netfilter/nfnetlink_queue.h
> +++ b/include/uapi/linux/netfilter/nfnetlink_queue.h
> @@ -49,6 +49,7 @@ enum nfqnl_attr_type {
>  	NFQA_EXP,			/* nf_conntrack_netlink.h */
>  	NFQA_UID,			/* __u32 sk uid */
>  	NFQA_GID,			/* __u32 sk gid */
> +	NFQA_SECCTX,			/* security context string */
>  
>  	__NFQA_MAX
>  };
> @@ -102,7 +103,8 @@ enum nfqnl_attr_config {
>  #define NFQA_CFG_F_CONNTRACK			(1 << 1)
>  #define NFQA_CFG_F_GSO				(1 << 2)
>  #define NFQA_CFG_F_UID_GID			(1 << 3)
> -#define NFQA_CFG_F_MAX				(1 << 4)
> +#define NFQA_CFG_F_SECCTX			(1 << 4)
> +#define NFQA_CFG_F_MAX				(1 << 5)
>  
>  /* flags for NFQA_SKB_INFO */
>  /* packet appears to have wrong checksums, but they are ok */
> diff --git a/net/netfilter/nfnetlink_queue_core.c b/net/netfilter/nfnetlink_queue_core.c
> index 0b98c74..2c35112 100644
> --- a/net/netfilter/nfnetlink_queue_core.c
> +++ b/net/netfilter/nfnetlink_queue_core.c
> @@ -278,6 +278,24 @@ nla_put_failure:
>  	return -1;
>  }
>  
> +static u32 nfqnl_get_sk_secctx(struct sk_buff *skb, char **secdata)
> +{
> +	u32 seclen = 0;

place

#if IS_ENABLED(CONFIG_NETWORK_SECMARK)

here?

I also think it makes sense to reject NFQA_CFG_F_SECCTX config flag in
nfqnl_recv_config() when IS_ENABLED(CONFIG_NETWORK_SECMARK) is not set;
i'd suggest to return EOPNOTSUPP in that case.

Re: [PATCH v3] nfnetlink_queue: add security context information

[Roman Kubiak]

Fixes applied (i'm not sure how i should use the mask parameter to check if NFQA_CFG_F_SECCTX is set
on the userspace side both the mask and the flag are the same value when i looked at how uid/gid
are set, maybe this could be done better?)

-- cut here

This patch adds an additional attribute when sending
packet information via netlink in netfilter_queue module.
It will send additional security context data, so that
userspace applications can verify this context against
their own security databases.

Signed-off-by: Roman Kubiak <r.kubiak@samsung.com>
---
v2:
- nfqnl_get_sk_secctx returns seclen now, this changes
- updated size calculation
- changed NFQA_SECCTX comment
- removed duplicate testing of NFQA_CFG_F flags

v3:
- NULL is not added to the security context anymore
- return 0 when socket is invalid in nfqnl_get_sk_secctx
- small intent change
- removed ret variable in nfqnl_get_sk_secctx

v4:
- removed security dependency, this patch does not
  require any changes in other subsystems
- nfqnl_get_sk_secctx returns seclen
- added IFDEF when using secmark from the sk_buff
  structure

v5:
- added a check to disable security context sending
  if CONFIG_NETWORK_SECMARK is not set
---
---
 include/uapi/linux/netfilter/nfnetlink_queue.h |  4 ++-
 net/netfilter/nfnetlink_queue_core.c           | 35 +++++++++++++++++++++++++-
 2 files changed, 37 insertions(+), 2 deletions(-)

diff --git a/include/uapi/linux/netfilter/nfnetlink_queue.h b/include/uapi/linux/netfilter/nfnetlink_queue.h
index 8dd819e..b67a853 100644
--- a/include/uapi/linux/netfilter/nfnetlink_queue.h
+++ b/include/uapi/linux/netfilter/nfnetlink_queue.h
@@ -49,6 +49,7 @@ enum nfqnl_attr_type {
 	NFQA_EXP,			/* nf_conntrack_netlink.h */
 	NFQA_UID,			/* __u32 sk uid */
 	NFQA_GID,			/* __u32 sk gid */
+	NFQA_SECCTX,			/* security context string */
 
 	__NFQA_MAX
 };
@@ -102,7 +103,8 @@ enum nfqnl_attr_config {
 #define NFQA_CFG_F_CONNTRACK			(1 << 1)
 #define NFQA_CFG_F_GSO				(1 << 2)
 #define NFQA_CFG_F_UID_GID			(1 << 3)
-#define NFQA_CFG_F_MAX				(1 << 4)
+#define NFQA_CFG_F_SECCTX			(1 << 4)
+#define NFQA_CFG_F_MAX				(1 << 5)
 
 /* flags for NFQA_SKB_INFO */
 /* packet appears to have wrong checksums, but they are ok */
diff --git a/net/netfilter/nfnetlink_queue_core.c b/net/netfilter/nfnetlink_queue_core.c
index 0b98c74..380aac6 100644
--- a/net/netfilter/nfnetlink_queue_core.c
+++ b/net/netfilter/nfnetlink_queue_core.c
@@ -278,6 +278,23 @@ nla_put_failure:
 	return -1;
 }
 
+static u32 nfqnl_get_sk_secctx(struct sk_buff *skb, char **secdata)
+{
+	u32 seclen = 0;
+#if IS_ENABLED(CONFIG_NETWORK_SECMARK)
+	if (!skb || !sk_fullsock(skb->sk))
+		return 0;
+
+	read_lock_bh(&skb->sk->sk_callback_lock);
+
+	if (skb->secmark)
+		security_secid_to_secctx(skb->secmark, secdata, &seclen);
+
+	read_unlock_bh(&skb->sk->sk_callback_lock);
+#endif
+	return seclen;
+}
+
 static struct sk_buff *
 nfqnl_build_packet_message(struct net *net, struct nfqnl_instance *queue,
 			   struct nf_queue_entry *entry,
@@ -297,6 +314,8 @@ nfqnl_build_packet_message(struct net *net, struct nfqnl_instance *queue,
 	struct nf_conn *ct = NULL;
 	enum ip_conntrack_info uninitialized_var(ctinfo);
 	bool csum_verify;
+	char *secdata = NULL;
+	u32 seclen = 0;
 
 	size =    nlmsg_total_size(sizeof(struct nfgenmsg))
 		+ nla_total_size(sizeof(struct nfqnl_msg_packet_hdr))
@@ -352,6 +371,12 @@ nfqnl_build_packet_message(struct net *net, struct nfqnl_instance *queue,
 			+ nla_total_size(sizeof(u_int32_t)));	/* gid */
 	}
 
+	if ((queue->flags & NFQA_CFG_F_SECCTX) && entskb->sk) {
+		seclen = nfqnl_get_sk_secctx(entskb, &secdata);
+		if (seclen)
+			size += nla_total_size(seclen);
+	}
+
 	skb = nfnetlink_alloc_skb(net, size, queue->peer_portid,
 				  GFP_ATOMIC);
 	if (!skb) {
@@ -479,6 +504,9 @@ nfqnl_build_packet_message(struct net *net, struct nfqnl_instance *queue,
 	    nfqnl_put_sk_uidgid(skb, entskb->sk) < 0)
 		goto nla_put_failure;
 
+	if (seclen && nla_put(skb, NFQA_SECCTX, seclen, secdata))
+		goto nla_put_failure;
+
 	if (ct && nfqnl_ct_put(skb, ct, ctinfo) < 0)
 		goto nla_put_failure;
 
@@ -1142,7 +1170,12 @@ nfqnl_recv_config(struct sock *ctnl, struct sk_buff *skb,
 			ret = -EOPNOTSUPP;
 			goto err_out_unlock;
 		}
-
+#if !IS_ENABLED(CONFIG_NETWORK_SECMARK)
+		if (flags == NFQA_CFG_F_SECCTX) {
+			ret = -EOPNOTSUPP;
+			goto err_out_unlock;
+		}
+#endif
 		spin_lock_bh(&queue->lock);
 		queue->flags &= ~mask;
 		queue->flags |= flags & mask;
-- 
1.9.1



On 06/10/2015 06:05 PM, Florian Westphal wrote:
> Roman Kubiak <r.kubiak@samsung.com> wrote:
>> diff --git a/include/uapi/linux/netfilter/nfnetlink_queue.h b/include/uapi/linux/netfilter/nfnetlink_queue.h
>> index 8dd819e..b67a853 100644
>> --- a/include/uapi/linux/netfilter/nfnetlink_queue.h
>> +++ b/include/uapi/linux/netfilter/nfnetlink_queue.h
>> @@ -49,6 +49,7 @@ enum nfqnl_attr_type {
>>  	NFQA_EXP,			/* nf_conntrack_netlink.h */
>>  	NFQA_UID,			/* __u32 sk uid */
>>  	NFQA_GID,			/* __u32 sk gid */
>> +	NFQA_SECCTX,			/* security context string */
>>  
>>  	__NFQA_MAX
>>  };
>> @@ -102,7 +103,8 @@ enum nfqnl_attr_config {
>>  #define NFQA_CFG_F_CONNTRACK			(1 << 1)
>>  #define NFQA_CFG_F_GSO				(1 << 2)
>>  #define NFQA_CFG_F_UID_GID			(1 << 3)
>> -#define NFQA_CFG_F_MAX				(1 << 4)
>> +#define NFQA_CFG_F_SECCTX			(1 << 4)
>> +#define NFQA_CFG_F_MAX				(1 << 5)
>>  
>>  /* flags for NFQA_SKB_INFO */
>>  /* packet appears to have wrong checksums, but they are ok */
>> diff --git a/net/netfilter/nfnetlink_queue_core.c b/net/netfilter/nfnetlink_queue_core.c
>> index 0b98c74..2c35112 100644
>> --- a/net/netfilter/nfnetlink_queue_core.c
>> +++ b/net/netfilter/nfnetlink_queue_core.c
>> @@ -278,6 +278,24 @@ nla_put_failure:
>>  	return -1;
>>  }
>>  
>> +static u32 nfqnl_get_sk_secctx(struct sk_buff *skb, char **secdata)
>> +{
>> +	u32 seclen = 0;
> 
> place
> 
> #if IS_ENABLED(CONFIG_NETWORK_SECMARK)
> 
> here?
> 
> I also think it makes sense to reject NFQA_CFG_F_SECCTX config flag in
> nfqnl_recv_config() when IS_ENABLED(CONFIG_NETWORK_SECMARK) is not set;
> i'd suggest to return EOPNOTSUPP in that case.
> 

-- 
--------------
 Roman Kubiak
--------------

Re: [PATCH v3] nfnetlink_queue: add security context information

[Florian Westphal]

Roman Kubiak <r.kubiak@samsung.com> wrote:
> Fixes applied (i'm not sure how i should use the mask parameter to check if NFQA_CFG_F_SECCTX is set
> on the userspace side both the mask and the flag are the same value when i looked at how uid/gid
> are set, maybe this could be done better?)

[..]

> @@ -1142,7 +1170,12 @@ nfqnl_recv_config(struct sock *ctnl, struct sk_buff *skb,
>  			ret = -EOPNOTSUPP;
>  			goto err_out_unlock;
>  		}
> -
> +#if !IS_ENABLED(CONFIG_NETWORK_SECMARK)
> +		if (flags == NFQA_CFG_F_SECCTX) {
> +			ret = -EOPNOTSUPP;
> +			goto err_out_unlock;
> +		}
> +#endif
>  		spin_lock_bh(&queue->lock);
>  		queue->flags &= ~mask;
>  		queue->flags |= flags & mask;

Based on last two lines it appears the test should be something like

if (flags & mask & NFQA_CFG_F_SECCTX)
	return -EOPNOTSUPP;

[ seems intent is to allow unsetting some flag(s) via
  flags = SOME_FEAT_I_WANT;
  mask = SOME_FEAT_I_WANT|SOME_FEAT_I_WANT_TO_SWITCH_OFF; ]

Re: [PATCH v3] nfnetlink_queue: add security context information

[Roman Kubiak]

This way works and seems sensible (i tested it)

a fixed patch below

-- cut here

This patch adds an additional attribute when sending
packet information via netlink in netfilter_queue module.
It will send additional security context data, so that
userspace applications can verify this context against
their own security databases.

Signed-off-by: Roman Kubiak <r.kubiak@samsung.com>
---
v2:
- nfqnl_get_sk_secctx returns seclen now, this changes
- updated size calculation
- changed NFQA_SECCTX comment
- removed duplicate testing of NFQA_CFG_F flags

v3:
- NULL is not added to the security context anymore
- return 0 when socket is invalid in nfqnl_get_sk_secctx
- small intent change
- removed ret variable in nfqnl_get_sk_secctx

v4:
- removed security dependency, this patch does not
  require any changes in other subsystems
- nfqnl_get_sk_secctx returns seclen
- added IFDEF when using secmark from the sk_buff
  structure

v5:
- added a check to disable security context sending
  if CONFIG_NETWORK_SECMARK is not set

v6:
- changed the way flags and mask are checked in
  nfqnl_recv_config

---
---
 include/uapi/linux/netfilter/nfnetlink_queue.h |  4 ++-
 net/netfilter/nfnetlink_queue_core.c           | 35 +++++++++++++++++++++++++-
 2 files changed, 37 insertions(+), 2 deletions(-)

diff --git a/include/uapi/linux/netfilter/nfnetlink_queue.h b/include/uapi/linux/netfilter/nfnetlink_queue.h
index 8dd819e..b67a853 100644
--- a/include/uapi/linux/netfilter/nfnetlink_queue.h
+++ b/include/uapi/linux/netfilter/nfnetlink_queue.h
@@ -49,6 +49,7 @@ enum nfqnl_attr_type {
 	NFQA_EXP,			/* nf_conntrack_netlink.h */
 	NFQA_UID,			/* __u32 sk uid */
 	NFQA_GID,			/* __u32 sk gid */
+	NFQA_SECCTX,			/* security context string */
 
 	__NFQA_MAX
 };
@@ -102,7 +103,8 @@ enum nfqnl_attr_config {
 #define NFQA_CFG_F_CONNTRACK			(1 << 1)
 #define NFQA_CFG_F_GSO				(1 << 2)
 #define NFQA_CFG_F_UID_GID			(1 << 3)
-#define NFQA_CFG_F_MAX				(1 << 4)
+#define NFQA_CFG_F_SECCTX			(1 << 4)
+#define NFQA_CFG_F_MAX				(1 << 5)
 
 /* flags for NFQA_SKB_INFO */
 /* packet appears to have wrong checksums, but they are ok */
diff --git a/net/netfilter/nfnetlink_queue_core.c b/net/netfilter/nfnetlink_queue_core.c
index 0b98c74..8c3f653 100644
--- a/net/netfilter/nfnetlink_queue_core.c
+++ b/net/netfilter/nfnetlink_queue_core.c
@@ -278,6 +278,23 @@ nla_put_failure:
 	return -1;
 }
 
+static u32 nfqnl_get_sk_secctx(struct sk_buff *skb, char **secdata)
+{
+	u32 seclen = 0;
+#if IS_ENABLED(CONFIG_NETWORK_SECMARK)
+	if (!skb || !sk_fullsock(skb->sk))
+		return 0;
+
+	read_lock_bh(&skb->sk->sk_callback_lock);
+
+	if (skb->secmark)
+		security_secid_to_secctx(skb->secmark, secdata, &seclen);
+
+	read_unlock_bh(&skb->sk->sk_callback_lock);
+#endif
+	return seclen;
+}
+
 static struct sk_buff *
 nfqnl_build_packet_message(struct net *net, struct nfqnl_instance *queue,
 			   struct nf_queue_entry *entry,
@@ -297,6 +314,8 @@ nfqnl_build_packet_message(struct net *net, struct nfqnl_instance *queue,
 	struct nf_conn *ct = NULL;
 	enum ip_conntrack_info uninitialized_var(ctinfo);
 	bool csum_verify;
+	char *secdata = NULL;
+	u32 seclen = 0;
 
 	size =    nlmsg_total_size(sizeof(struct nfgenmsg))
 		+ nla_total_size(sizeof(struct nfqnl_msg_packet_hdr))
@@ -352,6 +371,12 @@ nfqnl_build_packet_message(struct net *net, struct nfqnl_instance *queue,
 			+ nla_total_size(sizeof(u_int32_t)));	/* gid */
 	}
 
+	if ((queue->flags & NFQA_CFG_F_SECCTX) && entskb->sk) {
+		seclen = nfqnl_get_sk_secctx(entskb, &secdata);
+		if (seclen)
+			size += nla_total_size(seclen);
+	}
+
 	skb = nfnetlink_alloc_skb(net, size, queue->peer_portid,
 				  GFP_ATOMIC);
 	if (!skb) {
@@ -479,6 +504,9 @@ nfqnl_build_packet_message(struct net *net, struct nfqnl_instance *queue,
 	    nfqnl_put_sk_uidgid(skb, entskb->sk) < 0)
 		goto nla_put_failure;
 
+	if (seclen && nla_put(skb, NFQA_SECCTX, seclen, secdata))
+		goto nla_put_failure;
+
 	if (ct && nfqnl_ct_put(skb, ct, ctinfo) < 0)
 		goto nla_put_failure;
 
@@ -1142,7 +1170,12 @@ nfqnl_recv_config(struct sock *ctnl, struct sk_buff *skb,
 			ret = -EOPNOTSUPP;
 			goto err_out_unlock;
 		}
-
+#if !IS_ENABLED(CONFIG_NETWORK_SECMARK)
+		if (flags & mask & NFQA_CFG_F_SECCTX) {
+			ret = -EOPNOTSUPP;
+			goto err_out_unlock;
+		}
+#endif
 		spin_lock_bh(&queue->lock);
 		queue->flags &= ~mask;
 		queue->flags |= flags & mask;
-- 
1.9.1


On 06/12/2015 01:37 AM, Florian Westphal wrote:
> Roman Kubiak <r.kubiak@samsung.com> wrote:
>> Fixes applied (i'm not sure how i should use the mask parameter to check if NFQA_CFG_F_SECCTX is set
>> on the userspace side both the mask and the flag are the same value when i looked at how uid/gid
>> are set, maybe this could be done better?)
> 
> [..]
> 
>> @@ -1142,7 +1170,12 @@ nfqnl_recv_config(struct sock *ctnl, struct sk_buff *skb,
>>  			ret = -EOPNOTSUPP;
>>  			goto err_out_unlock;
>>  		}
>> -
>> +#if !IS_ENABLED(CONFIG_NETWORK_SECMARK)
>> +		if (flags == NFQA_CFG_F_SECCTX) {
>> +			ret = -EOPNOTSUPP;
>> +			goto err_out_unlock;
>> +		}
>> +#endif
>>  		spin_lock_bh(&queue->lock);
>>  		queue->flags &= ~mask;
>>  		queue->flags |= flags & mask;
> 
> Based on last two lines it appears the test should be something like
> 
> if (flags & mask & NFQA_CFG_F_SECCTX)
> 	return -EOPNOTSUPP;
> 
> [ seems intent is to allow unsetting some flag(s) via
>   flags = SOME_FEAT_I_WANT;
>   mask = SOME_FEAT_I_WANT|SOME_FEAT_I_WANT_TO_SWITCH_OFF; ]
> 
> 

-- 
--------------
 Roman Kubiak
--------------

Re: [PATCH v3] nfnetlink_queue: add security context information

[Florian Westphal]

Roman Kubiak <r.kubiak@samsung.com> wrote:
> This patch adds an additional attribute when sending
> packet information via netlink in netfilter_queue module.
> It will send additional security context data, so that
> userspace applications can verify this context against
> their own security databases.
> 
> Signed-off-by: Roman Kubiak <r.kubiak@samsung.com>

Looks good to me.

Acked-by: Florian Westphal <fw@strlen.de>

Re: [PATCH v3] nfnetlink_queue: add security context informationg

[Pablo Neira Ayuso]

On Fri, Jun 12, 2015 at 12:32:57PM +0200, Roman Kubiak wrote:
> This way works and seems sensible (i tested it)
> 
> a fixed patch below
> 
> -- cut here
> 
> This patch adds an additional attribute when sending
> packet information via netlink in netfilter_queue module.
> It will send additional security context data, so that
> userspace applications can verify this context against
> their own security databases.

Please, send the corresponding userspace updates for this. Thanks.

[PATCH] libmnl: security context retrieval in nf-queue example

[Roman Kubiak]

This patch is an addition to "[PATCH v3] nfnetlink_queue: add security context information"
It adds and example to libmnl that illustrates how to fetch security context.
A corresponding patch was sent for libnetfilter_queue already.

-- cut here

This patch modifies the example program for nf-queue
to demonstrate how to retriece security context information
for queued packages. This can also be easily extended to
retrieve other information supported by this subsystem.

Signed-off-by: Roman Kubiak <r.kubiak@samsung.com>
---
 examples/netfilter/nf-queue.c | 43 ++++++++++++++++++++++++++++++++++++++++---
 1 file changed, 40 insertions(+), 3 deletions(-)

diff --git a/examples/netfilter/nf-queue.c b/examples/netfilter/nf-queue.c
index 957e365..adafbed 100644
--- a/examples/netfilter/nf-queue.c
+++ b/examples/netfilter/nf-queue.c
@@ -21,7 +21,7 @@ static int parse_attr_cb(const struct nlattr *attr, void *data)
 		return MNL_CB_OK;
 
 	switch(type) {
-	case NFQA_MARK:
+	case NFQA_SECCTX:
 	case NFQA_IFINDEX_INDEV:
 	case NFQA_IFINDEX_OUTDEV:
 	case NFQA_IFINDEX_PHYSINDEV:
@@ -56,17 +56,25 @@ static int queue_cb(const struct nlmsghdr *nlh, void *data)
 {
 	struct nlattr *tb[NFQA_MAX+1] = {};
 	struct nfqnl_msg_packet_hdr *ph = NULL;
-	uint32_t id = 0;
+	uint32_t id = 0, seclen = 0;
+	const char *secctx = NULL;
 
 	mnl_attr_parse(nlh, sizeof(struct nfgenmsg), parse_attr_cb, tb);
 	if (tb[NFQA_PACKET_HDR]) {
 		ph = mnl_attr_get_payload(tb[NFQA_PACKET_HDR]);
 		id = ntohl(ph->packet_id);
 
-		printf("packet received (id=%u hw=0x%04x hook=%u)\n",
+		printf("packet received (id=%u hw=0x%04x hook=%u",
 		       id, ntohs(ph->hw_protocol), ph->hook);
 	}
 
+	if (tb[NFQA_SECCTX]) {
+		seclen = mnl_attr_get_payload_len(tb[NFQA_SECCTX]);
+		secctx = mnl_attr_get_str(tb[NFQA_SECCTX]);
+		printf(" secctx=%.*s", seclen, secctx);
+	}
+
+	printf(")\n");
 	return MNL_CB_OK + id;
 }
 
@@ -112,6 +120,27 @@ nfq_build_cfg_request(char *buf, uint8_t command, int queue_num)
 }
 
 static struct nlmsghdr *
+nfq_build_cfg_flags(char *buf, uint32_t mask, uint32_t flags, int queue_num)
+{
+	struct nlmsghdr *nlh = mnl_nlmsg_put_header(buf);
+	nlh->nlmsg_type	= (NFNL_SUBSYS_QUEUE << 8) | NFQNL_MSG_CONFIG;
+	nlh->nlmsg_flags = NLM_F_REQUEST;
+
+	struct nfgenmsg *nfg = mnl_nlmsg_put_extra_header(nlh, sizeof(*nfg));
+	nfg->nfgen_family = AF_UNSPEC;
+	nfg->version = NFNETLINK_V0;
+	nfg->res_id = htons(queue_num);
+
+	mask = htonl(mask);
+	flags = htonl(flags);
+
+	mnl_attr_put_u32(nlh, NFQA_CFG_FLAGS, flags);
+	mnl_attr_put_u32(nlh, NFQA_CFG_MASK, mask);
+
+	return nlh;
+}
+
+static struct nlmsghdr *
 nfq_build_cfg_params(char *buf, uint8_t mode, int range, int queue_num)
 {
 	struct nlmsghdr *nlh = mnl_nlmsg_put_header(buf);
@@ -209,6 +238,14 @@ int main(int argc, char *argv[])
 		exit(EXIT_FAILURE);
 	}
 
+	nlh = nfq_build_cfg_flags(buf, NFQA_CFG_F_SECCTX,
+					NFQA_CFG_F_SECCTX, queue_num);
+
+	if (mnl_socket_sendto(nl, nlh, nlh->nlmsg_len) < 0) {
+		perror("mnl_socket_sendto");
+		exit(EXIT_FAILURE);
+	}
+
 	ret = mnl_socket_recvfrom(nl, buf, sizeof(buf));
 	if (ret == -1) {
 		perror("mnl_socket_recvfrom");
-- 
2.0.1


On 06/12/2015 03:02 PM, Pablo Neira Ayuso wrote:
> On Fri, Jun 12, 2015 at 12:32:57PM +0200, Roman Kubiak wrote:
>> This way works and seems sensible (i tested it)
>>
>> a fixed patch below
>>
>> -- cut here
>>
>> This patch adds an additional attribute when sending
>> packet information via netlink in netfilter_queue module.
>> It will send additional security context data, so that
>> userspace applications can verify this context against
>> their own security databases.
> 
> Please, send the corresponding userspace updates for this. Thanks.
> --
> 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
> 

-- 
--------------
 Roman Kubiak
--------------

Re: [PATCH] libmnl: security context retrieval in nf-queue example

[Pablo Neira Ayuso]

On Tue, Jun 16, 2015 at 02:25:13PM +0200, Roman Kubiak wrote:
> This patch is an addition to "[PATCH v3] nfnetlink_queue: add security context information"
> It adds and example to libmnl that illustrates how to fetch security context.
> A corresponding patch was sent for libnetfilter_queue already.
> 
> -- cut here
> 
> This patch modifies the example program for nf-queue
> to demonstrate how to retriece security context information
> for queued packages. This can also be easily extended to
> retrieve other information supported by this subsystem.

This extension for the libmnl example is fine.

However, when we asked for changes, we actually mean that you enhance:

        libnetfilter_queue/src/nlmsg.c

to support this. Thanks.

Re: [PATCH] libmnl: security context retrieval in nf-queue example

[Roman Kubiak]

It seems that there is nothing i can really add to there except maybe:
diff --git a/src/nlmsg.c b/src/nlmsg.c
index aebdd5e..cabd8be 100644
--- a/src/nlmsg.c
+++ b/src/nlmsg.c
@@ -137,6 +137,7 @@ static int nfq_pkt_parse_attr_cb(const struct nlattr *attr, void *data)
        case NFQA_IFINDEX_PHYSOUTDEV:
        case NFQA_CAP_LEN:
        case NFQA_SKB_INFO:
+       case NFQA_SECCTX:
        case NFQA_UID:
        case NFQA_GID:
                if (mnl_attr_validate(attr, MNL_TYPE_U32) < 0)

but that's a one line, will this be sufficient ? if so i'll merge this with the previous libnetfilter_queue patch
and send it as one.

On 06/16/2015 02:37 PM, Pablo Neira Ayuso wrote:
> On Tue, Jun 16, 2015 at 02:25:13PM +0200, Roman Kubiak wrote:
>> This patch is an addition to "[PATCH v3] nfnetlink_queue: add security context information"
>> It adds and example to libmnl that illustrates how to fetch security context.
>> A corresponding patch was sent for libnetfilter_queue already.
>>
>> -- cut here
>>
>> This patch modifies the example program for nf-queue
>> to demonstrate how to retriece security context information
>> for queued packages. This can also be easily extended to
>> retrieve other information supported by this subsystem.
> 
> This extension for the libmnl example is fine.
> 
> However, when we asked for changes, we actually mean that you enhance:
> 
>         libnetfilter_queue/src/nlmsg.c
> 
> to support this. Thanks.
> 
> 

-- 
--------------
 Roman Kubiak
--------------

Re: [PATCH] libmnl: security context retrieval in nf-queue example

[Pablo Neira Ayuso]

On Tue, Jun 16, 2015 at 02:58:50PM +0200, Roman Kubiak wrote:
> It seems that there is nothing i can really add to there except maybe:

Please, send me Signed-off-by patch with title and description that I
can apply via git am. Thank you.

Re: [PATCH] libmnl: security context retrieval in nf-queue example

[Roman Kubiak]

Below is a complete libnetfilter_queue patch:


[PATCH] libnetfilter_queue: add security context information

This commit adds security context information structures
and functions.

This will allow userspace to find the security context of each
packet (if it exists) and make decisions based on that.
It should work for SELinux and SMACK.

Signed-off-by: Roman Kubiak <r.kubiak@samsung.com>
---
 include/libnetfilter_queue/libnetfilter_queue.h    |  2 ++
 include/libnetfilter_queue/linux_nfnetlink_queue.h |  4 +++-
 include/linux/netfilter/nfnetlink_queue.h          |  4 +++-
 src/libnetfilter_queue.c                           | 23 ++++++++++++++++++++++
 src/nlmsg.c                                        |  1 +
 utils/nfqnl_test.c                                 | 14 +++++++++++--
 6 files changed, 44 insertions(+), 4 deletions(-)

diff --git a/include/libnetfilter_queue/libnetfilter_queue.h b/include/libnetfilter_queue/libnetfilter_queue.h
index 8194a4f..d7734e6 100644
--- a/include/libnetfilter_queue/libnetfilter_queue.h
+++ b/include/libnetfilter_queue/libnetfilter_queue.h
@@ -105,6 +105,7 @@ extern uint32_t nfq_get_outdev(struct nfq_data *nfad);
 extern uint32_t nfq_get_physoutdev(struct nfq_data *nfad);
 extern int nfq_get_uid(struct nfq_data *nfad, uint32_t *uid);
 extern int nfq_get_gid(struct nfq_data *nfad, uint32_t *gid);
+extern int nfq_get_secctx(struct nfq_data *nfad, unsigned char **secdata);
 
 extern int nfq_get_indev_name(struct nlif_handle *nlif_handle,
 			      struct nfq_data *nfad, char *name);
@@ -129,6 +130,7 @@ enum {
 	NFQ_XML_TIME	= (1 << 5),
 	NFQ_XML_UID	= (1 << 6),
 	NFQ_XML_GID	= (1 << 7),
+	NFQ_XML_SECCTX  = (1 << 8),
 	NFQ_XML_ALL	= ~0U,
 };
 
diff --git a/include/libnetfilter_queue/linux_nfnetlink_queue.h b/include/libnetfilter_queue/linux_nfnetlink_queue.h
index 5b6ae95..1975dfa 100644
--- a/include/libnetfilter_queue/linux_nfnetlink_queue.h
+++ b/include/libnetfilter_queue/linux_nfnetlink_queue.h
@@ -53,6 +53,7 @@ enum nfqnl_attr_type {
 	NFQA_EXP,			/* nf_conntrack_netlink.h */
 	NFQA_UID,			/* __u32 sk uid */
 	NFQA_GID,			/* __u32 sk gid */
+	NFQA_SECCTX,			/* security context string */
 
 	__NFQA_MAX
 };
@@ -106,7 +107,8 @@ enum nfqnl_attr_config {
 #define NFQA_CFG_F_CONNTRACK			(1 << 1)
 #define NFQA_CFG_F_GSO				(1 << 2)
 #define NFQA_CFG_F_UID_GID			(1 << 3)
-#define NFQA_CFG_F_MAX				(1 << 4)
+#define NFQA_CFG_F_SECCTX			(1 << 4)
+#define NFQA_CFG_F_MAX				(1 << 5)
 
 /* flags for NFQA_SKB_INFO */
 /* packet appears to have wrong checksums, but they are ok */
diff --git a/include/linux/netfilter/nfnetlink_queue.h b/include/linux/netfilter/nfnetlink_queue.h
index 22f5d45..030672d 100644
--- a/include/linux/netfilter/nfnetlink_queue.h
+++ b/include/linux/netfilter/nfnetlink_queue.h
@@ -49,6 +49,7 @@ enum nfqnl_attr_type {
 	NFQA_EXP,			/* nf_conntrack_netlink.h */
 	NFQA_UID,			/* __u32 sk uid */
 	NFQA_GID,			/* __u32 sk gid */
+	NFQA_SECCTX,
 
 	__NFQA_MAX
 };
@@ -102,7 +103,8 @@ enum nfqnl_attr_config {
 #define NFQA_CFG_F_CONNTRACK			(1 << 1)
 #define NFQA_CFG_F_GSO				(1 << 2)
 #define NFQA_CFG_F_UID_GID			(1 << 3)
-#define NFQA_CFG_F_MAX				(1 << 4)
+#define NFQA_CFG_F_SECCTX			(1 << 4)
+#define NFQA_CFG_F_MAX				(1 << 5)
 
 /* flags for NFQA_SKB_INFO */
 /* packet appears to have wrong checksums, but they are ok */
diff --git a/src/libnetfilter_queue.c b/src/libnetfilter_queue.c
index c9ed865..84184ee 100644
--- a/src/libnetfilter_queue.c
+++ b/src/libnetfilter_queue.c
@@ -1218,6 +1218,29 @@ int nfq_get_gid(struct nfq_data *nfad, uint32_t *gid)
 }
 EXPORT_SYMBOL(nfq_get_gid);
 
+
+/**
+ * nfq_get_secctx - get the security context for this packet
+ * \param nfad Netlink packet data handle passed to callback function
+ * \param secdata data to write the security context to
+ *
+ * \return -1 on error, otherwise > 0
+ */
+int nfq_get_secctx(struct nfq_data *nfad, unsigned char **secdata)
+{
+	if (!nfnl_attr_present(nfad->data, NFQA_SECCTX))
+		return -1;
+
+	*secdata = (unsigned char *)nfnl_get_pointer_to_data(nfad->data,
+							NFQA_SECCTX, char);
+
+	if (*secdata)
+		return NFA_PAYLOAD(nfad->data[NFQA_SECCTX-1]);
+
+	return 0;
+}
+EXPORT_SYMBOL(nfq_get_secctx);
+
 /**
  * nfq_get_payload - get payload 
  * \param nfad Netlink packet data handle passed to callback function
diff --git a/src/nlmsg.c b/src/nlmsg.c
index aebdd5e..cabd8be 100644
--- a/src/nlmsg.c
+++ b/src/nlmsg.c
@@ -137,6 +137,7 @@ static int nfq_pkt_parse_attr_cb(const struct nlattr *attr, void *data)
 	case NFQA_IFINDEX_PHYSOUTDEV:
 	case NFQA_CAP_LEN:
 	case NFQA_SKB_INFO:
+	case NFQA_SECCTX:
 	case NFQA_UID:
 	case NFQA_GID:
 		if (mnl_attr_validate(attr, MNL_TYPE_U32) < 0)
diff --git a/utils/nfqnl_test.c b/utils/nfqnl_test.c
index b760cf0..783bc6c 100644
--- a/utils/nfqnl_test.c
+++ b/utils/nfqnl_test.c
@@ -17,7 +17,7 @@ static uint32_t print_pkt (struct nfq_data *tb)
 	struct nfqnl_msg_packet_hw *hwph;
 	uint32_t mark, ifi, uid, gid;
 	int ret;
-	unsigned char *data;
+	unsigned char *data, *secdata;
 
 	ph = nfq_get_msg_packet_hdr(tb);
 	if (ph) {
@@ -61,6 +61,10 @@ static uint32_t print_pkt (struct nfq_data *tb)
 	if (nfq_get_gid(tb, &gid))
 		printf("gid=%u ", gid);
 
+	ret = nfq_get_secctx(tb, &secdata);
+	if (ret > 0)
+		printf("secctx=\"%.*s\" ", ret, secdata);
+
 	ret = nfq_get_payload(tb, &data);
 	if (ret >= 0)
 		printf("payload_len=%d ", ret);
@@ -76,7 +80,7 @@ static int cb(struct nfq_q_handle *qh, struct nfgenmsg *nfmsg,
 {
 	uint32_t id = print_pkt(nfa);
 	printf("entering callback\n");
-	return nfq_set_verdict(qh, id, NF_ACCEPT, 0, NULL);
+	return nfq_set_verdict2(qh, id, NF_ACCEPT, 0x3, 0, NULL);
 }
 
 int main(int argc, char **argv)
@@ -134,6 +138,12 @@ int main(int argc, char **argv)
 				"retrieve process UID/GID.\n");
 	}
 
+	printf("setting flags to request security context\n");
+	if (nfq_set_queue_flags(qh, NFQA_CFG_F_SECCTX, NFQA_CFG_F_SECCTX)) {
+		fprintf(stderr, "This kernel version does not allow to "
+				"retrieve security context.\n");
+	}
+
 	printf("Waiting for packets...\n");
 
 	fd = nfq_fd(h);
-- 
2.0.1

Re: [PATCH] libmnl: security context retrieval in nf-queue example

[Pablo Neira Ayuso]

On Tue, Jun 16, 2015 at 06:14:47PM +0200, Roman Kubiak wrote:
> Below is a complete libnetfilter_queue patch:
> 
> 
> [PATCH] libnetfilter_queue: add security context information
> 
> This commit adds security context information structures
> and functions.
> 
> This will allow userspace to find the security context of each
> packet (if it exists) and make decisions based on that.
> It should work for SELinux and SMACK.

Applied with minor glitch. Thanks.

> @@ -76,7 +80,7 @@ static int cb(struct nfq_q_handle *qh, struct nfgenmsg *nfmsg,
>  {
>  	uint32_t id = print_pkt(nfa);
>  	printf("entering callback\n");
> -	return nfq_set_verdict(qh, id, NF_ACCEPT, 0, NULL);
> +	return nfq_set_verdict2(qh, id, NF_ACCEPT, 0x3, 0, NULL);

I have kept back this chunk to set the packet mark to 3. It doesn't
belong here.

Re: [PATCH v3] nfnetlink_queue: add security context information

[Pablo Neira Ayuso]

On Fri, Jun 12, 2015 at 12:32:57PM +0200, Roman Kubiak wrote:
> This way works and seems sensible (i tested it)
> 
> a fixed patch below
> 
> -- cut here
> 
> This patch adds an additional attribute when sending
> packet information via netlink in netfilter_queue module.
> It will send additional security context data, so that
> userspace applications can verify this context against
> their own security databases.
> 
> Signed-off-by: Roman Kubiak <r.kubiak@samsung.com>
> ---
> v2:
> - nfqnl_get_sk_secctx returns seclen now, this changes
> - updated size calculation
> - changed NFQA_SECCTX comment
> - removed duplicate testing of NFQA_CFG_F flags
> 
> v3:
> - NULL is not added to the security context anymore
> - return 0 when socket is invalid in nfqnl_get_sk_secctx
> - small intent change
> - removed ret variable in nfqnl_get_sk_secctx
> 
> v4:
> - removed security dependency, this patch does not
>   require any changes in other subsystems
> - nfqnl_get_sk_secctx returns seclen
> - added IFDEF when using secmark from the sk_buff
>   structure
> 
> v5:
> - added a check to disable security context sending
>   if CONFIG_NETWORK_SECMARK is not set
> 
> v6:
> - changed the way flags and mask are checked in
>   nfqnl_recv_config

Applied this v6. Thank you.

Re: [PATCH v3] nfnetlink_queue: add security context information

[Florian Westphal]

Roman Kubiak <r.kubiak@samsung.com> wrote:
> This patch adds an additional attribute when sending
> packet information via netlink in netfilter_queue module.
> It will send additional security context data, so that
> userspace applications can verify this context against
> their own security databases.
> 
> Signed-off-by: Roman Kubiak <r.kubiak@samsung.com>
> ---
> v2:
> - nfqnl_get_sk_secctx returns seclen now, this changes
> - updated size calculation
> - changed NFQA_SECCTX comment
> - removed duplicate testing of NFQA_CFG_F flags
> 
> v3:
> - NULL is not added to the security context anymore
> - return 0 when socket is invalid in nfqnl_get_sk_secctx
> - small intent change
> - removed ret variable in nfqnl_get_sk_secctx
> ---
>  include/uapi/linux/netfilter/nfnetlink_queue.h |  4 +++-
>  net/netfilter/nfnetlink_queue_core.c           | 31 ++++++++++++++++++++++++++
>  2 files changed, 34 insertions(+), 1 deletion(-)
> 
> diff --git a/include/uapi/linux/netfilter/nfnetlink_queue.h b/include/uapi/linux/netfilter/nfnetlink_queue.h
> index 8dd819e..b67a853 100644
> --- a/include/uapi/linux/netfilter/nfnetlink_queue.h
> +++ b/include/uapi/linux/netfilter/nfnetlink_queue.h
> @@ -49,6 +49,7 @@ enum nfqnl_attr_type {
>  	NFQA_EXP,			/* nf_conntrack_netlink.h */
>  	NFQA_UID,			/* __u32 sk uid */
>  	NFQA_GID,			/* __u32 sk gid */
> +	NFQA_SECCTX,			/* security context string */
>  
>  	__NFQA_MAX
>  };
> @@ -102,7 +103,8 @@ enum nfqnl_attr_config {
>  #define NFQA_CFG_F_CONNTRACK			(1 << 1)
>  #define NFQA_CFG_F_GSO				(1 << 2)
>  #define NFQA_CFG_F_UID_GID			(1 << 3)
> -#define NFQA_CFG_F_MAX				(1 << 4)
> +#define NFQA_CFG_F_SECCTX			(1 << 4)
> +#define NFQA_CFG_F_MAX				(1 << 5)
>  
>  /* flags for NFQA_SKB_INFO */
>  /* packet appears to have wrong checksums, but they are ok */
> diff --git a/net/netfilter/nfnetlink_queue_core.c b/net/netfilter/nfnetlink_queue_core.c
> index 0b98c74..ae4f520 100644
> --- a/net/netfilter/nfnetlink_queue_core.c
> +++ b/net/netfilter/nfnetlink_queue_core.c
> @@ -278,6 +278,24 @@ nla_put_failure:
>  	return -1;
>  }
>  
> +static u32 nfqnl_get_sk_secctx(struct sock *sk, char **secdata)
> +{
> +	u32 secid = 0;
> +	u32 seclen = 0;
> +
> +	if (!sk || !sk_fullsock(sk))
> +		return 0;

if (!sk) is not needed anymore

>  	bool csum_verify;
> +	char *secdata = NULL;
> +	u32 seclen = 0;
>  
>  	size =    nlmsg_total_size(sizeof(struct nfgenmsg))
>  		+ nla_total_size(sizeof(struct nfqnl_msg_packet_hdr))
> @@ -352,6 +372,12 @@ nfqnl_build_packet_message(struct net *net, struct nfqnl_instance *queue,
>  			+ nla_total_size(sizeof(u_int32_t)));	/* gid */
>  	}
>  
> +	if ((queue->flags & NFQA_CFG_F_SECCTX) && entskb->sk) {

... or remove entskb->sk test here (I have no preference where
you test this, either caller or callee is fine with me).

>  	skb = nfnetlink_alloc_skb(net, size, queue->peer_portid,
>  				  GFP_ATOMIC);
>  	if (!skb) {
> @@ -479,6 +505,11 @@ nfqnl_build_packet_message(struct net *net, struct nfqnl_instance *queue,
>  	    nfqnl_put_sk_uidgid(skb, entskb->sk) < 0)
>  		goto nla_put_failure;
>  
> +	if (seclen) {
> +		if (nla_put(skb, NFQA_SECCTX, seclen, secdata))
> +			goto nla_put_failure;
> +	}
> +

Nit: can be written as

	if (seclen && nla_put(skb, NFQA_SECCTX, seclen, secdata))
		goto nla_put_failure;

Otherwise I think this looks good.  Please consider sending v4 _after_
security_sk_getsecid situation is resolved.

It would be good to note in the --- section in case there is a
dependency; we always want to avoid "it won't build (without change
x from tree y)."

Thanks.

Re: [PATCH v3] nfnetlink_queue: add security context information

[Roman Kubiak]

I have one more question, there is a field "secmark" in the sk_buff structure, i was wondering if i can
safely use it instead of adding security_sk_getsecid

I did some tests and the secmark revolves exactly to secid if security marking is turned on and smack
has the option "Packet marking using secmarks for netfilter/CONFIG_SECURITY_SMACK_NETFILTER:" turned on

The function smack does it in, seems like it's doing exactly what one would expect:
static unsigned int smack_ipv4_output(const struct nf_hook_ops *ops,
                                        struct sk_buff *skb,
                                        const struct nf_hook_state *state)
{
        struct socket_smack *ssp;
        struct smack_known *skp;

        if (skb && skb->sk && skb->sk->sk_security) {
                ssp = skb->sk->sk_security;
                skp = ssp->smk_out;
                skb->secmark = skp->smk_secid;
        }

        return NF_ACCEPT;
}
and it's registered as an op
static struct nf_hook_ops smack_nf_ops[] = {
        {
                .hook =         smack_ipv4_output,
                .owner =        THIS_MODULE,
                .pf =           NFPROTO_IPV4,
                .hooknum =      NF_INET_LOCAL_OUT,
                .priority =     NF_IP_PRI_SELINUX_FIRST,
        },

(the same is done for ipv6)

So would it be safe to use it ?

best regards

On 05/27/2015 01:48 PM, Florian Westphal wrote:
> Roman Kubiak <r.kubiak@samsung.com> wrote:
>> This patch adds an additional attribute when sending
>> packet information via netlink in netfilter_queue module.
>> It will send additional security context data, so that
>> userspace applications can verify this context against
>> their own security databases.
>>
>> Signed-off-by: Roman Kubiak <r.kubiak@samsung.com>
>> ---
>> v2:
>> - nfqnl_get_sk_secctx returns seclen now, this changes
>> - updated size calculation
>> - changed NFQA_SECCTX comment
>> - removed duplicate testing of NFQA_CFG_F flags
>>
>> v3:
>> - NULL is not added to the security context anymore
>> - return 0 when socket is invalid in nfqnl_get_sk_secctx
>> - small intent change
>> - removed ret variable in nfqnl_get_sk_secctx
>> ---
>>  include/uapi/linux/netfilter/nfnetlink_queue.h |  4 +++-
>>  net/netfilter/nfnetlink_queue_core.c           | 31 ++++++++++++++++++++++++++
>>  2 files changed, 34 insertions(+), 1 deletion(-)
>>
>> diff --git a/include/uapi/linux/netfilter/nfnetlink_queue.h b/include/uapi/linux/netfilter/nfnetlink_queue.h
>> index 8dd819e..b67a853 100644
>> --- a/include/uapi/linux/netfilter/nfnetlink_queue.h
>> +++ b/include/uapi/linux/netfilter/nfnetlink_queue.h
>> @@ -49,6 +49,7 @@ enum nfqnl_attr_type {
>>  	NFQA_EXP,			/* nf_conntrack_netlink.h */
>>  	NFQA_UID,			/* __u32 sk uid */
>>  	NFQA_GID,			/* __u32 sk gid */
>> +	NFQA_SECCTX,			/* security context string */
>>  
>>  	__NFQA_MAX
>>  };
>> @@ -102,7 +103,8 @@ enum nfqnl_attr_config {
>>  #define NFQA_CFG_F_CONNTRACK			(1 << 1)
>>  #define NFQA_CFG_F_GSO				(1 << 2)
>>  #define NFQA_CFG_F_UID_GID			(1 << 3)
>> -#define NFQA_CFG_F_MAX				(1 << 4)
>> +#define NFQA_CFG_F_SECCTX			(1 << 4)
>> +#define NFQA_CFG_F_MAX				(1 << 5)
>>  
>>  /* flags for NFQA_SKB_INFO */
>>  /* packet appears to have wrong checksums, but they are ok */
>> diff --git a/net/netfilter/nfnetlink_queue_core.c b/net/netfilter/nfnetlink_queue_core.c
>> index 0b98c74..ae4f520 100644
>> --- a/net/netfilter/nfnetlink_queue_core.c
>> +++ b/net/netfilter/nfnetlink_queue_core.c
>> @@ -278,6 +278,24 @@ nla_put_failure:
>>  	return -1;
>>  }
>>  
>> +static u32 nfqnl_get_sk_secctx(struct sock *sk, char **secdata)
>> +{
>> +	u32 secid = 0;
>> +	u32 seclen = 0;
>> +
>> +	if (!sk || !sk_fullsock(sk))
>> +		return 0;
> 
> if (!sk) is not needed anymore
> 
>>  	bool csum_verify;
>> +	char *secdata = NULL;
>> +	u32 seclen = 0;
>>  
>>  	size =    nlmsg_total_size(sizeof(struct nfgenmsg))
>>  		+ nla_total_size(sizeof(struct nfqnl_msg_packet_hdr))
>> @@ -352,6 +372,12 @@ nfqnl_build_packet_message(struct net *net, struct nfqnl_instance *queue,
>>  			+ nla_total_size(sizeof(u_int32_t)));	/* gid */
>>  	}
>>  
>> +	if ((queue->flags & NFQA_CFG_F_SECCTX) && entskb->sk) {
> 
> ... or remove entskb->sk test here (I have no preference where
> you test this, either caller or callee is fine with me).
> 
>>  	skb = nfnetlink_alloc_skb(net, size, queue->peer_portid,
>>  				  GFP_ATOMIC);
>>  	if (!skb) {
>> @@ -479,6 +505,11 @@ nfqnl_build_packet_message(struct net *net, struct nfqnl_instance *queue,
>>  	    nfqnl_put_sk_uidgid(skb, entskb->sk) < 0)
>>  		goto nla_put_failure;
>>  
>> +	if (seclen) {
>> +		if (nla_put(skb, NFQA_SECCTX, seclen, secdata))
>> +			goto nla_put_failure;
>> +	}
>> +
> 
> Nit: can be written as
> 
> 	if (seclen && nla_put(skb, NFQA_SECCTX, seclen, secdata))
> 		goto nla_put_failure;
> 
> Otherwise I think this looks good.  Please consider sending v4 _after_
> security_sk_getsecid situation is resolved.
> 
> It would be good to note in the --- section in case there is a
> dependency; we always want to avoid "it won't build (without change
> x from tree y)."
> 
> Thanks.
> 

-- 
--------------
 Roman Kubiak
--------------