Re: [PATCH v18 05/23] net: Prepare UDS for security module stacking
On Thu, Jul 9, 2020 at 12:28 PM John Johansen
wrote:
>
> On 7/9/20 9:11 AM, Stephen Smalley wrote:
> > On Wed, Jul 8, 2020 at 8:23 PM Casey Schaufler
> > wrote:
> >>
> >> Change the data used in UDS SO_PEERSEC processing from a
> >> secid to a more general struct lsmblob. Update the
> >> security_socket_getpeersec_dgram() interface to use the
> >> lsmblob. There is a small amount of scaffolding code
> >> that will come out when the security_secid_to_secctx()
> >> code is brought in line with the lsmblob.
> >>
> >> The secid field of the unix_skb_parms structure has been
> >> replaced with a pointer to an lsmblob structure, and the
> >> lsmblob is allocated as needed. This is similar to how the
> >> list of passed files is managed. While an lsmblob structure
> >> will fit in the available space today, there is no guarantee
> >> that the addition of other data to the unix_skb_parms or
> >> support for additional security modules wouldn't exceed what
> >> is available.
> >>
> >> Reviewed-by: Kees Cook
> >> Signed-off-by: Casey Schaufler
> >> Cc: netdev@vger.kernel.org
> >> ---
> >
> >> diff --git a/net/unix/af_unix.c b/net/unix/af_unix.c
> >> index 3385a7a0b231..d246aefcf4da 100644
> >> --- a/net/unix/af_unix.c
> >> +++ b/net/unix/af_unix.c
> >> @@ -138,17 +138,23 @@ static struct hlist_head *unix_sockets_unbound(void
> >> *addr)
> >> #ifdef CONFIG_SECURITY_NETWORK
> >> static void unix_get_secdata(struct scm_cookie *scm, struct sk_buff *skb)
> >> {
> >> - UNIXCB(skb).secid = scm->secid;
> >> + UNIXCB(skb).lsmdata = kmemdup(&scm->lsmblob, sizeof(scm->lsmblob),
> >> + GFP_KERNEL);
> >> }
> >>
> >> static inline void unix_set_secdata(struct scm_cookie *scm, struct
> >> sk_buff *skb)
> >> {
> >> - scm->secid = UNIXCB(skb).secid;
> >> + if (likely(UNIXCB(skb).lsmdata))
> >> + scm->lsmblob = *(UNIXCB(skb).lsmdata);
> >> + else
> >> + lsmblob_init(&scm->lsmblob, 0);
> >> }
> >>
> >> static inline bool unix_secdata_eq(struct scm_cookie *scm, struct sk_buff
> >> *skb)
> >> {
> >> - return (scm->secid == UNIXCB(skb).secid);
> >> + if (likely(UNIXCB(skb).lsmdata))
> >> + return lsmblob_equal(&scm->lsmblob, UNIXCB(skb).lsmdata);
> >> + return false;
> >> }
> >
> > I don't think that this provides sensible behavior to userspace. On a
> > transient memory allocation failure, instead of returning an error to
> > the sender and letting them handle it, this will just proceed with
> > sending the message without its associated security information, and
> > potentially split messages on arbitrary boundaries because it cannot
> > tell whether the sender had the same security information. I think
> > you instead need to change unix_get_secdata() to return an error on
> > allocation failure and propagate that up to the sender. Not a fan of
> > this change in general both due to extra overhead on this code path
> > and potential for breakage on allocation failures. I know it was
> > motivated by paul's observation that we won't be able to fit many more
> > secids into the cb but not sure we have to go there prematurely,
> > especially absent its usage by upstream AA (no unix_stream_connect
> > hook implementation upstream). Also not sure how the whole bpf local
>
> I'm not sure how premature it is, I am running late for 5.9 but would
> like to land apparmor unix mediation in 5.10
Sorry I think I mischaracterized the condition under which this
support needs to be stacked. It seems to only be needed if using
SO_PASSSEC and SCM_SECURITY (i.e. datagram labeling), not just for
unix mediation or SO_PEERSEC IIUC. So not sure if that applies even
for downstream.
Re: [PATCH v18 05/23] net: Prepare UDS for security module stacking
On Wed, Jul 8, 2020 at 8:23 PM Casey Schaufler wrote:
>
> Change the data used in UDS SO_PEERSEC processing from a
> secid to a more general struct lsmblob. Update the
> security_socket_getpeersec_dgram() interface to use the
> lsmblob. There is a small amount of scaffolding code
> that will come out when the security_secid_to_secctx()
> code is brought in line with the lsmblob.
>
> The secid field of the unix_skb_parms structure has been
> replaced with a pointer to an lsmblob structure, and the
> lsmblob is allocated as needed. This is similar to how the
> list of passed files is managed. While an lsmblob structure
> will fit in the available space today, there is no guarantee
> that the addition of other data to the unix_skb_parms or
> support for additional security modules wouldn't exceed what
> is available.
>
> Reviewed-by: Kees Cook
> Signed-off-by: Casey Schaufler
> Cc: netdev@vger.kernel.org
> ---
> diff --git a/net/unix/af_unix.c b/net/unix/af_unix.c
> index 3385a7a0b231..d246aefcf4da 100644
> --- a/net/unix/af_unix.c
> +++ b/net/unix/af_unix.c
> @@ -138,17 +138,23 @@ static struct hlist_head *unix_sockets_unbound(void
> *addr)
> #ifdef CONFIG_SECURITY_NETWORK
> static void unix_get_secdata(struct scm_cookie *scm, struct sk_buff *skb)
> {
> - UNIXCB(skb).secid = scm->secid;
> + UNIXCB(skb).lsmdata = kmemdup(&scm->lsmblob, sizeof(scm->lsmblob),
> + GFP_KERNEL);
> }
>
> static inline void unix_set_secdata(struct scm_cookie *scm, struct sk_buff
> *skb)
> {
> - scm->secid = UNIXCB(skb).secid;
> + if (likely(UNIXCB(skb).lsmdata))
> + scm->lsmblob = *(UNIXCB(skb).lsmdata);
> + else
> + lsmblob_init(&scm->lsmblob, 0);
> }
>
> static inline bool unix_secdata_eq(struct scm_cookie *scm, struct sk_buff
> *skb)
> {
> - return (scm->secid == UNIXCB(skb).secid);
> + if (likely(UNIXCB(skb).lsmdata))
> + return lsmblob_equal(&scm->lsmblob, UNIXCB(skb).lsmdata);
> + return false;
> }
I don't think that this provides sensible behavior to userspace. On a
transient memory allocation failure, instead of returning an error to
the sender and letting them handle it, this will just proceed with
sending the message without its associated security information, and
potentially split messages on arbitrary boundaries because it cannot
tell whether the sender had the same security information. I think
you instead need to change unix_get_secdata() to return an error on
allocation failure and propagate that up to the sender. Not a fan of
this change in general both due to extra overhead on this code path
and potential for breakage on allocation failures. I know it was
motivated by paul's observation that we won't be able to fit many more
secids into the cb but not sure we have to go there prematurely,
especially absent its usage by upstream AA (no unix_stream_connect
hook implementation upstream). Also not sure how the whole bpf local
storage approach to supporting security modules (or at least bpf lsm)
might reduce need for expanding these structures?
Re: [PATCH v17 05/23] net: Prepare UDS for security module stacking
On Thu, May 14, 2020 at 7:25 PM Casey Schaufler wrote:
>
> Change the data used in UDS SO_PEERSEC processing from a
> secid to a more general struct lsmblob. Update the
> security_socket_getpeersec_dgram() interface to use the
> lsmblob. There is a small amount of scaffolding code
> that will come out when the security_secid_to_secctx()
> code is brought in line with the lsmblob.
>
> The secid field of the unix_skb_parms structure has been
> replaced with a pointer to an lsmblob structure, and the
> lsmblob is allocated as needed. This is similar to how the
> list of passed files is managed. While an lsmblob structure
> will fit in the available space today, there is no guarantee
> that the addition of other data to the unix_skb_parms or
> support for additional security modules wouldn't exceed what
> is available.
I preferred the previous approach (in v15 and earlier) but I see that
this was suggested by Paul. Lifecycle management of lsmdata seems
rather tenuous. I guess the real question is what does netdev prefer.
Regardless, you need to check for memory allocation failure below if
this approach stands.
> Reviewed-by: Kees Cook
> Signed-off-by: Casey Schaufler
> cc: netdev@vger.kernel.org
> ---
> diff --git a/net/unix/af_unix.c b/net/unix/af_unix.c
> index 3385a7a0b231..a5c1a029095d 100644
> --- a/net/unix/af_unix.c
> +++ b/net/unix/af_unix.c
> @@ -138,17 +138,18 @@ static struct hlist_head *unix_sockets_unbound(void
> *addr)
> #ifdef CONFIG_SECURITY_NETWORK
> static void unix_get_secdata(struct scm_cookie *scm, struct sk_buff *skb)
> {
> - UNIXCB(skb).secid = scm->secid;
> + UNIXCB(skb).lsmdata = kmemdup(&scm->lsmblob, sizeof(scm->lsmblob),
> + GFP_KERNEL);
> }
Somewhere you need to check for and handle kmemdup() failure here.
>
> static inline void unix_set_secdata(struct scm_cookie *scm, struct sk_buff
> *skb)
> {
> - scm->secid = UNIXCB(skb).secid;
> + scm->lsmblob = *(UNIXCB(skb).lsmdata);
> }
Lest we have a bad day here.
>
> static inline bool unix_secdata_eq(struct scm_cookie *scm, struct sk_buff
> *skb)
> {
> - return (scm->secid == UNIXCB(skb).secid);
> + return lsmblob_equal(&scm->lsmblob, UNIXCB(skb).lsmdata);
> }
Or here.
> diff --git a/net/unix/scm.c b/net/unix/scm.c
> index 8c40f2b32392..3094323935a4 100644
> --- a/net/unix/scm.c
> +++ b/net/unix/scm.c
> @@ -142,6 +142,12 @@ void unix_destruct_scm(struct sk_buff *skb)
> scm.pid = UNIXCB(skb).pid;
> if (UNIXCB(skb).fp)
> unix_detach_fds(&scm, skb);
> +#ifdef CONFIG_SECURITY_NETWORK
> + if (UNIXCB(skb).lsmdata) {
> + kfree(UNIXCB(skb).lsmdata);
> + UNIXCB(skb).lsmdata = NULL;
> + }
> +#endif
Does this suffice to ensure that lsmdata is always freed? Seems
weakly connected to the allocation.
Re: [PATCH net] selinux: do not report error on connect(AF_UNSPEC)
On 5/8/19 2:27 PM, Marcelo Ricardo Leitner wrote:
On Wed, May 08, 2019 at 02:13:17PM -0400, Stephen Smalley wrote:
On 5/8/19 2:12 PM, Stephen Smalley wrote:
On 5/8/19 9:32 AM, Paolo Abeni wrote:
calling connect(AF_UNSPEC) on an already connected TCP socket is an
established way to disconnect() such socket. After commit 68741a8adab9
("selinux: Fix ltp test connect-syscall failure") it no longer works
and, in the above scenario connect() fails with EAFNOSUPPORT.
Fix the above falling back to the generic/old code when the address
family
is not AF_INET{4,6}, but leave the SCTP code path untouched, as it has
specific constraints.
Fixes: 68741a8adab9 ("selinux: Fix ltp test connect-syscall failure")
Reported-by: Tom Deseyn
Signed-off-by: Paolo Abeni
---
security/selinux/hooks.c | 8
1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
index c61787b15f27..d82b87c16b0a 100644
--- a/security/selinux/hooks.c
+++ b/security/selinux/hooks.c
@@ -4649,7 +4649,7 @@ static int
selinux_socket_connect_helper(struct socket *sock,
struct lsm_network_audit net = {0,};
struct sockaddr_in *addr4 = NULL;
struct sockaddr_in6 *addr6 = NULL;
- unsigned short snum;
+ unsigned short snum = 0;
u32 sid, perm;
/* sctp_connectx(3) calls via selinux_sctp_bind_connect()
@@ -4674,12 +4674,12 @@ static int
selinux_socket_connect_helper(struct socket *sock,
break;
default:
/* Note that SCTP services expect -EINVAL, whereas
- * others expect -EAFNOSUPPORT.
+ * others must handle this at the protocol level:
+ * connect(AF_UNSPEC) on a connected socket is
+ * a documented way disconnect the socket.
*/
if (sksec->sclass == SECCLASS_SCTP_SOCKET)
return -EINVAL;
- else
- return -EAFNOSUPPORT;
I think we need to return 0 here. Otherwise, we'll fall through with an
uninitialized snum, triggering a random/bogus permission check.
Sorry, I see that you initialize snum above. Nonetheless, I think the
correct behavior here is to skip the check since this is a disconnect, not a
connect.
Skipping the check would make it less controllable. So should it
somehow re-use shutdown() stuff? It gets very confusing, and after
all, it still is, in essence, a connect() syscall.
The function checks CONNECT permission on entry, before reaching this
point. This logic is only in preparation for a further check
(NAME_CONNECT) on the port. In this case, there is no further check to
perform and we can just return.
}
err = sel_netport_sid(sk->sk_protocol, snum, &sid);
Re: [PATCH net] selinux: do not report error on connect(AF_UNSPEC)
On 5/8/19 2:12 PM, Stephen Smalley wrote:
On 5/8/19 9:32 AM, Paolo Abeni wrote:
calling connect(AF_UNSPEC) on an already connected TCP socket is an
established way to disconnect() such socket. After commit 68741a8adab9
("selinux: Fix ltp test connect-syscall failure") it no longer works
and, in the above scenario connect() fails with EAFNOSUPPORT.
Fix the above falling back to the generic/old code when the address
family
is not AF_INET{4,6}, but leave the SCTP code path untouched, as it has
specific constraints.
Fixes: 68741a8adab9 ("selinux: Fix ltp test connect-syscall failure")
Reported-by: Tom Deseyn
Signed-off-by: Paolo Abeni
---
security/selinux/hooks.c | 8
1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
index c61787b15f27..d82b87c16b0a 100644
--- a/security/selinux/hooks.c
+++ b/security/selinux/hooks.c
@@ -4649,7 +4649,7 @@ static int selinux_socket_connect_helper(struct
socket *sock,
struct lsm_network_audit net = {0,};
struct sockaddr_in *addr4 = NULL;
struct sockaddr_in6 *addr6 = NULL;
- unsigned short snum;
+ unsigned short snum = 0;
u32 sid, perm;
/* sctp_connectx(3) calls via selinux_sctp_bind_connect()
@@ -4674,12 +4674,12 @@ static int
selinux_socket_connect_helper(struct socket *sock,
break;
default:
/* Note that SCTP services expect -EINVAL, whereas
- * others expect -EAFNOSUPPORT.
+ * others must handle this at the protocol level:
+ * connect(AF_UNSPEC) on a connected socket is
+ * a documented way disconnect the socket.
*/
if (sksec->sclass == SECCLASS_SCTP_SOCKET)
return -EINVAL;
- else
- return -EAFNOSUPPORT;
I think we need to return 0 here. Otherwise, we'll fall through with an
uninitialized snum, triggering a random/bogus permission check.
Sorry, I see that you initialize snum above. Nonetheless, I think the
correct behavior here is to skip the check since this is a disconnect,
not a connect.
}
err = sel_netport_sid(sk->sk_protocol, snum, &sid);
Re: [PATCH net] selinux: do not report error on connect(AF_UNSPEC)
On 5/8/19 9:32 AM, Paolo Abeni wrote:
calling connect(AF_UNSPEC) on an already connected TCP socket is an
established way to disconnect() such socket. After commit 68741a8adab9
("selinux: Fix ltp test connect-syscall failure") it no longer works
and, in the above scenario connect() fails with EAFNOSUPPORT.
Fix the above falling back to the generic/old code when the address family
is not AF_INET{4,6}, but leave the SCTP code path untouched, as it has
specific constraints.
Fixes: 68741a8adab9 ("selinux: Fix ltp test connect-syscall failure")
Reported-by: Tom Deseyn
Signed-off-by: Paolo Abeni
---
security/selinux/hooks.c | 8
1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
index c61787b15f27..d82b87c16b0a 100644
--- a/security/selinux/hooks.c
+++ b/security/selinux/hooks.c
@@ -4649,7 +4649,7 @@ static int selinux_socket_connect_helper(struct socket
*sock,
struct lsm_network_audit net = {0,};
struct sockaddr_in *addr4 = NULL;
struct sockaddr_in6 *addr6 = NULL;
- unsigned short snum;
+ unsigned short snum = 0;
u32 sid, perm;
/* sctp_connectx(3) calls via selinux_sctp_bind_connect()
@@ -4674,12 +4674,12 @@ static int selinux_socket_connect_helper(struct socket
*sock,
break;
default:
/* Note that SCTP services expect -EINVAL, whereas
-* others expect -EAFNOSUPPORT.
+* others must handle this at the protocol level:
+* connect(AF_UNSPEC) on a connected socket is
+* a documented way disconnect the socket.
*/
if (sksec->sclass == SECCLASS_SCTP_SOCKET)
return -EINVAL;
- else
- return -EAFNOSUPPORT;
I think we need to return 0 here. Otherwise, we'll fall through with an
uninitialized snum, triggering a random/bogus permission check.
}
err = sel_netport_sid(sk->sk_protocol, snum, &sid);
Re: [PATCH] NETWORKING: avoid use IPCB in cipso_v4_error
On 2/14/19 1:00 PM, Nazarov Sergey wrote:
Hi, Paul!
I've found the problem and testing it with some very specific custom lsm
module. The test case was simple:
standard TCP/IP client-server application, where server opens CIPSO labeled TCP
socket, and client connecting
to this socket with forbidden labels. After several connections kernel crashing
with general memory protection or
kernel cache inconsistent error.
I think, the similar behaviour should be with selinux or smack in the same
conditions. But I don't know them
so good to reproduce situation.
For SELinux, you can use
https://github.com/SELinuxProject/selinux-testsuite
That includes testing of CIPSO, both connecting from a client with an
authorized level and from a client with an unauthorized level.
Not sure about Smack; there were some tests in LTP but I don't know if
they would exercise it.
After applying patch, I haven't kernel crashes.
But now I've made additional checks and found no response icmp packets. The
ip_options_compile requires
CAP_NET_RAW capability when CIPSO option compiling, if skb is NULL. I have no
other ideas than returning to
the early patch version with ip_options_compile modified. What do you think
about that?
14.02.2019, 00:42, "Paul Moore" :
On Tue, Feb 12, 2019 at 10:10 AM Nazarov Sergey wrote:
Since cipso_v4_error might be called from different network stack layers, we
can't safely use icmp_send there.
icmp_send copies IP options with ip_option_echo, which uses IPCB to take
access to IP header compiled data.
But after commit 971f10ec ("tcp: better TCP_SKB_CB layout to reduce cache line
misses"), IPCB can't be used
above IP layer.
This patch fixes the problem by creating in cipso_v4_error a local copy of
compiled IP options and using it with
introduced __icmp_send function. This looks some overloaded, but in quite
rare error conditions only.
The original discussion is here:
https://lore.kernel.org/linux-security-module/16659801547571...@sas1-890ba5c2334a.qloud-c.yandex.net/
Signed-off-by: Sergey Nazarov
---
include/net/icmp.h | 9 -
net/ipv4/cipso_ipv4.c | 18 --
net/ipv4/icmp.c | 7 ---
3 files changed, 28 insertions(+), 6 deletions(-)
Hi Sergey,
Thanks for your work on finding this and putting a fix together. As
we discussed previously, I think this looks good, but can you describe
the testing you did to verify that this works correctly?
diff --git a/include/net/icmp.h b/include/net/icmp.h
index 6ac3a5b..e0f709d 100644
--- a/include/net/icmp.h
+++ b/include/net/icmp.h
@@ -22,6 +22,7 @@
#include
#include
+#include
struct icmp_err {
int errno;
@@ -39,7 +40,13 @@ struct icmp_err {
struct sk_buff;
struct net;
-void icmp_send(struct sk_buff *skb_in, int type, int code, __be32 info);
+void __icmp_send(struct sk_buff *skb_in, int type, int code, __be32 info,
+ const struct ip_options *opt);
+static inline void icmp_send(struct sk_buff *skb_in, int type, int code,
__be32 info)
+{
+ __icmp_send(skb_in, type, code, info, &IPCB(skb_in)->opt);
+}
+
int icmp_rcv(struct sk_buff *skb);
int icmp_err(struct sk_buff *skb, u32 info);
int icmp_init(void);
diff --git a/net/ipv4/cipso_ipv4.c b/net/ipv4/cipso_ipv4.c
index 777fa3b..234d12e 100644
--- a/net/ipv4/cipso_ipv4.c
+++ b/net/ipv4/cipso_ipv4.c
@@ -1735,13 +1735,27 @@ int cipso_v4_validate(const struct sk_buff *skb,
unsigned char **option)
*/
void cipso_v4_error(struct sk_buff *skb, int error, u32 gateway)
{
+ unsigned char optbuf[sizeof(struct ip_options) + 40];
+ struct ip_options *opt = (struct ip_options *)optbuf;
+
if (ip_hdr(skb)->protocol == IPPROTO_ICMP || error != -EACCES)
return;
+ /*
+ * We might be called above the IP layer,
+ * so we can not use icmp_send and IPCB here.
+ */
+
+ memset(opt, 0, sizeof(struct ip_options));
+ opt->optlen = ip_hdr(skb)->ihl*4 - sizeof(struct iphdr);
+ memcpy(opt->__data, (unsigned char *)&(ip_hdr(skb)[1]), opt->optlen);
+ if (ip_options_compile(dev_net(skb->dev), opt, NULL))
+ return;
+
if (gateway)
- icmp_send(skb, ICMP_DEST_UNREACH, ICMP_NET_ANO, 0);
+ __icmp_send(skb, ICMP_DEST_UNREACH, ICMP_NET_ANO, 0, opt);
else
- icmp_send(skb, ICMP_DEST_UNREACH, ICMP_HOST_ANO, 0);
+ __icmp_send(skb, ICMP_DEST_UNREACH, ICMP_HOST_ANO, 0, opt);
}
/**
diff --git a/net/ipv4/icmp.c b/net/ipv4/icmp.c
index 065997f..3f24414 100644
--- a/net/ipv4/icmp.c
+++ b/net/ipv4/icmp.c
@@ -570,7 +570,8 @@ static void icmp_reply(struct icmp_bxm *icmp_param,
struct sk_buff *skb)
* MUST reply to only the first fragment.
*/
-void icmp_send(struct sk_buff *skb_in, int type, int code, __be32 info)
+void __icmp_send(struct sk_buff *skb_in, int type, int code, __be32 info,
+ const struct ip_options *opt)
{
struct iphdr *iph;
int room;
@@ -691,7 +692,7 @@ void i
Re: [PATCH] selinux: add AF_UNSPEC and INADDR_ANY checks to selinux_socket_bind()
On 05/09/2018 11:01 AM, Paul Moore wrote:
> On Wed, May 9, 2018 at 8:37 AM, Stephen Smalley wrote:
>> On 05/08/2018 08:25 PM, Paul Moore wrote:
>>> On Tue, May 8, 2018 at 2:40 PM, Stephen Smalley wrote:
>>>> On 05/08/2018 01:05 PM, Paul Moore wrote:
>>>>> On Tue, May 8, 2018 at 10:05 AM, Alexey Kodanev
>>>>> wrote:
>>>>>> Commit d452930fd3b9 ("selinux: Add SCTP support") breaks compatibility
>>>>>> with the old programs that can pass sockaddr_in with AF_UNSPEC and
>>>>>> INADDR_ANY to bind(). As a result, bind() returns EAFNOSUPPORT error.
>>>>>> It was found with LTP/asapi_01 test.
>>>>>>
>>>>>> Similar to commit 29c486df6a20 ("net: ipv4: relax AF_INET check in
>>>>>> bind()"), which relaxed AF_INET check for compatibility, add AF_UNSPEC
>>>>>> case to AF_INET and make sure that the address is INADDR_ANY.
>>>>>>
>>>>>> Also, in the end of selinux_socket_bind(), instead of adding AF_UNSPEC
>>>>>> to 'address->sa_family == AF_INET', verify AF_INET6 first.
>>>>>>
>>>>>> Fixes: d452930fd3b9 ("selinux: Add SCTP support")
>>>>>> Signed-off-by: Alexey Kodanev
>>>>>> ---
>>>>>> security/selinux/hooks.c | 12 +---
>>>>>> 1 file changed, 9 insertions(+), 3 deletions(-)
>>>>>
>>>>> Thanks for finding and reporting this regression.
>>>>>
>>>>> I think I would prefer to avoid having to duplicate the
>>>>> AF_UNSPEC/INADDR_ANY checking logic in the SELinux hook, even though
>>>>> it is a small bit of code and unlikely to change. I'm wondering if it
>>>>> would be better to check both the socket and sockaddr address family
>>>>> in the main if conditional inside selinux_socket_bind(), what do you
>>>>> think? Another option would be to go back to just checking the
>>>>> soackaddr address family; we moved away from that for a reason which
>>>>> escapes at the moment (code cleanliness?), but perhaps that was a
>>>>> mistake.
>>>>
>>>> We've always used the sk->sk_family there; it was only the recent code
>>>> from Richard that started
>>>> using the socket address family.
>>>
>>> Yes I know, I thought I was the one that suggested it at some point
>>> (I'll take the blame) ... although now that I'm looking at the git
>>> log, maybe I'm confusing it with something else.
>>>
>>>>> diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
>>>>> index 4cafe6a19167..a3789b167667 100644
>>>>> --- a/security/selinux/hooks.c
>>>>> +++ b/security/selinux/hooks.c
>>>>> @@ -4577,6 +4577,7 @@ static int selinux_socket_bind(struct socket *sock,
>>>>> struc>
>>>>> {
>>>>>struct sock *sk = sock->sk;
>>>>>u16 family;
>>>>> + u16 family_sa;
>>>>>int err;
>>>>>
>>>>>err = sock_has_perm(sk, SOCKET__BIND);
>>>>> @@ -4585,7 +4586,9 @@ static int selinux_socket_bind(struct socket *sock,
>>>>> struc>
>>>>>
>>>>>/* If PF_INET or PF_INET6, check name_bind permission for the
>>>>> port. */
>>>>>family = sk->sk_family;
>>>>> - if (family == PF_INET || family == PF_INET6) {
>>>>> + family_sa = address->sa_family;
>>>>> + if ((family == PF_INET || family == PF_INET6) &&
>>>>> + (family_sa == PF_INET || family_sa == PF_INET6)) {
>>>>
>>>> Wouldn't this allow bypassing the name_bind permission check by passing in
>>>> AF_UNSPEC?
>>>
>>> I believe these name_bind permission checkis skipped for AF_UNSPEC
>>> already, isn't it? The only way the name_bind check would be
>>> triggered is if the source port, snum, was non-zero and I didn't think
>>> that was really legal for AF_UNSPEC/INADDR_ANY, is it?
>>
>> 1) What in inet_bind() prevents that from occurring?
>> 2) Regardless, what about the node_bind check?
>
> Fair enough. As mentioned above, perhaps the right fix is to move the
> address family checking back to how it was pre-SCTP.
It isn't clear
Re: [PATCH] selinux: add AF_UNSPEC and INADDR_ANY checks to selinux_socket_bind()
On 05/08/2018 08:25 PM, Paul Moore wrote:
> On Tue, May 8, 2018 at 2:40 PM, Stephen Smalley wrote:
>> On 05/08/2018 01:05 PM, Paul Moore wrote:
>>> On Tue, May 8, 2018 at 10:05 AM, Alexey Kodanev
>>> wrote:
>>>> Commit d452930fd3b9 ("selinux: Add SCTP support") breaks compatibility
>>>> with the old programs that can pass sockaddr_in with AF_UNSPEC and
>>>> INADDR_ANY to bind(). As a result, bind() returns EAFNOSUPPORT error.
>>>> It was found with LTP/asapi_01 test.
>>>>
>>>> Similar to commit 29c486df6a20 ("net: ipv4: relax AF_INET check in
>>>> bind()"), which relaxed AF_INET check for compatibility, add AF_UNSPEC
>>>> case to AF_INET and make sure that the address is INADDR_ANY.
>>>>
>>>> Also, in the end of selinux_socket_bind(), instead of adding AF_UNSPEC
>>>> to 'address->sa_family == AF_INET', verify AF_INET6 first.
>>>>
>>>> Fixes: d452930fd3b9 ("selinux: Add SCTP support")
>>>> Signed-off-by: Alexey Kodanev
>>>> ---
>>>> security/selinux/hooks.c | 12 +---
>>>> 1 file changed, 9 insertions(+), 3 deletions(-)
>>>
>>> Thanks for finding and reporting this regression.
>>>
>>> I think I would prefer to avoid having to duplicate the
>>> AF_UNSPEC/INADDR_ANY checking logic in the SELinux hook, even though
>>> it is a small bit of code and unlikely to change. I'm wondering if it
>>> would be better to check both the socket and sockaddr address family
>>> in the main if conditional inside selinux_socket_bind(), what do you
>>> think? Another option would be to go back to just checking the
>>> soackaddr address family; we moved away from that for a reason which
>>> escapes at the moment (code cleanliness?), but perhaps that was a
>>> mistake.
>>
>> We've always used the sk->sk_family there; it was only the recent code from
>> Richard that started
>> using the socket address family.
>
> Yes I know, I thought I was the one that suggested it at some point
> (I'll take the blame) ... although now that I'm looking at the git
> log, maybe I'm confusing it with something else.
>
>>> diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
>>> index 4cafe6a19167..a3789b167667 100644
>>> --- a/security/selinux/hooks.c
>>> +++ b/security/selinux/hooks.c
>>> @@ -4577,6 +4577,7 @@ static int selinux_socket_bind(struct socket *sock,
>>> struc>
>>> {
>>>struct sock *sk = sock->sk;
>>>u16 family;
>>> + u16 family_sa;
>>>int err;
>>>
>>>err = sock_has_perm(sk, SOCKET__BIND);
>>> @@ -4585,7 +4586,9 @@ static int selinux_socket_bind(struct socket *sock,
>>> struc>
>>>
>>>/* If PF_INET or PF_INET6, check name_bind permission for the port.
>>> */
>>>family = sk->sk_family;
>>> - if (family == PF_INET || family == PF_INET6) {
>>> + family_sa = address->sa_family;
>>> + if ((family == PF_INET || family == PF_INET6) &&
>>> + (family_sa == PF_INET || family_sa == PF_INET6)) {
>>
>> Wouldn't this allow bypassing the name_bind permission check by passing in
>> AF_UNSPEC?
>
> I believe these name_bind permission checkis skipped for AF_UNSPEC
> already, isn't it? The only way the name_bind check would be
> triggered is if the source port, snum, was non-zero and I didn't think
> that was really legal for AF_UNSPEC/INADDR_ANY, is it?
1) What in inet_bind() prevents that from occurring?
2) Regardless, what about the node_bind check?
>
>>>char *addrp;
>>>struct sk_security_struct *sksec = sk->sk_security;
>>>struct common_audit_data ad;
>>> @@ -4601,7 +4604,7 @@ static int selinux_socket_bind(struct socket *sock,
>>> struc>
>>> * need to check address->sa_family as it is possible to have
>>> * sk->sk_family = PF_INET6 with addr->sa_family = AF_INET.
>>> */
>>> - switch (address->sa_family) {
>>> + switch (family_sa) {
>>>case AF_INET:
>>>if (addrlen < sizeof(struct sockaddr_in))
>>>return -EINVAL;
>>>
>>>> diff --git a/security/selinux/
Re: [PATCH] selinux: add AF_UNSPEC and INADDR_ANY checks to selinux_socket_bind()
On 05/08/2018 01:05 PM, Paul Moore wrote:
> On Tue, May 8, 2018 at 10:05 AM, Alexey Kodanev
> wrote:
>> Commit d452930fd3b9 ("selinux: Add SCTP support") breaks compatibility
>> with the old programs that can pass sockaddr_in with AF_UNSPEC and
>> INADDR_ANY to bind(). As a result, bind() returns EAFNOSUPPORT error.
>> It was found with LTP/asapi_01 test.
>>
>> Similar to commit 29c486df6a20 ("net: ipv4: relax AF_INET check in
>> bind()"), which relaxed AF_INET check for compatibility, add AF_UNSPEC
>> case to AF_INET and make sure that the address is INADDR_ANY.
>>
>> Also, in the end of selinux_socket_bind(), instead of adding AF_UNSPEC
>> to 'address->sa_family == AF_INET', verify AF_INET6 first.
>>
>> Fixes: d452930fd3b9 ("selinux: Add SCTP support")
>> Signed-off-by: Alexey Kodanev
>> ---
>> security/selinux/hooks.c | 12 +---
>> 1 file changed, 9 insertions(+), 3 deletions(-)
>
> Thanks for finding and reporting this regression.
>
> I think I would prefer to avoid having to duplicate the
> AF_UNSPEC/INADDR_ANY checking logic in the SELinux hook, even though
> it is a small bit of code and unlikely to change. I'm wondering if it
> would be better to check both the socket and sockaddr address family
> in the main if conditional inside selinux_socket_bind(), what do you
> think? Another option would be to go back to just checking the
> soackaddr address family; we moved away from that for a reason which
> escapes at the moment (code cleanliness?), but perhaps that was a
> mistake.
We've always used the sk->sk_family there; it was only the recent code from
Richard that started
using the socket address family.
>
> diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
> index 4cafe6a19167..a3789b167667 100644
> --- a/security/selinux/hooks.c
> +++ b/security/selinux/hooks.c
> @@ -4577,6 +4577,7 @@ static int selinux_socket_bind(struct socket *sock,
> struc>
> {
>struct sock *sk = sock->sk;
>u16 family;
> + u16 family_sa;
>int err;
>
>err = sock_has_perm(sk, SOCKET__BIND);
> @@ -4585,7 +4586,9 @@ static int selinux_socket_bind(struct socket *sock,
> struc>
>
>/* If PF_INET or PF_INET6, check name_bind permission for the port. */
>family = sk->sk_family;
> - if (family == PF_INET || family == PF_INET6) {
> + family_sa = address->sa_family;
> + if ((family == PF_INET || family == PF_INET6) &&
> + (family_sa == PF_INET || family_sa == PF_INET6)) {
Wouldn't this allow bypassing the name_bind permission check by passing in
AF_UNSPEC?
>char *addrp;
>struct sk_security_struct *sksec = sk->sk_security;
>struct common_audit_data ad;
> @@ -4601,7 +4604,7 @@ static int selinux_socket_bind(struct socket *sock,
> struc>
> * need to check address->sa_family as it is possible to have
> * sk->sk_family = PF_INET6 with addr->sa_family = AF_INET.
> */
> - switch (address->sa_family) {
> + switch (family_sa) {
>case AF_INET:
>if (addrlen < sizeof(struct sockaddr_in))
>return -EINVAL;
>
>> diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
>> index 4cafe6a..649a3be 100644
>> --- a/security/selinux/hooks.c
>> +++ b/security/selinux/hooks.c
>> @@ -4602,10 +4602,16 @@ static int selinux_socket_bind(struct socket *sock,
>> struct sockaddr *address, in
>> * sk->sk_family = PF_INET6 with addr->sa_family = AF_INET.
>> */
>> switch (address->sa_family) {
>> + case AF_UNSPEC:
>> case AF_INET:
>> if (addrlen < sizeof(struct sockaddr_in))
>> return -EINVAL;
>> addr4 = (struct sockaddr_in *)address;
>> +
>> + if (address->sa_family == AF_UNSPEC &&
>> + addr4->sin_addr.s_addr != htonl(INADDR_ANY))
>> + return -EAFNOSUPPORT;
>> +
>> snum = ntohs(addr4->sin_port);
>> addrp = (char *)&addr4->sin_addr.s_addr;
>> break;
>> @@ -4681,10 +4687,10 @@ static int selinux_socket_bind(struct socket *sock,
>> struct sockaddr *address, in
>> ad.u.net->sport = htons(snum);
>> ad.u.net->family = family;
>>
>> - if (address->sa_family == AF_INET)
>> - ad.u.net->v4info.saddr = addr4->sin_addr.s_addr;
>> - else
>> + if (address->sa_family == AF_INET6)
>> ad.u.net->v6info.saddr = addr6->sin6_addr;
>> + else
>> + ad.u.net->v4info.saddr = addr4->sin_addr.s_addr;
>>
>> err = avc_has_perm(&selinux_state,
>>sksec->sid, sid,
>> --
Re: [PATCH 3/3] selinux: provide unix_stream_socketpair callback
On 04/23/2018 09:30 AM, David Herrmann wrote:
> Make sure to implement the new unix_stream_socketpair callback so the
> SO_PEERSEC call on socketpair(2)s will return correct information.
>
> Signed-off-by: David Herrmann
Acked-by: Stephen Smalley
> ---
> security/selinux/hooks.c | 14 ++
> 1 file changed, 14 insertions(+)
>
> diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
> index 4cafe6a19167..828881d9a41d 100644
> --- a/security/selinux/hooks.c
> +++ b/security/selinux/hooks.c
> @@ -4905,6 +4905,18 @@ static int selinux_socket_unix_stream_connect(struct
> sock *sock,
> return 0;
> }
>
> +static int selinux_socket_unix_stream_socketpair(struct sock *socka,
> + struct sock *sockb)
> +{
> + struct sk_security_struct *sksec_a = socka->sk_security;
> + struct sk_security_struct *sksec_b = sockb->sk_security;
> +
> + sksec_a->peer_sid = sksec_b->sid;
> + sksec_b->peer_sid = sksec_a->sid;
> +
> + return 0;
> +}
> +
> static int selinux_socket_unix_may_send(struct socket *sock,
> struct socket *other)
> {
> @@ -6995,6 +7007,8 @@ static struct security_hook_list selinux_hooks[]
> __lsm_ro_after_init = {
> LSM_HOOK_INIT(inode_getsecctx, selinux_inode_getsecctx),
>
> LSM_HOOK_INIT(unix_stream_connect, selinux_socket_unix_stream_connect),
> + LSM_HOOK_INIT(unix_stream_socketpair,
> + selinux_socket_unix_stream_socketpair),
> LSM_HOOK_INIT(unix_may_send, selinux_socket_unix_may_send),
>
> LSM_HOOK_INIT(socket_create, selinux_socket_create),
>
Re: [BUG] kernel stack corruption during/after Netlabel error
On Wed, 2017-11-29 at 09:34 -0800, Eric Dumazet wrote: > On Wed, Nov 29, 2017 at 9:31 AM, Stephen Smalley > wrote: > > On Wed, 2017-11-29 at 21:26 +1100, James Morris wrote: > > > I'm seeing a kernel stack corruption bug (detected via gcc) when > > > running > > > the SELinux testsuite on a 4.15-rc1 kernel, in the 2nd > > > inet_socket > > > test: > > > > > > https://github.com/SELinuxProject/selinux-testsuite/blob/master/t > > > ests > > > /inet_socket/test > > > > > > # Verify that unauthorized client cannot communicate with the > > > server. > > > $result = system > > > "runcon -t test_inet_bad_client_t -- $basedir/client stream > > > 127.0.0.1 65535 2>&1"; > > > > > > This correctlly causes an access control error in the Netlabel > > > code, > > > and > > > the bug seems to be triggered during the ICMP send: > > > > > > [ 339.806024] SELinux: failure in selinux_parse_skb(), unable to > > > parse packet > > > [ 339.822505] Kernel panic - not syncing: stack-protector: > > > Kernel > > > stack is corrupted in: 81745af5 > > > [ 339.822505] > > > [ 339.852250] CPU: 4 PID: 3642 Comm: client Not tainted 4.15.0- > > > rc1- > > > test #15 > > > [ 339.868498] Hardware name: LENOVO 10FGS0VA1L/30BC, BIOS > > > FWKT68A 01/19/2017 > > > [ 339.885060] Call Trace: > > > [ 339.896875] > > > [ 339.908103] dump_stack+0x63/0x87 > > > [ 339.920645] panic+0xe8/0x248 > > > [ 339.932668] ? ip_push_pending_frames+0x33/0x40 > > > [ 339.946328] ? icmp_send+0x525/0x530 > > > [ 339.958861] ? kfree_skbmem+0x60/0x70 > > > [ 339.971431] __stack_chk_fail+0x1b/0x20 > > > [ 339.984049] icmp_send+0x525/0x530 > > > [ 339.996205] ? netlbl_skbuff_err+0x36/0x40 > > > [ 340.008997] ? selinux_netlbl_err+0x11/0x20 > > > [ 340.021816] ? selinux_socket_sock_rcv_skb+0x211/0x230 > > > [ 340.035529] ? security_sock_rcv_skb+0x3b/0x50 > > > [ 340.048471] ? sk_filter_trim_cap+0x44/0x1c0 > > > [ 340.061246] ? tcp_v4_inbound_md5_hash+0x69/0x1b0 > > > [ 340.074562] ? tcp_filter+0x2c/0x40 > > > [ 340.086400] ? tcp_v4_rcv+0x820/0xa20 > > > [ 340.098329] ? ip_local_deliver_finish+0x71/0x1a0 > > > [ 340.111279] ? ip_local_deliver+0x6f/0xe0 > > > [ 340.123535] ? ip_rcv_finish+0x3a0/0x3a0 > > > [ 340.135523] ? ip_rcv_finish+0xdb/0x3a0 > > > [ 340.147442] ? ip_rcv+0x27c/0x3c0 > > > [ 340.158668] ? inet_del_offload+0x40/0x40 > > > [ 340.170580] ? __netif_receive_skb_core+0x4ac/0x900 > > > [ 340.183285] ? rcu_accelerate_cbs+0x5b/0x80 > > > [ 340.195282] ? __netif_receive_skb+0x18/0x60 > > > [ 340.207288] ? process_backlog+0x95/0x140 > > > [ 340.218948] ? net_rx_action+0x26c/0x3b0 > > > [ 340.230416] ? __do_softirq+0xc9/0x26a > > > [ 340.241625] ? do_softirq_own_stack+0x2a/0x40 > > > [ 340.253368] > > > [ 340.262673] ? do_softirq+0x50/0x60 > > > [ 340.273450] ? __local_bh_enable_ip+0x57/0x60 > > > [ 340.285045] ? ip_finish_output2+0x175/0x350 > > > [ 340.296403] ? ip_finish_output+0x127/0x1d0 > > > [ 340.307665] ? nf_hook_slow+0x3c/0xb0 > > > [ 340.318230] ? ip_output+0x72/0xe0 > > > [ 340.328524] ? ip_fragment.constprop.54+0x80/0x80 > > > [ 340.340070] ? ip_local_out+0x35/0x40 > > > [ 340.350497] ? ip_queue_xmit+0x15c/0x3f0 > > > [ 340.361060] ? __kmalloc_reserve.isra.40+0x31/0x90 > > > [ 340.372484] ? __skb_clone+0x2e/0x130 > > > [ 340.382633] ? tcp_transmit_skb+0x558/0xa10 > > > [ 340.393262] ? tcp_connect+0x938/0xad0 > > > [ 340.403370] ? ktime_get_with_offset+0x4c/0xb0 > > > [ 340.414206] ? tcp_v4_connect+0x457/0x4e0 > > > [ 340.424471] ? __inet_stream_connect+0xb3/0x300 > > > [ 340.435195] ? inet_stream_connect+0x3b/0x60 > > > [ 340.445607] ? SYSC_connect+0xd9/0x110 > > > [ 340.455455] ? __audit_syscall_entry+0xaf/0x100 > > > [ 340.466112] ? syscall_trace_enter+0x1d0/0x2b0 > > > [ 340.476636] ? __audit_syscall_exit+0x209/0x290 > > > [ 340.487151] ? SyS_connect+0xe/0x10 > > > [ 340.496453] ? do_syscall_64+0x67/0x1b0 > > > [ 340.506078] ? entry_SYSCALL64_slow_path+0x25/0x25 > > > [ 340.516693] Kernel Offset: disabled > > > [ 340.526393] Rebooting in 11 seconds.. > > > > > >
Re: [BUG] kernel stack corruption during/after Netlabel error
On Wed, 2017-11-29 at 21:26 +1100, James Morris wrote: > I'm seeing a kernel stack corruption bug (detected via gcc) when > running > the SELinux testsuite on a 4.15-rc1 kernel, in the 2nd inet_socket > test: > > https://github.com/SELinuxProject/selinux-testsuite/blob/master/tests > /inet_socket/test > > # Verify that unauthorized client cannot communicate with the > server. > $result = system > "runcon -t test_inet_bad_client_t -- $basedir/client stream > 127.0.0.1 65535 2>&1"; > > This correctlly causes an access control error in the Netlabel code, > and > the bug seems to be triggered during the ICMP send: > > [ 339.806024] SELinux: failure in selinux_parse_skb(), unable to > parse packet > [ 339.822505] Kernel panic - not syncing: stack-protector: Kernel > stack is corrupted in: 81745af5 > [ 339.822505] > [ 339.852250] CPU: 4 PID: 3642 Comm: client Not tainted 4.15.0-rc1- > test #15 > [ 339.868498] Hardware name: LENOVO 10FGS0VA1L/30BC, BIOS > FWKT68A 01/19/2017 > [ 339.885060] Call Trace: > [ 339.896875] > [ 339.908103] dump_stack+0x63/0x87 > [ 339.920645] panic+0xe8/0x248 > [ 339.932668] ? ip_push_pending_frames+0x33/0x40 > [ 339.946328] ? icmp_send+0x525/0x530 > [ 339.958861] ? kfree_skbmem+0x60/0x70 > [ 339.971431] __stack_chk_fail+0x1b/0x20 > [ 339.984049] icmp_send+0x525/0x530 > [ 339.996205] ? netlbl_skbuff_err+0x36/0x40 > [ 340.008997] ? selinux_netlbl_err+0x11/0x20 > [ 340.021816] ? selinux_socket_sock_rcv_skb+0x211/0x230 > [ 340.035529] ? security_sock_rcv_skb+0x3b/0x50 > [ 340.048471] ? sk_filter_trim_cap+0x44/0x1c0 > [ 340.061246] ? tcp_v4_inbound_md5_hash+0x69/0x1b0 > [ 340.074562] ? tcp_filter+0x2c/0x40 > [ 340.086400] ? tcp_v4_rcv+0x820/0xa20 > [ 340.098329] ? ip_local_deliver_finish+0x71/0x1a0 > [ 340.111279] ? ip_local_deliver+0x6f/0xe0 > [ 340.123535] ? ip_rcv_finish+0x3a0/0x3a0 > [ 340.135523] ? ip_rcv_finish+0xdb/0x3a0 > [ 340.147442] ? ip_rcv+0x27c/0x3c0 > [ 340.158668] ? inet_del_offload+0x40/0x40 > [ 340.170580] ? __netif_receive_skb_core+0x4ac/0x900 > [ 340.183285] ? rcu_accelerate_cbs+0x5b/0x80 > [ 340.195282] ? __netif_receive_skb+0x18/0x60 > [ 340.207288] ? process_backlog+0x95/0x140 > [ 340.218948] ? net_rx_action+0x26c/0x3b0 > [ 340.230416] ? __do_softirq+0xc9/0x26a > [ 340.241625] ? do_softirq_own_stack+0x2a/0x40 > [ 340.253368] > [ 340.262673] ? do_softirq+0x50/0x60 > [ 340.273450] ? __local_bh_enable_ip+0x57/0x60 > [ 340.285045] ? ip_finish_output2+0x175/0x350 > [ 340.296403] ? ip_finish_output+0x127/0x1d0 > [ 340.307665] ? nf_hook_slow+0x3c/0xb0 > [ 340.318230] ? ip_output+0x72/0xe0 > [ 340.328524] ? ip_fragment.constprop.54+0x80/0x80 > [ 340.340070] ? ip_local_out+0x35/0x40 > [ 340.350497] ? ip_queue_xmit+0x15c/0x3f0 > [ 340.361060] ? __kmalloc_reserve.isra.40+0x31/0x90 > [ 340.372484] ? __skb_clone+0x2e/0x130 > [ 340.382633] ? tcp_transmit_skb+0x558/0xa10 > [ 340.393262] ? tcp_connect+0x938/0xad0 > [ 340.403370] ? ktime_get_with_offset+0x4c/0xb0 > [ 340.414206] ? tcp_v4_connect+0x457/0x4e0 > [ 340.424471] ? __inet_stream_connect+0xb3/0x300 > [ 340.435195] ? inet_stream_connect+0x3b/0x60 > [ 340.445607] ? SYSC_connect+0xd9/0x110 > [ 340.455455] ? __audit_syscall_entry+0xaf/0x100 > [ 340.466112] ? syscall_trace_enter+0x1d0/0x2b0 > [ 340.476636] ? __audit_syscall_exit+0x209/0x290 > [ 340.487151] ? SyS_connect+0xe/0x10 > [ 340.496453] ? do_syscall_64+0x67/0x1b0 > [ 340.506078] ? entry_SYSCALL64_slow_path+0x25/0x25 > [ 340.516693] Kernel Offset: disabled > [ 340.526393] Rebooting in 11 seconds.. > > This is mostly reliable, and I'm only seeing it on bare metal (not in > a > virtualbox vm). > > The SELinux skb parse error at the start only sometimes appears, and > looking at the code, I suspect some kind of memory corruption being > the > cause at that point (basic packet header checks). > > I bisected the bug down to the following change: > > commit bffa72cf7f9df842f0016ba03586039296b4caaf > Author: Eric Dumazet > Date: Tue Sep 19 05:14:24 2017 -0700 > > net: sk_buff rbnode reorg > ... > > > Anyone else able to reproduce this, or have any ideas on what's > happening? So far I haven't been able to reproduce with 4.15-rc1 or -linus.
Re: [PATCH 4/4] selinux: Add SCTP support
On Tue, 2017-11-28 at 14:39 -0500, Stephen Smalley wrote:
> On Mon, 2017-11-27 at 19:32 +, Richard Haines wrote:
> > The SELinux SCTP implementation is explained in:
> > Documentation/security/SELinux-sctp.rst
> >
> > Signed-off-by: Richard Haines
> > ---
> > Documentation/security/SELinux-sctp.rst | 104
> > security/selinux/hooks.c| 278
> > +---
> > security/selinux/include/classmap.h | 2 +-
> > security/selinux/include/netlabel.h | 15 +-
> > security/selinux/include/objsec.h | 4 +
> > security/selinux/netlabel.c | 128 +--
> > 6 files changed, 499 insertions(+), 32 deletions(-)
> > create mode 100644 Documentation/security/SELinux-sctp.rst
> >
> > diff --git a/Documentation/security/SELinux-sctp.rst
> > b/Documentation/security/SELinux-sctp.rst
> > new file mode 100644
> > index 000..f6a9162
> > --- /dev/null
> > +++ b/Documentation/security/SELinux-sctp.rst
> > @@ -0,0 +1,104 @@
> > +SCTP SELinux Support
> > +=
> > +
> > +Security Hooks
> > +===
> > +
> > +The ``Documentation/security/LSM-sctp.rst`` document describes how
> > the
> > +following sctp security hooks are utilised::
> > +
> > +security_sctp_assoc_request()
> > +security_sctp_bind_connect()
> > +security_sctp_sk_clone()
> > +security_inet_conn_established()
> > +
> > +
> > +Policy Statements
> > +==
> > +The following class and permissions to support SCTP are available
> > within the
> > +kernel::
> > +
> > +class sctp_socket inherits socket { node_bind }
> > +
> > +whenever the following policy capability is enabled::
> > +
> > +policycap extended_socket_class;
> > +
> > +SELinux SCTP support adds the ``name_connect`` permission for
> > connecting
> > +to a specific port type and the ``association`` permission that is
> > explained
> > +in the section below.
> > +
> > +If userspace tools have been updated, SCTP will support the
> > ``portcon``
> > +statement as shown in the following example::
> > +
> > +portcon sctp 1024-1036 system_u:object_r:sctp_ports_t:s0
> > +
> > +
> > +SCTP Bind, Connect and ASCONF Chunk Parameter Permission Checks
> > +
> > +The hook ``security_sctp_bind_connect()`` is called by SCTP to
> > check
> > +permissions required for ipv4/ipv6 addresses based on the ``@optna
> > me
> > `` as
> > +follows::
> > +
> > + --
> >
> > + | BIND Permission
> > Checks |
> > + | @optname | @address
> > contains |
> > + ||
> > ---|
> > + | SCTP_SOCKOPT_BINDX_ADD | One or more ipv4 / ipv6 addresses
> > |
> > + | SCTP_PRIMARY_ADDR | Single ipv4 or ipv6
> > address |
> > + | SCTP_SET_PEER_PRIMARY_ADDR | Single ipv4 or ipv6
> > address |
> > + --
> >
> > +
> > + --
> >
> > + | CONNECT Permission
> > Checks |
> > + | @optname | @address
> > contains |
> > + ||
> > ---|
> > + | SCTP_SOCKOPT_CONNECTX | One or more ipv4 / ipv6 addresses
> > |
> > + | SCTP_PARAM_ADD_IP | One or more ipv4 / ipv6 addresses
> > |
> > + | SCTP_SENDMSG_CONNECT | Single ipv4 or ipv6
> > address |
> > + | SCTP_PARAM_SET_PRIMARY | Single ipv4 or ipv6
> > address |
> > + --
> >
> > +
> > +
> > +SCTP Peer Labeling
> > +===
> > +An SCTP socket will only have one peer label assigned to it. This
> > will be
> > +assigned during the establishment of the first association. Once
> > the
> > peer
> > +label has been assigned, any new associations will have the
> > ``association``
> > +permission validated by checking the socket peer sid against the
> > received
> > +packets peer sid to determine wh
Re: [PATCH 4/4] selinux: Add SCTP support
On Mon, 2017-11-27 at 19:32 +, Richard Haines wrote:
> The SELinux SCTP implementation is explained in:
> Documentation/security/SELinux-sctp.rst
>
> Signed-off-by: Richard Haines
> ---
> Documentation/security/SELinux-sctp.rst | 104
> security/selinux/hooks.c| 278
> +---
> security/selinux/include/classmap.h | 2 +-
> security/selinux/include/netlabel.h | 15 +-
> security/selinux/include/objsec.h | 4 +
> security/selinux/netlabel.c | 128 +--
> 6 files changed, 499 insertions(+), 32 deletions(-)
> create mode 100644 Documentation/security/SELinux-sctp.rst
>
> diff --git a/Documentation/security/SELinux-sctp.rst
> b/Documentation/security/SELinux-sctp.rst
> new file mode 100644
> index 000..f6a9162
> --- /dev/null
> +++ b/Documentation/security/SELinux-sctp.rst
> @@ -0,0 +1,104 @@
> +SCTP SELinux Support
> +=
> +
> +Security Hooks
> +===
> +
> +The ``Documentation/security/LSM-sctp.rst`` document describes how
> the
> +following sctp security hooks are utilised::
> +
> +security_sctp_assoc_request()
> +security_sctp_bind_connect()
> +security_sctp_sk_clone()
> +security_inet_conn_established()
> +
> +
> +Policy Statements
> +==
> +The following class and permissions to support SCTP are available
> within the
> +kernel::
> +
> +class sctp_socket inherits socket { node_bind }
> +
> +whenever the following policy capability is enabled::
> +
> +policycap extended_socket_class;
> +
> +SELinux SCTP support adds the ``name_connect`` permission for
> connecting
> +to a specific port type and the ``association`` permission that is
> explained
> +in the section below.
> +
> +If userspace tools have been updated, SCTP will support the
> ``portcon``
> +statement as shown in the following example::
> +
> +portcon sctp 1024-1036 system_u:object_r:sctp_ports_t:s0
> +
> +
> +SCTP Bind, Connect and ASCONF Chunk Parameter Permission Checks
> +
> +The hook ``security_sctp_bind_connect()`` is called by SCTP to check
> +permissions required for ipv4/ipv6 addresses based on the ``@optname
> `` as
> +follows::
> +
> + --
> + | BIND Permission Checks |
> + | @optname | @address contains |
> + ||---|
> + | SCTP_SOCKOPT_BINDX_ADD | One or more ipv4 / ipv6 addresses |
> + | SCTP_PRIMARY_ADDR | Single ipv4 or ipv6 address |
> + | SCTP_SET_PEER_PRIMARY_ADDR | Single ipv4 or ipv6 address |
> + --
> +
> + --
> + | CONNECT Permission Checks |
> + | @optname | @address contains |
> + ||---|
> + | SCTP_SOCKOPT_CONNECTX | One or more ipv4 / ipv6 addresses |
> + | SCTP_PARAM_ADD_IP | One or more ipv4 / ipv6 addresses |
> + | SCTP_SENDMSG_CONNECT | Single ipv4 or ipv6 address |
> + | SCTP_PARAM_SET_PRIMARY | Single ipv4 or ipv6 address |
> + --
> +
> +
> +SCTP Peer Labeling
> +===
> +An SCTP socket will only have one peer label assigned to it. This
> will be
> +assigned during the establishment of the first association. Once the
> peer
> +label has been assigned, any new associations will have the
> ``association``
> +permission validated by checking the socket peer sid against the
> received
> +packets peer sid to determine whether the association should be
> allowed or
> +denied.
> +
> +NOTES:
> + 1) If peer labeling is not enabled, then the peer context will
> always be
> + ``SECINITSID_UNLABELED`` (``unlabeled_t`` in Reference
> Policy).
> +
> + 2) As SCTP can support more than one transport address per
> endpoint
> + (multi-homing) on a single socket, it is possible to configure
> policy
> + and NetLabel to provide different peer labels for each of
> these. As the
> + socket peer label is determined by the first associations
> transport
> + address, it is recommended that all peer labels are
> consistent.
> +
> + 3) **getpeercon**\(3) may be used by userspace to retrieve the
> sockets peer
> + context.
> +
> + 4) While not SCTP specific, be aware when using NetLabel that if
> a label
> + is assigned to a specific interface, and that interface 'goes
> down',
> + then the NetLabel service will remove the entry. Therefore
> ensure that
> + the network startup scripts call **netlabelctl**\(8) to set
> the required
> + label (see
[regression, 4.14] xfrm: Fix stack-out-of-bounds read in xfrm_state_find breaks selinux-testsuite
Hi,
4.14 is failing the selinux-testsuite labeled IPSEC tests despite
having just been fixed in commit cf37966751747727 ("xfrm: do
unconditional template resolution before pcpu cache check"). The
breaking commit is the very next one, commit c9f3f813d462c72d ("xfrm:
Fix stack-out-of-bounds read in xfrm_state_find."). Unlike the earlier
breakage, which caused use of the wrong SA, this one leads to a failure
on connect(). Running ip xfrm monitor during one of the failing tests
shows the following:
acquire proto ah
sel src 127.0.0.1/32 dst 127.0.0.1/32 proto tcp sport 0 dport 65535
dev lo
policy src 127.0.0.1/32 dst 127.0.0.1/32 proto tcp
security context
unconfined_u:unconfined_r:test_inet_client_t:s0-s0:c0.c1023
dir out priority 0 ptype main
tmpl src 0.0.0.0 dst 0.0.0.0
proto ah reqid 0 mode transport
Expired src 127.0.0.1 dst 0.0.0.0
proto ah spi 0x reqid 0 mode transport
replay-window 0
sel src 127.0.0.1/32 dst 127.0.0.1/32 proto tcp sport 0 dport
65535 dev lo
hard 1
On the last working commit, connect() instead succeeds and ip xfrm
monitor shows the following:
Async event (0x20) timer expired
src 127.0.0.1 dst 127.0.0.1 reqid 0x0 protocol ah SPI 0x200
Async event (0x10) replay update
src 127.0.0.1 dst 127.0.0.1 reqid 0x0 protocol ah SPI 0x200
Async event (0x10) replay update
src 127.0.0.1 dst 127.0.0.1 reqid 0x0 protocol ah SPI 0x200
Reproducer:
# Install a Fedora VM w/ SELinux enabled (default).
$ git clone https://github.com/SELinuxProject/selinux-testsuite/
# Make sure you have the requisite kernel config options enabled.
$ cd linux
$ ./scripts/kconfig/merge_config.sh .config ../selinux-
testsuite/defconfig
$ make
$ sudo make modules_install install
$ sudo reboot
# Install dependencies.
sudo dnf install perl-Test perl-Test-Harness perl-Test-Simple selinux-
policy-devel gcc libselinux-devel net-tools netlabel_tools iptables
# Build and run the tests
sudo make test
After running once as above, you can run just the inet socket tests
again via:
cd tests/inet_socket
./test
Re: [RFC PATCH 5/5] selinux: Add SCTP support
On Mon, 2017-11-13 at 17:40 -0500, Paul Moore wrote:
> On Mon, Nov 13, 2017 at 5:05 PM, Richard Haines
> wrote:
> > On Mon, 2017-11-06 at 19:09 -0500, Paul Moore wrote:
> > > On Tue, Oct 17, 2017 at 9:59 AM, Richard Haines
> > > wrote:
> > > > The SELinux SCTP implementation is explained in:
> > > > Documentation/security/SELinux-sctp.txt
> > > >
> > > > Signed-off-by: Richard Haines
> > > > ---
> > > > Documentation/security/SELinux-sctp.txt | 108 +
> > > > security/selinux/hooks.c| 268
> > > > ++--
> > > > security/selinux/include/classmap.h | 3 +-
> > > > security/selinux/include/netlabel.h | 9 +-
> > > > security/selinux/include/objsec.h | 5 +
> > > > security/selinux/netlabel.c | 52 ++-
> > > > 6 files changed, 427 insertions(+), 18 deletions(-)
> > > > create mode 100644 Documentation/security/SELinux-sctp.txt
>
> ...
>
> > > > +Policy Statements
> > > > +==
> > > > +The following class and permissions to support SCTP are
> > > > available
> > > > within the
> > > > +kernel:
> > > > +class sctp_socket inherits socket { node_bind }
> > > > +
> > > > +whenever the following policy capability is enabled:
> > > > +policycap extended_socket_class;
> > > > +
> > > > +The SELinux SCTP support adds the additional permissions that
> > > > are
> > > > explained
> > > > +in the sections below:
> > > > +association bindx connectx
> > >
> > > Is the distinction between bind and bindx significant? The same
> > > question applies to connect/connectx. I think we can probably
> > > just
> > > reuse bind and connect in these cases.
> >
> > This has been discussed before with Marcelo and keeping
> > bindx/connectx
> > is a useful distinction.
>
> My apologies, I must have forgotten/missed that discussion. Do you
> have an archive pointer?
>
> > > > +SCTP Peer Labeling
> > > > +===
> > > > +An SCTP socket will only have one peer label assigned to it.
> > > > This
> > > > will be
> > > > +assigned during the establishment of the first association.
> > > > Once
> > > > the peer
> > > > +label has been assigned, any new associations will have the
> > > > "association"
> > > > +permission validated by checking the socket peer sid against
> > > > the
> > > > received
> > > > +packets peer sid to determine whether the association should
> > > > be
> > > > allowed or
> > > > +denied.
> > > > +
> > > > +NOTES:
> > > > + 1) If peer labeling is not enabled, then the peer context
> > > > will
> > > > always be
> > > > + SECINITSID_UNLABELED (unlabeled_t in Reference Policy).
> > > > +
> > > > + 2) As SCTP supports multiple endpoints with multi-homing on
> > > > a
> > > > single socket
> > > > + it is recommended that peer labels are consistent.
> > >
> > > My apologies if I'm confused, but I thought it was multiple
> > > associations per-endpoint, with the associations providing the
> > > multi-homing functionality, no?
> >
> > I've reworded to:
> > As SCTP can support more than one transport address per endpoint
> > (multi-homing) on a single socket, it is possible to configure
> > policy
> > and NetLabel to provide different peer labels for each of these. As
> > the
> > socket peer label is determined by the first associations transport
> > address, it is recommended that all peer labels are consistent.
>
> I'm still not sure this makes complete sense to me, but since I'm
> still not 100% confident in my understanding of SCTP I'm willing to
> punt on this for the moment.
>
> > > > + 3) getpeercon(3) may be used by userspace to retrieve the
> > > > sockets peer
> > > > + context.
> > > > +
> > > > + 4) If using NetLabel be aware that if a label is assigned
> > > > to a
> > > > specific
> > > > + interface, and that interface 'goes down', then the
> > > > NetLabel
> > > > service
> > > > + will remove the entry. Therefore ensure that the network
> > > > startup scripts
> > > > + call netlabelctl(8) to set the required label (see
> > > > netlabel-
> > > > config(8)
> > > > + helper script for details).
> > >
> > > Maybe this will be made clear as I work my way through this
> > > patch,
> > > but
> > > how is point #4 SCTP specific?
> >
> > It's not, I added this as a useful hint as I keep forgetting about
> > it,
> > I'll reword to: While not SCTP specific, be aware .
>
> Okay. Better.
>
> > > > +/* Called whenever SCTP receives an INIT or INIT_ACK chunk */
> > > > +static int selinux_sctp_assoc_request(struct sctp_endpoint
> > > > *ep,
> > > > + struct sk_buff *skb,
> > > > + int sctp_cid)
> > > > +{
> > > > + struct sk_security_struct *sksec = ep->base.sk-
> > > > > sk_security;
> > > >
> > > > + struct common_audit_data ad;
> > > > + struct lsm_network_audit net = {0,};
> > > > + u8 peerlbl_active;
> > > > + u32 peer
Re: [RFC PATCH] xfrm: fix regression introduced by xdst pcpu cache
On Wed, 2017-11-01 at 17:39 -0400, Paul Moore wrote: > On Tue, Oct 31, 2017 at 7:08 PM, Florian Westphal > wrote: > > Paul Moore wrote: > > > On Mon, Oct 30, 2017 at 10:58 AM, Stephen Smalley > > gov> wrote: > > > > matching before (as in this patch) or after calling > > > > xfrm_bundle_ok()? > > > > > > I would probably make the LSM call the last check, as you've > > > done; but > > > I have to say that is just so it is consistent with the "LSM > > > last" > > > philosophy and not because of any performance related argument. > > > > > > > ... Also, > > > > do we need to test xfrm->sel.family before calling > > > > xfrm_selector_match > > > > (as in this patch) or not - xfrm_state_look_at() does so when > > > > the > > > > state is XFRM_STATE_VALID but not when it is _ERROR or > > > > _EXPIRED? > > > > > > Speaking purely from a SELinux perspective, I'm not sure it > > > matters: > > > as long as the labels match we are happy. However, from a > > > general > > > IPsec perspective it does seem like a reasonable thing. > > > > > > Granted I'm probably missing something, but it seems a little odd > > > that > > > the code isn't already checking that the selectors match (... > > > what am > > > I missing?). It does check the policies, maybe that is enough in > > > the > > > normal IPsec case? > > > > The assumption was that identical policies would yield the same > > SAs, > > but thats not correct. > > Well, to be fair, I think the assumption is valid for normal, > unlabeled IPsec. The problem comes when SELinux starts labeling SAs > and now you have multiple SAs for a given policy, each differing only > in the SELinux/LSM label. No, it is invalid for normal, unlabeled IPSEC too, in the case where one has defined xfrm state selectors. That's what my other testsuite patch (which is presently only on the xfrmselectortest branch) is exercising - matching of xfrm state selectors. But in any event, Florian's patch fixes both, so I'm fine with it. I don't know though how it compares performance-wise with walking the bundle and just calling security_xfrm_state_pol_flow_match() and xfrm_selector_match() on each one. > > Considering that adding the SELinux/LSM label effectively adds an > additional selector, I'm wondering if we should simply add the > SELinux/LSM label matching to xfrm_selector_match()? Looking quickly > at the code it seems as though we always follow xfrm_selector_match() > with a LSM check anyway, the one exception being in > __xfrm_policy_check() ... which *might* be a valid exception, as we > don't do our access checks for inbound traffic at that point in the > stack. Possibly, but that should probably be a separate patch. We should just fix this regression for 4.14, either via Florian's patch or by augmenting my patch to perform the matching calls on all of the xfrms. > > > > > diff --git a/net/xfrm/xfrm_policy.c b/net/xfrm/xfrm_policy.c > > > > index 2746b62..171818b 100644 > > > > --- a/net/xfrm/xfrm_policy.c > > > > +++ b/net/xfrm/xfrm_policy.c > > > > @@ -1820,6 +1820,11 @@ xfrm_resolve_and_create_bundle(struct > > > > xfrm_policy **pols, int num_pols, > > > > !xfrm_pol_dead(xdst) && > > > > memcmp(xdst->pols, pols, > > > > sizeof(struct xfrm_policy *) * num_pols) == > > > > 0 && > > > > + (!xdst->u.dst.xfrm->sel.family || > > > > +xfrm_selector_match(&xdst->u.dst.xfrm->sel, fl, > > > > +xdst->u.dst.xfrm->sel.family)) > > > > && > > > > + security_xfrm_state_pol_flow_match(xdst- > > > > >u.dst.xfrm, > > > > + xdst->pols[0], > > > > fl) && > > > > ... so this needs to walk the bundle and validate each selector. > > > > Alternatively we could always do template resolution and then check > > that all states found match those of the old pcpu xdst: > > > > diff --git a/net/xfrm/xfrm_policy.c b/net/xfrm/xfrm_policy.c > > --- a/net/xfrm/xfrm_policy.c > > +++ b/net/xfrm/xfrm_policy.c > > @@ -1786,19 +1786,23 @@ void xfrm_policy_cache_flush(void) > > put_online_cpus(); > > } > > > > -static bool xfrm
Re: [RFC PATCH] xfrm: fix regression introduced by xdst pcpu cache
On Wed, 2017-11-01 at 00:08 +0100, Florian Westphal wrote:
> Paul Moore wrote:
> > On Mon, Oct 30, 2017 at 10:58 AM, Stephen Smalley > v> wrote:
> > > matching before (as in this patch) or after calling
> > > xfrm_bundle_ok()?
> >
> > I would probably make the LSM call the last check, as you've done;
> > but
> > I have to say that is just so it is consistent with the "LSM last"
> > philosophy and not because of any performance related argument.
> >
> > > ... Also,
> > > do we need to test xfrm->sel.family before calling
> > > xfrm_selector_match
> > > (as in this patch) or not - xfrm_state_look_at() does so when the
> > > state is XFRM_STATE_VALID but not when it is _ERROR or _EXPIRED?
> >
> > Speaking purely from a SELinux perspective, I'm not sure it
> > matters:
> > as long as the labels match we are happy. However, from a general
> > IPsec perspective it does seem like a reasonable thing.
> >
> > Granted I'm probably missing something, but it seems a little odd
> > that
> > the code isn't already checking that the selectors match (... what
> > am
> > I missing?). It does check the policies, maybe that is enough in
> > the
> > normal IPsec case?
>
> The assumption was that identical policies would yield the same SAs,
> but thats not correct.
>
> > > diff --git a/net/xfrm/xfrm_policy.c b/net/xfrm/xfrm_policy.c
> > > index 2746b62..171818b 100644
> > > --- a/net/xfrm/xfrm_policy.c
> > > +++ b/net/xfrm/xfrm_policy.c
> > > @@ -1820,6 +1820,11 @@ xfrm_resolve_and_create_bundle(struct
> > > xfrm_policy **pols, int num_pols,
> > > !xfrm_pol_dead(xdst) &&
> > > memcmp(xdst->pols, pols,
> > > sizeof(struct xfrm_policy *) * num_pols) == 0
> > > &&
> > > + (!xdst->u.dst.xfrm->sel.family ||
> > > +xfrm_selector_match(&xdst->u.dst.xfrm->sel, fl,
> > > +xdst->u.dst.xfrm->sel.family))
> > > &&
> > > + security_xfrm_state_pol_flow_match(xdst->u.dst.xfrm,
> > > + xdst->pols[0], fl)
> > > &&
>
> ... so this needs to walk the bundle and validate each selector.
>
> Alternatively we could always do template resolution and then check
> that all states found match those of the old pcpu xdst:
With your patch below, the selinux-testsuite passes, and I couldn't
trigger any failures even running the inet_socket tests repeatedly.
Tested-by: Stephen Smalley
>
> diff --git a/net/xfrm/xfrm_policy.c b/net/xfrm/xfrm_policy.c
> --- a/net/xfrm/xfrm_policy.c
> +++ b/net/xfrm/xfrm_policy.c
> @@ -1786,19 +1786,23 @@ void xfrm_policy_cache_flush(void)
> put_online_cpus();
> }
>
> -static bool xfrm_pol_dead(struct xfrm_dst *xdst)
> +static bool xfrm_xdst_can_reuse(struct xfrm_dst *xdst,
> + struct xfrm_state * const xfrm[],
> + int num)
> {
> - unsigned int num_pols = xdst->num_pols;
> - unsigned int pol_dead = 0, i;
> + const struct dst_entry *dst = &xdst->u.dst;
> + int i;
>
> - for (i = 0; i < num_pols; i++)
> - pol_dead |= xdst->pols[i]->walk.dead;
> + if (xdst->num_xfrms != num)
> + return false;
>
> - /* Mark DST_OBSOLETE_DEAD to fail the next xfrm_dst_check()
> */
> - if (pol_dead)
> - xdst->u.dst.obsolete = DST_OBSOLETE_DEAD;
> + for (i = 0; i < num; i++) {
> + if (!dst || dst->xfrm != xfrm[i])
> + return false;
> + dst = dst->child;
> + }
>
> - return pol_dead;
> + return xfrm_bundle_ok(xdst);
> }
>
> static struct xfrm_dst *
> @@ -1812,26 +1816,28 @@ xfrm_resolve_and_create_bundle(struct
> xfrm_policy **pols, int num_pols,
> struct dst_entry *dst;
> int err;
>
> + /* Try to instantiate a bundle */
> + err = xfrm_tmpl_resolve(pols, num_pols, fl, xfrm, family);
> + if (err <= 0) {
> + if (err != 0 && err != -EAGAIN)
> + XFRM_INC_STATS(net,
> LINUX_MIB_XFRMOUTPOLERROR);
> + return ERR_PTR(err);
> + }
> +
> xdst = this_cpu_read(xfrm_last_dst);
> if (xdst &&
> xdst->u.dst.dev == dst_orig->dev &&
> xdst->num_pols == num_pols &&
> -
Re: [RFC PATCH] xfrm: fix regression introduced by xdst pcpu cache
On Tue, 2017-10-31 at 09:43 -0400, Stephen Smalley wrote:
> On Tue, 2017-10-31 at 12:11 +0100, Florian Westphal wrote:
> > Stephen Smalley wrote:
> > > Since 4.14-rc1, the selinux-testsuite has been encountering
> > > sporadic
> > > failures during testing of labeled IPSEC. git bisect pointed to
> > > commit ec30d78c14a813db39a647b6a348b4286 ("xfrm: add xdst pcpu
> > > cache").
> > > The xdst pcpu cache is only checking that the policies are the
> > > same,
> > > but does not validate that the policy, state, and flow match with
> > > respect
> > > to security context labeling. As a result, the wrong SA could be
> > > used
> > > and the receiver could end up performing permission checking and
> > > providing SO_PEERSEC or SCM_SECURITY values for the wrong
> > > security
> > > context.
> > > security_xfrm_state_pol_flow_match() exists for this purpose and
> > > is
> > > already called from xfrm_state_look_at() for matching purposes.
> > > Further, xfrm_state_look_at() also performs a
> > > xfrm_selector_match()
> > > test,
> > > which is also missing from the xdst pcpu cache logic. Add calls
> > > to
> > > both
> > > of these functions when validating the cache entry. With these
> > > changes,
> > > the selinux-testsuite passes all tests again.
> > >
> > > Fixes: ec30d78c14a813db39a647b6a348b4286ba4abf5 ("xfrm: add xdst
> > > pcpu cache")
> > > Signed-off-by: Stephen Smalley
> > > ---
> > > This is an RFC because I am not entirely confident in the fix,
> > > e.g.
> > > is it
> > > sufficient to perform this matching only on the first xfrm or do
> > > they all
> > > need to be walked as in xfrm_bundle_ok()? Also, should we
> > > perform
> > > this
> > > matching before (as in this patch) or after calling
> > > xfrm_bundle_ok()? Also,
> > > do we need to test xfrm->sel.family before calling
> > > xfrm_selector_match
> > > (as in this patch) or not - xfrm_state_look_at() does so when the
> > > state is XFRM_STATE_VALID but not when it is _ERROR or _EXPIRED?
> >
> > No idea.
> >
> > I looked at the old flow cache but i don't see any of these extra
> > checks there either.
> >
> > However, old flow cache stored flowi struct as key, and that
> > contains
> > a
> > flowi_secid, populated by the decode_session hooks.
> >
> > Was it enough to check for identical flowi_secid in the flowi
> > structs
> > to
> > avoid this problem or am i missing something?
>
> I'm not sure, but security_xfrm_state_pol_flow_match() ->
> selinux_xfrm_state_pol_flow_match() does more than just compare flow
> secids.
>
> Also, there is the separate issue of the missing
> xfrm_selector_match()
> call, which can also cause the wrong SA to be used independent of
> anything LSM/SELinux-related.
>
> It is a regression; the correct SA was being used prior to the xdst
> pcpu cache commit. Reproducible using the selinux-testsuite, most
> easily run on a Fedora VM,
> git clone https://github.com/SELinuxProject/selinux-testsuite/
> sudo dnf install perl-Test perl-Test-Harness perl-Test-Simple
> selinux-policy-devel gcc libselinux-devel net-tools netlabel_tools
> iptables
Actually, you should just run 'sudo make test' instead of the
individual commands below. I was breaking out the individual commands
to avoid running the rest of the testsuite unrelated to networking, but
that won't pick up all of the dependencies the first time. Sorry.
> sudo make -C policy load
> cd tests/inet_socket
> while sudo ./test; do : ; done
Re: [RFC PATCH] xfrm: fix regression introduced by xdst pcpu cache
On Tue, 2017-10-31 at 12:11 +0100, Florian Westphal wrote:
> Stephen Smalley wrote:
> > Since 4.14-rc1, the selinux-testsuite has been encountering
> > sporadic
> > failures during testing of labeled IPSEC. git bisect pointed to
> > commit ec30d78c14a813db39a647b6a348b4286 ("xfrm: add xdst pcpu
> > cache").
> > The xdst pcpu cache is only checking that the policies are the
> > same,
> > but does not validate that the policy, state, and flow match with
> > respect
> > to security context labeling. As a result, the wrong SA could be
> > used
> > and the receiver could end up performing permission checking and
> > providing SO_PEERSEC or SCM_SECURITY values for the wrong security
> > context.
> > security_xfrm_state_pol_flow_match() exists for this purpose and is
> > already called from xfrm_state_look_at() for matching purposes.
> > Further, xfrm_state_look_at() also performs a xfrm_selector_match()
> > test,
> > which is also missing from the xdst pcpu cache logic. Add calls to
> > both
> > of these functions when validating the cache entry. With these
> > changes,
> > the selinux-testsuite passes all tests again.
> >
> > Fixes: ec30d78c14a813db39a647b6a348b4286ba4abf5 ("xfrm: add xdst
> > pcpu cache")
> > Signed-off-by: Stephen Smalley
> > ---
> > This is an RFC because I am not entirely confident in the fix, e.g.
> > is it
> > sufficient to perform this matching only on the first xfrm or do
> > they all
> > need to be walked as in xfrm_bundle_ok()? Also, should we perform
> > this
> > matching before (as in this patch) or after calling
> > xfrm_bundle_ok()? Also,
> > do we need to test xfrm->sel.family before calling
> > xfrm_selector_match
> > (as in this patch) or not - xfrm_state_look_at() does so when the
> > state is XFRM_STATE_VALID but not when it is _ERROR or _EXPIRED?
>
> No idea.
>
> I looked at the old flow cache but i don't see any of these extra
> checks there either.
>
> However, old flow cache stored flowi struct as key, and that contains
> a
> flowi_secid, populated by the decode_session hooks.
>
> Was it enough to check for identical flowi_secid in the flowi structs
> to
> avoid this problem or am i missing something?
I'm not sure, but security_xfrm_state_pol_flow_match() ->
selinux_xfrm_state_pol_flow_match() does more than just compare flow
secids.
Also, there is the separate issue of the missing xfrm_selector_match()
call, which can also cause the wrong SA to be used independent of
anything LSM/SELinux-related.
It is a regression; the correct SA was being used prior to the xdst
pcpu cache commit. Reproducible using the selinux-testsuite, most
easily run on a Fedora VM,
git clone https://github.com/SELinuxProject/selinux-testsuite/
sudo dnf install perl-Test perl-Test-Harness perl-Test-Simple
selinux-policy-devel gcc libselinux-devel net-tools netlabel_tools iptables
sudo make -C policy load
cd tests/inet_socket
while sudo ./test; do : ; done
[RFC PATCH] xfrm: fix regression introduced by xdst pcpu cache
Since 4.14-rc1, the selinux-testsuite has been encountering sporadic
failures during testing of labeled IPSEC. git bisect pointed to
commit ec30d78c14a813db39a647b6a348b4286 ("xfrm: add xdst pcpu cache").
The xdst pcpu cache is only checking that the policies are the same,
but does not validate that the policy, state, and flow match with respect
to security context labeling. As a result, the wrong SA could be used
and the receiver could end up performing permission checking and
providing SO_PEERSEC or SCM_SECURITY values for the wrong security context.
security_xfrm_state_pol_flow_match() exists for this purpose and is
already called from xfrm_state_look_at() for matching purposes.
Further, xfrm_state_look_at() also performs a xfrm_selector_match() test,
which is also missing from the xdst pcpu cache logic. Add calls to both
of these functions when validating the cache entry. With these changes,
the selinux-testsuite passes all tests again.
Fixes: ec30d78c14a813db39a647b6a348b4286ba4abf5 ("xfrm: add xdst pcpu cache")
Signed-off-by: Stephen Smalley
---
This is an RFC because I am not entirely confident in the fix, e.g. is it
sufficient to perform this matching only on the first xfrm or do they all
need to be walked as in xfrm_bundle_ok()? Also, should we perform this
matching before (as in this patch) or after calling xfrm_bundle_ok()? Also,
do we need to test xfrm->sel.family before calling xfrm_selector_match
(as in this patch) or not - xfrm_state_look_at() does so when the
state is XFRM_STATE_VALID but not when it is _ERROR or _EXPIRED?
net/xfrm/xfrm_policy.c | 5 +
1 file changed, 5 insertions(+)
diff --git a/net/xfrm/xfrm_policy.c b/net/xfrm/xfrm_policy.c
index 2746b62..171818b 100644
--- a/net/xfrm/xfrm_policy.c
+++ b/net/xfrm/xfrm_policy.c
@@ -1820,6 +1820,11 @@ xfrm_resolve_and_create_bundle(struct xfrm_policy
**pols, int num_pols,
!xfrm_pol_dead(xdst) &&
memcmp(xdst->pols, pols,
sizeof(struct xfrm_policy *) * num_pols) == 0 &&
+ (!xdst->u.dst.xfrm->sel.family ||
+xfrm_selector_match(&xdst->u.dst.xfrm->sel, fl,
+xdst->u.dst.xfrm->sel.family)) &&
+ security_xfrm_state_pol_flow_match(xdst->u.dst.xfrm,
+ xdst->pols[0], fl) &&
xfrm_bundle_ok(xdst)) {
dst_hold(&xdst->u.dst);
return xdst;
--
2.9.5
Re: [RFC PATCH 5/5] selinux: Add SCTP support
On Tue, 2017-10-17 at 14:59 +0100, Richard Haines wrote:
> The SELinux SCTP implementation is explained in:
> Documentation/security/SELinux-sctp.txt
>
> Signed-off-by: Richard Haines
> ---
> Documentation/security/SELinux-sctp.txt | 108 +
> security/selinux/hooks.c| 268
> ++--
> security/selinux/include/classmap.h | 3 +-
> security/selinux/include/netlabel.h | 9 +-
> security/selinux/include/objsec.h | 5 +
> security/selinux/netlabel.c | 52 ++-
> 6 files changed, 427 insertions(+), 18 deletions(-)
> create mode 100644 Documentation/security/SELinux-sctp.txt
>
> diff --git a/Documentation/security/SELinux-sctp.txt
> b/Documentation/security/SELinux-sctp.txt
> new file mode 100644
> index 000..32e0255
> --- /dev/null
> +++ b/Documentation/security/SELinux-sctp.txt
> @@ -0,0 +1,108 @@
> + SCTP SELinux Support
> + ==
> +
> +Security Hooks
> +===
> +
> +The Documentation/security/LSM-sctp.txt document describes how the
> following
> +sctp security hooks are utilised:
> +security_sctp_assoc_request()
> +security_sctp_bind_connect()
> +security_sctp_sk_clone()
> +
> +security_inet_conn_established()
> +
> +
> +Policy Statements
> +==
> +The following class and permissions to support SCTP are available
> within the
> +kernel:
> +class sctp_socket inherits socket { node_bind }
> +
> +whenever the following policy capability is enabled:
> +policycap extended_socket_class;
> +
> +The SELinux SCTP support adds the additional permissions that are
> explained
> +in the sections below:
> +association bindx connectx
> +
> +If userspace tools have been updated, SCTP will support the portcon
> +statement as shown in the following example:
> +portcon sctp 1024-1036 system_u:object_r:sctp_ports_t:s0
> +
> +
> +SCTP Bind, Connect and ASCONF Chunk Parameter Permission Checks
> +
> +The hook security_sctp_bind_connect() is called by SCTP to check
> permissions
> +required for ipv4/ipv6 addresses based on the @optname as follows:
> +
> + --
> + | BINDX Permission Check|
> + | @optname | @address contains |
> + ||---|
> + | SCTP_SOCKOPT_BINDX_ADD | One or more ipv4 / ipv6 addresses |
> + --
> +
> + --
> + | BIND Permission Checks|
> + | @optname | @address contains |
> + ||---|
> + | SCTP_PRIMARY_ADDR | Single ipv4 or ipv6 address |
> + | SCTP_SET_PEER_PRIMARY_ADDR | Single ipv4 or ipv6 address |
> + --
> +
> + --
> + | CONNECTX Permission Check |
> + | @optname | @address contains |
> + ||---|
> + | SCTP_SOCKOPT_CONNECTX | One or more ipv4 / ipv6 addresses |
> + --
> +
> + --
> + | CONNECT Permission Checks |
> + | @optname | @address contains |
> + ||---|
> + | SCTP_SENDMSG_CONNECT | Single ipv4 or ipv6 address |
> + | SCTP_PARAM_ADD_IP | One or more ipv4 / ipv6 addresses |
> + | SCTP_PARAM_SET_PRIMARY | Single ipv4 or ipv6 address |
> + --
> +
> +SCTP Peer Labeling
> +===
> +An SCTP socket will only have one peer label assigned to it. This
> will be
> +assigned during the establishment of the first association. Once the
> peer
> +label has been assigned, any new associations will have the
> "association"
> +permission validated by checking the socket peer sid against the
> received
> +packets peer sid to determine whether the association should be
> allowed or
> +denied.
> +
> +NOTES:
> + 1) If peer labeling is not enabled, then the peer context will
> always be
> + SECINITSID_UNLABELED (unlabeled_t in Reference Policy).
> +
> + 2) As SCTP supports multiple endpoints with multi-homing on a
> single socket
> + it is recommended that peer labels are consistent.
> +
> + 3) getpeercon
Re: [PATCH net-next v5 5/5] selinux: bpf: Add addtional check for bpf object file receive
On Thu, 2017-10-12 at 13:55 -0700, Chenbo Feng wrote:
> From: Chenbo Feng
>
> Introduce a bpf object related check when sending and receiving files
> through unix domain socket as well as binder. It checks if the
> receiving
> process have privilege to read/write the bpf map or use the bpf
> program.
> This check is necessary because the bpf maps and programs are using a
> anonymous inode as their shared inode so the normal way of checking
> the
> files and sockets when passing between processes cannot work properly
> on
> eBPF object. This check only works when the BPF_SYSCALL is
> configured.
>
> Signed-off-by: Chenbo Feng
> ---
> include/linux/bpf.h | 3 +++
> kernel/bpf/syscall.c | 4 ++--
> security/selinux/hooks.c | 49
>
> 3 files changed, 54 insertions(+), 2 deletions(-)
>
> diff --git a/include/linux/bpf.h b/include/linux/bpf.h
> index 225740688ab7..81d6c01b8825 100644
> --- a/include/linux/bpf.h
> +++ b/include/linux/bpf.h
> @@ -285,6 +285,9 @@ int bpf_prog_array_copy_to_user(struct
> bpf_prog_array __rcu *progs,
> #ifdef CONFIG_BPF_SYSCALL
> DECLARE_PER_CPU(int, bpf_prog_active);
>
> +extern const struct file_operations bpf_map_fops;
> +extern const struct file_operations bpf_prog_fops;
> +
> #define BPF_PROG_TYPE(_id, _ops) \
> extern const struct bpf_verifier_ops _ops;
> #define BPF_MAP_TYPE(_id, _ops) \
> diff --git a/kernel/bpf/syscall.c b/kernel/bpf/syscall.c
> index d3e152e282d8..8bdb98aa7f34 100644
> --- a/kernel/bpf/syscall.c
> +++ b/kernel/bpf/syscall.c
> @@ -313,7 +313,7 @@ static ssize_t bpf_dummy_write(struct file *filp,
> const char __user *buf,
> return -EINVAL;
> }
>
> -static const struct file_operations bpf_map_fops = {
> +const struct file_operations bpf_map_fops = {
> #ifdef CONFIG_PROC_FS
> .show_fdinfo= bpf_map_show_fdinfo,
> #endif
> @@ -967,7 +967,7 @@ static void bpf_prog_show_fdinfo(struct seq_file
> *m, struct file *filp)
> }
> #endif
>
> -static const struct file_operations bpf_prog_fops = {
> +const struct file_operations bpf_prog_fops = {
> #ifdef CONFIG_PROC_FS
> .show_fdinfo= bpf_prog_show_fdinfo,
> #endif
> diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
> index 12cf7de8cbed..ef7e5c1de640 100644
> --- a/security/selinux/hooks.c
> +++ b/security/selinux/hooks.c
> @@ -1815,6 +1815,10 @@ static inline int file_path_has_perm(const
> struct cred *cred,
> return inode_has_perm(cred, file_inode(file), av, &ad);
> }
>
> +#ifdef CONFIG_BPF_SYSCALL
> +static int bpf_fd_pass(struct file *file, u32 sid);
> +#endif
> +
> /* Check whether a task can use an open file descriptor to
> access an inode in a given way. Check access to the
> descriptor itself, and then use dentry_has_perm to
> @@ -1845,6 +1849,12 @@ static int file_has_perm(const struct cred
> *cred,
> goto out;
> }
>
> +#ifdef CONFIG_BPF_SYSCALL
> + rc = bpf_fd_pass(file, cred_sid(cred));
> + if (rc)
> + return rc;
> +#endif
> +
> /* av is zero if only checking access to the descriptor. */
> rc = 0;
> if (av)
> @@ -2165,6 +2175,12 @@ static int selinux_binder_transfer_file(struct
> task_struct *from,
> return rc;
> }
>
> +#ifdef CONFIG_BPF_SYSCALL
> + rc = bpf_fd_pass(file, sid);
> + if (rc)
> + return rc;
> +#endif
> +
> if (unlikely(IS_PRIVATE(d_backing_inode(dentry
> return 0;
>
> @@ -6288,6 +6304,39 @@ static u32 bpf_map_fmode_to_av(fmode_t fmode)
> return av;
> }
>
> +/* This function will check the file pass through unix socket or
> binder to see
> + * if it is a bpf related object. And apply correspinding checks on
> the bpf
> + * object based on the type. The bpf maps and programs, not like
> other files and
> + * socket, are using a shared anonymous inode inside the kernel as
> their inode.
> + * So checking that inode cannot identify if the process have
> privilege to
> + * access the bpf object and that's why we have to add this
> additional check in
> + * selinux_file_receive and selinux_binder_transfer_files.
> + */
> +static int bpf_fd_pass(struct file *file, u32 sid)
> +{
> + struct bpf_security_struct *bpfsec;
> + struct bpf_prog *prog;
> + struct bpf_map *map;
> + int ret;
> +
> + if (file->f_op == &bpf_map_fops) {
> + map = file->private_data;
> + bpfsec = map->security;
> + ret = avc_has_perm(sid, bpfsec->sid,
> SECCLASS_BPF_MAP,
> + bpf_map_fmode_to_av(file-
> >f_mode), NULL);
> + if (ret)
> + return ret;
> + } else if (file->f_op == &bpf_prog_fops) {
> + prog = file->private_data;
> + bpfsec = prog->aux->security;
> + ret = avc_has_perm(sid, bpfsec->sid,
> SECCLASS_BPF_PROG,
> + BPF__PROG_RUN, NULL);
> +
Re: [PATCH net-next v5 5/5] selinux: bpf: Add addtional check for bpf object file receive
On Thu, 2017-10-12 at 13:55 -0700, Chenbo Feng wrote:
> From: Chenbo Feng
>
> Introduce a bpf object related check when sending and receiving files
> through unix domain socket as well as binder. It checks if the
> receiving
> process have privilege to read/write the bpf map or use the bpf
> program.
> This check is necessary because the bpf maps and programs are using a
> anonymous inode as their shared inode so the normal way of checking
> the
> files and sockets when passing between processes cannot work properly
> on
> eBPF object. This check only works when the BPF_SYSCALL is
> configured.
>
> Signed-off-by: Chenbo Feng
Acked-by: Stephen Smalley
> ---
> include/linux/bpf.h | 3 +++
> kernel/bpf/syscall.c | 4 ++--
> security/selinux/hooks.c | 49
>
> 3 files changed, 54 insertions(+), 2 deletions(-)
>
> diff --git a/include/linux/bpf.h b/include/linux/bpf.h
> index 225740688ab7..81d6c01b8825 100644
> --- a/include/linux/bpf.h
> +++ b/include/linux/bpf.h
> @@ -285,6 +285,9 @@ int bpf_prog_array_copy_to_user(struct
> bpf_prog_array __rcu *progs,
> #ifdef CONFIG_BPF_SYSCALL
> DECLARE_PER_CPU(int, bpf_prog_active);
>
> +extern const struct file_operations bpf_map_fops;
> +extern const struct file_operations bpf_prog_fops;
> +
> #define BPF_PROG_TYPE(_id, _ops) \
> extern const struct bpf_verifier_ops _ops;
> #define BPF_MAP_TYPE(_id, _ops) \
> diff --git a/kernel/bpf/syscall.c b/kernel/bpf/syscall.c
> index d3e152e282d8..8bdb98aa7f34 100644
> --- a/kernel/bpf/syscall.c
> +++ b/kernel/bpf/syscall.c
> @@ -313,7 +313,7 @@ static ssize_t bpf_dummy_write(struct file *filp,
> const char __user *buf,
> return -EINVAL;
> }
>
> -static const struct file_operations bpf_map_fops = {
> +const struct file_operations bpf_map_fops = {
> #ifdef CONFIG_PROC_FS
> .show_fdinfo= bpf_map_show_fdinfo,
> #endif
> @@ -967,7 +967,7 @@ static void bpf_prog_show_fdinfo(struct seq_file
> *m, struct file *filp)
> }
> #endif
>
> -static const struct file_operations bpf_prog_fops = {
> +const struct file_operations bpf_prog_fops = {
> #ifdef CONFIG_PROC_FS
> .show_fdinfo= bpf_prog_show_fdinfo,
> #endif
> diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
> index 12cf7de8cbed..ef7e5c1de640 100644
> --- a/security/selinux/hooks.c
> +++ b/security/selinux/hooks.c
> @@ -1815,6 +1815,10 @@ static inline int file_path_has_perm(const
> struct cred *cred,
> return inode_has_perm(cred, file_inode(file), av, &ad);
> }
>
> +#ifdef CONFIG_BPF_SYSCALL
> +static int bpf_fd_pass(struct file *file, u32 sid);
> +#endif
> +
> /* Check whether a task can use an open file descriptor to
> access an inode in a given way. Check access to the
> descriptor itself, and then use dentry_has_perm to
> @@ -1845,6 +1849,12 @@ static int file_has_perm(const struct cred
> *cred,
> goto out;
> }
>
> +#ifdef CONFIG_BPF_SYSCALL
> + rc = bpf_fd_pass(file, cred_sid(cred));
> + if (rc)
> + return rc;
> +#endif
> +
> /* av is zero if only checking access to the descriptor. */
> rc = 0;
> if (av)
> @@ -2165,6 +2175,12 @@ static int selinux_binder_transfer_file(struct
> task_struct *from,
> return rc;
> }
>
> +#ifdef CONFIG_BPF_SYSCALL
> + rc = bpf_fd_pass(file, sid);
> + if (rc)
> + return rc;
> +#endif
> +
> if (unlikely(IS_PRIVATE(d_backing_inode(dentry
> return 0;
>
> @@ -6288,6 +6304,39 @@ static u32 bpf_map_fmode_to_av(fmode_t fmode)
> return av;
> }
>
> +/* This function will check the file pass through unix socket or
> binder to see
> + * if it is a bpf related object. And apply correspinding checks on
> the bpf
> + * object based on the type. The bpf maps and programs, not like
> other files and
> + * socket, are using a shared anonymous inode inside the kernel as
> their inode.
> + * So checking that inode cannot identify if the process have
> privilege to
> + * access the bpf object and that's why we have to add this
> additional check in
> + * selinux_file_receive and selinux_binder_transfer_files.
> + */
> +static int bpf_fd_pass(struct file *file, u32 sid)
> +{
> + struct bpf_security_struct *bpfsec;
> + struct bpf_prog *prog;
> + struct bpf_map *map;
> + int ret;
> +
> + if (file->f_op == &bpf_map_fops) {
> + map = file->private_data;
> + bpfsec = map->security;
&g
Re: [PATCH net-next v5 4/5] selinux: bpf: Add selinux check for eBPF syscall operations
On Thu, 2017-10-12 at 13:55 -0700, Chenbo Feng wrote:
> From: Chenbo Feng
>
> Implement the actual checks introduced to eBPF related syscalls. This
> implementation use the security field inside bpf object to store a
> sid that
> identify the bpf object. And when processes try to access the object,
> selinux will check if processes have the right privileges. The
> creation
> of eBPF object are also checked at the general bpf check hook and new
> cmd introduced to eBPF domain can also be checked there.
>
> Signed-off-by: Chenbo Feng
> Acked-by: Alexei Starovoitov
Acked-by: Stephen Smalley
> ---
> security/selinux/hooks.c| 111
>
> security/selinux/include/classmap.h | 2 +
> security/selinux/include/objsec.h | 4 ++
> 3 files changed, 117 insertions(+)
>
> diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
> index f5d304736852..12cf7de8cbed 100644
> --- a/security/selinux/hooks.c
> +++ b/security/selinux/hooks.c
> @@ -85,6 +85,7 @@
> #include
> #include
> #include
> +#include
>
> #include "avc.h"
> #include "objsec.h"
> @@ -6252,6 +6253,106 @@ static void selinux_ib_free_security(void
> *ib_sec)
> }
> #endif
>
> +#ifdef CONFIG_BPF_SYSCALL
> +static int selinux_bpf(int cmd, union bpf_attr *attr,
> + unsigned int size)
> +{
> + u32 sid = current_sid();
> + int ret;
> +
> + switch (cmd) {
> + case BPF_MAP_CREATE:
> + ret = avc_has_perm(sid, sid, SECCLASS_BPF,
> BPF__MAP_CREATE,
> + NULL);
> + break;
> + case BPF_PROG_LOAD:
> + ret = avc_has_perm(sid, sid, SECCLASS_BPF,
> BPF__PROG_LOAD,
> + NULL);
> + break;
> + default:
> + ret = 0;
> + break;
> + }
> +
> + return ret;
> +}
> +
> +static u32 bpf_map_fmode_to_av(fmode_t fmode)
> +{
> + u32 av = 0;
> +
> + if (fmode & FMODE_READ)
> + av |= BPF__MAP_READ;
> + if (fmode & FMODE_WRITE)
> + av |= BPF__MAP_WRITE;
> + return av;
> +}
> +
> +static int selinux_bpf_map(struct bpf_map *map, fmode_t fmode)
> +{
> + u32 sid = current_sid();
> + struct bpf_security_struct *bpfsec;
> +
> + bpfsec = map->security;
> + return avc_has_perm(sid, bpfsec->sid, SECCLASS_BPF,
> + bpf_map_fmode_to_av(fmode), NULL);
> +}
> +
> +static int selinux_bpf_prog(struct bpf_prog *prog)
> +{
> + u32 sid = current_sid();
> + struct bpf_security_struct *bpfsec;
> +
> + bpfsec = prog->aux->security;
> + return avc_has_perm(sid, bpfsec->sid, SECCLASS_BPF,
> + BPF__PROG_RUN, NULL);
> +}
> +
> +static int selinux_bpf_map_alloc(struct bpf_map *map)
> +{
> + struct bpf_security_struct *bpfsec;
> +
> + bpfsec = kzalloc(sizeof(*bpfsec), GFP_KERNEL);
> + if (!bpfsec)
> + return -ENOMEM;
> +
> + bpfsec->sid = current_sid();
> + map->security = bpfsec;
> +
> + return 0;
> +}
> +
> +static void selinux_bpf_map_free(struct bpf_map *map)
> +{
> + struct bpf_security_struct *bpfsec = map->security;
> +
> + map->security = NULL;
> + kfree(bpfsec);
> +}
> +
> +static int selinux_bpf_prog_alloc(struct bpf_prog_aux *aux)
> +{
> + struct bpf_security_struct *bpfsec;
> +
> + bpfsec = kzalloc(sizeof(*bpfsec), GFP_KERNEL);
> + if (!bpfsec)
> + return -ENOMEM;
> +
> + bpfsec->sid = current_sid();
> + aux->security = bpfsec;
> +
> + return 0;
> +}
> +
> +static void selinux_bpf_prog_free(struct bpf_prog_aux *aux)
> +{
> + struct bpf_security_struct *bpfsec = aux->security;
> +
> + aux->security = NULL;
> + kfree(bpfsec);
> +}
> +#endif
> +
> static struct security_hook_list selinux_hooks[] __lsm_ro_after_init
> = {
> LSM_HOOK_INIT(binder_set_context_mgr,
> selinux_binder_set_context_mgr),
> LSM_HOOK_INIT(binder_transaction,
> selinux_binder_transaction),
> @@ -6471,6 +6572,16 @@ static struct security_hook_list
> selinux_hooks[] __lsm_ro_after_init = {
> LSM_HOOK_INIT(audit_rule_match, selinux_audit_rule_match),
> LSM_HOOK_INIT(audit_rule_free, selinux_audit_rule_free),
> #endif
> +
> +#ifdef CONFIG_BPF_SYSCALL
> + LSM_HOOK_INIT(bpf, selinux_bpf),
> + LSM_HOOK_INIT(bpf_map, selinux_bpf_map),
> + LSM_HOOK_INIT(bpf_prog, selinux_bpf_prog),
> + LSM_
Re: [PATCH net-next v3 5/5] selinux: bpf: Add addtional check for bpf object file receive
On Wed, 2017-10-11 at 13:43 -0700, Chenbo Feng via Selinux wrote:
> On Wed, Oct 11, 2017 at 5:54 AM, Stephen Smalley
> wrote:
> > On Tue, 2017-10-10 at 17:09 -0700, Chenbo Feng wrote:
> > > From: Chenbo Feng
> > >
> > > Introduce a bpf object related check when sending and receiving
> > > files
> > > through unix domain socket as well as binder. It checks if the
> > > receiving
> > > process have privilege to read/write the bpf map or use the bpf
> > > program.
> > > This check is necessary because the bpf maps and programs are
> > > using a
> > > anonymous inode as their shared inode so the normal way of
> > > checking
> > > the
> > > files and sockets when passing between processes cannot work
> > > properly
> > > on
> > > eBPF object. This check only works when the BPF_SYSCALL is
> > > configured.
> > > The information stored inside the file security struct is the
> > > same as
> > > the information in bpf object security struct.
> > >
> > > Signed-off-by: Chenbo Feng
> > > ---
> > > include/linux/lsm_hooks.h | 17 ++
> > > include/linux/security.h | 9 ++
> > > kernel/bpf/syscall.c | 27 ++--
> > > security/security.c | 8 +
> > > security/selinux/hooks.c | 67
> > > +++
> > > security/selinux/include/objsec.h | 9 ++
> > > 6 files changed, 135 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/include/linux/lsm_hooks.h
> > > b/include/linux/lsm_hooks.h
> > > index 7161d8e7ee79..517dea60b87b 100644
> > > --- a/include/linux/lsm_hooks.h
> > > +++ b/include/linux/lsm_hooks.h
> > > @@ -1385,6 +1385,19 @@
> > > * @bpf_prog_free_security:
> > > * Clean up the security information stored inside bpf prog.
> > > *
> > > + * @bpf_map_file:
> > > + * When creating a bpf map fd, set up the file security
> > > information with
> > > + * the bpf security information stored in the map struct. So
> > > when the map
> > > + * fd is passed between processes, the security module can
> > > directly read
> > > + * the security information from file security struct rather
> > > than the bpf
> > > + * security struct.
> > > + *
> > > + * @bpf_prog_file:
> > > + * When creating a bpf prog fd, set up the file security
> > > information with
> > > + * the bpf security information stored in the prog struct. So
> > > when the prog
> > > + * fd is passed between processes, the security module can
> > > directly read
> > > + * the security information from file security struct rather
> > > than the bpf
> > > + * security struct.
> > > */
> > > union security_list_options {
> > > int (*binder_set_context_mgr)(struct task_struct *mgr);
> > > @@ -1726,6 +1739,8 @@ union security_list_options {
> > > void (*bpf_map_free_security)(struct bpf_map *map);
> > > int (*bpf_prog_alloc_security)(struct bpf_prog_aux *aux);
> > > void (*bpf_prog_free_security)(struct bpf_prog_aux *aux);
> > > + void (*bpf_map_file)(struct bpf_map *map, struct file
> > > *file);
> > > + void (*bpf_prog_file)(struct bpf_prog_aux *aux, struct file
> > > *file);
> > > #endif /* CONFIG_BPF_SYSCALL */
> > > };
> > >
> > > @@ -1954,6 +1969,8 @@ struct security_hook_heads {
> > > struct list_head bpf_map_free_security;
> > > struct list_head bpf_prog_alloc_security;
> > > struct list_head bpf_prog_free_security;
> > > + struct list_head bpf_map_file;
> > > + struct list_head bpf_prog_file;
> > > #endif /* CONFIG_BPF_SYSCALL */
> > > } __randomize_layout;
> > >
> > > diff --git a/include/linux/security.h b/include/linux/security.h
> > > index 18800b0911e5..57573b794e2d 100644
> > > --- a/include/linux/security.h
> > > +++ b/include/linux/security.h
> > > @@ -1740,6 +1740,8 @@ extern int security_bpf_map_alloc(struct
> > > bpf_map *map);
> > > extern void security_bpf_map_free(struct bpf_map *map);
> > > extern int security_bpf_prog_alloc(struct bpf_prog_aux *aux);
> > > extern void security_bpf_prog_free(struct bpf_prog_aux *aux);
> > > +extern void security_bpf_map_file(struc
Re: [PATCH net-next v2 4/5] selinux: bpf: Add selinux check for eBPF syscall operations
On Tue, 2017-10-10 at 10:54 -0700, Chenbo Feng via Selinux wrote:
> On Tue, Oct 10, 2017 at 7:52 AM, Stephen Smalley
> wrote:
> > On Tue, 2017-10-10 at 10:18 -0400, Stephen Smalley wrote:
> > > On Mon, 2017-10-09 at 15:20 -0700, Chenbo Feng wrote:
> > > > From: Chenbo Feng
> > > >
> > > > Implement the actual checks introduced to eBPF related
> > > > syscalls.
> > > > This
> > > > implementation use the security field inside bpf object to
> > > > store a
> > > > sid that
> > > > identify the bpf object. And when processes try to access the
> > > > object,
> > > > selinux will check if processes have the right privileges. The
> > > > creation
> > > > of eBPF object are also checked at the general bpf check hook
> > > > and
> > > > new
> > > > cmd introduced to eBPF domain can also be checked there.
> > > >
> > > > Signed-off-by: Chenbo Feng
> > > > Acked-by: Alexei Starovoitov
> > > > ---
> > > > security/selinux/hooks.c| 111
> > > >
> > > > security/selinux/include/classmap.h | 2 +
> > > > security/selinux/include/objsec.h | 4 ++
> > > > 3 files changed, 117 insertions(+)
> > > >
> > > > diff --git a/security/selinux/hooks.c
> > > > b/security/selinux/hooks.c
> > > > index f5d304736852..41aba4e3d57c 100644
> > > > --- a/security/selinux/hooks.c
> > > > +++ b/security/selinux/hooks.c
> > > > @@ -85,6 +85,7 @@
> > > > #include
> > > > #include
> > > > #include
> > > > +#include
> > > >
> > > > #include "avc.h"
> > > > #include "objsec.h"
> > > > @@ -6252,6 +6253,106 @@ static void
> > > > selinux_ib_free_security(void
> > > > *ib_sec)
> > > > }
> > > > #endif
> > > >
> > > > +#ifdef CONFIG_BPF_SYSCALL
> > > > +static int selinux_bpf(int cmd, union bpf_attr *attr,
> > > > +unsigned int size)
> > > > +{
> > > > + u32 sid = current_sid();
> > > > + int ret;
> > > > +
> > > > + switch (cmd) {
> > > > + case BPF_MAP_CREATE:
> > > > + ret = avc_has_perm(sid, sid, SECCLASS_BPF_MAP,
> > > > BPF_MAP__CREATE,
> > > > + NULL);
> > > > + break;
> > > > + case BPF_PROG_LOAD:
> > > > + ret = avc_has_perm(sid, sid, SECCLASS_BPF_PROG,
> > > > BPF_PROG__LOAD,
> > > > + NULL);
> > > > + break;
> > > > + default:
> > > > + ret = 0;
> > > > + break;
> > > > + }
> > > > +
> > > > + return ret;
> > > > +}
> > > > +
> > > > +static u32 bpf_map_fmode_to_av(fmode_t fmode)
> > > > +{
> > > > + u32 av = 0;
> > > > +
> > > > + if (f_mode & FMODE_READ)
> > > > + av |= BPF_MAP__READ;
> > > > + if (f_mode & FMODE_WRITE)
> > > > + av |= BPF_MAP__WRITE;
> > > > + return av;
> > > > +}
> > > > +
> > > > +static int selinux_bpf_map(struct bpf_map *map, fmode_t fmode)
> > > > +{
> > > > + u32 sid = current_sid();
> > > > + struct bpf_security_struct *bpfsec;
> > > > +
> > > > + bpfsec = map->security;
> > > > + return avc_has_perm(sid, bpfsec->sid, SECCLASS_BPF_MAP,
> > > > + bpf_map_fmode_to_av(fmode), NULL);
> > > > +}
> > > > +
> > > > +static int selinux_bpf_prog(struct bpf_prog *prog)
> > > > +{
> > > > + u32 sid = current_sid();
> > > > + struct bpf_security_struct *bpfsec;
> > > > +
> > > > + bpfsec = prog->aux->security;
> > > > + return avc_has_perm(sid, bpfsec->sid, SECCLASS_BPF_PROG,
> > > > + BPF_PROG__USE, NULL);
> > > > +}
> > > > +
> > > > +static int selinux_bpf_map_alloc(struct bpf_map *map)
> > > > +{
> > > > + struct bpf_security_struct *bpfsec;
> > > > +
> > > >
Re: [PATCH net-next v3 5/5] selinux: bpf: Add addtional check for bpf object file receive
On Tue, 2017-10-10 at 17:09 -0700, Chenbo Feng wrote:
> From: Chenbo Feng
>
> Introduce a bpf object related check when sending and receiving files
> through unix domain socket as well as binder. It checks if the
> receiving
> process have privilege to read/write the bpf map or use the bpf
> program.
> This check is necessary because the bpf maps and programs are using a
> anonymous inode as their shared inode so the normal way of checking
> the
> files and sockets when passing between processes cannot work properly
> on
> eBPF object. This check only works when the BPF_SYSCALL is
> configured.
> The information stored inside the file security struct is the same as
> the information in bpf object security struct.
>
> Signed-off-by: Chenbo Feng
> ---
> include/linux/lsm_hooks.h | 17 ++
> include/linux/security.h | 9 ++
> kernel/bpf/syscall.c | 27 ++--
> security/security.c | 8 +
> security/selinux/hooks.c | 67
> +++
> security/selinux/include/objsec.h | 9 ++
> 6 files changed, 135 insertions(+), 2 deletions(-)
>
> diff --git a/include/linux/lsm_hooks.h b/include/linux/lsm_hooks.h
> index 7161d8e7ee79..517dea60b87b 100644
> --- a/include/linux/lsm_hooks.h
> +++ b/include/linux/lsm_hooks.h
> @@ -1385,6 +1385,19 @@
> * @bpf_prog_free_security:
> * Clean up the security information stored inside bpf prog.
> *
> + * @bpf_map_file:
> + * When creating a bpf map fd, set up the file security
> information with
> + * the bpf security information stored in the map struct. So
> when the map
> + * fd is passed between processes, the security module can
> directly read
> + * the security information from file security struct rather
> than the bpf
> + * security struct.
> + *
> + * @bpf_prog_file:
> + * When creating a bpf prog fd, set up the file security
> information with
> + * the bpf security information stored in the prog struct. So
> when the prog
> + * fd is passed between processes, the security module can
> directly read
> + * the security information from file security struct rather
> than the bpf
> + * security struct.
> */
> union security_list_options {
> int (*binder_set_context_mgr)(struct task_struct *mgr);
> @@ -1726,6 +1739,8 @@ union security_list_options {
> void (*bpf_map_free_security)(struct bpf_map *map);
> int (*bpf_prog_alloc_security)(struct bpf_prog_aux *aux);
> void (*bpf_prog_free_security)(struct bpf_prog_aux *aux);
> + void (*bpf_map_file)(struct bpf_map *map, struct file
> *file);
> + void (*bpf_prog_file)(struct bpf_prog_aux *aux, struct file
> *file);
> #endif /* CONFIG_BPF_SYSCALL */
> };
>
> @@ -1954,6 +1969,8 @@ struct security_hook_heads {
> struct list_head bpf_map_free_security;
> struct list_head bpf_prog_alloc_security;
> struct list_head bpf_prog_free_security;
> + struct list_head bpf_map_file;
> + struct list_head bpf_prog_file;
> #endif /* CONFIG_BPF_SYSCALL */
> } __randomize_layout;
>
> diff --git a/include/linux/security.h b/include/linux/security.h
> index 18800b0911e5..57573b794e2d 100644
> --- a/include/linux/security.h
> +++ b/include/linux/security.h
> @@ -1740,6 +1740,8 @@ extern int security_bpf_map_alloc(struct
> bpf_map *map);
> extern void security_bpf_map_free(struct bpf_map *map);
> extern int security_bpf_prog_alloc(struct bpf_prog_aux *aux);
> extern void security_bpf_prog_free(struct bpf_prog_aux *aux);
> +extern void security_bpf_map_file(struct bpf_map *map, struct file
> *file);
> +extern void security_bpf_prog_file(struct bpf_prog_aux *aux, struct
> file *file);
> #else
> static inline int security_bpf(int cmd, union bpf_attr *attr,
> unsigned int size)
> @@ -1772,6 +1774,13 @@ static inline int
> security_bpf_prog_alloc(struct bpf_prog_aux *aux)
>
> static inline void security_bpf_prog_free(struct bpf_prog_aux *aux)
> { }
> +
> +static inline void security_bpf_map_file(struct bpf_map *map, struct
> file *file)
> +{ }
> +
> +static inline void security_bpf_prog_file(struct bpf_prog_aux *aux,
> + struct file *file)
> +{ }
> #endif /* CONFIG_SECURITY */
> #endif /* CONFIG_BPF_SYSCALL */
>
> diff --git a/kernel/bpf/syscall.c b/kernel/bpf/syscall.c
> index 1cf31ddd7616..aee69e564c50 100644
> --- a/kernel/bpf/syscall.c
> +++ b/kernel/bpf/syscall.c
> @@ -324,11 +324,22 @@ static const struct file_operations
> bpf_map_fops = {
>
> int bpf_map_new_fd(struct bpf_map *map, int flags)
> {
> + int fd;
> + struct fd f;
> if (security_bpf_map(map, OPEN_FMODE(flags)))
> return -EPERM;
>
> - return anon_inode_getfd("bpf-map", &bpf_map_fops, map,
> + fd = anon_inode_getfd("bpf-map", &bpf_map_fops, map,
> flags | O_CLOEXEC);
> + if (fd < 0)
> + return fd;
> +
> +
Re: [Non-DoD Source] Re: [PATCH net-next v2 5/5] selinux: bpf: Add addtional check for bpf object file receive
On Tue, 2017-10-10 at 10:48 -0700, Chenbo Feng wrote:
> On Tue, Oct 10, 2017 at 7:24 AM, Stephen Smalley
> wrote:
> > On Mon, 2017-10-09 at 15:20 -0700, Chenbo Feng wrote:
> > > From: Chenbo Feng
> > >
> > > Introduce a bpf object related check when sending and receiving
> > > files
> > > through unix domain socket as well as binder. It checks if the
> > > receiving
> > > process have privilege to read/write the bpf map or use the bpf
> > > program.
> > > This check is necessary because the bpf maps and programs are
> > > using a
> > > anonymous inode as their shared inode so the normal way of
> > > checking
> > > the
> > > files and sockets when passing between processes cannot work
> > > properly
> > > on
> > > eBPF object. This check only works when the BPF_SYSCALL is
> > > configured.
> > > The information stored inside the file security struct is the
> > > same as
> > > the information in bpf object security struct.
> > >
> > > Signed-off-by: Chenbo Feng
> > > ---
> > > include/linux/bpf.h | 3 +++
> > > include/linux/lsm_hooks.h | 17 +
> > > include/linux/security.h | 9 +++
> > > kernel/bpf/syscall.c | 4 ++--
> > > security/security.c | 8 +++
> > > security/selinux/hooks.c | 61
> > > +++
> > > 6 files changed, 100 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/include/linux/bpf.h b/include/linux/bpf.h
> > > index 225740688ab7..81d6c01b8825 100644
> > > --- a/include/linux/bpf.h
> > > +++ b/include/linux/bpf.h
> > > @@ -285,6 +285,9 @@ int bpf_prog_array_copy_to_user(struct
> > > bpf_prog_array __rcu *progs,
> > > #ifdef CONFIG_BPF_SYSCALL
> > > DECLARE_PER_CPU(int, bpf_prog_active);
> > >
> > > +extern const struct file_operations bpf_map_fops;
> > > +extern const struct file_operations bpf_prog_fops;
> > > +
> > > #define BPF_PROG_TYPE(_id, _ops) \
> > > extern const struct bpf_verifier_ops _ops;
> > > #define BPF_MAP_TYPE(_id, _ops) \
> > > diff --git a/include/linux/lsm_hooks.h
> > > b/include/linux/lsm_hooks.h
> > > index 7161d8e7ee79..517dea60b87b 100644
> > > --- a/include/linux/lsm_hooks.h
> > > +++ b/include/linux/lsm_hooks.h
> > > @@ -1385,6 +1385,19 @@
> > > * @bpf_prog_free_security:
> > > * Clean up the security information stored inside bpf prog.
> > > *
> > > + * @bpf_map_file:
> > > + * When creating a bpf map fd, set up the file security
> > > information with
> > > + * the bpf security information stored in the map struct. So
> > > when the map
> > > + * fd is passed between processes, the security module can
> > > directly read
> > > + * the security information from file security struct rather
> > > than the bpf
> > > + * security struct.
> > > + *
> > > + * @bpf_prog_file:
> > > + * When creating a bpf prog fd, set up the file security
> > > information with
> > > + * the bpf security information stored in the prog struct. So
> > > when the prog
> > > + * fd is passed between processes, the security module can
> > > directly read
> > > + * the security information from file security struct rather
> > > than the bpf
> > > + * security struct.
> > > */
> > > union security_list_options {
> > > int (*binder_set_context_mgr)(struct task_struct *mgr);
> > > @@ -1726,6 +1739,8 @@ union security_list_options {
> > > void (*bpf_map_free_security)(struct bpf_map *map);
> > > int (*bpf_prog_alloc_security)(struct bpf_prog_aux *aux);
> > > void (*bpf_prog_free_security)(struct bpf_prog_aux *aux);
> > > + void (*bpf_map_file)(struct bpf_map *map, struct file
> > > *file);
> > > + void (*bpf_prog_file)(struct bpf_prog_aux *aux, struct file
> > > *file);
> > > #endif /* CONFIG_BPF_SYSCALL */
> > > };
> > >
> > > @@ -1954,6 +1969,8 @@ struct security_hook_heads {
> > > struct list_head bpf_map_free_security;
> > > struct list_head bpf_prog_alloc_security;
> > > struct list_head bpf_prog_free_security;
> > > + struct list_head bpf_map_file;
> > > + struct list_head bpf_prog_file;
> > > #endif /* CONFIG
Re: [PATCH net-next v2 4/5] selinux: bpf: Add selinux check for eBPF syscall operations
On Tue, 2017-10-10 at 10:18 -0400, Stephen Smalley wrote:
> On Mon, 2017-10-09 at 15:20 -0700, Chenbo Feng wrote:
> > From: Chenbo Feng
> >
> > Implement the actual checks introduced to eBPF related syscalls.
> > This
> > implementation use the security field inside bpf object to store a
> > sid that
> > identify the bpf object. And when processes try to access the
> > object,
> > selinux will check if processes have the right privileges. The
> > creation
> > of eBPF object are also checked at the general bpf check hook and
> > new
> > cmd introduced to eBPF domain can also be checked there.
> >
> > Signed-off-by: Chenbo Feng
> > Acked-by: Alexei Starovoitov
> > ---
> > security/selinux/hooks.c| 111
> >
> > security/selinux/include/classmap.h | 2 +
> > security/selinux/include/objsec.h | 4 ++
> > 3 files changed, 117 insertions(+)
> >
> > diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
> > index f5d304736852..41aba4e3d57c 100644
> > --- a/security/selinux/hooks.c
> > +++ b/security/selinux/hooks.c
> > @@ -85,6 +85,7 @@
> > #include
> > #include
> > #include
> > +#include
> >
> > #include "avc.h"
> > #include "objsec.h"
> > @@ -6252,6 +6253,106 @@ static void selinux_ib_free_security(void
> > *ib_sec)
> > }
> > #endif
> >
> > +#ifdef CONFIG_BPF_SYSCALL
> > +static int selinux_bpf(int cmd, union bpf_attr *attr,
> > + unsigned int size)
> > +{
> > + u32 sid = current_sid();
> > + int ret;
> > +
> > + switch (cmd) {
> > + case BPF_MAP_CREATE:
> > + ret = avc_has_perm(sid, sid, SECCLASS_BPF_MAP,
> > BPF_MAP__CREATE,
> > + NULL);
> > + break;
> > + case BPF_PROG_LOAD:
> > + ret = avc_has_perm(sid, sid, SECCLASS_BPF_PROG,
> > BPF_PROG__LOAD,
> > + NULL);
> > + break;
> > + default:
> > + ret = 0;
> > + break;
> > + }
> > +
> > + return ret;
> > +}
> > +
> > +static u32 bpf_map_fmode_to_av(fmode_t fmode)
> > +{
> > + u32 av = 0;
> > +
> > + if (f_mode & FMODE_READ)
> > + av |= BPF_MAP__READ;
> > + if (f_mode & FMODE_WRITE)
> > + av |= BPF_MAP__WRITE;
> > + return av;
> > +}
> > +
> > +static int selinux_bpf_map(struct bpf_map *map, fmode_t fmode)
> > +{
> > + u32 sid = current_sid();
> > + struct bpf_security_struct *bpfsec;
> > +
> > + bpfsec = map->security;
> > + return avc_has_perm(sid, bpfsec->sid, SECCLASS_BPF_MAP,
> > + bpf_map_fmode_to_av(fmode), NULL);
> > +}
> > +
> > +static int selinux_bpf_prog(struct bpf_prog *prog)
> > +{
> > + u32 sid = current_sid();
> > + struct bpf_security_struct *bpfsec;
> > +
> > + bpfsec = prog->aux->security;
> > + return avc_has_perm(sid, bpfsec->sid, SECCLASS_BPF_PROG,
> > + BPF_PROG__USE, NULL);
> > +}
> > +
> > +static int selinux_bpf_map_alloc(struct bpf_map *map)
> > +{
> > + struct bpf_security_struct *bpfsec;
> > +
> > + bpfsec = kzalloc(sizeof(*bpfsec), GFP_KERNEL);
> > + if (!bpfsec)
> > + return -ENOMEM;
> > +
> > + bpfsec->sid = current_sid();
> > + map->security = bpfsec;
> > +
> > + return 0;
> > +}
> > +
> > +static void selinux_bpf_map_free(struct bpf_map *map)
> > +{
> > + struct bpf_security_struct *bpfsec = map->security;
> > +
> > + map->security = NULL;
> > + kfree(bpfsec);
> > +}
> > +
> > +static int selinux_bpf_prog_alloc(struct bpf_prog_aux *aux)
> > +{
> > + struct bpf_security_struct *bpfsec;
> > +
> > + bpfsec = kzalloc(sizeof(*bpfsec), GFP_KERNEL);
> > + if (!bpfsec)
> > + return -ENOMEM;
> > +
> > + bpfsec->sid = current_sid();
> > + aux->security = bpfsec;
> > +
> > + return 0;
> > +}
> > +
> > +static void selinux_bpf_prog_free(struct bpf_prog_aux *aux)
> > +{
> > + struct bpf_security_struct *bpfsec = aux->security;
> > +
> > + aux->security = NULL;
> > + kfree(bpfsec);
> > +}
> > +#endif
> > +
> > static struct security_hook_li
Re: [PATCH net-next v2 5/5] selinux: bpf: Add addtional check for bpf object file receive
On Mon, 2017-10-09 at 15:20 -0700, Chenbo Feng wrote:
> From: Chenbo Feng
>
> Introduce a bpf object related check when sending and receiving files
> through unix domain socket as well as binder. It checks if the
> receiving
> process have privilege to read/write the bpf map or use the bpf
> program.
> This check is necessary because the bpf maps and programs are using a
> anonymous inode as their shared inode so the normal way of checking
> the
> files and sockets when passing between processes cannot work properly
> on
> eBPF object. This check only works when the BPF_SYSCALL is
> configured.
> The information stored inside the file security struct is the same as
> the information in bpf object security struct.
>
> Signed-off-by: Chenbo Feng
> ---
> include/linux/bpf.h | 3 +++
> include/linux/lsm_hooks.h | 17 +
> include/linux/security.h | 9 +++
> kernel/bpf/syscall.c | 4 ++--
> security/security.c | 8 +++
> security/selinux/hooks.c | 61
> +++
> 6 files changed, 100 insertions(+), 2 deletions(-)
>
> diff --git a/include/linux/bpf.h b/include/linux/bpf.h
> index 225740688ab7..81d6c01b8825 100644
> --- a/include/linux/bpf.h
> +++ b/include/linux/bpf.h
> @@ -285,6 +285,9 @@ int bpf_prog_array_copy_to_user(struct
> bpf_prog_array __rcu *progs,
> #ifdef CONFIG_BPF_SYSCALL
> DECLARE_PER_CPU(int, bpf_prog_active);
>
> +extern const struct file_operations bpf_map_fops;
> +extern const struct file_operations bpf_prog_fops;
> +
> #define BPF_PROG_TYPE(_id, _ops) \
> extern const struct bpf_verifier_ops _ops;
> #define BPF_MAP_TYPE(_id, _ops) \
> diff --git a/include/linux/lsm_hooks.h b/include/linux/lsm_hooks.h
> index 7161d8e7ee79..517dea60b87b 100644
> --- a/include/linux/lsm_hooks.h
> +++ b/include/linux/lsm_hooks.h
> @@ -1385,6 +1385,19 @@
> * @bpf_prog_free_security:
> * Clean up the security information stored inside bpf prog.
> *
> + * @bpf_map_file:
> + * When creating a bpf map fd, set up the file security
> information with
> + * the bpf security information stored in the map struct. So
> when the map
> + * fd is passed between processes, the security module can
> directly read
> + * the security information from file security struct rather
> than the bpf
> + * security struct.
> + *
> + * @bpf_prog_file:
> + * When creating a bpf prog fd, set up the file security
> information with
> + * the bpf security information stored in the prog struct. So
> when the prog
> + * fd is passed between processes, the security module can
> directly read
> + * the security information from file security struct rather
> than the bpf
> + * security struct.
> */
> union security_list_options {
> int (*binder_set_context_mgr)(struct task_struct *mgr);
> @@ -1726,6 +1739,8 @@ union security_list_options {
> void (*bpf_map_free_security)(struct bpf_map *map);
> int (*bpf_prog_alloc_security)(struct bpf_prog_aux *aux);
> void (*bpf_prog_free_security)(struct bpf_prog_aux *aux);
> + void (*bpf_map_file)(struct bpf_map *map, struct file
> *file);
> + void (*bpf_prog_file)(struct bpf_prog_aux *aux, struct file
> *file);
> #endif /* CONFIG_BPF_SYSCALL */
> };
>
> @@ -1954,6 +1969,8 @@ struct security_hook_heads {
> struct list_head bpf_map_free_security;
> struct list_head bpf_prog_alloc_security;
> struct list_head bpf_prog_free_security;
> + struct list_head bpf_map_file;
> + struct list_head bpf_prog_file;
> #endif /* CONFIG_BPF_SYSCALL */
> } __randomize_layout;
>
> diff --git a/include/linux/security.h b/include/linux/security.h
> index 18800b0911e5..57573b794e2d 100644
> --- a/include/linux/security.h
> +++ b/include/linux/security.h
> @@ -1740,6 +1740,8 @@ extern int security_bpf_map_alloc(struct
> bpf_map *map);
> extern void security_bpf_map_free(struct bpf_map *map);
> extern int security_bpf_prog_alloc(struct bpf_prog_aux *aux);
> extern void security_bpf_prog_free(struct bpf_prog_aux *aux);
> +extern void security_bpf_map_file(struct bpf_map *map, struct file
> *file);
> +extern void security_bpf_prog_file(struct bpf_prog_aux *aux, struct
> file *file);
> #else
> static inline int security_bpf(int cmd, union bpf_attr *attr,
> unsigned int size)
> @@ -1772,6 +1774,13 @@ static inline int
> security_bpf_prog_alloc(struct bpf_prog_aux *aux)
>
> static inline void security_bpf_prog_free(struct bpf_prog_aux *aux)
> { }
> +
> +static inline void security_bpf_map_file(struct bpf_map *map, struct
> file *file)
> +{ }
> +
> +static inline void security_bpf_prog_file(struct bpf_prog_aux *aux,
> + struct file *file)
> +{ }
> #endif /* CONFIG_SECURITY */
> #endif /* CONFIG_BPF_SYSCALL */
>
> diff --git a/kernel/bpf/syscall.c b/kernel/bpf/syscall.c
> index 1cf31ddd7616..b144181d3f3a 100644
> --- a/kernel/bpf/syscall.c
> +++ b/kernel/bp
Re: [PATCH net-next v2 4/5] selinux: bpf: Add selinux check for eBPF syscall operations
On Mon, 2017-10-09 at 15:20 -0700, Chenbo Feng wrote:
> From: Chenbo Feng
>
> Implement the actual checks introduced to eBPF related syscalls. This
> implementation use the security field inside bpf object to store a
> sid that
> identify the bpf object. And when processes try to access the object,
> selinux will check if processes have the right privileges. The
> creation
> of eBPF object are also checked at the general bpf check hook and new
> cmd introduced to eBPF domain can also be checked there.
>
> Signed-off-by: Chenbo Feng
> Acked-by: Alexei Starovoitov
> ---
> security/selinux/hooks.c| 111
>
> security/selinux/include/classmap.h | 2 +
> security/selinux/include/objsec.h | 4 ++
> 3 files changed, 117 insertions(+)
>
> diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
> index f5d304736852..41aba4e3d57c 100644
> --- a/security/selinux/hooks.c
> +++ b/security/selinux/hooks.c
> @@ -85,6 +85,7 @@
> #include
> #include
> #include
> +#include
>
> #include "avc.h"
> #include "objsec.h"
> @@ -6252,6 +6253,106 @@ static void selinux_ib_free_security(void
> *ib_sec)
> }
> #endif
>
> +#ifdef CONFIG_BPF_SYSCALL
> +static int selinux_bpf(int cmd, union bpf_attr *attr,
> + unsigned int size)
> +{
> + u32 sid = current_sid();
> + int ret;
> +
> + switch (cmd) {
> + case BPF_MAP_CREATE:
> + ret = avc_has_perm(sid, sid, SECCLASS_BPF_MAP,
> BPF_MAP__CREATE,
> + NULL);
> + break;
> + case BPF_PROG_LOAD:
> + ret = avc_has_perm(sid, sid, SECCLASS_BPF_PROG,
> BPF_PROG__LOAD,
> + NULL);
> + break;
> + default:
> + ret = 0;
> + break;
> + }
> +
> + return ret;
> +}
> +
> +static u32 bpf_map_fmode_to_av(fmode_t fmode)
> +{
> + u32 av = 0;
> +
> + if (f_mode & FMODE_READ)
> + av |= BPF_MAP__READ;
> + if (f_mode & FMODE_WRITE)
> + av |= BPF_MAP__WRITE;
> + return av;
> +}
> +
> +static int selinux_bpf_map(struct bpf_map *map, fmode_t fmode)
> +{
> + u32 sid = current_sid();
> + struct bpf_security_struct *bpfsec;
> +
> + bpfsec = map->security;
> + return avc_has_perm(sid, bpfsec->sid, SECCLASS_BPF_MAP,
> + bpf_map_fmode_to_av(fmode), NULL);
> +}
> +
> +static int selinux_bpf_prog(struct bpf_prog *prog)
> +{
> + u32 sid = current_sid();
> + struct bpf_security_struct *bpfsec;
> +
> + bpfsec = prog->aux->security;
> + return avc_has_perm(sid, bpfsec->sid, SECCLASS_BPF_PROG,
> + BPF_PROG__USE, NULL);
> +}
> +
> +static int selinux_bpf_map_alloc(struct bpf_map *map)
> +{
> + struct bpf_security_struct *bpfsec;
> +
> + bpfsec = kzalloc(sizeof(*bpfsec), GFP_KERNEL);
> + if (!bpfsec)
> + return -ENOMEM;
> +
> + bpfsec->sid = current_sid();
> + map->security = bpfsec;
> +
> + return 0;
> +}
> +
> +static void selinux_bpf_map_free(struct bpf_map *map)
> +{
> + struct bpf_security_struct *bpfsec = map->security;
> +
> + map->security = NULL;
> + kfree(bpfsec);
> +}
> +
> +static int selinux_bpf_prog_alloc(struct bpf_prog_aux *aux)
> +{
> + struct bpf_security_struct *bpfsec;
> +
> + bpfsec = kzalloc(sizeof(*bpfsec), GFP_KERNEL);
> + if (!bpfsec)
> + return -ENOMEM;
> +
> + bpfsec->sid = current_sid();
> + aux->security = bpfsec;
> +
> + return 0;
> +}
> +
> +static void selinux_bpf_prog_free(struct bpf_prog_aux *aux)
> +{
> + struct bpf_security_struct *bpfsec = aux->security;
> +
> + aux->security = NULL;
> + kfree(bpfsec);
> +}
> +#endif
> +
> static struct security_hook_list selinux_hooks[] __lsm_ro_after_init
> = {
> LSM_HOOK_INIT(binder_set_context_mgr,
> selinux_binder_set_context_mgr),
> LSM_HOOK_INIT(binder_transaction,
> selinux_binder_transaction),
> @@ -6471,6 +6572,16 @@ static struct security_hook_list
> selinux_hooks[] __lsm_ro_after_init = {
> LSM_HOOK_INIT(audit_rule_match, selinux_audit_rule_match),
> LSM_HOOK_INIT(audit_rule_free, selinux_audit_rule_free),
> #endif
> +
> +#ifdef CONFIG_BPF_SYSCALL
> + LSM_HOOK_INIT(bpf, selinux_bpf),
> + LSM_HOOK_INIT(bpf_map, selinux_bpf_map),
> + LSM_HOOK_INIT(bpf_prog, selinux_bpf_prog),
> + LSM_HOOK_INIT(bpf_map_alloc_security,
> selinux_bpf_map_alloc),
> + LSM_HOOK_INIT(bpf_prog_alloc_security,
> selinux_bpf_prog_alloc),
> + LSM_HOOK_INIT(bpf_map_free_security, selinux_bpf_map_free),
> + LSM_HOOK_INIT(bpf_prog_free_security,
> selinux_bpf_prog_free),
> +#endif
> };
>
> static __init int selinux_init(void)
> diff --git a/security/selinux/include/classmap.h
> b/security/selinux/include/classmap.h
> index 35ffb29a69cb..7253c5eea59c 100644
> --- a/security/selinux/include/classmap.h
> +++ b/security/selinux/include/classmap
Re: [PATCH net-next 4/4] selinux: bpf: Add addtional check for bpf object file receive
On Thu, 2017-10-05 at 09:37 -0400, Stephen Smalley wrote:
> On Wed, 2017-10-04 at 11:29 -0700, Chenbo Feng wrote:
> > From: Chenbo Feng
> >
> > Introduce a bpf object related check when sending and receiving
> > files
> > through unix domain socket as well as binder. It checks if the
> > receiving
> > process have privilege to read/write the bpf map or use the bpf
> > program.
> > This check is necessary because the bpf maps and programs are using
> > a
> > anonymous inode as their shared inode so the normal way of checking
> > the
> > files and sockets when passing between processes cannot work
> > properly
> > on
> > eBPF object. This check only works when the BPF_SYSCALL is
> > configured.
> >
> > Signed-off-by: Chenbo Feng
> > ---
> > include/linux/bpf.h | 3 +++
> > kernel/bpf/syscall.c | 4 ++--
> > security/selinux/hooks.c | 57
> > +++-
> > 3 files changed, 61 insertions(+), 3 deletions(-)
> >
> > diff --git a/include/linux/bpf.h b/include/linux/bpf.h
> > index d757ea3f2228..ac8428a36d56 100644
> > --- a/include/linux/bpf.h
> > +++ b/include/linux/bpf.h
> > @@ -250,6 +250,9 @@ int bpf_prog_test_run_skb(struct bpf_prog
> > *prog,
> > const union bpf_attr *kattr,
> > #ifdef CONFIG_BPF_SYSCALL
> > DECLARE_PER_CPU(int, bpf_prog_active);
> >
> > +extern const struct file_operations bpf_map_fops;
> > +extern const struct file_operations bpf_prog_fops;
> > +
> > #define BPF_PROG_TYPE(_id, _ops) \
> > extern const struct bpf_verifier_ops _ops;
> > #define BPF_MAP_TYPE(_id, _ops) \
> > diff --git a/kernel/bpf/syscall.c b/kernel/bpf/syscall.c
> > index 58ff769d58ab..5789a5359f0a 100644
> > --- a/kernel/bpf/syscall.c
> > +++ b/kernel/bpf/syscall.c
> > @@ -313,7 +313,7 @@ static ssize_t bpf_dummy_write(struct file
> > *filp,
> > const char __user *buf,
> > return -EINVAL;
> > }
> >
> > -static const struct file_operations bpf_map_fops = {
> > +const struct file_operations bpf_map_fops = {
> > #ifdef CONFIG_PROC_FS
> > .show_fdinfo= bpf_map_show_fdinfo,
> > #endif
> > @@ -965,7 +965,7 @@ static void bpf_prog_show_fdinfo(struct
> > seq_file
> > *m, struct file *filp)
> > }
> > #endif
> >
> > -static const struct file_operations bpf_prog_fops = {
> > +const struct file_operations bpf_prog_fops = {
> > #ifdef CONFIG_PROC_FS
> > .show_fdinfo= bpf_prog_show_fdinfo,
> > #endif
> > diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
> > index 41aba4e3d57c..381474ce3216 100644
> > --- a/security/selinux/hooks.c
> > +++ b/security/selinux/hooks.c
> > @@ -1847,6 +1847,7 @@ static int file_has_perm(const struct cred
> > *cred,
> >
> > /* av is zero if only checking access to the descriptor.
> > */
> > rc = 0;
> > +
> > if (av)
> > rc = inode_has_perm(cred, inode, av, &ad);
> >
> > @@ -2142,6 +2143,10 @@ static int
> > selinux_binder_transfer_binder(struct task_struct *from,
> > NULL);
> > }
> >
> > +#ifdef CONFIG_BPF_SYSCALL
> > +static int bpf_fd_pass(struct file *file, u32 sid);
> > +#endif
> > +
> > static int selinux_binder_transfer_file(struct task_struct *from,
> > struct task_struct *to,
> > struct file *file)
> > @@ -2165,6 +2170,12 @@ static int
> > selinux_binder_transfer_file(struct
> > task_struct *from,
> > return rc;
> > }
> >
> > +#ifdef CONFIG_BPF_SYSCALL
> > + rc = bpf_fd_pass(file, sid);
> > + if (rc)
> > + return rc;
> > +#endif
> > +
> > if (unlikely(IS_PRIVATE(d_backing_inode(dentry
> > return 0;
> >
> > @@ -3735,8 +3746,18 @@ static int
> > selinux_file_send_sigiotask(struct
> > task_struct *tsk,
> > static int selinux_file_receive(struct file *file)
> > {
> > const struct cred *cred = current_cred();
> > + int rc;
> > +
> > + rc = file_has_perm(cred, file, file_to_av(file));
> > + if (rc)
> > + goto out;
> > +
> > +#ifdef CONFIG_BPF_SYSCALL
> > + rc = bpf_fd_pass(file, cred_sid(sid));
> > +#endif
> >
> > - return file_has_perm(cred, file, file_to_av(file));
> > +out:
> > + return rc;
> > }
> &g
Re: [PATCH net-next 4/4] selinux: bpf: Add addtional check for bpf object file receive
On Wed, 2017-10-04 at 11:29 -0700, Chenbo Feng wrote:
> From: Chenbo Feng
>
> Introduce a bpf object related check when sending and receiving files
> through unix domain socket as well as binder. It checks if the
> receiving
> process have privilege to read/write the bpf map or use the bpf
> program.
> This check is necessary because the bpf maps and programs are using a
> anonymous inode as their shared inode so the normal way of checking
> the
> files and sockets when passing between processes cannot work properly
> on
> eBPF object. This check only works when the BPF_SYSCALL is
> configured.
>
> Signed-off-by: Chenbo Feng
> ---
> include/linux/bpf.h | 3 +++
> kernel/bpf/syscall.c | 4 ++--
> security/selinux/hooks.c | 57
> +++-
> 3 files changed, 61 insertions(+), 3 deletions(-)
>
> diff --git a/include/linux/bpf.h b/include/linux/bpf.h
> index d757ea3f2228..ac8428a36d56 100644
> --- a/include/linux/bpf.h
> +++ b/include/linux/bpf.h
> @@ -250,6 +250,9 @@ int bpf_prog_test_run_skb(struct bpf_prog *prog,
> const union bpf_attr *kattr,
> #ifdef CONFIG_BPF_SYSCALL
> DECLARE_PER_CPU(int, bpf_prog_active);
>
> +extern const struct file_operations bpf_map_fops;
> +extern const struct file_operations bpf_prog_fops;
> +
> #define BPF_PROG_TYPE(_id, _ops) \
> extern const struct bpf_verifier_ops _ops;
> #define BPF_MAP_TYPE(_id, _ops) \
> diff --git a/kernel/bpf/syscall.c b/kernel/bpf/syscall.c
> index 58ff769d58ab..5789a5359f0a 100644
> --- a/kernel/bpf/syscall.c
> +++ b/kernel/bpf/syscall.c
> @@ -313,7 +313,7 @@ static ssize_t bpf_dummy_write(struct file *filp,
> const char __user *buf,
> return -EINVAL;
> }
>
> -static const struct file_operations bpf_map_fops = {
> +const struct file_operations bpf_map_fops = {
> #ifdef CONFIG_PROC_FS
> .show_fdinfo= bpf_map_show_fdinfo,
> #endif
> @@ -965,7 +965,7 @@ static void bpf_prog_show_fdinfo(struct seq_file
> *m, struct file *filp)
> }
> #endif
>
> -static const struct file_operations bpf_prog_fops = {
> +const struct file_operations bpf_prog_fops = {
> #ifdef CONFIG_PROC_FS
> .show_fdinfo= bpf_prog_show_fdinfo,
> #endif
> diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
> index 41aba4e3d57c..381474ce3216 100644
> --- a/security/selinux/hooks.c
> +++ b/security/selinux/hooks.c
> @@ -1847,6 +1847,7 @@ static int file_has_perm(const struct cred
> *cred,
>
> /* av is zero if only checking access to the descriptor. */
> rc = 0;
> +
> if (av)
> rc = inode_has_perm(cred, inode, av, &ad);
>
> @@ -2142,6 +2143,10 @@ static int
> selinux_binder_transfer_binder(struct task_struct *from,
> NULL);
> }
>
> +#ifdef CONFIG_BPF_SYSCALL
> +static int bpf_fd_pass(struct file *file, u32 sid);
> +#endif
> +
> static int selinux_binder_transfer_file(struct task_struct *from,
> struct task_struct *to,
> struct file *file)
> @@ -2165,6 +2170,12 @@ static int selinux_binder_transfer_file(struct
> task_struct *from,
> return rc;
> }
>
> +#ifdef CONFIG_BPF_SYSCALL
> + rc = bpf_fd_pass(file, sid);
> + if (rc)
> + return rc;
> +#endif
> +
> if (unlikely(IS_PRIVATE(d_backing_inode(dentry
> return 0;
>
> @@ -3735,8 +3746,18 @@ static int selinux_file_send_sigiotask(struct
> task_struct *tsk,
> static int selinux_file_receive(struct file *file)
> {
> const struct cred *cred = current_cred();
> + int rc;
> +
> + rc = file_has_perm(cred, file, file_to_av(file));
> + if (rc)
> + goto out;
> +
> +#ifdef CONFIG_BPF_SYSCALL
> + rc = bpf_fd_pass(file, cred_sid(sid));
> +#endif
>
> - return file_has_perm(cred, file, file_to_av(file));
> +out:
> + return rc;
> }
>
> static int selinux_file_open(struct file *file, const struct cred
> *cred)
> @@ -6288,6 +6309,40 @@ static u32 bpf_map_fmode_to_av(fmode_t fmode)
> return av;
> }
>
> +/* This function will check the file pass through unix socket or
> binder to see
> + * if it is a bpf related object. And apply correspinding checks on
> the bpf
> + * object based on the type. The bpf maps and programs, not like
> other files and
> + * socket, are using a shared anonymous inode inside the kernel as
> their inode.
> + * So checking that inode cannot identify if the process have
> privilege to
> + * access the bpf object and that's why we have to add this
> additional check in
> + * selinux_file_receive and selinux_binder_transfer_files.
> + */
> +static int bpf_fd_pass(struct file *file, u32 sid)
> +{
> + struct bpf_security_struct *bpfsec;
> + u32 sid = cred_sid(cred);
> + struct bpf_prog *prog;
> + struct bpf_map *map;
> + int ret;
> +
> + if (file->f_op == &bpf_map_fops) {
> + map = file->private_data;
> + bpfsec =
Re: [PATCH net-next 3/4] selinux: bpf: Add selinux check for eBPF syscall operations
On Wed, 2017-10-04 at 11:29 -0700, Chenbo Feng wrote:
> From: Chenbo Feng
>
> Implement the actual checks introduced to eBPF related syscalls. This
> implementation use the security field inside bpf object to store a
> sid that
> identify the bpf object. And when processes try to access the object,
> selinux will check if processes have the right privileges. The
> creation
> of eBPF object are also checked at the general bpf check hook and new
> cmd introduced to eBPF domain can also be checked there.
>
> Signed-off-by: Chenbo Feng
> Acked-by: Alexei Starovoitov
> ---
> security/selinux/hooks.c| 111
>
> security/selinux/include/classmap.h | 2 +
> security/selinux/include/objsec.h | 4 ++
> 3 files changed, 117 insertions(+)
>
> diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
> index f5d304736852..41aba4e3d57c 100644
> --- a/security/selinux/hooks.c
> +++ b/security/selinux/hooks.c
> @@ -85,6 +85,7 @@
> #include
> #include
> #include
> +#include
>
> #include "avc.h"
> #include "objsec.h"
> @@ -6252,6 +6253,106 @@ static void selinux_ib_free_security(void
> *ib_sec)
> }
> #endif
>
> +#ifdef CONFIG_BPF_SYSCALL
> +static int selinux_bpf(int cmd, union bpf_attr *attr,
> + unsigned int size)
> +{
> + u32 sid = current_sid();
> + int ret;
> +
> + switch (cmd) {
> + case BPF_MAP_CREATE:
> + ret = avc_has_perm(sid, sid, SECCLASS_BPF_MAP,
> BPF_MAP__CREATE,
> + NULL);
> + break;
> + case BPF_PROG_LOAD:
> + ret = avc_has_perm(sid, sid, SECCLASS_BPF_PROG,
> BPF_PROG__LOAD,
> + NULL);
> + break;
> + default:
> + ret = 0;
> + break;
> + }
> +
> + return ret;
> +}
> +
> +static u32 bpf_map_fmode_to_av(fmode_t fmode)
> +{
> + u32 av = 0;
> +
> + if (f_mode & FMODE_READ)
> + av |= BPF_MAP__READ;
> + if (f_mode & FMODE_WRITE)
> + av |= BPF_MAP__WRITE;
> + return av;
> +}
> +
> +static int selinux_bpf_map(struct bpf_map *map, fmode_t fmode)
> +{
> + u32 sid = current_sid();
> + struct bpf_security_struct *bpfsec;
> +
> + bpfsec = map->security;
> + return avc_has_perm(sid, bpfsec->sid, SECCLASS_BPF_MAP,
> + bpf_map_fmode_to_av(fmode), NULL);
> +}
> +
> +static int selinux_bpf_prog(struct bpf_prog *prog)
> +{
> + u32 sid = current_sid();
> + struct bpf_security_struct *bpfsec;
> +
> + bpfsec = prog->aux->security;
I haven't looked closely at the bpf code, but is it guaranteed that
prog->aux cannot be NULL here? What's the difference in lifecycle for
bpf_prog vs bpf_prog_aux? Could the aux field be shared across progs
created by different processes?
> + return avc_has_perm(sid, bpfsec->sid, SECCLASS_BPF_PROG,
> + BPF_PROG__USE, NULL);
> +}
> +
> +static int selinux_bpf_map_alloc(struct bpf_map *map)
> +{
> + struct bpf_security_struct *bpfsec;
> +
> + bpfsec = kzalloc(sizeof(*bpfsec), GFP_KERNEL);
> + if (!bpfsec)
> + return -ENOMEM;
> +
> + bpfsec->sid = current_sid();
> + map->security = bpfsec;
> +
> + return 0;
> +}
> +
> +static void selinux_bpf_map_free(struct bpf_map *map)
> +{
> + struct bpf_security_struct *bpfsec = map->security;
> +
> + map->security = NULL;
> + kfree(bpfsec);
> +}
> +
> +static int selinux_bpf_prog_alloc(struct bpf_prog_aux *aux)
> +{
> + struct bpf_security_struct *bpfsec;
> +
> + bpfsec = kzalloc(sizeof(*bpfsec), GFP_KERNEL);
> + if (!bpfsec)
> + return -ENOMEM;
> +
> + bpfsec->sid = current_sid();
> + aux->security = bpfsec;
> +
> + return 0;
> +}
> +
> +static void selinux_bpf_prog_free(struct bpf_prog_aux *aux)
> +{
> + struct bpf_security_struct *bpfsec = aux->security;
> +
> + aux->security = NULL;
> + kfree(bpfsec);
> +}
> +#endif
> +
> static struct security_hook_list selinux_hooks[] __lsm_ro_after_init
> = {
> LSM_HOOK_INIT(binder_set_context_mgr,
> selinux_binder_set_context_mgr),
> LSM_HOOK_INIT(binder_transaction,
> selinux_binder_transaction),
> @@ -6471,6 +6572,16 @@ static struct security_hook_list
> selinux_hooks[] __lsm_ro_after_init = {
> LSM_HOOK_INIT(audit_rule_match, selinux_audit_rule_match),
> LSM_HOOK_INIT(audit_rule_free, selinux_audit_rule_free),
> #endif
> +
> +#ifdef CONFIG_BPF_SYSCALL
> + LSM_HOOK_INIT(bpf, selinux_bpf),
> + LSM_HOOK_INIT(bpf_map, selinux_bpf_map),
> + LSM_HOOK_INIT(bpf_prog, selinux_bpf_prog),
> + LSM_HOOK_INIT(bpf_map_alloc_security,
> selinux_bpf_map_alloc),
> + LSM_HOOK_INIT(bpf_prog_alloc_security,
> selinux_bpf_prog_alloc),
> + LSM_HOOK_INIT(bpf_map_free_security, selinux_bpf_map_free),
> + LSM_HOOK_INIT(bpf_prog_free_security,
> selinux_bpf_prog_free),
> +#endif
> };
>
> static __ini
Re: [PATCH 1/3] security: bpf: Add eBPF LSM hooks to security module
On Tue, 2017-09-05 at 15:24 -0700, Chenbo Feng via Selinux wrote:
> On Fri, Sep 1, 2017 at 5:50 AM, Stephen Smalley
> wrote:
> > On Thu, 2017-08-31 at 13:56 -0700, Chenbo Feng wrote:
> > > From: Chenbo Feng
> > >
> > > Introduce 5 LSM hooks to provide finer granularity controls on
> > > eBPF
> > > related operations including create eBPF maps, modify and read
> > > eBPF
> > > maps
> > > content and load eBPF programs to the kernel. Hooks use the new
> > > security
> > > pointer inside the eBPF map struct to store the owner's security
> > > information and the different security modules can perform
> > > different
> > > checks based on the information stored inside the security field.
> > >
> > > Signed-off-by: Chenbo Feng
> > > ---
> > > include/linux/lsm_hooks.h | 41
> > > +
> > > include/linux/security.h | 36
> > >
> > > security/security.c | 28
> > > 3 files changed, 105 insertions(+)
> > >
> > > diff --git a/include/linux/lsm_hooks.h
> > > b/include/linux/lsm_hooks.h
> > > index ce02f76a6188..3aaf9a08a983 100644
> > > --- a/include/linux/lsm_hooks.h
> > > +++ b/include/linux/lsm_hooks.h
> > > @@ -1353,6 +1353,32 @@
> > > * @inode we wish to get the security context of.
> > > * @ctx is a pointer in which to place the allocated security
> > > context.
> > > * @ctxlen points to the place to put the length of @ctx.
> > > + *
> > > + * Security hooks for using the eBPF maps and programs
> > > functionalities through
> > > + * eBPF syscalls.
> > > + *
> > > + * @bpf_map_create:
> > > + * Check permissions prior to creating a new bpf map.
> > > + * Return 0 if the permission is granted.
> > > + *
> > > + * @bpf_map_modify:
> > > + * Check permission prior to insert, update and delete map
> > > content.
> > > + * @map pointer to the struct bpf_map that contains map
> > > information.
> > > + * Return 0 if the permission is granted.
> > > + *
> > > + * @bpf_map_read:
> > > + * Check permission prior to read a bpf map content.
> > > + * @map pointer to the struct bpf_map that contains map
> > > information.
> > > + * Return 0 if the permission is granted.
> > > + *
> > > + * @bpf_prog_load:
> > > + * Check permission prior to load eBPF program.
> > > + * Return 0 if the permission is granted.
> > > + *
> > > + * @bpf_post_create:
> > > + * Initialize the bpf object security field inside struct
> > > bpf_maps and
> > > + * it is used for future security checks.
> > > + *
> > > */
> > > union security_list_options {
> > > int (*binder_set_context_mgr)(struct task_struct *mgr);
> > > @@ -1685,6 +1711,14 @@ union security_list_options {
> > > struct audit_context *actx);
> > > void (*audit_rule_free)(void *lsmrule);
> > > #endif /* CONFIG_AUDIT */
> > > +
> > > +#ifdef CONFIG_BPF_SYSCALL
> > > + int (*bpf_map_create)(void);
> > > + int (*bpf_map_read)(struct bpf_map *map);
> > > + int (*bpf_map_modify)(struct bpf_map *map);
> > > + int (*bpf_prog_load)(void);
> > > + int (*bpf_post_create)(struct bpf_map *map);
> > > +#endif /* CONFIG_BPF_SYSCALL */
> > > };
> > >
> > > struct security_hook_heads {
> > > @@ -1905,6 +1939,13 @@ struct security_hook_heads {
> > > struct list_head audit_rule_match;
> > > struct list_head audit_rule_free;
> > > #endif /* CONFIG_AUDIT */
> > > +#ifdef CONFIG_BPF_SYSCALL
> > > + struct list_head bpf_map_create;
> > > + struct list_head bpf_map_read;
> > > + struct list_head bpf_map_modify;
> > > + struct list_head bpf_prog_load;
> > > + struct list_head bpf_post_create;
> > > +#endif /* CONFIG_BPF_SYSCALL */
> > > } __randomize_layout;
> > >
> > > /*
> > > diff --git a/include/linux/security.h b/include/linux/security.h
> > > index 458e24bea2d4..0656a4f74d14 100644
> > > --- a/include/linux/security.h
> > > +++ b/include/linux/security.h
> > > @@ -31,6 +31,7 @@
> > > #include
> > >
Re: [PATCH 1/3] security: bpf: Add eBPF LSM hooks to security module
On Thu, 2017-08-31 at 13:56 -0700, Chenbo Feng wrote:
> From: Chenbo Feng
>
> Introduce 5 LSM hooks to provide finer granularity controls on eBPF
> related operations including create eBPF maps, modify and read eBPF
> maps
> content and load eBPF programs to the kernel. Hooks use the new
> security
> pointer inside the eBPF map struct to store the owner's security
> information and the different security modules can perform different
> checks based on the information stored inside the security field.
>
> Signed-off-by: Chenbo Feng
> ---
> include/linux/lsm_hooks.h | 41
> +
> include/linux/security.h | 36
> security/security.c | 28
> 3 files changed, 105 insertions(+)
>
> diff --git a/include/linux/lsm_hooks.h b/include/linux/lsm_hooks.h
> index ce02f76a6188..3aaf9a08a983 100644
> --- a/include/linux/lsm_hooks.h
> +++ b/include/linux/lsm_hooks.h
> @@ -1353,6 +1353,32 @@
> * @inode we wish to get the security context of.
> * @ctx is a pointer in which to place the allocated security
> context.
> * @ctxlen points to the place to put the length of @ctx.
> + *
> + * Security hooks for using the eBPF maps and programs
> functionalities through
> + * eBPF syscalls.
> + *
> + * @bpf_map_create:
> + * Check permissions prior to creating a new bpf map.
> + * Return 0 if the permission is granted.
> + *
> + * @bpf_map_modify:
> + * Check permission prior to insert, update and delete map
> content.
> + * @map pointer to the struct bpf_map that contains map
> information.
> + * Return 0 if the permission is granted.
> + *
> + * @bpf_map_read:
> + * Check permission prior to read a bpf map content.
> + * @map pointer to the struct bpf_map that contains map
> information.
> + * Return 0 if the permission is granted.
> + *
> + * @bpf_prog_load:
> + * Check permission prior to load eBPF program.
> + * Return 0 if the permission is granted.
> + *
> + * @bpf_post_create:
> + * Initialize the bpf object security field inside struct
> bpf_maps and
> + * it is used for future security checks.
> + *
> */
> union security_list_options {
> int (*binder_set_context_mgr)(struct task_struct *mgr);
> @@ -1685,6 +1711,14 @@ union security_list_options {
> struct audit_context *actx);
> void (*audit_rule_free)(void *lsmrule);
> #endif /* CONFIG_AUDIT */
> +
> +#ifdef CONFIG_BPF_SYSCALL
> + int (*bpf_map_create)(void);
> + int (*bpf_map_read)(struct bpf_map *map);
> + int (*bpf_map_modify)(struct bpf_map *map);
> + int (*bpf_prog_load)(void);
> + int (*bpf_post_create)(struct bpf_map *map);
> +#endif /* CONFIG_BPF_SYSCALL */
> };
>
> struct security_hook_heads {
> @@ -1905,6 +1939,13 @@ struct security_hook_heads {
> struct list_head audit_rule_match;
> struct list_head audit_rule_free;
> #endif /* CONFIG_AUDIT */
> +#ifdef CONFIG_BPF_SYSCALL
> + struct list_head bpf_map_create;
> + struct list_head bpf_map_read;
> + struct list_head bpf_map_modify;
> + struct list_head bpf_prog_load;
> + struct list_head bpf_post_create;
> +#endif /* CONFIG_BPF_SYSCALL */
> } __randomize_layout;
>
> /*
> diff --git a/include/linux/security.h b/include/linux/security.h
> index 458e24bea2d4..0656a4f74d14 100644
> --- a/include/linux/security.h
> +++ b/include/linux/security.h
> @@ -31,6 +31,7 @@
> #include
> #include
> #include
> +#include
>
> struct linux_binprm;
> struct cred;
> @@ -1735,6 +1736,41 @@ static inline void securityfs_remove(struct
> dentry *dentry)
>
> #endif
>
> +#ifdef CONFIG_BPF_SYSCALL
> +#ifdef CONFIG_SECURITY
> +int security_map_create(void);
> +int security_map_modify(struct bpf_map *map);
> +int security_map_read(struct bpf_map *map);
> +int security_prog_load(void);
> +int security_post_create(struct bpf_map *map);
> +#else
> +static inline int security_map_create(void)
> +{
> + return 0;
> +}
> +
> +static inline int security_map_read(struct bpf_map *map)
> +{
> + return 0;
> +}
> +
> +static inline int security_map_modify(struct bpf_map *map)
> +{
> + return 0;
> +}
> +
> +static inline int security_prog_load(void)
> +{
> + return 0;
> +}
> +
> +static inline int security_post_create(struct bpf_map *map)
> +{
> + return 0;
> +}
> +#endif /* CONFIG_SECURITY */
> +#endif /* CONFIG_BPF_SYSCALL */
These should be named consistently with the ones in lsm_hooks.h and
should unambiguously indicate that these are hooks for bpf
objects/operations, i.e. security_bpf_map_create(),
security_bpf_map_read(), etc.
Do you need this level of granularity?
Could you coalesce the map_create() and post_map_create() hooks into
one hook and just unwind the create in that case?
Why do you label bpf maps but not bpf progs? Should we be controlling
the ability to attach/detach a bpf prog (partly controlled by
CAP_NET_ADMIN, but also somewhat broad in scop
Re: Permissions for eBPF objects
On Fri, 2017-08-25 at 12:52 -0700, Chenbo Feng via Selinux wrote: > On Fri, Aug 25, 2017 at 12:45 PM, Jeffrey Vander Stoep com> wrote: > > On Fri, Aug 25, 2017 at 12:26 PM, Stephen Smalley > v> wrote: > > > On Fri, 2017-08-25 at 11:01 -0700, Jeffrey Vander Stoep via > > > Selinux > > > wrote: > > > > I’d like to get your thoughts on adding LSM permission checks > > > > on BPF > > > > objects. > > > > > > > > By default, the ability to create and use eBPF maps/programs > > > > requires > > > > CAP_SYS_ADMIN [1]. Alternatively, all processes can be granted > > > > access > > > > to bpf() functions. This seems like poor granularity. [2] > > > > > > > > Like files and sockets, eBPF maps and programs can be passed > > > > between > > > > processes by FD and have a number of functions that map cleanly > > > > to > > > > permissions. > > > > > > > > Let me know what you think. Are there simpler alternative > > > > approaches > > > > that we haven’t considered? > > > > > > Is it possible to create the map/program in one process (with > > > CAP_SYS_ADMIN), pass the resulting fd to netd, and then use it > > > there > > > (without requiring CAP_SYS_ADMIN in netd itself)? > > > > That might work. Any use of bpf() requires CAP_SYS_ADMIN but netd > > could potentially just apply the prog_fd to a socket: > > > > setsockopt(sockfd, SOL_SOCKET, SO_ATTACH_BPF, > > &prog_fd, sizeof(prog_fd)); > > > > This specific case might work. But other map and program related > operations can > only be done through syscalls. And the syscall can be set to only > allow > CAP_SYS_ADMIN processes to use it or open to all processes. So when > the > CAP_SYS_ADMIN limitation is enforced, netd will not be able to use > any of the > syscalls such as map_look_up, map_update, map_delete even if a > CAP_SYS_ADMIN process passed the fd to it. Here is how this > enforcement > implemented: > http://elixir.free- > electrons.com/linux/latest/source/kernel/bpf/syscall.c#L1005 I guess the question is whether netd needs to perform any of those operations itself, or if all of that can be done by another process and netd can just receive the fd over a unix socket and attach it. Not opposed to adding a LSM hook to bpf() and implementing a SELinux check there, just not 100% sure if you need it. > > > > > > > What level of granularity would be useful? Would it go beyond > > > just > > > being able to use bpf() at all? > > > > "use" might be sufficient. At least initially. > > > > I could see some others coming in handy. For example, a simple > > mapping > > of functionality to permissions gives: > > map_create, map_update, map_delete, map_read, prog_load, prog_use. > > > > Of course there's no sense in breaking "use" into multiple > > permissions if > > we expect the entire set to always be granted together. > > > > > > > > > > > > > Thanks! > > > > Jeff > > > > > > > > [1] http://man7.org/linux/man-pages/man2/bpf.2.html NOTES > > > > section > > > > [2] We are considering eBPF for network filtering by netd. > > > > Giving > > > > netd > > > > CAP_SYS_ADMIN would considerably increase netd’s privileges. > > > > Alternatively allowing all processes permission to use bpf() > > > > goes > > > > against the principle of least privilege exposing a lot of > > > > kernel > > > > attack surface to processes that do not actually need it. > > > >
Re: Permissions for eBPF objects
On Fri, 2017-08-25 at 11:01 -0700, Jeffrey Vander Stoep via Selinux wrote: > I’d like to get your thoughts on adding LSM permission checks on BPF > objects. > > By default, the ability to create and use eBPF maps/programs requires > CAP_SYS_ADMIN [1]. Alternatively, all processes can be granted access > to bpf() functions. This seems like poor granularity. [2] > > Like files and sockets, eBPF maps and programs can be passed between > processes by FD and have a number of functions that map cleanly to > permissions. > > Let me know what you think. Are there simpler alternative approaches > that we haven’t considered? Is it possible to create the map/program in one process (with CAP_SYS_ADMIN), pass the resulting fd to netd, and then use it there (without requiring CAP_SYS_ADMIN in netd itself)? What level of granularity would be useful? Would it go beyond just being able to use bpf() at all? > > Thanks! > Jeff > > [1] http://man7.org/linux/man-pages/man2/bpf.2.html NOTES section > [2] We are considering eBPF for network filtering by netd. Giving > netd > CAP_SYS_ADMIN would considerably increase netd’s privileges. > Alternatively allowing all processes permission to use bpf() goes > against the principle of least privilege exposing a lot of kernel > attack surface to processes that do not actually need it. >
Re: [Patch net] net: saving irq context for peernet2id()
On 10/21/2016 12:47 AM, Cong Wang wrote: > On Thu, Oct 20, 2016 at 4:35 PM, Cong Wang wrote: >> Since you want to test SELinux anyway, please test the attached one. >> > > Finally my kernel config is friendly to SELinux, and now there are several > tests fails: > > > Test Summary Report > --- > sysctl/test (Wstat: 0 Tests: 4 Failed: 2) > Failed tests: 1-2 > mmap/test (Wstat: 0 Tests: 44 Failed: 2) > Failed tests: 20, 26 > overlay/test (Wstat: 65280 Tests: 0 Failed: 0) > Non-zero exit status: 255 > Parse errors: Bad plan. You planned 121 tests but ran 0. > Files=44, Tests=306, 47 wallclock secs ( 0.23 usr 0.30 sys + 1.00 > cusr 7.35 csys = 8.88 CPU) > Result: FAIL > > Seems not related to my patch? Probably kernel config related. All tests pass for me on rawhide. Kernel config? > And, there is a kernel warning which is totally unrelated to my code: > > > [ 136.788836] [ cut here ] > [ 136.793046] WARNING: CPU: 1 PID: 1345 at fs/locks.c:615 > locks_unlink_lock_ctx+0xc7/0xcc > [ 136.799829] CPU: 1 PID: 1345 Comm: test_lease Not tainted 4.8.0+ #271 > [ 136.803814] Hardware name: Bochs Bochs, BIOS Bochs 01/01/2011 > [ 136.803814] c900036bfd38 8152c08e > > [ 136.803814] c900036bfd78 810841c0 0267036bfc88 > 8800db2371e8 > [ 136.803814] 8800db26f4c0 c900036bfe00 c900036bfe00 > 8800db239240 > [ 136.803814] Call Trace: > [ 136.803814] [] dump_stack+0x67/0x90 > [ 136.803814] [] __warn+0xbd/0xd8 > [ 136.803814] [] warn_slowpath_null+0x1d/0x1f > [ 136.803814] [] locks_unlink_lock_ctx+0xc7/0xcc > [ 136.803814] [] locks_delete_lock_ctx+0x17/0x3a > [ 136.803814] [] lease_modify+0xb0/0xb9 > [ 136.803814] [] locks_remove_file+0x93/0xbd > [ 136.803814] [] __fput+0xd0/0x19b > [ 136.803814] [] fput+0xe/0x10 > [ 136.803814] [] task_work_run+0x6c/0x97 > [ 136.803814] [] prepare_exit_to_usermode+0xb0/0xcc > [ 136.803814] [] syscall_return_slowpath+0x15e/0x1c2 > [ 136.803814] [] do_syscall_64+0x6f/0x76 > [ 136.803814] [] entry_SYSCALL64_slow_path+0x25/0x25 > [ 138.538370] ---[ end trace 90914a16bd3272b8 ]--- Don't see this on current kernels (e.g. 4.9-rc1).
Re: [Patch net] net: saving irq context for peernet2id()
On 10/20/2016 02:52 AM, Cong Wang wrote:
> A kernel warning inside __local_bh_enable_ip() was reported by people
> running SELinux, this is caused due to some SELinux functions
> (indirectly) call peernet2id() with IRQ disabled in process context,
> when we re-enable BH with IRQ disabled kernel complains. Shut up this
> warning by saving IRQ context in peernet2id(), BH is still implicitly
> disabled.
Not sure this suffices; kill_fasync() -> send_sigio() ->
send_sigio_to_task() -> sigio_perm() -> security_file_send_sigiotask()
-> selinux_file_send_sigiotask() -> ... -> audit_log() -> ... ->
peernet2id()
>
> Fixes: bc51dddf98c9 ("netns: avoid disabling irq for netns id")
> Reported-by: Stephen Smalley
> Reported-by: Elad Raz
> Tested-by: Paul Moore
> Signed-off-by: Cong Wang
> ---
> net/core/net_namespace.c | 9 +++--
> 1 file changed, 7 insertions(+), 2 deletions(-)
>
> diff --git a/net/core/net_namespace.c b/net/core/net_namespace.c
> index 989434f..17b52a2 100644
> --- a/net/core/net_namespace.c
> +++ b/net/core/net_namespace.c
> @@ -230,11 +230,16 @@ int peernet2id_alloc(struct net *net, struct net *peer)
> /* This function returns, if assigned, the id of a peer netns. */
> int peernet2id(struct net *net, struct net *peer)
> {
> + unsigned long flags;
> int id;
>
> - spin_lock_bh(&net->nsid_lock);
> + /* This is ugly, technically we only need to disable BH, however some
> + * callers (indirectly) call this with IRQ disabled, which implies
> + * that we should save the IRQ context here.
> + */
> + spin_lock_irqsave(&net->nsid_lock, flags);
> id = __peernet2id(net, peer);
> - spin_unlock_bh(&net->nsid_lock);
> + spin_unlock_irqrestore(&net->nsid_lock, flags);
> return id;
> }
> EXPORT_SYMBOL(peernet2id);
>
Re: Possible problem with e6afc8ac ("udp: remove headers from UDP packets before queueing")
On 06/01/2016 03:18 PM, Eric Dumazet wrote:
> On Wed, 2016-06-01 at 15:01 -0400, Paul Moore wrote:
>> Hello,
>>
>> I'm currently trying to debug a problem with 4.7-rc1 and labeled
>> networking over UDP. I'm having some difficulty with the latest
>> 4.7-rc1 builds on my test system at the moment so I haven't been able
>> to concisely identify the problem, but looking through the commits in
>> 4.7-rc1 I think there may be a problem with the following:
>>
>> commit e6afc8ace6dd5cef5e812f26c72579da8806f5ac
>> Author: samanthakumar
>> Date: Tue Apr 5 12:41:15 2016 -0400
>>
>>udp: remove headers from UDP packets before queueing
>>
>>Remove UDP transport headers before queueing packets for reception.
>>This change simplifies a follow-up patch to add MSG_PEEK support.
>>
>>Signed-off-by: Sam Kumar
>>Signed-off-by: Willem de Bruijn
>>Signed-off-by: David S. Miller
>>
>> ... it appears that this commit changes things so that sk_filter() is
>> only called when sk->sk_filter is not NULL. While this is fine for
>> the traditional socket filter case, it causes problems with LSMs that
>> make use of security_sock_rcv_skb() to enforce per-packet access
>> controls.
>>
>> Hopefully I'll get 4.7-rc1 booting soon and I can do a proper
>> bisection test around this patch, but I wanted to mention this now in
>> case others are seeing the same problem.
>>
>
> Thanks for the report. Please try following fix.
>
> sk_filter() got additional features like the skb_pfmemalloc() things and
> security_sock_rcv_skb()
This resolved the SELinux regression for me.
Tested-by: Stephen Smalley
>
> diff --git a/net/ipv4/udp.c b/net/ipv4/udp.c
> index d56c0559b477..0ff31d97d485 100644
> --- a/net/ipv4/udp.c
> +++ b/net/ipv4/udp.c
> @@ -1618,12 +1618,12 @@ int udp_queue_rcv_skb(struct sock *sk, struct sk_buff
> *skb)
> }
> }
>
> - if (rcu_access_pointer(sk->sk_filter)) {
> - if (udp_lib_checksum_complete(skb))
> + if (rcu_access_pointer(sk->sk_filter) &&
> + udp_lib_checksum_complete(skb))
> goto csum_error;
> - if (sk_filter(sk, skb))
> - goto drop;
> - }
> +
> + if (sk_filter(sk, skb))
> + goto drop;
>
> udp_csum_pull_header(skb);
> if (sk_rcvqueues_full(sk, sk->sk_rcvbuf)) {
> diff --git a/net/ipv6/udp.c b/net/ipv6/udp.c
> index 2da1896af934..f421c9f23c5b 100644
> --- a/net/ipv6/udp.c
> +++ b/net/ipv6/udp.c
> @@ -653,12 +653,12 @@ int udpv6_queue_rcv_skb(struct sock *sk, struct sk_buff
> *skb)
> }
> }
>
> - if (rcu_access_pointer(sk->sk_filter)) {
> - if (udp_lib_checksum_complete(skb))
> - goto csum_error;
> - if (sk_filter(sk, skb))
> - goto drop;
> - }
> + if (rcu_access_pointer(sk->sk_filter) &&
> + udp_lib_checksum_complete(skb))
> + goto csum_error;
> +
> + if (sk_filter(sk, skb))
> + goto drop;
>
> udp_csum_pull_header(skb);
> if (sk_rcvqueues_full(sk, sk->sk_rcvbuf)) {
>
>
> ___
> Selinux mailing list
> seli...@tycho.nsa.gov
> To unsubscribe, send email to selinux-le...@tycho.nsa.gov.
> To get help, send an email containing "help" to selinux-requ...@tycho.nsa.gov.
>
[PATCH] net/tipc: initialize security state for new connection socket
Calling connect() with an AF_TIPC socket would trigger a series
of error messages from SELinux along the lines of:
SELinux: Invalid class 0
type=AVC msg=audit(1434126658.487:34500): avc: denied { }
for pid=292 comm="kworker/u16:5" scontext=system_u:system_r:kernel_t:s0
tcontext=system_u:object_r:unlabeled_t:s0 tclass=
permissive=0
This was due to a failure to initialize the security state of the new
connection sock by the tipc code, leaving it with junk in the security
class field and an unlabeled secid. Add a call to security_sk_clone()
to inherit the security state from the parent socket.
Reported-by: Tim Shearer
Signed-off-by: Stephen Smalley
Acked-by: Paul Moore
---
net/tipc/socket.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/net/tipc/socket.c b/net/tipc/socket.c
index 46b6ed5..3a7567f 100644
--- a/net/tipc/socket.c
+++ b/net/tipc/socket.c
@@ -2007,6 +2007,7 @@ static int tipc_accept(struct socket *sock, struct socket
*new_sock, int flags)
res = tipc_sk_create(sock_net(sock->sk), new_sock, 0, 1);
if (res)
goto exit;
+ security_sk_clone(sock->sk, new_sock->sk);
new_sk = new_sock->sk;
new_tsock = tipc_sk(new_sk);
--
2.1.0
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH net-next] net/unix: support SCM_SECURITY for stream sockets
SCM_SECURITY was originally only implemented for datagram sockets,
not for stream sockets. However, SCM_CREDENTIALS is supported on
Unix stream sockets. For consistency, implement Unix stream support
for SCM_SECURITY as well. Also clean up the existing code and get
rid of the superfluous UNIXSID macro.
Motivated by https://bugzilla.redhat.com/show_bug.cgi?id=1224211,
where systemd was using SCM_CREDENTIALS and assumed wrongly that
SCM_SECURITY was also supported on Unix stream sockets.
Signed-off-by: Stephen Smalley
Acked-by: Paul Moore
---
include/net/af_unix.h | 1 -
net/unix/af_unix.c| 20
2 files changed, 16 insertions(+), 5 deletions(-)
diff --git a/include/net/af_unix.h b/include/net/af_unix.h
index a175ba4..4a167b3 100644
--- a/include/net/af_unix.h
+++ b/include/net/af_unix.h
@@ -39,7 +39,6 @@ struct unix_skb_parms {
};
#define UNIXCB(skb)(*(struct unix_skb_parms *)&((skb)->cb))
-#define UNIXSID(skb) (&UNIXCB((skb)).secid)
#define unix_state_lock(s) spin_lock(&unix_sk(s)->lock)
#define unix_state_unlock(s) spin_unlock(&unix_sk(s)->lock)
diff --git a/net/unix/af_unix.c b/net/unix/af_unix.c
index f25e167..03ee4d3 100644
--- a/net/unix/af_unix.c
+++ b/net/unix/af_unix.c
@@ -140,12 +140,17 @@ static struct hlist_head *unix_sockets_unbound(void *addr)
#ifdef CONFIG_SECURITY_NETWORK
static void unix_get_secdata(struct scm_cookie *scm, struct sk_buff *skb)
{
- memcpy(UNIXSID(skb), &scm->secid, sizeof(u32));
+ UNIXCB(skb).secid = scm->secid;
}
static inline void unix_set_secdata(struct scm_cookie *scm, struct sk_buff
*skb)
{
- scm->secid = *UNIXSID(skb);
+ scm->secid = UNIXCB(skb).secid;
+}
+
+static inline bool unix_secdata_eq(struct scm_cookie *scm, struct sk_buff *skb)
+{
+ return (scm->secid == UNIXCB(skb).secid);
}
#else
static inline void unix_get_secdata(struct scm_cookie *scm, struct sk_buff
*skb)
@@ -153,6 +158,11 @@ static inline void unix_get_secdata(struct scm_cookie
*scm, struct sk_buff *skb)
static inline void unix_set_secdata(struct scm_cookie *scm, struct sk_buff
*skb)
{ }
+
+static inline bool unix_secdata_eq(struct scm_cookie *scm, struct sk_buff *skb)
+{
+ return true;
+}
#endif /* CONFIG_SECURITY_NETWORK */
/*
@@ -1414,6 +1424,7 @@ static int unix_scm_to_skb(struct scm_cookie *scm, struct
sk_buff *skb, bool sen
UNIXCB(skb).uid = scm->creds.uid;
UNIXCB(skb).gid = scm->creds.gid;
UNIXCB(skb).fp = NULL;
+ unix_get_secdata(scm, skb);
if (scm->fp && send_fds)
err = unix_attach_fds(scm, skb);
@@ -1509,7 +1520,6 @@ static int unix_dgram_sendmsg(struct socket *sock, struct
msghdr *msg,
if (err < 0)
goto out_free;
max_level = err + 1;
- unix_get_secdata(&scm, skb);
skb_put(skb, len - data_len);
skb->data_len = data_len;
@@ -2118,11 +2128,13 @@ unlock:
/* Never glue messages from different writers */
if ((UNIXCB(skb).pid != scm.pid) ||
!uid_eq(UNIXCB(skb).uid, scm.creds.uid) ||
- !gid_eq(UNIXCB(skb).gid, scm.creds.gid))
+ !gid_eq(UNIXCB(skb).gid, scm.creds.gid) ||
+ !unix_secdata_eq(&scm, skb))
break;
} else if (test_bit(SOCK_PASSCRED, &sock->flags)) {
/* Copy credentials */
scm_set_cred(&scm, UNIXCB(skb).pid, UNIXCB(skb).uid,
UNIXCB(skb).gid);
+ unix_set_secdata(&scm, skb);
check_creds = true;
}
--
2.1.0
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [PATCH 3/3] mlsxfrm: Various fixes
On Wed, 2006-11-08 at 13:17 -0600, Venkat Yekkirala wrote: > > > > Not sure > > > > though when > > > > that would apply here, > > > > > > It could apply to xfrms if they happen to be using the context > > > represented by any of the initial SIDs. > > > > Which would happen when? > > If one were attempting to use a context pertaining to the unlabeled init > sid in the SPD and/or the SAD. But would I be correct in assuming that the > same sid (unlabeled init sid in all likelyhood) would end up being returned > when the context is turned into a sid, resulting in the SPD and the SAD > using > the same init sid, thus making a full-context compare unnecessary? Yes. > > What's the harm from just using the SID comparison and > > allowing for the > > possibility that there might be a few duplicates in rare > > circumstances? > > Does it break any assumptions in the rest of the logic? > > The best I can think of is if the SA's sid doesn't match the > socket's SID, IKE would come into play, if it's configured. > > I also wanted to conversely ask what harm exists if we did > a full-context compare in the event the sids didn't match? > > Are we just trying to generally avoid extra code? More complexity and overhead for no real gain. -- Stephen Smalley National Security Agency - To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [PATCH 3/3] mlsxfrm: Various fixes
On Wed, 2006-11-08 at 08:34 -0600, Venkat Yekkirala wrote: > > Such duplication can occur among the initial SIDs. > > For some reason I thought that could happen between an initial SID > and a non-initial SID. I think only in the case where a non-initial SID has been invalidated by policy reload that renders its context illegal and is thus remapped to the unlabeled SID. > > Not sure > > though when > > that would apply here, > > It could apply to xfrms if they happen to be using the context > represented by any of the initial SIDs. Which would happen when? > > and it would only apply if both SIDs > > were initial > > SIDs. > > OK. Will narrow the full context comparison to just this case. What's the harm from just using the SID comparison and allowing for the possibility that there might be a few duplicates in rare circumstances? Does it break any assumptions in the rest of the logic? -- Stephen Smalley National Security Agency - To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 3/3] mlsxfrm: Various fixes
On Tue, 2006-11-07 at 15:29 -0500, Paul Moore wrote:
> Venkat Yekkirala wrote:
> > +/*
> > + * security_sid_compare() - compares two given sid contexts.
> > + * Returns 1 if they are equal, 0 otherwise.
> > + */
> > +int security_sid_compare(u32 sid1, u32 sid2)
> > +{
> > + struct context *context1;
> > + struct context *context2;
> > + int rc;
> > +
> > + if (!ss_initialized)
> > + return 1;
> > +
> > + if (sid1 == sid2)
> > + return 1;
> > + else if (sid1 > SECINITSID_NUM && sid2 > SECINITSID_NUM)
> > + return 0;
> > +
> > + /* explicit comparison in order */
> > +
> > + POLICY_RDLOCK;
> > + context1 = sidtab_search(&sidtab, sid1);
> > + if (!context1) {
> > + printk(KERN_ERR "security_sid_compare: unrecognized SID "
> > + "%u\n", sid1);
> > + rc = 0;
> > + goto out_unlock;
> > + }
> > +
> > + context2 = sidtab_search(&sidtab, sid2);
> > + if (!context2) {
> > + printk(KERN_ERR "security_sid_compare: unrecognized SID "
> > + "%u\n", sid2);
> > + rc = 0;
> > + goto out_unlock;
> > + }
> > +
> > + rc = context_cmp(context1, context2);
> > +
> > +out_unlock:
> > + POLICY_RDUNLOCK;
> > + return rc;
> > +}
>
> I understand wanting a generic LSM interface to do secid token comparisons,
> but
> in the SELinux implementation of this function I think we can get away with
> only
> a simple "sid1 == sid2" since the security server shouldn't be creating
> duplicate SID/secid values for identical contexts, I think. Did you run into
> something in testing that would indicate otherwise?
Such duplication can occur among the initial SIDs. Not sure though when
that would apply here, and it would only apply if both SIDs were initial
SIDs.
--
Stephen Smalley
National Security Agency
-
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v4 1/2] NetLabel: secid reconciliation support
On Wed, 2006-10-04 at 15:27 -0400, Paul Moore wrote: > Venkat Yekkirala wrote: > >> * XFRM present > >> > >> xfrm_sid = > >> loc_sid = SECINITSID_NETMSG > >> nlbl_sid = SECSID_NULL/0 > >> ext_sid = xfrm_sid > >> final skb->secmark = avc_ok : ext_sid ? unchanged > >> > >> * NetLabel present > >> > >> xfrm_sid = SECSID_NULL/0 > >> loc_sid = SECSID_NULL/0 > >> nlbl_sid = > >> ext_sid = nlbl_sid > >> final skb->secmark = avc_ok : ext_sid ? unchanged > > > > I was referring to the differences in what getpeercon would > > return in the above 2 scenarios. > > > > In the xfrm case: final skb->secmark would be 0 resulting in getpeercon > > to return EPROTONOOPT > > In the "XFRM present" case above if the access is allowed (avc_ok is > true) then the final skb->secmark value is going to be set to the value > of ext_sid which is the xfrm_sid. Any calls made to getpeercon() would > return success with the context matching xfrm_sid. > > I have a hunch we are still on different pages here ... > > > In the NetLabel case: final skb->secmark would be network_t resulting in > > getpeercon to return network_t > > Yep, and I understand you would like to see it as unlabeled_t. I think > we both have valid arguments for either case and we are just waiting to > hear from others. I don't understand the argument for network_t, and it seems to violate our goals of 1) having consistent policy regardless of network labeling mechanism, and 2) having getpeercon() always return a subject label that can be used as a basis for avc_has_perm() and setexeccon() calls. -- Stephen Smalley National Security Agency - To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 1/1] NetLabel: secid reconciliation support
On Mon, 2006-10-02 at 14:06 -0400, [EMAIL PROTECTED] wrote: > plain text document attachment (netlabel-secid_support) > This patch provides the missing NetLabel support to the secid reconciliation > patchset. > > Signed-off-by: Paul Moore <[EMAIL PROTECTED]> > --- > security/selinux/hooks.c| 67 +++-- > security/selinux/include/objsec.h |1 > security/selinux/include/selinux_netlabel.h | 28 +++ > security/selinux/ss/services.c | 106 > ++-- > 4 files changed, 98 insertions(+), 104 deletions(-) > @@ -3725,7 +3723,16 @@ static int selinux_skb_flow_in(struct sk > if (xfrm_sid) > skb->secmark = xfrm_sid; > > - /* See if NetLabel can flow in thru the current secmark here */ > + err = selinux_netlbl_skb_sid(skb, skb->secmark, &nlbl_sid); > + BUG_ON(err); If this selinux_netlbl_skb_sid() call can fail for any reason other than a kernel bug, then this needs to goto out instead of using BUG_ON. For example, if the function can fail due to temporary memory pressure leading to a failed allocation, then you want to simply drop the packet, not panic the kernel. > + > + err = avc_has_perm(nlbl_sid, skb->secmark, SECCLASS_PACKET, > + PACKET__FLOW_IN, NULL); This means we end up with two flow_in checks each time, even if only one or none of the two labeling mechanisms was used, right? Given the conclusion on the discussion of what it means to use them together (just redundant), this seems to be pointless overhead. > @@ -3749,6 +3757,12 @@ static int selinux_skb_flow_out(struct s > struct sk_security_struct *sksec = skb->sk->sk_security; > skb->secmark = sksec->sid; > } > + > + err = selinux_netlbl_skb_sid(skb, skb->secmark, &nlbl_sid); > + BUG_ON(err); Same concern about BUG_ON as above. Also, I'd have expected this to happen at the same point that selinux_skb_xfrm_sid() is called. > + if (nlbl_sid) > + skb->secmark = nlbl_sid; And then I'd expect this to be moved up prior to the if (xfrm_sid) clause, turning that into an else clause, e.g.: if (nlbl_sid) skb->secmark = nlbl_sid; else if (xfrm_sid) skb->secmark = xfrm_sid; else if (skb->sk) ... > @@ -3913,9 +3928,15 @@ static unsigned int selinux_ip_postroute > skb->sk->sk_security; > skb->secmark = sksec->sid; > } > + > + err = selinux_netlbl_skb_sid(skb, > + skb->secmark, > + &nlbl_sid); > + BUG_ON(err); > + > + if (nlbl_sid) > + skb->secmark = nlbl_sid; Similar comments as above. > } > - err = avc_has_perm(skb->secmark, SECINITSID_UNLABELED, > -SECCLASS_PACKET, PACKET__FLOW_OUT, &ad); Why do you think you can remove this? -- Stephen Smalley National Security Agency - To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/9] secid reconciliation-v04: Add LSM hooks
On Sun, 2006-10-01 at 16:26 -0500, Venkat Yekkirala wrote:
> Add skb_policy_check and skb_netfilter_check hooks to LSM to enable
> reconciliation of the various security identifiers as well as enforce
> flow control on inbound (PREROUTING/INPUT) and outbound
> (OUTPUT/FORWARD/POSTROUTING)
> traffic.
>
> Signed-off-by: Venkat Yekkirala <[EMAIL PROTECTED]>
> ---
> include/linux/security.h | 41 -
> security/dummy.c | 13 +++
> 2 files changed, 53 insertions(+), 1 deletion(-)
> @@ -3150,7 +3185,11 @@ static inline int security_xfrm_state_al
> {
> if (!polsec)
> return 0;
> - return security_ops->xfrm_state_alloc_security(x, NULL, polsec, secid);
> + /*
> + * No need to pass polsec along since we want the context to be
> + * taken from secid which is usually from the sock.
> + */
> + return security_ops->xfrm_state_alloc_security(x, NULL, NULL, secid);
> }
As a follow-up patch, you could then drop polsec from the hook interface
in security_ops (but not the static inline function interface), and from
the underlying selinux functions. That would simplify
selinux_xfrm_sec_ctx_alloc() a bit and make the logic clearer.
--
Stephen Smalley
National Security Agency
-
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 7/9] secid reconciliation-v04: Enforcement for SELinux
On Mon, 2006-10-02 at 12:12 -0400, Paul Moore wrote:
> Venkat Yekkirala wrote:
> > This defines SELinux enforcement of the 2 new LSM hooks as well
> > as related changes elsewhere in the SELinux code.
> >
> > This also now keeps track of the peersid thru the establishment
> > of a connection on the server (tracking peersid on the client
> > is covered later in this patch set).
> >
> > Signed-off-by: Venkat Yekkirala <[EMAIL PROTECTED]>
> >
> > {snip}
> >
> > +static int selinux_skb_flow_in(struct sk_buff *skb, unsigned short family)
> > +{
> > + u32 xfrm_sid;
> > + int err;
> > +
> > + if (selinux_compat_net)
> > + return 1;
> > +
> > + /*
> > +* loopback traffic already labeled and
> > +* flow-controlled on outbound. We may
> > +* need to flow-control on the inbound
> > +* as well if there's ever a use-case for it.
> > +*/
> > + if (skb->dev == &loopback_dev)
> > + return 1;
> > +
> > + err = selinux_xfrm_decode_session(skb, &xfrm_sid, 0);
> > + BUG_ON(err);
>
> Just a quick question that has been nagging me for awhile - any
> particular reason why this is a BUG_ON() and not an "if (err) goto out;"?
It appears that selinux_xfrm_decode_session() can only legitimately
return an error if the last argument (ckall) is non-zero.
security_skb_classify_flow() was doing the same thing prior to this
patch series. It would be clearer if there were two separate interfaces
that internally use the same helper, with one of the functions returning
void.
> > + err = avc_has_perm(xfrm_sid, skb->secmark, SECCLASS_PACKET,
> > + PACKET__FLOW_IN, NULL);
> > + if (err)
> > + goto out;
> > +
> > + if (xfrm_sid)
> > + skb->secmark = xfrm_sid;
> > +
> > + /* See if NetLabel can flow in thru the current secmark here */
> > +
> > +out:
> > + return err ? 0 : 1;
> > +};
>
--
Stephen Smalley
National Security Agency
-
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 7/7] secid reconciliation-v03: Enforcement for SELinux
On Fri, 2006-09-29 at 10:33 -0400, James Morris wrote: > On Fri, 29 Sep 2006, Stephen Smalley wrote: > > > However, since the transition was removed in the flow_out case, it would > > be logical to remove it from the flow_in case as well, and that would > > have the side benefit of less overhead. > > How about adding secmark transitions later, if needed, perhaps with an > /selinux config control ? > > It does keep things simpler for now, in terms of getting this code merged, > deployed into distros and likely certified. Fine with me, unless Venkat has an immediate use case for such transitions in the flow_in case (but I think this is mostly my fault for suggesting transitions a while ago). -- Stephen Smalley National Security Agency - To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 7/7] secid reconciliation-v03: Enforcement for SELinux
On Fri, 2006-09-29 at 10:00 -0400, Joshua Brindle wrote:
> On Fri, 2006-09-29 at 08:59 -0400, Stephen Smalley wrote:
> > On Thu, 2006-09-28 at 23:52 -0400, Joshua Brindle wrote:
> > > Venkat Yekkirala wrote:
> > > >
> > > > +
> > > > + err = avc_has_perm(xfrm_sid, skb->secmark, SECCLASS_PACKET,
> > > > + PACKET__FLOW_IN, NULL);
> > > > + if (err)
> > > > + goto out;
> > > > +
> > > > + if (xfrm_sid) {
> > > > + err = security_transition_sid(xfrm_sid, skb->secmark,
> > > > + SECCLASS_PACKET,
> > > > &trans_sid);
> > > > + if (err)
> > > > + goto out;
> > > > +
> > > >
> > > I thought we weren't doing transitions to label packets anymore per the
> > > conference call?
> >
> > No, transitions are still part of the reconciliation process. By
> > default, this just means that we end up with the xfrm_sid (which is what
> > you want). But it allows us the freedom to define transitions on the
> > secmark label if desired, and those transitions can still yield subject
> > labels.
> >
>
> This is not consistent with my perception of the decision made in the
> conference call. I thought that the secid was either going to be 1) the
> secmark label if no external labeling is present or 2) the external
> label if it is present. The flow_in permission would be checked between
> the external label and the secmark label in either case (unlabeled in
> the case of #1)
The behavior you describe is precisely what will happen in the absence
of any type_transition rules on packet class in the policy, given the
way in which he has defined security_transition_sid on packet class.
The only question is whether there is any value in allowing a transition
to be configured in policy (and if such a transition is configured,
whether the resulting SID requires its own separate permission check,
which is what is usually done - the transition doesn't implicitly
authorize anything). I don't recall a particular decision on the
transition issue during the call nor do I see it in the posted notes.
However, since the transition was removed in the flow_out case, it would
be logical to remove it from the flow_in case as well, and that would
have the side benefit of less overhead.
--
Stephen Smalley
National Security Agency
-
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 7/7] secid reconciliation-v03: Enforcement for SELinux
On Thu, 2006-09-28 at 23:52 -0400, Joshua Brindle wrote:
> Venkat Yekkirala wrote:
> >
> > +
> > + err = avc_has_perm(xfrm_sid, skb->secmark, SECCLASS_PACKET,
> > + PACKET__FLOW_IN, NULL);
> > + if (err)
> > + goto out;
> > +
> > + if (xfrm_sid) {
> > + err = security_transition_sid(xfrm_sid, skb->secmark,
> > + SECCLASS_PACKET, &trans_sid);
> > + if (err)
> > + goto out;
> > +
> >
> I thought we weren't doing transitions to label packets anymore per the
> conference call?
No, transitions are still part of the reconciliation process. By
default, this just means that we end up with the xfrm_sid (which is what
you want). But it allows us the freedom to define transitions on the
secmark label if desired, and those transitions can still yield subject
labels.
--
Stephen Smalley
National Security Agency
-
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [Patch] kernel memory leak fix for af_unix datagram getpeersec patch
On Wed, 2006-08-02 at 02:47 -0400, Catherine Zhang wrote:
> Hi, all,
>
> Enclosed please find the updated patch incorporating comments from
> Stephen and Dave.
Note that this patch is intended for 2.6.18 as a bug fix for the memory
leak introduced by the original dgram peersec patches.
> Again thanks for your help!
> Catherine
>
> --
>
>
> From: [EMAIL PROTECTED]
>
> This patch implements a cleaner fix for the memory leak problem of the
> original
> unix datagram getpeersec patch. Instead of creating a security context each
> time a unix datagram is sent, we only create the security context when the
> receiver requests it.
>
> This new design requires modification of the current unix_getsecpeer_dgram
> LSM hook and addition of two new hooks, namely, secid_to_secctx and
> release_secctx. The former retrieves the security context and the latter
> releases it. A hook is required for releasing the security context because
> it is up to the security module to decide how that's done. In the case of
> Selinux, it's a simple kfree operation.
Acked-by: Stephen Smalley <[EMAIL PROTECTED]>
>
>
> ---
>
> include/linux/security.h | 41 +++--
> include/net/af_unix.h|6 ++
> include/net/scm.h| 29 +
> net/ipv4/ip_sockglue.c |9 +++--
> net/unix/af_unix.c | 17 +
> security/dummy.c | 14 --
> security/selinux/hooks.c | 38 --
> 7 files changed, 110 insertions(+), 44 deletions(-)
>
> diff -puN include/net/scm.h~af_unix-datagram-getpeersec-ml-fix
> include/net/scm.h
> --- linux-2.6.18-rc2/include/net/scm.h~af_unix-datagram-getpeersec-ml-fix
> 2006-07-22 21:28:21.0 -0400
> +++ linux-2.6.18-rc2-cxzhang/include/net/scm.h2006-08-01
> 22:43:50.0 -0400
> @@ -3,6 +3,7 @@
>
> #include
> #include
> +#include
>
> /* Well, we should have at least one descriptor open
> * to accept passed FDs 8)
> @@ -20,8 +21,7 @@ struct scm_cookie
> struct ucredcreds; /* Skb credentials */
> struct scm_fp_list *fp;/* Passed files */
> #ifdef CONFIG_SECURITY_NETWORK
> - char*secdata; /* Security context */
> - u32 seclen; /* Security length */
> + u32 secid; /* Passed security ID */
> #endif
> unsigned long seq;/* Connection seqno */
> };
> @@ -32,6 +32,16 @@ extern int __scm_send(struct socket *soc
> extern void __scm_destroy(struct scm_cookie *scm);
> extern struct scm_fp_list * scm_fp_dup(struct scm_fp_list *fpl);
>
> +#ifdef CONFIG_SECURITY_NETWORK
> +static __inline__ void unix_get_peersec_dgram(struct socket *sock, struct
> scm_cookie *scm)
> +{
> + security_socket_getpeersec_dgram(sock, NULL, &scm->secid);
> +}
> +#else
> +static __inline__ void unix_get_peersec_dgram(struct socket *sock, struct
> scm_cookie *scm)
> +{ }
> +#endif /* CONFIG_SECURITY_NETWORK */
> +
> static __inline__ void scm_destroy(struct scm_cookie *scm)
> {
> if (scm && scm->fp)
> @@ -47,6 +57,7 @@ static __inline__ int scm_send(struct so
> scm->creds.pid = p->tgid;
> scm->fp = NULL;
> scm->seq = 0;
> + unix_get_peersec_dgram(sock, scm);
> if (msg->msg_controllen <= 0)
> return 0;
> return __scm_send(sock, msg, scm);
> @@ -55,8 +66,18 @@ static __inline__ int scm_send(struct so
> #ifdef CONFIG_SECURITY_NETWORK
> static inline void scm_passec(struct socket *sock, struct msghdr *msg,
> struct scm_cookie *scm)
> {
> - if (test_bit(SOCK_PASSSEC, &sock->flags) && scm->secdata != NULL)
> - put_cmsg(msg, SOL_SOCKET, SCM_SECURITY, scm->seclen,
> scm->secdata);
> + char *secdata;
> + u32 seclen;
> + int err;
> +
> + if (test_bit(SOCK_PASSSEC, &sock->flags)) {
> + err = security_secid_to_secctx(scm->secid, &secdata, &seclen);
> +
> + if (!err) {
> + put_cmsg(msg, SOL_SOCKET, SCM_SECURITY, seclen,
> secdata);
> + security_release_secctx(secdata, seclen);
> + }
> + }
> }
> #else
> static inline void scm_passec(struct socket *sock, struct msghdr *msg,
> struct scm_cookie *scm)
> diff -puN net/unix/af_unix.c~af_unix-datagram-getpeersec-ml-fix
> net/unix/af_unix.c
> --- linux-2.6.18-rc2/net/unix/af_unix.c~af_unix-datagram-getpe
Re: RFC: kernel memory leak fix for af_unix datagram getpeersec
On Wed, 2006-07-26 at 16:19 -0400, Catherine Zhang wrote: > diff -puN include/net/scm.h~af_unix-datagram-getpeersec-ml-fix > include/net/scm.h > --- linux-2.6.18-rc2/include/net/scm.h~af_unix-datagram-getpeersec-ml-fix > 2006-07-22 21:28:21.0 -0400 > +++ linux-2.6.18-rc2-cxzhang/include/net/scm.h2006-07-24 > 11:19:54.0 -0400 > @@ -3,6 +3,7 @@ > > #include > #include > +#include > > /* Well, we should have at least one descriptor open > * to accept passed FDs 8) > @@ -20,8 +21,7 @@ struct scm_cookie > struct ucredcreds; /* Skb credentials */ > struct scm_fp_list *fp;/* Passed files */ > #ifdef CONFIG_SECURITY_NETWORK > - char*secdata; /* Security context */ > - u32 seclen; /* Security length */ > + u32 sid;/* Passed security ID */ I think that "secid" is what has been chosen for security identifiers outside of the core SELinux code to to avoid confusion with session identifiers. Lingering references to sid or ctxid are going to be converted to secid. > diff -puN net/unix/af_unix.c~af_unix-datagram-getpeersec-ml-fix > net/unix/af_unix.c > --- linux-2.6.18-rc2/net/unix/af_unix.c~af_unix-datagram-getpeersec-ml-fix > 2006-07-22 23:01:26.0 -0400 > +++ linux-2.6.18-rc2-cxzhang/net/unix/af_unix.c 2006-07-22 > 23:14:15.0 -0400 > @@ -1323,8 +1299,9 @@ static int unix_dgram_sendmsg(struct kio > memcpy(UNIXCREDS(skb), &siocb->scm->creds, sizeof(struct ucred)); > if (siocb->scm->fp) > unix_attach_fds(siocb->scm, skb); > - > - unix_get_peersec_dgram(skb); > +#ifdef CONFIG_SECURITY_NETWORK > + memcpy(UNIXSID(skb), &siocb->scm->sid, sizeof(u32)); > +#endif /* CONFIG_SECURITY_NETWORK */ You want to retain the static inlines, and just update their contents, not replace them with embedded #ifdefs. And this could be a direct assignment, right? -- Stephen Smalley National Security Agency - To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 10/10] MLSXFRM: Auto-labeling of child sockets
On Wed, 2006-07-12 at 16:15 -0500, Venkat Yekkirala wrote:
> This automatically labels the TCP, Unix stream, and dccp child sockets
> as well as openreqs to be at the same MLS level as the peer.
>
> Signed-off-by: Venkat Yekkirala <[EMAIL PROTECTED]>
> ---
> include/linux/security.h| 43 ++
> include/net/request_sock.h |1
> include/net/sock.h |1
> net/dccp/ipv4.c |3 +
> net/dccp/ipv6.c |7 +++-
> net/ipv4/inet_connection_sock.c |4 +-
> net/ipv4/syncookies.c |6 +++
> net/ipv4/tcp_ipv4.c |3 +
> net/ipv6/tcp_ipv6.c |6 ++-
> security/dummy.c| 18 +++
> security/selinux/hooks.c| 49 +-
> security/selinux/xfrm.c |1
> 12 files changed, 134 insertions(+), 8 deletions(-)
>
> --- linux-2.6.17.sk_policy/security/selinux/hooks.c 2006-07-12
> 09:18:59.0 -0500
> +++ linux-2.6.17/security/selinux/hooks.c 2006-07-12 14:55:16.0
> -0500
> @@ -3324,7 +3324,12 @@ static int selinux_socket_unix_stream_co
> /* server child socket */
> ssec = newsk->sk_security;
> ssec->peer_sid = isec->sid;
> -
> + err = security_sid_mls_copy(other_isec->sid, ssec->peer_sid,
> &ssec->sid);
> + if (err) {
> + printk(KERN_ERR "ERROR: security_sid_mls_copy failed.");
Drop the printk please. It isn't precisely a useful error message
anyway. If you need to audit such failures, then do it within the
function and use audit_log, and make the message useful.
> +int selinux_inet_conn_request(struct sock *sk, struct sk_buff *skb,
> +struct request_sock *req)
> +{
> + struct sk_security_struct *sksec = sk->sk_security;
> + int err;
> + u32 newsid = 0;
> + u32 peersid;
> +
> + BUG_ON(selinux_xfrm_decode_session(skb, &peersid, 0));
Doesn't seem suitable for a BUG_ON.
> +
> + err = security_sid_mls_copy(sksec->sid, peersid, &newsid);
> + if (err) {
> + printk(KERN_ERR "ERROR: security_sid_mls_copy failed.");
Drop the printk.
--
Stephen Smalley
National Security Agency
-
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 03/10] MLSXFRM: Add security sid to sock
On Wed, 2006-07-12 at 16:12 -0500, Venkat Yekkirala wrote:
> This adds security for IP sockets at the sock level. Security at the
> sock level is needed to enforce the SELinux security policy for security
> associations even when a sock is orphaned (such as in the TCP LAST_ACK state).
>
> Signed-off-by: Venkat Yekkirala <[EMAIL PROTECTED]>
> ---
>
> include/linux/security.h | 12
> include/net/sock.h| 13 +
> net/core/sock.c |2 +-
> security/dummy.c |5 +
> security/selinux/hooks.c | 27 +--
> security/selinux/include/objsec.h |1 +
> 6 files changed, 53 insertions(+), 7 deletions(-)
>
> @@ -3564,6 +3574,10 @@ static unsigned int selinux_sk_getsid_se
>
> if (isec)
> sock_sid = isec->sid;
> + else {
> + struct sk_security_struct *sksec = sk->sk_security;
> + sock_sid = sksec->sid;
> + }
>
> read_unlock_bh(&sk->sk_callback_lock);
> return sock_sid;
Is it ever possible for the isec->sid and the sksec->sid to be
inconsistent with one another? Could you just always return the
sksec->sid here and avoid the need to grab the isec altogether (dropping
the requirement for sk_callback_lock at the same time, since you no
longer need sk_socket)?
Likewise, given sksec->sid, why don't you change sock_rcv_skb to always
use it, and eliminate the need for the isec and the sk_callback_lock
there? Similarly for postroute_last's use of isec->sid. With direct
labeling of the sock, it is no longer necessary to extract the isec.
--
Stephen Smalley
National Security Agency
-
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [Patch 1/1] AF_UNIX Datagram getpeersec [Updated #2]
On Tue, 2006-06-27 at 03:16 -0400, James Morris wrote:
> On Tue, 27 Jun 2006, James Morris wrote:
>
> > I'll address that in a patch to follow. Could you please test these
> > updated patches? Thanks.
> >
>
>
> Below is a relative patch which only compiles this stuff into the core
> networking code when appropriate kernel config is selected.
>
> Please review.
>
>
> ---
>
> diff -purN -X dontdiff linux-2.6.p/include/net/scm.h
> linux-2.6.w/include/net/scm.h
> --- linux-2.6.p/include/net/scm.h 2006-06-27 02:26:02.0 -0400
> +++ linux-2.6.w/include/net/scm.h 2006-06-27 03:03:30.0 -0400
> @@ -19,7 +19,9 @@ struct scm_cookie
> {
> struct ucredcreds; /* Skb credentials */
> struct scm_fp_list *fp;/* Passed files */
> +#ifdef CONFIG_SECURITY_NETWORK
> char*secdata; /* Security context */
What about saving the u32 seclen with the secdata, and using it later
rather than recomputing strlen(secdata)? That also avoids encoding an
assumption in the af_unix code about the content of the data (i.e.
NUL-terminated string), leaving that to the security module.
> +#endif
> unsigned long seq;/* Connection seqno */
> };
>
> @@ -49,6 +51,17 @@ static __inline__ int scm_send(struct so
> return __scm_send(sock, msg, scm);
> }
>
> +#ifdef CONFIG_SECURITY_NETWORK
> +static inline void scm_passec(struct socket *sock, struct msghdr *msg,
> struct scm_cookie *scm)
> +{
> + if (test_bit(SOCK_PASSSEC, &sock->flags) && scm->secdata != NULL)
> + put_cmsg(msg, SOL_SOCKET, SCM_SECURITY, strlen(scm->secdata)+1,
> scm->secdata);
> +}
It would be cleaner if we had a scm->seclen to use above.
> +#ifdef CONFIG_SECURITY_NETWORKING
> +static void unix_get_peersec_dgram(struct sk_buff *skb)
> +{
> + int tmp = 0;
> +
> + err = security_socket_getpeersec_dgram(skb, UNIXSEC(skb), &tmp);
tmp should actually be u32 not int (also wrong in the original patch),
and it seems odd to throw it away rather than saving it and using it for
the put_cmsg.
--
Stephen Smalley
National Security Agency
-
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
[patch 1/1] netlink: encapsulate eff_cap usage within security framework
From: Darrel Goeddel <[EMAIL PROTECTED]>
This patch encapsulates the usage of eff_cap (in netlink_skb_params) within
the security framework by extending security_netlink_recv to include a required
capability parameter and converting all direct usage of eff_caps outside
of the lsm modules to use the interface. It also updates the SELinux
implementation of the security_netlink_send and security_netlink_recv
hooks to take advantage of the sid in the netlink_skb_params struct.
This also enables SELinux to perform auditing of netlink capability checks.
Please apply, for 2.6.18 if possible.
Signed-off-by: Darrel Goeddel <[EMAIL PROTECTED]>
Signed-off-by: Stephen Smalley <[EMAIL PROTECTED]>
Acked-by: James Morris <[EMAIL PROTECTED]>
---
include/linux/security.h| 13 +++--
kernel/audit.c |8
net/core/rtnetlink.c|2 +-
net/decnet/netfilter/dn_rtmsg.c |2 +-
net/ipv4/netfilter/ip_queue.c |2 +-
net/ipv6/netfilter/ip6_queue.c |2 +-
net/netfilter/nfnetlink.c |2 +-
net/netlink/genetlink.c |2 +-
net/xfrm/xfrm_user.c|2 +-
security/commoncap.c|4 ++--
security/dummy.c|4 ++--
security/selinux/hooks.c| 26 +-
12 files changed, 35 insertions(+), 34 deletions(-)
diff -X /home/sds/dontdiff -rup linux-2.6.17-git10/include/linux/security.h
linux-2.6.17-git10-ss2/include/linux/security.h
--- linux-2.6.17-git10/include/linux/security.h 2006-06-26 09:34:30.0
-0400
+++ linux-2.6.17-git10-ss2/include/linux/security.h 2006-06-26
09:50:59.0 -0400
@@ -67,7 +67,7 @@ struct xfrm_state;
struct xfrm_user_sec_ctx;
extern int cap_netlink_send(struct sock *sk, struct sk_buff *skb);
-extern int cap_netlink_recv(struct sk_buff *skb);
+extern int cap_netlink_recv(struct sk_buff *skb, int cap);
/*
* Values used in the task_security_ops calls
@@ -656,6 +656,7 @@ struct swap_info_struct;
* Check permission before processing the received netlink message in
* @skb.
* @skb contains the sk_buff structure for the netlink message.
+ * @cap indicates the capability required
* Return 0 if permission is granted.
*
* Security hooks for Unix domain networking.
@@ -1265,7 +1266,7 @@ struct security_operations {
struct sembuf * sops, unsigned nsops, int alter);
int (*netlink_send) (struct sock * sk, struct sk_buff * skb);
- int (*netlink_recv) (struct sk_buff * skb);
+ int (*netlink_recv) (struct sk_buff * skb, int cap);
/* allow module stacking */
int (*register_security) (const char *name,
@@ -2031,9 +2032,9 @@ static inline int security_netlink_send(
return security_ops->netlink_send(sk, skb);
}
-static inline int security_netlink_recv(struct sk_buff * skb)
+static inline int security_netlink_recv(struct sk_buff * skb, int cap)
{
- return security_ops->netlink_recv(skb);
+ return security_ops->netlink_recv(skb, cap);
}
/* prototypes */
@@ -2669,9 +2670,9 @@ static inline int security_netlink_send
return cap_netlink_send (sk, skb);
}
-static inline int security_netlink_recv (struct sk_buff *skb)
+static inline int security_netlink_recv (struct sk_buff *skb, int cap)
{
- return cap_netlink_recv (skb);
+ return cap_netlink_recv (skb, cap);
}
static inline struct dentry *securityfs_create_dir(const char *name,
diff -X /home/sds/dontdiff -rup linux-2.6.17-git10/kernel/audit.c
linux-2.6.17-git10-ss2/kernel/audit.c
--- linux-2.6.17-git10/kernel/audit.c 2006-06-26 09:34:30.0 -0400
+++ linux-2.6.17-git10-ss2/kernel/audit.c 2006-06-26 09:50:59.0
-0400
@@ -445,7 +445,7 @@ void audit_send_reply(int pid, int seq,
* Check for appropriate CAP_AUDIT_ capabilities on incoming audit
* control messages.
*/
-static int audit_netlink_ok(kernel_cap_t eff_cap, u16 msg_type)
+static int audit_netlink_ok(struct sk_buff *skb, u16 msg_type)
{
int err = 0;
@@ -459,13 +459,13 @@ static int audit_netlink_ok(kernel_cap_t
case AUDIT_DEL:
case AUDIT_DEL_RULE:
case AUDIT_SIGNAL_INFO:
- if (!cap_raised(eff_cap, CAP_AUDIT_CONTROL))
+ if (security_netlink_recv(skb, CAP_AUDIT_CONTROL))
err = -EPERM;
break;
case AUDIT_USER:
case AUDIT_FIRST_USER_MSG...AUDIT_LAST_USER_MSG:
case AUDIT_FIRST_USER_MSG2...AUDIT_LAST_USER_MSG2:
- if (!cap_raised(eff_cap, CAP_AUDIT_WRITE))
+ if (security_netlink_recv(skb, CAP_AUDIT_WRITE))
err = -EPERM;
break;
default: /* bad msg */
@@ -488,7 +488,7 @@ static int audit_receive_msg(struct sk_b
char*ctx;
u32 len;
- err = audit_netlink_ok(NETLINK_CB(skb
Re: [RFC] SECMARK 1.0
On Tue, 2006-05-09 at 12:40 -0400, James Morris wrote: > On Tue, 9 May 2006, Karl MacMillan wrote: > > > those connection, but my concern is that connection could, through error > > or exploit, be passed to another domain that should not receive packets > > from that type of connection (see below). > > Connection passing or inheritence should be subject to kernel MAC controls > anyway (also see below). > > > The use of a single related packet type loses the strong binding between > > the connection type (determined on connection) and domains, most likely > > because an established connection is passed to another process. > > > > For example, for xinetd to work all of the xinetd children would be > > allowed to receive all related packets (i.e., tracked_packet_t). This > > means that if xinetd incorrectly passed, say, an ftp connection to > > telnet it would still be allowed to receive those packets because they > > would be of type tracked_packet_t. Labeling using something like > > connmark seems to solve this problem. > > My understanding of xinetd is that it execs server applications, which > inherit the connection fd. In this case, flush_unauthorized_files() will > ensure that the new domain is authorized to access the fd. > > Stephen, can you confirm this? That doesn't help, as that is just a check based on the socket label, which will always be based on xinetd's label and won't reflect anything about the individual connection. -- Stephen Smalley National Security Agency - To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: updated [Patch 1/1] AF_UNIX Datagram getpeersec
On Fri, 2006-04-07 at 19:30 -0400, Catherine Zhang wrote: > Hi, James, Stephen, Dave and Chris, > > Enclosed please find the updated AF_UNIX patch. It addressed three major > issues in the previous patch. > > 1. No directly calling of the SELINUX function security_sid_to_context(). >The fix is to export this and other similar functions through >wrapper functions in selinux/exports.c. Most of this code is copied >from James' outstanding patch: > > http://people.redhat.com/jmorris/selinux/skfilter/kernel/12-skfilter-selinux-exports.patch This will ultimately collide with the ongoing audit work to introduce similar SELinux in-kernel interfaces for audit-by-context, netlink sender audit, and audit collection of SIDs rather than contexts to avoid the significant performance penalty associated with context generation on every operation. Hence, you need to look to the patches on linux-audit or viro's audit-current git tree (lspp.b6 or possibly newer) to ensure consistency with the interfaces that they will be introducing there, particularly since that work would likely be going in during the same time frame as your work (i.e. for 2.6.18). -- Stephen Smalley National Security Agency - To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] scm: fold __scm_send() into scm_send()
On Tue, 2006-03-21 at 08:32 -0500, Stephen Smalley wrote: > > I don't expect security_sk_sid() to be terribly expensive. It's not > > an AVC check, it's just propagating a label. But I've not done any > > benchmarking on that. > > No permission check there, but it looks like it does read lock > sk_callback_lock. Not sure if that is truly justified here. Ah, that is because it is also called from the xfrm code, introduced by Trent's patches. But that locking shouldn't be necessary from scm_send, right? So she likely wants a separate hook for it to avoid that overhead, or even just a direct SELinux interface? -- Stephen Smalley National Security Agency - To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] scm: fold __scm_send() into scm_send()
On Mon, 2006-03-20 at 15:15 -0800, Chris Wright wrote: > * Andrew Morton ([EMAIL PROTECTED]) wrote: > > Chris Wright <[EMAIL PROTECTED]> wrote: > > > Catherine, the security_sid_to_context() is a raw SELinux function which > > > crept into core code and should not have been there. The fallout fixes > > > included conditionally exporting security_sid_to_context, and finally > > > scm_send/recv unlining. > > > > Yes. So we're OK up the uninlining, right? > > Yes, although sid_to_context is meant to be analog to the other > get_peersec calls, and should really be made a proper part of the > interface (can be done later, correctness is the issue at hand). Yes, Catherine was told that she shouldn't be directly exporting security_sid_to_context, and was allegedly working on a fix. Note however that the expected solution is not a LSM interface but a set of properly encapsulated interfaces exported directly from SELinux, based on the iptables context matching patches by James. The same style of interface is being put forth for the audit LSPP work. The indirection of LSM serves no purpose here, as these users are specifically looking for functionality provided only by SELinux. > I don't expect security_sk_sid() to be terribly expensive. It's not > an AVC check, it's just propagating a label. But I've not done any > benchmarking on that. No permission check there, but it looks like it does read lock sk_callback_lock. Not sure if that is truly justified here. -- Stephen Smalley National Security Agency - To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/1] Corrections to LSM-IPSec Nethooks
On Fri, 2006-01-06 at 11:09 -0500, Trent Jaeger wrote: > Forgot signoff -- see below. > > On Jan 6, 2006, at 10:48 AM, Trent Jaeger wrote: > > > Hi, > > > > This patch contains two corrections to the LSM-IPsec Nethooks patches > > previously applied. > > > > (1) free a security context on a failed insert via xfrm_user > > interface in xfrm_add_policy. Memory leak. > > > > (2) change the authorization of the allocation of a security context > > in a xfrm_policy or xfrm_state from both relabelfrom and relabelto > > to setcontext. > > > > This is intended to be a correction to the 2.6.16 tree. > > Signed-off-by: Trent Jaeger <[EMAIL PROTECTED]> Acked-by: Stephen Smalley <[EMAIL PROTECTED]> -- Stephen Smalley National Security Agency - To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
