Skip to contentSkip to conversation

Security context information added to netfilter_queue

PATCH In review Roman Kubiak 3 participants 26 messages May 25, 2015 – Jun 30, 2015

Subsystems (2)
Networking netfilter
lore
Share & tools
lore

Roman Kubiak

author
#0 lore raw

PATCH 1/1 Security context information added to netfilter_queue

Quoted 1 line

From dccc2ca387d7b4dd16fff537ce2cab280517cab5 Mon Sep 17 00:00:00 2001

From: Roman Kubiak <[email protected]>
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 <[email protected]>
---
 include/uapi/linux/netfilter/nfnetlink_queue.h |  4 ++-
 net/netfilter/nfnetlink_queue_core.c           | 46 ++++++++++++++++++++++++++
 2 files changed, 49 insertions(+), 1 deletion(-)
include/uapi/linux/netfilter/nfnetlink_queue.h +3 -1
index 8dd819e..313935a 100644
@@ -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 */
net/netfilter/nfnetlink_queue_core.c +46 -0

76 lines not on this page · raw message

Signature · 1 line
1.9.1
maintainer
#1 lore raw
Quoted 44 lines · Roman Kubiak

From dccc2ca387d7b4dd16fff537ce2cab280517cab5 Mon Sep 17 00:00:00 2001
From: Roman Kubiak <[email protected]>
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 <[email protected]>
---
 include/uapi/linux/netfilter/nfnetlink_queue.h |  4 ++-
 net/netfilter/nfnetlink_queue_core.c           | 46 ++++++++++++++++++++++++++
 2 files changed, 49 insertions(+), 1 deletion(-)
include/uapi/linux/netfilter/nfnetlink_queue.h +3 -1
index 8dd819e..313935a 100644
@@ -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 */
net/netfilter/nfnetlink_queue_core.c +0 -0
index 0b98c74..de3b97a 100644

net/netfilter/nfnetlink_queue_core.c nla_put_failure: · line 282

Quoted 5 lines
net/netfilter/nfnetlink_queue_core.c +2 -0
@@ -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.

Quoted 8 lines
net/netfilter/nfnetlink_queue_core.c +3 -0
 	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))

net/netfilter/nfnetlink_queue_core.c nfqnl_build_packet_message(struct net *net, struct nfqnl_instance *queue, · line 384

Quoted 9 lines
net/netfilter/nfnetlink_queue_core.c +6 -0
@@ -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);
}

net/netfilter/nfnetlink_queue_core.c nfqnl_build_packet_message(struct net *net, struct nfqnl_instance *queue, · line 513

Quoted 6 lines
net/netfilter/nfnetlink_queue_core.c +1 -0
 	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)

net/netfilter/nfnetlink_queue_core.c nfqnl_build_packet_message(struct net *net, struct nfqnl_instance *queue, · line 519

Quoted 6 lines
net/netfilter/nfnetlink_queue_core.c +6 -0
+		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?

net/netfilter/nfnetlink_queue_core.c nfqnl_build_packet_message(struct net *net, struct nfqnl_instance *queue, · line 522

Quoted 2 lines
net/netfilter/nfnetlink_queue_core.c +2 -0
+		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().

maintainer
#2 lore raw
In reply to Roman Kubiak
Quoted 10 lines · Roman Kubiak

From dccc2ca387d7b4dd16fff537ce2cab280517cab5 Mon Sep 17 00:00:00 2001
From: Roman Kubiak <[email protected]>
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.

Quoted 11 lines
Signed-off-by Roman Kubiak <[email protected]>
---
 include/uapi/linux/netfilter/nfnetlink_queue.h |  4 ++-
 net/netfilter/nfnetlink_queue_core.c           | 46 ++++++++++++++++++++++++++
 2 files changed, 49 insertions(+), 1 deletion(-)
include/uapi/linux/netfilter/nfnetlink_queue.h +0 -0
index 8dd819e..313935a 100644

include/uapi/linux/netfilter/nfnetlink_queue.h enum nfqnl_attr_type { · line 52

Quoted 4 lines
include/uapi/linux/netfilter/nfnetlink_queue.h +1 -0
@@ -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 */

Quoted 20 lines
include/uapi/linux/netfilter/nfnetlink_queue.h +2 -1
 	__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 */
net/netfilter/nfnetlink_queue_core.c +0 -0
index 0b98c74..de3b97a 100644
@@ -278,6 +278,27 @@ nla_put_failure:
 	return -1;
 }

net/netfilter/nfnetlink_queue_core.c nla_put_failure: · line 290

Quoted 10 lines
net/netfilter/nfnetlink_queue_core.c +10 -0
+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))
author
#3 lore raw

