[PATCH LSM v2 0/2] security: SELinux/LSM label with MPTCP and accept

[PATCH LSM v2 0/2] security: SELinux/LSM label with MPTCP and accept

[Matthieu Baerts]

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.

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>
---
Changes in v2:
- Address Paul's comments, see the notes on each patch
- Link to v1: https://lore.kernel.org/r/20230419-upstream-lsm-next-20230419-mptcp-sublows-user-ctx-v1-0-9d4064cb0075@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           | 17 +++++++++++++++++
 security/selinux/hooks.c      | 16 ++++++++++++++++
 security/selinux/netlabel.c   |  8 ++++++--
 6 files changed, 52 insertions(+), 2 deletions(-)
---
base-commit: d82dcd9e21b77d338dc4875f3d4111f0db314a7c
change-id: 20230419-upstream-lsm-next-20230419-mptcp-sublows-user-ctx-eee658fafcba

Best regards,
-- 
Matthieu Baerts <matthieu.baerts@tessares.net>

[PATCH LSM v2 1/2] security, lsm: Introduce security_mptcp_add_subflow()

[Matthieu Baerts]

From: Paolo Abeni <pabeni@redhat.com>

MPTCP can create subflows in kernel context, and later indirectly
expose them to user-space, via the owning MPTCP socket.

As discussed in the reported link, the above causes unexpected failures
for server, MPTCP-enabled applications.

Let's introduce a new LSM hook to allow the security module to relabel
the subflow according to the owning user-space process, via the MPTCP
socket owning the subflow.

Note that the new hook requires both the MPTCP socket and the new
subflow. This could allow future extensions, e.g. explicitly validating
the MPTCP <-> subflow linkage.

Link: https://lore.kernel.org/mptcp/CAHC9VhTNh-YwiyTds=P1e3rixEDqbRTFj22bpya=+qJqfcaMfg@mail.gmail.com/
Signed-off-by: Paolo Abeni <pabeni@redhat.com>
Acked-by: Matthieu Baerts <matthieu.baerts@tessares.net>
Signed-off-by: Matthieu Baerts <matthieu.baerts@tessares.net>
---
v2:
 - Address Paul's comments:
   - clarification around "the owning process" in the commit message
   - making it clear the hook has to be called after the sk init part
   - consistent capitalization of "MPTCP"
---
 include/linux/lsm_hook_defs.h |  1 +
 include/linux/security.h      |  6 ++++++
 net/mptcp/subflow.c           |  6 ++++++
 security/security.c           | 17 +++++++++++++++++
 4 files changed, 30 insertions(+)

diff --git a/include/linux/lsm_hook_defs.h b/include/linux/lsm_hook_defs.h
index 6bb55e61e8e8..7308a1a7599b 100644
--- a/include/linux/lsm_hook_defs.h
+++ b/include/linux/lsm_hook_defs.h
@@ -343,6 +343,7 @@ LSM_HOOK(void, LSM_RET_VOID, sctp_sk_clone, struct sctp_association *asoc,
 	 struct sock *sk, struct sock *newsk)
 LSM_HOOK(int, 0, sctp_assoc_established, struct sctp_association *asoc,
 	 struct sk_buff *skb)
+LSM_HOOK(int, 0, mptcp_add_subflow, struct sock *sk, struct sock *ssk)
 #endif /* CONFIG_SECURITY_NETWORK */
 
 #ifdef CONFIG_SECURITY_INFINIBAND
diff --git a/include/linux/security.h b/include/linux/security.h
index cd23221ce9e6..80a0b37a9f26 100644
--- a/include/linux/security.h
+++ b/include/linux/security.h
@@ -1465,6 +1465,7 @@ void security_sctp_sk_clone(struct sctp_association *asoc, struct sock *sk,
 			    struct sock *newsk);
 int security_sctp_assoc_established(struct sctp_association *asoc,
 				    struct sk_buff *skb);
