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

[etienne]

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

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

[Paul Moore]

On Tuesday 24 February 2009 05:20:42 pm etienne wrote:
> 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?

Please elaborate a bit more on how you would intend a user to configure and 
make use of this.  Also, in what cases would you remove the NetLabel from a 
socket?  What cases would you keep it?

-- 
paul moore
linux @ hp

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

[etienne]

Paul Moore wrote:
> On Tuesday 24 February 2009 05:20:42 pm etienne wrote:
>> 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?
> 
> Please elaborate a bit more on how you would intend a user to configure and 
> make use of this.  Also, in what cases would you remove the NetLabel from a 
> socket?  What cases would you keep it?
> 
well, i think it is simple : let's say i want to run a "smack-labelled server" (apache, vsftpd, ...) 
clients connect from internet,  so the server admin/user  will want to add a "0.0.0.0/0 @" entry in netlabel
that will _fail_ because the server will send back "labeled" packets.

In this configuration, client IP matching the netlabel should receive packets unlabeled from server;
it's the case for UDP today, not for TCP. I don't find it intuitive or coherent, but maybe i'm missing something

client IPs that don't match the netlabel list should stay _labeled_, the postaccept wont remove it.

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

[Paul Moore]

On Tuesday 24 February 2009 05:59:59 pm etienne wrote:
> Paul Moore wrote:
> > On Tuesday 24 February 2009 05:20:42 pm etienne wrote:
> >> 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?
> >
> > Please elaborate a bit more on how you would intend a user to configure
> > and make use of this.  Also, in what cases would you remove the NetLabel
> > from a socket?  What cases would you keep it?
>
> well, i think it is simple : let's say i want to run a "smack-labelled
> server" (apache, vsftpd, ...) clients connect from internet,  so the server
> admin/user  will want to add a "0.0.0.0/0 @" entry in netlabel that will
> _fail_ because the server will send back "labeled" packets.

I had to go back and look at the address based labeling patches, I had somehow 
forgotten that the single label support in Smack can only be used for removing 
labels, not adding them.  With that in mind your approach should work although 
you will still get really bizarre behavior in the following case:

 * Service not running at the ambient label
 * Only address based label loaded into Smack is "0.0.0.0/0 @" (everything
   unlabeled)
 * Client connects to service using labeled networking

If you and Casey can live with labeled connection suddenly becoming unlabeled 
(I doubt the remote host will deal with it very gracefully) then go for it.

-- 
paul moore
linux @ hp

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

[Casey Schaufler]

Paul Moore wrote:
> ...
>> well, i think it is simple : let's say i want to run a "smack-labelled
>> server" (apache, vsftpd, ...) clients connect from internet,  so the server
>> admin/user  will want to add a "0.0.0.0/0 @" entry in netlabel that will
>> _fail_ because the server will send back "labeled" packets.
>>     
>
> I had to go back and look at the address based labeling patches, I had somehow 
> forgotten that the single label support in Smack can only be used for removing 
> labels, not adding them.  With that in mind your approach should work although 
> you will still get really bizarre behavior in the following case:
>
>  * Service not running at the ambient label
>  * Only address based label loaded into Smack is "0.0.0.0/0 @" (everything
>    unlabeled)
>  * Client connects to service using labeled networking
>
> If you and Casey can live with labeled connection suddenly becoming unlabeled 
> (I doubt the remote host will deal with it very gracefully) then go for it.
>   

The case where the netlabel entry "0.0.0.0/0 @" has been added
will unfortunately be a very common case because it say that while
the local machine does MAC the world as a whole does not. It also
means that the admin does not understand the implication that
local networking will no longer enforce MAC controls, or that for
some bizarre reason that it what he wants. In either case it is
very unlikely that he expects to connect to another system that
speaks CIPSO. For that reason I expect that the "bizarre behavior"
of labeled hosts to be quite rare.

I think that it might be necessary to introduce mechanism to specify
labeled hosts in addition to unlabeled hosts. That way one could
specify:
    0.0.0.0/0       @
    127.0.0.1       CIPSO
    192.168.1.103   CIPSO

and let everyone except the local host be unlabeled while the local
host enforces Real MAC policy.

