From: Ondrej Mosnacek <omosnace@redhat.com>
To: Paul Moore <paul@paul-moore.com>
Cc: Xin Long <lucien.xin@gmail.com>,
network dev <netdev@vger.kernel.org>,
SElinux list <selinux@vger.kernel.org>,
Linux Security Module list
<linux-security-module@vger.kernel.org>,
"linux-sctp @ vger . kernel . org" <linux-sctp@vger.kernel.org>,
"David S. Miller" <davem@davemloft.net>,
Jakub Kicinski <kuba@kernel.org>,
Marcelo Ricardo Leitner <marcelo.leitner@gmail.com>,
James Morris <jmorris@namei.org>,
Richard Haines <richard_c_haines@btinternet.com>
Subject: Re: [PATCHv2 net 4/4] security: implement sctp_assoc_established hook in selinux
Date: Thu, 4 Nov 2021 11:40:25 +0100 [thread overview]
Message-ID: <CAFqZXNv3eRYTEJprKr0FD7i2DcZ8ZygkVqsAQY4=e484-3cPeA@mail.gmail.com> (raw)
In-Reply-To: <CAHC9VhSjPVotYVb8-ABescHmnNnDL=9B3M0J=txiDOuyJNoYuw@mail.gmail.com>
On Thu, Nov 4, 2021 at 4:17 AM Paul Moore <paul@paul-moore.com> wrote:
> On Wed, Nov 3, 2021 at 9:46 PM Xin Long <lucien.xin@gmail.com> wrote:
> > On Wed, Nov 3, 2021 at 6:01 PM Paul Moore <paul@paul-moore.com> wrote:
> > > On Wed, Nov 3, 2021 at 1:36 PM Xin Long <lucien.xin@gmail.com> wrote:
> > > > On Wed, Nov 3, 2021 at 1:33 PM Xin Long <lucien.xin@gmail.com> wrote:
> > > > > On Wed, Nov 3, 2021 at 12:40 PM Ondrej Mosnacek <omosnace@redhat.com> wrote:
> > > > > > On Tue, Nov 2, 2021 at 1:03 PM Xin Long <lucien.xin@gmail.com> wrote:
> > > > > > >
> > > > > > > Different from selinux_inet_conn_established(), it also gives the
> > > > > > > secid to asoc->peer_secid in selinux_sctp_assoc_established(),
> > > > > > > as one UDP-type socket may have more than one asocs.
> > > > > > >
> > > > > > > Note that peer_secid in asoc will save the peer secid for this
> > > > > > > asoc connection, and peer_sid in sksec will just keep the peer
> > > > > > > secid for the latest connection. So the right use should be do
> > > > > > > peeloff for UDP-type socket if there will be multiple asocs in
> > > > > > > one socket, so that the peeloff socket has the right label for
> > > > > > > its asoc.
> > > > > > >
> > > > > > > v1->v2:
> > > > > > > - call selinux_inet_conn_established() to reduce some code
> > > > > > > duplication in selinux_sctp_assoc_established(), as Ondrej
> > > > > > > suggested.
> > > > > > > - when doing peeloff, it calls sock_create() where it actually
> > > > > > > gets secid for socket from socket_sockcreate_sid(). So reuse
> > > > > > > SECSID_WILD to ensure the peeloff socket keeps using that
> > > > > > > secid after calling selinux_sctp_sk_clone() for client side.
> > > > > >
> > > > > > Interesting... I find strange that SCTP creates the peeloff socket
> > > > > > using sock_create() rather than allocating it directly via
> > > > > > sock_alloc() like the other callers of sctp_copy_sock() (which calls
> > > > > > security_sctp_sk_clone()) do. Wouldn't it make more sense to avoid the
> > > > > > sock_create() call and just rely on the security_sctp_sk_clone()
> > > > > > semantic to set up the labels? Would anything break if
> > > > > > sctp_do_peeloff() switched to plain sock_alloc()?
> > > > > >
> > > > > > I'd rather we avoid this SECSID_WILD hack to support the weird
> > > > > > created-but-also-cloned socket hybrid and just make the peeloff socket
> > > > > > behave the same as an accept()-ed socket (i.e. no
> > > > > > security_socket_[post_]create() hook calls, just
> > > > > > security_sctp_sk_clone()).
> > >
> > > I believe the important part is that sctp_do_peeloff() eventually
> > > calls security_sctp_sk_clone() via way of sctp_copy_sock(). Assuming
> > > we have security_sctp_sk_clone() working properly I would expect that
> > > the new socket would be setup properly when sctp_do_peeloff() returns
> > > on success.
> > >
> > > ... and yes, that SECSID_WILD approach is *not* something we want to do.
> >
> > SECSID_WILD is used to avoid client's new socket's sid overwritten by
> > old socket's.
>
> In the case of security_sctp_sk_clone() the new client socket (the
> cloned socket) should inherit the label/sid from the original socket
> (the "parent" in the inherit-from-parent label inheritance behavior
> discussed earlier). The selinux_sctp_assoc_established() function
> should not change the socket's label/sid at all, only the peer label.
>
> > If I understand correctly, new socket's should keep using its original
> > sid, namely,
> > the one set from security_socket_[post_]create() on client side. I
> > AGREE with that.
> > Now I want to *confirm* this with you, as it's different from the last version's
> > 'inherit from parent socket' that Richard and Ondrej reviewed.
>
> Unfortunately I think we are struggling to communicate because you are
> not familiar with SELinux concepts and I'm not as well versed in SCTP
> as you are. As things currently stand, I am getting a disconnect
> between your explanations and the code you have submitted; they simply
> aren't consistent from my perspective.
>
> In an effort to help provide something that is hopefully a bit more
> clear, here are the selinux_sctp_sk_clone() and
> selinux_sctp_assoc_established() functions which I believe we need.
> If you feel these are incorrect, please explain and/or provide edits:
>
> static void selinux_sctp_sk_clone(struct sctp_association *asoc,
> struct sock *sk, struct sock *newsk)
> {
> struct sk_security_struct *sksec = sk->sk_security;
> struct sk_security_struct *newsksec = newsk->sk_security;
>
> /* If policy does not support SECCLASS_SCTP_SOCKET then call
> * the non-sctp clone version.
> */
> if (!selinux_policycap_extsockclass())
> return selinux_sk_clone_security(sk, newsk);
>
> newsksec->secid = sksec->secid;
> newsksec->peer_sid = asoc->peer_secid;
> newsksec->sclass = sksec->sclass;
> selinux_netlbl_sctp_sk_clone(sk, newsk);
> }
>
> static void selinux_sctp_assoc_established(struct sctp_association *asoc,
> struct sk_buff *skb)
> {
> struct sk_security_struct *sksec = asoc->base.sk->sk_security;
>
> selinux_inet_conn_established(asoc->base.sk, skb);
> asoc->peer_secid = sksec->peer_sid;
> }
This code would be functionally equivalent to the v1 patchset for the
client side, but on server side you want to set newsksec->secid to
asoc->secid, as this contains the "connection secid" computed by
selinux_conn_sid() in selinux_sctp_assoc_request(). This is supposed
to mirror what selinux_inet_conn_request() -> selinux_inet_csk_clone()
does for non-SCTP sockets. So I think we should rather go back to the
v1 patchset variant, where the parent socket's sid is stashed in
asoc->secid to be picked up by selinux_sctp_sk_clone().
As for the sctp_do_peeloff-calls-sock_create problem - I was oblivious
about the difference between the sock vs. socket structs, so this
would be a bit more difficult to fix than replacing one function call.
But if we end up just overwriting the label assigned in
selinux_socket_post_create() as it is now, then the only difference is
an unexpected SCTP_SOCKET__CREATE permission check and a pointless
computation of socket_sockcreate_sid(), so it can be addressed
separately. I'll try to suggest a patch and then we can discuss
whether it makes sense or not.
--
Ondrej Mosnacek
Software Engineer, Linux Security - SELinux kernel
Red Hat, Inc.
next prev parent reply other threads:[~2021-11-04 10:40 UTC|newest]
Thread overview: 20+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-11-02 12:02 [PATCHv2 net 0/4] security: fixups for the security hooks in sctp Xin Long
2021-11-02 12:02 ` [PATCHv2 net 1/4] security: pass asoc to sctp_assoc_request and sctp_sk_clone Xin Long
2021-11-02 12:02 ` [PATCHv2 net 2/4] security: call security_sctp_assoc_request in sctp_sf_do_5_1D_ce Xin Long
2021-11-02 12:02 ` [PATCHv2 net 3/4] security: add sctp_assoc_established hook Xin Long
2021-11-02 12:02 ` [PATCHv2 net 4/4] security: implement sctp_assoc_established hook in selinux Xin Long
2021-11-03 16:40 ` Ondrej Mosnacek
2021-11-03 17:33 ` Xin Long
2021-11-03 17:36 ` Xin Long
2021-11-03 22:01 ` Paul Moore
2021-11-04 1:46 ` Xin Long
2021-11-04 3:17 ` Paul Moore
2021-11-04 10:17 ` Richard Haines
2021-11-04 10:40 ` Ondrej Mosnacek [this message]
2021-11-04 19:28 ` Paul Moore
2021-11-04 10:56 ` Xin Long
2021-11-04 11:02 ` David Miller
2021-11-04 19:10 ` Paul Moore
2021-11-04 19:49 ` Xin Long
2021-11-04 20:07 ` Paul Moore
2021-11-03 11:20 ` [PATCHv2 net 0/4] security: fixups for the security hooks in sctp patchwork-bot+netdevbpf
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to='CAFqZXNv3eRYTEJprKr0FD7i2DcZ8ZygkVqsAQY4=e484-3cPeA@mail.gmail.com' \
--to=omosnace@redhat.com \
--cc=davem@davemloft.net \
--cc=jmorris@namei.org \
--cc=kuba@kernel.org \
--cc=linux-sctp@vger.kernel.org \
--cc=linux-security-module@vger.kernel.org \
--cc=lucien.xin@gmail.com \
--cc=marcelo.leitner@gmail.com \
--cc=netdev@vger.kernel.org \
--cc=paul@paul-moore.com \
--cc=richard_c_haines@btinternet.com \
--cc=selinux@vger.kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
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).