socket options ... again

socket options ... again

[Paul Moore]

I was looking at some policy and just realized that it's pretty liberal in
handing out the socket setopt permission to domains; for some reason I was
thought this was a rather rare thing.  This concerns me slightly because this is
how NetLabel attaches security attributes to outgoing packets.

As a result I'm starting to look at how to best protect the NetLabel security
attributes in the socket's IP options.  It is pretty straight forward to hook
into the selinux_socket_setsockopt() function and watch for the
IPPROTO_IP/IP_OPTIONS combination but my question to all of you is what is the
"right" thing to do in such a case?  I can think of the following options (all
of which are fairly easy to implement):

1. If NetLabel is compiled into the kernel deny the application the right to set
the socket's IP options.  This is pretty heavy handed but it would preserve the
security attributes; although I have no idea the problems it might cause in the
application space.

2. If the socket has a NetLabel attached to it simply deny the access, otherwise
allow it to proceed.  This has the problem in that it could allow a "bad"
application to write it's own "evil" security attributes to the socket's IP
option, although if there is no NetLabel attached to the socket (i.e. NetLabel
was told to ignore this domain) this may be okay.

3. If the application sets it's own security attributes, clear the socket's
option field and replace it with the correct NetLabel option even if the
NetLabel configuration for that domain is to leave the packets unlabeled.  The
problem here is that we would be allowing the setsockopt() syscall to proceed
without error then we would end up resetting the options on the socket (this
would happen on the first write to the socket).

While option #1 is the easiest I don't really like the idea of simply denying
applications the ability to set IP options (although I question how common this
really is).  Option #3 is perhaps the most compatible, although it could lead to
strange behavior with socket options changing underneath the application.  Which
leads me to option #2, it does have the drawback I mentioned above, but then
again there is nothing stopping applications from doing this currently.

Thoughts on these options?  Can anyone else think of a better solution?

-- 
paul moore
linux security @ hp

--
This message was distributed to subscribers of the selinux mailing list.
If you no longer wish to subscribe, send mail to majordomo@tycho.nsa.gov with
the words "unsubscribe selinux" without quotes as the message.

Re: socket options ... again

[Stephen Smalley]

On Thu, 2006-10-26 at 12:21 -0400, Paul Moore wrote:
> I was looking at some policy and just realized that it's pretty liberal in
> handing out the socket setopt permission to domains; for some reason I was
> thought this was a rather rare thing.  This concerns me slightly because this is
> how NetLabel attaches security attributes to outgoing packets.
> 
> As a result I'm starting to look at how to best protect the NetLabel security
> attributes in the socket's IP options.  It is pretty straight forward to hook
> into the selinux_socket_setsockopt() function and watch for the
> IPPROTO_IP/IP_OPTIONS combination but my question to all of you is what is the
> "right" thing to do in such a case?  I can think of the following options (all
> of which are fairly easy to implement):
> 
> 1. If NetLabel is compiled into the kernel deny the application the right to set
> the socket's IP options.  This is pretty heavy handed but it would preserve the
> security attributes; although I have no idea the problems it might cause in the
> application space.
> 
> 2. If the socket has a NetLabel attached to it simply deny the access, otherwise
> allow it to proceed.  This has the problem in that it could allow a "bad"
> application to write it's own "evil" security attributes to the socket's IP
> option, although if there is no NetLabel attached to the socket (i.e. NetLabel
> was told to ignore this domain) this may be okay.
> 
> 3. If the application sets it's own security attributes, clear the socket's
> option field and replace it with the correct NetLabel option even if the
> NetLabel configuration for that domain is to leave the packets unlabeled.  The
> problem here is that we would be allowing the setsockopt() syscall to proceed
> without error then we would end up resetting the options on the socket (this
> would happen on the first write to the socket).
> 
> While option #1 is the easiest I don't really like the idea of simply denying
> applications the ability to set IP options (although I question how common this
> really is).  Option #3 is perhaps the most compatible, although it could lead to
> strange behavior with socket options changing underneath the application.  Which
> leads me to option #2, it does have the drawback I mentioned above, but then
> again there is nothing stopping applications from doing this currently.
> 
> Thoughts on these options?  Can anyone else think of a better solution?

ip_options_compile() checks CAP_NET_RAW for IPOPT_SEC, IPOPT_SID, and
any unknown options.  Is there a reason to not do likewise for setting
IPOPT_CIPSO from userspace (which you need to distinguish in some manner
from your internal setting)?

-- 
Stephen Smalley
National Security Agency