I personally find it reprehensible that the attitude that network
communications ought to be exempt from access controls is so
pervasive, but I bend to the will of the people. The interest in
Smack since the introduction of the web ("@") label has grown
dramatically.

I am still reviewing and verifying these patches, which look
fine so far, but I know better than to let my eyes make the
call when I have computers that are so much better at finding
software flaws.

Thank you again for the work and reviews. I am working on my
end. Really.

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

[etienne]

Casey Schaufler wrote:
> Paul Moore wrote:
>> ...
>>> well, i think it is simple : let's say i want to run a "smack-labelled
>>> server" (apache, vsftpd, ...) clients connect from internet,  so the server
>>> admin/user  will want to add a "0.0.0.0/0 @" entry in netlabel that will
>>> _fail_ because the server will send back "labeled" packets.
>>>     
>> I had to go back and look at the address based labeling patches, I had somehow 
>> forgotten that the single label support in Smack can only be used for removing 
>> labels, not adding them.  With that in mind your approach should work although 
>> you will still get really bizarre behavior in the following case:
>>
>>  * Service not running at the ambient label
>>  * Only address based label loaded into Smack is "0.0.0.0/0 @" (everything
>>    unlabeled)
>>  * Client connects to service using labeled networking
>>
>> If you and Casey can live with labeled connection suddenly becoming unlabeled 
>> (I doubt the remote host will deal with it very gracefully) then go for it.
>>   
> 
> The case where the netlabel entry "0.0.0.0/0 @" has been added
> will unfortunately be a very common case because it say that while
> the local machine does MAC the world as a whole does not. It also
> means that the admin does not understand the implication that
> local networking will no longer enforce MAC controls, or that for
> some bizarre reason that it what he wants. In either case it is
> very unlikely that he expects to connect to another system that
> speaks CIPSO. For that reason I expect that the "bizarre behavior"
> of labeled hosts to be quite rare.
> 
> I think that it might be necessary to introduce mechanism to specify
> labeled hosts in addition to unlabeled hosts. That way one could
> specify:
>     0.0.0.0/0       @
>     127.0.0.1       CIPSO
>     192.168.1.103   CIPSO
> 
yes, i guess it makes a lot of sense; the corp network can be labeled
but internet will stay  unlabeled 


> and let everyone except the local host be unlabeled while the local
> host enforces Real MAC policy.
> 
> I personally find it reprehensible that the attitude that network
> communications ought to be exempt from access controls is so
> pervasive, but I bend to the will of the people. The interest in
> Smack since the introduction of the web ("@") label has grown
> dramatically.
> 
> I am still reviewing and verifying these patches, which look
> fine so far, but I know better than to let my eyes make the
> call when I have computers that are so much better at finding
> software flaws.
> 
> Thank you again for the work and reviews. I am working on my
> end. Really.
> 
> 
> 

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

[etienne]

Paul Moore wrote:
> On Tuesday 24 February 2009 05:59:59 pm etienne wrote:
>> Paul Moore wrote:
>>> On Tuesday 24 February 2009 05:20:42 pm etienne wrote:
>>>> 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?
>>> Please elaborate a bit more on how you would intend a user to configure
>>> and make use of this.  Also, in what cases would you remove the NetLabel
>>> from a socket?  What cases would you keep it?
>> well, i think it is simple : let's say i want to run a "smack-labelled
>> server" (apache, vsftpd, ...) clients connect from internet,  so the server
>> admin/user  will want to add a "0.0.0.0/0 @" entry in netlabel that will
>> _fail_ because the server will send back "labeled" packets.
> 
> I had to go back and look at the address based labeling patches, I had somehow 
> forgotten that the single label support in Smack can only be used for removing 
> labels, not adding them.  With that in mind your approach should work although 
> you will still get really bizarre behavior in the following case:
> 
>  * Service not running at the ambient label
>  * Only address based label loaded into Smack is "0.0.0.0/0 @" (everything
>    unlabeled)
>  * Client connects to service using labeled networking
> 
yes i agree. I think, as Casey mentionned, that we should improve the /smack/netlabel
semantics to have "@IP/MASK CISPO"
and have smack_host_label return NULL in that case. 

