Re: [MPTCP] Re: [RFC PATCH] selinux: handle MPTCP consistently with TCP

Re: [MPTCP] Re: [RFC PATCH] selinux: handle MPTCP consistently with TCP

[Paul Moore]

On Thu, Dec 3, 2020 at 12:24 PM Mat Martineau
<mathew.j.martineau@linux.intel.com> wrote:
> On Wed, 2 Dec 2020, Paolo Abeni wrote:
> > On Wed, 2020-12-02 at 11:31 +0100, Paolo Abeni wrote:
> >> The MPTCP protocol uses a specific protocol value, even if
> >> it's an extension to TCP. Additionally, MPTCP sockets
> >> could 'fall-back' to TCP at run-time, depending on peer MPTCP
> >> support and available resources.
> >>
> >> As a consequence of the specific protocol number, selinux
> >> applies the raw_socket class to MPTCP sockets.
> >>
> >> Existing TCP application converted to MPTCP - or forced to
> >> use MPTCP socket with user-space hacks - will need an
> >> updated policy to run successfully.
> >>
> >> This change lets selinux attach the TCP socket class to
> >> MPTCP sockets, too, so that no policy changes are needed in
> >> the above scenario.
> >>
> >> Signed-off-by: Paolo Abeni <pabeni@redhat.com>
> >> ---
> >>  security/selinux/hooks.c | 5 +++--
> >>  1 file changed, 3 insertions(+), 2 deletions(-)
> >>
> >> diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
> >> index 6b1826fc3658..9a6b4bf1bc5b 100644
> >> --- a/security/selinux/hooks.c
> >> +++ b/security/selinux/hooks.c
> >> @@ -1120,7 +1120,8 @@ static inline u16 inode_mode_to_security_class(umode_t mode)
> >>
> >>  static inline int default_protocol_stream(int protocol)
> >>  {
> >> -    return (protocol == IPPROTO_IP || protocol == IPPROTO_TCP);
> >> +    return (protocol == IPPROTO_IP || protocol == IPPROTO_TCP ||
> >> +            protocol == IPPROTO_MPTCP);
> >>  }
>
> This looks like a good default to me.
>
> >>
> >>  static inline int default_protocol_dgram(int protocol)
> >> @@ -1152,7 +1153,7 @@ static inline u16 socket_type_to_security_class(int family, int type, int protoc
> >>                              return SECCLASS_TCP_SOCKET;
> >>                      else if (extsockclass && protocol == IPPROTO_SCTP)
> >>                              return SECCLASS_SCTP_SOCKET;
> >> -                    else
> >> +                    elseextsockclass
> >
> > Whoops, my bad! I don't know how this chunk slipped-in. I'll fix it in
> > the formal submission for inclusion, if there is agreement on this
> > change.
>
> Ok, looks fine to send after fixup.

I'm not very well versed in MPTCP, but this *seems* okay to me, minus
the else-crud chunk.  Just to confirm my understanding, while MPTCP
allows one TCP connection/stream to be subdivided and distributed
across multiple interfaces, it does not allow multiple TCP streams to
be multiplexed on a single connection, yes?

> I think there may be a small fix required to smack too, but that's an
> entirely different patch.

It would probably be a good idea to CC the LSM list just to make sure
all of the LSMs are made aware (done on this email).

-- 
paul moore
www.paul-moore.com

Re: [MPTCP] Re: [RFC PATCH] selinux: handle MPTCP consistently with TCP

[Florian Westphal]

Paul Moore <paul@paul-moore.com> wrote:
> I'm not very well versed in MPTCP, but this *seems* okay to me, minus
> the else-crud chunk.  Just to confirm my understanding, while MPTCP
> allows one TCP connection/stream to be subdivided and distributed
> across multiple interfaces, it does not allow multiple TCP streams to
> be multiplexed on a single connection, yes?

Its the latter.  The application sees a TCP interface (socket), but
data may be carried over multiple individual tcp streams on the wire.

Re: [MPTCP] Re: [RFC PATCH] selinux: handle MPTCP consistently with TCP

[Paul Moore]

On Thu, Dec 3, 2020 at 6:54 PM Florian Westphal <fw@strlen.de> wrote:
> Paul Moore <paul@paul-moore.com> wrote:
> > I'm not very well versed in MPTCP, but this *seems* okay to me, minus
> > the else-crud chunk.  Just to confirm my understanding, while MPTCP
> > allows one TCP connection/stream to be subdivided and distributed
> > across multiple interfaces, it does not allow multiple TCP streams to
> > be multiplexed on a single connection, yes?
>
> Its the latter.  The application sees a TCP interface (socket), but
> data may be carried over multiple individual tcp streams on the wire.

Hmm, that may complicate things a bit from a SELinux perspective.  Maybe not.

Just to make sure I understand, with MPTCP, a client that
traditionally opened multiple TCP sockets to talk to a server would
now just open a single MPTCP socket and create multiple sub-flows
instead of multiple TCP sockets?

-- 
paul moore
www.paul-moore.com

Re: [MPTCP] Re: [RFC PATCH] selinux: handle MPTCP consistently with TCP

[Paolo Abeni]

On Thu, 2020-12-03 at 21:24 -0500, Paul Moore wrote:
> On Thu, Dec 3, 2020 at 6:54 PM Florian Westphal <fw@strlen.de> wrote:
> > Paul Moore <paul@paul-moore.com> wrote:
> > > I'm not very well versed in MPTCP, but this *seems* okay to me, minus
> > > the else-crud chunk.  Just to confirm my understanding, while MPTCP
> > > allows one TCP connection/stream to be subdivided and distributed
> > > across multiple interfaces, it does not allow multiple TCP streams to
> > > be multiplexed on a single connection, yes?
> > 
> > Its the latter.  The application sees a TCP interface (socket), but
> > data may be carried over multiple individual tcp streams on the wire.
> 
> Hmm, that may complicate things a bit from a SELinux perspective.  Maybe not.
> 
> Just to make sure I understand, with MPTCP, a client that
> traditionally opened multiple TCP sockets to talk to a server would
> now just open a single MPTCP socket and create multiple sub-flows
> instead of multiple TCP sockets?

I expect most clients will not be updated specifically for MPTCP,
except changing the protocol number at socket creation time - and we
would like to avoid even that.

If a given application creates multiple sockets, it will still do that
with MPTCP. The kernel, according to the configuration provided by the
user-space and/or by the peer, may try to create additional subflows
for each MPTCP sockets, using different local or remote address and/or
port number. Each subflow is represented inside the kernel as a TCP
'struct sock' with specific ULP operations. No related 'struct socket'
is exposed to user-space.

Cheers,

Paolo

Re: [MPTCP] Re: [RFC PATCH] selinux: handle MPTCP consistently with TCP

[Paul Moore]

On Fri, Dec 4, 2020 at 5:04 AM Paolo Abeni <pabeni@redhat.com> wrote:
>
> On Thu, 2020-12-03 at 21:24 -0500, Paul Moore wrote:
> > On Thu, Dec 3, 2020 at 6:54 PM Florian Westphal <fw@strlen.de> wrote:
> > > Paul Moore <paul@paul-moore.com> wrote:
> > > > I'm not very well versed in MPTCP, but this *seems* okay to me, minus
> > > > the else-crud chunk.  Just to confirm my understanding, while MPTCP
> > > > allows one TCP connection/stream to be subdivided and distributed
> > > > across multiple interfaces, it does not allow multiple TCP streams to
> > > > be multiplexed on a single connection, yes?
> > >
> > > Its the latter.  The application sees a TCP interface (socket), but
> > > data may be carried over multiple individual tcp streams on the wire.
> >
> > Hmm, that may complicate things a bit from a SELinux perspective.  Maybe not.
> >
> > Just to make sure I understand, with MPTCP, a client that
> > traditionally opened multiple TCP sockets to talk to a server would
> > now just open a single MPTCP socket and create multiple sub-flows
> > instead of multiple TCP sockets?
>
> I expect most clients will not be updated specifically for MPTCP,
> except changing the protocol number at socket creation time - and we
> would like to avoid even that.
>
> If a given application creates multiple sockets, it will still do that
> with MPTCP. The kernel, according to the configuration provided by the
> user-space and/or by the peer, may try to create additional subflows
> for each MPTCP sockets, using different local or remote address and/or
> port number. Each subflow is represented inside the kernel as a TCP
> 'struct sock' with specific ULP operations. No related 'struct socket'
> is exposed to user-space.

Hmm, okay, there are probably a few other things we need to worry
about then from a SELinux point of view.  Smack may be okay since it
largely ignores sockets as a security entity, but Casey would be the
one to comment on that.  I'm not certain of the current AppArmor
network controls, or the other LSMs for that matter, but they should
be seeing this conversation on the LSM list so I assume they will
comment as necessary.

For SELinux the issue is that we need to track state in the sock
struct, via sock->sk_security, and that state needs to be initialized
and set properly.  Similarly with TCP request_sock structs, via
request_sock->{secid,peer_secid}.  Is the MPTCP code allocating and/or
otherwise creating socks or request_socks outside of the regular TCP
code?  We would also be concerned about socket structs, but I'm
guessing that code reuses the TCP code based on what you've said.

-- 
paul moore
www.paul-moore.com

Re: [MPTCP] Re: [RFC PATCH] selinux: handle MPTCP consistently with TCP

[Paolo Abeni]

Hello,

I'm sorry for the latency, I'll have limited internet access till
tomorrow.

On Fri, 2020-12-04 at 18:22 -0500, Paul Moore wrote:
> For SELinux the issue is that we need to track state in the sock
> struct, via sock->sk_security, and that state needs to be initialized
> and set properly. 

As far as I can see, for regular sockets, sk_security is allocated via:

- sk_prot_alloc() -> security_sk_alloc() for client/listener sockets
- sk_clone_lock() -> sock_copy() for server sockets

MPTCP uses the above helpers, sk_security should be initialized
properly.

MPTCP goes through an additional sk_prot_alloc() for each subflow, so
each of them will get it's own independent context. The subflows are
not exposed to any syscall (accept()/recvmsg()/sendmsg()/poll()/...),
so I guess selinux will mostly ignored them right?

The kernel will pick some of them to actually send the data, and, on
the receive side, will move the data from the subflows into the user-
space visible mptcp socket.

>  Similarly with TCP request_sock structs, via
> request_sock->{secid,peer_secid}.  Is the MPTCP code allocating and/or
> otherwise creating socks or request_socks outside of the regular TCP
> code?  

Request sockets are easier, I guess/hope: MPTCP handles them very
closely to plain TCP.

> We would also be concerned about socket structs, but I'm
> guessing that code reuses the TCP code based on what you've said.

Only the main MPTCP 'struct socket' is exposed to the user space, and
that is allocated via the usual __sys_socket() call-chain. I guess that
should be fine. If you could provide some more context (what I should
look after) I can dig more.

Thanks!

Paolo

Re: [MPTCP] Re: [RFC PATCH] selinux: handle MPTCP consistently with TCP

[Paul Moore]

On Tue, Dec 8, 2020 at 10:35 AM Paolo Abeni <pabeni@redhat.com> wrote:
>
> Hello,
>
> I'm sorry for the latency, I'll have limited internet access till
> tomorrow.
>
> On Fri, 2020-12-04 at 18:22 -0500, Paul Moore wrote:
> > For SELinux the issue is that we need to track state in the sock
> > struct, via sock->sk_security, and that state needs to be initialized
> > and set properly.
>
> As far as I can see, for regular sockets, sk_security is allocated via:
>
> - sk_prot_alloc() -> security_sk_alloc() for client/listener sockets
> - sk_clone_lock() -> sock_copy() for server sockets
>
> MPTCP uses the above helpers, sk_security should be initialized
> properly.

At least for SELinux, the security_socket_post_create() hook is
critical too as that is where the SELinux sock/socket state values are
actually set; see selinux_socket_post_create() for the SELinux hook.

> MPTCP goes through an additional sk_prot_alloc() for each subflow, so
> each of them will get it's own independent context. The subflows are
> not exposed to any syscall (accept()/recvmsg()/sendmsg()/poll()/...),
> so I guess selinux will mostly ignored them right?

SELinux cares quite a bit about the sock structs, they are an
important part of the per-packet access controls as well as a few
other things, so we need to make sure the SELinux state is managed
properly.

From what you have said so far, it is starting to sound like labeling
the subflows with the same label as the parent socket is a reasonable
solution.  In that case, it seems like doing a security_sk_clone()
between the main socket/sock and the new subflow sock should work.

> >  Similarly with TCP request_sock structs, via
> > request_sock->{secid,peer_secid}.  Is the MPTCP code allocating and/or
> > otherwise creating socks or request_socks outside of the regular TCP
> > code?
>
> Request sockets are easier, I guess/hope: MPTCP handles them very
> closely to plain TCP.

Are there a calls to security_inet_conn_request() and
security_inet_csk_clone() in the MPTCP code path?  As an example look
at tcp_conn_request() and inet_csk_clone_lock() for IPv4.

> > We would also be concerned about socket structs, but I'm
> > guessing that code reuses the TCP code based on what you've said.
>
> Only the main MPTCP 'struct socket' is exposed to the user space, and
> that is allocated via the usual __sys_socket() call-chain. I guess that
> should be fine. If you could provide some more context (what I should
> look after) I can dig more.

Hopefully the stuff above should help, if not let me know :)

