On 6/13/2017 11:50 PM, Piotr Sawicki wrote:

Hi,

My name is Piotr. Currently I'm involved in maintaining the Nether service (a user-space firewall used in Tizen). I have a few remarks about this patch.

Thanks for the review. It is most helpful.

On 06/09/2017 04:41 AM, Casey Schaufler wrote:

quoted

Subject: [PATCH RFC] Smack: More sanity in the use of Netlabel

diff --git a/security/smack/smack_lsm.c b/security/smack/smack_lsm.c

@@ -4042,15 +4094,19 @@ static int smack_socket_sock_rcv_skb(struct sock *sk, struct sk_buff *skb)
          rc = smk_access(skp, ssp->smk_in, MAY_WRITE, &ad);
          rc = smk_bu_note("IPv4 delivery", skp, ssp->smk_in,
                      MAY_WRITE, rc);
-        if (rc != 0)
+        if (rc == 0)
+            break;
+        if (by_host)
+            icmp_send(skb, ICMP_DEST_UNREACH, ICMP_HOST_ANO, 0);
+        else
              netlbl_skbuff_err(skb, sk->sk_family, rc, 0);
          break;
  #if IS_ENABLED(CONFIG_IPV6)
      case PF_INET6:
-        proto = smk_skb_to_addr_ipv6(skb, &sadd);
-        if (proto != IPPROTO_UDP && proto != IPPROTO_TCP)
+        rc = smk_skb_to_addr_ipv6(skb, &sadd);
+        if (rc != IPPROTO_UDP && rc != IPPROTO_TCP)
              break;

The PF_INET6 socket may receive IPv4 packets too. In this case smk_skb_to_addr_ipv6() returns -EINVAL or some rubbish value. Furthermore, the smk_skb_to_addr_ipv6() function returns a detected protocol type (e.g. DCCP). If it is neither TCP nor UDP, then the packet will be blocked.

Which behavior do you think would be proper? I can't tell if
this is an observation or a complaint.

I wonder why are the other protocols not handled here (e.g. UDP Lite, DCCP)?

No one has asked for it. Patches welcome!

quoted

-#ifdef SMACK_IPV6_SECMARK_LABELING
+#ifdef CONFIG_SECURITY_SMACK_NETFILTER
          if (skb && skb->secmark != 0)
              skp = smack_from_secid(skb->secmark);
          else

@@ -4066,10 +4122,9 @@ static int smack_socket_sock_rcv_skb(struct sock *sk, struct sk_buff *skb)
          rc = smk_access(skp, ssp->smk_in, MAY_WRITE, &ad);
          rc = smk_bu_note("IPv6 delivery", skp, ssp->smk_in,
                      MAY_WRITE, rc);
-#endif /* SMACK_IPV6_SECMARK_LABELING */
-#ifdef SMACK_IPV6_PORT_LABELING
+#else
          rc = smk_ipv6_port_check(sk, &sadd, SMK_RECEIVING);
-#endif /* SMACK_IPV6_PORT_LABELING */
+#endif /* CONFIG_SECURITY_SMACK_NETFILTER */
          break;
  #endif /* CONFIG_IPV6 */
      }

@@ -4149,11 +4204,14 @@ static int smack_socket_getpeersec_dgram(struct socket *sock,
          s = ssp->smk_out->smk_secid;
          break;
      case PF_INET:
-#ifdef CONFIG_SECURITY_SMACK_NETFILTER
+        skp = smack_ipv4_skb_host_label(skb);
+        if (skp) {
+            s = skp->smk_secid;
+            break;
+        }

There are three functions which have very similar fragments of code. They deduce a Smack label from an incoming socket buffer. I've noticed some inconsistencies:
- In the smack_socket_sock_recv_skb() function skp defaults to smack_net_ambient.
- In smack_socket_getpeersec_dgram() the secid variable defaults to 0, which means the invalid secid.
- In the smack_inet_conn_request() function the default value is smack_known_huh.

Is it intentional?

I'll have to look and see. As I was working on this I noticed
some inconsistency, and did clean some of it up. Thank you
for the comment.

quoted

diff --git a/security/smack/smack_netfilter.c b/security/smack/smack_netfilter.c
index 205b785..9904f37 100644
--- a/security/smack/smack_netfilter.c
+++ b/security/smack/smack_netfilter.c

@@ -51,7 +51,9 @@ static unsigned int smack_ipv4_output(void *priv,
      if (sk && sk->sk_security) {
          ssp = sk->sk_security;
          skp = ssp->smk_out;
-        skb->secmark = skp->smk_secid;
+        if (ssp->smk_state == SMK_SOCK_DEFERRED &&
+            netlbl_skbuff_setattr(skb, PF_INET, &skp->smk_netlabel))
+            return NF_DROP;
      }
        return NF_ACCEPT;

The above change will affect the NFQUEUE mechanism. The secmark field of a socket buffer is used by the nfqnl_get_sk_secctx() function (net/netfilter/nfnetlink_queue.c) to retrieve a Smack label (a security context). Please take a look at this commit regarding libnetfilter_queue: https://git.netfilter.org/libnetfilter_queue/commit/?id=46912f1c18e01b63660a56ea7d9c572741e06117 The Nether service (https://wiki.tizen.org/Security:Nether) uses libnetfilter_queue to implement a software firewall. It utilizes the security context and UDI/GID fields of a netlink message to make a decision about what to do with an outgoing packet.

I'll put the skb->secmark = skp->smk_secid; back.

Also, I've noticed an inconsistency of handling the secmark field for IPv4 and IPv6 protocols. In smack_ipv6_output function() the skp->smk_secid field is copied to skb->secmark.

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