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.

On 06/09/2017 04:41 AM, Casey Schaufler wrote: > 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.

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

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

> 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=46912f1c18e01b636... 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.

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.