| Message ID | 20230419-upstream-lsm-next-20230419-mptcp-sublows-user-ctx-v1-0-9d4064cb0075@tessares.net (mailing list archive) |
|---|---|
| Headers | show |
| Series | security: SELinux/LSM label with MPTCP and accept | expand |
On Wed, Apr 19, 2023 at 1:44 PM Matthieu Baerts <matthieu.baerts@tessares.net> wrote: > > In [1], Ondrej Mosnacek explained they discovered the (userspace-facing) > sockets returned by accept(2) when using MPTCP always end up with the > label representing the kernel (typically system_u:system_r:kernel_t:s0), > while it would make more sense to inherit the context from the parent > socket (the one that is passed to accept(2)). Thanks to the > participation of Paul Moore in the discussions, modifications on MPTCP > side have started and the result is available here. > > Paolo Abeni worked hard to refactor the initialisation of the first > subflow of a listen socket. The first subflow allocation is no longer > done at the initialisation of the socket but later, when the connection > request is received or when requested by the userspace. This was a > prerequisite to proper support of SELinux/LSM labels with MPTCP and > accept. The last batch containing the commit ddb1a072f858 ("mptcp: move > first subflow allocation at mpc access time") [2] has been recently > accepted and applied in netdev/net-next repo [3]. > > This series of 2 patches is based on top of the lsm/next branch. Despite > the fact they depend on commits that are in netdev/net-next repo to > support the new feature, they can be applied in lsm/next without > creating conflicts with net-next or causing build issues. These two > patches on top of lsm/next still passes all the MPTCP-specific tests. > The only thing is that the new feature only works properly with the > patches that are on netdev/net-next. The tests with the new labels have > been done on top of them. > > It then looks OK to us to send these patches for review on your side. We > hope that's OK for you as well. If the patches look good to you and if > you prefer, it is fine to apply these patches before or after having > synced the lsm/next branch with Linus' tree when it will include the > modifications from the netdev/net-next repo. > > Regarding the two patches, the first one introduces a new LSM hook > called from MPTCP side when creating a new subflow socket. This hook > allows the security module to relabel the subflow according to the owing > process. The second one implements this new hook on the SELinux side. Thank you so much for working on this, I really appreciate the help! As far as potential merge issues with netdev/net-next and lsm/next, I think we'll be okay. I have a general policy[1] of not accepting new patchsets, unless critical bugfixes, past rc5/rc6 so this would be merged into lsm/next *after* the current merge window closes and presumably after the netdev/net-next branch finds its way into Linus' tree. [1] https://github.com/LinuxSecurityModule/kernel/blob/main/README.md -- paul-moore.com
Hi Paul, On 19/04/2023 23:30, Paul Moore wrote: > On Wed, Apr 19, 2023 at 1:44 PM Matthieu Baerts > <matthieu.baerts@tessares.net> wrote: >> >> In [1], Ondrej Mosnacek explained they discovered the (userspace-facing) >> sockets returned by accept(2) when using MPTCP always end up with the >> label representing the kernel (typically system_u:system_r:kernel_t:s0), >> while it would make more sense to inherit the context from the parent >> socket (the one that is passed to accept(2)). Thanks to the >> participation of Paul Moore in the discussions, modifications on MPTCP >> side have started and the result is available here. >> >> Paolo Abeni worked hard to refactor the initialisation of the first >> subflow of a listen socket. The first subflow allocation is no longer >> done at the initialisation of the socket but later, when the connection >> request is received or when requested by the userspace. This was a >> prerequisite to proper support of SELinux/LSM labels with MPTCP and >> accept. The last batch containing the commit ddb1a072f858 ("mptcp: move >> first subflow allocation at mpc access time") [2] has been recently >> accepted and applied in netdev/net-next repo [3]. >> >> This series of 2 patches is based on top of the lsm/next branch. Despite >> the fact they depend on commits that are in netdev/net-next repo to >> support the new feature, they can be applied in lsm/next without >> creating conflicts with net-next or causing build issues. These two >> patches on top of lsm/next still passes all the MPTCP-specific tests. >> The only thing is that the new feature only works properly with the >> patches that are on netdev/net-next. The tests with the new labels have >> been done on top of them. >> >> It then looks OK to us to send these patches for review on your side. We >> hope that's OK for you as well. If the patches look good to you and if >> you prefer, it is fine to apply these patches before or after having >> synced the lsm/next branch with Linus' tree when it will include the >> modifications from the netdev/net-next repo. >> >> Regarding the two patches, the first one introduces a new LSM hook >> called from MPTCP side when creating a new subflow socket. This hook >> allows the security module to relabel the subflow according to the owing >> process. The second one implements this new hook on the SELinux side. > > Thank you so much for working on this, I really appreciate the help! Thank you for the review! We are working on a v2 addressing your comments. Just one small detail regarding these comments: I hope you don't mind if we use "MPTCP socket" instead of "main MPTCP socket". Per connection, there is one MPTCP socket and possibly multiple subflow (TCP) sockets. There is then no concept of "main MPTCP socket". > As far as potential merge issues with netdev/net-next and lsm/next, I > think we'll be okay. I have a general policy[1] of not accepting new > patchsets, unless critical bugfixes, past rc5/rc6 so this would be > merged into lsm/next *after* the current merge window closes and > presumably after the netdev/net-next branch finds its way into Linus' > tree. It makes sense, we understand. These two patches were ready for a bit of time but we wanted to send them only after the prerequisite commits applied in net-next first. But that got delayed because we had a couple of nasty issues with them :) We hope it will not be an issue for you to maintain them in your tree for a couple of months but we tried to minimised the modifications in MPTCP code. Do not hesitate to reach us if there are some issues with them! Cheers, Matt
On Thu, Apr 20, 2023 at 12:55 PM Matthieu Baerts <matthieu.baerts@tessares.net> wrote: > On 19/04/2023 23:30, Paul Moore wrote: > > On Wed, Apr 19, 2023 at 1:44 PM Matthieu Baerts > > <matthieu.baerts@tessares.net> wrote: > >> > >> In [1], Ondrej Mosnacek explained they discovered the (userspace-facing) > >> sockets returned by accept(2) when using MPTCP always end up with the > >> label representing the kernel (typically system_u:system_r:kernel_t:s0), > >> while it would make more sense to inherit the context from the parent > >> socket (the one that is passed to accept(2)). Thanks to the > >> participation of Paul Moore in the discussions, modifications on MPTCP > >> side have started and the result is available here. > >> > >> Paolo Abeni worked hard to refactor the initialisation of the first > >> subflow of a listen socket. The first subflow allocation is no longer > >> done at the initialisation of the socket but later, when the connection > >> request is received or when requested by the userspace. This was a > >> prerequisite to proper support of SELinux/LSM labels with MPTCP and > >> accept. The last batch containing the commit ddb1a072f858 ("mptcp: move > >> first subflow allocation at mpc access time") [2] has been recently > >> accepted and applied in netdev/net-next repo [3]. > >> > >> This series of 2 patches is based on top of the lsm/next branch. Despite > >> the fact they depend on commits that are in netdev/net-next repo to > >> support the new feature, they can be applied in lsm/next without > >> creating conflicts with net-next or causing build issues. These two > >> patches on top of lsm/next still passes all the MPTCP-specific tests. > >> The only thing is that the new feature only works properly with the > >> patches that are on netdev/net-next. The tests with the new labels have > >> been done on top of them. > >> > >> It then looks OK to us to send these patches for review on your side. We > >> hope that's OK for you as well. If the patches look good to you and if > >> you prefer, it is fine to apply these patches before or after having > >> synced the lsm/next branch with Linus' tree when it will include the > >> modifications from the netdev/net-next repo. > >> > >> Regarding the two patches, the first one introduces a new LSM hook > >> called from MPTCP side when creating a new subflow socket. This hook > >> allows the security module to relabel the subflow according to the owing > >> process. The second one implements this new hook on the SELinux side. > > > > Thank you so much for working on this, I really appreciate the help! > > Thank you for the review! > > We are working on a v2 addressing your comments. Thanks for getting v2 out so quickly. I'm getting caught up on other issue while we're in the merge window right now, but I'll give the v2 patchset a look in the not-to-distant future. I'm fairly confident we'll get it merged this dev cycle. > Just one small detail regarding these comments: I hope you don't mind if > we use "MPTCP socket" instead of "main MPTCP socket". Per connection, > there is one MPTCP socket and possibly multiple subflow (TCP) sockets. > There is then no concept of "main MPTCP socket". Sure, no problem. > > As far as potential merge issues with netdev/net-next and lsm/next, I > > think we'll be okay. I have a general policy[1] of not accepting new > > patchsets, unless critical bugfixes, past rc5/rc6 so this would be > > merged into lsm/next *after* the current merge window closes and > > presumably after the netdev/net-next branch finds its way into Linus' > > tree. > > It makes sense, we understand. These two patches were ready for a bit of > time but we wanted to send them only after the prerequisite commits > applied in net-next first. But that got delayed because we had a couple > of nasty issues with them :) > > We hope it will not be an issue for you to maintain them in your tree > for a couple of months but we tried to minimised the modifications in > MPTCP code. Do not hesitate to reach us if there are some issues with them! No worries, I'm happy to maintain them in either the LSM or the SELinux tree (I'll need to remind myself of the changes to see which is the best fit). -- paul-moore.com
In [1], Ondrej Mosnacek explained they discovered the (userspace-facing) sockets returned by accept(2) when using MPTCP always end up with the label representing the kernel (typically system_u:system_r:kernel_t:s0), while it would make more sense to inherit the context from the parent socket (the one that is passed to accept(2)). Thanks to the participation of Paul Moore in the discussions, modifications on MPTCP side have started and the result is available here. Paolo Abeni worked hard to refactor the initialisation of the first subflow of a listen socket. The first subflow allocation is no longer done at the initialisation of the socket but later, when the connection request is received or when requested by the userspace. This was a prerequisite to proper support of SELinux/LSM labels with MPTCP and accept. The last batch containing the commit ddb1a072f858 ("mptcp: move first subflow allocation at mpc access time") [2] has been recently accepted and applied in netdev/net-next repo [3]. This series of 2 patches is based on top of the lsm/next branch. Despite the fact they depend on commits that are in netdev/net-next repo to support the new feature, they can be applied in lsm/next without creating conflicts with net-next or causing build issues. These two patches on top of lsm/next still passes all the MPTCP-specific tests. The only thing is that the new feature only works properly with the patches that are on netdev/net-next. The tests with the new labels have been done on top of them. It then looks OK to us to send these patches for review on your side. We hope that's OK for you as well. If the patches look good to you and if you prefer, it is fine to apply these patches before or after having synced the lsm/next branch with Linus' tree when it will include the modifications from the netdev/net-next repo. Regarding the two patches, the first one introduces a new LSM hook called from MPTCP side when creating a new subflow socket. This hook allows the security module to relabel the subflow according to the owing process. The second one implements this new hook on the SELinux side. Link: https://lore.kernel.org/netdev/CAFqZXNs2LF-OoQBUiiSEyranJUXkPLcCfBkMkwFeM6qEwMKCTw@mail.gmail.com/ [1] Link: https://git.kernel.org/netdev/net-next/c/ddb1a072f858 [2] Link: https://lore.kernel.org/netdev/20230414-upstream-net-next-20230414-mptcp-refactor-first-subflow-init-v1-0-04d177057eb9@tessares.net/ [3] Signed-off-by: Matthieu Baerts <matthieu.baerts@tessares.net> --- Paolo Abeni (2): security, lsm: Introduce security_mptcp_add_subflow() selinux: Implement mptcp_add_subflow hook include/linux/lsm_hook_defs.h | 1 + include/linux/security.h | 6 ++++++ net/mptcp/subflow.c | 6 ++++++ security/security.c | 15 +++++++++++++++ security/selinux/hooks.c | 16 ++++++++++++++++ security/selinux/netlabel.c | 8 ++++++-- 6 files changed, 50 insertions(+), 2 deletions(-) --- base-commit: d82dcd9e21b77d338dc4875f3d4111f0db314a7c change-id: 20230419-upstream-lsm-next-20230419-mptcp-sublows-user-ctx-eee658fafcba Best regards,