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

[Christoph Paasch <cpaasch at apple.com>]

[-- 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