* [PATCH] selinux: SCTP fixes, including ASCONF
@ 2022-08-09 22:12 Paul Moore
2022-08-11 10:03 ` Ondrej Mosnacek
0 siblings, 1 reply; 5+ messages in thread
From: Paul Moore @ 2022-08-09 22:12 UTC (permalink / raw)
To: selinux
This patch makes two changes to how SELinux processes SCTP traffic:
* Considering the multi-homed nature of SCTP, all SCTP traffic is
marked as NLBL_REQSKB from a NetLabel perspective so that
traffic is labeled on a per-packet basis using the destination IP,
and not the on-the-wire label cached at the socket layer.
* New permissions have been added to the "sctp_socket" object class:
sctp_socket/{asconf_addip, asconf_connect}. These new permissions
are gated by the "sctp_asconf" SELinux policy capability, and
control the ability of ASCONF to add a new IP address to an
association and set the primary IP of the association. The ASCONF
access control points now work like the examples below; <socket> is
the local socket's label, <port> is the label of the network port,
and <peer> is the network peer label (dependent on the labeled
networking configuration).
- legacy policy (no sctp_asconf)
allow <socket> <port>:sctp_socket { name_connect };
- updated policy without labeled networking (enabled sctp_asconf)
allow <socket> <port>:sctp_socket { asconf_connect };
- updated policy with labeled networking (enabled sctp_asconf)
allow <peer> <socket>:sctp_socket { asconf_addip };
allow <socket> <port>:sctp_socket { asconf_connect };
Cc: stable@vger.kernel.org
Fixes: d452930fd3b9 ("selinux: Add SCTP support")
Signed-off-by: Paul Moore <paul@paul-moore.com>
---
security/selinux/hooks.c | 86 +++++++++++++++++++---------
security/selinux/include/classmap.h | 3 +
security/selinux/include/netlabel.h | 1
security/selinux/include/policycap.h | 1
security/selinux/include/policycap_names.h | 3 +
security/selinux/include/security.h | 7 ++
security/selinux/netlabel.c | 30 ++++++++--
7 files changed, 95 insertions(+), 36 deletions(-)
diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
index 1bbd53321d13..02751a66c5d8 100644
--- a/security/selinux/hooks.c
+++ b/security/selinux/hooks.c
@@ -4530,7 +4530,7 @@ static int socket_sockcreate_sid(const struct task_security_struct *tsec,
secclass, NULL, socksid);
}
-static int sock_has_perm(struct sock *sk, u32 perms)
+static int sock_has_perm_subj(u32 subj, struct sock *sk, u32 perms)
{
struct sk_security_struct *sksec = sk->sk_security;
struct common_audit_data ad;
@@ -4544,8 +4544,12 @@ static int sock_has_perm(struct sock *sk, u32 perms)
ad.u.net->sk = sk;
return avc_has_perm(&selinux_state,
- current_sid(), sksec->sid, sksec->sclass, perms,
- &ad);
+ subj, sksec->sid, sksec->sclass, perms, &ad);
+}
+
+static int sock_has_perm(struct sock *sk, u32 perms)
+{
+ return sock_has_perm_subj(current_sid(), sk, perms);
}
static int selinux_socket_create(int family, int type,
@@ -4752,16 +4756,13 @@ static int selinux_socket_bind(struct socket *sock, struct sockaddr *address, in
/* This supports connect(2) and SCTP connect services such as sctp_connectx(3)
* and sctp_sendmsg(3) as described in Documentation/security/SCTP.rst
*/
-static int selinux_socket_connect_helper(struct socket *sock,
+static int selinux_socket_connect_helper(struct socket *sock, u32 perm,
struct sockaddr *address, int addrlen)
{
struct sock *sk = sock->sk;
struct sk_security_struct *sksec = sk->sk_security;
int err;
- err = sock_has_perm(sk, SOCKET__CONNECT);
- if (err)
- return err;
if (addrlen < offsetofend(struct sockaddr, sa_family))
return -EINVAL;
@@ -4783,7 +4784,7 @@ static int selinux_socket_connect_helper(struct socket *sock,
struct sockaddr_in *addr4 = NULL;
struct sockaddr_in6 *addr6 = NULL;
unsigned short snum;
- u32 sid, perm;
+ u32 sid;
/* sctp_connectx(3) calls via selinux_sctp_bind_connect()
* that validates multiple connect addresses. Because of this
@@ -4817,18 +4818,6 @@ static int selinux_socket_connect_helper(struct socket *sock,
if (err)
return err;
- switch (sksec->sclass) {
- case SECCLASS_TCP_SOCKET:
- perm = TCP_SOCKET__NAME_CONNECT;
- break;
- case SECCLASS_DCCP_SOCKET:
- perm = DCCP_SOCKET__NAME_CONNECT;
- break;
- case SECCLASS_SCTP_SOCKET:
- perm = SCTP_SOCKET__NAME_CONNECT;
- break;
- }
-
ad.type = LSM_AUDIT_DATA_NET;
ad.u.net = &net;
ad.u.net->dport = htons(snum);
@@ -4847,9 +4836,26 @@ static int selinux_socket_connect(struct socket *sock,
struct sockaddr *address, int addrlen)
{
int err;
+ u32 perm;
struct sock *sk = sock->sk;
+ struct sk_security_struct *sksec;
- err = selinux_socket_connect_helper(sock, address, addrlen);
+ err = sock_has_perm(sk, SOCKET__CONNECT);
+ if (err)
+ return err;
+ sksec = sk->sk_security;
+ switch (sksec->sclass) {
+ case SECCLASS_TCP_SOCKET:
+ perm = TCP_SOCKET__NAME_CONNECT;
+ break;
+ case SECCLASS_DCCP_SOCKET:
+ perm = DCCP_SOCKET__NAME_CONNECT;
+ break;
+ case SECCLASS_SCTP_SOCKET:
+ perm = SCTP_SOCKET__NAME_CONNECT;
+ break;
+ }
+ err = selinux_socket_connect_helper(sock, perm, address, addrlen);
if (err)
return err;
@@ -5360,17 +5366,24 @@ static int selinux_sctp_bind_connect(struct sock *sk, int optname,
int addrlen)
{
int len, err = 0, walk_size = 0;
+ int pcap_asconf;
+ int peerlbl;
+ u32 perm;
void *addr_buf;
struct sockaddr *addr;
struct socket *sock;
+ struct sk_security_struct *sksec;
if (!selinux_policycap_extsockclass())
return 0;
- /* Process one or more addresses that may be IPv4 or IPv6 */
+ pcap_asconf = selinux_policycap_sctp_asconf();
+ peerlbl = selinux_peerlbl_enabled();
+ sksec = sk->sk_security;
sock = sk->sk_socket;
addr_buf = address;
+ /* Process one or more addresses that may be IPv4 or IPv6 */
while (walk_size < addrlen) {
if (walk_size + sizeof(sa_family_t) > addrlen)
return -EINVAL;
@@ -5393,18 +5406,21 @@ static int selinux_sctp_bind_connect(struct sock *sk, int optname,
err = -EINVAL;
switch (optname) {
- /* Bind checks */
+ /* bind() style checks */
case SCTP_PRIMARY_ADDR:
case SCTP_SET_PEER_PRIMARY_ADDR:
case SCTP_SOCKOPT_BINDX_ADD:
err = selinux_socket_bind(sock, addr, len);
break;
- /* Connect checks */
+ /* connect() style checks */
case SCTP_SOCKOPT_CONNECTX:
- case SCTP_PARAM_SET_PRIMARY:
- case SCTP_PARAM_ADD_IP:
case SCTP_SENDMSG_CONNECT:
- err = selinux_socket_connect_helper(sock, addr, len);
+ err = sock_has_perm(sk, SOCKET__CONNECT);
+ if (err)
+ return err;
+ err = selinux_socket_connect_helper(sock,
+ SCTP_SOCKET__NAME_CONNECT,
+ addr, len);
if (err)
return err;
@@ -5421,6 +5437,22 @@ static int selinux_sctp_bind_connect(struct sock *sk, int optname,
*/
err = selinux_netlbl_socket_connect_locked(sk, addr);
break;
+ /* ASCONF checks (IETF RFC 5061) */
+ case SCTP_PARAM_ADD_IP:
+ case SCTP_PARAM_SET_PRIMARY:
+ if (pcap_asconf) {
+ if (peerlbl) {
+ err = sock_has_perm_subj(sksec->peer_sid,
+ sk, SCTP_SOCKET__ASCONF_ADDIP);
+ if (err)
+ return err;
+ }
+ perm = SCTP_SOCKET__ASCONF_CONNECT;
+ } else
+ perm = SCTP_SOCKET__NAME_CONNECT;
+ err = selinux_socket_connect_helper(sock, perm,
+ addr, len);
+ break;
}
if (err)
diff --git a/security/selinux/include/classmap.h b/security/selinux/include/classmap.h
index ff757ae5f253..8d4e3b4d160e 100644
--- a/security/selinux/include/classmap.h
+++ b/security/selinux/include/classmap.h
@@ -179,7 +179,8 @@ const struct security_class_mapping secclass_map[] = {
{ COMMON_CAP2_PERMS, NULL } },
{ "sctp_socket",
{ COMMON_SOCK_PERMS,
- "node_bind", "name_connect", "association", NULL } },
+ "node_bind", "name_connect", "association", "asconf_addip",
+ "asconf_connect", NULL } },
{ "icmp_socket",
{ COMMON_SOCK_PERMS,
"node_bind", NULL } },
diff --git a/security/selinux/include/netlabel.h b/security/selinux/include/netlabel.h
index 4d0456d3d459..32aa8d4724fa 100644
--- a/security/selinux/include/netlabel.h
+++ b/security/selinux/include/netlabel.h
@@ -55,7 +55,6 @@ int selinux_netlbl_socket_setsockopt(struct socket *sock,
int selinux_netlbl_socket_connect(struct sock *sk, struct sockaddr *addr);
int selinux_netlbl_socket_connect_locked(struct sock *sk,
struct sockaddr *addr);
-
#else
static inline void selinux_netlbl_cache_invalidate(void)
{
diff --git a/security/selinux/include/policycap.h b/security/selinux/include/policycap.h
index f35d3458e71d..6aaee9e12812 100644
--- a/security/selinux/include/policycap.h
+++ b/security/selinux/include/policycap.h
@@ -12,6 +12,7 @@ enum {
POLICYDB_CAP_NNP_NOSUID_TRANSITION,
POLICYDB_CAP_GENFS_SECLABEL_SYMLINKS,
POLICYDB_CAP_IOCTL_SKIP_CLOEXEC,
+ POLICYDB_CAP_SCTP_ASCONF,
__POLICYDB_CAP_MAX
};
#define POLICYDB_CAP_MAX (__POLICYDB_CAP_MAX - 1)
diff --git a/security/selinux/include/policycap_names.h b/security/selinux/include/policycap_names.h
index 2a87fc3702b8..e6d8fbddbbdc 100644
--- a/security/selinux/include/policycap_names.h
+++ b/security/selinux/include/policycap_names.h
@@ -13,7 +13,8 @@ const char *const selinux_policycap_names[__POLICYDB_CAP_MAX] = {
"cgroup_seclabel",
"nnp_nosuid_transition",
"genfs_seclabel_symlinks",
- "ioctl_skip_cloexec"
+ "ioctl_skip_cloexec",
+ "sctp_asconf",
};
#endif /* _SELINUX_POLICYCAP_NAMES_H_ */
diff --git a/security/selinux/include/security.h b/security/selinux/include/security.h
index 393aff41d3ef..98d3e6c0a146 100644
--- a/security/selinux/include/security.h
+++ b/security/selinux/include/security.h
@@ -230,6 +230,13 @@ static inline bool selinux_policycap_ioctl_skip_cloexec(void)
return READ_ONCE(state->policycap[POLICYDB_CAP_IOCTL_SKIP_CLOEXEC]);
}
+static inline bool selinux_policycap_sctp_asconf(void)
+{
+ struct selinux_state *state = &selinux_state;
+
+ return READ_ONCE(state->policycap[POLICYDB_CAP_SCTP_ASCONF]);
+}
+
struct selinux_policy_convert_data;
struct selinux_load_state {
diff --git a/security/selinux/netlabel.c b/security/selinux/netlabel.c
index 1321f15799e2..28d0ead32416 100644
--- a/security/selinux/netlabel.c
+++ b/security/selinux/netlabel.c
@@ -373,10 +373,10 @@ void selinux_netlbl_inet_csk_clone(struct sock *sk, u16 family)
*/
void selinux_netlbl_sctp_sk_clone(struct sock *sk, struct sock *newsk)
{
- struct sk_security_struct *sksec = sk->sk_security;
struct sk_security_struct *newsksec = newsk->sk_security;
- newsksec->nlbl_state = sksec->nlbl_state;
+ /* SCTP is multi-homed so we must label each packet based on dest IP */
+ newsksec->nlbl_state = NLBL_REQSKB;
}
/**
@@ -401,6 +401,17 @@ int selinux_netlbl_socket_post_create(struct sock *sk, u16 family)
secattr = selinux_netlbl_sock_genattr(sk);
if (secattr == NULL)
return -ENOMEM;
+
+ /* SCTP has the ability to communicate with multiple endpoints for a
+ * given association so we need to force NLBL_REQSKB so that we always
+ * label traffic based on the destination endpoint and not the
+ * association's connection
+ */
+ if (sk->sk_protocol == IPPROTO_SCTP) {
+ sksec->nlbl_state = NLBL_REQSKB;
+ return 0;
+ }
+
rc = netlbl_sock_setattr(sk, family, secattr);
switch (rc) {
case 0:
@@ -548,10 +559,17 @@ static int selinux_netlbl_socket_connect_helper(struct sock *sk,
struct sk_security_struct *sksec = sk->sk_security;
struct netlbl_lsm_secattr *secattr;
- /* connected sockets are allowed to disconnect when the address family
- * is set to AF_UNSPEC, if that is what is happening we want to reset
- * the socket */
- if (addr->sa_family == AF_UNSPEC) {
+ /* special handling for AF_UNSPEC and IPPROTO_SCTP:
+ * - sockets are allowed to disconnect when the address family
+ * is set to AF_UNSPEC, if that is what is happening we want to reset
+ * the socket
+ * - SCTP has the ability to communicate with multiple endpoints for
+ * a given association so we need to force NLBL_REQSKB so that we
+ * always label traffic based on the destination endpoint and not
+ * the association's connection, see similar comment in
+ * selinux_netlbl_socket_post_create()
+ */
+ if (addr->sa_family == AF_UNSPEC || sk->sk_protocol == IPPROTO_SCTP) {
netlbl_sock_delattr(sk);
sksec->nlbl_state = NLBL_REQSKB;
rc = 0;
^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH] selinux: SCTP fixes, including ASCONF
2022-08-09 22:12 [PATCH] selinux: SCTP fixes, including ASCONF Paul Moore
@ 2022-08-11 10:03 ` Ondrej Mosnacek
2022-08-11 15:04 ` Paul Moore
0 siblings, 1 reply; 5+ messages in thread
From: Ondrej Mosnacek @ 2022-08-11 10:03 UTC (permalink / raw)
To: Paul Moore; +Cc: SElinux list
On Wed, Aug 10, 2022 at 12:14 AM Paul Moore <paul@paul-moore.com> wrote:
> This patch makes two changes to how SELinux processes SCTP traffic:
Why one patch for two independent changes?
>
> * Considering the multi-homed nature of SCTP, all SCTP traffic is
> marked as NLBL_REQSKB from a NetLabel perspective so that
> traffic is labeled on a per-packet basis using the destination IP,
> and not the on-the-wire label cached at the socket layer.
>
> * New permissions have been added to the "sctp_socket" object class:
> sctp_socket/{asconf_addip, asconf_connect}. These new permissions
> are gated by the "sctp_asconf" SELinux policy capability, and
> control the ability of ASCONF to add a new IP address to an
> association and set the primary IP of the association. The ASCONF
> access control points now work like the examples below; <socket> is
> the local socket's label, <port> is the label of the network port,
> and <peer> is the network peer label (dependent on the labeled
> networking configuration).
>
> - legacy policy (no sctp_asconf)
>
> allow <socket> <port>:sctp_socket { name_connect };
>
> - updated policy without labeled networking (enabled sctp_asconf)
>
> allow <socket> <port>:sctp_socket { asconf_connect };
>
> - updated policy with labeled networking (enabled sctp_asconf)
>
> allow <peer> <socket>:sctp_socket { asconf_addip };
> allow <socket> <port>:sctp_socket { asconf_connect };
>
> Cc: stable@vger.kernel.org
> Fixes: d452930fd3b9 ("selinux: Add SCTP support")
> Signed-off-by: Paul Moore <paul@paul-moore.com>
> ---
> security/selinux/hooks.c | 86 +++++++++++++++++++---------
> security/selinux/include/classmap.h | 3 +
> security/selinux/include/netlabel.h | 1
> security/selinux/include/policycap.h | 1
> security/selinux/include/policycap_names.h | 3 +
> security/selinux/include/security.h | 7 ++
> security/selinux/netlabel.c | 30 ++++++++--
> 7 files changed, 95 insertions(+), 36 deletions(-)
>
> diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
> index 1bbd53321d13..02751a66c5d8 100644
> --- a/security/selinux/hooks.c
> +++ b/security/selinux/hooks.c
<snip>
> @@ -5421,6 +5437,22 @@ static int selinux_sctp_bind_connect(struct sock *sk, int optname,
> */
> err = selinux_netlbl_socket_connect_locked(sk, addr);
> break;
> + /* ASCONF checks (IETF RFC 5061) */
> + case SCTP_PARAM_ADD_IP:
> + case SCTP_PARAM_SET_PRIMARY:
> + if (pcap_asconf) {
> + if (peerlbl) {
> + err = sock_has_perm_subj(sksec->peer_sid,
> + sk, SCTP_SOCKET__ASCONF_ADDIP);
How about splitting this into SCTP_SOCKET__ADD_IP_FROM and
SCTP_SOCKET__SET_PRIMARY_FROM? "_FROM" to give a hint that the
scontext is a peer label as with NODE__RECVFROM; and split so that one
can allow SET_PRIMARY and not ADD_IP, as the former is much less of a
security concern than the latter. For example, the client could first
do connectx() with multiple addresses controlled by itself and then it
might be desirable to allow the remote peer to switch between them via
SET_PRIMARY, yet keep ADD_IP (which can actually modify the address
list) disallowed.
Also, using the "ADDIP" name for both is just misleading as
SET_PRIMARY doesn't add any address, only switches between the already
added ones (and note that these might be added not only via a previous
ADD_IP, but also by the client itself via CONNECTX).
> + if (err)
> + return err;
> + }
> + perm = SCTP_SOCKET__ASCONF_CONNECT;
Again, if it was up to me, I'd split the permission up into ADD_IP and
SET_PRIMARY. And I'd like to reiterate once more that validating the
port seems unnecessary here, since only the address can be changed via
ASCONF, the port is the same for the whole association (just look at
the definition of struct sctp_association for an indirect proof). If
you insist on the name_connect semantics regardless, let's at least
name the permissions "add_ip_name_connect" and
"set_primary_name_connect" so that it's clear that the tcontext is a
port context; otherwise they can be simply:
allow <socket> self:sctp_socket { add_ip set_primary };
(Possibly all with the "asconf_" prefix for more clarity, but IMHO
it's not necessary.)
> + } else
> + perm = SCTP_SOCKET__NAME_CONNECT;
> + err = selinux_socket_connect_helper(sock, perm,
> + addr, len);
> + break;
> }
>
> if (err)
<snip>
> diff --git a/security/selinux/netlabel.c b/security/selinux/netlabel.c
> index 1321f15799e2..28d0ead32416 100644
> --- a/security/selinux/netlabel.c
> +++ b/security/selinux/netlabel.c
I believe you forgot to modify also
selinux_netlbl_sctp_assoc_request() in a similar fashion as the other
functions below?
> @@ -373,10 +373,10 @@ void selinux_netlbl_inet_csk_clone(struct sock *sk, u16 family)
> */
> void selinux_netlbl_sctp_sk_clone(struct sock *sk, struct sock *newsk)
> {
> - struct sk_security_struct *sksec = sk->sk_security;
> struct sk_security_struct *newsksec = newsk->sk_security;
>
> - newsksec->nlbl_state = sksec->nlbl_state;
> + /* SCTP is multi-homed so we must label each packet based on dest IP */
> + newsksec->nlbl_state = NLBL_REQSKB;
> }
>
> /**
> @@ -401,6 +401,17 @@ int selinux_netlbl_socket_post_create(struct sock *sk, u16 family)
> secattr = selinux_netlbl_sock_genattr(sk);
> if (secattr == NULL)
> return -ENOMEM;
> +
> + /* SCTP has the ability to communicate with multiple endpoints for a
> + * given association so we need to force NLBL_REQSKB so that we always
> + * label traffic based on the destination endpoint and not the
> + * association's connection
> + */
> + if (sk->sk_protocol == IPPROTO_SCTP) {
> + sksec->nlbl_state = NLBL_REQSKB;
> + return 0;
> + }
> +
> rc = netlbl_sock_setattr(sk, family, secattr);
> switch (rc) {
> case 0:
> @@ -548,10 +559,17 @@ static int selinux_netlbl_socket_connect_helper(struct sock *sk,
> struct sk_security_struct *sksec = sk->sk_security;
> struct netlbl_lsm_secattr *secattr;
>
> - /* connected sockets are allowed to disconnect when the address family
> - * is set to AF_UNSPEC, if that is what is happening we want to reset
> - * the socket */
> - if (addr->sa_family == AF_UNSPEC) {
> + /* special handling for AF_UNSPEC and IPPROTO_SCTP:
> + * - sockets are allowed to disconnect when the address family
> + * is set to AF_UNSPEC, if that is what is happening we want to reset
> + * the socket
> + * - SCTP has the ability to communicate with multiple endpoints for
> + * a given association so we need to force NLBL_REQSKB so that we
> + * always label traffic based on the destination endpoint and not
> + * the association's connection, see similar comment in
> + * selinux_netlbl_socket_post_create()
> + */
> + if (addr->sa_family == AF_UNSPEC || sk->sk_protocol == IPPROTO_SCTP) {
> netlbl_sock_delattr(sk);
> sksec->nlbl_state = NLBL_REQSKB;
> rc = 0;
Instead of overloading the AF_UNSPEC condition, how about just adding:
if (sk->sk_protocol == IPPROTO_SCTP)
return 0;
...before this one with a separate comment? Overloading the disconnect
condition just makes the code harder to follow. Just bailing out in
the connect helper should be enough once you also patch up
selinux_netlbl_sctp_assoc_request(), since those are the only places
that could change an SCTP socket's nlbl_state away from NLBL_REQSKB or
set any netlabel options on it (well, except when a user explicitly
sets some via setsockopt(2), but in that case we probably shouldn't
delete them even on a disconnect... it's corner-case bugs all the way
down...).
--
Ondrej Mosnacek
Senior Software Engineer, Linux Security - SELinux kernel
Red Hat, Inc.
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] selinux: SCTP fixes, including ASCONF
2022-08-11 10:03 ` Ondrej Mosnacek
@ 2022-08-11 15:04 ` Paul Moore
2022-08-15 20:56 ` Paul Moore
2022-08-23 15:34 ` Ondrej Mosnacek
0 siblings, 2 replies; 5+ messages in thread
From: Paul Moore @ 2022-08-11 15:04 UTC (permalink / raw)
To: Ondrej Mosnacek; +Cc: SElinux list
On Thu, Aug 11, 2022 at 6:03 AM Ondrej Mosnacek <omosnace@redhat.com> wrote:
> On Wed, Aug 10, 2022 at 12:14 AM Paul Moore <paul@paul-moore.com> wrote:
> > This patch makes two changes to how SELinux processes SCTP traffic:
>
> Why one patch for two independent changes?
Taken separately the REQSKB change would be somewhat difficult to
detect as a regular user, putting the two changes into a single patch
means that one can use the presence of the "sctp_asconf" policy
capability as an indicator that the REQSKB fix is present in the
running kernel.
> > @@ -5421,6 +5437,22 @@ static int selinux_sctp_bind_connect(struct sock *sk, int optname,
> > */
> > err = selinux_netlbl_socket_connect_locked(sk, addr);
> > break;
> > + /* ASCONF checks (IETF RFC 5061) */
> > + case SCTP_PARAM_ADD_IP:
> > + case SCTP_PARAM_SET_PRIMARY:
> > + if (pcap_asconf) {
> > + if (peerlbl) {
> > + err = sock_has_perm_subj(sksec->peer_sid,
> > + sk, SCTP_SOCKET__ASCONF_ADDIP);
>
> How about splitting this into SCTP_SOCKET__ADD_IP_FROM and
> SCTP_SOCKET__SET_PRIMARY_FROM? "_FROM" to give a hint that the
> scontext is a peer label as with NODE__RECVFROM ...
I prefer the permission names as they are (without the "_FROM" suffix)
as they better align with the SCTP RFCs.
> ... and split so that one
> can allow SET_PRIMARY and not ADD_IP, as the former is much less of a
> security concern than the latter.
I agree that ADD_IP is a much larger concern, arguably really the only
concern, since you can't set the primary address without first adding
it to the association. It may sound odd, but since SET_PRIMARY MUST
(let's use some of that RFC language here <g>) come after a matching
ADD_IP, I made them equivalent in this patch so that both would be
covered by the same set of allow rules. In other words, the policy
author really only needs to worry about the ADD_IP operation since
that is the critical parameter/op. I did debate dropping the
SET_PRIMARY entirely, but keeping it made the legacy case easier and
in the peerlbl/TRUE case it still offers some value in the case where
the SET_PRIMARY is done at a different time from the ADD_IP
(potentially different network peer labels for each parameter/op).
> Also, using the "ADDIP" name for both is just misleading as
> SET_PRIMARY doesn't add any address, only switches between the already
> added ones (and note that these might be added not only via a previous
> ADD_IP, but also by the client itself via CONNECTX).
Yes, I read the RFC too, but see my comments above.
> > + if (err)
> > + return err;
> > + }
> > + perm = SCTP_SOCKET__ASCONF_CONNECT;
>
> Again, if it was up to me, I'd split the permission up into ADD_IP and
> SET_PRIMARY. And I'd like to reiterate once more that validating the
> port seems unnecessary here ...
To touch on the port issue quickly - while part of the motivation was
to blend better with the existing legacy case, I think there may also
be value in being able to control different ASCONF policies on a
port-by-port basis. For example, maybe you want to allow pretty much
anything from an ASCONF perspective for the http ports but you want to
be much more restrictive about ssh.
> > diff --git a/security/selinux/netlabel.c b/security/selinux/netlabel.c
> > index 1321f15799e2..28d0ead32416 100644
> > --- a/security/selinux/netlabel.c
> > +++ b/security/selinux/netlabel.c
>
> I believe you forgot to modify also
> selinux_netlbl_sctp_assoc_request() in a similar fashion as the other
> functions below?
Good point. I suspect selinux_netlbl_sctp_assoc_request() will be
greatly simplified too, that will be nice.
> > @@ -373,10 +373,10 @@ void selinux_netlbl_inet_csk_clone(struct sock *sk, u16 family)
> > */
> > void selinux_netlbl_sctp_sk_clone(struct sock *sk, struct sock *newsk)
> > {
> > - struct sk_security_struct *sksec = sk->sk_security;
> > struct sk_security_struct *newsksec = newsk->sk_security;
> >
> > - newsksec->nlbl_state = sksec->nlbl_state;
> > + /* SCTP is multi-homed so we must label each packet based on dest IP */
> > + newsksec->nlbl_state = NLBL_REQSKB;
> > }
> >
> > /**
> > @@ -401,6 +401,17 @@ int selinux_netlbl_socket_post_create(struct sock *sk, u16 family)
> > secattr = selinux_netlbl_sock_genattr(sk);
> > if (secattr == NULL)
> > return -ENOMEM;
> > +
> > + /* SCTP has the ability to communicate with multiple endpoints for a
> > + * given association so we need to force NLBL_REQSKB so that we always
> > + * label traffic based on the destination endpoint and not the
> > + * association's connection
> > + */
> > + if (sk->sk_protocol == IPPROTO_SCTP) {
> > + sksec->nlbl_state = NLBL_REQSKB;
> > + return 0;
> > + }
> > +
> > rc = netlbl_sock_setattr(sk, family, secattr);
> > switch (rc) {
> > case 0:
> > @@ -548,10 +559,17 @@ static int selinux_netlbl_socket_connect_helper(struct sock *sk,
> > struct sk_security_struct *sksec = sk->sk_security;
> > struct netlbl_lsm_secattr *secattr;
> >
> > - /* connected sockets are allowed to disconnect when the address family
> > - * is set to AF_UNSPEC, if that is what is happening we want to reset
> > - * the socket */
> > - if (addr->sa_family == AF_UNSPEC) {
> > + /* special handling for AF_UNSPEC and IPPROTO_SCTP:
> > + * - sockets are allowed to disconnect when the address family
> > + * is set to AF_UNSPEC, if that is what is happening we want to reset
> > + * the socket
> > + * - SCTP has the ability to communicate with multiple endpoints for
> > + * a given association so we need to force NLBL_REQSKB so that we
> > + * always label traffic based on the destination endpoint and not
> > + * the association's connection, see similar comment in
> > + * selinux_netlbl_socket_post_create()
> > + */
> > + if (addr->sa_family == AF_UNSPEC || sk->sk_protocol == IPPROTO_SCTP) {
> > netlbl_sock_delattr(sk);
> > sksec->nlbl_state = NLBL_REQSKB;
> > rc = 0;
>
> Instead of overloading the AF_UNSPEC condition, how about just adding:
>
> if (sk->sk_protocol == IPPROTO_SCTP)
> return 0;
>
> ...before this one with a separate comment? Overloading the disconnect
> condition just makes the code harder to follow.
Heh. Okay, I see your point, but if that bit of code is too hard to
follow for someone they are going to have a very rough time with the
rest of the kernel code ;)
I'm going to leave this as-is.
> Just bailing out in
> the connect helper should be enough once you also patch up
> selinux_netlbl_sctp_assoc_request(), since those are the only places
> that could change an SCTP socket's nlbl_state away from NLBL_REQSKB or
> set any netlabel options on it (well, except when a user explicitly
> sets some via setsockopt(2), but in that case we probably shouldn't
> delete them even on a disconnect...
Just as a FYI, the socket_setsockopt hook prevents a user from
overwriting an existing NetLabel socket option, and in the REQSKB case
we either overwrite any socket inherited NetLabel socket options in
the case of CIPSO or add another hop-by-hop option in the case of
CALIPSO (it's quicker that way). We don't explicitly block a user
form setting a on-the-wire packet label using setsockopt() in the case
where no labeling is configured in case the application wants to
manage things on their own, but we do provide a socket-level
permission check (socket/setopt) so that policy developers can block
this if they want.
--
paul-moore.com
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] selinux: SCTP fixes, including ASCONF
2022-08-11 15:04 ` Paul Moore
@ 2022-08-15 20:56 ` Paul Moore
2022-08-23 15:34 ` Ondrej Mosnacek
1 sibling, 0 replies; 5+ messages in thread
From: Paul Moore @ 2022-08-15 20:56 UTC (permalink / raw)
To: Ondrej Mosnacek; +Cc: SElinux list
On Thu, Aug 11, 2022 at 11:04 AM Paul Moore <paul@paul-moore.com> wrote:
> On Thu, Aug 11, 2022 at 6:03 AM Ondrej Mosnacek <omosnace@redhat.com> wrote:
> > On Wed, Aug 10, 2022 at 12:14 AM Paul Moore <paul@paul-moore.com> wrote:
...
> > I believe you forgot to modify also
> > selinux_netlbl_sctp_assoc_request() in a similar fashion as the other
> > functions below?
>
> Good point. I suspect selinux_netlbl_sctp_assoc_request() will be
> greatly simplified too, that will be nice.
I'm beginning to think the SELinux/SCTP code is cursed :/
That change brought up some other issues (more of the
multiple-associations-on-a-single-socket variety) which I need to
think about some more, so I'm putting this on the back-burner for a
little bit while I sort out the io_uring and other things which are
more straightforward and need to get into linux-next ASAP.
--
paul-moore.com
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] selinux: SCTP fixes, including ASCONF
2022-08-11 15:04 ` Paul Moore
2022-08-15 20:56 ` Paul Moore
@ 2022-08-23 15:34 ` Ondrej Mosnacek
1 sibling, 0 replies; 5+ messages in thread
From: Ondrej Mosnacek @ 2022-08-23 15:34 UTC (permalink / raw)
To: Paul Moore; +Cc: SElinux list
(Sorry for the late reply, I was on vacation last week.)
On Thu, Aug 11, 2022 at 5:04 PM Paul Moore <paul@paul-moore.com> wrote:
> On Thu, Aug 11, 2022 at 6:03 AM Ondrej Mosnacek <omosnace@redhat.com> wrote:
> > On Wed, Aug 10, 2022 at 12:14 AM Paul Moore <paul@paul-moore.com> wrote:
> > > This patch makes two changes to how SELinux processes SCTP traffic:
> >
> > Why one patch for two independent changes?
>
> Taken separately the REQSKB change would be somewhat difficult to
> detect as a regular user, putting the two changes into a single patch
> means that one can use the presence of the "sctp_asconf" policy
> capability as an indicator that the REQSKB fix is present in the
> running kernel.
Why would one need to detect it, though? For selinux-testsuite? For
that it will be sufficient if the REQSKB fix is applied before the
ASCONF fix. Anyway, this is bikeshedding and I don't insist on
splitting the patches, just that I don't see an obvious reason so I'd
like to understand why it's being done this way.
> > > @@ -5421,6 +5437,22 @@ static int selinux_sctp_bind_connect(struct sock *sk, int optname,
> > > */
> > > err = selinux_netlbl_socket_connect_locked(sk, addr);
> > > break;
> > > + /* ASCONF checks (IETF RFC 5061) */
> > > + case SCTP_PARAM_ADD_IP:
> > > + case SCTP_PARAM_SET_PRIMARY:
> > > + if (pcap_asconf) {
> > > + if (peerlbl) {
> > > + err = sock_has_perm_subj(sksec->peer_sid,
> > > + sk, SCTP_SOCKET__ASCONF_ADDIP);
> >
> > How about splitting this into SCTP_SOCKET__ADD_IP_FROM and
> > SCTP_SOCKET__SET_PRIMARY_FROM? "_FROM" to give a hint that the
> > scontext is a peer label as with NODE__RECVFROM ...
>
> I prefer the permission names as they are (without the "_FROM" suffix)
> as they better align with the SCTP RFCs.
I don't see how the suffix is relevant to alignment to RFCs, but
anyway I'm beginning to doubt if the _FROM suffix makes that much
sense... Let's leave it at ASCONF_SOMETHING then. (But see below why I
still think it should be split into two permissions or apply only to
ADD_IP.)
> > ... and split so that one
> > can allow SET_PRIMARY and not ADD_IP, as the former is much less of a
> > security concern than the latter.
>
> I agree that ADD_IP is a much larger concern, arguably really the only
> concern, since you can't set the primary address without first adding
> it to the association. It may sound odd, but since SET_PRIMARY MUST
> (let's use some of that RFC language here <g>) come after a matching
> ADD_IP,
I don't think that's necessarily true. 5.4 says "A sender MUST only
send a set primary request to an address that is already considered
part of the association." But the addresses in the association don't
all have to come from ASCONF, they can be also populated by the client
itself via CONNECTX. So unless I'm mistaken, it is possible to open a
multi-address association via CONNECTX and then let the remote peer
indicate its preferred primary address via SET_PRIMARY. If (at SELinux
level) I wanted to allow this scenario, but prevent the remote peer
from adding any extra addresses to the association, I wouldn't be able
to do so with this patch.
> I made them equivalent in this patch so that both would be
> covered by the same set of allow rules. In other words, the policy
> author really only needs to worry about the ADD_IP operation since
> that is the critical parameter/op. I did debate dropping the
> SET_PRIMARY entirely, but keeping it made the legacy case easier and
> in the peerlbl/TRUE case it still offers some value in the case where
> the SET_PRIMARY is done at a different time from the ADD_IP
> (potentially different network peer labels for each parameter/op).
>
> > Also, using the "ADDIP" name for both is just misleading as
> > SET_PRIMARY doesn't add any address, only switches between the already
> > added ones (and note that these might be added not only via a previous
> > ADD_IP, but also by the client itself via CONNECTX).
>
> Yes, I read the RFC too, but see my comments above.
>
> > > + if (err)
> > > + return err;
> > > + }
> > > + perm = SCTP_SOCKET__ASCONF_CONNECT;
> >
> > Again, if it was up to me, I'd split the permission up into ADD_IP and
> > SET_PRIMARY. And I'd like to reiterate once more that validating the
> > port seems unnecessary here ...
>
> To touch on the port issue quickly - while part of the motivation was
> to blend better with the existing legacy case, I think there may also
> be value in being able to control different ASCONF policies on a
> port-by-port basis. For example, maybe you want to allow pretty much
> anything from an ASCONF perspective for the http ports but you want to
> be much more restrictive about ssh.
Hmm, I guess that might be a valid use case indeed... Though I wonder
if we shouldn't keep the "name" part in the permission name. At least
I get the impression that the "name" in "name_connect" and
"name_bind" means we are checking against a port's label. Sorry if
that's a misunderstanding.
> > > diff --git a/security/selinux/netlabel.c b/security/selinux/netlabel.c
> > > index 1321f15799e2..28d0ead32416 100644
> > > --- a/security/selinux/netlabel.c
> > > +++ b/security/selinux/netlabel.c
> >
> > I believe you forgot to modify also
> > selinux_netlbl_sctp_assoc_request() in a similar fashion as the other
> > functions below?
>
> Good point. I suspect selinux_netlbl_sctp_assoc_request() will be
> greatly simplified too, that will be nice.
>
> > > @@ -373,10 +373,10 @@ void selinux_netlbl_inet_csk_clone(struct sock *sk, u16 family)
> > > */
> > > void selinux_netlbl_sctp_sk_clone(struct sock *sk, struct sock *newsk)
> > > {
> > > - struct sk_security_struct *sksec = sk->sk_security;
> > > struct sk_security_struct *newsksec = newsk->sk_security;
> > >
> > > - newsksec->nlbl_state = sksec->nlbl_state;
> > > + /* SCTP is multi-homed so we must label each packet based on dest IP */
> > > + newsksec->nlbl_state = NLBL_REQSKB;
> > > }
> > >
> > > /**
> > > @@ -401,6 +401,17 @@ int selinux_netlbl_socket_post_create(struct sock *sk, u16 family)
> > > secattr = selinux_netlbl_sock_genattr(sk);
> > > if (secattr == NULL)
> > > return -ENOMEM;
> > > +
> > > + /* SCTP has the ability to communicate with multiple endpoints for a
> > > + * given association so we need to force NLBL_REQSKB so that we always
> > > + * label traffic based on the destination endpoint and not the
> > > + * association's connection
> > > + */
> > > + if (sk->sk_protocol == IPPROTO_SCTP) {
> > > + sksec->nlbl_state = NLBL_REQSKB;
> > > + return 0;
> > > + }
> > > +
> > > rc = netlbl_sock_setattr(sk, family, secattr);
> > > switch (rc) {
> > > case 0:
> > > @@ -548,10 +559,17 @@ static int selinux_netlbl_socket_connect_helper(struct sock *sk,
> > > struct sk_security_struct *sksec = sk->sk_security;
> > > struct netlbl_lsm_secattr *secattr;
> > >
> > > - /* connected sockets are allowed to disconnect when the address family
> > > - * is set to AF_UNSPEC, if that is what is happening we want to reset
> > > - * the socket */
> > > - if (addr->sa_family == AF_UNSPEC) {
> > > + /* special handling for AF_UNSPEC and IPPROTO_SCTP:
> > > + * - sockets are allowed to disconnect when the address family
> > > + * is set to AF_UNSPEC, if that is what is happening we want to reset
> > > + * the socket
> > > + * - SCTP has the ability to communicate with multiple endpoints for
> > > + * a given association so we need to force NLBL_REQSKB so that we
> > > + * always label traffic based on the destination endpoint and not
> > > + * the association's connection, see similar comment in
> > > + * selinux_netlbl_socket_post_create()
> > > + */
> > > + if (addr->sa_family == AF_UNSPEC || sk->sk_protocol == IPPROTO_SCTP) {
> > > netlbl_sock_delattr(sk);
> > > sksec->nlbl_state = NLBL_REQSKB;
> > > rc = 0;
> >
> > Instead of overloading the AF_UNSPEC condition, how about just adding:
> >
> > if (sk->sk_protocol == IPPROTO_SCTP)
> > return 0;
> >
> > ...before this one with a separate comment? Overloading the disconnect
> > condition just makes the code harder to follow.
>
> Heh. Okay, I see your point, but if that bit of code is too hard to
> follow for someone they are going to have a very rough time with the
> rest of the kernel code ;)
>
> I'm going to leave this as-is.
Well, yes, the kernel contains a lot of ugly code, but that's no
excuse to add more... Yet, this is another bikeshedding that is maybe
worth raising but not worth arguing about, so I'm not going to scratch
it further.
> > Just bailing out in
> > the connect helper should be enough once you also patch up
> > selinux_netlbl_sctp_assoc_request(), since those are the only places
> > that could change an SCTP socket's nlbl_state away from NLBL_REQSKB or
> > set any netlabel options on it (well, except when a user explicitly
> > sets some via setsockopt(2), but in that case we probably shouldn't
> > delete them even on a disconnect...
>
> Just as a FYI, the socket_setsockopt hook prevents a user from
> overwriting an existing NetLabel socket option, and in the REQSKB case
> we either overwrite any socket inherited NetLabel socket options in
> the case of CIPSO or add another hop-by-hop option in the case of
> CALIPSO (it's quicker that way). We don't explicitly block a user
> form setting a on-the-wire packet label using setsockopt() in the case
> where no labeling is configured in case the application wants to
> manage things on their own, but we do provide a socket-level
> permission check (socket/setopt) so that policy developers can block
> this if they want.
Well, yes, but none of that explains why we call
netlbl_sock_delattr(sk) unconditionally on disconnect. Shouldn't the
whole AF_UNSPEC block be executed only if sksec->nlbl_state ==
NLBL_CONNLABELED? Because of the guard in
selinux_netlbl_socket_connect_locked() the state can be only either
NLBL_CONNLABELED or NLBL_REQSKB here. In the case of NLBL_CONNLABELED,
deleting the NetLabel options is logical, but in the case of
NLBL_REQSKB we would be deleting options set by the user, which
doesn't seem right.
It is a pre-existing bug, but you are trying to exacerbate it even
more by reusing the conditional for SCTP, so perhaps worth addressing
beforehand.
One more side thought: It's weird that currently we will allow setting
the socket options in case of NLBL_REQSKB, but not in the case where
the socket has a NETLBL_NLTYPE_UNLABELED entry. In this case there are
no options set on the socket by NetLabel and it is marked as
NLBL_LABELED, which means that selinux_netlbl_socket_setsockopt() will
reject setting NetLabel options on it.
Oh, and while I'm at it - does anybody know what is the intent behind
the mysterious if-else in selinux_netlbl_inet_csk_clone()?
--
Ondrej Mosnacek
Senior Software Engineer, Linux Security - SELinux kernel
Red Hat, Inc.
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2022-08-23 17:44 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-08-09 22:12 [PATCH] selinux: SCTP fixes, including ASCONF Paul Moore
2022-08-11 10:03 ` Ondrej Mosnacek
2022-08-11 15:04 ` Paul Moore
2022-08-15 20:56 ` Paul Moore
2022-08-23 15:34 ` Ondrej Mosnacek
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.