-- 
paul moore
www.paul-moore.com

Re: [MPTCP] Re: [RFC PATCH] selinux: handle MPTCP consistently with TCP

[Paolo Abeni]

On Tue, 2020-12-08 at 18:35 -0500, Paul Moore wrote:
> On Tue, Dec 8, 2020 at 10:35 AM Paolo Abeni <pabeni@redhat.com> wrote:
> > Hello,
> > 
> > I'm sorry for the latency, I'll have limited internet access till
> > tomorrow.
> > 
> > On Fri, 2020-12-04 at 18:22 -0500, Paul Moore wrote:
> > > For SELinux the issue is that we need to track state in the sock
> > > struct, via sock->sk_security, and that state needs to be initialized
> > > and set properly.
> > 
> > As far as I can see, for regular sockets, sk_security is allocated via:
> > 
> > - sk_prot_alloc() -> security_sk_alloc() for client/listener sockets
> > - sk_clone_lock() -> sock_copy() for server sockets
> > 
> > MPTCP uses the above helpers, sk_security should be initialized
> > properly.
> 
> At least for SELinux, the security_socket_post_create() hook is
> critical too as that is where the SELinux sock/socket state values are
> actually set; see selinux_socket_post_create() for the SELinux hook.

MPTCP sockets are created via the conventional sys_socket() call path
or sk_clone_lock(). MPTCP subflows are created via sock_create_kern()
or csk_af_ops->syn_recv_sock().

