Re: [PATCH][SMACK] add a socket_post_accept hook to fix netlabel issues with labeled TCP servers V1

* Re: [PATCH][SMACK] add a socket_post_accept hook to fix netlabel issues with labeled TCP servers V1
       [not found] ` <fa.BogfdiS32WCl3kqw5KFzeBPP0jc@ifi.uio.no>
@ 2009-02-24 22:20   ` etienne
  2009-02-24 22:38     ` Paul Moore
  0 siblings, 1 reply; 11+ messages in thread
From: etienne @ 2009-02-24 22:20 UTC (permalink / raw)
  To: Paul Moore; +Cc: etienne, Casey Schaufler, Linux Kernel Mailing List, LSM

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

^ permalink raw reply	[flat|nested] 11+ messages in thread