| Message ID | 3336b397dda1d15ee9fb87107f9cc21a5d1fe510.1606904940.git.pabeni@redhat.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [RFC] selinux: handle MPTCP consistently with TCP | expand |
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); > } > > 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. Thanks! Paolo
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 think there may be a small fix required to smack too, but that's an entirely different patch. -- Mat Martineau Intel
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 <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.
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?
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
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.
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
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 :)
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
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!
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); } 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 return SECCLASS_RAWIP_SOCKET; case SOCK_DGRAM: if (default_protocol_dgram(protocol))
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(-)