[RFC PATCH 4/5] netlabel: Add SCTP support

Add support to label SCTP associations and cater for a situation where
family = PF_INET6 with an ip_hdr(skb)->version = 4.

Signed-off-by: Richard Haines <redacted>
---
 include/net/netlabel.h            |  3 ++
 net/netlabel/netlabel_kapi.c      | 80 +++++++++++++++++++++++++++++++++++++++
 net/netlabel/netlabel_unlabeled.c | 10 +++++
 3 files changed, 93 insertions(+)

diff --git a/include/net/netlabel.h b/include/net/netlabel.h
index 72d6435..7348966 100644
--- a/include/net/netlabel.h
+++ b/include/net/netlabel.h

@@ -494,6 +494,9 @@ int netlbl_conn_setattr(struct sock *sk,
 			const struct netlbl_lsm_secattr *secattr);
 int netlbl_req_setattr(struct request_sock *req,
 		       const struct netlbl_lsm_secattr *secattr);
+int netlbl_sctp_setattr(struct sock *sk,
+			struct sk_buff *skb,
+			const struct netlbl_lsm_secattr *secattr);
 void netlbl_req_delattr(struct request_sock *req);
 int netlbl_skbuff_setattr(struct sk_buff *skb,
 			  u16 family,

diff --git a/net/netlabel/netlabel_kapi.c b/net/netlabel/netlabel_kapi.c
index ea7c670..1c82bbe 100644
--- a/net/netlabel/netlabel_kapi.c
+++ b/net/netlabel/netlabel_kapi.c

@@ -1121,6 +1121,7 @@ int netlbl_conn_setattr(struct sock *sk,
 	switch (addr->sa_family) {
 	case AF_INET:
 		addr4 = (struct sockaddr_in *)addr;
+
 		entry = netlbl_domhsh_getentry_af4(secattr->domain,
 						   addr4->sin_addr.s_addr);
 		if (entry == NULL) {

@@ -1177,6 +1178,85 @@ int netlbl_conn_setattr(struct sock *sk,
 }
 
 /**
+ * netlbl_sctp_setattr - Label an incoming sctp association socket using
+ * the correct protocol
+ * @sk: the socket to label
+ * @skb: the packet
+ * @secattr: the security attributes
+ *
+ * Description:
+ * Attach the correct label to the given socket using the security attributes
+ * specified in @secattr.  Returns zero on success, negative values on failure.
+ *
+ */
+int netlbl_sctp_setattr(struct sock *sk,
+			struct sk_buff *skb,
+			const struct netlbl_lsm_secattr *secattr)
+{
+	int ret_val = -EINVAL;
+	struct netlbl_dommap_def *entry;
+	struct iphdr *hdr4;
+#if IS_ENABLED(CONFIG_IPV6)
+	struct ipv6hdr *hdr6;
+#endif
+
+	rcu_read_lock();
+	switch (sk->sk_family) {
+	case AF_INET:
+		hdr4 = ip_hdr(skb);
+
+		entry = netlbl_domhsh_getentry_af4(secattr->domain,
+						   hdr4->saddr);
+		if (entry == NULL) {
+			ret_val = -ENOENT;
+			goto sctp_setattr_return;
+		}
+		switch (entry->type) {
+		case NETLBL_NLTYPE_CIPSOV4:
+			ret_val = cipso_v4_sock_setattr(sk, entry->cipso,
+							secattr);
+			break;
+		case NETLBL_NLTYPE_UNLABELED:
+			netlbl_sock_delattr(sk);
+			ret_val = 0;
+			break;
+		default:
+			ret_val = -ENOENT;
+		}
+		break;
+#if IS_ENABLED(CONFIG_IPV6)
+	case AF_INET6:
+		hdr6 = ipv6_hdr(skb);
+		entry = netlbl_domhsh_getentry_af6(secattr->domain,
+						   &hdr6->saddr);
+		if (entry == NULL) {
+			ret_val = -ENOENT;
+			goto sctp_setattr_return;
+		}
+		switch (entry->type) {
+		case NETLBL_NLTYPE_CALIPSO:
+			ret_val = calipso_sock_setattr(sk, entry->calipso,
+						       secattr);
+			break;
+		case NETLBL_NLTYPE_UNLABELED:
+			netlbl_sock_delattr(sk);
+			ret_val = 0;
+			break;
+		default:
+			ret_val = -ENOENT;
+		}
+		break;
+#endif /* IPv6 */
+	default:
+		ret_val = -EPROTONOSUPPORT;
+	}
+
+sctp_setattr_return:
+	rcu_read_unlock();
+	return ret_val;
+}
+
+/**
  * netlbl_req_setattr - Label a request socket using the correct protocol
  * @req: the request socket to label
  * @secattr: the security attributes

diff --git a/net/netlabel/netlabel_unlabeled.c b/net/netlabel/netlabel_unlabeled.c
index 22dc1b9..c070dfc 100644
--- a/net/netlabel/netlabel_unlabeled.c
+++ b/net/netlabel/netlabel_unlabeled.c

@@ -1472,6 +1472,16 @@ int netlbl_unlabel_getattr(const struct sk_buff *skb,
 		iface = rcu_dereference(netlbl_unlhsh_def);
 	if (iface == NULL || !iface->valid)
 		goto unlabel_getattr_nolabel;
+
+#if IS_ENABLED(CONFIG_IPV6)
+	/* When resolving a fallback label, check the sk_buff version as
+	 * it is possible (e.g. SCTP) to have family = PF_INET6 while
+	 * receiving ip_hdr(skb)->version = 4.
+	 */
+	if (family == PF_INET6 && ip_hdr(skb)->version == 4)
+		family = PF_INET;
+#endif /* IPv6 */
+
 	switch (family) {
 	case PF_INET: {
 		struct iphdr *hdr4;

-- 
2.13.6

On Tue, Oct 17, 2017 at 9:58 AM, Richard Haines
[off-list ref] wrote:

I'm guessing this bit of extra whitespace was an accident; but just in
case, drop it from this patch please.

It seems like we should try to leverage the code in
netlbl_conn_setattr() a bit more.  I would suggest either tweaking the
callers to use a sockaddr struct and netlbl_conn_setattr(), or
implement netlbl_sctp_setattr() as a simple wrapper around
netlbl_conn_setattr() ... the former seems a bit cleaner, but I
suspect patch 5/5 will make it clear which approach is better.

It seems like this should be pulled out into it's own patch as a fix
that extends beyond SCTP, what do you think?

-- 
paul moore
www.paul-moore.com

On Mon, 2017-11-06 at 18:15 -0500, Paul Moore wrote:

On Tue, Oct 17, 2017 at 9:58 AM, Richard Haines
[off-list ref] wrote:

quoted

Add support to label SCTP associations and cater for a situation
where
family = PF_INET6 with an ip_hdr(skb)->version = 4.

Signed-off-by: Richard Haines <redacted>
---
 include/net/netlabel.h            |  3 ++
 net/netlabel/netlabel_kapi.c      | 80
+++++++++++++++++++++++++++++++++++++++
 net/netlabel/netlabel_unlabeled.c | 10 +++++
 3 files changed, 93 insertions(+)

diff --git a/include/net/netlabel.h b/include/net/netlabel.h
index 72d6435..7348966 100644
--- a/include/net/netlabel.h
+++ b/include/net/netlabel.h

@@ -494,6 +494,9 @@ int netlbl_conn_setattr(struct sock *sk,
                        const struct netlbl_lsm_secattr *secattr);
 int netlbl_req_setattr(struct request_sock *req,
                       const struct netlbl_lsm_secattr *secattr);
+int netlbl_sctp_setattr(struct sock *sk,
+                       struct sk_buff *skb,
+                       const struct netlbl_lsm_secattr *secattr);
 void netlbl_req_delattr(struct request_sock *req);
 int netlbl_skbuff_setattr(struct sk_buff *skb,
                          u16 family,

diff --git a/net/netlabel/netlabel_kapi.c

b/net/netlabel/netlabel_kapi.c
index ea7c670..1c82bbe 100644

--- a/net/netlabel/netlabel_kapi.c
+++ b/net/netlabel/netlabel_kapi.c

@@ -1121,6 +1121,7 @@ int netlbl_conn_setattr(struct sock *sk,
        switch (addr->sa_family) {
        case AF_INET:
                addr4 = (struct sockaddr_in *)addr;
+

I'm guessing this bit of extra whitespace was an accident; but just
in
case, drop it from this patch please.

Done

quoted

                entry = netlbl_domhsh_getentry_af4(secattr->domain,
                                                   addr4-

quoted

sin_addr.s_addr);

                if (entry == NULL) {

@@ -1177,6 +1178,85 @@ int netlbl_conn_setattr(struct sock *sk,
 }

 /**
+ * netlbl_sctp_setattr - Label an incoming sctp association socket

using
+ * the correct protocol
+ * @sk: the socket to label
+ * @skb: the packet
+ * @secattr: the security attributes
+ *
+ * Description:
+ * Attach the correct label to the given socket using the security
attributes
+ * specified in @secattr.  Returns zero on success, negative
values on failure.
+ *
+ */
+int netlbl_sctp_setattr(struct sock *sk,
+                       struct sk_buff *skb,
+                       const struct netlbl_lsm_secattr *secattr)
+{
+       int ret_val = -EINVAL;
+       struct netlbl_dommap_def *entry;
+       struct iphdr *hdr4;
+#if IS_ENABLED(CONFIG_IPV6)
+       struct ipv6hdr *hdr6;
+#endif
+
+       rcu_read_lock();
+       switch (sk->sk_family) {
+       case AF_INET:
+               hdr4 = ip_hdr(skb);
+
+               entry = netlbl_domhsh_getentry_af4(secattr->domain,
+                                                  hdr4->saddr);
+               if (entry == NULL) {
+                       ret_val = -ENOENT;
+                       goto sctp_setattr_return;
+               }
+               switch (entry->type) {
+               case NETLBL_NLTYPE_CIPSOV4:
+                       ret_val = cipso_v4_sock_setattr(sk, entry-

quoted

cipso,

+                                                       secattr);
+                       break;
+               case NETLBL_NLTYPE_UNLABELED:
+                       netlbl_sock_delattr(sk);
+                       ret_val = 0;
+                       break;
+               default:
+                       ret_val = -ENOENT;
+               }
+               break;
+#if IS_ENABLED(CONFIG_IPV6)
+       case AF_INET6:
+               hdr6 = ipv6_hdr(skb);
+               entry = netlbl_domhsh_getentry_af6(secattr->domain,
+                                                  &hdr6->saddr);
+               if (entry == NULL) {
+                       ret_val = -ENOENT;
+                       goto sctp_setattr_return;
+               }
+               switch (entry->type) {
+               case NETLBL_NLTYPE_CALIPSO:
+                       ret_val = calipso_sock_setattr(sk, entry-

quoted

calipso,

+                                                      secattr);
+                       break;
+               case NETLBL_NLTYPE_UNLABELED:
+                       netlbl_sock_delattr(sk);
+                       ret_val = 0;
+                       break;
+               default:
+                       ret_val = -ENOENT;
+               }
+               break;
+#endif /* IPv6 */
+       default:
+               ret_val = -EPROTONOSUPPORT;
+       }
+
+sctp_setattr_return:
+       rcu_read_unlock();
+       return ret_val;
+}

It seems like we should try to leverage the code in
netlbl_conn_setattr() a bit more.  I would suggest either tweaking
the
callers to use a sockaddr struct and netlbl_conn_setattr(), or
implement netlbl_sctp_setattr() as a simple wrapper around
netlbl_conn_setattr() ... the former seems a bit cleaner, but I
suspect patch 5/5 will make it clear which approach is better.

I've now modified selinux_netlbl_sctp_assoc_request() to extract the ip
address info from skb and add to a sockaddr struct, then call
netlbl_conn_setattr(). This removes any specific SCTP code from
netlabel_kapi.c

quoted

diff --git a/net/netlabel/netlabel_unlabeled.c

b/net/netlabel/netlabel_unlabeled.c
index 22dc1b9..c070dfc 100644

--- a/net/netlabel/netlabel_unlabeled.c
+++ b/net/netlabel/netlabel_unlabeled.c

@@ -1472,6 +1472,16 @@ int netlbl_unlabel_getattr(const struct

sk_buff *skb,
                iface = rcu_dereference(netlbl_unlhsh_def);
        if (iface == NULL || !iface->valid)
                goto unlabel_getattr_nolabel;
+
+#if IS_ENABLED(CONFIG_IPV6)
+       /* When resolving a fallback label, check the sk_buff
version as
+        * it is possible (e.g. SCTP) to have family = PF_INET6
while
+        * receiving ip_hdr(skb)->version = 4.
+        */
+       if (family == PF_INET6 && ip_hdr(skb)->version == 4)
+               family = PF_INET;
+#endif /* IPv6 */
+

It seems like this should be pulled out into it's own patch as a fix
that extends beyond SCTP, what do you think?

I'll submit this as a separate NetLabel patch today.

Thanks for all your comments on the patchset, will probably take a few
weeks to respond to them all.

On Mon, Nov 13, 2017 at 3:50 PM, Richard Haines
[off-list ref] wrote:

On Mon, 2017-11-06 at 18:15 -0500, Paul Moore wrote:

quoted

On Tue, Oct 17, 2017 at 9:58 AM, Richard Haines
[off-list ref] wrote:

quoted

Add support to label SCTP associations and cater for a situation
where
family = PF_INET6 with an ip_hdr(skb)->version = 4.

Signed-off-by: Richard Haines <redacted>
---
 include/net/netlabel.h            |  3 ++
 net/netlabel/netlabel_kapi.c      | 80
+++++++++++++++++++++++++++++++++++++++
 net/netlabel/netlabel_unlabeled.c | 10 +++++
 3 files changed, 93 insertions(+)

quoted

quoted

 /**
+ * netlbl_sctp_setattr - Label an incoming sctp association socket
using
+ * the correct protocol
+ * @sk: the socket to label
+ * @skb: the packet
+ * @secattr: the security attributes
+ *
+ * Description:
+ * Attach the correct label to the given socket using the security
attributes
+ * specified in @secattr.  Returns zero on success, negative
values on failure.
+ *
+ */
+int netlbl_sctp_setattr(struct sock *sk,
+                       struct sk_buff *skb,
+                       const struct netlbl_lsm_secattr *secattr)
+{
+       int ret_val = -EINVAL;
+       struct netlbl_dommap_def *entry;
+       struct iphdr *hdr4;
+#if IS_ENABLED(CONFIG_IPV6)
+       struct ipv6hdr *hdr6;
+#endif
+
+       rcu_read_lock();
+       switch (sk->sk_family) {
+       case AF_INET:
+               hdr4 = ip_hdr(skb);
+
+               entry = netlbl_domhsh_getentry_af4(secattr->domain,
+                                                  hdr4->saddr);
+               if (entry == NULL) {
+                       ret_val = -ENOENT;
+                       goto sctp_setattr_return;
+               }
+               switch (entry->type) {
+               case NETLBL_NLTYPE_CIPSOV4:
+                       ret_val = cipso_v4_sock_setattr(sk, entry-

quoted

cipso,

+                                                       secattr);
+                       break;
+               case NETLBL_NLTYPE_UNLABELED:
+                       netlbl_sock_delattr(sk);
+                       ret_val = 0;
+                       break;
+               default:
+                       ret_val = -ENOENT;
+               }
+               break;
+#if IS_ENABLED(CONFIG_IPV6)
+       case AF_INET6:
+               hdr6 = ipv6_hdr(skb);
+               entry = netlbl_domhsh_getentry_af6(secattr->domain,
+                                                  &hdr6->saddr);
+               if (entry == NULL) {
+                       ret_val = -ENOENT;
+                       goto sctp_setattr_return;
+               }
+               switch (entry->type) {
+               case NETLBL_NLTYPE_CALIPSO:
+                       ret_val = calipso_sock_setattr(sk, entry-

quoted

calipso,

+                                                      secattr);
+                       break;
+               case NETLBL_NLTYPE_UNLABELED:
+                       netlbl_sock_delattr(sk);
+                       ret_val = 0;
+                       break;
+               default:
+                       ret_val = -ENOENT;
+               }
+               break;
+#endif /* IPv6 */
+       default:
+               ret_val = -EPROTONOSUPPORT;
+       }
+
+sctp_setattr_return:
+       rcu_read_unlock();
+       return ret_val;
+}

It seems like we should try to leverage the code in
netlbl_conn_setattr() a bit more.  I would suggest either tweaking
the
callers to use a sockaddr struct and netlbl_conn_setattr(), or
implement netlbl_sctp_setattr() as a simple wrapper around
netlbl_conn_setattr() ... the former seems a bit cleaner, but I
suspect patch 5/5 will make it clear which approach is better.

I've now modified selinux_netlbl_sctp_assoc_request() to extract the ip
address info from skb and add to a sockaddr struct, then call
netlbl_conn_setattr(). This removes any specific SCTP code from
netlabel_kapi.c

Great, I think that's probably the best option.

quoted

quoted

diff --git a/net/netlabel/netlabel_unlabeled.c

b/net/netlabel/netlabel_unlabeled.c
index 22dc1b9..c070dfc 100644

--- a/net/netlabel/netlabel_unlabeled.c
+++ b/net/netlabel/netlabel_unlabeled.c

@@ -1472,6 +1472,16 @@ int netlbl_unlabel_getattr(const struct

sk_buff *skb,
                iface = rcu_dereference(netlbl_unlhsh_def);
        if (iface == NULL || !iface->valid)
                goto unlabel_getattr_nolabel;
+
+#if IS_ENABLED(CONFIG_IPV6)
+       /* When resolving a fallback label, check the sk_buff
version as
+        * it is possible (e.g. SCTP) to have family = PF_INET6
while
+        * receiving ip_hdr(skb)->version = 4.
+        */
+       if (family == PF_INET6 && ip_hdr(skb)->version == 4)
+               family = PF_INET;
+#endif /* IPv6 */
+

It seems like this should be pulled out into it's own patch as a fix
that extends beyond SCTP, what do you think?

I'll submit this as a separate NetLabel patch today.

Saw it and ACK'd it, thank you.

Thanks for all your comments on the patchset, will probably take a few
weeks to respond to them all.

No worries, we've waited some time for someone to implement proper
SCTP support, a few more weeks isn't going to hurt anyone :)

Also, apologies for some confusion on my part regarding SCTP in my
latest comments; I've since learned I wasn't thinking about endpoints
and associations correctly.

-- 
paul moore
www.paul-moore.com