+int security_mptcp_add_subflow(struct sock *sk, struct sock *ssk);
 
 #else	/* CONFIG_SECURITY_NETWORK */
 static inline int security_unix_stream_connect(struct sock *sock,
@@ -1692,6 +1693,11 @@ static inline int security_sctp_assoc_established(struct sctp_association *asoc,
 {
 	return 0;
 }
+
+static inline int security_mptcp_add_subflow(struct sock *sk, struct sock *ssk)
+{
+	return 0;
+}
 #endif	/* CONFIG_SECURITY_NETWORK */
 
 #ifdef CONFIG_SECURITY_INFINIBAND
diff --git a/net/mptcp/subflow.c b/net/mptcp/subflow.c
index 4ae1a7304cf0..d361749cabff 100644
--- a/net/mptcp/subflow.c
+++ b/net/mptcp/subflow.c
@@ -1692,6 +1692,10 @@ int mptcp_subflow_create_socket(struct sock *sk, unsigned short family,
 
 	lock_sock_nested(sf->sk, SINGLE_DEPTH_NESTING);
 
+	err = security_mptcp_add_subflow(sk, sf->sk);
+	if (err)
+		goto release_ssk;
+
 	/* the newly created socket has to be in the same cgroup as its parent */
 	mptcp_attach_cgroup(sk, sf->sk);
 
@@ -1704,6 +1708,8 @@ int mptcp_subflow_create_socket(struct sock *sk, unsigned short family,
 	get_net_track(net, &sf->sk->ns_tracker, GFP_KERNEL);
 	sock_inuse_add(net, 1);
 	err = tcp_set_ulp(sf->sk, "mptcp");
+
+release_ssk:
 	release_sock(sf->sk);
 
 	if (err) {
diff --git a/security/security.c b/security/security.c
index f4170efcddda..a12e44925942 100644
--- a/security/security.c
+++ b/security/security.c
@@ -4667,6 +4667,23 @@ int security_sctp_assoc_established(struct sctp_association *asoc,
 }
 EXPORT_SYMBOL(security_sctp_assoc_established);
 
+/**
+ * security_mptcp_add_subflow() - Inherit the LSM label from the MPTCP socket
+ * @sk: the owning MPTCP socket
+ * @ssk: the new subflow
+ *
+ * Update the labeling for the given MPTCP subflow, to match the one of the
+ * owning MPTCP socket. This hook has to be called after the socket creation and
+ * initialization via the security_socket_create() and
+ * security_socket_post_create() LSM hooks.
+ *
+ * Return: Returns 0 on success or a negative error code on failure.
+ */
+int security_mptcp_add_subflow(struct sock *sk, struct sock *ssk)
+{
+	return call_int_hook(mptcp_add_subflow, 0, sk, ssk);
+}
+
 #endif	/* CONFIG_SECURITY_NETWORK */
 
 #ifdef CONFIG_SECURITY_INFINIBAND

-- 
2.39.2

Re: [PATCH v2 1/2] security, lsm: Introduce security_mptcp_add_subflow()

[Paul Moore]

On Apr 20, 2023 Matthieu Baerts <matthieu.baerts@tessares.net> wrote:
> 
> MPTCP can create subflows in kernel context, and later indirectly
> expose them to user-space, via the owning MPTCP socket.
> 
> As discussed in the reported link, the above causes unexpected failures
> for server, MPTCP-enabled applications.
> 
> Let's introduce a new LSM hook to allow the security module to relabel
> the subflow according to the owning user-space process, via the MPTCP
> socket owning the subflow.
> 
> Note that the new hook requires both the MPTCP socket and the new
> subflow. This could allow future extensions, e.g. explicitly validating
> the MPTCP <-> subflow linkage.
> 
> Link: https://lore.kernel.org/mptcp/CAHC9VhTNh-YwiyTds=P1e3rixEDqbRTFj22bpya=+qJqfcaMfg@mail.gmail.com/
> Signed-off-by: Paolo Abeni <pabeni@redhat.com>
> Acked-by: Matthieu Baerts <matthieu.baerts@tessares.net>
> Signed-off-by: Matthieu Baerts <matthieu.baerts@tessares.net>
> ---
> v2:
>  - Address Paul's comments:
>    - clarification around "the owning process" in the commit message
>    - making it clear the hook has to be called after the sk init part
>    - consistent capitalization of "MPTCP"
> ---
>  include/linux/lsm_hook_defs.h |  1 +
>  include/linux/security.h      |  6 ++++++
>  net/mptcp/subflow.c           |  6 ++++++
>  security/security.c           | 17 +++++++++++++++++
>  4 files changed, 30 insertions(+)

This looks good to me, merged into selinux/next - thank you for all
the work that went into this!

--
paul-moore.com

[PATCH LSM v2 2/2] selinux: Implement mptcp_add_subflow hook

[Matthieu Baerts]

From: Paolo Abeni <pabeni@redhat.com>

Newly added subflows should inherit the LSM label from the associated
MPTCP socket regardless of the current context.

This patch implements the above copying sid and class from the MPTCP
socket context, deleting the existing subflow label, if any, and then
re-creating the correct one.

The new helper reuses the selinux_netlbl_sk_security_free() function,
and the latter can end-up being called multiple times with the same
argument; we additionally need to make it idempotent.

Signed-off-by: Paolo Abeni <pabeni@redhat.com>
Acked-by: Matthieu Baerts <matthieu.baerts@tessares.net>
Signed-off-by: Matthieu Baerts <matthieu.baerts@tessares.net>
---
v2:
 - Address Paul's comments:
   - use "MPTCP socket" instead of "msk" in the commit message
   - "updated" context instead of "current" one in the comment
---
 security/selinux/hooks.c    | 16 ++++++++++++++++
 security/selinux/netlabel.c |  8 ++++++--
 2 files changed, 22 insertions(+), 2 deletions(-)

diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
index 9a5bdfc21314..67e6cd18ad59 100644
--- a/security/selinux/hooks.c
+++ b/security/selinux/hooks.c
@@ -5476,6 +5476,21 @@ static void selinux_sctp_sk_clone(struct sctp_association *asoc, struct sock *sk
 	selinux_netlbl_sctp_sk_clone(sk, newsk);
 }
 
+static int selinux_mptcp_add_subflow(struct sock *sk, struct sock *ssk)
+{
+	struct sk_security_struct *ssksec = ssk->sk_security;
+	struct sk_security_struct *sksec = sk->sk_security;
+
+	ssksec->sclass = sksec->sclass;
+	ssksec->sid = sksec->sid;
+
+	/* replace the existing subflow label deleting the existing one
+	 * and re-recreating a new label using the updated context
+	 */
+	selinux_netlbl_sk_security_free(ssksec);
+	return selinux_netlbl_socket_post_create(ssk, ssk->sk_family);
+}
+
 static int selinux_inet_conn_request(const struct sock *sk, struct sk_buff *skb,
 				     struct request_sock *req)
 {
@@ -7216,6 +7231,7 @@ static struct security_hook_list selinux_hooks[] __lsm_ro_after_init = {
 	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(mptcp_add_subflow, selinux_mptcp_add_subflow),
 	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),
diff --git a/security/selinux/netlabel.c b/security/selinux/netlabel.c
index 1321f15799e2..33187e38def7 100644
--- a/security/selinux/netlabel.c
+++ b/security/selinux/netlabel.c
@@ -155,8 +155,12 @@ void selinux_netlbl_err(struct sk_buff *skb, u16 family, int error, int gateway)
  */
 void selinux_netlbl_sk_security_free(struct sk_security_struct *sksec)
 {
-	if (sksec->nlbl_secattr != NULL)
-		netlbl_secattr_free(sksec->nlbl_secattr);
+	if (!sksec->nlbl_secattr)
+		return;
+
+	netlbl_secattr_free(sksec->nlbl_secattr);
+	sksec->nlbl_secattr = NULL;
+	sksec->nlbl_state = NLBL_UNSET;
 }
 
 /**

-- 
2.39.2

Re: [PATCH v2 2/2] selinux: Implement mptcp_add_subflow hook

[Paul Moore]

On Apr 20, 2023 Matthieu Baerts <matthieu.baerts@tessares.net> wrote:
> 
> Newly added subflows should inherit the LSM label from the associated
> MPTCP socket regardless of the current context.
> 
> This patch implements the above copying sid and class from the MPTCP
> socket context, deleting the existing subflow label, if any, and then
> re-creating the correct one.
> 
> The new helper reuses the selinux_netlbl_sk_security_free() function,
> and the latter can end-up being called multiple times with the same
> argument; we additionally need to make it idempotent.
> 
> Signed-off-by: Paolo Abeni <pabeni@redhat.com>
> Acked-by: Matthieu Baerts <matthieu.baerts@tessares.net>
> Signed-off-by: Matthieu Baerts <matthieu.baerts@tessares.net>
> ---
> v2:
>  - Address Paul's comments:
>    - use "MPTCP socket" instead of "msk" in the commit message
>    - "updated" context instead of "current" one in the comment
> ---
>  security/selinux/hooks.c    | 16 ++++++++++++++++
>  security/selinux/netlabel.c |  8 ++++++--
>  2 files changed, 22 insertions(+), 2 deletions(-)

Also merged into selinux/next, thanks again.

--
paul-moore.com

Re: [PATCH LSM v2 0/2] security: SELinux/LSM label with MPTCP and accept

[Ondrej Mosnacek]

On Thu, Apr 20, 2023 at 7:17 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.
>
> 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>
> ---
> Changes in v2:
> - Address Paul's comments, see the notes on each patch
> - Link to v1: https://lore.kernel.org/r/20230419-upstream-lsm-next-20230419-mptcp-sublows-user-ctx-v1-0-9d4064cb0075@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           | 17 +++++++++++++++++
>  security/selinux/hooks.c      | 16 ++++++++++++++++
>  security/selinux/netlabel.c   |  8 ++++++--
>  6 files changed, 52 insertions(+), 2 deletions(-)
> ---
> base-commit: d82dcd9e21b77d338dc4875f3d4111f0db314a7c
> change-id: 20230419-upstream-lsm-next-20230419-mptcp-sublows-user-ctx-eee658fafcba
>
> Best regards,
> --
> Matthieu Baerts <matthieu.baerts@tessares.net>
>

I haven't yet looked closer at the code in this series, but I can at
least confirm that with the series (applied on top of net-next) the
selinux-testsuite now passes when run under mptcpize, with one caveat:

The "client" test prog in the inet_socket subtest sets the SO_SNDTIMEO
socket option on the client socket, but the subtest takes
significantly longer to complete than when run without mptcpize. That
suggests to me that there is possibly some (pre-existing) issue with
MPTCP where the send/receive timeouts are not being passed to the
subflow socket(s), leading to a longer wait (I guess the default is
higher?) when the timeout is expected to be hit (there are test cases
where we expect packets to be dropped due to SELinux access controls,
which we detect via timeout). I'm only taking a wild guess at the root
cause here, but I hope you guys will be able to figure it out and fix
whatever needs fixing :)

--
Ondrej Mosnacek
Senior Software Engineer, Linux Security - SELinux kernel
Red Hat, Inc.

Re: [PATCH LSM v2 0/2] security: SELinux/LSM label with MPTCP and accept

[Paolo Abeni]

On Thu, 2023-05-04 at 16:14 +0200, Ondrej Mosnacek wrote:
> On Thu, Apr 20, 2023 at 7:17 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.
> > 
> > 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>
> > ---
> > Changes in v2:
> > - Address Paul's comments, see the notes on each patch
> > - Link to v1: https://lore.kernel.org/r/20230419-upstream-lsm-next-20230419-mptcp-sublows-user-ctx-v1-0-9d4064cb0075@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           | 17 +++++++++++++++++
> >  security/selinux/hooks.c      | 16 ++++++++++++++++
> >  security/selinux/netlabel.c   |  8 ++++++--
> >  6 files changed, 52 insertions(+), 2 deletions(-)
> > ---
> > base-commit: d82dcd9e21b77d338dc4875f3d4111f0db314a7c
> > change-id: 20230419-upstream-lsm-next-20230419-mptcp-sublows-user-ctx-eee658fafcba
> > 
> > Best regards,
> > --
> > Matthieu Baerts <matthieu.baerts@tessares.net>
> > 
> 
> I haven't yet looked closer at the code in this series, but I can at
> least confirm that with the series (applied on top of net-next) the
> selinux-testsuite now passes when run under mptcpize, with one caveat:
> 
> The "client" test prog in the inet_socket subtest sets the SO_SNDTIMEO
> socket option on the client socket, but the subtest takes
> significantly longer to complete than when run without mptcpize. That
> suggests to me that there is possibly some (pre-existing) issue with
> MPTCP where the send/receive timeouts are not being passed to the
> subflow socket(s), leading to a longer wait (I guess the default is
> higher?) 

Indeed the behavior you describe is due to some mptcp bug in handling
the SO_{SND,RCV}TIMEO socket tions, and it's really unrelated to the
initially reported selinux issue.

If you could file an issue on our tracker, that would help ;)

Thanks!

Paolo

Re: [PATCH LSM v2 0/2] security: SELinux/LSM label with MPTCP and accept

[Ondrej Mosnacek]

On Thu, May 4, 2023 at 6:13 PM Paolo Abeni <pabeni@redhat.com> wrote:
> On Thu, 2023-05-04 at 16:14 +0200, Ondrej Mosnacek wrote:
> > On Thu, Apr 20, 2023 at 7:17 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.
> > >
> > > 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>
> > > ---
> > > Changes in v2:
> > > - Address Paul's comments, see the notes on each patch
> > > - Link to v1: https://lore.kernel.org/r/20230419-upstream-lsm-next-20230419-mptcp-sublows-user-ctx-v1-0-9d4064cb0075@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           | 17 +++++++++++++++++
> > >  security/selinux/hooks.c      | 16 ++++++++++++++++
> > >  security/selinux/netlabel.c   |  8 ++++++--
> > >  6 files changed, 52 insertions(+), 2 deletions(-)
> > > ---
> > > base-commit: d82dcd9e21b77d338dc4875f3d4111f0db314a7c
> > > change-id: 20230419-upstream-lsm-next-20230419-mptcp-sublows-user-ctx-eee658fafcba
> > >
> > > Best regards,
> > > --
> > > Matthieu Baerts <matthieu.baerts@tessares.net>
> > >
> >
> > I haven't yet looked closer at the code in this series, but I can at
> > least confirm that with the series (applied on top of net-next) the
> > selinux-testsuite now passes when run under mptcpize, with one caveat:
> >
> > The "client" test prog in the inet_socket subtest sets the SO_SNDTIMEO
> > socket option on the client socket, but the subtest takes
> > significantly longer to complete than when run without mptcpize. That
> > suggests to me that there is possibly some (pre-existing) issue with
> > MPTCP where the send/receive timeouts are not being passed to the
> > subflow socket(s), leading to a longer wait (I guess the default is
> > higher?)
>
> Indeed the behavior you describe is due to some mptcp bug in handling
> the SO_{SND,RCV}TIMEO socket tions, and it's really unrelated to the
> initially reported selinux issue.

Definitely unrelated, just wanted to report the bug :)

> If you could file an issue on our tracker, that would help ;)

I was about to ask where that tracker is, but then it occured to me to
check MAINTAINERS and the link is right there, so yes, will do :)


--
Ondrej Mosnacek
Senior Software Engineer, Linux Security - SELinux kernel
Red Hat, Inc.

[PATCH v2 0/2] lsm: introduce and use security_mptcp_add_subflow()

[Paolo Abeni]

This series is an attempt to solve the LSM labeling breakage
reported here:

https://lore.kernel.org/linux-security-module/CAHC9VhSQnhH3UL4gqzu+YiA1Q3YyLLCv88gLJOvw-0+uw5Lvkw@mail.gmail.com/

As per previous discussion, a new LSM hook is introduced and
invoked by the mptcp code to let LSMs set the appropriate label
for the newly created subflow.

I'm not sure the chosen hook name is a perfect fit, any suggestion
more then welcome.
The new hook requires both the mptcp socket reference and the
subflow socket reference, even if the provided LSM implementation
for selinux ends-up accessing only the subflow socket. Possibly
other LSM implementation could need or use the addtional parameter.

Tested vs the issue reproducer and mptcp self-tests.

v1 -> v2:
 - fix a few build issues with unusual configurations reported
   by bots

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/lsm_hooks.h     |  9 +++++++++
 include/linux/security.h      |  6 ++++++
 net/mptcp/subflow.c           |  6 ++++++
 security/security.c           |  5 +++++
 security/selinux/hooks.c      | 27 +++++++++++++++++++++++++++
 security/selinux/netlabel.c   |  4 +++-
 7 files changed, 57 insertions(+), 1 deletion(-)

-- 
2.38.1

[PATCH v2 2/2] selinux: Implement mptcp_add_subflow hook

[Paolo Abeni]

Newly added subflows should inherit the associated label
from the current process context, regarless of the sk_kern_sock
flag value.

This patch implements the above resetting the subflow sid, deleting
the existing subflow label, if any, and then re-creating a new one.

The new helper reuses the selinux_netlbl_sk_security_free() function,
and it can end-up being called multiple times with the same argument;
we additionally need to make it idempotent.

Signed-off-by: Paolo Abeni <pabeni@redhat.com>
---
v1 -> v2:
 - fix build issue with !CONFIG_NETLABEL
---
 security/selinux/hooks.c    | 27 +++++++++++++++++++++++++++
 security/selinux/netlabel.c |  4 +++-
 2 files changed, 30 insertions(+), 1 deletion(-)

diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
index 3c5be76a9199..f785600b666a 100644
--- a/security/selinux/hooks.c
+++ b/security/selinux/hooks.c
@@ -5476,6 +5476,32 @@ static void selinux_sctp_sk_clone(struct sctp_association *asoc, struct sock *sk
 	selinux_netlbl_sctp_sk_clone(sk, newsk);
 }
 
+static int selinux_mptcp_add_subflow(struct sock *sk, struct sock *ssk)
+{
+	const struct task_security_struct *tsec = selinux_cred(current_cred());
+	struct sk_security_struct *ssksec = ssk->sk_security;
+	u16 sclass;
+	u32 sid;
+	int err;
+
+	/* create the sid using the current cred, regardless of the ssk kern
+	 * flag
+	 */
+	sclass = socket_type_to_security_class(ssk->sk_family, ssk->sk_type,
+					       ssk->sk_protocol);
+	err = socket_sockcreate_sid(tsec, sclass, &sid);
+	if (err)
+		return err;
+
+	ssksec->sid = sid;
+
+	/* replace the existing subflow label deleting the existing one
+	 * and re-recrating a new label using the current context
+	 */
+	selinux_netlbl_sk_security_free(ssksec);
+	return selinux_netlbl_socket_post_create(ssk, ssk->sk_family);
+}
+
 static int selinux_inet_conn_request(const struct sock *sk, struct sk_buff *skb,
 				     struct request_sock *req)
 {
@@ -7216,6 +7242,7 @@ static struct security_hook_list selinux_hooks[] __lsm_ro_after_init = {
 	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(mptcp_add_subflow, selinux_mptcp_add_subflow),
 	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),
diff --git a/security/selinux/netlabel.c b/security/selinux/netlabel.c
index 1321f15799e2..8e0080b8a8ef 100644
--- a/security/selinux/netlabel.c
+++ b/security/selinux/netlabel.c
@@ -155,8 +155,10 @@ void selinux_netlbl_err(struct sk_buff *skb, u16 family, int error, int gateway)
  */
 void selinux_netlbl_sk_security_free(struct sk_security_struct *sksec)
 {
-	if (sksec->nlbl_secattr != NULL)
+	if (sksec->nlbl_secattr != NULL) {
 		netlbl_secattr_free(sksec->nlbl_secattr);
+		sksec->nlbl_secattr = NULL;
+	}
 }
 
 /**
-- 
2.38.1

Re: [PATCH v2 2/2] selinux: Implement mptcp_add_subflow hook

[Paul Moore]

On Mon, Dec 19, 2022 at 12:34 PM Paolo Abeni <pabeni@redhat.com> wrote:
>
> Newly added subflows should inherit the associated label
> from the current process context, regarless of the sk_kern_sock
> flag value.
>
> This patch implements the above resetting the subflow sid, deleting
> the existing subflow label, if any, and then re-creating a new one.
>
> The new helper reuses the selinux_netlbl_sk_security_free() function,
> and it can end-up being called multiple times with the same argument;
> we additionally need to make it idempotent.
>
> Signed-off-by: Paolo Abeni <pabeni@redhat.com>
> ---
> v1 -> v2:
>  - fix build issue with !CONFIG_NETLABEL
> ---
>  security/selinux/hooks.c    | 27 +++++++++++++++++++++++++++
>  security/selinux/netlabel.c |  4 +++-
>  2 files changed, 30 insertions(+), 1 deletion(-)
>
> diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
> index 3c5be76a9199..f785600b666a 100644
> --- a/security/selinux/hooks.c
> +++ b/security/selinux/hooks.c
> @@ -5476,6 +5476,32 @@ static void selinux_sctp_sk_clone(struct sctp_association *asoc, struct sock *sk
>         selinux_netlbl_sctp_sk_clone(sk, newsk);
>  }
>
> +static int selinux_mptcp_add_subflow(struct sock *sk, struct sock *ssk)
> +{
> +       const struct task_security_struct *tsec = selinux_cred(current_cred());
> +       struct sk_security_struct *ssksec = ssk->sk_security;
> +       u16 sclass;
> +       u32 sid;
> +       int err;
> +
> +       /* create the sid using the current cred, regardless of the ssk kern
> +        * flag
> +        */
> +       sclass = socket_type_to_security_class(ssk->sk_family, ssk->sk_type,
> +                                              ssk->sk_protocol);
> +       err = socket_sockcreate_sid(tsec, sclass, &sid);
> +       if (err)
> +               return err;
> +
> +       ssksec->sid = sid;
> +
> +       /* replace the existing subflow label deleting the existing one
> +        * and re-recrating a new label using the current context
> +        */
> +       selinux_netlbl_sk_security_free(ssksec);
> +       return selinux_netlbl_socket_post_create(ssk, ssk->sk_family);
> +}

I thought the idea was to ensure that new subflows of an existing
MPTCP connection would be created with the same label as the main
MPTCP connection socket?  The code above labels the new subflow based
on the current process, not the main MPTCP connection; it matches the
commit description, but not what we had previously discussed - or I am
horribly mis-remembering something? :)

I was expecting something more like the following:

static int selinux_mptcp_add_subflow(...)
{
  struct sk_security_struct *sksec = sk->sk_security;
  struct sk_security_struct *ssksec = ssk->sk_security;

  ssksec->sclass = sksec->sclass;
  ssksec->sid = sksec->sid;

  selinux_netlbl_sk_security_free(ssksec);
  selinux_netlbl_socket_post_create(ssk, ssk->sk_family);
}

> diff --git a/security/selinux/netlabel.c b/security/selinux/netlabel.c
> index 1321f15799e2..8e0080b8a8ef 100644
> --- a/security/selinux/netlabel.c
> +++ b/security/selinux/netlabel.c
> @@ -155,8 +155,10 @@ void selinux_netlbl_err(struct sk_buff *skb, u16 family, int error, int gateway)
>   */
>  void selinux_netlbl_sk_security_free(struct sk_security_struct *sksec)
>  {
> -       if (sksec->nlbl_secattr != NULL)
> +       if (sksec->nlbl_secattr != NULL) {
>                 netlbl_secattr_free(sksec->nlbl_secattr);
> +               sksec->nlbl_secattr = NULL;
> +       }
>  }

This is pretty nitpicky, but it might be a little cleaner to use the
pattern below.  At the very least I think it tends to better match a
lot of the various free helpers in the kernel.

void free_stuff(void *ptr)
{
  if (!ptr)
    return;
  free_properly(ptr);
}

I would probably also reset sk_security_struct::nlbl_state too, so
maybe something like the following:

void selinux_netlbl_sk_security_free(...)
{
  if (!sksec)
    return;
  netlbl_secattr_free(sksec->nlbl_secattr);
  sksec->nlbl_secattr = NULL;
  sksec->nlbl_state = NLBL_UNSET;
}

-- 
paul-moore.com

Re: [PATCH v2 2/2] selinux: Implement mptcp_add_subflow hook

[Paolo Abeni]

On Tue, 2022-12-20 at 17:07 -0500, Paul Moore wrote:
> On Mon, Dec 19, 2022 at 12:34 PM Paolo Abeni <pabeni@redhat.com> wrote:
> > 
> > Newly added subflows should inherit the associated label
> > from the current process context, regarless of the sk_kern_sock
> > flag value.
> > 
> > This patch implements the above resetting the subflow sid, deleting
> > the existing subflow label, if any, and then re-creating a new one.
> > 
> > The new helper reuses the selinux_netlbl_sk_security_free() function,
> > and it can end-up being called multiple times with the same argument;
> > we additionally need to make it idempotent.
> > 
> > Signed-off-by: Paolo Abeni <pabeni@redhat.com>
> > ---
> > v1 -> v2:
> >  - fix build issue with !CONFIG_NETLABEL
> > ---
> >  security/selinux/hooks.c    | 27 +++++++++++++++++++++++++++
> >  security/selinux/netlabel.c |  4 +++-
> >  2 files changed, 30 insertions(+), 1 deletion(-)
> > 
> > diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
> > index 3c5be76a9199..f785600b666a 100644
> > --- a/security/selinux/hooks.c
> > +++ b/security/selinux/hooks.c
> > @@ -5476,6 +5476,32 @@ static void selinux_sctp_sk_clone(struct sctp_association *asoc, struct sock *sk
> >         selinux_netlbl_sctp_sk_clone(sk, newsk);
> >  }
> > 
> > +static int selinux_mptcp_add_subflow(struct sock *sk, struct sock *ssk)
> > +{
> > +       const struct task_security_struct *tsec = selinux_cred(current_cred());
> > +       struct sk_security_struct *ssksec = ssk->sk_security;
> > +       u16 sclass;
> > +       u32 sid;
> > +       int err;
> > +
> > +       /* create the sid using the current cred, regardless of the ssk kern
> > +        * flag
> > +        */
> > +       sclass = socket_type_to_security_class(ssk->sk_family, ssk->sk_type,
> > +                                              ssk->sk_protocol);
> > +       err = socket_sockcreate_sid(tsec, sclass, &sid);
> > +       if (err)
> > +               return err;
> > +
> > +       ssksec->sid = sid;
> > +
> > +       /* replace the existing subflow label deleting the existing one
> > +        * and re-recrating a new label using the current context
> > +        */
> > +       selinux_netlbl_sk_security_free(ssksec);
> > +       return selinux_netlbl_socket_post_create(ssk, ssk->sk_family);
> > +}
> 
> I thought the idea was to ensure that new subflows of an existing
> MPTCP connection would be created with the same label as the main
> MPTCP connection socket?  The code above labels the new subflow based
> on the current process, not the main MPTCP connection; it matches the
> commit description, but not what we had previously discussed - or I am
> horribly mis-remembering something? :)

You are right, I picked a wrong turn.

I just tested the other option and there is another problem :(

The first subflow creations happens inside af_inet->create, via the sk-
>sk_prot->init() hook. The security_socket_post_create() call on the
owning MPTCP sockets happens after that point. So we copy data from a
not yet initialized security context (and the test fail badly).

There are a few options to cope with that:
- [ugly hack] call  security_socket_post_create() on the mptcp code
before creating the subflow. I experimented this just to double the
problem and a possible solution.

- refactor the mptcp code to create the first subflow on later
syscalls, as needed. This will require quite a bit of refactoring in
the MPTCP protocol as we will need also to update the
shutdown/disconnect accordingly (currently we keep the first subflow
around, instead we will need to close it).

- use the code proposed in these patches as-is ;) 

WDYT?

Thanks,

Paolo

Re: [PATCH v2 2/2] selinux: Implement mptcp_add_subflow hook

[Paul Moore]

On Wed, Dec 21, 2022 at 2:24 PM Paolo Abeni <pabeni@redhat.com> wrote:
> On Tue, 2022-12-20 at 17:07 -0500, Paul Moore wrote:
> > On Mon, Dec 19, 2022 at 12:34 PM Paolo Abeni <pabeni@redhat.com> wrote:
> > >
> > > Newly added subflows should inherit the associated label
> > > from the current process context, regarless of the sk_kern_sock
> > > flag value.
> > >
> > > This patch implements the above resetting the subflow sid, deleting
> > > the existing subflow label, if any, and then re-creating a new one.
> > >
> > > The new helper reuses the selinux_netlbl_sk_security_free() function,
> > > and it can end-up being called multiple times with the same argument;
> > > we additionally need to make it idempotent.
> > >
> > > Signed-off-by: Paolo Abeni <pabeni@redhat.com>
> > > ---
> > > v1 -> v2:
> > >  - fix build issue with !CONFIG_NETLABEL
> > > ---
> > >  security/selinux/hooks.c    | 27 +++++++++++++++++++++++++++
> > >  security/selinux/netlabel.c |  4 +++-
> > >  2 files changed, 30 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
> > > index 3c5be76a9199..f785600b666a 100644
> > > --- a/security/selinux/hooks.c
> > > +++ b/security/selinux/hooks.c
> > > @@ -5476,6 +5476,32 @@ static void selinux_sctp_sk_clone(struct sctp_association *asoc, struct sock *sk
> > >         selinux_netlbl_sctp_sk_clone(sk, newsk);
> > >  }
> > >
> > > +static int selinux_mptcp_add_subflow(struct sock *sk, struct sock *ssk)
> > > +{
> > > +       const struct task_security_struct *tsec = selinux_cred(current_cred());
> > > +       struct sk_security_struct *ssksec = ssk->sk_security;
> > > +       u16 sclass;
> > > +       u32 sid;
> > > +       int err;
> > > +
> > > +       /* create the sid using the current cred, regardless of the ssk kern
> > > +        * flag
> > > +        */
> > > +       sclass = socket_type_to_security_class(ssk->sk_family, ssk->sk_type,
> > > +                                              ssk->sk_protocol);
> > > +       err = socket_sockcreate_sid(tsec, sclass, &sid);
> > > +       if (err)
> > > +               return err;
> > > +
> > > +       ssksec->sid = sid;
> > > +
> > > +       /* replace the existing subflow label deleting the existing one
> > > +        * and re-recrating a new label using the current context
> > > +        */
> > > +       selinux_netlbl_sk_security_free(ssksec);
> > > +       return selinux_netlbl_socket_post_create(ssk, ssk->sk_family);
> > > +}
> >
> > I thought the idea was to ensure that new subflows of an existing
> > MPTCP connection would be created with the same label as the main
> > MPTCP connection socket?  The code above labels the new subflow based
> > on the current process, not the main MPTCP connection; it matches the
> > commit description, but not what we had previously discussed - or I am
> > horribly mis-remembering something? :)
>
> You are right, I picked a wrong turn.
>
> I just tested the other option and there is another problem :(

It's never easy, is it? ;)

> The first subflow creations happens inside af_inet->create, via the sk-
> >sk_prot->init() hook. The security_socket_post_create() call on the
> owning MPTCP sockets happens after that point. So we copy data from a
> not yet initialized security context (and the test fail badly).

Hmmm.  Let's come back to this later on down this email.

> There are a few options to cope with that:
> - [ugly hack] call  security_socket_post_create() on the mptcp code
> before creating the subflow. I experimented this just to double the
> problem and a possible solution.

I'm guessing "[ugly hack]" is probably a bit of an understatement.
Let's see if we can do better before we explore this option too much
further.

> - refactor the mptcp code to create the first subflow on later
> syscalls, as needed. This will require quite a bit of refactoring in
> the MPTCP protocol as we will need also to update the
> shutdown/disconnect accordingly (currently we keep the first subflow
> around, instead we will need to close it).

Let's see if we can avoid having to do this too.

> - use the code proposed in these patches as-is ;)

That's a non-starter from a SELinux perspective as the label is
incorrect.  It would be similar to saying that for each fork() call
the resulting child process would always run as root since we couldn't
figure out how to assign the credentials properly :)

> WDYT?

Let's go back to the the inet_create() case for a little bit.  I'm
thinking we might be able to do something by leveraging the
sk_alloc()->sk_prot_alloc()->security_sk_alloc() code path.  As
inet_create() is going to be called from task context here, it seems
like we could do the sock's sid/sclass determination here, cached in
separate fields in the sk_security_struct if necessary, and use those
in a new MPTCP subflow hook.  We could also update
selinux_socket_post_create() to take advantage of this as well.  We
could also possibly pass the proto struct into security_sk_alloc() if
we needed to identify IPPROTO_MPTCP there as well.

I'll admit to not chasing down all the details, but I suspect this may
be the cleanest option - thoughts?

--
paul-moore.com

Re: [PATCH v2 2/2] selinux: Implement mptcp_add_subflow hook

[Paolo Abeni]

On Wed, 2022-12-21 at 20:21 -0500, Paul Moore wrote:
> On Wed, Dec 21, 2022 at 2:24 PM Paolo Abeni <pabeni@redhat.com> wrote:
> > I just tested the other option and there is another problem :(
> 
> It's never easy, is it? ;)
> 
> > The first subflow creations happens inside af_inet->create, via the sk-
> > > sk_prot->init() hook. The security_socket_post_create() call on the
> > owning MPTCP sockets happens after that point. So we copy data from a
> > not yet initialized security context (and the test fail badly).
> 
> Hmmm.  Let's come back to this later on down this email.
> 
> > There are a few options to cope with that:
> > - [ugly hack] call  security_socket_post_create() on the mptcp code
> > before creating the subflow. I experimented this just to double the
> > problem and a possible solution.
> 
> I'm guessing "[ugly hack]" is probably a bit of an understatement.
> Let's see if we can do better before we explore this option too much
> further.

Yup, I compiled the list in "brainstom-mode", trying to include
whatever would be possible even if clearly not suitable. 

[...]

> > WDYT?
> 
> Let's go back to the the inet_create() case for a little bit.  I'm
> thinking we might be able to do something by leveraging the
> sk_alloc()->sk_prot_alloc()->security_sk_alloc() code path.  As
> inet_create() is going to be called from task context here, it seems
> like we could do the sock's sid/sclass determination here, cached in
> separate fields in the sk_security_struct if necessary, and use those
> in a new MPTCP subflow hook.  We could also update
> selinux_socket_post_create() to take advantage of this as well.  We
> could also possibly pass the proto struct into security_sk_alloc() if
> we needed to identify IPPROTO_MPTCP there as well.
> 
> I'll admit to not chasing down all the details, but I suspect this may
> be the cleanest option - thoughts?

Thanks, I did not consider such possibility!

I think we should be careful to avoid increasing sk_security_struct
size. Currently it is 16 bytes, nicely matching a kmalloc slab, any
increase will move it on kmalloc-32 bytes slab possibly causing
performance and memory regressions).

More importantly, I think there is a problem with the 
sk_clone_lock() -> sk_prot_alloc() -> security_sk_alloc()
code path. 

sk_clone_lock() happens in BH context, if security_transition_sid()
needs process context that would be a problem - quickly skimming the
code it does not look so, I need to double check.

sk_clone_lock() is in a very critical path - socket creation for
incoming connections. The sid-related operation there will be
unnecessary/discarded by later the selinux_inet_csk_clone(), this will
likelly cause performance regressions even for plain TCP sockets.

Perhaps the cleanest option could be the one involving the mptcp
refactoring, moving subflow creation at a later stage. It could have
some minor side benefit for MPTCP, too - solving:

https://github.com/multipath-tcp/mptcp_net-next/issues/290

but I'm not fond of that option because it will require quite a bit of
time: we need first to have the mptcp refactor in place and then cook
the lsm patches. I guess such process will require at least 2 release
cycles, due to the needed mptcp(netdev)/lsm trees synchronization.

If that would prove to be the most reasonable option, could we consider
to transiently merge first something alike:

https://lore.kernel.org/mptcp/CAHC9VhSQnhH3UL4gqzu+YiA1Q3YyLLCv88gLJOvw-0+uw5Lvkw@mail.gmail.com/T/#m06c612f84f6b6fe759e670573b2c8092df71607b

to have a workable short-term solution, and later revert it when the
final solution would be in place?

Thanks,

Paolo

Re: [PATCH v2 2/2] selinux: Implement mptcp_add_subflow hook

[Paul Moore]

On Thu, Dec 22, 2022 at 10:57 AM Paolo Abeni <pabeni@redhat.com> wrote:
>
> On Wed, 2022-12-21 at 20:21 -0500, Paul Moore wrote:
> > On Wed, Dec 21, 2022 at 2:24 PM Paolo Abeni <pabeni@redhat.com> wrote:
> > > I just tested the other option and there is another problem :(
> >
> > It's never easy, is it? ;)
> >
> > > The first subflow creations happens inside af_inet->create, via the sk-
> > > > sk_prot->init() hook. The security_socket_post_create() call on the
> > > owning MPTCP sockets happens after that point. So we copy data from a
> > > not yet initialized security context (and the test fail badly).
> >
> > Hmmm.  Let's come back to this later on down this email.
> >
> > > There are a few options to cope with that:
> > > - [ugly hack] call  security_socket_post_create() on the mptcp code
> > > before creating the subflow. I experimented this just to double the
> > > problem and a possible solution.
> >
> > I'm guessing "[ugly hack]" is probably a bit of an understatement.
> > Let's see if we can do better before we explore this option too much
> > further.
>
> Yup, I compiled the list in "brainstom-mode", trying to include
> whatever would be possible even if clearly not suitable.
>
> [...]
>
> > > WDYT?
> >
> > Let's go back to the the inet_create() case for a little bit.  I'm
> > thinking we might be able to do something by leveraging the
> > sk_alloc()->sk_prot_alloc()->security_sk_alloc() code path.  As
> > inet_create() is going to be called from task context here, it seems
> > like we could do the sock's sid/sclass determination here, cached in
> > separate fields in the sk_security_struct if necessary, and use those
> > in a new MPTCP subflow hook.  We could also update
> > selinux_socket_post_create() to take advantage of this as well.  We
> > could also possibly pass the proto struct into security_sk_alloc() if
> > we needed to identify IPPROTO_MPTCP there as well.
> >
> > I'll admit to not chasing down all the details, but I suspect this may
> > be the cleanest option - thoughts?
>
> Thanks, I did not consider such possibility!
>
> I think we should be careful to avoid increasing sk_security_struct
> size. Currently it is 16 bytes, nicely matching a kmalloc slab, any
> increase will move it on kmalloc-32 bytes slab possibly causing
> performance and memory regressions).

FWIW, it is likely that this will end up growing in the future to
support stacking LSMs.  It's unfortunate, messy, and generally ugly,
but inevitable.

See the selinux_inode() function as a similar example using
inode/inode_security_struct.

> More importantly, I think there is a problem with the
> sk_clone_lock() -> sk_prot_alloc() -> security_sk_alloc()
> code path.
>
> sk_clone_lock() happens in BH context, if security_transition_sid()
> needs process context that would be a problem - quickly skimming the
> code it does not look so, I need to double check.

The problem is that in both selinux_socket_create() and
selinux_socket_post_create() the credentials from @current are needed
to determine the sock/sk_security_stuct label.  In
selinux_socket_create() @current's credentials are also used to
determine if the socket can be created.

It's looking like doing labeling determinations in the
security_sk_alloc() struct is not going to work.  While
sk_clone_lock() will end up calling into
security_sk_clone()/selinux_sk_clone_security() via sock_copy(), if I
understand you correctly that won't help as the main MPTCP socket is
not yet setup (e.g. selinux_socket_post_create() has not yet been run
on the main socket).

> Perhaps the cleanest option could be the one involving the mptcp
> refactoring, moving subflow creation at a later stage. It could have
> some minor side benefit for MPTCP, too - solving:
>
> https://github.com/multipath-tcp/mptcp_net-next/issues/290
>
> but I'm not fond of that option because it will require quite a bit of
> time: we need first to have the mptcp refactor in place and then cook
> the lsm patches. I guess such process will require at least 2 release
> cycles, due to the needed mptcp(netdev)/lsm trees synchronization.

I generally take the long term view when it comes to Linux Kernel
development; given the nature of the kernel, and all the constraints
that come with it, I would much rather pursue solutions that we
believe have the longest staying power.

I'm also happy to work on, and/or review, LSM patches in conjunction
with a MPTCP refactor.  If the only reason to split the work over two
kernel releases is to soften the blow during the merge window, I think
we can work that out in a single release ... at least I say that now
:)