Overall the above matches what plain TCP does: client sockets and
listener sockets will hit selinux_socket_post_create(), server sockets
will hit security_sk_clone().

> > >  Similarly with TCP request_sock structs, via
> > > request_sock->{secid,peer_secid}.  Is the MPTCP code allocating and/or
> > > otherwise creating socks or request_socks outside of the regular TCP
> > > code?
> > 
> > Request sockets are easier, I guess/hope: MPTCP handles them very
> > closely to plain TCP.
> 
> Are there a calls to security_inet_conn_request() and
> security_inet_csk_clone() in the MPTCP code path?  As an example look
> at tcp_conn_request() and inet_csk_clone_lock() for IPv4.

MPTCP subflows call both the above, via the relevant TCP call-path.
MPTCP sockets calls security_inet_conn_request() for client sockets on
connect(), but it looks like we currently lack a call
to security_inet_csk_clone() for server MPTCP sockets, as they are
created via direct call to sk_clone_lock().

I think that could be easily handled with an MPTCP patch.

> > > We would also be concerned about socket structs, but I'm
> > > guessing that code reuses the TCP code based on what you've said.
> > 
> > Only the main MPTCP 'struct socket' is exposed to the user space, and
> > that is allocated via the usual __sys_socket() call-chain. I guess that
> > should be fine. If you could provide some more context (what I should
> > look after) I can dig more.
> 
> Hopefully the stuff above should help, if not let me know :)

