[PATCH v2 2/3] selinux: fix address family in bind() and connect() to match address/port

[Alexey Kodanev]

Since sctp_bindx() and sctp_connectx() can have multiple addresses,
sk_family can differ from sa_family. Therefore, selinux_socket_bind()
and selinux_socket_connect_helper(), which process sockaddr structure
(address and port), should use the address family from that structure
too, and not from the socket one.

The initialization of the data for the audit record is moved above,
in selinux_socket_bind(), so that there is no duplicate changes and
code.

Fixes: d452930fd3b9 ("selinux: Add SCTP support")
Suggested-by: Paul Moore <***@paul-moore.com>
Signed-off-by: Alexey Kodanev <***@oracle.com>
---

v2: new patch in v2

security/selinux/hooks.c | 18 +++++++-----------
1 file changed, 7 insertions(+), 11 deletions(-)

diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
index 1ed7004..e7882e5a 100644
--- a/security/selinux/hooks.c
+++ b/security/selinux/hooks.c
@@ -4630,6 +4630,11 @@ static int selinux_socket_bind(struct socket *sock, struct sockaddr *address, in
goto err_af;
}

+ ad.type = LSM_AUDIT_DATA_NET;
+ ad.u.net = &net;
+ ad.u.net->sport = htons(snum);
+ ad.u.net->family = family_sa;
+
if (snum) {
int low, high;

@@ -4641,10 +4646,6 @@ static int selinux_socket_bind(struct socket *sock, struct sockaddr *address, in
snum, &sid);
if (err)
goto out;
- ad.type = LSM_AUDIT_DATA_NET;
- ad.u.net = &net;
- ad.u.net->sport = htons(snum);
- ad.u.net->family = family;
err = avc_has_perm(&selinux_state,
sksec->sid, sid,
sksec->sclass,
@@ -4676,15 +4677,10 @@ static int selinux_socket_bind(struct socket *sock, struct sockaddr *address, in
break;
}

- err = sel_netnode_sid(addrp, family, &sid);
+ err = sel_netnode_sid(addrp, family_sa, &sid);
if (err)
goto out;

- ad.type = LSM_AUDIT_DATA_NET;
- ad.u.net = &net;
- ad.u.net->sport = htons(snum);
- ad.u.net->family = family;
-
if (family_sa == AF_INET)
ad.u.net->v4info.saddr = addr4->sin_addr.s_addr;
else
@@ -4780,7 +4776,7 @@ static int selinux_socket_connect_helper(struct socket *sock,
ad.type = LSM_AUDIT_DATA_NET;
ad.u.net = &net;
ad.u.net->dport = htons(snum);
- ad.u.net->family = sk->sk_family;
+ ad.u.net->family = address->sa_family;
err = avc_has_perm(&selinux_state,
sksec->sid, sid, sksec->sclass, perm, &ad);
if (err)

[Alexey Kodanev]

Allow to pass the socket address structure with AF_UNSPEC family for
compatibility purposes. selinux_socket_bind() will further check it
for INADDR_ANY and selinux_socket_connect_helper() should return
EINVAL.

For a bad address family return EINVAL instead of AFNOSUPPORT error,
i.e. what is expected from SCTP protocol in such case.

Fixes: d452930fd3b9 ("selinux: Add SCTP support")
Suggested-by: Paul Moore <***@paul-moore.com>
Signed-off-by: Alexey Kodanev <***@oracle.com>
---

v2: new patch in v2

security/selinux/hooks.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
index e7882e5a..be5817d 100644
--- a/security/selinux/hooks.c
+++ b/security/selinux/hooks.c
@@ -5277,6 +5277,7 @@ static int selinux_sctp_bind_connect(struct sock *sk, int optname,
while (walk_size < addrlen) {
addr = addr_buf;
switch (addr->sa_family) {
+ case AF_UNSPEC:
case AF_INET:
len = sizeof(struct sockaddr_in);
break;
@@ -5284,7 +5285,7 @@ static int selinux_sctp_bind_connect(struct sock *sk, int optname,
len = sizeof(struct sockaddr_in6);
break;
default:
- return -EAFNOSUPPORT;
+ return -EINVAL;
}

err = -EINVAL;

[Richard Haines via Selinux]

Commit d452930fd3b9 ("selinux: Add SCTP support") breaks
compatibility
with the old programs that can pass sockaddr_in structure with
AF_UNSPEC
and INADDR_ANY to bind(). As a result, bind() returns EAFNOSUPPORT
error.
This 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.
Fixes: d452930fd3b9 ("selinux: Add SCTP support")
---
* return EINVAL for SCTP socket if sa_family is AF_UNSPEC and
address is not INADDR_ANY
* add new 'sa_family' variable so that it equals either AF_INET
or AF_INET6. Besides, it it will be used in the next patch that
fixes audit record.
security/selinux/hooks.c | 29 +++++++++++++++++++----------
1 file changed, 19 insertions(+), 10 deletions(-)
diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
index 4cafe6a..1ed7004 100644
--- a/security/selinux/hooks.c
+++ b/security/selinux/hooks.c
@@ -4576,6 +4576,7 @@ static int selinux_socket_post_create(struct
socket *sock, int family,
static int selinux_socket_bind(struct socket *sock, struct sockaddr
*address, int addrlen)
{
struct sock *sk = sock->sk;
+ struct sk_security_struct *sksec = sk->sk_security;
u16 family;
int err;
@@ -4587,11 +4588,11 @@ static int selinux_socket_bind(struct socket
*sock, struct sockaddr *address, in
family = sk->sk_family;
if (family == PF_INET || family == PF_INET6) {
char *addrp;
- struct sk_security_struct *sksec = sk->sk_security;
struct common_audit_data ad;
struct lsm_network_audit net = {0,};
struct sockaddr_in *addr4 = NULL;
struct sockaddr_in6 *addr6 = NULL;
+ u16 family_sa = address->sa_family;
unsigned short snum;
u32 sid, node_perm;
@@ -4601,11 +4602,20 @@ static int selinux_socket_bind(struct socket
*sock, struct sockaddr *address, in
* 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) {
if (addrlen < sizeof(struct sockaddr_in))
return -EINVAL;
addr4 = (struct sockaddr_in *)address;
+ if (family_sa == AF_UNSPEC) {
+ /* see __inet_bind(), we only want
to allow
+ * AF_UNSPEC if the address is
INADDR_ANY
+ */
+ if (addr4->sin_addr.s_addr !=
htonl(INADDR_ANY))
+ goto err_af;
+ family_sa = AF_INET;
+ }
snum = ntohs(addr4->sin_port);
addrp = (char *)&addr4->sin_addr.s_addr;
break;
@@ -4617,13 +4627,7 @@ static int selinux_socket_bind(struct socket
*sock, struct sockaddr *address, in
addrp = (char *)&addr6->sin6_addr.s6_addr;
break;
- /* Note that SCTP services expect -EINVAL,
whereas
- * others expect -EAFNOSUPPORT.
- */
- if (sksec->sclass == SECCLASS_SCTP_SOCKET)
- return -EINVAL;
- else
- return -EAFNOSUPPORT;
+ goto err_af;
}
if (snum) {
@@ -4681,7 +4685,7 @@ 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)
+ if (family_sa == AF_INET)
ad.u.net->v4info.saddr = addr4-

Post by Alexey Kodanev
sin_addr.s_addr;

else
ad.u.net->v6info.saddr = addr6->sin6_addr;
@@ -4694,6 +4698,11 @@ static int selinux_socket_bind(struct socket
*sock, struct sockaddr *address, in
}
return err;
+ /* Note that SCTP services expect -EINVAL, others
-EAFNOSUPPORT. */
+ if (sksec->sclass == SECCLASS_SCTP_SOCKET)
+ return -EINVAL;
+ return -EAFNOSUPPORT;
}
/* This supports connect(2) and SCTP connect services such as
sctp_connectx(3)

Tested all three patches with no unexpected problems on kernel from [1]
using:

1) lksctp-tools - Passed except test_1_to_1_events as per [2]
2) sctp-tests - As above
3) selinux-testsuite with my SCTP patch [3] - Passes all sctp and
inet_socket tests.
4) The LTP "./runltp -pq -f connect-syscall" (see [4]) - Passes

[1] https://github.com/SELinuxProject/selinux-kernel/tree/next
[2] https://github.com/sctp/lksctp-tools/issues/24
[3] https://marc.info/?l=selinux&m=152156947715709&w=2
[4] https://marc.info/?l=selinux&m=151990968221563&w=2

[Paul Moore]

On Fri, May 11, 2018 at 1:15 PM, Alexey Kodanev

Commit d452930fd3b9 ("selinux: Add SCTP support") breaks compatibility
with the old programs that can pass sockaddr_in structure with AF_UNSPEC
and INADDR_ANY to bind(). As a result, bind() returns EAFNOSUPPORT error.
This 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.
Fixes: d452930fd3b9 ("selinux: Add SCTP support")
---
* return EINVAL for SCTP socket if sa_family is AF_UNSPEC and
address is not INADDR_ANY
* add new 'sa_family' variable so that it equals either AF_INET
or AF_INET6. Besides, it it will be used in the next patch that
fixes audit record.
security/selinux/hooks.c | 29 +++++++++++++++++++----------
1 file changed, 19 insertions(+), 10 deletions(-)

All three patches looked good to me. I've merged them into
selinux/stable-4.17 and assuming nothing breaks in the next day or two
I'll send it up to Linus mid-week.

Thanks everyone!