Basically when it comes down to it, I want to make sure that any fix
we come up with *works*.  In my mind that means doing the LSM fix in
conjunction with the rework; I would hate to see all of the rework
happen only to find out later that it still didn't solve the LSM
problem.

Does that make sense?

> If that would prove to be the most reasonable option, could we consider
> to transiently merge first something alike:
>
> https://lore.kernel.org/mptcp/CAHC9VhSQnhH3UL4gqzu+YiA1Q3YyLLCv88gLJOvw-0+uw5Lvkw@mail.gmail.com/T/#m06c612f84f6b6fe759e670573b2c8092df71607b
>
> to have a workable short-term solution, and later revert it when the
> final solution would be in place?

I would need to go back through that to make sure that it makes sense,
and ensure that the hook is idempotent for SELinux, AppArmor, and
Smack (current hook implementations), *aaaand* if we promise that this
is just a *temporary* hack I think I would be okay with that.

-- 
paul-moore.com

Re: [PATCH v2 2/2] selinux: Implement mptcp_add_subflow hook

[Paolo Abeni]

Hello,

I'm sorry for the long delay:  I was on PTO with limited internet
access.

On Fri, 2022-12-23 at 12:11 -0500, Paul Moore wrote:
> On Thu, Dec 22, 2022 at 10:57 AM Paolo Abeni <pabeni@redhat.com> wrote:
> > 
> > On Wed, 2022-12-21 at 20:21 -0500, Paul Moore wrote:
> > > On Wed, Dec 21, 2022 at 2:24 PM Paolo Abeni <pabeni@redhat.com> wrote:
> > > > I just tested the other option and there is another problem :(
> > > 
> > > It's never easy, is it? ;)
> > > 
> > > > The first subflow creations happens inside af_inet->create, via the sk-
> > > > > sk_prot->init() hook. The security_socket_post_create() call on the
> > > > owning MPTCP sockets happens after that point. So we copy data from a
> > > > not yet initialized security context (and the test fail badly).
> > > 
> > > Hmmm.  Let's come back to this later on down this email.
> > > 
> > > > There are a few options to cope with that:
> > > > - [ugly hack] call  security_socket_post_create() on the mptcp code
> > > > before creating the subflow. I experimented this just to double the
> > > > problem and a possible solution.
> > > 
> > > I'm guessing "[ugly hack]" is probably a bit of an understatement.
> > > Let's see if we can do better before we explore this option too much
> > > further.
> > 
> > Yup, I compiled the list in "brainstom-mode", trying to include
> > whatever would be possible even if clearly not suitable.
> > 
> > [...]
> > 
> > > > WDYT?
> > > 
> > > Let's go back to the the inet_create() case for a little bit.  I'm
> > > thinking we might be able to do something by leveraging the
> > > sk_alloc()->sk_prot_alloc()->security_sk_alloc() code path.  As
> > > inet_create() is going to be called from task context here, it seems
> > > like we could do the sock's sid/sclass determination here, cached in
> > > separate fields in the sk_security_struct if necessary, and use those
> > > in a new MPTCP subflow hook.  We could also update
> > > selinux_socket_post_create() to take advantage of this as well.  We
> > > could also possibly pass the proto struct into security_sk_alloc() if
> > > we needed to identify IPPROTO_MPTCP there as well.
> > > 
> > > I'll admit to not chasing down all the details, but I suspect this may
> > > be the cleanest option - thoughts?
> > 
> > Thanks, I did not consider such possibility!
> > 
> > I think we should be careful to avoid increasing sk_security_struct
> > size. Currently it is 16 bytes, nicely matching a kmalloc slab, any
> > increase will move it on kmalloc-32 bytes slab possibly causing
> > performance and memory regressions).
> 
> FWIW, it is likely that this will end up growing in the future to
> support stacking LSMs.  It's unfortunate, messy, and generally ugly,
> but inevitable.
> 
> See the selinux_inode() function as a similar example using
> inode/inode_security_struct.
> 
> > More importantly, I think there is a problem with the
> > sk_clone_lock() -> sk_prot_alloc() -> security_sk_alloc()
> > code path.
> > 
> > sk_clone_lock() happens in BH context, if security_transition_sid()
> > needs process context that would be a problem - quickly skimming the
> > code it does not look so, I need to double check.
> 
> The problem is that in both selinux_socket_create() and
> selinux_socket_post_create() the credentials from @current are needed
> to determine the sock/sk_security_stuct label.  In
> selinux_socket_create() @current's credentials are also used to
> determine if the socket can be created.
> 
> It's looking like doing labeling determinations in the
> security_sk_alloc() struct is not going to work.  While
> sk_clone_lock() will end up calling into
> security_sk_clone()/selinux_sk_clone_security() via sock_copy(), if I
> understand you correctly that won't help as the main MPTCP socket is
> not yet setup (e.g. selinux_socket_post_create() has not yet been run
> on the main socket).
> 
> > Perhaps the cleanest option could be the one involving the mptcp
> > refactoring, moving subflow creation at a later stage. It could have
> > some minor side benefit for MPTCP, too - solving:
> > 
> > https://github.com/multipath-tcp/mptcp_net-next/issues/290
> > 
> > but I'm not fond of that option because it will require quite a bit of
> > time: we need first to have the mptcp refactor in place and then cook
> > the lsm patches. I guess such process will require at least 2 release
> > cycles, due to the needed mptcp(netdev)/lsm trees synchronization.
> 
> I generally take the long term view when it comes to Linux Kernel
> development; given the nature of the kernel, and all the constraints
> that come with it, I would much rather pursue solutions that we
> believe have the longest staying power.
> 
> I'm also happy to work on, and/or review, LSM patches in conjunction
> with a MPTCP refactor.  If the only reason to split the work over two
> kernel releases is to soften the blow during the merge window, I think
> we can work that out in a single release ... at least I say that now
> :)

