[MPTCP] [PATCH net] mptcp: fix NULL ptr dereference on bad MPJ

[MPTCP] [PATCH net] mptcp: fix NULL ptr dereference on bad MPJ

[Paolo Abeni]

[-- Attachment #1: Type: text/plain, Size: 1529 bytes --]

If an msk listener receives an MPJ carrying an invalid token, it
will zero the request socket msk entry. That should later
cause fallback and subflow reset - as per RFC - at
subflow_syn_recv_sock() time due to failing hmac validation.

Since commit 4cf8b7e48a09 ("subflow: introduce and use
mptcp_can_accept_new_subflow()"), we unconditionally dereference
- in mptcp_can_accept_new_subflow - the subflow request msk
before performing hmac validation. In the above scenario we
hit a NULL ptr dereference.

Address the issue doing the hmac validation earlier.

Fixes: 4cf8b7e48a09 ("subflow: introduce and use mptcp_can_accept_new_subflow()")
Tested-by: Davide Caratti <dcaratti(a)redhat.com>
Signed-off-by: Paolo Abeni <pabeni(a)redhat.com>
---
 net/mptcp/subflow.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/net/mptcp/subflow.c b/net/mptcp/subflow.c
index ac4a1fe3550b..953906e40742 100644
--- a/net/mptcp/subflow.c
+++ b/net/mptcp/subflow.c
@@ -543,9 +543,8 @@ static struct sock *subflow_syn_recv_sock(const struct sock *sk,
 			fallback = true;
 	} else if (subflow_req->mp_join) {
 		mptcp_get_options(skb, &mp_opt);
-		if (!mp_opt.mp_join ||
-		    !mptcp_can_accept_new_subflow(subflow_req->msk) ||
-		    !subflow_hmac_valid(req, &mp_opt)) {
+		if (!mp_opt.mp_join || !subflow_hmac_valid(req, &mp_opt) ||
+		    !mptcp_can_accept_new_subflow(subflow_req->msk)) {
 			SUBFLOW_REQ_INC_STATS(req, MPTCP_MIB_JOINACKMAC);
 			fallback = true;
 		}
-- 
2.26.2

[PATCH net] mptcp: fix NULL ptr dereference on bad MPJ

[Paolo Abeni]

If an msk listener receives an MPJ carrying an invalid token, it
will zero the request socket msk entry. That should later
cause fallback and subflow reset - as per RFC - at
subflow_syn_recv_sock() time due to failing hmac validation.

Since commit 4cf8b7e48a09 ("subflow: introduce and use
mptcp_can_accept_new_subflow()"), we unconditionally dereference
- in mptcp_can_accept_new_subflow - the subflow request msk
before performing hmac validation. In the above scenario we
hit a NULL ptr dereference.

Address the issue doing the hmac validation earlier.

Fixes: 4cf8b7e48a09 ("subflow: introduce and use mptcp_can_accept_new_subflow()")
Tested-by: Davide Caratti <dcaratti@redhat.com>
Signed-off-by: Paolo Abeni <pabeni@redhat.com>
---
 net/mptcp/subflow.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/net/mptcp/subflow.c b/net/mptcp/subflow.c
index ac4a1fe3550b..953906e40742 100644
--- a/net/mptcp/subflow.c
+++ b/net/mptcp/subflow.c
@@ -543,9 +543,8 @@ static struct sock *subflow_syn_recv_sock(const struct sock *sk,
 			fallback = true;
 	} else if (subflow_req->mp_join) {
 		mptcp_get_options(skb, &mp_opt);
-		if (!mp_opt.mp_join ||
-		    !mptcp_can_accept_new_subflow(subflow_req->msk) ||
-		    !subflow_hmac_valid(req, &mp_opt)) {
+		if (!mp_opt.mp_join || !subflow_hmac_valid(req, &mp_opt) ||
+		    !mptcp_can_accept_new_subflow(subflow_req->msk)) {
 			SUBFLOW_REQ_INC_STATS(req, MPTCP_MIB_JOINACKMAC);
 			fallback = true;
 		}
-- 
2.26.2

[MPTCP] Re: [PATCH net] mptcp: fix NULL ptr dereference on bad MPJ

[Matthieu Baerts]

[-- Attachment #1: Type: text/plain, Size: 1031 bytes --]

Hi Paolo,

On 26/11/2020 15:17, Paolo Abeni wrote:
> If an msk listener receives an MPJ carrying an invalid token, it
> will zero the request socket msk entry. That should later
> cause fallback and subflow reset - as per RFC - at
> subflow_syn_recv_sock() time due to failing hmac validation.
> 
> Since commit 4cf8b7e48a09 ("subflow: introduce and use
> mptcp_can_accept_new_subflow()"), we unconditionally dereference
> - in mptcp_can_accept_new_subflow - the subflow request msk
> before performing hmac validation. In the above scenario we
> hit a NULL ptr dereference.
> 
> Address the issue doing the hmac validation earlier.
> 
> Fixes: 4cf8b7e48a09 ("subflow: introduce and use mptcp_can_accept_new_subflow()")
> Tested-by: Davide Caratti <dcaratti(a)redhat.com>
> Signed-off-by: Paolo Abeni <pabeni(a)redhat.com>

Good catch! Thank you for the patch!

Reviewed-by: Matthieu Baerts <matthieu.baerts(a)tessares.net>

Cheers,
Matt
-- 
Tessares | Belgium | Hybrid Access Solutions
www.tessares.net

Re: [PATCH net] mptcp: fix NULL ptr dereference on bad MPJ

[Matthieu Baerts]

Hi Paolo,

On 26/11/2020 15:17, Paolo Abeni wrote:
> If an msk listener receives an MPJ carrying an invalid token, it
> will zero the request socket msk entry. That should later
> cause fallback and subflow reset - as per RFC - at
> subflow_syn_recv_sock() time due to failing hmac validation.
> 
> Since commit 4cf8b7e48a09 ("subflow: introduce and use
> mptcp_can_accept_new_subflow()"), we unconditionally dereference
> - in mptcp_can_accept_new_subflow - the subflow request msk
> before performing hmac validation. In the above scenario we
> hit a NULL ptr dereference.
> 
> Address the issue doing the hmac validation earlier.
> 
> Fixes: 4cf8b7e48a09 ("subflow: introduce and use mptcp_can_accept_new_subflow()")
> Tested-by: Davide Caratti <dcaratti@redhat.com>
> Signed-off-by: Paolo Abeni <pabeni@redhat.com>

Good catch! Thank you for the patch!

Reviewed-by: Matthieu Baerts <matthieu.baerts@tessares.net>

Cheers,
Matt
-- 
Tessares | Belgium | Hybrid Access Solutions
www.tessares.net

[MPTCP] Re: [PATCH net] mptcp: fix NULL ptr dereference on bad MPJ

[Jakub Kicinski]

[-- Attachment #1: Type: text/plain, Size: 1055 bytes --]

On Thu, 26 Nov 2020 16:23:08 +0100 Matthieu Baerts wrote:
> On 26/11/2020 15:17, Paolo Abeni wrote:
> > If an msk listener receives an MPJ carrying an invalid token, it
> > will zero the request socket msk entry. That should later
> > cause fallback and subflow reset - as per RFC - at
> > subflow_syn_recv_sock() time due to failing hmac validation.
> > 
> > Since commit 4cf8b7e48a09 ("subflow: introduce and use
> > mptcp_can_accept_new_subflow()"), we unconditionally dereference
> > - in mptcp_can_accept_new_subflow - the subflow request msk
> > before performing hmac validation. In the above scenario we
> > hit a NULL ptr dereference.
> > 
> > Address the issue doing the hmac validation earlier.
> > 
> > Fixes: 4cf8b7e48a09 ("subflow: introduce and use mptcp_can_accept_new_subflow()")
> > Tested-by: Davide Caratti <dcaratti(a)redhat.com>
> > Signed-off-by: Paolo Abeni <pabeni(a)redhat.com>  
> 
> Good catch! Thank you for the patch!
> 
> Reviewed-by: Matthieu Baerts <matthieu.baerts(a)tessares.net>

Applied, thanks!

Re: [PATCH net] mptcp: fix NULL ptr dereference on bad MPJ

[Jakub Kicinski]

On Thu, 26 Nov 2020 16:23:08 +0100 Matthieu Baerts wrote:
> On 26/11/2020 15:17, Paolo Abeni wrote:
> > If an msk listener receives an MPJ carrying an invalid token, it
> > will zero the request socket msk entry. That should later
> > cause fallback and subflow reset - as per RFC - at
> > subflow_syn_recv_sock() time due to failing hmac validation.
> > 
> > Since commit 4cf8b7e48a09 ("subflow: introduce and use
> > mptcp_can_accept_new_subflow()"), we unconditionally dereference
> > - in mptcp_can_accept_new_subflow - the subflow request msk
> > before performing hmac validation. In the above scenario we
> > hit a NULL ptr dereference.
> > 
> > Address the issue doing the hmac validation earlier.
> > 
> > Fixes: 4cf8b7e48a09 ("subflow: introduce and use mptcp_can_accept_new_subflow()")
> > Tested-by: Davide Caratti <dcaratti@redhat.com>
> > Signed-off-by: Paolo Abeni <pabeni@redhat.com>  
> 
> Good catch! Thank you for the patch!
> 
> Reviewed-by: Matthieu Baerts <matthieu.baerts@tessares.net>

Applied, thanks!

[MPTCP] Re: [PATCH net] mptcp: fix NULL ptr dereference on bad MPJ

[Davide Caratti]

[-- Attachment #1: Type: text/plain, Size: 1062 bytes --]

On Thu, 2020-11-26 at 11:15 +0100, Paolo Abeni wrote:
> If an msk listener receives an MPJ carrying an invalid token, it
> will zero the request socket msk entry. That should later
> cause fallback and subflow reset - as per RFC - at
> subflow_syn_recv_sock() time due to failing hmac validation.
> 
> Since commit 4cf8b7e48a09 ("subflow: introduce and use
> mptcp_can_accept_new_subflow()"), we unconditionally dereference
> - in mptcp_can_accept_new_subflow - the subflow request msk
> before performing hmac validation. In the above scenario we
> hit a NULL ptr dereference.
> 
> Address the issue doing the hmac validation earlier.
> 
> Fixes: 4cf8b7e48a09 ("subflow: introduce and use mptcp_can_accept_new_subflow()")
> Signed-off-by: Paolo Abeni <pabeni(a)redhat.com>

kernel does not crash anymore. It still sends a 0 HMAC, and resets the
connection after the 3WHS - see https://paste.centos.org/view/d962741c
(not sure if it's RFC compliant, but for sure it fixes the issue :) )

Tested-by: Davide Caratti <dcaratti(a)redhat.com>

[MPTCP] Re: [PATCH net] mptcp: fix NULL ptr dereference on bad MPJ

[Matthieu Baerts]

[-- Attachment #1: Type: text/plain, Size: 981 bytes --]

Hi Paolo,

On 26/11/2020 11:15, Paolo Abeni wrote:
> If an msk listener receives an MPJ carrying an invalid token, it
> will zero the request socket msk entry. That should later
> cause fallback and subflow reset - as per RFC - at
> subflow_syn_recv_sock() time due to failing hmac validation.
> 
> Since commit 4cf8b7e48a09 ("subflow: introduce and use
> mptcp_can_accept_new_subflow()"), we unconditionally dereference
> - in mptcp_can_accept_new_subflow - the subflow request msk
> before performing hmac validation. In the above scenario we
> hit a NULL ptr dereference.
> 
> Address the issue doing the hmac validation earlier.

Thanks for the fix!

Reviewed-by: Matthieu Baerts <matthieu.baerts(a)tessares.net>

Just added in our tree:
- 8e76b1043ebe: mptcp: fix NULL ptr dereference on bad MPJ
- Results: a7a4732c5cdb..aad014029dd8

Tests + export are in progress!

Cheers,
Matt
-- 
Tessares | Belgium | Hybrid Access Solutions
www.tessares.net