yes, it helped, thanks!

My understanding is that the MPTCP implementation aligns with this
proposed patch - modulo the required changed mentioned above, which
looks like a MPTCP bug.

Cheers,

Paolo

Re: [MPTCP] Re: [RFC PATCH] selinux: handle MPTCP consistently with TCP

[Paul Moore]

On Wed, Dec 9, 2020 at 5:02 AM Paolo Abeni <pabeni@redhat.com> wrote:
> On Tue, 2020-12-08 at 18:35 -0500, Paul Moore wrote:
> > On Tue, Dec 8, 2020 at 10:35 AM Paolo Abeni <pabeni@redhat.com> wrote:
> > > Hello,
> > >
> > > I'm sorry for the latency, I'll have limited internet access till
> > > tomorrow.
> > >
> > > On Fri, 2020-12-04 at 18:22 -0500, Paul Moore wrote:
> > > > For SELinux the issue is that we need to track state in the sock
> > > > struct, via sock->sk_security, and that state needs to be initialized
> > > > and set properly.
> > >
> > > As far as I can see, for regular sockets, sk_security is allocated via:
> > >
> > > - sk_prot_alloc() -> security_sk_alloc() for client/listener sockets
> > > - sk_clone_lock() -> sock_copy() for server sockets
> > >
> > > MPTCP uses the above helpers, sk_security should be initialized
> > > properly.
> >
> > At least for SELinux, the security_socket_post_create() hook is
> > critical too as that is where the SELinux sock/socket state values are
> > actually set; see selinux_socket_post_create() for the SELinux hook.
>
> MPTCP sockets are created via the conventional sys_socket() call path
> or sk_clone_lock(). MPTCP subflows are created via sock_create_kern()
> or csk_af_ops->syn_recv_sock().
>
> Overall the above matches what plain TCP does: client sockets and
> listener sockets will hit selinux_socket_post_create(), server sockets
> will hit security_sk_clone().
>
> > > >  Similarly with TCP request_sock structs, via
> > > > request_sock->{secid,peer_secid}.  Is the MPTCP code allocating and/or
> > > > otherwise creating socks or request_socks outside of the regular TCP
> > > > code?
> > >
> > > Request sockets are easier, I guess/hope: MPTCP handles them very
> > > closely to plain TCP.
> >
> > Are there a calls to security_inet_conn_request() and
> > security_inet_csk_clone() in the MPTCP code path?  As an example look
> > at tcp_conn_request() and inet_csk_clone_lock() for IPv4.
>
> MPTCP subflows call both the above, via the relevant TCP call-path.
> MPTCP sockets calls security_inet_conn_request() for client sockets on
> connect(), but it looks like we currently lack a call
> to security_inet_csk_clone() for server MPTCP sockets, as they are
> created via direct call to sk_clone_lock().
>
> I think that could be easily handled with an MPTCP patch.
>
> > > > We would also be concerned about socket structs, but I'm
> > > > guessing that code reuses the TCP code based on what you've said.
> > >
> > > Only the main MPTCP 'struct socket' is exposed to the user space, and
> > > that is allocated via the usual __sys_socket() call-chain. I guess that
> > > should be fine. If you could provide some more context (what I should
> > > look after) I can dig more.
> >
> > Hopefully the stuff above should help, if not let me know :)
>
> yes, it helped, thanks!
>
> My understanding is that the MPTCP implementation aligns with this
> proposed patch - modulo the required changed mentioned above, which
> looks like a MPTCP bug.

Great, thanks for taking the time to go through all this with me/us.
When you're ready with an updated patch(set), be sure to send it to
both the SELinux and LSM lists so we can look it over, ACK, etc.

Thanks!

-- 
paul moore
www.paul-moore.com