* [PATCHv2 net 0/4] security: fixups for the security hooks in sctp @ 2021-11-02 12:02 Xin Long 2021-11-02 12:02 ` [PATCHv2 net 1/4] security: pass asoc to sctp_assoc_request and sctp_sk_clone Xin Long ` (4 more replies) 0 siblings, 5 replies; 20+ messages in thread From: Xin Long @ 2021-11-02 12:02 UTC (permalink / raw) To: network dev, selinux, linux-security-module, linux-sctp Cc: davem, kuba, Marcelo Ricardo Leitner, James Morris, Paul Moore, Richard Haines, Ondrej Mosnacek There are a couple of problems in the currect security hooks in sctp: 1. The hooks incorrectly treat sctp_endpoint in SCTP as request_sock in TCP, while it's in fact no more than an extension of the sock, and represents the local host. It is created when sock is created, not when a conn request comes. sctp_association is actually the correct one to represent the connection, and created when a conn request arrives. 2. security_sctp_assoc_request() hook should also be called in processing COOKIE ECHO, as that's the place where the real assoc is created and used in the future. The problems above may cause accept sk, peeloff sk or client sk having the incorrect security labels. So this patchset is to change some hooks and pass asoc into them and save these secids into asoc, as well as add the missing sctp_assoc_request hook into the COOKIE ECHO processing. v1->v2: - See each patch, and thanks the help from Ondrej, Paul and Richard. Xin Long (4): security: pass asoc to sctp_assoc_request and sctp_sk_clone security: call security_sctp_assoc_request in sctp_sf_do_5_1D_ce security: add sctp_assoc_established hook security: implement sctp_assoc_established hook in selinux Documentation/security/SCTP.rst | 65 +++++++++++++++-------------- include/linux/lsm_hook_defs.h | 6 ++- include/linux/lsm_hooks.h | 13 ++++-- include/linux/security.h | 17 +++++--- include/net/sctp/structs.h | 20 ++++----- net/sctp/sm_statefuns.c | 31 ++++++++------ net/sctp/socket.c | 5 +-- security/security.c | 15 +++++-- security/selinux/hooks.c | 34 ++++++++++----- security/selinux/include/netlabel.h | 4 +- security/selinux/netlabel.c | 18 ++++---- 11 files changed, 133 insertions(+), 95 deletions(-) -- 2.27.0 ^ permalink raw reply [flat|nested] 20+ messages in thread
* [PATCHv2 net 1/4] security: pass asoc to sctp_assoc_request and sctp_sk_clone 2021-11-02 12:02 [PATCHv2 net 0/4] security: fixups for the security hooks in sctp Xin Long @ 2021-11-02 12:02 ` Xin Long 2021-11-02 12:02 ` [PATCHv2 net 2/4] security: call security_sctp_assoc_request in sctp_sf_do_5_1D_ce Xin Long ` (3 subsequent siblings) 4 siblings, 0 replies; 20+ messages in thread From: Xin Long @ 2021-11-02 12:02 UTC (permalink / raw) To: network dev, selinux, linux-security-module, linux-sctp Cc: davem, kuba, Marcelo Ricardo Leitner, James Morris, Paul Moore, Richard Haines, Ondrej Mosnacek This patch is to move secid and peer_secid from endpoint to association, and pass asoc to sctp_assoc_request and sctp_sk_clone instead of ep. As ep is the local endpoint and asoc represents a connection, and in SCTP one sk/ep could have multiple asoc/connection, saving secid/peer_secid for new asoc will overwrite the old asoc's. Note that since asoc can be passed as NULL, security_sctp_assoc_request() is moved to the place right after the new_asoc is created in sctp_sf_do_5_1B_init() and sctp_sf_do_unexpected_init(). v1->v2: - fix the description of selinux_netlbl_skbuff_setsid(), as Jakub noticed. - fix the annotation in selinux_sctp_assoc_request(), as Richard Noticed. Fixes: 72e89f50084c ("security: Add support for SCTP security hooks") Reported-by: Prashanth Prahlad <pprahlad@redhat.com> Reviewed-by: Richard Haines <richard_c_haines@btinternet.com> Tested-by: Richard Haines <richard_c_haines@btinternet.com> Signed-off-by: Xin Long <lucien.xin@gmail.com> --- Documentation/security/SCTP.rst | 28 ++++++++++++++-------------- include/linux/lsm_hook_defs.h | 4 ++-- include/linux/lsm_hooks.h | 8 ++++---- include/linux/security.h | 10 +++++----- include/net/sctp/structs.h | 20 ++++++++++---------- net/sctp/sm_statefuns.c | 26 +++++++++++++------------- net/sctp/socket.c | 5 ++--- security/security.c | 8 ++++---- security/selinux/hooks.c | 22 +++++++++++----------- security/selinux/include/netlabel.h | 4 ++-- security/selinux/netlabel.c | 18 +++++++++--------- 11 files changed, 76 insertions(+), 77 deletions(-) diff --git a/Documentation/security/SCTP.rst b/Documentation/security/SCTP.rst index 0bcf6c1245ee..415b548d9ce0 100644 --- a/Documentation/security/SCTP.rst +++ b/Documentation/security/SCTP.rst @@ -26,11 +26,11 @@ described in the `SCTP SELinux Support`_ chapter. security_sctp_assoc_request() ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ -Passes the ``@ep`` and ``@chunk->skb`` of the association INIT packet to the +Passes the ``@asoc`` and ``@chunk->skb`` of the association INIT packet to the security module. Returns 0 on success, error on failure. :: - @ep - pointer to sctp endpoint structure. + @asoc - pointer to sctp association structure. @skb - pointer to skbuff of association packet. @@ -117,9 +117,9 @@ Called whenever a new socket is created by **accept**\(2) calls **sctp_peeloff**\(3). :: - @ep - pointer to current sctp endpoint structure. + @asoc - pointer to current sctp association structure. @sk - pointer to current sock structure. - @sk - pointer to new sock structure. + @newsk - pointer to new sock structure. security_inet_conn_established() @@ -200,22 +200,22 @@ hooks with the SELinux specifics expanded below:: security_sctp_assoc_request() ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ -Passes the ``@ep`` and ``@chunk->skb`` of the association INIT packet to the +Passes the ``@asoc`` and ``@chunk->skb`` of the association INIT packet to the security module. Returns 0 on success, error on failure. :: - @ep - pointer to sctp endpoint structure. + @asoc - pointer to sctp association structure. @skb - pointer to skbuff of association packet. The security module performs the following operations: - IF this is the first association on ``@ep->base.sk``, then set the peer + IF this is the first association on ``@asoc->base.sk``, then set the peer sid to that in ``@skb``. This will ensure there is only one peer sid - assigned to ``@ep->base.sk`` that may support multiple associations. + assigned to ``@asoc->base.sk`` that may support multiple associations. - ELSE validate the ``@ep->base.sk peer_sid`` against the ``@skb peer sid`` + ELSE validate the ``@asoc->base.sk peer_sid`` against the ``@skb peer sid`` to determine whether the association should be allowed or denied. - Set the sctp ``@ep sid`` to socket's sid (from ``ep->base.sk``) with + Set the sctp ``@asoc sid`` to socket's sid (from ``asoc->base.sk``) with MLS portion taken from ``@skb peer sid``. This will be used by SCTP TCP style sockets and peeled off connections as they cause a new socket to be generated. @@ -259,13 +259,13 @@ security_sctp_sk_clone() Called whenever a new socket is created by **accept**\(2) (i.e. a TCP style socket) or when a socket is 'peeled off' e.g userspace calls **sctp_peeloff**\(3). ``security_sctp_sk_clone()`` will set the new -sockets sid and peer sid to that contained in the ``@ep sid`` and -``@ep peer sid`` respectively. +sockets sid and peer sid to that contained in the ``@asoc sid`` and +``@asoc peer sid`` respectively. :: - @ep - pointer to current sctp endpoint structure. + @asoc - pointer to current sctp association structure. @sk - pointer to current sock structure. - @sk - pointer to new sock structure. + @newsk - pointer to new sock structure. security_inet_conn_established() diff --git a/include/linux/lsm_hook_defs.h b/include/linux/lsm_hook_defs.h index 2adeea44c0d5..0024273a7382 100644 --- a/include/linux/lsm_hook_defs.h +++ b/include/linux/lsm_hook_defs.h @@ -328,11 +328,11 @@ LSM_HOOK(int, 0, tun_dev_create, void) LSM_HOOK(int, 0, tun_dev_attach_queue, void *security) LSM_HOOK(int, 0, tun_dev_attach, struct sock *sk, void *security) LSM_HOOK(int, 0, tun_dev_open, void *security) -LSM_HOOK(int, 0, sctp_assoc_request, struct sctp_endpoint *ep, +LSM_HOOK(int, 0, sctp_assoc_request, struct sctp_association *asoc, struct sk_buff *skb) LSM_HOOK(int, 0, sctp_bind_connect, struct sock *sk, int optname, struct sockaddr *address, int addrlen) -LSM_HOOK(void, LSM_RET_VOID, sctp_sk_clone, struct sctp_endpoint *ep, +LSM_HOOK(void, LSM_RET_VOID, sctp_sk_clone, struct sctp_association *asoc, struct sock *sk, struct sock *newsk) #endif /* CONFIG_SECURITY_NETWORK */ diff --git a/include/linux/lsm_hooks.h b/include/linux/lsm_hooks.h index 5c4c5c0602cb..240b92d89852 100644 --- a/include/linux/lsm_hooks.h +++ b/include/linux/lsm_hooks.h @@ -1024,9 +1024,9 @@ * Security hooks for SCTP * * @sctp_assoc_request: - * Passes the @ep and @chunk->skb of the association INIT packet to + * Passes the @asoc and @chunk->skb of the association INIT packet to * the security module. - * @ep pointer to sctp endpoint structure. + * @asoc pointer to sctp association structure. * @skb pointer to skbuff of association packet. * Return 0 on success, error on failure. * @sctp_bind_connect: @@ -1044,9 +1044,9 @@ * Called whenever a new socket is created by accept(2) (i.e. a TCP * style socket) or when a socket is 'peeled off' e.g userspace * calls sctp_peeloff(3). - * @ep pointer to current sctp endpoint structure. + * @asoc pointer to current sctp association structure. * @sk pointer to current sock structure. - * @sk pointer to new sock structure. + * @newsk pointer to new sock structure. * * Security hooks for Infiniband * diff --git a/include/linux/security.h b/include/linux/security.h index 5b7288521300..a16407444871 100644 --- a/include/linux/security.h +++ b/include/linux/security.h @@ -179,7 +179,7 @@ struct xfrm_policy; struct xfrm_state; struct xfrm_user_sec_ctx; struct seq_file; -struct sctp_endpoint; +struct sctp_association; #ifdef CONFIG_MMU extern unsigned long mmap_min_addr; @@ -1418,10 +1418,10 @@ int security_tun_dev_create(void); int security_tun_dev_attach_queue(void *security); int security_tun_dev_attach(struct sock *sk, void *security); int security_tun_dev_open(void *security); -int security_sctp_assoc_request(struct sctp_endpoint *ep, struct sk_buff *skb); +int security_sctp_assoc_request(struct sctp_association *asoc, struct sk_buff *skb); int security_sctp_bind_connect(struct sock *sk, int optname, struct sockaddr *address, int addrlen); -void security_sctp_sk_clone(struct sctp_endpoint *ep, struct sock *sk, +void security_sctp_sk_clone(struct sctp_association *asoc, struct sock *sk, struct sock *newsk); #else /* CONFIG_SECURITY_NETWORK */ @@ -1624,7 +1624,7 @@ static inline int security_tun_dev_open(void *security) return 0; } -static inline int security_sctp_assoc_request(struct sctp_endpoint *ep, +static inline int security_sctp_assoc_request(struct sctp_association *asoc, struct sk_buff *skb) { return 0; @@ -1637,7 +1637,7 @@ static inline int security_sctp_bind_connect(struct sock *sk, int optname, return 0; } -static inline void security_sctp_sk_clone(struct sctp_endpoint *ep, +static inline void security_sctp_sk_clone(struct sctp_association *asoc, struct sock *sk, struct sock *newsk) { diff --git a/include/net/sctp/structs.h b/include/net/sctp/structs.h index 651bba654d77..899c29c326ba 100644 --- a/include/net/sctp/structs.h +++ b/include/net/sctp/structs.h @@ -1355,16 +1355,6 @@ struct sctp_endpoint { reconf_enable:1; __u8 strreset_enable; - - /* Security identifiers from incoming (INIT). These are set by - * security_sctp_assoc_request(). These will only be used by - * SCTP TCP type sockets and peeled off connections as they - * cause a new socket to be generated. security_sctp_sk_clone() - * will then plug these into the new socket. - */ - - u32 secid; - u32 peer_secid; }; /* Recover the outter endpoint structure. */ @@ -2104,6 +2094,16 @@ struct sctp_association { __u64 abandoned_unsent[SCTP_PR_INDEX(MAX) + 1]; __u64 abandoned_sent[SCTP_PR_INDEX(MAX) + 1]; + /* Security identifiers from incoming (INIT). These are set by + * security_sctp_assoc_request(). These will only be used by + * SCTP TCP type sockets and peeled off connections as they + * cause a new socket to be generated. security_sctp_sk_clone() + * will then plug these into the new socket. + */ + + u32 secid; + u32 peer_secid; + struct rcu_head rcu; }; diff --git a/net/sctp/sm_statefuns.c b/net/sctp/sm_statefuns.c index fb3da4d8f4a3..3206374209bc 100644 --- a/net/sctp/sm_statefuns.c +++ b/net/sctp/sm_statefuns.c @@ -326,11 +326,6 @@ enum sctp_disposition sctp_sf_do_5_1B_init(struct net *net, struct sctp_packet *packet; int len; - /* Update socket peer label if first association. */ - if (security_sctp_assoc_request((struct sctp_endpoint *)ep, - chunk->skb)) - return sctp_sf_pdiscard(net, ep, asoc, type, arg, commands); - /* 6.10 Bundling * An endpoint MUST NOT bundle INIT, INIT ACK or * SHUTDOWN COMPLETE with any other chunks. @@ -415,6 +410,12 @@ enum sctp_disposition sctp_sf_do_5_1B_init(struct net *net, if (!new_asoc) goto nomem; + /* Update socket peer label if first association. */ + if (security_sctp_assoc_request(new_asoc, chunk->skb)) { + sctp_association_free(new_asoc); + return sctp_sf_pdiscard(net, ep, asoc, type, arg, commands); + } + if (sctp_assoc_set_bind_addr_from_ep(new_asoc, sctp_scope(sctp_source(chunk)), GFP_ATOMIC) < 0) @@ -780,7 +781,6 @@ enum sctp_disposition sctp_sf_do_5_1D_ce(struct net *net, } } - /* Delay state machine commands until later. * * Re-build the bind address for the association is done in @@ -1517,11 +1517,6 @@ static enum sctp_disposition sctp_sf_do_unexpected_init( struct sctp_packet *packet; int len; - /* Update socket peer label if first association. */ - if (security_sctp_assoc_request((struct sctp_endpoint *)ep, - chunk->skb)) - return sctp_sf_pdiscard(net, ep, asoc, type, arg, commands); - /* 6.10 Bundling * An endpoint MUST NOT bundle INIT, INIT ACK or * SHUTDOWN COMPLETE with any other chunks. @@ -1594,6 +1589,12 @@ static enum sctp_disposition sctp_sf_do_unexpected_init( if (!new_asoc) goto nomem; + /* Update socket peer label if first association. */ + if (security_sctp_assoc_request(new_asoc, chunk->skb)) { + sctp_association_free(new_asoc); + return sctp_sf_pdiscard(net, ep, asoc, type, arg, commands); + } + if (sctp_assoc_set_bind_addr_from_ep(new_asoc, sctp_scope(sctp_source(chunk)), GFP_ATOMIC) < 0) goto nomem; @@ -2255,8 +2256,7 @@ enum sctp_disposition sctp_sf_do_5_2_4_dupcook( } /* Update socket peer label if first association. */ - if (security_sctp_assoc_request((struct sctp_endpoint *)ep, - chunk->skb)) { + if (security_sctp_assoc_request(new_asoc, chunk->skb)) { sctp_association_free(new_asoc); return sctp_sf_pdiscard(net, ep, asoc, type, arg, commands); } diff --git a/net/sctp/socket.c b/net/sctp/socket.c index 6b937bfd4751..33391254fa82 100644 --- a/net/sctp/socket.c +++ b/net/sctp/socket.c @@ -9412,7 +9412,6 @@ void sctp_copy_sock(struct sock *newsk, struct sock *sk, struct inet_sock *inet = inet_sk(sk); struct inet_sock *newinet; struct sctp_sock *sp = sctp_sk(sk); - struct sctp_endpoint *ep = sp->ep; newsk->sk_type = sk->sk_type; newsk->sk_bound_dev_if = sk->sk_bound_dev_if; @@ -9457,9 +9456,9 @@ void sctp_copy_sock(struct sock *newsk, struct sock *sk, net_enable_timestamp(); /* Set newsk security attributes from original sk and connection - * security attribute from ep. + * security attribute from asoc. */ - security_sctp_sk_clone(ep, sk, newsk); + security_sctp_sk_clone(asoc, sk, newsk); } static inline void sctp_copy_descendant(struct sock *sk_to, diff --git a/security/security.c b/security/security.c index 9ffa9e9c5c55..b0f1c007aa3b 100644 --- a/security/security.c +++ b/security/security.c @@ -2366,9 +2366,9 @@ int security_tun_dev_open(void *security) } EXPORT_SYMBOL(security_tun_dev_open); -int security_sctp_assoc_request(struct sctp_endpoint *ep, struct sk_buff *skb) +int security_sctp_assoc_request(struct sctp_association *asoc, struct sk_buff *skb) { - return call_int_hook(sctp_assoc_request, 0, ep, skb); + return call_int_hook(sctp_assoc_request, 0, asoc, skb); } EXPORT_SYMBOL(security_sctp_assoc_request); @@ -2380,10 +2380,10 @@ int security_sctp_bind_connect(struct sock *sk, int optname, } EXPORT_SYMBOL(security_sctp_bind_connect); -void security_sctp_sk_clone(struct sctp_endpoint *ep, struct sock *sk, +void security_sctp_sk_clone(struct sctp_association *asoc, struct sock *sk, struct sock *newsk) { - call_void_hook(sctp_sk_clone, ep, sk, newsk); + call_void_hook(sctp_sk_clone, asoc, sk, newsk); } EXPORT_SYMBOL(security_sctp_sk_clone); diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c index e7ebd45ca345..a9977a2ae8ac 100644 --- a/security/selinux/hooks.c +++ b/security/selinux/hooks.c @@ -5356,10 +5356,10 @@ static void selinux_sock_graft(struct sock *sk, struct socket *parent) * connect(2), sctp_connectx(3) or sctp_sendmsg(3) (with no association * already present). */ -static int selinux_sctp_assoc_request(struct sctp_endpoint *ep, +static int selinux_sctp_assoc_request(struct sctp_association *asoc, struct sk_buff *skb) { - struct sk_security_struct *sksec = ep->base.sk->sk_security; + struct sk_security_struct *sksec = asoc->base.sk->sk_security; struct common_audit_data ad; struct lsm_network_audit net = {0,}; u8 peerlbl_active; @@ -5376,7 +5376,7 @@ static int selinux_sctp_assoc_request(struct sctp_endpoint *ep, /* This will return peer_sid = SECSID_NULL if there are * no peer labels, see security_net_peersid_resolve(). */ - err = selinux_skb_peerlbl_sid(skb, ep->base.sk->sk_family, + err = selinux_skb_peerlbl_sid(skb, asoc->base.sk->sk_family, &peer_sid); if (err) return err; @@ -5400,7 +5400,7 @@ static int selinux_sctp_assoc_request(struct sctp_endpoint *ep, */ ad.type = LSM_AUDIT_DATA_NET; ad.u.net = &net; - ad.u.net->sk = ep->base.sk; + ad.u.net->sk = asoc->base.sk; err = avc_has_perm(&selinux_state, sksec->peer_sid, peer_sid, sksec->sclass, SCTP_SOCKET__ASSOCIATION, &ad); @@ -5409,7 +5409,7 @@ static int selinux_sctp_assoc_request(struct sctp_endpoint *ep, } /* Compute the MLS component for the connection and store - * the information in ep. This will be used by SCTP TCP type + * the information in asoc. This will be used by SCTP TCP type * sockets and peeled off connections as they cause a new * socket to be generated. selinux_sctp_sk_clone() will then * plug this into the new socket. @@ -5418,11 +5418,11 @@ static int selinux_sctp_assoc_request(struct sctp_endpoint *ep, if (err) return err; - ep->secid = conn_sid; - ep->peer_secid = peer_sid; + asoc->secid = conn_sid; + asoc->peer_secid = peer_sid; /* Set any NetLabel labels including CIPSO/CALIPSO options. */ - return selinux_netlbl_sctp_assoc_request(ep, skb); + return selinux_netlbl_sctp_assoc_request(asoc, skb); } /* Check if sctp IPv4/IPv6 addresses are valid for binding or connecting @@ -5507,7 +5507,7 @@ static int selinux_sctp_bind_connect(struct sock *sk, int optname, } /* Called whenever a new socket is created by accept(2) or sctp_peeloff(3). */ -static void selinux_sctp_sk_clone(struct sctp_endpoint *ep, struct sock *sk, +static void selinux_sctp_sk_clone(struct sctp_association *asoc, struct sock *sk, struct sock *newsk) { struct sk_security_struct *sksec = sk->sk_security; @@ -5519,8 +5519,8 @@ static void selinux_sctp_sk_clone(struct sctp_endpoint *ep, struct sock *sk, if (!selinux_policycap_extsockclass()) return selinux_sk_clone_security(sk, newsk); - newsksec->sid = ep->secid; - newsksec->peer_sid = ep->peer_secid; + newsksec->sid = asoc->secid; + newsksec->peer_sid = asoc->peer_secid; newsksec->sclass = sksec->sclass; selinux_netlbl_sctp_sk_clone(sk, newsk); } diff --git a/security/selinux/include/netlabel.h b/security/selinux/include/netlabel.h index 0c58f62dc6ab..4d0456d3d459 100644 --- a/security/selinux/include/netlabel.h +++ b/security/selinux/include/netlabel.h @@ -39,7 +39,7 @@ int selinux_netlbl_skbuff_getsid(struct sk_buff *skb, int selinux_netlbl_skbuff_setsid(struct sk_buff *skb, u16 family, u32 sid); -int selinux_netlbl_sctp_assoc_request(struct sctp_endpoint *ep, +int selinux_netlbl_sctp_assoc_request(struct sctp_association *asoc, struct sk_buff *skb); int selinux_netlbl_inet_conn_request(struct request_sock *req, u16 family); void selinux_netlbl_inet_csk_clone(struct sock *sk, u16 family); @@ -98,7 +98,7 @@ static inline int selinux_netlbl_skbuff_setsid(struct sk_buff *skb, return 0; } -static inline int selinux_netlbl_sctp_assoc_request(struct sctp_endpoint *ep, +static inline int selinux_netlbl_sctp_assoc_request(struct sctp_association *asoc, struct sk_buff *skb) { return 0; diff --git a/security/selinux/netlabel.c b/security/selinux/netlabel.c index abaab7683840..1e0b63848751 100644 --- a/security/selinux/netlabel.c +++ b/security/selinux/netlabel.c @@ -260,30 +260,30 @@ int selinux_netlbl_skbuff_setsid(struct sk_buff *skb, /** * selinux_netlbl_sctp_assoc_request - Label an incoming sctp association. - * @ep: incoming association endpoint. + * @asoc: incoming association. * @skb: the packet. * * Description: - * A new incoming connection is represented by @ep, ...... + * A new incoming connection is represented by @asoc, ...... * Returns zero on success, negative values on failure. * */ -int selinux_netlbl_sctp_assoc_request(struct sctp_endpoint *ep, +int selinux_netlbl_sctp_assoc_request(struct sctp_association *asoc, struct sk_buff *skb) { int rc; struct netlbl_lsm_secattr secattr; - struct sk_security_struct *sksec = ep->base.sk->sk_security; + struct sk_security_struct *sksec = asoc->base.sk->sk_security; struct sockaddr_in addr4; struct sockaddr_in6 addr6; - if (ep->base.sk->sk_family != PF_INET && - ep->base.sk->sk_family != PF_INET6) + if (asoc->base.sk->sk_family != PF_INET && + asoc->base.sk->sk_family != PF_INET6) return 0; netlbl_secattr_init(&secattr); rc = security_netlbl_sid_to_secattr(&selinux_state, - ep->secid, &secattr); + asoc->secid, &secattr); if (rc != 0) goto assoc_request_return; @@ -293,11 +293,11 @@ int selinux_netlbl_sctp_assoc_request(struct sctp_endpoint *ep, if (ip_hdr(skb)->version == 4) { addr4.sin_family = AF_INET; addr4.sin_addr.s_addr = ip_hdr(skb)->saddr; - rc = netlbl_conn_setattr(ep->base.sk, (void *)&addr4, &secattr); + rc = netlbl_conn_setattr(asoc->base.sk, (void *)&addr4, &secattr); } else if (IS_ENABLED(CONFIG_IPV6) && ip_hdr(skb)->version == 6) { addr6.sin6_family = AF_INET6; addr6.sin6_addr = ipv6_hdr(skb)->saddr; - rc = netlbl_conn_setattr(ep->base.sk, (void *)&addr6, &secattr); + rc = netlbl_conn_setattr(asoc->base.sk, (void *)&addr6, &secattr); } else { rc = -EAFNOSUPPORT; } -- 2.27.0 ^ permalink raw reply related [flat|nested] 20+ messages in thread
* [PATCHv2 net 2/4] security: call security_sctp_assoc_request in sctp_sf_do_5_1D_ce 2021-11-02 12:02 [PATCHv2 net 0/4] security: fixups for the security hooks in sctp Xin Long 2021-11-02 12:02 ` [PATCHv2 net 1/4] security: pass asoc to sctp_assoc_request and sctp_sk_clone Xin Long @ 2021-11-02 12:02 ` Xin Long 2021-11-02 12:02 ` [PATCHv2 net 3/4] security: add sctp_assoc_established hook Xin Long ` (2 subsequent siblings) 4 siblings, 0 replies; 20+ messages in thread From: Xin Long @ 2021-11-02 12:02 UTC (permalink / raw) To: network dev, selinux, linux-security-module, linux-sctp Cc: davem, kuba, Marcelo Ricardo Leitner, James Morris, Paul Moore, Richard Haines, Ondrej Mosnacek The asoc created when receives the INIT chunk is a temporary one, it will be deleted after INIT_ACK chunk is replied. So for the real asoc created in sctp_sf_do_5_1D_ce() when the COOKIE_ECHO chunk is received, security_sctp_assoc_request() should also be called. v1->v2: - fix some typo and grammar errors, noticed by Ondrej. Fixes: 72e89f50084c ("security: Add support for SCTP security hooks") Reported-by: Prashanth Prahlad <pprahlad@redhat.com> Reviewed-by: Richard Haines <richard_c_haines@btinternet.com> Tested-by: Richard Haines <richard_c_haines@btinternet.com> Signed-off-by: Xin Long <lucien.xin@gmail.com> --- Documentation/security/SCTP.rst | 15 +++++++++------ net/sctp/sm_statefuns.c | 5 +++++ 2 files changed, 14 insertions(+), 6 deletions(-) diff --git a/Documentation/security/SCTP.rst b/Documentation/security/SCTP.rst index 415b548d9ce0..d5fd6ccc3dcb 100644 --- a/Documentation/security/SCTP.rst +++ b/Documentation/security/SCTP.rst @@ -151,9 +151,9 @@ establishing an association. INIT ---------------------------------------------> sctp_sf_do_5_1B_init() Respond to an INIT chunk. - SCTP peer endpoint "A" is - asking for an association. Call - security_sctp_assoc_request() + SCTP peer endpoint "A" is asking + for a temporary association. + Call security_sctp_assoc_request() to set the peer label if first association. If not first association, check @@ -163,9 +163,12 @@ establishing an association. | discard the packet. | COOKIE ECHO ------------------------------------------> - | - | - | + sctp_sf_do_5_1D_ce() + Respond to an COOKIE ECHO chunk. + Confirm the cookie and create a + permanent association. + Call security_sctp_assoc_request() to + do the same as for INIT chunk Response. <------------------------------------------- COOKIE ACK | | sctp_sf_do_5_1E_ca | diff --git a/net/sctp/sm_statefuns.c b/net/sctp/sm_statefuns.c index 3206374209bc..b818532c3fc2 100644 --- a/net/sctp/sm_statefuns.c +++ b/net/sctp/sm_statefuns.c @@ -781,6 +781,11 @@ enum sctp_disposition sctp_sf_do_5_1D_ce(struct net *net, } } + if (security_sctp_assoc_request(new_asoc, chunk->skb)) { + sctp_association_free(new_asoc); + return sctp_sf_pdiscard(net, ep, asoc, type, arg, commands); + } + /* Delay state machine commands until later. * * Re-build the bind address for the association is done in -- 2.27.0 ^ permalink raw reply related [flat|nested] 20+ messages in thread
* [PATCHv2 net 3/4] security: add sctp_assoc_established hook 2021-11-02 12:02 [PATCHv2 net 0/4] security: fixups for the security hooks in sctp Xin Long 2021-11-02 12:02 ` [PATCHv2 net 1/4] security: pass asoc to sctp_assoc_request and sctp_sk_clone Xin Long 2021-11-02 12:02 ` [PATCHv2 net 2/4] security: call security_sctp_assoc_request in sctp_sf_do_5_1D_ce Xin Long @ 2021-11-02 12:02 ` Xin Long 2021-11-02 12:02 ` [PATCHv2 net 4/4] security: implement sctp_assoc_established hook in selinux Xin Long 2021-11-03 11:20 ` [PATCHv2 net 0/4] security: fixups for the security hooks in sctp patchwork-bot+netdevbpf 4 siblings, 0 replies; 20+ messages in thread From: Xin Long @ 2021-11-02 12:02 UTC (permalink / raw) To: network dev, selinux, linux-security-module, linux-sctp Cc: davem, kuba, Marcelo Ricardo Leitner, James Morris, Paul Moore, Richard Haines, Ondrej Mosnacek security_sctp_assoc_established() is added to replace security_inet_conn_established() called in sctp_sf_do_5_1E_ca(), so that asoc can be accessed in security subsystem and save the peer secid to asoc->peer_secid. v1->v2: - fix the return value of security_sctp_assoc_established() in security.h, found by kernel test robot and Ondrej. Fixes: 72e89f50084c ("security: Add support for SCTP security hooks") Reported-by: Prashanth Prahlad <pprahlad@redhat.com> Reviewed-by: Richard Haines <richard_c_haines@btinternet.com> Tested-by: Richard Haines <richard_c_haines@btinternet.com> Signed-off-by: Xin Long <lucien.xin@gmail.com> --- Documentation/security/SCTP.rst | 22 ++++++++++------------ include/linux/lsm_hook_defs.h | 2 ++ include/linux/lsm_hooks.h | 5 +++++ include/linux/security.h | 7 +++++++ net/sctp/sm_statefuns.c | 2 +- security/security.c | 7 +++++++ 6 files changed, 32 insertions(+), 13 deletions(-) diff --git a/Documentation/security/SCTP.rst b/Documentation/security/SCTP.rst index d5fd6ccc3dcb..406cc68b8808 100644 --- a/Documentation/security/SCTP.rst +++ b/Documentation/security/SCTP.rst @@ -15,10 +15,7 @@ For security module support, three SCTP specific hooks have been implemented:: security_sctp_assoc_request() security_sctp_bind_connect() security_sctp_sk_clone() - -Also the following security hook has been utilised:: - - security_inet_conn_established() + security_sctp_assoc_established() The usage of these hooks are described below with the SELinux implementation described in the `SCTP SELinux Support`_ chapter. @@ -122,11 +119,12 @@ calls **sctp_peeloff**\(3). @newsk - pointer to new sock structure. -security_inet_conn_established() +security_sctp_assoc_established() ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ -Called when a COOKIE ACK is received:: +Called when a COOKIE ACK is received, and the peer secid will be +saved into ``@asoc->peer_secid`` for client:: - @sk - pointer to sock structure. + @asoc - pointer to sctp association structure. @skb - pointer to skbuff of the COOKIE ACK packet. @@ -134,7 +132,7 @@ Security Hooks used for Association Establishment ------------------------------------------------- The following diagram shows the use of ``security_sctp_bind_connect()``, -``security_sctp_assoc_request()``, ``security_inet_conn_established()`` when +``security_sctp_assoc_request()``, ``security_sctp_assoc_established()`` when establishing an association. :: @@ -172,7 +170,7 @@ establishing an association. <------------------------------------------- COOKIE ACK | | sctp_sf_do_5_1E_ca | - Call security_inet_conn_established() | + Call security_sctp_assoc_established() | to set the peer label. | | | | If SCTP_SOCKET_TCP or peeled off @@ -198,7 +196,7 @@ hooks with the SELinux specifics expanded below:: security_sctp_assoc_request() security_sctp_bind_connect() security_sctp_sk_clone() - security_inet_conn_established() + security_sctp_assoc_established() security_sctp_assoc_request() @@ -271,12 +269,12 @@ sockets sid and peer sid to that contained in the ``@asoc sid`` and @newsk - pointer to new sock structure. -security_inet_conn_established() +security_sctp_assoc_established() ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ Called when a COOKIE ACK is received where it sets the connection's peer sid to that in ``@skb``:: - @sk - pointer to sock structure. + @asoc - pointer to sctp association structure. @skb - pointer to skbuff of the COOKIE ACK packet. diff --git a/include/linux/lsm_hook_defs.h b/include/linux/lsm_hook_defs.h index 0024273a7382..e9870118cc67 100644 --- a/include/linux/lsm_hook_defs.h +++ b/include/linux/lsm_hook_defs.h @@ -334,6 +334,8 @@ LSM_HOOK(int, 0, sctp_bind_connect, struct sock *sk, int optname, struct sockaddr *address, int addrlen) LSM_HOOK(void, LSM_RET_VOID, sctp_sk_clone, struct sctp_association *asoc, struct sock *sk, struct sock *newsk) +LSM_HOOK(void, LSM_RET_VOID, sctp_assoc_established, struct sctp_association *asoc, + struct sk_buff *skb) #endif /* CONFIG_SECURITY_NETWORK */ #ifdef CONFIG_SECURITY_INFINIBAND diff --git a/include/linux/lsm_hooks.h b/include/linux/lsm_hooks.h index 240b92d89852..ba42c22204e2 100644 --- a/include/linux/lsm_hooks.h +++ b/include/linux/lsm_hooks.h @@ -1047,6 +1047,11 @@ * @asoc pointer to current sctp association structure. * @sk pointer to current sock structure. * @newsk pointer to new sock structure. + * @sctp_assoc_established: + * Passes the @asoc and @chunk->skb of the association COOKIE_ACK packet + * to the security module. + * @asoc pointer to sctp association structure. + * @skb pointer to skbuff of association packet. * * Security hooks for Infiniband * diff --git a/include/linux/security.h b/include/linux/security.h index a16407444871..c2ac6b15e50b 100644 --- a/include/linux/security.h +++ b/include/linux/security.h @@ -1423,6 +1423,8 @@ int security_sctp_bind_connect(struct sock *sk, int optname, struct sockaddr *address, int addrlen); void security_sctp_sk_clone(struct sctp_association *asoc, struct sock *sk, struct sock *newsk); +void security_sctp_assoc_established(struct sctp_association *asoc, + struct sk_buff *skb); #else /* CONFIG_SECURITY_NETWORK */ static inline int security_unix_stream_connect(struct sock *sock, @@ -1642,6 +1644,11 @@ static inline void security_sctp_sk_clone(struct sctp_association *asoc, struct sock *newsk) { } + +static inline void security_sctp_assoc_established(struct sctp_association *asoc, + struct sk_buff *skb) +{ +} #endif /* CONFIG_SECURITY_NETWORK */ #ifdef CONFIG_SECURITY_INFINIBAND diff --git a/net/sctp/sm_statefuns.c b/net/sctp/sm_statefuns.c index b818532c3fc2..5fabaa54b77d 100644 --- a/net/sctp/sm_statefuns.c +++ b/net/sctp/sm_statefuns.c @@ -946,7 +946,7 @@ enum sctp_disposition sctp_sf_do_5_1E_ca(struct net *net, sctp_add_cmd_sf(commands, SCTP_CMD_INIT_COUNTER_RESET, SCTP_NULL()); /* Set peer label for connection. */ - security_inet_conn_established(ep->base.sk, chunk->skb); + security_sctp_assoc_established((struct sctp_association *)asoc, chunk->skb); /* RFC 2960 5.1 Normal Establishment of an Association * diff --git a/security/security.c b/security/security.c index b0f1c007aa3b..4b2b4b5beb27 100644 --- a/security/security.c +++ b/security/security.c @@ -2387,6 +2387,13 @@ void security_sctp_sk_clone(struct sctp_association *asoc, struct sock *sk, } EXPORT_SYMBOL(security_sctp_sk_clone); +void security_sctp_assoc_established(struct sctp_association *asoc, + struct sk_buff *skb) +{ + call_void_hook(sctp_assoc_established, asoc, skb); +} +EXPORT_SYMBOL(security_sctp_assoc_established); + #endif /* CONFIG_SECURITY_NETWORK */ #ifdef CONFIG_SECURITY_INFINIBAND -- 2.27.0 ^ permalink raw reply related [flat|nested] 20+ messages in thread
* [PATCHv2 net 4/4] security: implement sctp_assoc_established hook in selinux 2021-11-02 12:02 [PATCHv2 net 0/4] security: fixups for the security hooks in sctp Xin Long ` (2 preceding siblings ...) 2021-11-02 12:02 ` [PATCHv2 net 3/4] security: add sctp_assoc_established hook Xin Long @ 2021-11-02 12:02 ` Xin Long 2021-11-03 16:40 ` Ondrej Mosnacek 2021-11-03 11:20 ` [PATCHv2 net 0/4] security: fixups for the security hooks in sctp patchwork-bot+netdevbpf 4 siblings, 1 reply; 20+ messages in thread From: Xin Long @ 2021-11-02 12:02 UTC (permalink / raw) To: network dev, selinux, linux-security-module, linux-sctp Cc: davem, kuba, Marcelo Ricardo Leitner, James Morris, Paul Moore, Richard Haines, Ondrej Mosnacek Different from selinux_inet_conn_established(), it also gives the secid to asoc->peer_secid in selinux_sctp_assoc_established(), as one UDP-type socket may have more than one asocs. Note that peer_secid in asoc will save the peer secid for this asoc connection, and peer_sid in sksec will just keep the peer secid for the latest connection. So the right use should be do peeloff for UDP-type socket if there will be multiple asocs in one socket, so that the peeloff socket has the right label for its asoc. v1->v2: - call selinux_inet_conn_established() to reduce some code duplication in selinux_sctp_assoc_established(), as Ondrej suggested. - when doing peeloff, it calls sock_create() where it actually gets secid for socket from socket_sockcreate_sid(). So reuse SECSID_WILD to ensure the peeloff socket keeps using that secid after calling selinux_sctp_sk_clone() for client side. Fixes: 72e89f50084c ("security: Add support for SCTP security hooks") Reported-by: Prashanth Prahlad <pprahlad@redhat.com> Reviewed-by: Richard Haines <richard_c_haines@btinternet.com> Tested-by: Richard Haines <richard_c_haines@btinternet.com> Signed-off-by: Xin Long <lucien.xin@gmail.com> --- security/selinux/hooks.c | 14 +++++++++++++- 1 file changed, 13 insertions(+), 1 deletion(-) diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c index a9977a2ae8ac..341cd5dccbf5 100644 --- a/security/selinux/hooks.c +++ b/security/selinux/hooks.c @@ -5519,7 +5519,8 @@ static void selinux_sctp_sk_clone(struct sctp_association *asoc, struct sock *sk if (!selinux_policycap_extsockclass()) return selinux_sk_clone_security(sk, newsk); - newsksec->sid = asoc->secid; + if (asoc->secid != SECSID_WILD) + newsksec->sid = asoc->secid; newsksec->peer_sid = asoc->peer_secid; newsksec->sclass = sksec->sclass; selinux_netlbl_sctp_sk_clone(sk, newsk); @@ -5575,6 +5576,16 @@ static void selinux_inet_conn_established(struct sock *sk, struct sk_buff *skb) selinux_skb_peerlbl_sid(skb, family, &sksec->peer_sid); } +static void selinux_sctp_assoc_established(struct sctp_association *asoc, + struct sk_buff *skb) +{ + struct sk_security_struct *sksec = asoc->base.sk->sk_security; + + selinux_inet_conn_established(asoc->base.sk, skb); + asoc->peer_secid = sksec->peer_sid; + asoc->secid = SECSID_WILD; +} + static int selinux_secmark_relabel_packet(u32 sid) { const struct task_security_struct *__tsec; @@ -7290,6 +7301,7 @@ static struct security_hook_list selinux_hooks[] __lsm_ro_after_init = { LSM_HOOK_INIT(sctp_assoc_request, selinux_sctp_assoc_request), LSM_HOOK_INIT(sctp_sk_clone, selinux_sctp_sk_clone), LSM_HOOK_INIT(sctp_bind_connect, selinux_sctp_bind_connect), + LSM_HOOK_INIT(sctp_assoc_established, selinux_sctp_assoc_established), LSM_HOOK_INIT(inet_conn_request, selinux_inet_conn_request), LSM_HOOK_INIT(inet_csk_clone, selinux_inet_csk_clone), LSM_HOOK_INIT(inet_conn_established, selinux_inet_conn_established), -- 2.27.0 ^ permalink raw reply related [flat|nested] 20+ messages in thread
* Re: [PATCHv2 net 4/4] security: implement sctp_assoc_established hook in selinux 2021-11-02 12:02 ` [PATCHv2 net 4/4] security: implement sctp_assoc_established hook in selinux Xin Long @ 2021-11-03 16:40 ` Ondrej Mosnacek 2021-11-03 17:33 ` Xin Long 0 siblings, 1 reply; 20+ messages in thread From: Ondrej Mosnacek @ 2021-11-03 16:40 UTC (permalink / raw) To: Xin Long Cc: network dev, SElinux list, Linux Security Module list, linux-sctp @ vger . kernel . org, David S. Miller, Jakub Kicinski, Marcelo Ricardo Leitner, James Morris, Paul Moore, Richard Haines Hi Xin, On Tue, Nov 2, 2021 at 1:03 PM Xin Long <lucien.xin@gmail.com> wrote: > > Different from selinux_inet_conn_established(), it also gives the > secid to asoc->peer_secid in selinux_sctp_assoc_established(), > as one UDP-type socket may have more than one asocs. > > Note that peer_secid in asoc will save the peer secid for this > asoc connection, and peer_sid in sksec will just keep the peer > secid for the latest connection. So the right use should be do > peeloff for UDP-type socket if there will be multiple asocs in > one socket, so that the peeloff socket has the right label for > its asoc. > > v1->v2: > - call selinux_inet_conn_established() to reduce some code > duplication in selinux_sctp_assoc_established(), as Ondrej > suggested. > - when doing peeloff, it calls sock_create() where it actually > gets secid for socket from socket_sockcreate_sid(). So reuse > SECSID_WILD to ensure the peeloff socket keeps using that > secid after calling selinux_sctp_sk_clone() for client side. Interesting... I find strange that SCTP creates the peeloff socket using sock_create() rather than allocating it directly via sock_alloc() like the other callers of sctp_copy_sock() (which calls security_sctp_sk_clone()) do. Wouldn't it make more sense to avoid the sock_create() call and just rely on the security_sctp_sk_clone() semantic to set up the labels? Would anything break if sctp_do_peeloff() switched to plain sock_alloc()? I'd rather we avoid this SECSID_WILD hack to support the weird created-but-also-cloned socket hybrid and just make the peeloff socket behave the same as an accept()-ed socket (i.e. no security_socket_[post_]create() hook calls, just security_sctp_sk_clone()). > > Fixes: 72e89f50084c ("security: Add support for SCTP security hooks") > Reported-by: Prashanth Prahlad <pprahlad@redhat.com> > Reviewed-by: Richard Haines <richard_c_haines@btinternet.com> > Tested-by: Richard Haines <richard_c_haines@btinternet.com> You made non-trivial changes since the last revision in this patch, so you should have also dropped the Reviewed-by and Tested-by here. Now David has merged the patches probably under the impression that they have been reviewed/approved from the SELinux side, which isn't completely true. > Signed-off-by: Xin Long <lucien.xin@gmail.com> > --- > security/selinux/hooks.c | 14 +++++++++++++- > 1 file changed, 13 insertions(+), 1 deletion(-) > > diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c > index a9977a2ae8ac..341cd5dccbf5 100644 > --- a/security/selinux/hooks.c > +++ b/security/selinux/hooks.c > @@ -5519,7 +5519,8 @@ static void selinux_sctp_sk_clone(struct sctp_association *asoc, struct sock *sk > if (!selinux_policycap_extsockclass()) > return selinux_sk_clone_security(sk, newsk); > > - newsksec->sid = asoc->secid; > + if (asoc->secid != SECSID_WILD) > + newsksec->sid = asoc->secid; > newsksec->peer_sid = asoc->peer_secid; > newsksec->sclass = sksec->sclass; > selinux_netlbl_sctp_sk_clone(sk, newsk); > @@ -5575,6 +5576,16 @@ static void selinux_inet_conn_established(struct sock *sk, struct sk_buff *skb) > selinux_skb_peerlbl_sid(skb, family, &sksec->peer_sid); > } > > +static void selinux_sctp_assoc_established(struct sctp_association *asoc, > + struct sk_buff *skb) > +{ > + struct sk_security_struct *sksec = asoc->base.sk->sk_security; > + > + selinux_inet_conn_established(asoc->base.sk, skb); > + asoc->peer_secid = sksec->peer_sid; > + asoc->secid = SECSID_WILD; > +} > + > static int selinux_secmark_relabel_packet(u32 sid) > { > const struct task_security_struct *__tsec; > @@ -7290,6 +7301,7 @@ static struct security_hook_list selinux_hooks[] __lsm_ro_after_init = { > LSM_HOOK_INIT(sctp_assoc_request, selinux_sctp_assoc_request), > LSM_HOOK_INIT(sctp_sk_clone, selinux_sctp_sk_clone), > LSM_HOOK_INIT(sctp_bind_connect, selinux_sctp_bind_connect), > + LSM_HOOK_INIT(sctp_assoc_established, selinux_sctp_assoc_established), > LSM_HOOK_INIT(inet_conn_request, selinux_inet_conn_request), > LSM_HOOK_INIT(inet_csk_clone, selinux_inet_csk_clone), > LSM_HOOK_INIT(inet_conn_established, selinux_inet_conn_established), > -- > 2.27.0 > -- Ondrej Mosnacek Software Engineer, Linux Security - SELinux kernel Red Hat, Inc. ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCHv2 net 4/4] security: implement sctp_assoc_established hook in selinux 2021-11-03 16:40 ` Ondrej Mosnacek @ 2021-11-03 17:33 ` Xin Long 2021-11-03 17:36 ` Xin Long 0 siblings, 1 reply; 20+ messages in thread From: Xin Long @ 2021-11-03 17:33 UTC (permalink / raw) To: Ondrej Mosnacek Cc: network dev, SElinux list, Linux Security Module list, linux-sctp @ vger . kernel . org, David S. Miller, Jakub Kicinski, Marcelo Ricardo Leitner, James Morris, Paul Moore, Richard Haines On Wed, Nov 3, 2021 at 12:40 PM Ondrej Mosnacek <omosnace@redhat.com> wrote: > > Hi Xin, > > On Tue, Nov 2, 2021 at 1:03 PM Xin Long <lucien.xin@gmail.com> wrote: > > > > Different from selinux_inet_conn_established(), it also gives the > > secid to asoc->peer_secid in selinux_sctp_assoc_established(), > > as one UDP-type socket may have more than one asocs. > > > > Note that peer_secid in asoc will save the peer secid for this > > asoc connection, and peer_sid in sksec will just keep the peer > > secid for the latest connection. So the right use should be do > > peeloff for UDP-type socket if there will be multiple asocs in > > one socket, so that the peeloff socket has the right label for > > its asoc. > > > > v1->v2: > > - call selinux_inet_conn_established() to reduce some code > > duplication in selinux_sctp_assoc_established(), as Ondrej > > suggested. > > - when doing peeloff, it calls sock_create() where it actually > > gets secid for socket from socket_sockcreate_sid(). So reuse > > SECSID_WILD to ensure the peeloff socket keeps using that > > secid after calling selinux_sctp_sk_clone() for client side. > > Interesting... I find strange that SCTP creates the peeloff socket > using sock_create() rather than allocating it directly via > sock_alloc() like the other callers of sctp_copy_sock() (which calls > security_sctp_sk_clone()) do. Wouldn't it make more sense to avoid the > sock_create() call and just rely on the security_sctp_sk_clone() > semantic to set up the labels? Would anything break if > sctp_do_peeloff() switched to plain sock_alloc()? > > I'd rather we avoid this SECSID_WILD hack to support the weird > created-but-also-cloned socket hybrid and just make the peeloff socket > behave the same as an accept()-ed socket (i.e. no > security_socket_[post_]create() hook calls, just > security_sctp_sk_clone()). please check Paul's comment: """ The initial SCTP client association would need to take it's label from the parent process so perhaps that is the right answer for all SCTP client associations[2]. [1] I would expect server side associations to follow the more complicated selinux_conn_sid() labeling, just as we do for TCP/stream connections today. [2] I'm guessing the client associations might also want to follow the setsockcreatecon(3) behavior, see selinux_sockcreate_sid() for more info. """ That's what I got from it: For client side, secid should be copied from its parent socket directly, but get it from socket_sockcreate_sid(). and you? > > > > > Fixes: 72e89f50084c ("security: Add support for SCTP security hooks") > > Reported-by: Prashanth Prahlad <pprahlad@redhat.com> > > Reviewed-by: Richard Haines <richard_c_haines@btinternet.com> > > Tested-by: Richard Haines <richard_c_haines@btinternet.com> > > You made non-trivial changes since the last revision in this patch, so > you should have also dropped the Reviewed-by and Tested-by here. Now > David has merged the patches probably under the impression that they > have been reviewed/approved from the SELinux side, which isn't > completely true. Oh, that's a mistake, I thought I didn't add it. Will he be able to test this new patchset? Thanks. > > > Signed-off-by: Xin Long <lucien.xin@gmail.com> > > --- > > security/selinux/hooks.c | 14 +++++++++++++- > > 1 file changed, 13 insertions(+), 1 deletion(-) > > > > diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c > > index a9977a2ae8ac..341cd5dccbf5 100644 > > --- a/security/selinux/hooks.c > > +++ b/security/selinux/hooks.c > > @@ -5519,7 +5519,8 @@ static void selinux_sctp_sk_clone(struct sctp_association *asoc, struct sock *sk > > if (!selinux_policycap_extsockclass()) > > return selinux_sk_clone_security(sk, newsk); > > > > - newsksec->sid = asoc->secid; > > + if (asoc->secid != SECSID_WILD) > > + newsksec->sid = asoc->secid; > > newsksec->peer_sid = asoc->peer_secid; > > newsksec->sclass = sksec->sclass; > > selinux_netlbl_sctp_sk_clone(sk, newsk); > > @@ -5575,6 +5576,16 @@ static void selinux_inet_conn_established(struct sock *sk, struct sk_buff *skb) > > selinux_skb_peerlbl_sid(skb, family, &sksec->peer_sid); > > } > > > > +static void selinux_sctp_assoc_established(struct sctp_association *asoc, > > + struct sk_buff *skb) > > +{ > > + struct sk_security_struct *sksec = asoc->base.sk->sk_security; > > + > > + selinux_inet_conn_established(asoc->base.sk, skb); > > + asoc->peer_secid = sksec->peer_sid; > > + asoc->secid = SECSID_WILD; > > +} > > + > > static int selinux_secmark_relabel_packet(u32 sid) > > { > > const struct task_security_struct *__tsec; > > @@ -7290,6 +7301,7 @@ static struct security_hook_list selinux_hooks[] __lsm_ro_after_init = { > > LSM_HOOK_INIT(sctp_assoc_request, selinux_sctp_assoc_request), > > LSM_HOOK_INIT(sctp_sk_clone, selinux_sctp_sk_clone), > > LSM_HOOK_INIT(sctp_bind_connect, selinux_sctp_bind_connect), > > + LSM_HOOK_INIT(sctp_assoc_established, selinux_sctp_assoc_established), > > LSM_HOOK_INIT(inet_conn_request, selinux_inet_conn_request), > > LSM_HOOK_INIT(inet_csk_clone, selinux_inet_csk_clone), > > LSM_HOOK_INIT(inet_conn_established, selinux_inet_conn_established), > > -- > > 2.27.0 > > > > -- > Ondrej Mosnacek > Software Engineer, Linux Security - SELinux kernel > Red Hat, Inc. > ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCHv2 net 4/4] security: implement sctp_assoc_established hook in selinux 2021-11-03 17:33 ` Xin Long @ 2021-11-03 17:36 ` Xin Long 2021-11-03 22:01 ` Paul Moore 0 siblings, 1 reply; 20+ messages in thread From: Xin Long @ 2021-11-03 17:36 UTC (permalink / raw) To: Ondrej Mosnacek Cc: network dev, SElinux list, Linux Security Module list, linux-sctp @ vger . kernel . org, David S. Miller, Jakub Kicinski, Marcelo Ricardo Leitner, James Morris, Paul Moore, Richard Haines On Wed, Nov 3, 2021 at 1:33 PM Xin Long <lucien.xin@gmail.com> wrote: > > On Wed, Nov 3, 2021 at 12:40 PM Ondrej Mosnacek <omosnace@redhat.com> wrote: > > > > Hi Xin, > > > > On Tue, Nov 2, 2021 at 1:03 PM Xin Long <lucien.xin@gmail.com> wrote: > > > > > > Different from selinux_inet_conn_established(), it also gives the > > > secid to asoc->peer_secid in selinux_sctp_assoc_established(), > > > as one UDP-type socket may have more than one asocs. > > > > > > Note that peer_secid in asoc will save the peer secid for this > > > asoc connection, and peer_sid in sksec will just keep the peer > > > secid for the latest connection. So the right use should be do > > > peeloff for UDP-type socket if there will be multiple asocs in > > > one socket, so that the peeloff socket has the right label for > > > its asoc. > > > > > > v1->v2: > > > - call selinux_inet_conn_established() to reduce some code > > > duplication in selinux_sctp_assoc_established(), as Ondrej > > > suggested. > > > - when doing peeloff, it calls sock_create() where it actually > > > gets secid for socket from socket_sockcreate_sid(). So reuse > > > SECSID_WILD to ensure the peeloff socket keeps using that > > > secid after calling selinux_sctp_sk_clone() for client side. > > > > Interesting... I find strange that SCTP creates the peeloff socket > > using sock_create() rather than allocating it directly via > > sock_alloc() like the other callers of sctp_copy_sock() (which calls > > security_sctp_sk_clone()) do. Wouldn't it make more sense to avoid the > > sock_create() call and just rely on the security_sctp_sk_clone() > > semantic to set up the labels? Would anything break if > > sctp_do_peeloff() switched to plain sock_alloc()? > > > > I'd rather we avoid this SECSID_WILD hack to support the weird > > created-but-also-cloned socket hybrid and just make the peeloff socket > > behave the same as an accept()-ed socket (i.e. no > > security_socket_[post_]create() hook calls, just > > security_sctp_sk_clone()). > > please check Paul's comment: > > """ > The initial SCTP client association would > need to take it's label from the parent process so perhaps that is the > right answer for all SCTP client associations[2]. > > [1] I would expect server side associations to follow the more > complicated selinux_conn_sid() labeling, just as we do for TCP/stream > connections today. > > [2] I'm guessing the client associations might also want to follow the > setsockcreatecon(3) behavior, see selinux_sockcreate_sid() for more > info. > """ > > That's what I got from it: > For client side, secid should be copied from its parent socket directly, but > get it from socket_sockcreate_sid(). For client side, secid should NOT be copied from its parent socket directly, but gets it from socket_sockcreate_sid(). > > and you? > > > > > > > > > Fixes: 72e89f50084c ("security: Add support for SCTP security hooks") > > > Reported-by: Prashanth Prahlad <pprahlad@redhat.com> > > > Reviewed-by: Richard Haines <richard_c_haines@btinternet.com> > > > Tested-by: Richard Haines <richard_c_haines@btinternet.com> > > > > You made non-trivial changes since the last revision in this patch, so > > you should have also dropped the Reviewed-by and Tested-by here. Now > > David has merged the patches probably under the impression that they > > have been reviewed/approved from the SELinux side, which isn't > > completely true. > Oh, that's a mistake, I thought I didn't add it. > Will he be able to test this new patchset? > > Thanks. > > > > > > Signed-off-by: Xin Long <lucien.xin@gmail.com> > > > --- > > > security/selinux/hooks.c | 14 +++++++++++++- > > > 1 file changed, 13 insertions(+), 1 deletion(-) > > > > > > diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c > > > index a9977a2ae8ac..341cd5dccbf5 100644 > > > --- a/security/selinux/hooks.c > > > +++ b/security/selinux/hooks.c > > > @@ -5519,7 +5519,8 @@ static void selinux_sctp_sk_clone(struct sctp_association *asoc, struct sock *sk > > > if (!selinux_policycap_extsockclass()) > > > return selinux_sk_clone_security(sk, newsk); > > > > > > - newsksec->sid = asoc->secid; > > > + if (asoc->secid != SECSID_WILD) > > > + newsksec->sid = asoc->secid; > > > newsksec->peer_sid = asoc->peer_secid; > > > newsksec->sclass = sksec->sclass; > > > selinux_netlbl_sctp_sk_clone(sk, newsk); > > > @@ -5575,6 +5576,16 @@ static void selinux_inet_conn_established(struct sock *sk, struct sk_buff *skb) > > > selinux_skb_peerlbl_sid(skb, family, &sksec->peer_sid); > > > } > > > > > > +static void selinux_sctp_assoc_established(struct sctp_association *asoc, > > > + struct sk_buff *skb) > > > +{ > > > + struct sk_security_struct *sksec = asoc->base.sk->sk_security; > > > + > > > + selinux_inet_conn_established(asoc->base.sk, skb); > > > + asoc->peer_secid = sksec->peer_sid; > > > + asoc->secid = SECSID_WILD; > > > +} > > > + > > > static int selinux_secmark_relabel_packet(u32 sid) > > > { > > > const struct task_security_struct *__tsec; > > > @@ -7290,6 +7301,7 @@ static struct security_hook_list selinux_hooks[] __lsm_ro_after_init = { > > > LSM_HOOK_INIT(sctp_assoc_request, selinux_sctp_assoc_request), > > > LSM_HOOK_INIT(sctp_sk_clone, selinux_sctp_sk_clone), > > > LSM_HOOK_INIT(sctp_bind_connect, selinux_sctp_bind_connect), > > > + LSM_HOOK_INIT(sctp_assoc_established, selinux_sctp_assoc_established), > > > LSM_HOOK_INIT(inet_conn_request, selinux_inet_conn_request), > > > LSM_HOOK_INIT(inet_csk_clone, selinux_inet_csk_clone), > > > LSM_HOOK_INIT(inet_conn_established, selinux_inet_conn_established), > > > -- > > > 2.27.0 > > > > > > > -- > > Ondrej Mosnacek > > Software Engineer, Linux Security - SELinux kernel > > Red Hat, Inc. > > ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCHv2 net 4/4] security: implement sctp_assoc_established hook in selinux 2021-11-03 17:36 ` Xin Long @ 2021-11-03 22:01 ` Paul Moore 2021-11-04 1:46 ` Xin Long 0 siblings, 1 reply; 20+ messages in thread From: Paul Moore @ 2021-11-03 22:01 UTC (permalink / raw) To: Xin Long Cc: Ondrej Mosnacek, network dev, SElinux list, Linux Security Module list, linux-sctp @ vger . kernel . org, David S. Miller, Jakub Kicinski, Marcelo Ricardo Leitner, James Morris, Richard Haines On Wed, Nov 3, 2021 at 1:36 PM Xin Long <lucien.xin@gmail.com> wrote: > On Wed, Nov 3, 2021 at 1:33 PM Xin Long <lucien.xin@gmail.com> wrote: > > On Wed, Nov 3, 2021 at 12:40 PM Ondrej Mosnacek <omosnace@redhat.com> wrote: > > > On Tue, Nov 2, 2021 at 1:03 PM Xin Long <lucien.xin@gmail.com> wrote: > > > > > > > > Different from selinux_inet_conn_established(), it also gives the > > > > secid to asoc->peer_secid in selinux_sctp_assoc_established(), > > > > as one UDP-type socket may have more than one asocs. > > > > > > > > Note that peer_secid in asoc will save the peer secid for this > > > > asoc connection, and peer_sid in sksec will just keep the peer > > > > secid for the latest connection. So the right use should be do > > > > peeloff for UDP-type socket if there will be multiple asocs in > > > > one socket, so that the peeloff socket has the right label for > > > > its asoc. > > > > > > > > v1->v2: > > > > - call selinux_inet_conn_established() to reduce some code > > > > duplication in selinux_sctp_assoc_established(), as Ondrej > > > > suggested. > > > > - when doing peeloff, it calls sock_create() where it actually > > > > gets secid for socket from socket_sockcreate_sid(). So reuse > > > > SECSID_WILD to ensure the peeloff socket keeps using that > > > > secid after calling selinux_sctp_sk_clone() for client side. > > > > > > Interesting... I find strange that SCTP creates the peeloff socket > > > using sock_create() rather than allocating it directly via > > > sock_alloc() like the other callers of sctp_copy_sock() (which calls > > > security_sctp_sk_clone()) do. Wouldn't it make more sense to avoid the > > > sock_create() call and just rely on the security_sctp_sk_clone() > > > semantic to set up the labels? Would anything break if > > > sctp_do_peeloff() switched to plain sock_alloc()? > > > > > > I'd rather we avoid this SECSID_WILD hack to support the weird > > > created-but-also-cloned socket hybrid and just make the peeloff socket > > > behave the same as an accept()-ed socket (i.e. no > > > security_socket_[post_]create() hook calls, just > > > security_sctp_sk_clone()). I believe the important part is that sctp_do_peeloff() eventually calls security_sctp_sk_clone() via way of sctp_copy_sock(). Assuming we have security_sctp_sk_clone() working properly I would expect that the new socket would be setup properly when sctp_do_peeloff() returns on success. ... and yes, that SECSID_WILD approach is *not* something we want to do. In my mind, selinux_sctp_sk_clone() should end up looking like this. void selinux_sctp_sk_clone(asoc, sk, newsk) { struct sk_security_struct sksec = sk->sk_security; struct sk_security_struct newsksec = newsk->sk_security; if (!selinux_policycap_extsockclass()) return selinux_sk_clone_security(sk, newsk); newsksec->sid = sksec->secid; newsksec->peer_sid = asoc->peer_secid; newsksec->sclass = sksec->sclass; selinux_netlbl_sctp_sk_clone(sk, newsk); } Also, to be clear, the "assoc->secid = SECSID_WILD;" line should be removed from selinux_sctp_assoc_established(). If we are treating SCTP associations similarly to TCP connections, the association's label/secid should be set once and not changed during the life of the association. > > > > Fixes: 72e89f50084c ("security: Add support for SCTP security hooks") > > > > Reported-by: Prashanth Prahlad <pprahlad@redhat.com> > > > > Reviewed-by: Richard Haines <richard_c_haines@btinternet.com> > > > > Tested-by: Richard Haines <richard_c_haines@btinternet.com> > > > > > > You made non-trivial changes since the last revision in this patch, so > > > you should have also dropped the Reviewed-by and Tested-by here. Now > > > David has merged the patches probably under the impression that they > > > have been reviewed/approved from the SELinux side, which isn't > > > completely true. > > > > Oh, that's a mistake, I thought I didn't add it. > > Will he be able to test this new patchset? While I tend to try to avoid reverts as much as possible, I think the right thing to do is to get these patches reverted out of DaveM's tree while we continue to sort this out and do all of the necessary testing and verification. Xin Long, please work with the netdev folks to get your patchset reverted and then respin this patchset using the feedback provided. -- paul moore www.paul-moore.com ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCHv2 net 4/4] security: implement sctp_assoc_established hook in selinux 2021-11-03 22:01 ` Paul Moore @ 2021-11-04 1:46 ` Xin Long 2021-11-04 3:17 ` Paul Moore 0 siblings, 1 reply; 20+ messages in thread From: Xin Long @ 2021-11-04 1:46 UTC (permalink / raw) To: Paul Moore Cc: Ondrej Mosnacek, network dev, SElinux list, Linux Security Module list, linux-sctp @ vger . kernel . org, David S. Miller, Jakub Kicinski, Marcelo Ricardo Leitner, James Morris, Richard Haines On Wed, Nov 3, 2021 at 6:01 PM Paul Moore <paul@paul-moore.com> wrote: > > On Wed, Nov 3, 2021 at 1:36 PM Xin Long <lucien.xin@gmail.com> wrote: > > On Wed, Nov 3, 2021 at 1:33 PM Xin Long <lucien.xin@gmail.com> wrote: > > > On Wed, Nov 3, 2021 at 12:40 PM Ondrej Mosnacek <omosnace@redhat.com> wrote: > > > > On Tue, Nov 2, 2021 at 1:03 PM Xin Long <lucien.xin@gmail.com> wrote: > > > > > > > > > > Different from selinux_inet_conn_established(), it also gives the > > > > > secid to asoc->peer_secid in selinux_sctp_assoc_established(), > > > > > as one UDP-type socket may have more than one asocs. > > > > > > > > > > Note that peer_secid in asoc will save the peer secid for this > > > > > asoc connection, and peer_sid in sksec will just keep the peer > > > > > secid for the latest connection. So the right use should be do > > > > > peeloff for UDP-type socket if there will be multiple asocs in > > > > > one socket, so that the peeloff socket has the right label for > > > > > its asoc. > > > > > > > > > > v1->v2: > > > > > - call selinux_inet_conn_established() to reduce some code > > > > > duplication in selinux_sctp_assoc_established(), as Ondrej > > > > > suggested. > > > > > - when doing peeloff, it calls sock_create() where it actually > > > > > gets secid for socket from socket_sockcreate_sid(). So reuse > > > > > SECSID_WILD to ensure the peeloff socket keeps using that > > > > > secid after calling selinux_sctp_sk_clone() for client side. > > > > > > > > Interesting... I find strange that SCTP creates the peeloff socket > > > > using sock_create() rather than allocating it directly via > > > > sock_alloc() like the other callers of sctp_copy_sock() (which calls > > > > security_sctp_sk_clone()) do. Wouldn't it make more sense to avoid the > > > > sock_create() call and just rely on the security_sctp_sk_clone() > > > > semantic to set up the labels? Would anything break if > > > > sctp_do_peeloff() switched to plain sock_alloc()? > > > > > > > > I'd rather we avoid this SECSID_WILD hack to support the weird > > > > created-but-also-cloned socket hybrid and just make the peeloff socket > > > > behave the same as an accept()-ed socket (i.e. no > > > > security_socket_[post_]create() hook calls, just > > > > security_sctp_sk_clone()). > > I believe the important part is that sctp_do_peeloff() eventually > calls security_sctp_sk_clone() via way of sctp_copy_sock(). Assuming > we have security_sctp_sk_clone() working properly I would expect that > the new socket would be setup properly when sctp_do_peeloff() returns > on success. > > ... and yes, that SECSID_WILD approach is *not* something we want to do. SECSID_WILD is used to avoid client's new socket's sid overwritten by old socket's. If I understand correctly, new socket's should keep using its original sid, namely, the one set from security_socket_[post_]create() on client side. I AGREE with that. Now I want to *confirm* this with you, as it's different from the last version's 'inherit from parent socket' that Richard and Ondrej reviewed. > > In my mind, selinux_sctp_sk_clone() should end up looking like this. > > void selinux_sctp_sk_clone(asoc, sk, newsk) > { > struct sk_security_struct sksec = sk->sk_security; > struct sk_security_struct newsksec = newsk->sk_security; > > if (!selinux_policycap_extsockclass()) > return selinux_sk_clone_security(sk, newsk); > > newsksec->sid = sksec->secid; > newsksec->peer_sid = asoc->peer_secid; > newsksec->sclass = sksec->sclass; > selinux_netlbl_sctp_sk_clone(sk, newsk); > } Let's say, this socket has 3 associations now, how can we ensure the new socket's sid is set to the right sid? I don't think we can use "sksec->secid" in this place, this is not TCP. > > Also, to be clear, the "assoc->secid = SECSID_WILD;" line should be > removed from selinux_sctp_assoc_established(). If we are treating > SCTP associations similarly to TCP connections, the association's > label/secid should be set once and not changed during the life of the > association. The association's label/secid will never change once set in this patchset. it's just a temporary record, and later it will be used to set socket's label/secid. I think that's the idea at the beginning. > > > > > > Fixes: 72e89f50084c ("security: Add support for SCTP security hooks") > > > > > Reported-by: Prashanth Prahlad <pprahlad@redhat.com> > > > > > Reviewed-by: Richard Haines <richard_c_haines@btinternet.com> > > > > > Tested-by: Richard Haines <richard_c_haines@btinternet.com> > > > > > > > > You made non-trivial changes since the last revision in this patch, so > > > > you should have also dropped the Reviewed-by and Tested-by here. Now > > > > David has merged the patches probably under the impression that they > > > > have been reviewed/approved from the SELinux side, which isn't > > > > completely true. > > > > > > Oh, that's a mistake, I thought I didn't add it. > > > Will he be able to test this new patchset? > > While I tend to try to avoid reverts as much as possible, I think the > right thing to do is to get these patches reverted out of DaveM's tree > while we continue to sort this out and do all of the necessary testing > and verification. > > Xin Long, please work with the netdev folks to get your patchset > reverted and then respin this patchset using the feedback provided. Hi, Paul, The original issue this patchset fixes is a crucial one (it could cause peeloff sockets on client side to not work) which I think can already be fixed now. If you think SECSID_WILD is tricky but no better way yet, my suggestion is to leave it for now until we have a better solution to follow up. As I couldn't find a better way to work it out. Also, we may want to hear Richard's opinion on how it should work and how this should be fixed. Thanks. > > -- > paul moore > www.paul-moore.com ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCHv2 net 4/4] security: implement sctp_assoc_established hook in selinux 2021-11-04 1:46 ` Xin Long @ 2021-11-04 3:17 ` Paul Moore 2021-11-04 10:17 ` Richard Haines ` (3 more replies) 0 siblings, 4 replies; 20+ messages in thread From: Paul Moore @ 2021-11-04 3:17 UTC (permalink / raw) To: Xin Long Cc: Ondrej Mosnacek, network dev, SElinux list, Linux Security Module list, linux-sctp @ vger . kernel . org, David S. Miller, Jakub Kicinski, Marcelo Ricardo Leitner, James Morris, Richard Haines On Wed, Nov 3, 2021 at 9:46 PM Xin Long <lucien.xin@gmail.com> wrote: > On Wed, Nov 3, 2021 at 6:01 PM Paul Moore <paul@paul-moore.com> wrote: > > On Wed, Nov 3, 2021 at 1:36 PM Xin Long <lucien.xin@gmail.com> wrote: > > > On Wed, Nov 3, 2021 at 1:33 PM Xin Long <lucien.xin@gmail.com> wrote: > > > > On Wed, Nov 3, 2021 at 12:40 PM Ondrej Mosnacek <omosnace@redhat.com> wrote: > > > > > On Tue, Nov 2, 2021 at 1:03 PM Xin Long <lucien.xin@gmail.com> wrote: > > > > > > > > > > > > Different from selinux_inet_conn_established(), it also gives the > > > > > > secid to asoc->peer_secid in selinux_sctp_assoc_established(), > > > > > > as one UDP-type socket may have more than one asocs. > > > > > > > > > > > > Note that peer_secid in asoc will save the peer secid for this > > > > > > asoc connection, and peer_sid in sksec will just keep the peer > > > > > > secid for the latest connection. So the right use should be do > > > > > > peeloff for UDP-type socket if there will be multiple asocs in > > > > > > one socket, so that the peeloff socket has the right label for > > > > > > its asoc. > > > > > > > > > > > > v1->v2: > > > > > > - call selinux_inet_conn_established() to reduce some code > > > > > > duplication in selinux_sctp_assoc_established(), as Ondrej > > > > > > suggested. > > > > > > - when doing peeloff, it calls sock_create() where it actually > > > > > > gets secid for socket from socket_sockcreate_sid(). So reuse > > > > > > SECSID_WILD to ensure the peeloff socket keeps using that > > > > > > secid after calling selinux_sctp_sk_clone() for client side. > > > > > > > > > > Interesting... I find strange that SCTP creates the peeloff socket > > > > > using sock_create() rather than allocating it directly via > > > > > sock_alloc() like the other callers of sctp_copy_sock() (which calls > > > > > security_sctp_sk_clone()) do. Wouldn't it make more sense to avoid the > > > > > sock_create() call and just rely on the security_sctp_sk_clone() > > > > > semantic to set up the labels? Would anything break if > > > > > sctp_do_peeloff() switched to plain sock_alloc()? > > > > > > > > > > I'd rather we avoid this SECSID_WILD hack to support the weird > > > > > created-but-also-cloned socket hybrid and just make the peeloff socket > > > > > behave the same as an accept()-ed socket (i.e. no > > > > > security_socket_[post_]create() hook calls, just > > > > > security_sctp_sk_clone()). > > > > I believe the important part is that sctp_do_peeloff() eventually > > calls security_sctp_sk_clone() via way of sctp_copy_sock(). Assuming > > we have security_sctp_sk_clone() working properly I would expect that > > the new socket would be setup properly when sctp_do_peeloff() returns > > on success. > > > > ... and yes, that SECSID_WILD approach is *not* something we want to do. > > SECSID_WILD is used to avoid client's new socket's sid overwritten by > old socket's. In the case of security_sctp_sk_clone() the new client socket (the cloned socket) should inherit the label/sid from the original socket (the "parent" in the inherit-from-parent label inheritance behavior discussed earlier). The selinux_sctp_assoc_established() function should not change the socket's label/sid at all, only the peer label. > If I understand correctly, new socket's should keep using its original > sid, namely, > the one set from security_socket_[post_]create() on client side. I > AGREE with that. > Now I want to *confirm* this with you, as it's different from the last version's > 'inherit from parent socket' that Richard and Ondrej reviewed. Unfortunately I think we are struggling to communicate because you are not familiar with SELinux concepts and I'm not as well versed in SCTP as you are. As things currently stand, I am getting a disconnect between your explanations and the code you have submitted; they simply aren't consistent from my perspective. In an effort to help provide something that is hopefully a bit more clear, here are the selinux_sctp_sk_clone() and selinux_sctp_assoc_established() functions which I believe we need. If you feel these are incorrect, please explain and/or provide edits: static void selinux_sctp_sk_clone(struct sctp_association *asoc, struct sock *sk, struct sock *newsk) { struct sk_security_struct *sksec = sk->sk_security; struct sk_security_struct *newsksec = newsk->sk_security; /* If policy does not support SECCLASS_SCTP_SOCKET then call * the non-sctp clone version. */ if (!selinux_policycap_extsockclass()) return selinux_sk_clone_security(sk, newsk); newsksec->secid = sksec->secid; newsksec->peer_sid = asoc->peer_secid; newsksec->sclass = sksec->sclass; selinux_netlbl_sctp_sk_clone(sk, newsk); } static void selinux_sctp_assoc_established(struct sctp_association *asoc, struct sk_buff *skb) { struct sk_security_struct *sksec = asoc->base.sk->sk_security; selinux_inet_conn_established(asoc->base.sk, skb); asoc->peer_secid = sksec->peer_sid; } > > > > > > Fixes: 72e89f50084c ("security: Add support for SCTP security hooks") > > > > > > Reported-by: Prashanth Prahlad <pprahlad@redhat.com> > > > > > > Reviewed-by: Richard Haines <richard_c_haines@btinternet.com> > > > > > > Tested-by: Richard Haines <richard_c_haines@btinternet.com> > > > > > > > > > > You made non-trivial changes since the last revision in this patch, so > > > > > you should have also dropped the Reviewed-by and Tested-by here. Now > > > > > David has merged the patches probably under the impression that they > > > > > have been reviewed/approved from the SELinux side, which isn't > > > > > completely true. > > > > > > > > Oh, that's a mistake, I thought I didn't add it. > > > > Will he be able to test this new patchset? > > > > While I tend to try to avoid reverts as much as possible, I think the > > right thing to do is to get these patches reverted out of DaveM's tree > > while we continue to sort this out and do all of the necessary testing > > and verification. > > > > Xin Long, please work with the netdev folks to get your patchset > > reverted and then respin this patchset using the feedback provided. > > Hi, Paul, > > The original issue this patchset fixes is a crucial one (it could cause > peeloff sockets on client side to not work) which I think > can already be fixed now. If you think SECSID_WILD is tricky but > no better way yet, my suggestion is to leave it for now until we have > a better solution to follow up. As I couldn't find a better way to work > it out. Also, we may want to hear Richard's opinion on how it should > work and how this should be fixed. While I understand you did not intend to mislead DaveM and the netdev folks with the v2 patchset, your failure to properly manage the patchset's metadata *did* mislead them and as a result a patchset with serious concerns from the SELinux side was merged. You need to revert this patchset while we continue to discuss, develop, and verify a proper fix that we can all agree on. If you decide not to revert this patchset I will work with DaveM to do it for you, and that is not something any of us wants. -- paul moore www.paul-moore.com ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCHv2 net 4/4] security: implement sctp_assoc_established hook in selinux 2021-11-04 3:17 ` Paul Moore @ 2021-11-04 10:17 ` Richard Haines 2021-11-04 10:40 ` Ondrej Mosnacek ` (2 subsequent siblings) 3 siblings, 0 replies; 20+ messages in thread From: Richard Haines @ 2021-11-04 10:17 UTC (permalink / raw) To: Paul Moore, Xin Long Cc: Ondrej Mosnacek, network dev, SElinux list, Linux Security Module list, linux-sctp @ vger . kernel . org, David S. Miller, Jakub Kicinski, Marcelo Ricardo Leitner, James Morris On Wed, 2021-11-03 at 23:17 -0400, Paul Moore wrote: > On Wed, Nov 3, 2021 at 9:46 PM Xin Long <lucien.xin@gmail.com> wrote: > > On Wed, Nov 3, 2021 at 6:01 PM Paul Moore <paul@paul-moore.com> > > wrote: > > > On Wed, Nov 3, 2021 at 1:36 PM Xin Long <lucien.xin@gmail.com> > > > wrote: > > > > On Wed, Nov 3, 2021 at 1:33 PM Xin Long <lucien.xin@gmail.com> > > > > wrote: > > > > > On Wed, Nov 3, 2021 at 12:40 PM Ondrej Mosnacek > > > > > <omosnace@redhat.com> wrote: > > > > > > On Tue, Nov 2, 2021 at 1:03 PM Xin Long > > > > > > <lucien.xin@gmail.com> wrote: > > > > > > > > > > > > > > Different from selinux_inet_conn_established(), it also > > > > > > > gives the > > > > > > > secid to asoc->peer_secid in > > > > > > > selinux_sctp_assoc_established(), > > > > > > > as one UDP-type socket may have more than one asocs. > > > > > > > > > > > > > > Note that peer_secid in asoc will save the peer secid for > > > > > > > this > > > > > > > asoc connection, and peer_sid in sksec will just keep the > > > > > > > peer > > > > > > > secid for the latest connection. So the right use should be > > > > > > > do > > > > > > > peeloff for UDP-type socket if there will be multiple asocs > > > > > > > in > > > > > > > one socket, so that the peeloff socket has the right label > > > > > > > for > > > > > > > its asoc. > > > > > > > > > > > > > > v1->v2: > > > > > > > - call selinux_inet_conn_established() to reduce some > > > > > > > code > > > > > > > duplication in selinux_sctp_assoc_established(), as > > > > > > > Ondrej > > > > > > > suggested. > > > > > > > - when doing peeloff, it calls sock_create() where it > > > > > > > actually > > > > > > > gets secid for socket from socket_sockcreate_sid(). So > > > > > > > reuse > > > > > > > SECSID_WILD to ensure the peeloff socket keeps using > > > > > > > that > > > > > > > secid after calling selinux_sctp_sk_clone() for client > > > > > > > side. > > > > > > > > > > > > Interesting... I find strange that SCTP creates the peeloff > > > > > > socket > > > > > > using sock_create() rather than allocating it directly via > > > > > > sock_alloc() like the other callers of sctp_copy_sock() > > > > > > (which calls > > > > > > security_sctp_sk_clone()) do. Wouldn't it make more sense to > > > > > > avoid the > > > > > > sock_create() call and just rely on the > > > > > > security_sctp_sk_clone() > > > > > > semantic to set up the labels? Would anything break if > > > > > > sctp_do_peeloff() switched to plain sock_alloc()? > > > > > > > > > > > > I'd rather we avoid this SECSID_WILD hack to support the > > > > > > weird > > > > > > created-but-also-cloned socket hybrid and just make the > > > > > > peeloff socket > > > > > > behave the same as an accept()-ed socket (i.e. no > > > > > > security_socket_[post_]create() hook calls, just > > > > > > security_sctp_sk_clone()). > > > > > > I believe the important part is that sctp_do_peeloff() eventually > > > calls security_sctp_sk_clone() via way of sctp_copy_sock(). > > > Assuming > > > we have security_sctp_sk_clone() working properly I would expect > > > that > > > the new socket would be setup properly when sctp_do_peeloff() > > > returns > > > on success. > > > > > > ... and yes, that SECSID_WILD approach is *not* something we want > > > to do. > > > > SECSID_WILD is used to avoid client's new socket's sid overwritten by > > old socket's. > > In the case of security_sctp_sk_clone() the new client socket (the > cloned socket) should inherit the label/sid from the original socket > (the "parent" in the inherit-from-parent label inheritance behavior > discussed earlier). The selinux_sctp_assoc_established() function > should not change the socket's label/sid at all, only the peer label. > > > If I understand correctly, new socket's should keep using its > > original > > sid, namely, > > the one set from security_socket_[post_]create() on client side. I > > AGREE with that. > > Now I want to *confirm* this with you, as it's different from the > > last version's > > 'inherit from parent socket' that Richard and Ondrej reviewed. > > Unfortunately I think we are struggling to communicate because you are > not familiar with SELinux concepts and I'm not as well versed in SCTP > as you are. As things currently stand, I am getting a disconnect > between your explanations and the code you have submitted; they simply > aren't consistent from my perspective. > > In an effort to help provide something that is hopefully a bit more > clear, here are the selinux_sctp_sk_clone() and > selinux_sctp_assoc_established() functions which I believe we need. > If you feel these are incorrect, please explain and/or provide edits: > > static void selinux_sctp_sk_clone(struct sctp_association *asoc, > struct sock *sk, struct sock > *newsk) > { > struct sk_security_struct *sksec = sk->sk_security; > struct sk_security_struct *newsksec = newsk->sk_security; > > /* If policy does not support SECCLASS_SCTP_SOCKET then call > * the non-sctp clone version. > */ > if (!selinux_policycap_extsockclass()) > return selinux_sk_clone_security(sk, newsk); > > newsksec->secid = sksec->secid; This should be: newsksec->sid = sksec->sid; > newsksec->peer_sid = asoc->peer_secid; > newsksec->sclass = sksec->sclass; > selinux_netlbl_sctp_sk_clone(sk, newsk); > } > > static void selinux_sctp_assoc_established(struct sctp_association > *asoc, > struct sk_buff *skb) > { > struct sk_security_struct *sksec = asoc->base.sk->sk_security; > > selinux_inet_conn_established(asoc->base.sk, skb); > asoc->peer_secid = sksec->peer_sid; > } ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCHv2 net 4/4] security: implement sctp_assoc_established hook in selinux 2021-11-04 3:17 ` Paul Moore 2021-11-04 10:17 ` Richard Haines @ 2021-11-04 10:40 ` Ondrej Mosnacek 2021-11-04 19:28 ` Paul Moore 2021-11-04 10:56 ` Xin Long 2021-11-04 11:02 ` David Miller 3 siblings, 1 reply; 20+ messages in thread From: Ondrej Mosnacek @ 2021-11-04 10:40 UTC (permalink / raw) To: Paul Moore Cc: Xin Long, network dev, SElinux list, Linux Security Module list, linux-sctp @ vger . kernel . org, David S. Miller, Jakub Kicinski, Marcelo Ricardo Leitner, James Morris, Richard Haines On Thu, Nov 4, 2021 at 4:17 AM Paul Moore <paul@paul-moore.com> wrote: > On Wed, Nov 3, 2021 at 9:46 PM Xin Long <lucien.xin@gmail.com> wrote: > > On Wed, Nov 3, 2021 at 6:01 PM Paul Moore <paul@paul-moore.com> wrote: > > > On Wed, Nov 3, 2021 at 1:36 PM Xin Long <lucien.xin@gmail.com> wrote: > > > > On Wed, Nov 3, 2021 at 1:33 PM Xin Long <lucien.xin@gmail.com> wrote: > > > > > On Wed, Nov 3, 2021 at 12:40 PM Ondrej Mosnacek <omosnace@redhat.com> wrote: > > > > > > On Tue, Nov 2, 2021 at 1:03 PM Xin Long <lucien.xin@gmail.com> wrote: > > > > > > > > > > > > > > Different from selinux_inet_conn_established(), it also gives the > > > > > > > secid to asoc->peer_secid in selinux_sctp_assoc_established(), > > > > > > > as one UDP-type socket may have more than one asocs. > > > > > > > > > > > > > > Note that peer_secid in asoc will save the peer secid for this > > > > > > > asoc connection, and peer_sid in sksec will just keep the peer > > > > > > > secid for the latest connection. So the right use should be do > > > > > > > peeloff for UDP-type socket if there will be multiple asocs in > > > > > > > one socket, so that the peeloff socket has the right label for > > > > > > > its asoc. > > > > > > > > > > > > > > v1->v2: > > > > > > > - call selinux_inet_conn_established() to reduce some code > > > > > > > duplication in selinux_sctp_assoc_established(), as Ondrej > > > > > > > suggested. > > > > > > > - when doing peeloff, it calls sock_create() where it actually > > > > > > > gets secid for socket from socket_sockcreate_sid(). So reuse > > > > > > > SECSID_WILD to ensure the peeloff socket keeps using that > > > > > > > secid after calling selinux_sctp_sk_clone() for client side. > > > > > > > > > > > > Interesting... I find strange that SCTP creates the peeloff socket > > > > > > using sock_create() rather than allocating it directly via > > > > > > sock_alloc() like the other callers of sctp_copy_sock() (which calls > > > > > > security_sctp_sk_clone()) do. Wouldn't it make more sense to avoid the > > > > > > sock_create() call and just rely on the security_sctp_sk_clone() > > > > > > semantic to set up the labels? Would anything break if > > > > > > sctp_do_peeloff() switched to plain sock_alloc()? > > > > > > > > > > > > I'd rather we avoid this SECSID_WILD hack to support the weird > > > > > > created-but-also-cloned socket hybrid and just make the peeloff socket > > > > > > behave the same as an accept()-ed socket (i.e. no > > > > > > security_socket_[post_]create() hook calls, just > > > > > > security_sctp_sk_clone()). > > > > > > I believe the important part is that sctp_do_peeloff() eventually > > > calls security_sctp_sk_clone() via way of sctp_copy_sock(). Assuming > > > we have security_sctp_sk_clone() working properly I would expect that > > > the new socket would be setup properly when sctp_do_peeloff() returns > > > on success. > > > > > > ... and yes, that SECSID_WILD approach is *not* something we want to do. > > > > SECSID_WILD is used to avoid client's new socket's sid overwritten by > > old socket's. > > In the case of security_sctp_sk_clone() the new client socket (the > cloned socket) should inherit the label/sid from the original socket > (the "parent" in the inherit-from-parent label inheritance behavior > discussed earlier). The selinux_sctp_assoc_established() function > should not change the socket's label/sid at all, only the peer label. > > > If I understand correctly, new socket's should keep using its original > > sid, namely, > > the one set from security_socket_[post_]create() on client side. I > > AGREE with that. > > Now I want to *confirm* this with you, as it's different from the last version's > > 'inherit from parent socket' that Richard and Ondrej reviewed. > > Unfortunately I think we are struggling to communicate because you are > not familiar with SELinux concepts and I'm not as well versed in SCTP > as you are. As things currently stand, I am getting a disconnect > between your explanations and the code you have submitted; they simply > aren't consistent from my perspective. > > In an effort to help provide something that is hopefully a bit more > clear, here are the selinux_sctp_sk_clone() and > selinux_sctp_assoc_established() functions which I believe we need. > If you feel these are incorrect, please explain and/or provide edits: > > static void selinux_sctp_sk_clone(struct sctp_association *asoc, > struct sock *sk, struct sock *newsk) > { > struct sk_security_struct *sksec = sk->sk_security; > struct sk_security_struct *newsksec = newsk->sk_security; > > /* If policy does not support SECCLASS_SCTP_SOCKET then call > * the non-sctp clone version. > */ > if (!selinux_policycap_extsockclass()) > return selinux_sk_clone_security(sk, newsk); > > newsksec->secid = sksec->secid; > newsksec->peer_sid = asoc->peer_secid; > newsksec->sclass = sksec->sclass; > selinux_netlbl_sctp_sk_clone(sk, newsk); > } > > static void selinux_sctp_assoc_established(struct sctp_association *asoc, > struct sk_buff *skb) > { > struct sk_security_struct *sksec = asoc->base.sk->sk_security; > > selinux_inet_conn_established(asoc->base.sk, skb); > asoc->peer_secid = sksec->peer_sid; > } This code would be functionally equivalent to the v1 patchset for the client side, but on server side you want to set newsksec->secid to asoc->secid, as this contains the "connection secid" computed by selinux_conn_sid() in selinux_sctp_assoc_request(). This is supposed to mirror what selinux_inet_conn_request() -> selinux_inet_csk_clone() does for non-SCTP sockets. So I think we should rather go back to the v1 patchset variant, where the parent socket's sid is stashed in asoc->secid to be picked up by selinux_sctp_sk_clone(). As for the sctp_do_peeloff-calls-sock_create problem - I was oblivious about the difference between the sock vs. socket structs, so this would be a bit more difficult to fix than replacing one function call. But if we end up just overwriting the label assigned in selinux_socket_post_create() as it is now, then the only difference is an unexpected SCTP_SOCKET__CREATE permission check and a pointless computation of socket_sockcreate_sid(), so it can be addressed separately. I'll try to suggest a patch and then we can discuss whether it makes sense or not. -- Ondrej Mosnacek Software Engineer, Linux Security - SELinux kernel Red Hat, Inc. ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCHv2 net 4/4] security: implement sctp_assoc_established hook in selinux 2021-11-04 10:40 ` Ondrej Mosnacek @ 2021-11-04 19:28 ` Paul Moore 0 siblings, 0 replies; 20+ messages in thread From: Paul Moore @ 2021-11-04 19:28 UTC (permalink / raw) To: Ondrej Mosnacek Cc: Xin Long, network dev, SElinux list, Linux Security Module list, linux-sctp @ vger . kernel . org, David S. Miller, Jakub Kicinski, Marcelo Ricardo Leitner, James Morris, Richard Haines On Thu, Nov 4, 2021 at 6:40 AM Ondrej Mosnacek <omosnace@redhat.com> wrote: > On Thu, Nov 4, 2021 at 4:17 AM Paul Moore <paul@paul-moore.com> wrote: > > On Wed, Nov 3, 2021 at 9:46 PM Xin Long <lucien.xin@gmail.com> wrote: > > > On Wed, Nov 3, 2021 at 6:01 PM Paul Moore <paul@paul-moore.com> wrote: > > > > On Wed, Nov 3, 2021 at 1:36 PM Xin Long <lucien.xin@gmail.com> wrote: > > > > > On Wed, Nov 3, 2021 at 1:33 PM Xin Long <lucien.xin@gmail.com> wrote: > > > > > > On Wed, Nov 3, 2021 at 12:40 PM Ondrej Mosnacek <omosnace@redhat.com> wrote: > > > > > > > On Tue, Nov 2, 2021 at 1:03 PM Xin Long <lucien.xin@gmail.com> wrote: > > > > > > > > > > > > > > > > Different from selinux_inet_conn_established(), it also gives the > > > > > > > > secid to asoc->peer_secid in selinux_sctp_assoc_established(), > > > > > > > > as one UDP-type socket may have more than one asocs. > > > > > > > > > > > > > > > > Note that peer_secid in asoc will save the peer secid for this > > > > > > > > asoc connection, and peer_sid in sksec will just keep the peer > > > > > > > > secid for the latest connection. So the right use should be do > > > > > > > > peeloff for UDP-type socket if there will be multiple asocs in > > > > > > > > one socket, so that the peeloff socket has the right label for > > > > > > > > its asoc. > > > > > > > > > > > > > > > > v1->v2: > > > > > > > > - call selinux_inet_conn_established() to reduce some code > > > > > > > > duplication in selinux_sctp_assoc_established(), as Ondrej > > > > > > > > suggested. > > > > > > > > - when doing peeloff, it calls sock_create() where it actually > > > > > > > > gets secid for socket from socket_sockcreate_sid(). So reuse > > > > > > > > SECSID_WILD to ensure the peeloff socket keeps using that > > > > > > > > secid after calling selinux_sctp_sk_clone() for client side. > > > > > > > > > > > > > > Interesting... I find strange that SCTP creates the peeloff socket > > > > > > > using sock_create() rather than allocating it directly via > > > > > > > sock_alloc() like the other callers of sctp_copy_sock() (which calls > > > > > > > security_sctp_sk_clone()) do. Wouldn't it make more sense to avoid the > > > > > > > sock_create() call and just rely on the security_sctp_sk_clone() > > > > > > > semantic to set up the labels? Would anything break if > > > > > > > sctp_do_peeloff() switched to plain sock_alloc()? > > > > > > > > > > > > > > I'd rather we avoid this SECSID_WILD hack to support the weird > > > > > > > created-but-also-cloned socket hybrid and just make the peeloff socket > > > > > > > behave the same as an accept()-ed socket (i.e. no > > > > > > > security_socket_[post_]create() hook calls, just > > > > > > > security_sctp_sk_clone()). > > > > > > > > I believe the important part is that sctp_do_peeloff() eventually > > > > calls security_sctp_sk_clone() via way of sctp_copy_sock(). Assuming > > > > we have security_sctp_sk_clone() working properly I would expect that > > > > the new socket would be setup properly when sctp_do_peeloff() returns > > > > on success. > > > > > > > > ... and yes, that SECSID_WILD approach is *not* something we want to do. > > > > > > SECSID_WILD is used to avoid client's new socket's sid overwritten by > > > old socket's. > > > > In the case of security_sctp_sk_clone() the new client socket (the > > cloned socket) should inherit the label/sid from the original socket > > (the "parent" in the inherit-from-parent label inheritance behavior > > discussed earlier). The selinux_sctp_assoc_established() function > > should not change the socket's label/sid at all, only the peer label. > > > > > If I understand correctly, new socket's should keep using its original > > > sid, namely, > > > the one set from security_socket_[post_]create() on client side. I > > > AGREE with that. > > > Now I want to *confirm* this with you, as it's different from the last version's > > > 'inherit from parent socket' that Richard and Ondrej reviewed. > > > > Unfortunately I think we are struggling to communicate because you are > > not familiar with SELinux concepts and I'm not as well versed in SCTP > > as you are. As things currently stand, I am getting a disconnect > > between your explanations and the code you have submitted; they simply > > aren't consistent from my perspective. > > > > In an effort to help provide something that is hopefully a bit more > > clear, here are the selinux_sctp_sk_clone() and > > selinux_sctp_assoc_established() functions which I believe we need. > > If you feel these are incorrect, please explain and/or provide edits: > > > > static void selinux_sctp_sk_clone(struct sctp_association *asoc, > > struct sock *sk, struct sock *newsk) > > { > > struct sk_security_struct *sksec = sk->sk_security; > > struct sk_security_struct *newsksec = newsk->sk_security; > > > > /* If policy does not support SECCLASS_SCTP_SOCKET then call > > * the non-sctp clone version. > > */ > > if (!selinux_policycap_extsockclass()) > > return selinux_sk_clone_security(sk, newsk); > > > > newsksec->secid = sksec->secid; > > newsksec->peer_sid = asoc->peer_secid; > > newsksec->sclass = sksec->sclass; > > selinux_netlbl_sctp_sk_clone(sk, newsk); > > } > > > > static void selinux_sctp_assoc_established(struct sctp_association *asoc, > > struct sk_buff *skb) > > { > > struct sk_security_struct *sksec = asoc->base.sk->sk_security; > > > > selinux_inet_conn_established(asoc->base.sk, skb); > > asoc->peer_secid = sksec->peer_sid; > > } > > This code would be functionally equivalent to the v1 patchset for the > client side, but on server side you want to set newsksec->secid to > asoc->secid, as this contains the "connection secid" computed by > selinux_conn_sid() in selinux_sctp_assoc_request(). This is supposed > to mirror what selinux_inet_conn_request() -> selinux_inet_csk_clone() > does for non-SCTP sockets. So I think we should rather go back to the > v1 patchset variant, where the parent socket's sid is stashed in > asoc->secid to be picked up by selinux_sctp_sk_clone(). > > As for the sctp_do_peeloff-calls-sock_create problem - I was oblivious > about the difference between the sock vs. socket structs, so this > would be a bit more difficult to fix than replacing one function call. > But if we end up just overwriting the label assigned in > selinux_socket_post_create() as it is now, then the only difference is > an unexpected SCTP_SOCKET__CREATE permission check and a pointless > computation of socket_sockcreate_sid(), so it can be addressed > separately. I'll try to suggest a patch and then we can discuss > whether it makes sense or not. Okay, I'll wait on that patchset before commenting further. -- paul moore www.paul-moore.com ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCHv2 net 4/4] security: implement sctp_assoc_established hook in selinux 2021-11-04 3:17 ` Paul Moore 2021-11-04 10:17 ` Richard Haines 2021-11-04 10:40 ` Ondrej Mosnacek @ 2021-11-04 10:56 ` Xin Long 2021-11-04 11:02 ` David Miller 3 siblings, 0 replies; 20+ messages in thread From: Xin Long @ 2021-11-04 10:56 UTC (permalink / raw) To: Paul Moore Cc: Ondrej Mosnacek, network dev, SElinux list, Linux Security Module list, linux-sctp @ vger . kernel . org, David S. Miller, Jakub Kicinski, Marcelo Ricardo Leitner, James Morris, Richard Haines On Wed, Nov 3, 2021 at 11:17 PM Paul Moore <paul@paul-moore.com> wrote: > > On Wed, Nov 3, 2021 at 9:46 PM Xin Long <lucien.xin@gmail.com> wrote: > > On Wed, Nov 3, 2021 at 6:01 PM Paul Moore <paul@paul-moore.com> wrote: > > > On Wed, Nov 3, 2021 at 1:36 PM Xin Long <lucien.xin@gmail.com> wrote: > > > > On Wed, Nov 3, 2021 at 1:33 PM Xin Long <lucien.xin@gmail.com> wrote: > > > > > On Wed, Nov 3, 2021 at 12:40 PM Ondrej Mosnacek <omosnace@redhat.com> wrote: > > > > > > On Tue, Nov 2, 2021 at 1:03 PM Xin Long <lucien.xin@gmail.com> wrote: > > > > > > > > > > > > > > Different from selinux_inet_conn_established(), it also gives the > > > > > > > secid to asoc->peer_secid in selinux_sctp_assoc_established(), > > > > > > > as one UDP-type socket may have more than one asocs. > > > > > > > > > > > > > > Note that peer_secid in asoc will save the peer secid for this > > > > > > > asoc connection, and peer_sid in sksec will just keep the peer > > > > > > > secid for the latest connection. So the right use should be do > > > > > > > peeloff for UDP-type socket if there will be multiple asocs in > > > > > > > one socket, so that the peeloff socket has the right label for > > > > > > > its asoc. > > > > > > > > > > > > > > v1->v2: > > > > > > > - call selinux_inet_conn_established() to reduce some code > > > > > > > duplication in selinux_sctp_assoc_established(), as Ondrej > > > > > > > suggested. > > > > > > > - when doing peeloff, it calls sock_create() where it actually > > > > > > > gets secid for socket from socket_sockcreate_sid(). So reuse > > > > > > > SECSID_WILD to ensure the peeloff socket keeps using that > > > > > > > secid after calling selinux_sctp_sk_clone() for client side. > > > > > > > > > > > > Interesting... I find strange that SCTP creates the peeloff socket > > > > > > using sock_create() rather than allocating it directly via > > > > > > sock_alloc() like the other callers of sctp_copy_sock() (which calls > > > > > > security_sctp_sk_clone()) do. Wouldn't it make more sense to avoid the > > > > > > sock_create() call and just rely on the security_sctp_sk_clone() > > > > > > semantic to set up the labels? Would anything break if > > > > > > sctp_do_peeloff() switched to plain sock_alloc()? > > > > > > > > > > > > I'd rather we avoid this SECSID_WILD hack to support the weird > > > > > > created-but-also-cloned socket hybrid and just make the peeloff socket > > > > > > behave the same as an accept()-ed socket (i.e. no > > > > > > security_socket_[post_]create() hook calls, just > > > > > > security_sctp_sk_clone()). > > > > > > I believe the important part is that sctp_do_peeloff() eventually > > > calls security_sctp_sk_clone() via way of sctp_copy_sock(). Assuming > > > we have security_sctp_sk_clone() working properly I would expect that > > > the new socket would be setup properly when sctp_do_peeloff() returns > > > on success. > > > > > > ... and yes, that SECSID_WILD approach is *not* something we want to do. > > > > SECSID_WILD is used to avoid client's new socket's sid overwritten by > > old socket's. > > In the case of security_sctp_sk_clone() the new client socket (the > cloned socket) should inherit the label/sid from the original socket """ The initial SCTP client association would need to take it's label from the parent process so perhaps that is the right answer for all SCTP client associations[2]. [2] I'm guessing the client associations might also want to follow the setsockcreatecon(3) behavior, see selinux_sockcreate_sid() for more info. """ What I got is to take it's label from the parent process, which means we get it from socket_sockcreate_sid(), not directly copy from parent socket. It seems I misunderstood that, Sorry, maybe we should just use the v1 patchset. > (the "parent" in the inherit-from-parent label inheritance behavior > discussed earlier). The selinux_sctp_assoc_established() function > should not change the socket's label/sid at all, only the peer label. Right, that's what it currently does in this patchset, no *socket* sid is changed, and only *socket*'s peer label. { struct sk_security_struct *sksec = asoc->base.sk->sk_security; selinux_inet_conn_established(asoc->base.sk, skb); asoc->peer_secid = sksec->peer_sid; asoc->secid = SECSID_WILD; } > > > If I understand correctly, new socket's should keep using its original > > sid, namely, > > the one set from security_socket_[post_]create() on client side. I > > AGREE with that. > > Now I want to *confirm* this with you, as it's different from the last version's > > 'inherit from parent socket' that Richard and Ondrej reviewed. > > Unfortunately I think we are struggling to communicate because you are > not familiar with SELinux concepts and I'm not as well versed in SCTP > as you are. As things currently stand, I am getting a disconnect > between your explanations and the code you have submitted; they simply > aren't consistent from my perspective. > > In an effort to help provide something that is hopefully a bit more > clear, here are the selinux_sctp_sk_clone() and > selinux_sctp_assoc_established() functions which I believe we need. > If you feel these are incorrect, please explain and/or provide edits: > > static void selinux_sctp_sk_clone(struct sctp_association *asoc, > struct sock *sk, struct sock *newsk) > { > struct sk_security_struct *sksec = sk->sk_security; > struct sk_security_struct *newsksec = newsk->sk_security; > > /* If policy does not support SECCLASS_SCTP_SOCKET then call > * the non-sctp clone version. > */ > if (!selinux_policycap_extsockclass()) > return selinux_sk_clone_security(sk, newsk); > > newsksec->secid = sksec->secid; > newsksec->peer_sid = asoc->peer_secid; > newsksec->sclass = sksec->sclass; > selinux_netlbl_sctp_sk_clone(sk, newsk); > } here, SCTP is one-to-many socket, and it means one socket can have multiple associations or connections, so for sksec->sid in one socket it can only save the latest cid, if we peel off an old one, it will get the wrong cid on server side. > > static void selinux_sctp_assoc_established(struct sctp_association *asoc, > struct sk_buff *skb) > { > struct sk_security_struct *sksec = asoc->base.sk->sk_security; > > selinux_inet_conn_established(asoc->base.sk, skb); > asoc->peer_secid = sksec->peer_sid; > } > > > > > > > > Fixes: 72e89f50084c ("security: Add support for SCTP security hooks") > > > > > > > Reported-by: Prashanth Prahlad <pprahlad@redhat.com> > > > > > > > Reviewed-by: Richard Haines <richard_c_haines@btinternet.com> > > > > > > > Tested-by: Richard Haines <richard_c_haines@btinternet.com> > > > > > > > > > > > > You made non-trivial changes since the last revision in this patch, so > > > > > > you should have also dropped the Reviewed-by and Tested-by here. Now > > > > > > David has merged the patches probably under the impression that they > > > > > > have been reviewed/approved from the SELinux side, which isn't > > > > > > completely true. > > > > > > > > > > Oh, that's a mistake, I thought I didn't add it. > > > > > Will he be able to test this new patchset? > > > > > > While I tend to try to avoid reverts as much as possible, I think the > > > right thing to do is to get these patches reverted out of DaveM's tree > > > while we continue to sort this out and do all of the necessary testing > > > and verification. > > > > > > Xin Long, please work with the netdev folks to get your patchset > > > reverted and then respin this patchset using the feedback provided. > > > > Hi, Paul, > > > > The original issue this patchset fixes is a crucial one (it could cause > > peeloff sockets on client side to not work) which I think > > can already be fixed now. If you think SECSID_WILD is tricky but > > no better way yet, my suggestion is to leave it for now until we have > > a better solution to follow up. As I couldn't find a better way to work > > it out. Also, we may want to hear Richard's opinion on how it should > > work and how this should be fixed. > > While I understand you did not intend to mislead DaveM and the netdev > folks with the v2 patchset, your failure to properly manage the > patchset's metadata *did* mislead them and as a result a patchset with > serious concerns from the SELinux side was merged. You need to revert > this patchset while we continue to discuss, develop, and verify a > proper fix that we can all agree on. If you decide not to revert this > patchset I will work with DaveM to do it for you, and that is not > something any of us wants. > > -- > paul moore > www.paul-moore.com ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCHv2 net 4/4] security: implement sctp_assoc_established hook in selinux 2021-11-04 3:17 ` Paul Moore ` (2 preceding siblings ...) 2021-11-04 10:56 ` Xin Long @ 2021-11-04 11:02 ` David Miller 2021-11-04 19:10 ` Paul Moore 3 siblings, 1 reply; 20+ messages in thread From: David Miller @ 2021-11-04 11:02 UTC (permalink / raw) To: paul Cc: lucien.xin, omosnace, netdev, selinux, linux-security-module, linux-sctp, kuba, marcelo.leitner, jmorris, richard_c_haines From: Paul Moore <paul@paul-moore.com> Date: Wed, 3 Nov 2021 23:17:00 -0400 > > While I understand you did not intend to mislead DaveM and the netdev > folks with the v2 patchset, your failure to properly manage the > patchset's metadata *did* mislead them and as a result a patchset with > serious concerns from the SELinux side was merged. You need to revert > this patchset while we continue to discuss, develop, and verify a > proper fix that we can all agree on. If you decide not to revert this > patchset I will work with DaveM to do it for you, and that is not > something any of us wants. I would prefer a follow-up rathewr than a revert at this point. Please work with Xin to come up with a fix that works for both of you. Thanks. ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCHv2 net 4/4] security: implement sctp_assoc_established hook in selinux 2021-11-04 11:02 ` David Miller @ 2021-11-04 19:10 ` Paul Moore 2021-11-04 19:49 ` Xin Long 0 siblings, 1 reply; 20+ messages in thread From: Paul Moore @ 2021-11-04 19:10 UTC (permalink / raw) To: David Miller Cc: lucien.xin, omosnace, netdev, selinux, linux-security-module, linux-sctp, kuba, marcelo.leitner, jmorris, richard_c_haines On Thu, Nov 4, 2021 at 7:02 AM David Miller <davem@davemloft.net> wrote: > From: Paul Moore <paul@paul-moore.com> > Date: Wed, 3 Nov 2021 23:17:00 -0400 > > > > While I understand you did not intend to mislead DaveM and the netdev > > folks with the v2 patchset, your failure to properly manage the > > patchset's metadata *did* mislead them and as a result a patchset with > > serious concerns from the SELinux side was merged. You need to revert > > this patchset while we continue to discuss, develop, and verify a > > proper fix that we can all agree on. If you decide not to revert this > > patchset I will work with DaveM to do it for you, and that is not > > something any of us wants. > > I would prefer a follow-up rathewr than a revert at this point. > > Please work with Xin to come up with a fix that works for both of you. We are working with Xin (see this thread), but you'll notice there is still not a clear consensus on the best path forward. The only thing I am clear on at this point is that the current code in linux-next is *not* something we want from a SELinux perspective. I don't like leaving known bad code like this in linux-next for more than a day or two so please revert it, now. If your policy is to merge substantive non-network subsystem changes into the network tree without the proper ACKs from the other subsystem maintainers, it would seem reasonable to also be willing to revert those patches when the affected subsystems request it. I understand that if a patchset is being ignored you might feel the need to act without an explicit ACK, but this particular patchset wasn't even a day old before you merged into the netdev tree. Not to mention that the patchset was posted during the second day of the merge window, a time when many maintainers are busy testing code, sending pull requests to Linus, and generally managing merge window fallout. -- paul moore www.paul-moore.com ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCHv2 net 4/4] security: implement sctp_assoc_established hook in selinux 2021-11-04 19:10 ` Paul Moore @ 2021-11-04 19:49 ` Xin Long 2021-11-04 20:07 ` Paul Moore 0 siblings, 1 reply; 20+ messages in thread From: Xin Long @ 2021-11-04 19:49 UTC (permalink / raw) To: Paul Moore Cc: David Miller, Ondrej Mosnacek, network dev, SElinux list, LSM List, linux-sctp @ vger . kernel . org, Jakub Kicinski, Marcelo Ricardo Leitner, jmorris, Richard Haines On Thu, Nov 4, 2021 at 3:10 PM Paul Moore <paul@paul-moore.com> wrote: > > On Thu, Nov 4, 2021 at 7:02 AM David Miller <davem@davemloft.net> wrote: > > From: Paul Moore <paul@paul-moore.com> > > Date: Wed, 3 Nov 2021 23:17:00 -0400 > > > > > > While I understand you did not intend to mislead DaveM and the netdev > > > folks with the v2 patchset, your failure to properly manage the > > > patchset's metadata *did* mislead them and as a result a patchset with > > > serious concerns from the SELinux side was merged. You need to revert > > > this patchset while we continue to discuss, develop, and verify a > > > proper fix that we can all agree on. If you decide not to revert this > > > patchset I will work with DaveM to do it for you, and that is not > > > something any of us wants. > > > > I would prefer a follow-up rathewr than a revert at this point. > > > > Please work with Xin to come up with a fix that works for both of you. > > We are working with Xin (see this thread), but you'll notice there is > still not a clear consensus on the best path forward. The only thing > I am clear on at this point is that the current code in linux-next is > *not* something we want from a SELinux perspective. I don't like > leaving known bad code like this in linux-next for more than a day or > two so please revert it, now. If your policy is to merge substantive > non-network subsystem changes into the network tree without the proper > ACKs from the other subsystem maintainers, it would seem reasonable to > also be willing to revert those patches when the affected subsystems > request it. > > I understand that if a patchset is being ignored you might feel the > need to act without an explicit ACK, but this particular patchset > wasn't even a day old before you merged into the netdev tree. Not to > mention that the patchset was posted during the second day of the > merge window, a time when many maintainers are busy testing code, > sending pull requests to Linus, and generally managing merge window > fallout. > > -- > paul moore > www.paul-moore.com Hi Paul, It's applied on net tree, I think mostly because I posted this on net.git tree. Also, it's well related to the network part and affects SCTP protocol quite a lot. I wanted to post it on selinux tree: pcmoore/selinux.git, but I noticed the commit on top is written in 2019: commit 6e6934bae891681bc23b2536fff20e0898683f2c (HEAD -> main, origin/main, origin/HEAD) Author: Paul Moore <paul@paul-moore.com> Date: Tue Sep 17 15:02:56 2019 -0400 selinux: add a SELinux specific README.md DO NOT SUBMIT UPSTREAM Then I thought this tree was no longer active, sorry about that. ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCHv2 net 4/4] security: implement sctp_assoc_established hook in selinux 2021-11-04 19:49 ` Xin Long @ 2021-11-04 20:07 ` Paul Moore 0 siblings, 0 replies; 20+ messages in thread From: Paul Moore @ 2021-11-04 20:07 UTC (permalink / raw) To: Xin Long Cc: David Miller, Ondrej Mosnacek, network dev, SElinux list, LSM List, linux-sctp @ vger . kernel . org, Jakub Kicinski, Marcelo Ricardo Leitner, jmorris, Richard Haines On Thu, Nov 4, 2021 at 3:49 PM Xin Long <lucien.xin@gmail.com> wrote: > On Thu, Nov 4, 2021 at 3:10 PM Paul Moore <paul@paul-moore.com> wrote: > > On Thu, Nov 4, 2021 at 7:02 AM David Miller <davem@davemloft.net> wrote: > > > From: Paul Moore <paul@paul-moore.com> > > > Date: Wed, 3 Nov 2021 23:17:00 -0400 > > > > > > > > While I understand you did not intend to mislead DaveM and the netdev > > > > folks with the v2 patchset, your failure to properly manage the > > > > patchset's metadata *did* mislead them and as a result a patchset with > > > > serious concerns from the SELinux side was merged. You need to revert > > > > this patchset while we continue to discuss, develop, and verify a > > > > proper fix that we can all agree on. If you decide not to revert this > > > > patchset I will work with DaveM to do it for you, and that is not > > > > something any of us wants. > > > > > > I would prefer a follow-up rathewr than a revert at this point. > > > > > > Please work with Xin to come up with a fix that works for both of you. > > > > We are working with Xin (see this thread), but you'll notice there is > > still not a clear consensus on the best path forward. The only thing > > I am clear on at this point is that the current code in linux-next is > > *not* something we want from a SELinux perspective. I don't like > > leaving known bad code like this in linux-next for more than a day or > > two so please revert it, now. If your policy is to merge substantive > > non-network subsystem changes into the network tree without the proper > > ACKs from the other subsystem maintainers, it would seem reasonable to > > also be willing to revert those patches when the affected subsystems > > request it. > > > > I understand that if a patchset is being ignored you might feel the > > need to act without an explicit ACK, but this particular patchset > > wasn't even a day old before you merged into the netdev tree. Not to > > mention that the patchset was posted during the second day of the > > merge window, a time when many maintainers are busy testing code, > > sending pull requests to Linus, and generally managing merge window > > fallout. > > Hi Paul, > > It's applied on net tree, I think mostly because I posted this on net.git tree. > Also, it's well related to the network part and affects SCTP protocol > quite a lot. Yes, I know it is in the net tree, that is how it made its way into linux-next. I wouldn't have merged it yet, and if not me who else would have merged it beside the netdev folks? Am I misunderstanding your comment? > I wanted to post it on selinux tree: pcmoore/selinux.git, but I noticed the > commit on top is written in 2019: > > commit 6e6934bae891681bc23b2536fff20e0898683f2c (HEAD -> main, > origin/main, origin/HEAD) > Author: Paul Moore <paul@paul-moore.com> > Date: Tue Sep 17 15:02:56 2019 -0400 > > selinux: add a SELinux specific README.md > > DO NOT SUBMIT UPSTREAM > > Then I thought this tree was no longer active, sorry about that. Like many kernel trees the default/main branch for the SELinux tree doesn't contain anything useful, for the SELinux tree (and audit for that matter) it is basically just the most recent major/minor tag from Linus tree with a single tree specific README.md file patch so that the GitHub mirror has a pretty landing page and a canonical reference for how the tree is maintained. * https://github.com/SELinuxProject/selinux-kernel The general approach to the SELinux tree, as documented in the README.md, is to do all of the linux-next work in the selinux/next branch with the stable work happening in the selinux/stable-X.Y branches. FWIW, once we've resolved things I would be happy to have the patchset live in the SELinux tree as opposed to the netdev tree. -- paul moore www.paul-moore.com ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCHv2 net 0/4] security: fixups for the security hooks in sctp 2021-11-02 12:02 [PATCHv2 net 0/4] security: fixups for the security hooks in sctp Xin Long ` (3 preceding siblings ...) 2021-11-02 12:02 ` [PATCHv2 net 4/4] security: implement sctp_assoc_established hook in selinux Xin Long @ 2021-11-03 11:20 ` patchwork-bot+netdevbpf 4 siblings, 0 replies; 20+ messages in thread From: patchwork-bot+netdevbpf @ 2021-11-03 11:20 UTC (permalink / raw) To: Xin Long Cc: netdev, selinux, linux-security-module, linux-sctp, davem, kuba, marcelo.leitner, jmorris, paul, richard_c_haines, omosnace Hello: This series was applied to netdev/net.git (master) by David S. Miller <davem@davemloft.net>: On Tue, 2 Nov 2021 08:02:46 -0400 you wrote: > There are a couple of problems in the currect security hooks in sctp: > > 1. The hooks incorrectly treat sctp_endpoint in SCTP as request_sock in > TCP, while it's in fact no more than an extension of the sock, and > represents the local host. It is created when sock is created, not > when a conn request comes. sctp_association is actually the correct > one to represent the connection, and created when a conn request > arrives. > > [...] Here is the summary with links: - [PATCHv2,net,1/4] security: pass asoc to sctp_assoc_request and sctp_sk_clone https://git.kernel.org/netdev/net/c/c081d53f97a1 - [PATCHv2,net,2/4] security: call security_sctp_assoc_request in sctp_sf_do_5_1D_ce https://git.kernel.org/netdev/net/c/e215dab1c490 - [PATCHv2,net,3/4] security: add sctp_assoc_established hook https://git.kernel.org/netdev/net/c/7c2ef0240e6a - [PATCHv2,net,4/4] security: implement sctp_assoc_established hook in selinux https://git.kernel.org/netdev/net/c/e7310c94024c You are awesome, thank you! -- Deet-doot-dot, I am a bot. https://korg.docs.kernel.org/patchwork/pwbot.html ^ permalink raw reply [flat|nested] 20+ messages in thread
end of thread, other threads:[~2021-11-04 20:07 UTC | newest] Thread overview: 20+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2021-11-02 12:02 [PATCHv2 net 0/4] security: fixups for the security hooks in sctp Xin Long 2021-11-02 12:02 ` [PATCHv2 net 1/4] security: pass asoc to sctp_assoc_request and sctp_sk_clone Xin Long 2021-11-02 12:02 ` [PATCHv2 net 2/4] security: call security_sctp_assoc_request in sctp_sf_do_5_1D_ce Xin Long 2021-11-02 12:02 ` [PATCHv2 net 3/4] security: add sctp_assoc_established hook Xin Long 2021-11-02 12:02 ` [PATCHv2 net 4/4] security: implement sctp_assoc_established hook in selinux Xin Long 2021-11-03 16:40 ` Ondrej Mosnacek 2021-11-03 17:33 ` Xin Long 2021-11-03 17:36 ` Xin Long 2021-11-03 22:01 ` Paul Moore 2021-11-04 1:46 ` Xin Long 2021-11-04 3:17 ` Paul Moore 2021-11-04 10:17 ` Richard Haines 2021-11-04 10:40 ` Ondrej Mosnacek 2021-11-04 19:28 ` Paul Moore 2021-11-04 10:56 ` Xin Long 2021-11-04 11:02 ` David Miller 2021-11-04 19:10 ` Paul Moore 2021-11-04 19:49 ` Xin Long 2021-11-04 20:07 ` Paul Moore 2021-11-03 11:20 ` [PATCHv2 net 0/4] security: fixups for the security hooks in sctp patchwork-bot+netdevbpf
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).