PATCH v2 1/1 nfnetlink_queue: add security context information

[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 <[email protected]>

---
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(-)
include/uapi/linux/netfilter/nfnetlink_queue.h +3 -1
index 8dd819e..b67a853 100644
@@ -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 */
net/netfilter/nfnetlink_queue_core.c +45 -0

75 lines not on this page · raw message

Signature · 1 line
1.9.1
Quoted 73 lines · Pablo Neira Ayuso
Quoted 10 lines · Roman Kubiak

From dccc2ca387d7b4dd16fff537ce2cab280517cab5 Mon Sep 17 00:00:00 2001
From: Roman Kubiak <[email protected]>
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.

Quoted 11 lines
Signed-off-by Roman Kubiak <[email protected]>
---
 include/uapi/linux/netfilter/nfnetlink_queue.h |  4 ++-
 net/netfilter/nfnetlink_queue_core.c           | 46 ++++++++++++++++++++++++++
 2 files changed, 49 insertions(+), 1 deletion(-)
include/uapi/linux/netfilter/nfnetlink_queue.h +0 -0
index 8dd819e..313935a 100644

include/uapi/linux/netfilter/nfnetlink_queue.h enum nfqnl_attr_type { · line 52

Quoted 4 lines
include/uapi/linux/netfilter/nfnetlink_queue.h +1 -0
@@ -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 */

Quoted 20 lines
include/uapi/linux/netfilter/nfnetlink_queue.h +2 -1
 	__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 */
net/netfilter/nfnetlink_queue_core.c +0 -0
index 0b98c74..de3b97a 100644
@@ -278,6 +278,27 @@ nla_put_failure:
 	return -1;
 }

net/netfilter/nfnetlink_queue_core.c nla_put_failure: · line 290

Quoted 10 lines
net/netfilter/nfnetlink_queue_core.c +10 -0
+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))
Signature · 3 lines
--------------
 Roman Kubiak
--------------
maintainer
#4 lore raw

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

Quoted 6 lines · 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)

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

net/netfilter/nfnetlink_queue_core.c nla_put_failure: · line 288

Quoted 8 lines
net/netfilter/nfnetlink_queue_core.c +8 -0
+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?

net/netfilter/nfnetlink_queue_core.c nfqnl_build_packet_message(struct net *net, struct nfqnl_instance *queue, · line 382

Quoted 4 lines
net/netfilter/nfnetlink_queue_core.c +4 -0
+	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.

author
#5 lore raw

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

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

Quoted 35 lines · Florian Westphal
Quoted 6 lines · 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)

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

net/netfilter/nfnetlink_queue_core.c nla_put_failure: · line 288

Quoted 8 lines
net/netfilter/nfnetlink_queue_core.c +8 -0
+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?

net/netfilter/nfnetlink_queue_core.c nfqnl_build_packet_message(struct net *net, struct nfqnl_instance *queue, · line 382

Quoted 4 lines
net/netfilter/nfnetlink_queue_core.c +4 -0
+	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 [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Signature · 3 lines
--------------
 Roman Kubiak
--------------
maintainer
#6 lore raw

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

Quoted 2 lines · 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 ?

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

Quoted 6 lines

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])

author
#7 lore raw

PATCH v3 1/1 nfnetlink_queue: add security context information

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 <[email protected]>

---
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(-)
include/uapi/linux/netfilter/nfnetlink_queue.h +3 -1
index 8dd819e..b67a853 100644
@@ -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 */
net/netfilter/nfnetlink_queue_core.c +31 -0
index 0b98c74..ae4f520 100644
@@ -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;
Signature · 1 line
1.9.1
author
#8 lore raw

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

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.

Quoted 114 lines · 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 <[email protected]>

---
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(-)
include/uapi/linux/netfilter/nfnetlink_queue.h +3 -1
index 8dd819e..b67a853 100644
@@ -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 */
net/netfilter/nfnetlink_queue_core.c +31 -0
index 0b98c74..ae4f520 100644
@@ -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;
Signature · 3 lines
--------------
 Roman Kubiak
--------------
maintainer
#9 lore raw

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

Quoted 4 lines · 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

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:

Quoted 8 lines
Quoted 8 lines
net/netfilter/nfnetlink_queue_core.c +0 -0
index 0b98c74..ae4f520 100644
@@ -278,6 +278,24 @@ nla_put_failure:
 	return -1;
 }
Quoted 4 lines

net/netfilter/nfnetlink_queue_core.c nla_put_failure: · line 284

Quoted 4 lines
net/netfilter/nfnetlink_queue_core.c +4 -0
+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.