--
This message was distributed to subscribers of the selinux mailing list.
If you no longer wish to subscribe, send mail to majordomo@tycho.nsa.gov with
the words "unsubscribe selinux" without quotes as the message.

Re: socket options ... again

[Paul Moore]

Stephen Smalley wrote:
> On Thu, 2006-10-26 at 12:21 -0400, Paul Moore wrote:
> 
>>Thoughts on these options?  Can anyone else think of a better solution?
> 
> ip_options_compile() checks CAP_NET_RAW for IPOPT_SEC, IPOPT_SID, and
> any unknown options.  Is there a reason to not do likewise for setting
> IPOPT_CIPSO from userspace (which you need to distinguish in some manner
> from your internal setting)?

Ah ha ... thank you for the lightbulb moment.

Adding CAP_NET_RAW in ip_options_compile() would solve the problem in a manner
which makes the most sense from an application point of view.  However, it does
present a problem in that potentially even applications without CAP_NET_RAW will
need to have a CIPSO options set by the kernel, as you noted in the comment
about distinguishing between NetLabel and applications.

Right now the CIPSO code only makes use of ip_options_compile() in two places
(that I can think of): the first is when a packet enters the system via
ip_rcv_options() in ip_rcv_finish(), the second when a socket is created by an
application and cipso_v4_socket_setattr() is called.  I don't believe
CAP_NET_RAW should be an issue in the first case (please correct me if I'm
wrong) but it would cause problems in the second case.  Considering the blank
slate of a new socket and the very simple nature of the CIPSO code in
ip_options_compile() it may be the best option is to simply handle the
ip_options_compile() tasks directly in cipso_v4_socket_setattr().

Does this sound reasonable?

-- 
paul moore
linux security @ hp

--
This message was distributed to subscribers of the selinux mailing list.
If you no longer wish to subscribe, send mail to majordomo@tycho.nsa.gov with
the words "unsubscribe selinux" without quotes as the message.

Re: socket options ... again

[Stephen Smalley]

On Thu, 2006-10-26 at 13:23 -0400, Paul Moore wrote:
> Stephen Smalley wrote:
> > On Thu, 2006-10-26 at 12:21 -0400, Paul Moore wrote:
> > 
> >>Thoughts on these options?  Can anyone else think of a better solution?
> > 
> > ip_options_compile() checks CAP_NET_RAW for IPOPT_SEC, IPOPT_SID, and
> > any unknown options.  Is there a reason to not do likewise for setting
> > IPOPT_CIPSO from userspace (which you need to distinguish in some manner
> > from your internal setting)?
> 
> Ah ha ... thank you for the lightbulb moment.
> 
> Adding CAP_NET_RAW in ip_options_compile() would solve the problem in a manner
> which makes the most sense from an application point of view.  However, it does
> present a problem in that potentially even applications without CAP_NET_RAW will
> need to have a CIPSO options set by the kernel, as you noted in the comment
> about distinguishing between NetLabel and applications.
> 
> Right now the CIPSO code only makes use of ip_options_compile() in two places
> (that I can think of): the first is when a packet enters the system via
> ip_rcv_options() in ip_rcv_finish(), the second when a socket is created by an
> application and cipso_v4_socket_setattr() is called.  I don't believe
> CAP_NET_RAW should be an issue in the first case (please correct me if I'm
> wrong) but it would cause problems in the second case.  Considering the blank
> slate of a new socket and the very simple nature of the CIPSO code in
> ip_options_compile() it may be the best option is to simply handle the
> ip_options_compile() tasks directly in cipso_v4_socket_setattr().
> 
> Does this sound reasonable?

To clarify, the existing capable checks in ip_options_compile() are only
applied on the output processing (they check for a NULL skb), never on
the input side.

It seems like your internal use of ip_options_compile() from cipso is
overkill, since it then just calls back into your cipso code for the
CIPSO option, right?  So just handling it directly within the cipso code
makes sense to me.

-- 
Stephen Smalley
National Security Agency


--
This message was distributed to subscribers of the selinux mailing list.
If you no longer wish to subscribe, send mail to majordomo@tycho.nsa.gov with
the words "unsubscribe selinux" without quotes as the message.

Re: socket options ... again

[Paul Moore]

