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:
> 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!
> -#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.
> 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.
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.