author
#10 lore raw

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

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 <[email protected]>

---
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(-)
include/uapi/linux/netfilter/nfnetlink_queue.h +3 -1
index 8dd819e..b67a853 100644
@@ -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 */
net/netfilter/nfnetlink_queue_core.c +29 -0
index 0b98c74..2c35112 100644
@@ -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;
Signature · 1 line
1.9.1
Quoted 37 lines · Pablo Neira Ayuso
Quoted 4 lines · 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

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:

Quoted 8 lines
Quoted 8 lines
net/netfilter/nfnetlink_queue_core.c +0 -0
index 0b98c74..ae4f520 100644
@@ -278,6 +278,24 @@ nla_put_failure:
 	return -1;
 }
Quoted 4 lines

net/netfilter/nfnetlink_queue_core.c nla_put_failure: · line 284

Quoted 4 lines
net/netfilter/nfnetlink_queue_core.c +4 -0
+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 [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Signature · 3 lines
--------------
 Roman Kubiak
--------------
maintainer
#11 lore raw

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

Quoted 27 lines · Roman Kubiak
include/uapi/linux/netfilter/nfnetlink_queue.h +3 -1
index 8dd819e..b67a853 100644
@@ -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 */
net/netfilter/nfnetlink_queue_core.c +0 -0
index 0b98c74..2c35112 100644

net/netfilter/nfnetlink_queue_core.c nla_put_failure: · line 283

Quoted 6 lines
net/netfilter/nfnetlink_queue_core.c +3 -0
@@ -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.

author
#12 lore raw

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

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 <[email protected]>

---
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(-)
include/uapi/linux/netfilter/nfnetlink_queue.h +3 -1
index 8dd819e..b67a853 100644
@@ -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 */
net/netfilter/nfnetlink_queue_core.c +34 -1
index 0b98c74..380aac6 100644
@@ -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;
Signature · 1 line
1.9.1
Quoted 45 lines · Florian Westphal
Quoted 27 lines · Roman Kubiak
include/uapi/linux/netfilter/nfnetlink_queue.h +3 -1
index 8dd819e..b67a853 100644
@@ -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 */
net/netfilter/nfnetlink_queue_core.c +0 -0
index 0b98c74..2c35112 100644

net/netfilter/nfnetlink_queue_core.c nla_put_failure: · line 283

Quoted 6 lines
net/netfilter/nfnetlink_queue_core.c +3 -0
@@ -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.

Signature · 3 lines
--------------
 Roman Kubiak
--------------
maintainer
#13 lore raw

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

Quoted 3 lines · 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?)

[..]

Quoted 4 lines
net/netfilter/nfnetlink_queue_core.c +0 -0
@@ -1142,7 +1170,12 @@ nfqnl_recv_config(struct sock *ctnl, struct sk_buff *skb,
 			ret = -EOPNOTSUPP;
 			goto err_out_unlock;
 		}

net/netfilter/nfnetlink_queue_core.c nfqnl_recv_config(struct sock *ctnl, struct sk_buff *skb, · line 1181

Quoted 10 lines
net/netfilter/nfnetlink_queue_core.c +6 -1
-
+#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; ]

author
#14 lore raw

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

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 <[email protected]>

---
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(-)
include/uapi/linux/netfilter/nfnetlink_queue.h +3 -1
index 8dd819e..b67a853 100644
@@ -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 */
net/netfilter/nfnetlink_queue_core.c +34 -1
index 0b98c74..8c3f653 100644
@@ -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;
Signature · 1 line
1.9.1
Quoted 32 lines · Florian Westphal
Quoted 3 lines · 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?)

[..]

Quoted 4 lines
(quoted diff) +0 -0
@@ -1142,7 +1170,12 @@ nfqnl_recv_config(struct sock *ctnl, struct sk_buff *skb,
 			ret = -EOPNOTSUPP;
 			goto err_out_unlock;
 		}