Stephen Smalley wrote:
> On Thu, 2006-10-26 at 13:23 -0400, Paul Moore wrote:
> 
>>Stephen Smalley wrote:
>>
>>>On Thu, 2006-10-26 at 12:21 -0400, Paul Moore wrote:
>>>
>>>
>>>>Thoughts on these options?  Can anyone else think of a better solution?
>>>
>>>ip_options_compile() checks CAP_NET_RAW for IPOPT_SEC, IPOPT_SID, and
>>>any unknown options.  Is there a reason to not do likewise for setting
>>>IPOPT_CIPSO from userspace (which you need to distinguish in some manner
>>>from your internal setting)?
>>
>>Ah ha ... thank you for the lightbulb moment.
>>
>>Adding CAP_NET_RAW in ip_options_compile() would solve the problem in a manner
>>which makes the most sense from an application point of view.  However, it does
>>present a problem in that potentially even applications without CAP_NET_RAW will
>>need to have a CIPSO options set by the kernel, as you noted in the comment
>>about distinguishing between NetLabel and applications.
>>
>>Right now the CIPSO code only makes use of ip_options_compile() in two places
>>(that I can think of): the first is when a packet enters the system via
>>ip_rcv_options() in ip_rcv_finish(), the second when a socket is created by an
>>application and cipso_v4_socket_setattr() is called.  I don't believe
>>CAP_NET_RAW should be an issue in the first case (please correct me if I'm
>>wrong) but it would cause problems in the second case.  Considering the blank
>>slate of a new socket and the very simple nature of the CIPSO code in
>>ip_options_compile() it may be the best option is to simply handle the
>>ip_options_compile() tasks directly in cipso_v4_socket_setattr().
>>
>>Does this sound reasonable?
> 
> To clarify, the existing capable checks in ip_options_compile() are only
> applied on the output processing (they check for a NULL skb), never on
> the input side.

Yep.

> It seems like your internal use of ip_options_compile() from cipso is
> overkill, since it then just calls back into your cipso code for the
> CIPSO option, right?  So just handling it directly within the cipso code
> makes sense to me.

Yes, you are right there is a lot of unnecessary stuff there; its a bit of a
holdover from the initial code which used ip_options_get().  The original
thought was that since it wasn't really a critical path it was probably better
to reuse what was there even if it was slower.

I'm working on a patch now which I should have out in a few hours.

-- 
paul moore
linux security @ hp

--
This message was distributed to subscribers of the selinux mailing list.
If you no longer wish to subscribe, send mail to majordomo@tycho.nsa.gov with
the words "unsubscribe selinux" without quotes as the message.

Re: socket options ... again

[Paul Moore]

Stephen Smalley wrote:
> On Thu, 2006-10-26 at 12:21 -0400, Paul Moore wrote:
> 
>>I was looking at some policy and just realized that it's pretty liberal in
>>handing out the socket setopt permission to domains; for some reason I was
>>thought this was a rather rare thing.  This concerns me slightly because this is
>>how NetLabel attaches security attributes to outgoing packets.
>>
>>As a result I'm starting to look at how to best protect the NetLabel security
>>attributes in the socket's IP options.  It is pretty straight forward to hook
>>into the selinux_socket_setsockopt() function and watch for the
>>IPPROTO_IP/IP_OPTIONS combination but my question to all of you is what is the
>>"right" thing to do in such a case?  I can think of the following options (all
>>of which are fairly easy to implement):
>>
>>1. If NetLabel is compiled into the kernel deny the application the right to set
>>the socket's IP options.  This is pretty heavy handed but it would preserve the
>>security attributes; although I have no idea the problems it might cause in the
>>application space.
>>
>>2. If the socket has a NetLabel attached to it simply deny the access, otherwise
>>allow it to proceed.  This has the problem in that it could allow a "bad"
>>application to write it's own "evil" security attributes to the socket's IP
>>option, although if there is no NetLabel attached to the socket (i.e. NetLabel
>>was told to ignore this domain) this may be okay.
>>
>>3. If the application sets it's own security attributes, clear the socket's
>>option field and replace it with the correct NetLabel option even if the
>>NetLabel configuration for that domain is to leave the packets unlabeled.  The
>>problem here is that we would be allowing the setsockopt() syscall to proceed
>>without error then we would end up resetting the options on the socket (this
>>would happen on the first write to the socket).
>>
>>While option #1 is the easiest I don't really like the idea of simply denying
>>applications the ability to set IP options (although I question how common this
>>really is).  Option #3 is perhaps the most compatible, although it could lead to
>>strange behavior with socket options changing underneath the application.  Which
>>leads me to option #2, it does have the drawback I mentioned above, but then
>>again there is nothing stopping applications from doing this currently.
>>
>>Thoughts on these options?  Can anyone else think of a better solution?
> 
> ip_options_compile() checks CAP_NET_RAW for IPOPT_SEC, IPOPT_SID, and
> any unknown options.  Is there a reason to not do likewise for setting
> IPOPT_CIPSO from userspace (which you need to distinguish in some manner
> from your internal setting)?