I thought about doing the MPTCP and selinux patches sequentially to
both avoid the possibly untrivial conflicts resultion issues and to
ensure that the mptcp patches are in place when the selinux ones are
applied, as there is a fuctional dependency. 

> Basically when it comes down to it, I want to make sure that any fix
> we come up with *works*.  In my mind that means doing the LSM fix in
> conjunction with the rework; I would hate to see all of the rework
> happen only to find out later that it still didn't solve the LSM
> problem.
> 
> Does that make sense?

Indeed it makes sense to me.

I think we can address that concern in a quite consolidated way. We
usually include in the MPTCP tree a (very limited) number of patches
that will not be submitted to the netdev because belong to other trees
and/or are handled/owned by others devel. 

We use the above e.g. to fix build and/or functional issues in our
self-tests caused by other subsystems without the need to wait for the
proper fix to land into vanilla. When such event happen, we simply drop
the local copy of the fixup patch.

We could use a similar schema in this scenario. We can include the the
LSM patches to the mptcp in our tree while the rework is in progress to
ensure that overall the effort really addresses the LSM issue.

We can rebase the LSM patches as needed to address conflicts as
needed/if/when they pops up.

Once that the mptcp patches will land into the LSM tree, we will submit
formally the LSM ones to you. During the process I'll check and ensure
that the LSM issue is really/still fixed. Would that work for you?

