* [PATCH net 0/4] security: fixups for the security hooks in sctp
@ 2021-10-22 6:36 Xin Long
2021-10-22 6:36 ` [PATCH net 1/4] security: pass asoc to sctp_assoc_request and sctp_sk_clone Xin Long
` (4 more replies)
0 siblings, 5 replies; 21+ messages in thread
From: Xin Long @ 2021-10-22 6:36 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.
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 | 18 +++++---
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 | 36 +++++++++++-----
security/selinux/include/netlabel.h | 4 +-
security/selinux/netlabel.c | 14 +++----
11 files changed, 135 insertions(+), 92 deletions(-)
--
2.27.0
^ permalink raw reply [flat|nested] 21+ messages in thread* [PATCH net 1/4] security: pass asoc to sctp_assoc_request and sctp_sk_clone 2021-10-22 6:36 [PATCH net 0/4] security: fixups for the security hooks in sctp Xin Long @ 2021-10-22 6:36 ` Xin Long 2021-10-22 15:35 ` Jakub Kicinski 2021-10-24 13:50 ` Richard Haines 2021-10-22 6:36 ` [PATCH net 2/4] security: call security_sctp_assoc_request in sctp_sf_do_5_1D_ce Xin Long ` (3 subsequent siblings) 4 siblings, 2 replies; 21+ messages in thread From: Xin Long @ 2021-10-22 6:36 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(). Fixes: 72e89f50084c ("security: Add support for SCTP security hooks") Reported-by: Prashanth Prahlad <pprahlad@redhat.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 | 20 ++++++++++---------- security/selinux/include/netlabel.h | 4 ++-- security/selinux/netlabel.c | 14 +++++++------- 11 files changed, 73 insertions(+), 74 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..f025fc00421b 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); @@ -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..43d72f776a7d 100644 --- a/security/selinux/netlabel.c +++ b/security/selinux/netlabel.c @@ -268,22 +268,22 @@ int selinux_netlbl_skbuff_setsid(struct sk_buff *skb, * 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] 21+ messages in thread
* Re: [PATCH net 1/4] security: pass asoc to sctp_assoc_request and sctp_sk_clone 2021-10-22 6:36 ` [PATCH net 1/4] security: pass asoc to sctp_assoc_request and sctp_sk_clone Xin Long @ 2021-10-22 15:35 ` Jakub Kicinski 2021-10-23 4:25 ` Xin Long 2021-10-24 13:50 ` Richard Haines 1 sibling, 1 reply; 21+ messages in thread From: Jakub Kicinski @ 2021-10-22 15:35 UTC (permalink / raw) To: Xin Long Cc: network dev, selinux, linux-security-module, linux-sctp, davem, Marcelo Ricardo Leitner, James Morris, Paul Moore, Richard Haines, Ondrej Mosnacek On Fri, 22 Oct 2021 02:36:09 -0400 Xin Long wrote: > 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(). > > Fixes: 72e89f50084c ("security: Add support for SCTP security hooks") > Reported-by: Prashanth Prahlad <pprahlad@redhat.com> > Signed-off-by: Xin Long <lucien.xin@gmail.com> missed one? security/selinux/netlabel.c:274: warning: Function parameter or member 'asoc' not described in 'selinux_netlbl_sctp_assoc_request' security/selinux/netlabel.c:274: warning: Excess function parameter 'ep' description in 'selinux_netlbl_sctp_assoc_request' ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH net 1/4] security: pass asoc to sctp_assoc_request and sctp_sk_clone 2021-10-22 15:35 ` Jakub Kicinski @ 2021-10-23 4:25 ` Xin Long 0 siblings, 0 replies; 21+ messages in thread From: Xin Long @ 2021-10-23 4:25 UTC (permalink / raw) To: Jakub Kicinski Cc: network dev, selinux, LSM List, linux-sctp @ vger . kernel . org, davem, Marcelo Ricardo Leitner, James Morris, Paul Moore, Richard Haines, Ondrej Mosnacek On Fri, Oct 22, 2021 at 11:36 PM Jakub Kicinski <kuba@kernel.org> wrote: > > On Fri, 22 Oct 2021 02:36:09 -0400 Xin Long wrote: > > 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(). > > > > Fixes: 72e89f50084c ("security: Add support for SCTP security hooks") > > Reported-by: Prashanth Prahlad <pprahlad@redhat.com> > > Signed-off-by: Xin Long <lucien.xin@gmail.com> > > missed one? > > security/selinux/netlabel.c:274: warning: Function parameter or member > 'asoc' not described in 'selinux_netlbl_sctp_assoc_request' > security/selinux/netlabel.c:274: warning: Excess function parameter 'ep' description in 'selinux_netlbl_sctp_assoc_request' Yup, the function description also needs fixing: @@ -260,11 +260,11 @@ 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. * */ Thanks. ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH net 1/4] security: pass asoc to sctp_assoc_request and sctp_sk_clone 2021-10-22 6:36 ` [PATCH net 1/4] security: pass asoc to sctp_assoc_request and sctp_sk_clone Xin Long 2021-10-22 15:35 ` Jakub Kicinski @ 2021-10-24 13:50 ` Richard Haines 1 sibling, 0 replies; 21+ messages in thread From: Richard Haines @ 2021-10-24 13:50 UTC (permalink / raw) To: Xin Long, network dev, selinux, linux-security-module, linux-sctp Cc: davem, kuba, Marcelo Ricardo Leitner, James Morris, Paul Moore, Ondrej Mosnacek On Fri, 2021-10-22 at 02:36 -0400, Xin Long wrote: > 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(). > > Fixes: 72e89f50084c ("security: Add support for SCTP security hooks") > Reported-by: Prashanth Prahlad <pprahlad@redhat.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 | 20 ++++++++++---------- > security/selinux/include/netlabel.h | 4 ++-- > security/selinux/netlabel.c | 14 +++++++------- > 11 files changed, 73 insertions(+), 74 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..f025fc00421b 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); > @@ -5418,11 +5418,11 @@ static int selinux_sctp_assoc_request(struct > sctp_endpoint *ep, I found another nit where ep needs changing: diff -ur a/security/selinux/hooks.c b/security/selinux/hooks.c --- a/security/selinux/hooks.c 2021-10-22 12:29:23.771796000 +0100 +++ b/security/selinux/hooks.c 2021-10-23 09:39:40.514988202 +0100 @@ -5409,7 +5409,7 @@ } /* 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. > 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..43d72f776a7d 100644 > --- a/security/selinux/netlabel.c > +++ b/security/selinux/netlabel.c > @@ -268,22 +268,22 @@ int selinux_netlbl_skbuff_setsid(struct sk_buff > *skb, > * 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; > } ^ permalink raw reply [flat|nested] 21+ messages in thread
* [PATCH net 2/4] security: call security_sctp_assoc_request in sctp_sf_do_5_1D_ce 2021-10-22 6:36 [PATCH net 0/4] security: fixups for the security hooks in sctp Xin Long 2021-10-22 6:36 ` [PATCH net 1/4] security: pass asoc to sctp_assoc_request and sctp_sk_clone Xin Long @ 2021-10-22 6:36 ` Xin Long 2021-10-25 7:58 ` Ondrej Mosnacek 2021-10-22 6:36 ` [PATCH net 3/4] security: add sctp_assoc_established hook Xin Long ` (2 subsequent siblings) 4 siblings, 1 reply; 21+ messages in thread From: Xin Long @ 2021-10-22 6:36 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 delete after INIT_ACK chunk is replied. So for the real asoc created in sctp_sf_do_5_1D_ce() when receives the COOKIE_ECHO chunk, security_sctp_assoc_request() should also be called. Fixes: 72e89f50084c ("security: Add support for SCTP security hooks") Reported-by: Prashanth Prahlad <pprahlad@redhat.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..9a38067762e5 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 an 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 an + 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] 21+ messages in thread
* Re: [PATCH net 2/4] security: call security_sctp_assoc_request in sctp_sf_do_5_1D_ce 2021-10-22 6:36 ` [PATCH net 2/4] security: call security_sctp_assoc_request in sctp_sf_do_5_1D_ce Xin Long @ 2021-10-25 7:58 ` Ondrej Mosnacek 0 siblings, 0 replies; 21+ messages in thread From: Ondrej Mosnacek @ 2021-10-25 7:58 UTC (permalink / raw) To: Xin Long Cc: network dev, SElinux list, Linux Security Module list, linux-sctp, David S. Miller, Jakub Kicinski, Marcelo Ricardo Leitner, James Morris, Paul Moore, Richard Haines On Fri, Oct 22, 2021 at 8:36 AM Xin Long <lucien.xin@gmail.com> wrote: > The asoc created when receives the INIT chunk is a temporary one, it > will be delete after INIT_ACK chunk is replied. So for the real asoc Nit: s/delete/deleted/ > created in sctp_sf_do_5_1D_ce() when receives the COOKIE_ECHO chunk, Nit: s/receives the COOKIE_ECHO chunk/the COOKIE_ECHO chunk is received/ > security_sctp_assoc_request() should also be called. > > Fixes: 72e89f50084c ("security: Add support for SCTP security hooks") > Reported-by: Prashanth Prahlad <pprahlad@redhat.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..9a38067762e5 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 an temporary association. Nit: s/an/a/ > + 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 an Nit: s/an/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 > -- Ondrej Mosnacek Software Engineer, Linux Security - SELinux kernel Red Hat, Inc. ^ permalink raw reply [flat|nested] 21+ messages in thread
* [PATCH net 3/4] security: add sctp_assoc_established hook 2021-10-22 6:36 [PATCH net 0/4] security: fixups for the security hooks in sctp Xin Long 2021-10-22 6:36 ` [PATCH net 1/4] security: pass asoc to sctp_assoc_request and sctp_sk_clone Xin Long 2021-10-22 6:36 ` [PATCH net 2/4] security: call security_sctp_assoc_request in sctp_sf_do_5_1D_ce Xin Long @ 2021-10-22 6:36 ` Xin Long 2021-10-24 18:45 ` kernel test robot ` (2 more replies) 2021-10-22 6:36 ` [PATCH net 4/4] security: implement sctp_assoc_established hook in selinux Xin Long 2021-10-24 13:42 ` [PATCH net 0/4] security: fixups for the security hooks in sctp Richard Haines 4 siblings, 3 replies; 21+ messages in thread From: Xin Long @ 2021-10-22 6:36 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. Fixes: 72e89f50084c ("security: Add support for SCTP security hooks") Reported-by: Prashanth Prahlad <pprahlad@redhat.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 | 8 ++++++++ net/sctp/sm_statefuns.c | 2 +- security/security.c | 7 +++++++ 6 files changed, 33 insertions(+), 13 deletions(-) diff --git a/Documentation/security/SCTP.rst b/Documentation/security/SCTP.rst index 9a38067762e5..3ebbcd80b3e7 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..11cdddf9685c 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,12 @@ 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) +{ + return 0; +} #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] 21+ messages in thread
* Re: [PATCH net 3/4] security: add sctp_assoc_established hook 2021-10-22 6:36 ` [PATCH net 3/4] security: add sctp_assoc_established hook Xin Long @ 2021-10-24 18:45 ` kernel test robot 2021-10-25 5:01 ` kernel test robot 2021-10-25 8:01 ` Ondrej Mosnacek 2 siblings, 0 replies; 21+ messages in thread From: kernel test robot @ 2021-10-24 18:45 UTC (permalink / raw) To: Xin Long, network dev, selinux, linux-security-module, linux-sctp Cc: llvm, kbuild-all, davem, kuba, Marcelo Ricardo Leitner, James Morris, Paul Moore, Richard Haines [-- Attachment #1: Type: text/plain, Size: 12862 bytes --] Hi Xin, Thank you for the patch! Yet something to improve: [auto build test ERROR on net/master] url: https://github.com/0day-ci/linux/commits/Xin-Long/security-fixups-for-the-security-hooks-in-sctp/20211022-143827 base: https://git.kernel.org/pub/scm/linux/kernel/git/davem/net.git 397430b50a363d8b7bdda00522123f82df6adc5e config: hexagon-buildonly-randconfig-r006-20211024 (attached as .config) compiler: clang version 14.0.0 (https://github.com/llvm/llvm-project a709787cd988aaca847995bd08cc9348c9c6c956) reproduce (this is a W=1 build): wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross chmod +x ~/bin/make.cross # https://github.com/0day-ci/linux/commit/32fba59611e67404b515f7864aa67a3abd2f7978 git remote add linux-review https://github.com/0day-ci/linux git fetch --no-tags linux-review Xin-Long/security-fixups-for-the-security-hooks-in-sctp/20211022-143827 git checkout 32fba59611e67404b515f7864aa67a3abd2f7978 # save the attached .config to linux build tree COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 ARCH=hexagon If you fix the issue, kindly add following tag as appropriate Reported-by: kernel test robot <lkp@intel.com> All error/warnings (new ones prefixed by >>): In file included from fs/open.c:19: >> include/linux/security.h:1651:2: error: void function 'security_sctp_assoc_established' should not return a value [-Wreturn-type] return 0; ^ ~ 1 error generated. -- In file included from fs/pipe.c:17: In file included from include/linux/pseudo_fs.h:4: In file included from include/linux/fs_context.h:14: >> include/linux/security.h:1651:2: error: void function 'security_sctp_assoc_established' should not return a value [-Wreturn-type] return 0; ^ ~ fs/pipe.c:755:15: warning: no previous prototype for function 'account_pipe_buffers' [-Wmissing-prototypes] unsigned long account_pipe_buffers(struct user_struct *user, ^ fs/pipe.c:755:1: note: declare 'static' if the function is not intended to be used outside of this translation unit unsigned long account_pipe_buffers(struct user_struct *user, ^ static fs/pipe.c:761:6: warning: no previous prototype for function 'too_many_pipe_buffers_soft' [-Wmissing-prototypes] bool too_many_pipe_buffers_soft(unsigned long user_bufs) ^ fs/pipe.c:761:1: note: declare 'static' if the function is not intended to be used outside of this translation unit bool too_many_pipe_buffers_soft(unsigned long user_bufs) ^ static fs/pipe.c:768:6: warning: no previous prototype for function 'too_many_pipe_buffers_hard' [-Wmissing-prototypes] bool too_many_pipe_buffers_hard(unsigned long user_bufs) ^ fs/pipe.c:768:1: note: declare 'static' if the function is not intended to be used outside of this translation unit bool too_many_pipe_buffers_hard(unsigned long user_bufs) ^ static fs/pipe.c:775:6: warning: no previous prototype for function 'pipe_is_unprivileged_user' [-Wmissing-prototypes] bool pipe_is_unprivileged_user(void) ^ fs/pipe.c:775:1: note: declare 'static' if the function is not intended to be used outside of this translation unit bool pipe_is_unprivileged_user(void) ^ static fs/pipe.c:1245:5: warning: no previous prototype for function 'pipe_resize_ring' [-Wmissing-prototypes] int pipe_resize_ring(struct pipe_inode_info *pipe, unsigned int nr_slots) ^ fs/pipe.c:1245:1: note: declare 'static' if the function is not intended to be used outside of this translation unit int pipe_resize_ring(struct pipe_inode_info *pipe, unsigned int nr_slots) ^ static 5 warnings and 1 error generated. -- In file included from fs/d_path.c:2: In file included from include/linux/syscalls.h:87: In file included from include/trace/syscall.h:7: In file included from include/linux/trace_events.h:10: In file included from include/linux/perf_event.h:59: >> include/linux/security.h:1651:2: error: void function 'security_sctp_assoc_established' should not return a value [-Wreturn-type] return 0; ^ ~ fs/d_path.c:320:7: warning: no previous prototype for function 'simple_dname' [-Wmissing-prototypes] char *simple_dname(struct dentry *dentry, char *buffer, int buflen) ^ fs/d_path.c:320:1: note: declare 'static' if the function is not intended to be used outside of this translation unit char *simple_dname(struct dentry *dentry, char *buffer, int buflen) ^ static 1 warning and 1 error generated. -- In file included from fs/statfs.c:2: In file included from include/linux/syscalls.h:87: In file included from include/trace/syscall.h:7: In file included from include/linux/trace_events.h:10: In file included from include/linux/perf_event.h:59: >> include/linux/security.h:1651:2: error: void function 'security_sctp_assoc_established' should not return a value [-Wreturn-type] return 0; ^ ~ >> fs/statfs.c:131:3: warning: 'memcpy' will always overflow; destination buffer has size 64, but size argument is 88 [-Wfortify-source] memcpy(&buf, st, sizeof(*st)); ^ 1 warning and 1 error generated. -- In file included from ipc/msg.c:33: >> include/linux/security.h:1651:2: error: void function 'security_sctp_assoc_established' should not return a value [-Wreturn-type] return 0; ^ ~ >> ipc/msg.c:496:20: warning: implicit conversion from 'int' to 'unsigned short' changes value from 32768000 to 0 [-Wconstant-conversion] msginfo->msgseg = MSGSEG; ~ ^~~~~~ include/uapi/linux/msg.h:87:38: note: expanded from macro 'MSGSEG' #define MSGSEG (__MSGSEG <= 0xffff ? __MSGSEG : 0xffff) ^~~~~~~~ include/uapi/linux/msg.h:86:36: note: expanded from macro '__MSGSEG' #define __MSGSEG ((MSGPOOL * 1024) / MSGSSZ) /* max no. of segments */ ~~~~~~~~~~~~~~~~~^~~~~~~~ 1 warning and 1 error generated. -- In file included from kernel/printk/printk.c:34: >> include/linux/security.h:1651:2: error: void function 'security_sctp_assoc_established' should not return a value [-Wreturn-type] return 0; ^ ~ kernel/printk/printk.c:175:5: warning: no previous prototype for function 'devkmsg_sysctl_set_loglvl' [-Wmissing-prototypes] int devkmsg_sysctl_set_loglvl(struct ctl_table *table, int write, ^ kernel/printk/printk.c:175:1: note: declare 'static' if the function is not intended to be used outside of this translation unit int devkmsg_sysctl_set_loglvl(struct ctl_table *table, int write, ^ static 1 warning and 1 error generated. -- In file included from fs/afs/dir.c:16: In file included from fs/afs/internal.h:25: In file included from include/net/sock.h:46: In file included from include/linux/netdevice.h:45: In file included from include/uapi/linux/neighbour.h:6: In file included from include/linux/netlink.h:9: In file included from include/net/scm.h:8: >> include/linux/security.h:1651:2: error: void function 'security_sctp_assoc_established' should not return a value [-Wreturn-type] return 0; ^ ~ fs/afs/dir.c:164:11: warning: format specifies type 'unsigned short' but the argument has type 'int' [-Wformat] ntohs(dbuf->blocks[tmp].hdr.magic)); ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ include/linux/printk.h:446:60: note: expanded from macro 'printk' #define printk(fmt, ...) printk_index_wrap(_printk, fmt, ##__VA_ARGS__) ~~~ ^~~~~~~~~~~ include/linux/printk.h:418:19: note: expanded from macro 'printk_index_wrap' _p_func(_fmt, ##__VA_ARGS__); \ ~~~~ ^~~~~~~~~~~ include/linux/byteorder/generic.h:142:18: note: expanded from macro 'ntohs' #define ntohs(x) ___ntohs(x) ^~~~~~~~~~~ include/linux/byteorder/generic.h:137:21: note: expanded from macro '___ntohs' #define ___ntohs(x) __be16_to_cpu(x) ^~~~~~~~~~~~~~~~ include/uapi/linux/byteorder/little_endian.h:42:26: note: expanded from macro '__be16_to_cpu' #define __be16_to_cpu(x) __swab16((__force __u16)(__be16)(x)) ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ include/uapi/linux/swab.h:105:2: note: expanded from macro '__swab16' (__builtin_constant_p((__u16)(x)) ? \ ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ 1 warning and 1 error generated. -- In file included from drivers/char/mem.c:25: In file included from include/linux/shmem_fs.h:11: In file included from include/linux/fs_parser.h:11: In file included from include/linux/fs_context.h:14: >> include/linux/security.h:1651:2: error: void function 'security_sctp_assoc_established' should not return a value [-Wreturn-type] return 0; ^ ~ drivers/char/mem.c:95:13: warning: no previous prototype for function 'unxlate_dev_mem_ptr' [-Wmissing-prototypes] void __weak unxlate_dev_mem_ptr(phys_addr_t phys, void *addr) ^ drivers/char/mem.c:94:29: note: expanded from macro 'unxlate_dev_mem_ptr' #define unxlate_dev_mem_ptr unxlate_dev_mem_ptr ^ drivers/char/mem.c:95:1: note: declare 'static' if the function is not intended to be used outside of this translation unit void __weak unxlate_dev_mem_ptr(phys_addr_t phys, void *addr) ^ static 1 warning and 1 error generated. -- In file included from drivers/char/random.c:335: In file included from include/linux/syscalls.h:87: In file included from include/trace/syscall.h:7: In file included from include/linux/trace_events.h:10: In file included from include/linux/perf_event.h:59: >> include/linux/security.h:1651:2: error: void function 'security_sctp_assoc_established' should not return a value [-Wreturn-type] return 0; ^ ~ >> drivers/char/random.c:1257:41: warning: shift count >= width of type [-Wshift-count-overflow] c_high = (sizeof(cycles) > 4) ? cycles >> 32 : 0; ^ ~~ drivers/char/random.c:1258:35: warning: shift count >= width of type [-Wshift-count-overflow] j_high = (sizeof(now) > 4) ? now >> 32 : 0; ^ ~~ drivers/char/random.c:2272:6: warning: no previous prototype for function 'add_hwgenerator_randomness' [-Wmissing-prototypes] void add_hwgenerator_randomness(const char *buffer, size_t count, ^ drivers/char/random.c:2272:1: note: declare 'static' if the function is not intended to be used outside of this translation unit void add_hwgenerator_randomness(const char *buffer, size_t count, ^ static 3 warnings and 1 error generated. -- In file included from fs/cifs/ioctl.c:16: In file included from fs/cifs/cifspdu.h:12: In file included from include/net/sock.h:46: In file included from include/linux/netdevice.h:45: In file included from include/uapi/linux/neighbour.h:6: In file included from include/linux/netlink.h:9: In file included from include/net/scm.h:8: >> include/linux/security.h:1651:2: error: void function 'security_sctp_assoc_established' should not return a value [-Wreturn-type] return 0; ^ ~ fs/cifs/ioctl.c:324:10: warning: variable 'caps' set but not used [-Wunused-but-set-variable] __u64 caps; ^ 1 warning and 1 error generated. -- In file included from fs/kernfs/file.c:19: In file included from fs/kernfs/kernfs-internal.h:20: In file included from include/linux/fs_context.h:14: >> include/linux/security.h:1651:2: error: void function 'security_sctp_assoc_established' should not return a value [-Wreturn-type] return 0; ^ ~ fs/kernfs/file.c:128:15: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic] return NULL + !*ppos; ~~~~ ^ 1 warning and 1 error generated. vim +/security_sctp_assoc_established +1651 include/linux/security.h 1647 1648 static inline void security_sctp_assoc_established(struct sctp_association *asoc, 1649 struct sk_buff *skb) 1650 { > 1651 return 0; 1652 } 1653 #endif /* CONFIG_SECURITY_NETWORK */ 1654 --- 0-DAY CI Kernel Test Service, Intel Corporation https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org [-- Attachment #2: .config.gz --] [-- Type: application/gzip, Size: 28898 bytes --] ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH net 3/4] security: add sctp_assoc_established hook 2021-10-22 6:36 ` [PATCH net 3/4] security: add sctp_assoc_established hook Xin Long 2021-10-24 18:45 ` kernel test robot @ 2021-10-25 5:01 ` kernel test robot 2021-10-25 8:01 ` Ondrej Mosnacek 2 siblings, 0 replies; 21+ messages in thread From: kernel test robot @ 2021-10-25 5:01 UTC (permalink / raw) To: Xin Long, network dev, selinux, linux-security-module, linux-sctp Cc: llvm, kbuild-all, davem, kuba, Marcelo Ricardo Leitner, James Morris, Paul Moore, Richard Haines [-- Attachment #1: Type: text/plain, Size: 5004 bytes --] Hi Xin, Thank you for the patch! Perhaps something to improve: [auto build test WARNING on net/master] url: https://github.com/0day-ci/linux/commits/Xin-Long/security-fixups-for-the-security-hooks-in-sctp/20211022-143827 base: https://git.kernel.org/pub/scm/linux/kernel/git/davem/net.git 397430b50a363d8b7bdda00522123f82df6adc5e config: hexagon-randconfig-r041-20211025 (attached as .config) compiler: clang version 14.0.0 (https://github.com/llvm/llvm-project a461fa64bb37cffd73f683c74f6b0780379fc2ca) reproduce (this is a W=1 build): wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross chmod +x ~/bin/make.cross # https://github.com/0day-ci/linux/commit/32fba59611e67404b515f7864aa67a3abd2f7978 git remote add linux-review https://github.com/0day-ci/linux git fetch --no-tags linux-review Xin-Long/security-fixups-for-the-security-hooks-in-sctp/20211022-143827 git checkout 32fba59611e67404b515f7864aa67a3abd2f7978 # save the attached .config to linux build tree COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 ARCH=hexagon If you fix the issue, kindly add following tag as appropriate Reported-by: kernel test robot <lkp@intel.com> All warnings (new ones prefixed by >>): In file included from drivers/gpu/drm/vgem/vgem_drv.c:36: In file included from include/linux/shmem_fs.h:11: In file included from include/linux/fs_parser.h:11: In file included from include/linux/fs_context.h:14: include/linux/security.h:1651:2: error: void function 'security_sctp_assoc_established' should not return a value [-Wreturn-type] return 0; ^ ~ >> drivers/gpu/drm/vgem/vgem_drv.c:460:10: warning: shift count >= width of type [-Wshift-count-overflow] DMA_BIT_MASK(64)); ^~~~~~~~~~~~~~~~ include/linux/dma-mapping.h:76:54: note: expanded from macro 'DMA_BIT_MASK' #define DMA_BIT_MASK(n) (((n) == 64) ? ~0ULL : ((1ULL<<(n))-1)) ^ ~~~ 1 warning and 1 error generated. vim +460 drivers/gpu/drm/vgem/vgem_drv.c 502e95c6678505 Zach Reizner 2015-03-04 444 502e95c6678505 Zach Reizner 2015-03-04 445 static int __init vgem_init(void) 502e95c6678505 Zach Reizner 2015-03-04 446 { 502e95c6678505 Zach Reizner 2015-03-04 447 int ret; bcc0ef7f57e51e Daniel Vetter 2020-09-09 448 struct platform_device *pdev; 502e95c6678505 Zach Reizner 2015-03-04 449 bcc0ef7f57e51e Daniel Vetter 2020-09-09 450 pdev = platform_device_register_simple("vgem", -1, NULL, 0); bcc0ef7f57e51e Daniel Vetter 2020-09-09 451 if (IS_ERR(pdev)) bcc0ef7f57e51e Daniel Vetter 2020-09-09 452 return PTR_ERR(pdev); e2aff44868ae60 Laura Abbott 2017-05-04 453 bcc0ef7f57e51e Daniel Vetter 2020-09-09 454 if (!devres_open_group(&pdev->dev, NULL, GFP_KERNEL)) { bcc0ef7f57e51e Daniel Vetter 2020-09-09 455 ret = -ENOMEM; bcc0ef7f57e51e Daniel Vetter 2020-09-09 456 goto out_unregister; 502e95c6678505 Zach Reizner 2015-03-04 457 } 502e95c6678505 Zach Reizner 2015-03-04 458 bcc0ef7f57e51e Daniel Vetter 2020-09-09 459 dma_coerce_mask_and_coherent(&pdev->dev, e2aff44868ae60 Laura Abbott 2017-05-04 @460 DMA_BIT_MASK(64)); bcc0ef7f57e51e Daniel Vetter 2020-09-09 461 bcc0ef7f57e51e Daniel Vetter 2020-09-09 462 vgem_device = devm_drm_dev_alloc(&pdev->dev, &vgem_driver, bcc0ef7f57e51e Daniel Vetter 2020-09-09 463 struct vgem_device, drm); bcc0ef7f57e51e Daniel Vetter 2020-09-09 464 if (IS_ERR(vgem_device)) { bcc0ef7f57e51e Daniel Vetter 2020-09-09 465 ret = PTR_ERR(vgem_device); bcc0ef7f57e51e Daniel Vetter 2020-09-09 466 goto out_devres; bcc0ef7f57e51e Daniel Vetter 2020-09-09 467 } bcc0ef7f57e51e Daniel Vetter 2020-09-09 468 vgem_device->platform = pdev; e2aff44868ae60 Laura Abbott 2017-05-04 469 315f0242aa2b1e Chris Wilson 2017-05-08 470 /* Final step: expose the device/driver to userspace */ 315f0242aa2b1e Chris Wilson 2017-05-08 471 ret = drm_dev_register(&vgem_device->drm, 0); 502e95c6678505 Zach Reizner 2015-03-04 472 if (ret) bcc0ef7f57e51e Daniel Vetter 2020-09-09 473 goto out_devres; 502e95c6678505 Zach Reizner 2015-03-04 474 502e95c6678505 Zach Reizner 2015-03-04 475 return 0; 502e95c6678505 Zach Reizner 2015-03-04 476 bcc0ef7f57e51e Daniel Vetter 2020-09-09 477 out_devres: bcc0ef7f57e51e Daniel Vetter 2020-09-09 478 devres_release_group(&pdev->dev, NULL); d5c04dff24870e Deepak Sharma 2018-10-23 479 out_unregister: bcc0ef7f57e51e Daniel Vetter 2020-09-09 480 platform_device_unregister(pdev); 502e95c6678505 Zach Reizner 2015-03-04 481 return ret; 502e95c6678505 Zach Reizner 2015-03-04 482 } 502e95c6678505 Zach Reizner 2015-03-04 483 --- 0-DAY CI Kernel Test Service, Intel Corporation https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org [-- Attachment #2: .config.gz --] [-- Type: application/gzip, Size: 27999 bytes --] ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH net 3/4] security: add sctp_assoc_established hook 2021-10-22 6:36 ` [PATCH net 3/4] security: add sctp_assoc_established hook Xin Long 2021-10-24 18:45 ` kernel test robot 2021-10-25 5:01 ` kernel test robot @ 2021-10-25 8:01 ` Ondrej Mosnacek 2 siblings, 0 replies; 21+ messages in thread From: Ondrej Mosnacek @ 2021-10-25 8:01 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 On Fri, Oct 22, 2021 at 8:36 AM Xin Long <lucien.xin@gmail.com> wrote: > > 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. > > Fixes: 72e89f50084c ("security: Add support for SCTP security hooks") > Reported-by: Prashanth Prahlad <pprahlad@redhat.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 | 8 ++++++++ > net/sctp/sm_statefuns.c | 2 +- > security/security.c | 7 +++++++ > 6 files changed, 33 insertions(+), 13 deletions(-) [...] > diff --git a/include/linux/security.h b/include/linux/security.h > index a16407444871..11cdddf9685c 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,12 @@ 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) > +{ > + return 0; It has now been pointed out by the kernel robot as well, but you are returning a value from a function with return type void here. > +} > #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 > -- Ondrej Mosnacek Software Engineer, Linux Security - SELinux kernel Red Hat, Inc. ^ permalink raw reply [flat|nested] 21+ messages in thread
* [PATCH net 4/4] security: implement sctp_assoc_established hook in selinux 2021-10-22 6:36 [PATCH net 0/4] security: fixups for the security hooks in sctp Xin Long ` (2 preceding siblings ...) 2021-10-22 6:36 ` [PATCH net 3/4] security: add sctp_assoc_established hook Xin Long @ 2021-10-22 6:36 ` Xin Long 2021-10-25 8:17 ` Ondrej Mosnacek 2021-10-24 13:42 ` [PATCH net 0/4] security: fixups for the security hooks in sctp Richard Haines 4 siblings, 1 reply; 21+ messages in thread From: Xin Long @ 2021-10-22 6:36 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. Fixes: 72e89f50084c ("security: Add support for SCTP security hooks") Reported-by: Prashanth Prahlad <pprahlad@redhat.com> Signed-off-by: Xin Long <lucien.xin@gmail.com> --- security/selinux/hooks.c | 16 ++++++++++++++++ 1 file changed, 16 insertions(+) diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c index f025fc00421b..793fdcbc68bd 100644 --- a/security/selinux/hooks.c +++ b/security/selinux/hooks.c @@ -5525,6 +5525,21 @@ static void selinux_sctp_sk_clone(struct sctp_association *asoc, struct sock *sk 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; + u16 family = asoc->base.sk->sk_family; + + /* handle mapped IPv4 packets arriving via IPv6 sockets */ + if (family == PF_INET6 && skb->protocol == htons(ETH_P_IP)) + family = PF_INET; + + selinux_skb_peerlbl_sid(skb, family, &sksec->peer_sid); + asoc->secid = sksec->sid; + asoc->peer_secid = sksec->peer_sid; +} + static int selinux_inet_conn_request(const struct sock *sk, struct sk_buff *skb, struct request_sock *req) { @@ -7290,6 +7305,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] 21+ messages in thread
* Re: [PATCH net 4/4] security: implement sctp_assoc_established hook in selinux 2021-10-22 6:36 ` [PATCH net 4/4] security: implement sctp_assoc_established hook in selinux Xin Long @ 2021-10-25 8:17 ` Ondrej Mosnacek 2021-10-25 10:51 ` Xin Long 0 siblings, 1 reply; 21+ messages in thread From: Ondrej Mosnacek @ 2021-10-25 8:17 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 On Fri, Oct 22, 2021 at 8:36 AM 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. Hm... this sounds like something we should also try to fix (if possible). In access control we can't trust userspace to do the right thing - receiving from multiple peers on one SOCK_SEQPACKET socket shouldn't cause checking against the wrong peer_sid. But that can be addressed separately. (And maybe it's even already accounted for somehow - I didn't yet look at the code closely.) > > Fixes: 72e89f50084c ("security: Add support for SCTP security hooks") > Reported-by: Prashanth Prahlad <pprahlad@redhat.com> > Signed-off-by: Xin Long <lucien.xin@gmail.com> > --- > security/selinux/hooks.c | 16 ++++++++++++++++ > 1 file changed, 16 insertions(+) > > diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c > index f025fc00421b..793fdcbc68bd 100644 > --- a/security/selinux/hooks.c > +++ b/security/selinux/hooks.c > @@ -5525,6 +5525,21 @@ static void selinux_sctp_sk_clone(struct sctp_association *asoc, struct sock *sk > 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; > + u16 family = asoc->base.sk->sk_family; > + > + /* handle mapped IPv4 packets arriving via IPv6 sockets */ > + if (family == PF_INET6 && skb->protocol == htons(ETH_P_IP)) > + family = PF_INET; > + > + selinux_skb_peerlbl_sid(skb, family, &sksec->peer_sid); You could replace the above with `selinux_inet_conn_established(asoc->base.sk, skb);` to reduce code duplication. > + asoc->secid = sksec->sid; > + asoc->peer_secid = sksec->peer_sid; > +} > + > static int selinux_inet_conn_request(const struct sock *sk, struct sk_buff *skb, > struct request_sock *req) > { > @@ -7290,6 +7305,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] 21+ messages in thread
* Re: [PATCH net 4/4] security: implement sctp_assoc_established hook in selinux 2021-10-25 8:17 ` Ondrej Mosnacek @ 2021-10-25 10:51 ` Xin Long 2021-10-25 12:08 ` Ondrej Mosnacek 0 siblings, 1 reply; 21+ messages in thread From: Xin Long @ 2021-10-25 10:51 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 Mon, Oct 25, 2021 at 4:17 PM Ondrej Mosnacek <omosnace@redhat.com> wrote: > > On Fri, Oct 22, 2021 at 8:36 AM 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. > > Hm... this sounds like something we should also try to fix (if > possible). In access control we can't trust userspace to do the right > thing - receiving from multiple peers on one SOCK_SEQPACKET socket > shouldn't cause checking against the wrong peer_sid. But that can be > addressed separately. (And maybe it's even already accounted for > somehow - I didn't yet look at the code closely.) > > > > > Fixes: 72e89f50084c ("security: Add support for SCTP security hooks") > > Reported-by: Prashanth Prahlad <pprahlad@redhat.com> > > Signed-off-by: Xin Long <lucien.xin@gmail.com> > > --- > > security/selinux/hooks.c | 16 ++++++++++++++++ > > 1 file changed, 16 insertions(+) > > > > diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c > > index f025fc00421b..793fdcbc68bd 100644 > > --- a/security/selinux/hooks.c > > +++ b/security/selinux/hooks.c > > @@ -5525,6 +5525,21 @@ static void selinux_sctp_sk_clone(struct sctp_association *asoc, struct sock *sk > > 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; > > + u16 family = asoc->base.sk->sk_family; > > + > > + /* handle mapped IPv4 packets arriving via IPv6 sockets */ > > + if (family == PF_INET6 && skb->protocol == htons(ETH_P_IP)) > > + family = PF_INET; > > + > > + selinux_skb_peerlbl_sid(skb, family, &sksec->peer_sid); > > You could replace the above with > `selinux_inet_conn_established(asoc->base.sk, skb);` to reduce code > duplication. Hi Ondrej, will do, thanks! > > > + asoc->secid = sksec->sid; > > + asoc->peer_secid = sksec->peer_sid; > > +} > > + Now I'm thinking: 'peer_sid' should be correct here. BUT 'sid' is copied from its parent socket. Later when doing peel-off, asoc->secid will be set back to the peel-off socket's sksec->sid. Do you think this is okay? or should the peel-off socket have its own sksec->sid, which might be different from the parent socket's? (Note the socket's sid initially was set in selinux_socket_post_create()) > > static int selinux_inet_conn_request(const struct sock *sk, struct sk_buff *skb, > > struct request_sock *req) > > { > > @@ -7290,6 +7305,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] 21+ messages in thread
* Re: [PATCH net 4/4] security: implement sctp_assoc_established hook in selinux 2021-10-25 10:51 ` Xin Long @ 2021-10-25 12:08 ` Ondrej Mosnacek [not found] ` <CADvbK_eE9VhB2cWzHSk_LNm_VemEt9vm=FMMVYzo5eVH=zEhKw@mail.gmail.com> 0 siblings, 1 reply; 21+ messages in thread From: Ondrej Mosnacek @ 2021-10-25 12:08 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, Stephen Smalley On Mon, Oct 25, 2021 at 12:51 PM Xin Long <lucien.xin@gmail.com> wrote: > > On Mon, Oct 25, 2021 at 4:17 PM Ondrej Mosnacek <omosnace@redhat.com> wrote: > > > > On Fri, Oct 22, 2021 at 8:36 AM 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. > > > > Hm... this sounds like something we should also try to fix (if > > possible). In access control we can't trust userspace to do the right > > thing - receiving from multiple peers on one SOCK_SEQPACKET socket > > shouldn't cause checking against the wrong peer_sid. But that can be > > addressed separately. (And maybe it's even already accounted for > > somehow - I didn't yet look at the code closely.) > > > > > > > > Fixes: 72e89f50084c ("security: Add support for SCTP security hooks") > > > Reported-by: Prashanth Prahlad <pprahlad@redhat.com> > > > Signed-off-by: Xin Long <lucien.xin@gmail.com> > > > --- > > > security/selinux/hooks.c | 16 ++++++++++++++++ > > > 1 file changed, 16 insertions(+) > > > > > > diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c > > > index f025fc00421b..793fdcbc68bd 100644 > > > --- a/security/selinux/hooks.c > > > +++ b/security/selinux/hooks.c > > > @@ -5525,6 +5525,21 @@ static void selinux_sctp_sk_clone(struct sctp_association *asoc, struct sock *sk > > > 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; > > > + u16 family = asoc->base.sk->sk_family; > > > + > > > + /* handle mapped IPv4 packets arriving via IPv6 sockets */ > > > + if (family == PF_INET6 && skb->protocol == htons(ETH_P_IP)) > > > + family = PF_INET; > > > + > > > + selinux_skb_peerlbl_sid(skb, family, &sksec->peer_sid); > > > > You could replace the above with > > `selinux_inet_conn_established(asoc->base.sk, skb);` to reduce code > > duplication. > Hi Ondrej, > > will do, thanks! > > > > > > + asoc->secid = sksec->sid; > > > + asoc->peer_secid = sksec->peer_sid; > > > +} > > > + > Now I'm thinking: 'peer_sid' should be correct here. > > BUT 'sid' is copied from its parent socket. Later when doing peel-off, > asoc->secid will be set back to the peel-off socket's sksec->sid. Hi, I'm not sure I follow... When doing peel-off, security_sctp_sk_clone() should be called, which sets the peel-off socket's sksec->sid to asoc->secid, not the other way around. (Are we hitting the language barrier here? :) > Do you think this is okay? or should the peel-off socket have its own > sksec->sid, which might be different from the parent socket's? > (Note the socket's sid initially was set in selinux_socket_post_create()) I *think* in case of a client socket it is expected for the peeloff-style child socket to just inherit the same sksec->sid. But frankly I haven't been with SELinux and kernel development long enough to understand the intricacies of SELinux's network connection handling very well... Hopefully Paul/Richard/Stephen can give a more confident answer/review here. -- Ondrej Mosnacek Software Engineer, Linux Security - SELinux kernel Red Hat, Inc. ^ permalink raw reply [flat|nested] 21+ messages in thread
[parent not found: <CADvbK_eE9VhB2cWzHSk_LNm_VemEt9vm=FMMVYzo5eVH=zEhKw@mail.gmail.com>]
* Re: [PATCH net 4/4] security: implement sctp_assoc_established hook in selinux [not found] ` <CADvbK_eE9VhB2cWzHSk_LNm_VemEt9vm=FMMVYzo5eVH=zEhKw@mail.gmail.com> @ 2021-10-25 21:51 ` Paul Moore 2021-10-26 4:47 ` Xin Long 0 siblings, 1 reply; 21+ messages in thread From: Paul Moore @ 2021-10-25 21:51 UTC (permalink / raw) To: Xin Long Cc: Ondrej Mosnacek, David S. Miller, Jakub Kicinski, James Morris, Linux Security Module list, Marcelo Ricardo Leitner, Richard Haines, SElinux list, Stephen Smalley, linux-sctp @ vger . kernel . org, network dev On Mon, Oct 25, 2021 at 10:11 AM Xin Long <lucien.xin@gmail.com> wrote: > On Mon, Oct 25, 2021 at 8:08 PM Ondrej Mosnacek <omosnace@redhat.com> wrote: >> On Mon, Oct 25, 2021 at 12:51 PM Xin Long <lucien.xin@gmail.com> wrote: >> > On Mon, Oct 25, 2021 at 4:17 PM Ondrej Mosnacek <omosnace@redhat.com> wrote: >> > > On Fri, Oct 22, 2021 at 8:36 AM 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. >> > > >> > > Hm... this sounds like something we should also try to fix (if >> > > possible). In access control we can't trust userspace to do the right >> > > thing - receiving from multiple peers on one SOCK_SEQPACKET socket >> > > shouldn't cause checking against the wrong peer_sid. But that can be >> > > addressed separately. (And maybe it's even already accounted for >> > > somehow - I didn't yet look at the code closely.) There are a couple of things we need to worry about here: the per-packet access controls (e.g. can this packet be received by this socket?) and the userspace peer label queries (e.g. SO_GETPEERSEC and IP_CMSG_PASSSEC). The per-packet access controls work by checking the individual packet's security label against the corresponding sock label on the system (sk->sk_security->sid). Because of this it is important that the sock's label is correct. For unconnected sockets this is fairly straightforward as it follows the usual inherit-from-parent[1] behavior we see in other areas of SELinux. For connected stream sockets this can be a bit more complicated. However, since we are only discussing the client side things aren't too bad with the behavior essentially the same, inherit-from-parent, with the only interesting piece worth noting being the sksec->peer_sid (sk->sk_security->peer_sid) that we record from the packet passed to the LSM/SELinux hook (using selinux_skb_peerlbl_sid()). The sksec->peer_sid is recorded primarily so that the kernel can correctly respond to SO_GETPEERSEC requests from userspace; it shouldn't be used in any access control decisions. In the case of SCTP, I would expect things to behave similarly: the sksec->peer_sid should match the packet label of the traffic which acknowledged/accepted the new connection, e.g. the other end of the connected socket. You will have to forgive me some of the details, it's been a while since I last looked at the SCTP bits, but I would expect that if a client created a new connection and/or spun-off a new socket the new socket's sksec->peer_sid would have the same property, it would represent the security label of the other end of the connection/association. [1] Yes, there is setsockcreatecon(), but that isn't important for this discussion. >> > > > Fixes: 72e89f50084c ("security: Add support for SCTP security hooks") >> > > > Reported-by: Prashanth Prahlad <pprahlad@redhat.com> >> > > > Signed-off-by: Xin Long <lucien.xin@gmail.com> >> > > > --- >> > > > security/selinux/hooks.c | 16 ++++++++++++++++ >> > > > 1 file changed, 16 insertions(+) >> > > > >> > > > diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c >> > > > index f025fc00421b..793fdcbc68bd 100644 >> > > > --- a/security/selinux/hooks.c >> > > > +++ b/security/selinux/hooks.c >> > > > @@ -5525,6 +5525,21 @@ static void selinux_sctp_sk_clone(struct sctp_association *asoc, struct sock *sk >> > > > 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; >> > > > + u16 family = asoc->base.sk->sk_family; >> > > > + >> > > > + /* handle mapped IPv4 packets arriving via IPv6 sockets */ >> > > > + if (family == PF_INET6 && skb->protocol == htons(ETH_P_IP)) >> > > > + family = PF_INET; >> > > > + >> > > > + selinux_skb_peerlbl_sid(skb, family, &sksec->peer_sid); >> > > >> > > You could replace the above with >> > > `selinux_inet_conn_established(asoc->base.sk, skb);` to reduce code >> > > duplication. >> > Hi Ondrej, >> > >> > will do, thanks! >> > >> > > >> > > > + asoc->secid = sksec->sid; >> > > > + asoc->peer_secid = sksec->peer_sid; >> > > > +} >> > > > + >> > Now I'm thinking: 'peer_sid' should be correct here. >> > >> > BUT 'sid' is copied from its parent socket. Later when doing peel-off, >> > asoc->secid will be set back to the peel-off socket's sksec->sid. >> >> Hi, >> >> I'm not sure I follow... When doing peel-off, security_sctp_sk_clone() >> should be called, which sets the peel-off socket's sksec->sid to >> asoc->secid, not the other way around. (Are we hitting the language >> barrier here? :) > > Right, sorry. > > Set the peel-off socket's sksec->sid to asoc->secid, I meant :D For the sake of clarity, let's scribble down some pseudo code to discuss :) Taking into account the feedback above, I arrived at the code below (corrections are welcome if I misunderstood what you wanted to convey) with my comments after: static void selinux_sctp_assoc_established(asoc, skb) { struct sock *sk = asoc->base.sk; struct sk_security_struct *sksec = sk->sk_security; selinux_inet_conn_established(sk, skb); asoc->secid = sksec->peer_sid; asoc->peer_secid = sksec->peer_sid; } My only concern with the above code is the 'asoc->secid = sksec->peer_sid' assignment. As this particular association is a client side association I would expect it to follow the normal inherit-from-parent behavior as described above and not take the label of remote peer, however I could be misunderstanding some of the SCTP specifics here. My initial reaction is that we need to adjust the LSM/SELinux hook as well as the call site in sctp_sf_do_5_1D_ce() to pass both 'new_asoc' as well 'asoc' and set 'new_asoc->secid' to 'asoc->secid' to better mirror the existing stream/TCP behavior on the client side. Does that make sense? If not, what am I missing :) -- paul moore www.paul-moore.com ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH net 4/4] security: implement sctp_assoc_established hook in selinux 2021-10-25 21:51 ` Paul Moore @ 2021-10-26 4:47 ` Xin Long 2021-10-26 20:30 ` Paul Moore 0 siblings, 1 reply; 21+ messages in thread From: Xin Long @ 2021-10-26 4:47 UTC (permalink / raw) To: Paul Moore Cc: Ondrej Mosnacek, David S. Miller, Jakub Kicinski, James Morris, Linux Security Module list, Marcelo Ricardo Leitner, Richard Haines, SElinux list, Stephen Smalley, linux-sctp @ vger . kernel . org, network dev On Tue, Oct 26, 2021 at 5:51 AM Paul Moore <paul@paul-moore.com> wrote: > > On Mon, Oct 25, 2021 at 10:11 AM Xin Long <lucien.xin@gmail.com> wrote: > > On Mon, Oct 25, 2021 at 8:08 PM Ondrej Mosnacek <omosnace@redhat.com> wrote: > >> On Mon, Oct 25, 2021 at 12:51 PM Xin Long <lucien.xin@gmail.com> wrote: > >> > On Mon, Oct 25, 2021 at 4:17 PM Ondrej Mosnacek <omosnace@redhat.com> wrote: > >> > > On Fri, Oct 22, 2021 at 8:36 AM 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. > >> > > > >> > > Hm... this sounds like something we should also try to fix (if > >> > > possible). In access control we can't trust userspace to do the right > >> > > thing - receiving from multiple peers on one SOCK_SEQPACKET socket > >> > > shouldn't cause checking against the wrong peer_sid. But that can be > >> > > addressed separately. (And maybe it's even already accounted for > >> > > somehow - I didn't yet look at the code closely.) > > There are a couple of things we need to worry about here: the > per-packet access controls (e.g. can this packet be received by this > socket?) and the userspace peer label queries (e.g. SO_GETPEERSEC and > IP_CMSG_PASSSEC). > > The per-packet access controls work by checking the individual > packet's security label against the corresponding sock label on the > system (sk->sk_security->sid). Because of this it is important that > the sock's label is correct. For unconnected sockets this is fairly > straightforward as it follows the usual inherit-from-parent[1] > behavior we see in other areas of SELinux. For connected stream > sockets this can be a bit more complicated. However, since we are > only discussing the client side things aren't too bad with the > behavior essentially the same, inherit-from-parent, with the only > interesting piece worth noting being the sksec->peer_sid > (sk->sk_security->peer_sid) that we record from the packet passed to > the LSM/SELinux hook (using selinux_skb_peerlbl_sid()). The > sksec->peer_sid is recorded primarily so that the kernel can correctly > respond to SO_GETPEERSEC requests from userspace; it shouldn't be used > in any access control decisions. Hi, Paul Understand now, the issue reported seems caused by when doing peel-off the peel-off socket gets the uninitialised sid from 'ep' on the client, though it should be "asoc". Thanks! > > In the case of SCTP, I would expect things to behave similarly: the > sksec->peer_sid should match the packet label of the traffic which > acknowledged/accepted the new connection, e.g. the other end of the > connected socket. You will have to forgive me some of the details, > it's been a while since I last looked at the SCTP bits, but I would > expect that if a client created a new connection and/or spun-off a new > socket the new socket's sksec->peer_sid would have the same property, > it would represent the security label of the other end of the > connection/association. In SCTP, a socket doesn't represent a peer connection, it's more an object binding some addresses and receiving incoming connecting request, then creates 'asoc' to represent the connection, so asoc-> peer_secid represents the security label of the other end of the connection/association. After doing peel-off, it makes one asoc 'bind' to one new socket, and this socket is used for userspace to control this asoc (conection), so naturally we set sksec->peer_sid to asoc->secid for access control in socket. As for the 'parent' sk/ep, we can't use it to record peer_scid, as when there are multiple connections, the later one will overwrite the older one. > > [1] Yes, there is setsockcreatecon(), but that isn't important for > this discussion. > > >> > > > Fixes: 72e89f50084c ("security: Add support for SCTP security hooks") > >> > > > Reported-by: Prashanth Prahlad <pprahlad@redhat.com> > >> > > > Signed-off-by: Xin Long <lucien.xin@gmail.com> > >> > > > --- > >> > > > security/selinux/hooks.c | 16 ++++++++++++++++ > >> > > > 1 file changed, 16 insertions(+) > >> > > > > >> > > > diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c > >> > > > index f025fc00421b..793fdcbc68bd 100644 > >> > > > --- a/security/selinux/hooks.c > >> > > > +++ b/security/selinux/hooks.c > >> > > > @@ -5525,6 +5525,21 @@ static void selinux_sctp_sk_clone(struct sctp_association *asoc, struct sock *sk > >> > > > 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; > >> > > > + u16 family = asoc->base.sk->sk_family; > >> > > > + > >> > > > + /* handle mapped IPv4 packets arriving via IPv6 sockets */ > >> > > > + if (family == PF_INET6 && skb->protocol == htons(ETH_P_IP)) > >> > > > + family = PF_INET; > >> > > > + > >> > > > + selinux_skb_peerlbl_sid(skb, family, &sksec->peer_sid); > >> > > > >> > > You could replace the above with > >> > > `selinux_inet_conn_established(asoc->base.sk, skb);` to reduce code > >> > > duplication. > >> > Hi Ondrej, > >> > > >> > will do, thanks! > >> > > >> > > > >> > > > + asoc->secid = sksec->sid; > >> > > > + asoc->peer_secid = sksec->peer_sid; > >> > > > +} > >> > > > + > >> > Now I'm thinking: 'peer_sid' should be correct here. > >> > > >> > BUT 'sid' is copied from its parent socket. Later when doing peel-off, > >> > asoc->secid will be set back to the peel-off socket's sksec->sid. > >> > >> Hi, > >> > >> I'm not sure I follow... When doing peel-off, security_sctp_sk_clone() > >> should be called, which sets the peel-off socket's sksec->sid to > >> asoc->secid, not the other way around. (Are we hitting the language > >> barrier here? :) > > > > Right, sorry. > > > > Set the peel-off socket's sksec->sid to asoc->secid, I meant :D > > For the sake of clarity, let's scribble down some pseudo code to > discuss :) Taking into account the feedback above, I arrived at the > code below (corrections are welcome if I misunderstood what you wanted > to convey) with my comments after: > > static void selinux_sctp_assoc_established(asoc, skb) > { > struct sock *sk = asoc->base.sk; > struct sk_security_struct *sksec = sk->sk_security; > > selinux_inet_conn_established(sk, skb); > asoc->secid = sksec->peer_sid; > asoc->peer_secid = sksec->peer_sid; > } > > My only concern with the above code is the 'asoc->secid = > sksec->peer_sid' assignment. As this particular association is a > client side association I would expect it to follow the normal > inherit-from-parent behavior as described above and not take the label > of remote peer, however I could be misunderstanding some of the SCTP selinux_sctp_assoc_established() is the first place to set sksec->peer_sid for this connection on client, so sksec->peer_sid is the 'parent' socket's peer_sid to this asoc. > specifics here. My initial reaction is that we need to adjust the > LSM/SELinux hook as well as the call site in sctp_sf_do_5_1D_ce() to > pass both 'new_asoc' as well 'asoc' and set 'new_asoc->secid' to > 'asoc->secid' to better mirror the existing stream/TCP behavior on the > client side. Sorry, there's something about SCTP that I should've made clear: sctp_sf_do_5_1D_ce() is normally called when no existing asoc (in most cases) or the existing asoc's state is CLOSE. #define TYPE_SCTP_COOKIE_ECHO { \ /* SCTP_STATE_CLOSED */ \ <---- (also include asoc is NULL) TYPE_SCTP_FUNC(sctp_sf_do_5_1D_ce), \ and for the existing asoc case, SCTP doesn't use anything from it for the new asoc, but just ignores it. IF these are conflicts when inserting transports for the new asoc with the existing asoc, sctp_process_init() will handle it and return errors. Just note that the existing asoc is NOT the one created when processing INIT chunk, and that INIT one is a temporary asoc and is deleted right after finishing processing INIT. But if you mean the hooks in sctp_sf_do_5_2_4_dupcook() where it's processing a duplicate COOKIE_ECHO and copying things from the existing asoc, maybe it should do such things, we can make another thread for this corner case. > > Does that make sense? If not, what am I missing :) > > -- > paul moore > www.paul-moore.com ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH net 4/4] security: implement sctp_assoc_established hook in selinux 2021-10-26 4:47 ` Xin Long @ 2021-10-26 20:30 ` Paul Moore 2021-10-27 4:00 ` Xin Long 0 siblings, 1 reply; 21+ messages in thread From: Paul Moore @ 2021-10-26 20:30 UTC (permalink / raw) To: Xin Long Cc: Ondrej Mosnacek, David S. Miller, Jakub Kicinski, James Morris, Linux Security Module list, Marcelo Ricardo Leitner, Richard Haines, SElinux list, Stephen Smalley, linux-sctp @ vger . kernel . org, network dev On Tue, Oct 26, 2021 at 12:47 AM Xin Long <lucien.xin@gmail.com> wrote: > On Tue, Oct 26, 2021 at 5:51 AM Paul Moore <paul@paul-moore.com> wrote: > > On Mon, Oct 25, 2021 at 10:11 AM Xin Long <lucien.xin@gmail.com> wrote: > > > On Mon, Oct 25, 2021 at 8:08 PM Ondrej Mosnacek <omosnace@redhat.com> wrote: > > >> On Mon, Oct 25, 2021 at 12:51 PM Xin Long <lucien.xin@gmail.com> wrote: > > >> > On Mon, Oct 25, 2021 at 4:17 PM Ondrej Mosnacek <omosnace@redhat.com> wrote: > > >> > > On Fri, Oct 22, 2021 at 8:36 AM 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. > > >> > > > > >> > > Hm... this sounds like something we should also try to fix (if > > >> > > possible). In access control we can't trust userspace to do the right > > >> > > thing - receiving from multiple peers on one SOCK_SEQPACKET socket > > >> > > shouldn't cause checking against the wrong peer_sid. But that can be > > >> > > addressed separately. (And maybe it's even already accounted for > > >> > > somehow - I didn't yet look at the code closely.) > > > > There are a couple of things we need to worry about here: the > > per-packet access controls (e.g. can this packet be received by this > > socket?) and the userspace peer label queries (e.g. SO_GETPEERSEC and > > IP_CMSG_PASSSEC). > > > > The per-packet access controls work by checking the individual > > packet's security label against the corresponding sock label on the > > system (sk->sk_security->sid). Because of this it is important that > > the sock's label is correct. For unconnected sockets this is fairly > > straightforward as it follows the usual inherit-from-parent[1] > > behavior we see in other areas of SELinux. For connected stream > > sockets this can be a bit more complicated. However, since we are > > only discussing the client side things aren't too bad with the > > behavior essentially the same, inherit-from-parent, with the only > > interesting piece worth noting being the sksec->peer_sid > > (sk->sk_security->peer_sid) that we record from the packet passed to > > the LSM/SELinux hook (using selinux_skb_peerlbl_sid()). The > > sksec->peer_sid is recorded primarily so that the kernel can correctly > > respond to SO_GETPEERSEC requests from userspace; it shouldn't be used > > in any access control decisions. > > Hi, Paul > > Understand now, the issue reported seems caused by when > doing peel-off the peel-off socket gets the uninitialised sid > from 'ep' on the client, though it should be "asoc". Hi Xin Long, Yes, that is my understanding. I got the impression from the thread that there was some confusion about the different labels and what they were used for in SELinux, I was trying to provide some background in the text above. If you are already familiar with how things should work you can disregard it :) > > In the case of SCTP, I would expect things to behave similarly: the > > sksec->peer_sid should match the packet label of the traffic which > > acknowledged/accepted the new connection, e.g. the other end of the > > connected socket. You will have to forgive me some of the details, > > it's been a while since I last looked at the SCTP bits, but I would > > expect that if a client created a new connection and/or spun-off a new > > socket the new socket's sksec->peer_sid would have the same property, > > it would represent the security label of the other end of the > > connection/association. > > In SCTP, a socket doesn't represent a peer connection, it's more an > object binding some addresses and receiving incoming connecting > request, then creates 'asoc' to represent the connection, so asoc-> > peer_secid represents the security label of the other end of the > connection/association. As mentioned previously the asoc->peer_secid *should* be the security label of the remote end, so I think we are okay here. My concern remains the asoc->secid label as I don't believe it is being set to the correct value (more on that below). > After doing peel-off, it makes one asoc 'bind' to one new socket, > and this socket is used for userspace to control this asoc (conection), > so naturally we set sksec->peer_sid to asoc->secid for access control > in socket. The sksec->peer_sid represents the security label of the remote end so it should be set to the asoc->peer_secid and *not* the asoc->secid value. Yes, they are presently the same value in your patches, but I believe that is a mistake; I believe the asoc->secid value should be set to that of the parent (see the prior inherit-from-parent discussion) which in this case would likely be either the parent association or the client process, I'm not entirely clear on which is correct in the SCTP case. 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. -- paul moore www.paul-moore.com ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH net 4/4] security: implement sctp_assoc_established hook in selinux 2021-10-26 20:30 ` Paul Moore @ 2021-10-27 4:00 ` Xin Long 2021-10-27 14:41 ` Paul Moore 0 siblings, 1 reply; 21+ messages in thread From: Xin Long @ 2021-10-27 4:00 UTC (permalink / raw) To: Paul Moore Cc: Ondrej Mosnacek, David S. Miller, Jakub Kicinski, James Morris, Linux Security Module list, Marcelo Ricardo Leitner, Richard Haines, SElinux list, Stephen Smalley, linux-sctp @ vger . kernel . org, network dev On Wed, Oct 27, 2021 at 4:30 AM Paul Moore <paul@paul-moore.com> wrote: > > On Tue, Oct 26, 2021 at 12:47 AM Xin Long <lucien.xin@gmail.com> wrote: > > On Tue, Oct 26, 2021 at 5:51 AM Paul Moore <paul@paul-moore.com> wrote: > > > On Mon, Oct 25, 2021 at 10:11 AM Xin Long <lucien.xin@gmail.com> wrote: > > > > On Mon, Oct 25, 2021 at 8:08 PM Ondrej Mosnacek <omosnace@redhat.com> wrote: > > > >> On Mon, Oct 25, 2021 at 12:51 PM Xin Long <lucien.xin@gmail.com> wrote: > > > >> > On Mon, Oct 25, 2021 at 4:17 PM Ondrej Mosnacek <omosnace@redhat.com> wrote: > > > >> > > On Fri, Oct 22, 2021 at 8:36 AM 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. > > > >> > > > > > >> > > Hm... this sounds like something we should also try to fix (if > > > >> > > possible). In access control we can't trust userspace to do the right > > > >> > > thing - receiving from multiple peers on one SOCK_SEQPACKET socket > > > >> > > shouldn't cause checking against the wrong peer_sid. But that can be > > > >> > > addressed separately. (And maybe it's even already accounted for > > > >> > > somehow - I didn't yet look at the code closely.) > > > > > > There are a couple of things we need to worry about here: the > > > per-packet access controls (e.g. can this packet be received by this > > > socket?) and the userspace peer label queries (e.g. SO_GETPEERSEC and > > > IP_CMSG_PASSSEC). > > > > > > The per-packet access controls work by checking the individual > > > packet's security label against the corresponding sock label on the > > > system (sk->sk_security->sid). Because of this it is important that > > > the sock's label is correct. For unconnected sockets this is fairly > > > straightforward as it follows the usual inherit-from-parent[1] > > > behavior we see in other areas of SELinux. For connected stream > > > sockets this can be a bit more complicated. However, since we are > > > only discussing the client side things aren't too bad with the > > > behavior essentially the same, inherit-from-parent, with the only > > > interesting piece worth noting being the sksec->peer_sid > > > (sk->sk_security->peer_sid) that we record from the packet passed to > > > the LSM/SELinux hook (using selinux_skb_peerlbl_sid()). The > > > sksec->peer_sid is recorded primarily so that the kernel can correctly > > > respond to SO_GETPEERSEC requests from userspace; it shouldn't be used > > > in any access control decisions. > > > > Hi, Paul > > > > Understand now, the issue reported seems caused by when > > doing peel-off the peel-off socket gets the uninitialised sid > > from 'ep' on the client, though it should be "asoc". > > Hi Xin Long, > > Yes, that is my understanding. I got the impression from the thread > that there was some confusion about the different labels and what they > were used for in SELinux, I was trying to provide some background in > the text above. If you are already familiar with how things should > work you can disregard it :) > > > > In the case of SCTP, I would expect things to behave similarly: the > > > sksec->peer_sid should match the packet label of the traffic which > > > acknowledged/accepted the new connection, e.g. the other end of the > > > connected socket. You will have to forgive me some of the details, > > > it's been a while since I last looked at the SCTP bits, but I would > > > expect that if a client created a new connection and/or spun-off a new > > > socket the new socket's sksec->peer_sid would have the same property, > > > it would represent the security label of the other end of the > > > connection/association. > > > > In SCTP, a socket doesn't represent a peer connection, it's more an > > object binding some addresses and receiving incoming connecting > > request, then creates 'asoc' to represent the connection, so asoc-> > > peer_secid represents the security label of the other end of the > > connection/association. > > As mentioned previously the asoc->peer_secid *should* be the security > label of the remote end, so I think we are okay here. My concern > remains the asoc->secid label as I don't believe it is being set to > the correct value (more on that below). > > > After doing peel-off, it makes one asoc 'bind' to one new socket, > > and this socket is used for userspace to control this asoc (conection), > > so naturally we set sksec->peer_sid to asoc->secid for access control > > in socket. > > The sksec->peer_sid represents the security label of the remote end so > it should be set to the asoc->peer_secid and *not* the asoc->secid Right, sorry, it was a copy-paste error, it should've been "asoc->peer_secid". > value. Yes, they are presently the same value in your patches, but I > believe that is a mistake; I believe the asoc->secid value should be > set to that of the parent (see the prior inherit-from-parent > discussion) which in this case would likely be either the parent > association or the client process, I'm not entirely clear on which is Yes, I think that's what the current patch does in selinux_sctp_assoc_established(). > correct in the SCTP case. 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. Yes, selinux_conn_sid() is currently called in selinux_sctp_assoc_request() for the server side. > > [2] I'm guessing the client associations might also want to follow the > setsockcreatecon(3) behavior, see selinux_sockcreate_sid() for more > info. OK, I think we are on the same page now, I will post v2. Thanks! > > -- > paul moore > www.paul-moore.com ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH net 4/4] security: implement sctp_assoc_established hook in selinux 2021-10-27 4:00 ` Xin Long @ 2021-10-27 14:41 ` Paul Moore 0 siblings, 0 replies; 21+ messages in thread From: Paul Moore @ 2021-10-27 14:41 UTC (permalink / raw) To: Xin Long Cc: Ondrej Mosnacek, David S. Miller, Jakub Kicinski, James Morris, Linux Security Module list, Marcelo Ricardo Leitner, Richard Haines, SElinux list, Stephen Smalley, linux-sctp @ vger . kernel . org, network dev On Wed, Oct 27, 2021 at 12:00 AM Xin Long <lucien.xin@gmail.com> wrote: > OK, I think we are on the same page now, I will post v2. I'm not quite as confident we are on the same page just yet, but I agree that having a new revision is a good idea; if nothing else it will help reset the discussion to focus on updated patches - thanks! -- paul moore www.paul-moore.com ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH net 0/4] security: fixups for the security hooks in sctp 2021-10-22 6:36 [PATCH net 0/4] security: fixups for the security hooks in sctp Xin Long ` (3 preceding siblings ...) 2021-10-22 6:36 ` [PATCH net 4/4] security: implement sctp_assoc_established hook in selinux Xin Long @ 2021-10-24 13:42 ` Richard Haines 4 siblings, 0 replies; 21+ messages in thread From: Richard Haines @ 2021-10-24 13:42 UTC (permalink / raw) To: Xin Long, network dev, selinux, linux-security-module, linux-sctp Cc: davem, kuba, Marcelo Ricardo Leitner, James Morris, Paul Moore, Ondrej Mosnacek On Fri, 2021-10-22 at 02:36 -0400, Xin Long 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. > > 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. I've built this patchset on kernel 5.15-rc5 with no problems. I tested this using the SELinux testsuite with Ondrej's "[PATCH testsuite] tests/sctp: add client peeloff tests" [1] added. All SCTP tests ran with no errors. Also ran the sctp-tests from [2] with no errors. [1] https://lore.kernel.org/selinux/20211021144543.740762-1-omosnace@redhat.com/ [2] https://github.com/sctp/sctp-tests.git Reviewed-by: Richard Haines <richard_c_haines@btinternet.com> Tested-by: Richard Haines <richard_c_haines@btinternet.com> > > 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 | 18 +++++--- > 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 | 36 +++++++++++----- > security/selinux/include/netlabel.h | 4 +- > security/selinux/netlabel.c | 14 +++---- > 11 files changed, 135 insertions(+), 92 deletions(-) > ^ permalink raw reply [flat|nested] 21+ messages in thread
end of thread, other threads:[~2021-10-27 14:41 UTC | newest]
Thread overview: 21+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-10-22 6:36 [PATCH net 0/4] security: fixups for the security hooks in sctp Xin Long
2021-10-22 6:36 ` [PATCH net 1/4] security: pass asoc to sctp_assoc_request and sctp_sk_clone Xin Long
2021-10-22 15:35 ` Jakub Kicinski
2021-10-23 4:25 ` Xin Long
2021-10-24 13:50 ` Richard Haines
2021-10-22 6:36 ` [PATCH net 2/4] security: call security_sctp_assoc_request in sctp_sf_do_5_1D_ce Xin Long
2021-10-25 7:58 ` Ondrej Mosnacek
2021-10-22 6:36 ` [PATCH net 3/4] security: add sctp_assoc_established hook Xin Long
2021-10-24 18:45 ` kernel test robot
2021-10-25 5:01 ` kernel test robot
2021-10-25 8:01 ` Ondrej Mosnacek
2021-10-22 6:36 ` [PATCH net 4/4] security: implement sctp_assoc_established hook in selinux Xin Long
2021-10-25 8:17 ` Ondrej Mosnacek
2021-10-25 10:51 ` Xin Long
2021-10-25 12:08 ` Ondrej Mosnacek
[not found] ` <CADvbK_eE9VhB2cWzHSk_LNm_VemEt9vm=FMMVYzo5eVH=zEhKw@mail.gmail.com>
2021-10-25 21:51 ` Paul Moore
2021-10-26 4:47 ` Xin Long
2021-10-26 20:30 ` Paul Moore
2021-10-27 4:00 ` Xin Long
2021-10-27 14:41 ` Paul Moore
2021-10-24 13:42 ` [PATCH net 0/4] security: fixups for the security hooks in sctp Richard Haines
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).