Ungh.  I really wanted to believe it was that simple, but upon further
inspection it looks like simply adding CAP_NET_RAW only prevents applications
from setting their own CIPSO option it does not prevent users from replacing a
CIPSO option with another option like source routing.  With this in mind I think
adding the CAP_NET_RAW check and going with option #2 (see above) is the best
choice ... thoughts?

-- 
paul moore
linux security @ hp

--
This message was distributed to subscribers of the selinux mailing list.
If you no longer wish to subscribe, send mail to majordomo@tycho.nsa.gov with
the words "unsubscribe selinux" without quotes as the message.

[RFC] protect NetLabel options from setsockopt (was: socket options ... again)

[Paul Moore]

Paul Moore wrote:
> 
> Ungh.  I really wanted to believe it was that simple, but upon further
> inspection it looks like simply adding CAP_NET_RAW only prevents applications
> from setting their own CIPSO option it does not prevent users from replacing a
> CIPSO option with another option like source routing.  With this in mind I think
> adding the CAP_NET_RAW check and going with option #2 (see above) is the best
> choice ... thoughts?
> 

Below is a quick patch which adds both the CAP_NET_RAW check and option #2 check
(see the rest of the thread for details).  You will also notice some slight
locking changes in selinux_netlbl_inode_permission(); I believe these are
necessary to prevent a race condition with the checks in
selinux_netlbl_socket_setsockopt().

I haven't had much chance to test this code yet (all I know is it compiles and
boots) but I won't be in a position where I can do much testing this Friday or
over the weekend so I thought it might be a good idea to at least throw this out
as a RFC patch to let people comment.  If there are no objections and nothing
pops up during testing I'll post this for inclusion early next week.

(My apologies for the whitespace issues in the diff below but I wasn't sure how
to post a patch w/o losing the threading any other way)

