Re: [MPTCP] [PATCH] mptcp: avoid validating MP_CAPABLE keys on 3way HS handling

Re: [MPTCP] [PATCH] mptcp: avoid validating MP_CAPABLE keys on 3way HS handling

[Peter Krystad]

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


Looks good to me.

On Wed, 2019-07-17 at 10:57 +0200, Paolo Abeni wrote:
> The server side already stored the key on syn, and 3way HS ack
> reception is fragile: the ack can be lost, and the following
> data pkts processed instead.
> 
> This fixes the pending self-test issue in my testbed.
> 
> Suggested-by: Peter Krystad <peter.krystad(a)linux.intel.com>
> Suggested-by: Christoph Paasch <cpaasch(a)apple.com>
> Fixes: af550dd282ca ("mptcp: Create SUBFLOW socket for incoming connections")
> Signed-off-by: Paolo Abeni <pabeni(a)redhat.com>
> --
> I'm ok with squashing into the Fixes commit, but that will likely cause
> conflicts.
> 
> We can possible have [in the future] a similar issue with MP_JOIN, as we
> validate the token on 3way HS ack reception, and that is fragile ??!
> ---
>  net/mptcp/subflow.c | 10 ++--------
>  1 file changed, 2 insertions(+), 8 deletions(-)
> 
> diff --git a/net/mptcp/subflow.c b/net/mptcp/subflow.c
> index fdc5aecab897..d91d817b8779 100644
> --- a/net/mptcp/subflow.c
> +++ b/net/mptcp/subflow.c
> @@ -143,14 +143,8 @@ static struct sock *subflow_syn_recv_sock(const struct sock *sk,
>  
>  	pr_debug("listener=%p, req=%p, conn=%p", listener, req, listener->conn);
>  
> -	if (subflow_req->mp_capable) {
> -		opt_rx.mptcp.mp_capable = 0;
> -		mptcp_get_options(skb, &opt_rx);
> -		if (!opt_rx.mptcp.mp_capable ||
> -		    subflow_req->local_key != opt_rx.mptcp.rcvr_key ||
> -		    subflow_req->remote_key != opt_rx.mptcp.sndr_key)
> -			return NULL;
> -	} else if (subflow_req->mp_join) {
> +	/* if the sk is MP_CAPABLE, we already received the client key */
> +	if (!subflow_req->mp_capable && subflow_req->mp_join) {
>  		opt_rx.mptcp.mp_join = 0;
>  		mptcp_get_options(skb, &opt_rx);
>  		if (!opt_rx.mptcp.mp_join || token_join_valid(req, &opt_rx))

Re: [MPTCP] [PATCH] mptcp: avoid validating MP_CAPABLE keys on 3way HS handling

[Matthieu Baerts]

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

Hi Paolo, Peter, Mat, Christoph,

On 17/07/2019 19:22, Peter Krystad wrote:
> 
> Looks good to me.
> 
> On Wed, 2019-07-17 at 10:57 +0200, Paolo Abeni wrote:
>> The server side already stored the key on syn, and 3way HS ack
>> reception is fragile: the ack can be lost, and the following
>> data pkts processed instead.
>>
>> This fixes the pending self-test issue in my testbed.
>>
>> Suggested-by: Peter Krystad <peter.krystad(a)linux.intel.com>
>> Suggested-by: Christoph Paasch <cpaasch(a)apple.com>
>> Fixes: af550dd282ca ("mptcp: Create SUBFLOW socket for incoming connections")
>> Signed-off-by: Paolo Abeni <pabeni(a)redhat.com>

Thank you for the patch, the reviews and the suggestions!

- 7f07e3f76561: "squashed" in "mptcp: Create SUBFLOW socket for incoming
connections"
- 17ba3a852c83: add Paolo Abeni' signed-off
- 1d55bee52791: conflict in
t/mptcp-Add-handling-of-incoming-MP_JOIN-requests
- 3e0f02bac2b3..57b73d36a000: result

Compilations is OK but tests are in progress. I has to relaunch them,
see [1] for more details. I will notify you if there are some issues
with the tests but I am sure they will be green ;-)

Cheers,
Matt

[1]
https://github.com/multipath-tcp/mptcp_net-next/commit/c53c4283cae29e73e2622d11c37cd05e90d90605
-- 
Matthieu Baerts | R&D Engineer
matthieu.baerts(a)tessares.net
Tessares SA | Hybrid Access Solutions
www.tessares.net
1 Avenue Jean Monnet, 1348 Louvain-la-Neuve, Belgium

Re: [MPTCP] [PATCH] mptcp: avoid validating MP_CAPABLE keys on 3way HS handling

[Peter Krystad]

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

On Wed, 2019-07-17 at 11:05 +0200, Christoph Paasch wrote:
> > On Jul 17, 2019, at 10:57 AM, Paolo Abeni <pabeni(a)redhat.com> wrote:
> > 
> > The server side already stored the key on syn, and 3way HS ack
> > reception is fragile: the ack can be lost, and the following
> > data pkts processed instead.
> > 
> > This fixes the pending self-test issue in my testbed.
> > 
> > Suggested-by: Peter Krystad <peter.krystad(a)linux.intel.com>
> > Suggested-by: Christoph Paasch <cpaasch(a)apple.com>
> > Fixes: af550dd282ca ("mptcp: Create SUBFLOW socket for incoming connections")
> > Signed-off-by: Paolo Abeni <pabeni(a)redhat.com>
> > --
> > I'm ok with squashing into the Fixes commit, but that will likely cause
> > conflicts.
> > 
> > We can possible have [in the future] a similar issue with MP_JOIN, as we
> > validate the token on 3way HS ack reception, and that is fragile ??!
> 
> without having easy access at the code right now:
> 
> How is the function token_join_valid() doing the validation? Because, the third ACK does not contain the token. Only the HMAC. So, upon reception of the third ACK of a new subflow the server has to validate the HMAC. The issue with a lost third ACK does not apply for new subflows because here MPTCP uses a 4-way handshake. The client won't send any data until it received the fourth ACK from the server.
> 

token_join_valid() uses the token received in the SYN (and saved in the
request sock) to look up the MPTCP connection sock, where the keys from the
original connection are saved. HMAC is calculated from keys, the nonce sent in
the SYN-ACK, and the nonce received in the SYN. But we may still have the same
problem that options are not sent on re-transmission of a lost third ACK.

Peter.

> 
> Patch looks good by the way!
> 
> Cheers,
> Christoph
> 
> > ---
> > net/mptcp/subflow.c | 10 ++--------
> > 1 file changed, 2 insertions(+), 8 deletions(-)
> > 
> > diff --git a/net/mptcp/subflow.c b/net/mptcp/subflow.c
> > index fdc5aecab897..d91d817b8779 100644
> > --- a/net/mptcp/subflow.c
> > +++ b/net/mptcp/subflow.c
> > @@ -143,14 +143,8 @@ static struct sock *subflow_syn_recv_sock(const struct sock *sk,
> > 
> > 	pr_debug("listener=%p, req=%p, conn=%p", listener, req, listener->conn);
> > 
> > -	if (subflow_req->mp_capable) {
> > -		opt_rx.mptcp.mp_capable = 0;
> > -		mptcp_get_options(skb, &opt_rx);
> > -		if (!opt_rx.mptcp.mp_capable ||
> > -		    subflow_req->local_key != opt_rx.mptcp.rcvr_key ||
> > -		    subflow_req->remote_key != opt_rx.mptcp.sndr_key)
> > -			return NULL;
> > -	} else if (subflow_req->mp_join) {
> > +	/* if the sk is MP_CAPABLE, we already received the client key */
> > +	if (!subflow_req->mp_capable && subflow_req->mp_join) {
> > 		opt_rx.mptcp.mp_join = 0;
> > 		mptcp_get_options(skb, &opt_rx);
> > 		if (!opt_rx.mptcp.mp_join || token_join_valid(req, &opt_rx))
> > -- 
> > 2.20.1
> > 
> > _______________________________________________
> > mptcp mailing list
> > mptcp(a)lists.01.org
> > https://lists.01.org/mailman/listinfo/mptcp
> 
> _______________________________________________
> mptcp mailing list
> mptcp(a)lists.01.org
> https://lists.01.org/mailman/listinfo/mptcp

Re: [MPTCP] [PATCH] mptcp: avoid validating MP_CAPABLE keys on 3way HS handling

[Christoph Paasch]

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



> On Jul 17, 2019, at 10:57 AM, Paolo Abeni <pabeni(a)redhat.com> wrote:
> 
> The server side already stored the key on syn, and 3way HS ack
> reception is fragile: the ack can be lost, and the following
> data pkts processed instead.
> 
> This fixes the pending self-test issue in my testbed.
> 
> Suggested-by: Peter Krystad <peter.krystad(a)linux.intel.com>
> Suggested-by: Christoph Paasch <cpaasch(a)apple.com>
> Fixes: af550dd282ca ("mptcp: Create SUBFLOW socket for incoming connections")
> Signed-off-by: Paolo Abeni <pabeni(a)redhat.com>
> --
> I'm ok with squashing into the Fixes commit, but that will likely cause
> conflicts.
> 
> We can possible have [in the future] a similar issue with MP_JOIN, as we
> validate the token on 3way HS ack reception, and that is fragile ??!

without having easy access at the code right now:

How is the function token_join_valid() doing the validation? Because, the third ACK does not contain the token. Only the HMAC. So, upon reception of the third ACK of a new subflow the server has to validate the HMAC. The issue with a lost third ACK does not apply for new subflows because here MPTCP uses a 4-way handshake. The client won't send any data until it received the fourth ACK from the server.



Patch looks good by the way!

Cheers,
Christoph

> ---
> net/mptcp/subflow.c | 10 ++--------
> 1 file changed, 2 insertions(+), 8 deletions(-)
> 
> diff --git a/net/mptcp/subflow.c b/net/mptcp/subflow.c
> index fdc5aecab897..d91d817b8779 100644
> --- a/net/mptcp/subflow.c
> +++ b/net/mptcp/subflow.c
> @@ -143,14 +143,8 @@ static struct sock *subflow_syn_recv_sock(const struct sock *sk,
> 
> 	pr_debug("listener=%p, req=%p, conn=%p", listener, req, listener->conn);
> 
> -	if (subflow_req->mp_capable) {
> -		opt_rx.mptcp.mp_capable = 0;
> -		mptcp_get_options(skb, &opt_rx);
> -		if (!opt_rx.mptcp.mp_capable ||
> -		    subflow_req->local_key != opt_rx.mptcp.rcvr_key ||
> -		    subflow_req->remote_key != opt_rx.mptcp.sndr_key)
> -			return NULL;
> -	} else if (subflow_req->mp_join) {
> +	/* if the sk is MP_CAPABLE, we already received the client key */
> +	if (!subflow_req->mp_capable && subflow_req->mp_join) {
> 		opt_rx.mptcp.mp_join = 0;
> 		mptcp_get_options(skb, &opt_rx);
> 		if (!opt_rx.mptcp.mp_join || token_join_valid(req, &opt_rx))
> -- 
> 2.20.1
> 
> _______________________________________________
> mptcp mailing list
> mptcp(a)lists.01.org
> https://lists.01.org/mailman/listinfo/mptcp

[MPTCP] [PATCH] mptcp: avoid validating MP_CAPABLE keys on 3way HS handling

[Paolo Abeni]

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

The server side already stored the key on syn, and 3way HS ack
reception is fragile: the ack can be lost, and the following
data pkts processed instead.

This fixes the pending self-test issue in my testbed.

Suggested-by: Peter Krystad <peter.krystad(a)linux.intel.com>
Suggested-by: Christoph Paasch <cpaasch(a)apple.com>
Fixes: af550dd282ca ("mptcp: Create SUBFLOW socket for incoming connections")
Signed-off-by: Paolo Abeni <pabeni(a)redhat.com>
--
I'm ok with squashing into the Fixes commit, but that will likely cause
conflicts.

We can possible have [in the future] a similar issue with MP_JOIN, as we
validate the token on 3way HS ack reception, and that is fragile ??!
---
 net/mptcp/subflow.c | 10 ++--------
 1 file changed, 2 insertions(+), 8 deletions(-)

diff --git a/net/mptcp/subflow.c b/net/mptcp/subflow.c
index fdc5aecab897..d91d817b8779 100644
--- a/net/mptcp/subflow.c
+++ b/net/mptcp/subflow.c
@@ -143,14 +143,8 @@ static struct sock *subflow_syn_recv_sock(const struct sock *sk,
 
 	pr_debug("listener=%p, req=%p, conn=%p", listener, req, listener->conn);
 
-	if (subflow_req->mp_capable) {
-		opt_rx.mptcp.mp_capable = 0;
-		mptcp_get_options(skb, &opt_rx);
-		if (!opt_rx.mptcp.mp_capable ||
-		    subflow_req->local_key != opt_rx.mptcp.rcvr_key ||
-		    subflow_req->remote_key != opt_rx.mptcp.sndr_key)
-			return NULL;
-	} else if (subflow_req->mp_join) {
+	/* if the sk is MP_CAPABLE, we already received the client key */
+	if (!subflow_req->mp_capable && subflow_req->mp_join) {
 		opt_rx.mptcp.mp_join = 0;
 		mptcp_get_options(skb, &opt_rx);
 		if (!opt_rx.mptcp.mp_join || token_join_valid(req, &opt_rx))
-- 
2.20.1