| Message ID | CAHC9VhRmX+Z+ignXXBrSGyUkpxudO76Fv-6o_My=skh-U8Ccgw@mail.gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
On 10.05.2018 01:02, Paul Moore wrote: ... > I just had a better look at this and I believe that Alexey and Stephen > are right: this is the best option. My apologies for the noise > earlier. However, while looking at the code I think there are some > additional necessary changes: > > * In the case of an SCTP socket, we should return -EINVAL, just as we > do with other address families. Right. > * While not strictly related to AF_UNSPEC, we really should be passing > the address family of the sockaddr, and not the socket, to functions > that need to interpret the bind address/port. That looks like a correct solution. I guess we need the same fix for sctp_connectx(), in selinux_socket_connect_helper(). > > I'm waiting for my kernel to compile so I haven't given this any > sanity testing, but the patch below is what I think we need ... > > diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c > index 4cafe6a19167..5f30045b2053 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, i > nt addrlen) > { > struct sock *sk = sock->sk; > + struct sk_security_struct *sksec = sk->sk_security; > u16 family; > int err; > > @@ -4587,13 +4588,13 @@ static int selinux_socket_bind(struct socket *sock, stru > ct 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; > unsigned short snum; > u32 sid, node_perm; > + u16 family_sa = address->sa_family; > > /* > * sctp_bindx(3) calls via selinux_sctp_bind_connect() > @@ -4601,11 +4602,19 @@ static int selinux_socket_bind(struct socket *sock, stru > ct 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) { > + case AF_UNSPEC: > case AF_INET: > 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,15 +4626,14 @@ static int selinux_socket_bind(struct socket *sock, stru > ct sockaddr *address, in > addrp = (char *)&addr6->sin6_addr.s6_addr; > break; > default: > - /* Note that SCTP services expect -EINVAL, whereas > - * others expect -EAFNOSUPPORT. > - */ > - if (sksec->sclass == SECCLASS_SCTP_SOCKET) > - return -EINVAL; > - else > - return -EAFNOSUPPORT; > + 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; > + May be we could move setting ad.u.net->v{4|6}info.saddr here as well? Will send a v2 of this patch so that SCTP socket returns EINVAL with AF_UNSPEC. Should I prepare a patch with correcting 'ad.u.net->family' and sel_netnode_sid()? Thanks, Alexey > if (snum) { > int low, high; > > @@ -4637,10 +4645,6 @@ static int selinux_socket_bind(struct socket *sock, struc > t 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, > @@ -4672,16 +4676,11 @@ static int selinux_socket_bind(struct socket *sock, stru > ct 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 (address->sa_family == AF_INET) > + if (family_sa == AF_INET) > ad.u.net->v4info.saddr = addr4->sin_addr.s_addr; > else > ad.u.net->v6info.saddr = addr6->sin6_addr; > @@ -4694,6 +4693,12 @@ static int selinux_socket_bind(struct socket *sock, struc > t sockaddr *address, in > } > out: > return err; > +err_af: > + /* Note that SCTP services expect -EINVAL, others -EAFNOSUPPORT. */ > + if (sksec->sclass == SECCLASS_SCTP_SOCKET) > + return -EINVAL; > + else > + return -EAFNOSUPPORT; > } > > /* This supports connect(2) and SCTP connect services such as sctp_connectx(3) >
diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c index 4cafe6a19167..5f30045b2053 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, i nt addrlen) { struct sock *sk = sock->sk; + struct sk_security_struct *sksec = sk->sk_security; u16 family;