> > If that would prove to be the most reasonable option, could we consider
> > to transiently merge first something alike:
> > 
> > https://lore.kernel.org/mptcp/CAHC9VhSQnhH3UL4gqzu+YiA1Q3YyLLCv88gLJOvw-0+uw5Lvkw@mail.gmail.com/T/#m06c612f84f6b6fe759e670573b2c8092df71607b
> > 
> > to have a workable short-term solution, and later revert it when the
> > final solution would be in place?
> 
> I would need to go back through that to make sure that it makes sense,
> and ensure that the hook is idempotent for SELinux, AppArmor, and
> Smack (current hook implementations), *aaaand* if we promise that this
> is just a *temporary* hack I think I would be okay with that.

I would appreciate that addtional extra mile a lot, as it will allow a
(temporary!) fix to be delivered quite earlier than what the above
process will provide. 

Of course the deal is that I'll take ownership of pursuing the complete
fix till resolution.

Many thanks!

Paolo

Re: [PATCH v2 2/2] selinux: Implement mptcp_add_subflow hook

[Paul Moore]

On Mon, Jan 9, 2023 at 5:31 AM Paolo Abeni <pabeni@redhat.com> wrote:
> Hello,
>
> I'm sorry for the long delay:  I was on PTO with limited internet
> access.

