[redhat-lspp] Updated NetLabel patch
Paul Moore
paul.moore at hp.com
Wed Jun 14 15:32:22 UTC 2006
Stephen Smalley wrote:
> On Tue, 2006-06-13 at 13:19 -0400, Paul Moore wrote:
>>
>>I'll be posting a more "reviewer friendly" patchset in a week or so once
>>this has been out for a few days and I have had a chance to work on the
>>patch a bit more (discussed on Monday's concall).
>
Wow, I wasn't expecting anyone to really review this patch (it was an
attachment, one big file, laced with arsenic, etc.), it was intended
more for Steve G's inclusion into the LSPP kernel. So thank you very
much for the comments ...
> diff -purN linux-2.6.16.i686/security/selinux/hooks.c linux-2.6.16.i686-netlabel_06132006/security/selinux/hooks.c
> --- linux-2.6.16.i686/security/selinux/hooks.c 2006-06-13 10:47:10.000000000 -0400
> +++ linux-2.6.16.i686-netlabel_06132006/security/selinux/hooks.c 2006-06-13 11:19:59.000000000 -0400
> @@ -2935,6 +2938,16 @@ static void selinux_socket_post_create(s
> isec->sid = kern ? SECINITSID_KERNEL : tsec->sid;
> isec->initialized = 1;
>
> +#ifdef CONFIG_NETLABEL
> + if (family == PF_INET)
> + /* PM - this will throw errors unless netlabel is configured
> + so if you enable netlabel you must make sure you configure
> + it early in your init scripts (i.e. before you bring up
> + networking) ... or should we just drop the BUG_ON() and
> + audit the failure? */
> + BUG_ON(security_netlbl_socket_setsid(sock, isec->sid));
> +#endif /* CONFIG_NETLABEL */
> +
>
> This needs to be changed to either propagate an error to the caller and
> cleanup (per your separate posting about socket_post_create) or to audit
> the issue. It looks like this can fail for reasons other than an actual
> kernel-internal bug.
Yes, I'm not very happy about this solution either, hence my comments
and posting to the LSM list. I'm still holding out hope that we can
resolve this with a change to the LSM hook as the thought of auditing
the failure and just letting it continue is not very appealing to me (or
the LSPP evaluators/users I suspect). On the plus side it sounds like
there are no real objections to modifying the post_create() hook so this
should work itself out in the next patch.
> Naturally, the usual guidance about #ifdefs applies, make sure you've
> followed Documentation/SubmittingPatches, CodingStyle, and now
> SubmitChecklist. You also need to re-base against an upstream kernel if
> you are truly targeting 2.6.18. It would be nice if the audit tree were
> fully flushed into -mm.
Promise you won't laugh to hard when I tell you that I just found the
CodingStyle/SubmittingPatches files last week (I just now found out
about the SubmitChecklist) ... :) I'm working on incorporating their
instructions into the code, it looks like this is another spot I missed.
Regarding the "patch basing", as I mentioned before these patches are
targeting the lspp.* kernels which are still based on the 2.6.17-mumble
tree.
I need to learn how to use git/quilt and when I do I'll start basing
patches against the 2.6.18 git trees. In the meantime I try to keep an
eye on the various patches and so far I haven't seen any changes that
would be a *major* disruption to the NetLabel code; mostly it would just
be a shifting of where the security_netlbl_* functions are called.
> @@ -3113,6 +3126,19 @@ static int selinux_socket_accept(struct
> return 0;
> }
>
> +#ifdef CONFIG_NETLABEL
> +static void selinux_socket_post_accept(struct socket *sock,
> + struct socket *newsock)
> +{
> + if (newsock->sk != NULL && newsock->sk->sk_family == PF_INET)
> + /* PM - see my comments in the socket_post_create() hook, the
> + problem is not quite as bad here since you have already
> + created at least one pf_inet socket and gotten traffic on
> + it but the core question still remains: BUG() or audit? */
> + BUG_ON(security_netlbl_socket_accept(sock, newsock));
> +}
> +#endif /* CONFIG_NETLABEL */
>
> Likewise here, but post_accept occurs too late, so audit may be your
> only option here.
See my earlier comments as well as my email to the LSM list from this
morning (6/14).
> @@ -3221,6 +3247,9 @@ static int selinux_socket_sock_rcv_skb(s
> struct socket *sock;
> struct net_device *dev;
> struct avc_audit_data ad;
> +#ifdef CONFIG_NETLABEL
> + u32 netlbl_sid;
> +#endif
>
> family = sk->sk_family;
> if (family != PF_INET && family != PF_INET6)
>
> The old sock_rcv_skb processing has been split by the secmark patches
> (already in the net-2.6.18 tree), with the old checks going into
> selinux_sock_rcv_skb_compat, which is only called if the compatibility
> mode is enabled. So your changes here need to be re-based.
See my earlier comments about re-basing the patch. The only question I
have is if I should add the NetLabel hooks to the compat() function as
well as the new rcv_skb()? I would think the safest thing to do would
be to only add the NetLabel hooks to the new rcv_skb() but there is some
question about RH providing SECMARK support in upcoming releases (i.e.
packets would still flow through rcv_skb_compat()) ... do you have a
strong opinion on this?
> @@ -3270,6 +3299,7 @@ static int selinux_socket_sock_rcv_skb(s
> default:
> netif_perm = NETIF__RAWIP_RECV;
> node_perm = NODE__RAWIP_RECV;
> + recv_perm = RAWIP_SOCKET__RECV_MSG;
> break;
> }
>
> @@ -3294,7 +3324,7 @@ static int selinux_socket_sock_rcv_skb(s
> if (err)
> goto out;
>
> - if (recv_perm) {
> + if (recv_perm != 0 && recv_perm != RAWIP_SOCKET__RECV_MSG) {
> u32 port_sid;
>
> /* Fixme: make this more efficient */
> @@ -3306,10 +3336,46 @@ static int selinux_socket_sock_rcv_skb(s
>
> err = avc_has_perm(sock_sid, port_sid,
> sock_class, recv_perm, &ad);
> + if (err)
> + goto out;
> }
>
> - if (!err)
> - err = selinux_xfrm_sock_rcv_skb(sock_sid, skb);
> + err = selinux_xfrm_sock_rcv_skb(sock_sid, skb);
> +#ifdef CONFIG_NETLABEL
> + if (err == 0)
> + goto out;
>
> So if IPSEC-labeled, ignore CIPSO option? What if you wanted to use
> CIPSO to individually label different packets/streams that use the same
> SA? Also seems to need reconcialiation with Venkat's patches for MLS
> xfrm support.
>
This is the greater question of how to resolve all the networking hooks
and I have given up any opinions on how to best handle this ... as you
pointed out the NetLabel check is only done if the IPsec SA is not
labeled. Would you prefer the NetLabel check to happen regardless (it's
an easy change to make)?
> +
> + err = security_netlbl_skbuff_getsid(skb, sock_sid, &netlbl_sid);
> + if (err)
> + goto out;
>
> What overhead does this add when CIPSO isn't being used at all?
I haven't posted any performance numbers because the code is still in a
state of flux, but I have done some very simple netperf runs over
loopback on some older code and the impact has only been a few percent
when using the default unlabeled NetLabel configuration. I suspect this
will improve once the code becomes more stable and I can spend some time
with a profiler ...
> + if (netlbl_sid == SECINITSID_UNLABELED)
> + /* PM - this may look strange (it is) but it is here to
> + preserve backward compatability */
> + err = avc_has_perm(sock_sid,
> + SECINITSID_UNLABELED,
> + SECCLASS_ASSOCIATION,
> + ASSOCIATION__RECVFROM,
> + NULL);
> + else
> + err = avc_has_perm(sock_sid,
> + netlbl_sid,
> + sock_class,
> + recv_perm,
> + &ad);
>
> I think we need to restructure this to retain encapsulation of xfrm and
> netlabel.
Do you have any suggestions?
> diff -purN linux-2.6.16.i686/security/selinux/nlmsgtab.c linux-2.6.16.i686-netlabel_06132006/security/selinux/nlmsgtab.c
> --- linux-2.6.16.i686/security/selinux/nlmsgtab.c 2006-06-13 10:47:09.000000000 -0400
> +++ linux-2.6.16.i686-netlabel_06132006/security/selinux/nlmsgtab.c 2006-06-13 11:20:00.000000000 -0400
> @@ -31,92 +32,101 @@ struct nlmsg_perm
>
> static struct nlmsg_perm nlmsg_route_perms[] =
> {
> - { RTM_NEWLINK, NETLINK_ROUTE_SOCKET__NLMSG_WRITE },
> - { RTM_DELLINK, NETLINK_ROUTE_SOCKET__NLMSG_WRITE },
> - { RTM_GETLINK, NETLINK_ROUTE_SOCKET__NLMSG_READ },
>
> Try to avoid extraneous diffs unrelated to your changes - if you want to
> do whitespace cleanups, do them via separate patch.
>
> @@ -169,6 +179,19 @@ int selinux_nlmsg_lookup(u16 sclass, u16
> }
> break;
>
> + case SECCLASS_NETLINK_NETLABEL_SOCKET:
> + /* PM - This is really a poor fit right now as NetLabel uses
> + the nlmsg_type value to specify the "class" of NetLabel
> + message (i.e. management, cipso, etc.). One way to fix
> + this would be to refactor the NetLabel netlink API so
> + that it uses the nlmsg_type in a way that more closely
> + resembles the other netlink consumers, however, that is
> + going to require a non-trivial rework at this point so lets
> + just leave it as is right now unless somebody complains. */
> + err = nlmsg_perm(nlmsg_type, perm, nlmsg_netlabel_perms,
> + sizeof(nlmsg_netlabel_perms));
> + break;
> +
>
> Let me be the first to complain ;) Also, note that there is ongoing
> discussion about how to deal with netlink more generally as part of
> addressing recent additions that aren't yet specifically controlled
> (defaulting to generic netlink class) by SELinux.
Argh, I knew somebody was going to complan - I just figured it would be
James ;) I apologize in advance, but I have to ask, how hard are you
complaining? Anything can of course be changed but I'm looking at some
already tough dates from RH for RHEL5 ...
> diff -purN linux-2.6.16.i686/security/selinux/ss/services.c linux-2.6.16.i686-netlabel_06132006/security/selinux/ss/services.c
> --- linux-2.6.16.i686/security/selinux/ss/services.c 2006-06-13 10:47:10.000000000 -0400
> +++ linux-2.6.16.i686-netlabel_06132006/security/selinux/ss/services.c 2006-06-13 11:20:00.000000000 -0400
> <snip>
> +/**
> + * security_netlbl_socket_accept - Handle the labeling of an accept()ed socket
> + * @sock: the original socket
> + * @newsock: the new accept()ed socket
> + *
> + * Description:
> + * Attempt to label a socket using the NetLabel mechanism based on the packets
> + * in the queue and the original socket's SID. Returns zero values on success,
> + * negative values on failure.
> + *
> + */
> +int security_netlbl_socket_accept(struct socket *sock, struct socket *newsock)
> +{
> + int ret_val;
> + struct inode_security_struct *isec = SOCK_INODE(sock)->i_security;
> + struct netlbl_lsm_secattr secattr;
> + u32 newsock_sid;
> + u16 sock_class;
> + u32 relabelto_perm;
> +
> + if (!ss_initialized)
> + return 0;
> + netlbl_secattr_init(&secattr);
> +
> + ret_val = netlbl_socket_getattr(newsock, &secattr);
> + if (ret_val != 0)
> + goto netlbl_socket_accept_return;
> + ret_val = security_netlbl_secattr_to_sid(NULL,
> + &secattr,
> + isec->sid,
> + &newsock_sid);
> + if (ret_val != 0 || newsock_sid == SECINITSID_UNLABELED)
> + goto netlbl_socket_accept_return;
> +
> + sock_class = isec->sclass;
> + switch (sock_class) {
> + case SECCLASS_UDP_SOCKET:
> + relabelto_perm = UDP_SOCKET__RELABELTO;
> + break;
> + case SECCLASS_TCP_SOCKET:
> + relabelto_perm = TCP_SOCKET__RELABELTO;
> + break;
> + default:
> + relabelto_perm = RAWIP_SOCKET__RELABELTO;
> + }
> +
> + /* PM - should we have a "RELABELFROM" check too? */
> + /* PM - i suspect we should audit this socket relabel */
> + ret_val = avc_has_perm(isec->sid,
> + newsock_sid,
> + sock_class,
> + relabelto_perm,
> + NULL);
> + if (ret_val != 0)
> + goto netlbl_socket_accept_return;
> +
> + isec = SOCK_INODE(newsock)->i_security;
> + isec->sid = newsock_sid;
>
> NAK. Relabeling of the socket based on the packet label is almost
> certainly not the right thing to do, and it violates non-tranquility.
> What are you really trying to do here?
The real need from my point of view is the call to
security_netlbl_socket_setsid() so that we can set the IP options
correctly for the accept()'ed socket. I thought it also just made sense
to relabel the socket's SID as well to reflect the change.
I can drop the change in the socket's SID but I think there should still
be some relabelto check before accept()'ing the socket.
--
paul moore
linux security @ hp
More information about the redhat-lspp
mailing list