Review nfqnl_recv_config(struct sock *ctnl, struct sk_buff *skb, · line 1181

Quoted 10 lines
(quoted diff) +6 -1
-
+#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; ]

Signature · 3 lines
--------------
 Roman Kubiak
--------------
maintainer Acked-by
#15 lore raw

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

Quoted 7 lines · Roman Kubiak

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 <[email protected]>

Looks good to me.

Acked-by Florian Westphal <[email protected]>
maintainer
#16 lore raw

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

In reply to Roman Kubiak
Quoted 11 lines · 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.

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

author
#17 lore raw

PATCH 1/1 libmnl: security context retrieval in nf-queue example

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 <[email protected]>
---
 examples/netfilter/nf-queue.c | 43 ++++++++++++++++++++++++++++++++++++++++---
 1 file changed, 40 insertions(+), 3 deletions(-)
examples/netfilter/nf-queue.c +40 -3
index 957e365..adafbed 100644
@@ -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");
Signature · 1 line
2.0.1
Quoted 19 lines · Pablo Neira Ayuso
Quoted 11 lines · 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.

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 [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Signature · 3 lines
--------------
 Roman Kubiak
--------------
maintainer
#18 lore raw

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

Quoted 10 lines · 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.

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.

author
#19 lore raw

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

It seems that there is nothing i can really add to there except maybe:

src/nlmsg.c +1 -0
index aebdd5e..cabd8be 100644
@@ -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.

Quoted 21 lines · Pablo Neira Ayuso
Quoted 10 lines · 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.

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.

Signature · 3 lines
--------------
 Roman Kubiak
--------------
maintainer
#20 lore raw

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

Quoted 1 line · Roman Kubiak

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.

author
#21 lore raw

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

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 <[email protected]>
---
 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(-)
include/libnetfilter_queue/libnetfilter_queue.h +2 -0
index 8194a4f..d7734e6 100644
@@ -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,
 };
include/libnetfilter_queue/linux_nfnetlink_queue.h +3 -1
index 5b6ae95..1975dfa 100644
@@ -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 */
include/linux/netfilter/nfnetlink_queue.h +3 -1
index 22f5d45..030672d 100644
@@ -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 */
src/libnetfilter_queue.c +23 -0
index c9ed865..84184ee 100644
@@ -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
src/nlmsg.c +1 -0
index aebdd5e..cabd8be 100644
@@ -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)
utils/nfqnl_test.c +12 -2
index b760cf0..783bc6c 100644
@@ -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);
Signature · 1 line
2.0.1
maintainer
#22 lore raw

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

Quoted 11 lines · 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.

Applied with minor glitch. Thanks.

utils/nfqnl_test.c static int cb(struct nfq_q_handle *qh, struct nfgenmsg *nfmsg, · line 83

Quoted 6 lines
utils/nfqnl_test.c +1 -1
@@ -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.

maintainer Applied
#23 lore raw

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

In reply to Roman Kubiak
Quoted 33 lines · 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 <[email protected]>

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

Quoted 7 lines

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.

maintainer
#24 lore raw

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

In reply to Roman Kubiak
Quoted 51 lines · Roman Kubiak

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 <[email protected]>

---
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(-)
include/uapi/linux/netfilter/nfnetlink_queue.h +3 -1
index 8dd819e..b67a853 100644
@@ -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 */
net/netfilter/nfnetlink_queue_core.c +0 -0
index 0b98c74..ae4f520 100644

net/netfilter/nfnetlink_queue_core.c nla_put_failure: · line 287

Quoted 10 lines
net/netfilter/nfnetlink_queue_core.c +7 -0
@@ -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

net/netfilter/nfnetlink_queue_core.c nfqnl_build_packet_message(struct net *net, struct nfqnl_instance *queue, · line 375

Quoted 11 lines
net/netfilter/nfnetlink_queue_core.c +3 -0
 	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).

net/netfilter/nfnetlink_queue_core.c nfqnl_build_packet_message(struct net *net, struct nfqnl_instance *queue, · line 512

Quoted 12 lines
net/netfilter/nfnetlink_queue_core.c +5 -0
 	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.

author
#25 lore raw

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

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

Quoted 107 lines · Florian Westphal
Quoted 51 lines · Roman Kubiak

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 <[email protected]>

---
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(-)
include/uapi/linux/netfilter/nfnetlink_queue.h +3 -1
index 8dd819e..b67a853 100644
@@ -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 */
net/netfilter/nfnetlink_queue_core.c +0 -0
index 0b98c74..ae4f520 100644

net/netfilter/nfnetlink_queue_core.c nla_put_failure: · line 287

Quoted 10 lines
net/netfilter/nfnetlink_queue_core.c +7 -0
@@ -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

net/netfilter/nfnetlink_queue_core.c nfqnl_build_packet_message(struct net *net, struct nfqnl_instance *queue, · line 375

Quoted 11 lines
net/netfilter/nfnetlink_queue_core.c +3 -0
 	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).

net/netfilter/nfnetlink_queue_core.c nfqnl_build_packet_message(struct net *net, struct nfqnl_instance *queue, · line 512

Quoted 12 lines
net/netfilter/nfnetlink_queue_core.c +5 -0
 	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.

Signature · 3 lines
--------------
 Roman Kubiak
--------------