No worries, I hear even kernel developers are allowed to take time off
occasionally ... at least that's what they tell me ;)

(I hope you're enjoying the time away!)

> On Fri, 2022-12-23 at 12:11 -0500, Paul Moore wrote:
> > On Thu, Dec 22, 2022 at 10:57 AM Paolo Abeni <pabeni@redhat.com> wrote:
> > > On Wed, 2022-12-21 at 20:21 -0500, Paul Moore wrote:
> > > > On Wed, Dec 21, 2022 at 2:24 PM Paolo Abeni <pabeni@redhat.com> wrote:

...

> > I generally take the long term view when it comes to Linux Kernel
> > development; given the nature of the kernel, and all the constraints
> > that come with it, I would much rather pursue solutions that we
> > believe have the longest staying power.
> >
> > I'm also happy to work on, and/or review, LSM patches in conjunction
> > with a MPTCP refactor.  If the only reason to split the work over two
> > kernel releases is to soften the blow during the merge window, I think
> > we can work that out in a single release ... at least I say that now
> > :)
>
> I thought about doing the MPTCP and selinux patches sequentially to
> both avoid the possibly untrivial conflicts resultion issues and to
> ensure that the mptcp patches are in place when the selinux ones are
> applied, as there is a fuctional dependency.

Sure, when working across subsystems there is usually some sort of
dependency challenge which dictates which side is tackled first, I
just wanted to make sure we don't consider one side "done" until we
finish both sides.

