From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1760824AbZBXWU6 (ORCPT ); Tue, 24 Feb 2009 17:20:58 -0500 Received: (majordomo@vger.kernel.org) by vger.kernel.org id S1759198AbZBXWUs (ORCPT ); Tue, 24 Feb 2009 17:20:48 -0500 Received: from smtp3.tech.numericable.fr ([82.216.111.39]:33265 "EHLO smtp3.tech.numericable.fr" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1759073AbZBXWUr (ORCPT ); Tue, 24 Feb 2009 17:20:47 -0500 Message-ID: <49A472BA.8090805@numericable.fr> Date: Tue, 24 Feb 2009 23:20:42 +0100 From: etienne User-Agent: Thunderbird 2.0.0.19 (X11/20090105) MIME-Version: 1.0 To: Paul Moore CC: etienne , Casey Schaufler , Linux Kernel Mailing List , LSM Subject: Re: [PATCH][SMACK] add a socket_post_accept hook to fix netlabel issues with labeled TCP servers V1 References: In-Reply-To: Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Paul Moore wrote: > On Tuesday 24 February 2009 04:28:24 pm etienne wrote: >> /** >> + * smack_socket_post_access - post access check >> + * @sock: the socket >> + * @newsock : the grafted sock >> + * >> + * we have to match client IP against smack_host_label() >> + */ >> +static void smack_socket_post_accept(struct socket *sock, struct socket >> *newsock) +{ >> + char *hostsp; >> + struct sockaddr_storage address; >> + struct sockaddr_in *sin; >> + struct sockaddr_in6 *sin6; >> + struct in6_addr *addr6; >> + struct socket_smack *ssp = newsock->sk->sk_security; >> + int len; >> + >> + if (sock->sk == NULL) >> + return; >> + >> + /* sockets can listen on both IPv4 & IPv6, >> + and fallback to V4 if client is V4 */ >> + if (newsock->sk->sk_family != AF_INET && newsock->sk->sk_family != >> AF_INET6) + return; >> + >> + /* get the client IP address **/ >> + newsock->ops->getname(newsock, (struct sockaddr *)&address, &len, 2); >> + >> + switch (newsock->sk->sk_family) { >> + case AF_INET: >> + sin = (struct sockaddr_in *)&address; >> + break; >> + case AF_INET6: >> + sin6 = (struct sockaddr_in6 *)&address; >> + addr6 = &sin6->sin6_addr; >> + /* if a V4 client connects to a V6 listening server, >> + * we will get a IPV6_ADDR_MAPPED mapped address here >> + * we have to handle this case too >> + * the test below is ipv6_addr_type()== IPV6_ADDR_MAPPED >> + * without the requirement to have IPv6 compiled in >> + */ >> + if ((addr6->s6_addr32[0] | addr6->s6_addr32[1]) == 0 && >> + addr6->s6_addr32[2] == htonl(0x0000ffff)) { >> + __be32 addr = sin6->sin6_addr.s6_addr32[3]; >> + __be16 port = sin6->sin6_port; >> + sin = (struct sockaddr_in *)&address; >> + sin->sin_family = AF_INET; >> + sin->sin_port = port; >> + sin->sin_addr.s_addr = addr; >> + } else { >> + /* standard IPv6, we'll send unlabeled */ >> + smack_netlabel(newsock->sk, SMACK_UNLABELED_SOCKET); >> + return; >> + } >> + break; >> + default: >> + /** not possible to be there **/ >> + return; >> + } >> + /* so, is there a label for the source IP **/ >> + hostsp = smack_host_label(sin); >> + >> + if (hostsp == NULL) { >> + if (ssp->smk_labeled != SMACK_CIPSO_SOCKET) >> + smack_netlabel(newsock->sk, SMACK_CIPSO_SOCKET); >> + return; >> + } >> + if (ssp->smk_labeled != SMACK_UNLABELED_SOCKET) >> + smack_netlabel(newsock->sk, SMACK_UNLABELED_SOCKET); >> + return; >> +} > > NAK, you can't ignore return values like that. > > I'm sorry I didn't get a chance to respond to your email from this morning, > but the problem with the post_accept() hook is that you can't fail in this > hook. There has been a _lot_ of discussion about this over the past couple of > years on the LSM list. You should check the archives for all the details but > the main problem is that the post_accept() hook is too late to deny the > incoming connection so you can't reject the connection at that point in any > sane manner. well, i don't want to reject the connection here :) >I think I'm going to draft a patch to remove the post_accept() > hook since no one in-tree is using it and it's existence seems to cause more > problems than it solves. > > Now, I understand that your patch doesn't actually enforce any access controls > but it does call smack_netlabel() in several places and that function can fail The smack_netlabel(newsock->sk, SMACK_CIPSO_SOCKET) can failed, but has no interest in this function (because the socket has already be SMACK_CIPSO_SOCKET labeled by the policy) I can remove it. but smack_netlabel(SMACK_UNLABELED_SOCKET) cannot fail, and that's what i'm interested in could this make the patch acceptable? > for a variety of reasons (actually it is netlbl_sock_setattr() which will > fail) and if it does fail you could end up in a bad place. If you want to > ensure _all_ the packets are labeled correctly based on destination address > Smack will need to adopt the netfilter hooks as mentioned previously. > I'll have a look, if there's no other solution. But checking _all_ packets against the netlabel list, i guess performance won't like it thanks Etienne