Do this by extracting the peer labeling per-association logic from
selinux_sctp_assoc_request() into a new helper
selinux_sctp_process_new_assoc() and use this helper in both
selinux_sctp_assoc_request() and selinux_sctp_assoc_established(). This
ensures that the peer labeling behavior as documented in
Documentation/security/SCTP.rst is applied both on the client and server
side:
"""
An SCTP socket will only have one peer label assigned to it. This will be
assigned during the establishment of the first association. Any further
associations on this socket will have their packet peer label compared to
the sockets peer label, and only if they are different will the
``association`` permission be validated. This is validated by checking the
socket peer sid against the received packets peer sid to determine whether
the association should be allowed or denied.
"""
At the same time, it also ensures that the peer label of the association
is set to the correct value, such that if it is peeled off into a new
socket, the socket's peer label will then be set to the association's
peer label, same as it already works on the server side.
While selinux_inet_conn_established() (which we are replacing by
selinux_sctp_assoc_established() for SCTP) only deals with assigning a
peer label to the connection (socket), in case of SCTP we need to also
copy the (local) socket label to the association, so that
selinux_sctp_sk_clone() can then pick it up for the new socket in case
of SCTP peeloff.
Careful readers will notice that the selinux_sctp_process_new_assoc()
helper also includes the "IPv4 packet received over an IPv6 socket"
check, even though it hadn't been in selinux_sctp_assoc_request()
before. While such check is not necessary in
selinux_inet_conn_request() (because struct request_sock's family field
is already set according to the skb's family), here it is needed, as we
don't have request_sock and we take the initial family from the socket.
In selinux_sctp_assoc_established() it is similarly needed as well (and
also selinux_inet_conn_established() already has it).
Fixes: 72e89f50084c ("security: Add support for SCTP security hooks")
Reported-by: Prashanth Prahlad <[email protected]>
Based-on-patch-by: Xin Long <[email protected]>
Signed-off-by: Ondrej Mosnacek <[email protected]>
---
security/selinux/hooks.c | 90 +++++++++++++++++++++++++++++-----------
1 file changed, 66 insertions(+), 24 deletions(-)
diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
index ab32303e6618..dafabb4dcc64 100644
--- a/security/selinux/hooks.c
+++ b/security/selinux/hooks.c
@@ -5238,37 +5238,38 @@ static void selinux_sock_graft(struct sock *sk, struct socket *parent)
sksec->sclass = isec->sclass;
}
-/* Called whenever SCTP receives an INIT chunk. This happens when an incoming
- * connect(2), sctp_connectx(3) or sctp_sendmsg(3) (with no association
- * already present).
+/*
+ * Determines peer_secid for the asoc and updates socket's peer label
+ * if it's the first association on the socket.
*/
-static int selinux_sctp_assoc_request(struct sctp_association *asoc,
- struct sk_buff *skb)
+static int selinux_sctp_process_new_assoc(struct sctp_association *asoc,
+ struct sk_buff *skb)
{
- struct sk_security_struct *sksec = asoc->base.sk->sk_security;
+ struct sock *sk = asoc->base.sk;
+ u16 family = sk->sk_family;
+ struct sk_security_struct *sksec = sk->sk_security;
struct common_audit_data ad;
struct lsm_network_audit net = {0,};
- u8 peerlbl_active;
- u32 peer_sid = SECINITSID_UNLABELED;
- u32 conn_sid;
- int err = 0;
+ int err;
- if (!selinux_policycap_extsockclass())
- return 0;
+ /* handle mapped IPv4 packets arriving via IPv6 sockets */
+ if (family == PF_INET6 && skb->protocol == htons(ETH_P_IP))
+ family = PF_INET;
- peerlbl_active = selinux_peerlbl_enabled();
+ if (selinux_peerlbl_enabled()) {
+ asoc->peer_secid = SECSID_NULL;
- if (peerlbl_active) {
/* This will return peer_sid = SECSID_NULL if there are
* no peer labels, see security_net_peersid_resolve().
*/
- err = selinux_skb_peerlbl_sid(skb, asoc->base.sk->sk_family,
- &peer_sid);
+ err = selinux_skb_peerlbl_sid(skb, family, &asoc->peer_secid);
if (err)
return err;
- if (peer_sid == SECSID_NULL)
- peer_sid = SECINITSID_UNLABELED;
+ if (asoc->peer_secid == SECSID_NULL)
+ asoc->peer_secid = SECINITSID_UNLABELED;
+ } else {
+ asoc->peer_secid = SECINITSID_UNLABELED;
}
if (sksec->sctp_assoc_state == SCTP_ASSOC_UNSET) {
@@ -5279,8 +5280,8 @@ static int selinux_sctp_assoc_request(struct sctp_association *asoc,
* then it is approved by policy and used as the primary
* peer SID for getpeercon(3).
*/
- sksec->peer_sid = peer_sid;
- } else if (sksec->peer_sid != peer_sid) {
+ sksec->peer_sid = asoc->peer_secid;
+ } else if (sksec->peer_sid != asoc->peer_secid) {
/* Other association peer SIDs are checked to enforce
* consistency among the peer SIDs.
*/
@@ -5288,11 +5289,32 @@ static int selinux_sctp_assoc_request(struct sctp_association *asoc,
ad.u.net = &net;
ad.u.net->sk = asoc->base.sk;
err = avc_has_perm(&selinux_state,
- sksec->peer_sid, peer_sid, sksec->sclass,
- SCTP_SOCKET__ASSOCIATION, &ad);
+ sksec->peer_sid, asoc->peer_secid,
+ sksec->sclass, SCTP_SOCKET__ASSOCIATION,
+ &ad);
if (err)
return err;
}
+ return 0;
+}
+
+/* Called whenever SCTP receives an INIT or COOKIE ECHO chunk. This
+ * happens on an incoming connect(2), sctp_connectx(3) or
+ * sctp_sendmsg(3) (with no association already present).
+ */
+static int selinux_sctp_assoc_request(struct sctp_association *asoc,
+ struct sk_buff *skb)
+{
+ struct sk_security_struct *sksec = asoc->base.sk->sk_security;
+ u32 conn_sid;
+ int err;
+
+ if (!selinux_policycap_extsockclass())
+ return 0;
+
+ err = selinux_sctp_process_new_assoc(asoc, skb);
+ if (err)
+ return err;
/* Compute the MLS component for the connection and store
* the information in asoc. This will be used by SCTP TCP type
@@ -5300,17 +5322,36 @@ static int selinux_sctp_assoc_request(struct sctp_association *asoc,
* socket to be generated. selinux_sctp_sk_clone() will then
* plug this into the new socket.
*/
- err = selinux_conn_sid(sksec->sid, peer_sid, &conn_sid);
+ err = selinux_conn_sid(sksec->sid, asoc->peer_secid, &conn_sid);
if (err)
return err;
asoc->secid = conn_sid;
- asoc->peer_secid = peer_sid;
/* Set any NetLabel labels including CIPSO/CALIPSO options. */
return selinux_netlbl_sctp_assoc_request(asoc, skb);
}
+/* Called when SCTP receives a COOKIE ACK chunk as the final
+ * response to an association request (initited by us).
+ */
+static int selinux_sctp_assoc_established(struct sctp_association *asoc,
+ struct sk_buff *skb)
+{
+ struct sk_security_struct *sksec = asoc->base.sk->sk_security;
+
+ if (!selinux_policycap_extsockclass())
+ return 0;
+
+ /* Inherit secid from the parent socket - this will be picked up
+ * by selinux_sctp_sk_clone() if the association gets peeled off
+ * into a new socket.
+ */
+ asoc->secid = sksec->sid;
+
+ return selinux_sctp_process_new_assoc(asoc, skb);
+}
+
/* Check if sctp IPv4/IPv6 addresses are valid for binding or connecting
* based on their @optname.
*/
@@ -7131,6 +7172,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.34.1
On Sat, Feb 12, 2022 at 12:59 PM Ondrej Mosnacek <[email protected]> wrote:
>
> Do this by extracting the peer labeling per-association logic from
> selinux_sctp_assoc_request() into a new helper
> selinux_sctp_process_new_assoc() and use this helper in both
> selinux_sctp_assoc_request() and selinux_sctp_assoc_established(). This
> ensures that the peer labeling behavior as documented in
> Documentation/security/SCTP.rst is applied both on the client and server
> side:
> """
> An SCTP socket will only have one peer label assigned to it. This will be
> assigned during the establishment of the first association. Any further
> associations on this socket will have their packet peer label compared to
> the sockets peer label, and only if they are different will the
> ``association`` permission be validated. This is validated by checking the
> socket peer sid against the received packets peer sid to determine whether
> the association should be allowed or denied.
> """
>
> At the same time, it also ensures that the peer label of the association
> is set to the correct value, such that if it is peeled off into a new
> socket, the socket's peer label will then be set to the association's
> peer label, same as it already works on the server side.
>
> While selinux_inet_conn_established() (which we are replacing by
> selinux_sctp_assoc_established() for SCTP) only deals with assigning a
> peer label to the connection (socket), in case of SCTP we need to also
> copy the (local) socket label to the association, so that
> selinux_sctp_sk_clone() can then pick it up for the new socket in case
> of SCTP peeloff.
>
> Careful readers will notice that the selinux_sctp_process_new_assoc()
> helper also includes the "IPv4 packet received over an IPv6 socket"
> check, even though it hadn't been in selinux_sctp_assoc_request()
> before. While such check is not necessary in
> selinux_inet_conn_request() (because struct request_sock's family field
> is already set according to the skb's family), here it is needed, as we
> don't have request_sock and we take the initial family from the socket.
> In selinux_sctp_assoc_established() it is similarly needed as well (and
> also selinux_inet_conn_established() already has it).
>
> Fixes: 72e89f50084c ("security: Add support for SCTP security hooks")
> Reported-by: Prashanth Prahlad <[email protected]>
> Based-on-patch-by: Xin Long <[email protected]>
> Signed-off-by: Ondrej Mosnacek <[email protected]>
> ---
> security/selinux/hooks.c | 90 +++++++++++++++++++++++++++++-----------
> 1 file changed, 66 insertions(+), 24 deletions(-)
This patch, and patch 1/2, look good to me; I'm assuming this resolves
all of the known SELinux/SCTP problems identified before the new year?
If I can get an ACK from one of the SCTP and/or netdev folks I'll
merge this into the selinux/next branch.
--
paul-moore.com
On Mon, 14 Feb 2022 17:14:04 -0500 Paul Moore wrote:
> If I can get an ACK from one of the SCTP and/or netdev folks I'll
> merge this into the selinux/next branch.
No objections here FWIW, I'd defer the official acking to the SCTP
maintainers.
On Tue, Feb 15, 2022 at 11:58 AM Marcelo Ricardo Leitner
<[email protected]> wrote:
>
>
>
> Em seg., 14 de fev. de 2022 21:54, Jakub Kicinski <[email protected]> escreveu:
>>
>> On Mon, 14 Feb 2022 17:14:04 -0500 Paul Moore wrote:
>> > If I can get an ACK from one of the SCTP and/or netdev folks I'll
>> > merge this into the selinux/next branch.
>>
>> No objections here FWIW, I'd defer the official acking to the SCTP
>> maintainers.
>
>
> None from my side either, but I really want to hear from Xin. He has worked on this since day 0.
>
Looks okay to me.
The difference from the old one is that: with
selinux_sctp_process_new_assoc() called in
selinux_sctp_assoc_established(), the client sksec->peer_sid is using
the first asoc's peer_secid, instead of the latest asoc's peer_secid.
And not sure if it will cause any problems when doing the extra check
sksec->peer_sid != asoc->peer_secid for the latest asoc and *returns
err*. But I don't know about selinux, I guess there must be a reason
from selinux side.
I will ACK on patch 0/2.
Thanks Ondrej for working on this patiently.
On Mon, Feb 14, 2022 at 11:13 PM Xin Long <[email protected]> wrote:
> Looks okay to me.
>
> The difference from the old one is that: with
> selinux_sctp_process_new_assoc() called in
> selinux_sctp_assoc_established(), the client sksec->peer_sid is using
> the first asoc's peer_secid, instead of the latest asoc's peer_secid.
> And not sure if it will cause any problems when doing the extra check
> sksec->peer_sid != asoc->peer_secid for the latest asoc and *returns
> err*. But I don't know about selinux, I guess there must be a reason
> from selinux side.
Generally speaking we don't want to change any SELinux socket labels
once it has been created. While the peer_sid is a bit different,
changing it after userspace has access to the socket could be
problematic. In the case where the peer_sid differs between the two
we have a permission check which allows policy to control this
behavior which seems like the best option at this point.
> I will ACK on patch 0/2.
Thanks, I'm going to go ahead and merge these two patches into
selinux/next right now.
--
paul-moore.com
On Tue, Feb 15, 2022 at 9:03 PM Paul Moore <[email protected]> wrote:
> On Mon, Feb 14, 2022 at 11:13 PM Xin Long <[email protected]> wrote:
> > Looks okay to me.
> >
> > The difference from the old one is that: with
> > selinux_sctp_process_new_assoc() called in
> > selinux_sctp_assoc_established(), the client sksec->peer_sid is using
> > the first asoc's peer_secid, instead of the latest asoc's peer_secid.
> > And not sure if it will cause any problems when doing the extra check
> > sksec->peer_sid != asoc->peer_secid for the latest asoc and *returns
> > err*. But I don't know about selinux, I guess there must be a reason
> > from selinux side.
>
> Generally speaking we don't want to change any SELinux socket labels
> once it has been created. While the peer_sid is a bit different,
> changing it after userspace has access to the socket could be
> problematic. In the case where the peer_sid differs between the two
> we have a permission check which allows policy to control this
> behavior which seems like the best option at this point.
I think that maybe Xin was referring to the fact that on error return
from the hook the return code information is lost and the assoc is
just silently dropped (but I may have misunderstood). In case of a
denial (avc_has_perm() returning -EACCESS) this isn't much of a
problem, because the denial is logged in the audit log, so there is a
way to figure out why opening the association failed. In case of other
errors we could indeed do better and either log an SELINUX_ERR audit
event or at least pr_err() into the console, but there are likely
several other existing cases like this, so it would be best to do this
cleanup independently in another patch (if anyone feels up to the
task...).
--
Ondrej Mosnacek
Software Engineer, Linux Security - SELinux kernel
Red Hat, Inc.
On Mon, Feb 14, 2022 at 11:14 PM Paul Moore <[email protected]> wrote:
> On Sat, Feb 12, 2022 at 12:59 PM Ondrej Mosnacek <[email protected]> wrote:
> >
> > Do this by extracting the peer labeling per-association logic from
> > selinux_sctp_assoc_request() into a new helper
> > selinux_sctp_process_new_assoc() and use this helper in both
> > selinux_sctp_assoc_request() and selinux_sctp_assoc_established(). This
> > ensures that the peer labeling behavior as documented in
> > Documentation/security/SCTP.rst is applied both on the client and server
> > side:
> > """
> > An SCTP socket will only have one peer label assigned to it. This will be
> > assigned during the establishment of the first association. Any further
> > associations on this socket will have their packet peer label compared to
> > the sockets peer label, and only if they are different will the
> > ``association`` permission be validated. This is validated by checking the
> > socket peer sid against the received packets peer sid to determine whether
> > the association should be allowed or denied.
> > """
> >
> > At the same time, it also ensures that the peer label of the association
> > is set to the correct value, such that if it is peeled off into a new
> > socket, the socket's peer label will then be set to the association's
> > peer label, same as it already works on the server side.
> >
> > While selinux_inet_conn_established() (which we are replacing by
> > selinux_sctp_assoc_established() for SCTP) only deals with assigning a
> > peer label to the connection (socket), in case of SCTP we need to also
> > copy the (local) socket label to the association, so that
> > selinux_sctp_sk_clone() can then pick it up for the new socket in case
> > of SCTP peeloff.
> >
> > Careful readers will notice that the selinux_sctp_process_new_assoc()
> > helper also includes the "IPv4 packet received over an IPv6 socket"
> > check, even though it hadn't been in selinux_sctp_assoc_request()
> > before. While such check is not necessary in
> > selinux_inet_conn_request() (because struct request_sock's family field
> > is already set according to the skb's family), here it is needed, as we
> > don't have request_sock and we take the initial family from the socket.
> > In selinux_sctp_assoc_established() it is similarly needed as well (and
> > also selinux_inet_conn_established() already has it).
> >
> > Fixes: 72e89f50084c ("security: Add support for SCTP security hooks")
> > Reported-by: Prashanth Prahlad <[email protected]>
> > Based-on-patch-by: Xin Long <[email protected]>
> > Signed-off-by: Ondrej Mosnacek <[email protected]>
> > ---
> > security/selinux/hooks.c | 90 +++++++++++++++++++++++++++++-----------
> > 1 file changed, 66 insertions(+), 24 deletions(-)
>
> This patch, and patch 1/2, look good to me; I'm assuming this resolves
> all of the known SELinux/SCTP problems identified before the new year?
No, not really. There is still the inconsistency that peeloff sockets
go through the socket_[post_]create hooks and then the label computed
is overwritten by the sctp_sk_clone hook. But it's a different issue
unrelated to this one. I'm still in the process of cooking up the
patches and figuring out the consequences (other LSMs would be
affected by the change, too, so it is tricky...).
--
Ondrej Mosnacek
Software Engineer, Linux Security - SELinux kernel
Red Hat, Inc.