Index: net-2.6_sockopt/net/ipv4/cipso_ipv4.c
===================================================================
--- net-2.6_sockopt.orig/net/ipv4/cipso_ipv4.c
+++ net-2.6_sockopt/net/ipv4/cipso_ipv4.c
@@ -1307,7 +1307,8 @@ int cipso_v4_socket_setattr(const struct

        /* We can't use ip_options_get() directly because it makes a call to
         * ip_options_get_alloc() which allocates memory with GFP_KERNEL and
-        * we can't block here. */
+        * we won't always have CAP_NET_RAW even though we _always_ want to
+        * set the IPOPT_CIPSO option. */
        opt_len = (buf_len + 3) & ~3;
        opt = kzalloc(sizeof(*opt) + opt_len, GFP_ATOMIC);
        if (opt == NULL) {
@@ -1317,11 +1318,9 @@ int cipso_v4_socket_setattr(const struct
        memcpy(opt->__data, buf, buf_len);
        opt->optlen = opt_len;
        opt->is_data = 1;
+       opt->cipso = sizeof(struct iphdr);
        kfree(buf);
        buf = NULL;
-       ret_val = ip_options_compile(opt, NULL);
-       if (ret_val != 0)
-               goto socket_setattr_failure;

        sk_inet = inet_sk(sk);
        if (sk_inet->is_icsk) {
Index: net-2.6_sockopt/net/ipv4/ip_options.c
===================================================================
--- net-2.6_sockopt.orig/net/ipv4/ip_options.c
+++ net-2.6_sockopt/net/ipv4/ip_options.c
@@ -443,7 +443,7 @@ int ip_options_compile(struct ip_options
                                opt->router_alert = optptr - iph;
                        break;
                      case IPOPT_CIPSO:
-                       if (opt->cipso) {
+                       if ((!skb && !capable(CAP_NET_RAW)) || opt->cipso) {
                                pp_ptr = optptr;
                                goto error;
                        }
Index: net-2.6_sockopt/security/selinux/hooks.c
===================================================================
--- net-2.6_sockopt.orig/security/selinux/hooks.c
+++ net-2.6_sockopt/security/selinux/hooks.c
@@ -3313,7 +3313,13 @@ static int selinux_socket_getpeername(st

 static int selinux_socket_setsockopt(struct socket *sock,int level,int optname) {
-       return socket_has_perm(current, sock, SOCKET__SETOPT);
+       int err;
+
+       err = socket_has_perm(current, sock, SOCKET__SETOPT);
+       if (err)
+               return err;
+
+       return selinux_netlbl_socket_setsockopt(sock, level, optname);
 }

 static int selinux_socket_getsockopt(struct socket *sock, int level,
Index: net-2.6_sockopt/security/selinux/include/selinux_netlabel.h
===================================================================
--- net-2.6_sockopt.orig/security/selinux/include/selinux_netlabel.h
+++ net-2.6_sockopt/security/selinux/include/selinux_netlabel.h
@@ -53,6 +53,9 @@ void selinux_netlbl_sk_security_init(str
 void selinux_netlbl_sk_clone_security(struct sk_security_struct *ssec,
                                      struct sk_security_struct *newssec);
 int selinux_netlbl_inode_permission(struct inode *inode, int mask);
+int selinux_netlbl_socket_setsockopt(struct socket *sock,
+                                    int level,
+                                    int optname);
 #else
 static inline void selinux_netlbl_cache_invalidate(void)
 {
@@ -114,6 +117,13 @@ static inline int selinux_netlbl_inode_p
 {
        return 0;
 }
+
+static inline int selinux_netlbl_socket_setsockopt(struct socket *sock,
+                                                  int level,
+                                                  int optname)
+{
+       return 0;
+}
 #endif /* CONFIG_NETLABEL */

 #endif
Index: net-2.6_sockopt/security/selinux/ss/services.c
===================================================================
--- net-2.6_sockopt.orig/security/selinux/ss/services.c
+++ net-2.6_sockopt/security/selinux/ss/services.c
@@ -2576,20 +2576,19 @@ int selinux_netlbl_inode_permission(stru
        struct sk_security_struct *sksec;
        struct socket *sock;

-       if (!S_ISSOCK(inode->i_mode))
+       if (!S_ISSOCK(inode->i_mode) || !(mask & (MAY_WRITE | MAY_APPEND)))
                return 0;

        sock = SOCKET_I(inode);
        isec = inode->i_security;
        sksec = sock->sk->sk_security;
        mutex_lock(&isec->lock);
-       if (unlikely(sksec->nlbl_state == NLBL_REQUIRE &&
-                    (mask & (MAY_WRITE | MAY_APPEND)))) {
-               lock_sock(sock->sk);
+       lock_sock(sock->sk);
+       if (unlikely(sksec->nlbl_state == NLBL_REQUIRE))
                rc = selinux_netlbl_socket_setsid(sock, sksec->sid);
-               release_sock(sock->sk);
-       } else
+       else
                rc = 0;
+       release_sock(sock->sk);
        mutex_unlock(&isec->lock);

        return rc;
@@ -2682,4 +2681,39 @@ u32 selinux_netlbl_socket_getpeersec_dgr

        return peer_sid;
 }
+
+/**
+ * selinux_netlbl_socket_setsockopt - Do not allow users to remove a NetLabel
+ * @sock: the socket
+ * @level: the socket level or protocol
+ * @optname: the socket option name
+ *
+ * Description:
+ * Check the setsockopt() call and if the user is trying to replace the IP
+ * options on a socket and a NetLabel is in place for the socket deny the
+ * access; otherwise allow the access.  Returns zero when the access is
+ * allowed, -EACCES when denied, and other negative values on error.
+ *
+ */
+int selinux_netlbl_socket_setsockopt(struct socket *sock,
+                                    int level,
+                                    int optname)
+{
+       int rc = 0;
+       struct sk_security_struct *sksec = sock->sk->sk_security;
+       struct netlbl_lsm_secattr secattr;
+
+       lock_sock(sock->sk);
+       if (level == IPPROTO_IP && optname == IP_OPTIONS &&
+           sksec->nlbl_state == NLBL_LABELED) {
+               netlbl_secattr_init(&secattr);
+               rc = netlbl_socket_getattr(sock, &secattr);
+               if (rc == 0 && (secattr.cache || secattr.mls_lvl_vld))
+                       rc = -EACCES;
+               netlbl_secattr_destroy(&secattr);
+       }
+       release_sock(sock->sk);
+
+       return rc;
+}
 #endif /* CONFIG_NETLABEL */

-- 
paul moore
linux security @ hp

--
This message was distributed to subscribers of the selinux mailing list.
If you no longer wish to subscribe, send mail to majordomo@tycho.nsa.gov with
the words "unsubscribe selinux" without quotes as the message.