In practice label based clients IP should be known by the sysadmin (it's your corporate network). 
The information we should inject to smack is where the clients using labeled networking are, because I expect they
 are fewer than the unlabeled one 

I personnaly haven't seen a deployement so far (and working for  a french ISP, i've seen quite a lot of customer corporate networks, arguably not the most secured).
But maybe it has been more deployed in US?

regards
Etienne

> If you and Casey can live with labeled connection suddenly becoming unlabeled 
> (I doubt the remote host will deal with it very gracefully) then go for it.
> 
what bothered me the most with current code is that we had different behaviors for tcp and udp 
(which are very different beasts, ok, but should get the same smack/CIPSO treatment because _smack user interface
deals with layer3 information not layer4_). 

The _most important_ thing, whatever decision we make, is to DOCUMENT the behavior. You know, 10 days before i was 
"only a user",  i had to dig into the code precisely because i didn't understand well the few docs.
(but it was _a lot_ of fun ;) )

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

[Paul Moore]

On Tuesday 24 February 2009 06:36:59 pm Paul Moore wrote:
> On Tuesday 24 February 2009 05:59:59 pm etienne wrote:
> > Paul Moore wrote:
> > > On Tuesday 24 February 2009 05:20:42 pm etienne wrote:
> > >> 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?
> > >
> > > Please elaborate a bit more on how you would intend a user to configure
> > > and make use of this.  Also, in what cases would you remove the
> > > NetLabel from a socket?  What cases would you keep it?
> >
> > well, i think it is simple : let's say i want to run a "smack-labelled
> > server" (apache, vsftpd, ...) clients connect from internet,  so the
> > server admin/user  will want to add a "0.0.0.0/0 @" entry in netlabel
> > that will _fail_ because the server will send back "labeled" packets.
>
> I had to go back and look at the address based labeling patches, I had
> somehow forgotten that the single label support in Smack can only be used
> for removing labels, not adding them.  With that in mind your approach
> should work although you will still get really bizarre behavior in the
> following case:
>
>  * Service not running at the ambient label
>  * Only address based label loaded into Smack is "0.0.0.0/0 @" (everything
>    unlabeled)
>  * Client connects to service using labeled networking
>
> If you and Casey can live with labeled connection suddenly becoming
> unlabeled (I doubt the remote host will deal with it very gracefully) then
> go for it.

The more I thought about this last night the more it bothered me so I decided 
to take a quick look to see if I could come up with something that would let 
me sleep easier.  The patch below is likely whitespace mangled and probably 
won't apply cleanly but since I haven't done any testing I consider that a 
good thing.

Take a look at the patch below and see if it accomplishes what you want/need; 
I think this is a much better approach than the socket_post_accept() method.

diff --git a/security/smack/smack_lsm.c b/security/smack/smack_lsm.c
index 0278bc0..6419e83 100644
--- a/security/smack/smack_lsm.c
+++ b/security/smack/smack_lsm.c
@@ -20,6 +20,7 @@
 #include <linux/ext2_fs.h>
 #include <linux/kd.h>
 #include <asm/ioctls.h>
+#include <linux/ip.h>
 #include <linux/tcp.h>
 #include <linux/udp.h>
 #include <linux/mutex.h>