> > Basically when it comes down to it, I want to make sure that any fix
> > we come up with *works*.  In my mind that means doing the LSM fix in
> > conjunction with the rework; I would hate to see all of the rework
> > happen only to find out later that it still didn't solve the LSM
> > problem.
> >
> > Does that make sense?
>
> Indeed it makes sense to me.
>
> I think we can address that concern in a quite consolidated way. We
> usually include in the MPTCP tree a (very limited) number of patches
> that will not be submitted to the netdev because belong to other trees
> and/or are handled/owned by others devel.
>
> We use the above e.g. to fix build and/or functional issues in our
> self-tests caused by other subsystems without the need to wait for the
> proper fix to land into vanilla. When such event happen, we simply drop
> the local copy of the fixup patch.
>
> We could use a similar schema in this scenario. We can include the the
> LSM patches to the mptcp in our tree while the rework is in progress to
> ensure that overall the effort really addresses the LSM issue.
>
> We can rebase the LSM patches as needed to address conflicts as
> needed/if/when they pops up.
>
> Once that the mptcp patches will land into the LSM tree, we will submit
> formally the LSM ones to you. During the process I'll check and ensure
> that the LSM issue is really/still fixed. Would that work for you?

Sure, I'm pretty flexible on how we do these things, I just want it to
end up fixed in Linus' tree and there are plenty of ways we can make
that happen :)

> > > If that would prove to be the most reasonable option, could we consider
> > > to transiently merge first something alike:
> > >
> > > https://lore.kernel.org/mptcp/CAHC9VhSQnhH3UL4gqzu+YiA1Q3YyLLCv88gLJOvw-0+uw5Lvkw@mail.gmail.com/T/#m06c612f84f6b6fe759e670573b2c8092df71607b
> > >
> > > to have a workable short-term solution, and later revert it when the
> > > final solution would be in place?
> >
> > I would need to go back through that to make sure that it makes sense,
> > and ensure that the hook is idempotent for SELinux, AppArmor, and
> > Smack (current hook implementations), *aaaand* if we promise that this
> > is just a *temporary* hack I think I would be okay with that.
>
> I would appreciate that addtional extra mile a lot, as it will allow a
> (temporary!) fix to be delivered quite earlier than what the above
> process will provide.
>
> Of course the deal is that I'll take ownership of pursuing the complete
> fix till resolution.

Okay, let me do the investigation mentioned above to make sure this is
possible and report back (I need to refresh my memory first, the
holiday break kinda flushed this out of my mind ;).

-- 
paul-moore.com