@@ -2559,21 +2560,40 @@ static void smack_sock_graft(struct sock *sk, struct 
socket *parent)
 static int smack_inet_conn_request(struct sock *sk, struct sk_buff *skb,
 				   struct request_sock *req)
 {
+	u16 family = sk->sk_family;
 	struct netlbl_lsm_secattr skb_secattr;
 	struct socket_smack *ssp = sk->sk_security;
 	char smack[SMK_LABELLEN];
 	int rc;
 
-	if (skb == NULL)
-		return -EACCES;
+	/* handle mapped IPv4 packets arriving via IPv6 sockets */
+	if (family == PF_INET6 && skb->protocol == htons(ETH_P_IP))
+		family = PF_INET;
 
 	netlbl_secattr_init(&skb_secattr);
+	
 	rc = netlbl_skbuff_getattr(skb, sk->sk_family, &skb_secattr);
-	if (rc == 0)
+	if (rc == 0) {
+		if (family == PF_INET &&
+		    skb_secattr.type != NETLBL_NLTYPE_UNLABELED) {
+			struct iphdr *hdr = ip_hdr(skb);
+			struct sockaddr_in addr;
+			
+			/* if we are going to treat the other side of this
+			 * connection as a single label, unlabeled host we
+			 * shouldn't allow it to initiate a labeled
+			 * connection because we will end up confusing
+			 * everyone when we suddenly drop the labeling later */
+			addr.sin_addr.s_addr = hdr->saddr;
+			if (smack_host_label(&addr) != NULL) {
+				rc = -EACCES;
+				goto inet_conn_request_return;
+			}
+		}
 		smack_from_secattr(&skb_secattr, smack);
-	else
+	} else
 		strncpy(smack, smack_known_huh.smk_known, SMK_MAXLEN);
-	netlbl_secattr_destroy(&skb_secattr);
+
 	/*
 	 * Receiving a packet requires that the other end
 	 * be able to write here. Read access is not required.
@@ -2585,9 +2605,45 @@ static int smack_inet_conn_request(struct sock *sk, 
 	if (rc == 0)
 		strncpy(ssp->smk_packet, smack, SMK_MAXLEN);
 
+inet_conn_request_return:
+	netlbl_secattr_destroy(&skb_secattr);
 	return rc;
 }
 
+/**
+ * smack_inet_conn_established - Setup a new inbound connection
+ * @sk: the new child socket
+ * @skb: the inbound packet
+ *
+ * Perform the setup of a new inbound stream connection; this basically means
+ * check to see if the other end of the connection is configured as a single
+ * or multi-label host and enure the new connection's socket is configured
+ * correctly.
+ */
+static void smack_inet_conn_established(struct sock *sk, struct sk_buff *skb)
+{
+	struct iphdr *hdr;
+	struct sockaddr_in addr;
+
+	/* we only need to bother with IPv4 since we don't do IPv6 labeling */
+	if (skb->protocol != htons(ETH_P_IP))
+		return;
+
+	hdr = ip_hdr(skb);
+	addr.sin_addr.s_addr = hdr->saddr;
+	if (smack_host_label(&addr) == NULL)
+		return;
+
+	/* the other end of this connection is configured as a single label,
+	 * unlabeled host so we need to make sure we aren't going to label
+	 * the socket */
+	/* NOTE: this is _very_ important - we can only _remove_ the label at
+	 * this point, trying to add a label to the socket here could result
+	 * in a failure which we can't safely catch here due to the inability
+	 * to signal an error */
+	smack_netlabel(sk, SMACK_UNLABELED_SOCKET);
+}
+
 /*
  * Key management security hooks
  *
@@ -2940,6 +2996,7 @@ struct security_operations smack_ops = {
 	.sk_free_security = 		smack_sk_free_security,
 	.sock_graft = 			smack_sock_graft,
 	.inet_conn_request = 		smack_inet_conn_request,
+	.inet_conn_established =	smack_inet_conn_established,
 
  /* key management security hooks */
 #ifdef CONFIG_KEYS

-- 
paul moore
linux @ hp

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

[etienne]

Paul Moore wrote:
> On Tuesday 24 February 2009 06:36:59 pm Paul Moore wrote:
>> On Tuesday 24 February 2009 05:59:59 pm etienne wrote:
>>> Paul Moore wrote:
>>>> On Tuesday 24 February 2009 05:20:42 pm etienne wrote:
>>>>> 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?
>>>> Please elaborate a bit more on how you would intend a user to configure
>>>> and make use of this.  Also, in what cases would you remove the
>>>> NetLabel from a socket?  What cases would you keep it?
>>> well, i think it is simple : let's say i want to run a "smack-labelled
>>> server" (apache, vsftpd, ...) clients connect from internet,  so the
>>> server admin/user  will want to add a "0.0.0.0/0 @" entry in netlabel
>>> that will _fail_ because the server will send back "labeled" packets.
>> I had to go back and look at the address based labeling patches, I had
>> somehow forgotten that the single label support in Smack can only be used
>> for removing labels, not adding them.  With that in mind your approach
>> should work although you will still get really bizarre behavior in the
>> following case:
>>
>>  * Service not running at the ambient label
>>  * Only address based label loaded into Smack is "0.0.0.0/0 @" (everything
>>    unlabeled)
>>  * Client connects to service using labeled networking
>>
>> If you and Casey can live with labeled connection suddenly becoming
>> unlabeled (I doubt the remote host will deal with it very gracefully) then
>> go for it.
> 
> The more I thought about this last night the more it bothered me so I decided 
> to take a quick look to see if I could come up with something that would let 
> me sleep easier.  The patch below is likely whitespace mangled and probably 
> won't apply cleanly but since I haven't done any testing I consider that a 
> good thing.

Hi Paul,
sorry for the trouble. I'll do some testing of your patch
I guess you're right, you're approach seems more complete

thanks
Etienne

> 
> Take a look at the patch below and see if it accomplishes what you want/need; 
> I think this is a much better approach than the socket_post_accept() method.
> 
> diff --git a/security/smack/smack_lsm.c b/security/smack/smack_lsm.c
> index 0278bc0..6419e83 100644
> --- a/security/smack/smack_lsm.c
> +++ b/security/smack/smack_lsm.c
> @@ -20,6 +20,7 @@
>  #include <linux/ext2_fs.h>
>  #include <linux/kd.h>
>  #include <asm/ioctls.h>
> +#include <linux/ip.h>
>  #include <linux/tcp.h>
>  #include <linux/udp.h>
>  #include <linux/mutex.h>
> @@ -2559,21 +2560,40 @@ static void smack_sock_graft(struct sock *sk, struct 
> socket *parent)
>  static int smack_inet_conn_request(struct sock *sk, struct sk_buff *skb,
>  				   struct request_sock *req)
>  {
> +	u16 family = sk->sk_family;
>  	struct netlbl_lsm_secattr skb_secattr;
>  	struct socket_smack *ssp = sk->sk_security;
>  	char smack[SMK_LABELLEN];
>  	int rc;
>  
> -	if (skb == NULL)
> -		return -EACCES;
> +	/* handle mapped IPv4 packets arriving via IPv6 sockets */
> +	if (family == PF_INET6 && skb->protocol == htons(ETH_P_IP))
> +		family = PF_INET;
>  
>  	netlbl_secattr_init(&skb_secattr);
> +	
>  	rc = netlbl_skbuff_getattr(skb, sk->sk_family, &skb_secattr);
> -	if (rc == 0)
> +	if (rc == 0) {
> +		if (family == PF_INET &&
> +		    skb_secattr.type != NETLBL_NLTYPE_UNLABELED) {
> +			struct iphdr *hdr = ip_hdr(skb);
> +			struct sockaddr_in addr;
> +			
> +			/* if we are going to treat the other side of this
> +			 * connection as a single label, unlabeled host we
> +			 * shouldn't allow it to initiate a labeled
> +			 * connection because we will end up confusing
> +			 * everyone when we suddenly drop the labeling later */
> +			addr.sin_addr.s_addr = hdr->saddr;
> +			if (smack_host_label(&addr) != NULL) {
> +				rc = -EACCES;
> +				goto inet_conn_request_return;
> +			}
> +		}
>  		smack_from_secattr(&skb_secattr, smack);
> -	else
> +	} else
>  		strncpy(smack, smack_known_huh.smk_known, SMK_MAXLEN);
> -	netlbl_secattr_destroy(&skb_secattr);
> +
>  	/*
>  	 * Receiving a packet requires that the other end
>  	 * be able to write here. Read access is not required.
> @@ -2585,9 +2605,45 @@ static int smack_inet_conn_request(struct sock *sk, 
>  	if (rc == 0)
>  		strncpy(ssp->smk_packet, smack, SMK_MAXLEN);
>  
> +inet_conn_request_return:
> +	netlbl_secattr_destroy(&skb_secattr);
>  	return rc;
>  }
>
> +/**
> + * smack_inet_conn_established - Setup a new inbound connection
> + * @sk: the new child socket
> + * @skb: the inbound packet
> + *
> + * Perform the setup of a new inbound stream connection; this basically means
> + * check to see if the other end of the connection is configured as a single
> + * or multi-label host and enure the new connection's socket is configured
> + * correctly.
> + */
> +static void smack_inet_conn_established(struct sock *sk, struct sk_buff *skb)
> +{
> +	struct iphdr *hdr;
> +	struct sockaddr_in addr;
> +
> +	/* we only need to bother with IPv4 since we don't do IPv6 labeling */
> +	if (skb->protocol != htons(ETH_P_IP))
> +		return;
> +
> +	hdr = ip_hdr(skb);
> +	addr.sin_addr.s_addr = hdr->saddr;
> +	if (smack_host_label(&addr) == NULL)
> +		return;
> +
> +	/* the other end of this connection is configured as a single label,
> +	 * unlabeled host so we need to make sure we aren't going to label
> +	 * the socket */
> +	/* NOTE: this is _very_ important - we can only _remove_ the label at
> +	 * this point, trying to add a label to the socket here could result
> +	 * in a failure which we can't safely catch here due to the inability
> +	 * to signal an error */
> +	smack_netlabel(sk, SMACK_UNLABELED_SOCKET);
> +}
> +
>  /*
>   * Key management security hooks
>   *
> @@ -2940,6 +2996,7 @@ struct security_operations smack_ops = {
>  	.sk_free_security = 		smack_sk_free_security,
>  	.sock_graft = 			smack_sock_graft,
>  	.inet_conn_request = 		smack_inet_conn_request,
> +	.inet_conn_established =	smack_inet_conn_established,
>  
>   /* key management security hooks */
>  #ifdef CONFIG_KEYS
> 

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

[etienne]

hello,

Today, if  a  TCP server run with a SMACK non-ambient label, it will send labeled packets back to the client,
_even_ if the clients IP are in the /smack/netlabel "whitelist"
that's because "smack_socket_post_create" hook set labeled CIPSO packets unconditionnally
On connect, they are removed if the dest matches the /smack/netlabel

for ->accept, there is no such "feature"; if the client that just connect is in the /smack/netlabel,
SMACK send packeted label (although it shouldn't)
This breaks some applications (like sshd)


The following patch  adds a "post_access" hook to get the client IP and check it against the netlabel list. 
Please comment

regards,
Etienne

Signed-off-by: <etienne.basset@numericable.fr>
--
diff --git a/security/smack/smack_lsm.c b/security/smack/smack_lsm.c
index e6f89d6..74206db 100644
--- a/security/smack/smack_lsm.c
+++ b/security/smack/smack_lsm.c
@@ -27,6 +27,7 @@
 #include <net/netlabel.h>
 #include <net/cipso_ipv4.h>
 #include <linux/audit.h>
+#include <net/ipv6.h>
 
 #include "smack.h"
 
@@ -1566,6 +1567,78 @@ static int smack_socket_connect(struct socket *sock, struct sockaddr *sap,
 }
 
 /**
+ * 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;
+}
+
+/**
  * smack_flags_to_may - convert S_ to MAY_ values
  * @flags: the S_ value
  *
@@ -2906,6 +2979,7 @@ struct security_operations smack_ops = {
 
 	.socket_post_create = 		smack_socket_post_create,
 	.socket_connect =		smack_socket_connect,
+	.socket_post_accept =           smack_socket_post_accept,
 	.socket_sendmsg =		smack_socket_sendmsg,
 	.socket_sock_rcv_skb = 		smack_socket_sock_rcv_skb,
 	.socket_getpeersec_stream =	smack_socket_getpeersec_stream,
@@ -2936,7 +3010,7 @@ struct security_operations smack_ops = {
 };
 
 
-static __init init_smack_know_list(void)
+static __init void init_smack_know_list(void)
 {
 	list_add(&smack_known_huh.list, &smack_known_list);
 	list_add(&smack_known_hat.list, &smack_known_list);
@@ -2944,6 +3018,7 @@ static __init init_smack_know_list(void)
 	list_add(&smack_known_floor.list, &smack_known_list);
 	list_add(&smack_known_invalid.list, &smack_known_list);
 	list_add(&smack_known_web.list, &smack_known_list);
+	return;
 }
 
 /**

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

[Paul Moore]

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.  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 
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. 

-- 
paul moore
linux @ hp