[MPTCP] Re: [PATCH mptcp-next] mptcp: use rightmost 64 bits in ADD_ADDR HMAC

[MPTCP] Re: [PATCH mptcp-next] mptcp: use rightmost 64 bits in ADD_ADDR HMAC

[Christoph Paasch]

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

On 18/05/20 - 15:29:16, Todd Malsbary wrote:
> This changes the HMAC used in the ADD_ADDR option from the leftmost 64
> bits to the rightmost 64 bits as described in RFC 8684, section 3.4.1.
> 
> Signed-off-by: Todd Malsbary <todd.malsbary(a)linux.intel.com>

Nice find!


Acked-by: Christoph Paasch <cpaasch(a)apple.com>

And I need to change out-of-tree MPTCP as well ;-)


Christoph

[MPTCP] Re: [PATCH mptcp-next] mptcp: use rightmost 64 bits in ADD_ADDR HMAC

[Mat Martineau]

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

On Wed, 20 May 2020, Christoph Paasch wrote:

> On 19/05/20 - 19:00:51, Todd Malsbary wrote:
>> Hi Christoph,
>>
>> On Tue, 2020-05-19 at 18:09 -0700, Christoph Paasch wrote:
>>> Hello,
>>>
>>> On 18/05/20 - 15:29:16, Todd Malsbary wrote:
>>>> This changes the HMAC used in the ADD_ADDR option from the leftmost 64
>>>> bits to the rightmost 64 bits as described in RFC 8684, section 3.4.1.
>>>>
>>>> Signed-off-by: Todd Malsbary <todd.malsbary(a)linux.intel.com>
>>>> ---
>>>>
>>>> I found this while adding support to packetdrill for the ADD_ADDR v1
>>>> option.  The computation I used for the HMAC in the pending
>>>> packetdrill work gave a different result than what was being computed
>>>> by the kernel.
>>>>
>>>> It appears that the usage of the leftmost bits has been in place since
>>>> at least 2016 in the out-of-tree code (commit de09a83186666).  Given
>>>> that, is the issue in the implementation or the RFC?
>>>>
>>>>  net/mptcp/options.c | 4 ++--
>>>>  1 file changed, 2 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/net/mptcp/options.c b/net/mptcp/options.c
>>>> index ece6f92cf7d1..0e11fb3d908f 100644
>>>> --- a/net/mptcp/options.c
>>>> +++ b/net/mptcp/options.c
>>>> @@ -550,7 +550,7 @@ static u64 add_addr_generate_hmac(u64 key1, u64 key2, u8 addr_id,
>>>>
>>>>  	mptcp_crypto_hmac_sha(key1, key2, msg, 7, hmac);
>>>>
>>>> -	return get_unaligned_be64(hmac);
>>>> +	return get_unaligned_be64(&hmac[MPTCP_ADDR_HMAC_LEN - sizeof(u64)]);
>>>
>>> as I was trying to do the same change in out-of-tree MPTCP, I realize
>>> something maybe a little bit late:
>>>
>>> I think MPTCP_ADDR_HMAC_LEN is the wrong variable, no?
>>>
>>> We want to take the rightmost bits of the SHA-256 output. Meaning, we need
>>> to get the right most out of mptcp_hashed_key in mptcp_crypto_hmac_sha().
>>>
>>> Because this latter function is already only copying the first 160bits to
>>> hashed_out - thus we are missing the remaining bits...
>>>
>>>
>>> Right?
>>>
>>>
>>
>> I wondered the same thing, but my read of the RFC is the rightmost 64
>> bits of the 160 bits.  From Section 3.4.1:
>>
>> "The Truncated HMAC parameter present in this option is the rightmost
>> 64 bits of an HMAC, negotiated and calculated in the same way as for
>> MP_JOIN as described in Section 3.2."
>>
>> Then back in Section 3.2 the HMAC is defined as:
>>
>> "This specification defines that HMAC as defined in [RFC2104] is
>> used, along with the SHA-256 hash algorithm [RFC6234], and that the
>> output is truncated to the leftmost 160 bits (20 octets). Due to
>> option space limitations, the HMAC included in the SYN/ACK is
>> truncated to the leftmost 64 bits, but this is acceptable, ..."
>>
>> I left the "Due to ..." sentence in there since that implies to me that
>> the leftmost/rightmost selection is taken after the 160 bit truncation,
>> but I'll admit that it could argued either way.  Thoughts?
>
> I would not have interpret it that way. I thought that the back-reference to
> Section 3.2 is more about the key-concatenation. The truncation to leftmost 160bits
> is to me just for the third ACK.

It is ambiguously worded to me, but the thing that registered to me after 
looking at more context is that the 160-bit version for the third ACK is 
also referred to as a "Truncated HMAC" (in RFC6864, it was just "HMAC" 
since it was SHA1 and did not require truncation). That makes me lean 
toward using the rightmost bits of the full SHA256 hash. It also makes it 
so the truncated HMAC in the ADD_ADDR can't be copied from the 160-bit 
truncation seen on the wire (if that's even worth anything).

--
Mat Martineau
Intel

[MPTCP] Re: [PATCH mptcp-next] mptcp: use rightmost 64 bits in ADD_ADDR HMAC

[Christoph Paasch]

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

On 19/05/20 - 19:00:51, Todd Malsbary wrote:
> Hi Christoph,
> 
> On Tue, 2020-05-19 at 18:09 -0700, Christoph Paasch wrote:
> > Hello,
> > 
> > On 18/05/20 - 15:29:16, Todd Malsbary wrote:
> > > This changes the HMAC used in the ADD_ADDR option from the leftmost 64
> > > bits to the rightmost 64 bits as described in RFC 8684, section 3.4.1.
> > > 
> > > Signed-off-by: Todd Malsbary <todd.malsbary(a)linux.intel.com>
> > > ---
> > > 
> > > I found this while adding support to packetdrill for the ADD_ADDR v1
> > > option.  The computation I used for the HMAC in the pending
> > > packetdrill work gave a different result than what was being computed
> > > by the kernel.
> > > 
> > > It appears that the usage of the leftmost bits has been in place since
> > > at least 2016 in the out-of-tree code (commit de09a83186666).  Given
> > > that, is the issue in the implementation or the RFC?
> > > 
> > >  net/mptcp/options.c | 4 ++--
> > >  1 file changed, 2 insertions(+), 2 deletions(-)
> > > 
> > > diff --git a/net/mptcp/options.c b/net/mptcp/options.c
> > > index ece6f92cf7d1..0e11fb3d908f 100644
> > > --- a/net/mptcp/options.c
> > > +++ b/net/mptcp/options.c
> > > @@ -550,7 +550,7 @@ static u64 add_addr_generate_hmac(u64 key1, u64 key2, u8 addr_id,
> > >  
> > >  	mptcp_crypto_hmac_sha(key1, key2, msg, 7, hmac);
> > >  
> > > -	return get_unaligned_be64(hmac);
> > > +	return get_unaligned_be64(&hmac[MPTCP_ADDR_HMAC_LEN - sizeof(u64)]);
> > 
> > as I was trying to do the same change in out-of-tree MPTCP, I realize
> > something maybe a little bit late:
> > 
> > I think MPTCP_ADDR_HMAC_LEN is the wrong variable, no?
> > 
> > We want to take the rightmost bits of the SHA-256 output. Meaning, we need
> > to get the right most out of mptcp_hashed_key in mptcp_crypto_hmac_sha().
> > 
> > Because this latter function is already only copying the first 160bits to
> > hashed_out - thus we are missing the remaining bits...
> > 
> > 
> > Right?
> > 
> > 
> 
> I wondered the same thing, but my read of the RFC is the rightmost 64
> bits of the 160 bits.  From Section 3.4.1:
> 
> "The Truncated HMAC parameter present in this option is the rightmost
> 64 bits of an HMAC, negotiated and calculated in the same way as for
> MP_JOIN as described in Section 3.2."
> 
> Then back in Section 3.2 the HMAC is defined as:
> 
> "This specification defines that HMAC as defined in [RFC2104] is
> used, along with the SHA-256 hash algorithm [RFC6234], and that the
> output is truncated to the leftmost 160 bits (20 octets). Due to
> option space limitations, the HMAC included in the SYN/ACK is
> truncated to the leftmost 64 bits, but this is acceptable, ..."
> 
> I left the "Due to ..." sentence in there since that implies to me that
> the leftmost/rightmost selection is taken after the 160 bit truncation,
> but I'll admit that it could argued either way.  Thoughts?

I would not have interpret it that way. I thought that the back-reference to
Section 3.2 is more about the key-concatenation. The truncation to leftmost 160bits
is to me just for the third ACK.


Christoph

[MPTCP] Re: [PATCH mptcp-next] mptcp: use rightmost 64 bits in ADD_ADDR HMAC

[Todd Malsbary]

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

Hi Christoph,

On Tue, 2020-05-19 at 18:09 -0700, Christoph Paasch wrote:
> Hello,
> 
> On 18/05/20 - 15:29:16, Todd Malsbary wrote:
> > This changes the HMAC used in the ADD_ADDR option from the leftmost 64
> > bits to the rightmost 64 bits as described in RFC 8684, section 3.4.1.
> > 
> > Signed-off-by: Todd Malsbary <todd.malsbary(a)linux.intel.com>
> > ---
> > 
> > I found this while adding support to packetdrill for the ADD_ADDR v1
> > option.  The computation I used for the HMAC in the pending
> > packetdrill work gave a different result than what was being computed
> > by the kernel.
> > 
> > It appears that the usage of the leftmost bits has been in place since
> > at least 2016 in the out-of-tree code (commit de09a83186666).  Given
> > that, is the issue in the implementation or the RFC?
> > 
> >  net/mptcp/options.c | 4 ++--
> >  1 file changed, 2 insertions(+), 2 deletions(-)
> > 
> > diff --git a/net/mptcp/options.c b/net/mptcp/options.c
> > index ece6f92cf7d1..0e11fb3d908f 100644
> > --- a/net/mptcp/options.c
> > +++ b/net/mptcp/options.c
> > @@ -550,7 +550,7 @@ static u64 add_addr_generate_hmac(u64 key1, u64 key2, u8 addr_id,
> >  
> >  	mptcp_crypto_hmac_sha(key1, key2, msg, 7, hmac);
> >  
> > -	return get_unaligned_be64(hmac);
> > +	return get_unaligned_be64(&hmac[MPTCP_ADDR_HMAC_LEN - sizeof(u64)]);
> 
> as I was trying to do the same change in out-of-tree MPTCP, I realize
> something maybe a little bit late:
> 
> I think MPTCP_ADDR_HMAC_LEN is the wrong variable, no?
> 
> We want to take the rightmost bits of the SHA-256 output. Meaning, we need
> to get the right most out of mptcp_hashed_key in mptcp_crypto_hmac_sha().
> 
> Because this latter function is already only copying the first 160bits to
> hashed_out - thus we are missing the remaining bits...
> 
> 
> Right?
> 
> 

I wondered the same thing, but my read of the RFC is the rightmost 64
bits of the 160 bits.  From Section 3.4.1:

"The Truncated HMAC parameter present in this option is the rightmost
64 bits of an HMAC, negotiated and calculated in the same way as for
MP_JOIN as described in Section 3.2."

Then back in Section 3.2 the HMAC is defined as:

"This specification defines that HMAC as defined in [RFC2104] is
used, along with the SHA-256 hash algorithm [RFC6234], and that the
output is truncated to the leftmost 160 bits (20 octets). Due to
option space limitations, the HMAC included in the SYN/ACK is
truncated to the leftmost 64 bits, but this is acceptable, ..."

I left the "Due to ..." sentence in there since that implies to me that
the leftmost/rightmost selection is taken after the 160 bit truncation,
but I'll admit that it could argued either way.  Thoughts?

-Todd

[MPTCP] Re: [PATCH mptcp-next] mptcp: use rightmost 64 bits in ADD_ADDR HMAC

[Christoph Paasch]

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

Hello,

On 18/05/20 - 15:29:16, Todd Malsbary wrote:
> This changes the HMAC used in the ADD_ADDR option from the leftmost 64
> bits to the rightmost 64 bits as described in RFC 8684, section 3.4.1.
> 
> Signed-off-by: Todd Malsbary <todd.malsbary(a)linux.intel.com>
> ---
> 
> I found this while adding support to packetdrill for the ADD_ADDR v1
> option.  The computation I used for the HMAC in the pending
> packetdrill work gave a different result than what was being computed
> by the kernel.
> 
> It appears that the usage of the leftmost bits has been in place since
> at least 2016 in the out-of-tree code (commit de09a83186666).  Given
> that, is the issue in the implementation or the RFC?
> 
>  net/mptcp/options.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/net/mptcp/options.c b/net/mptcp/options.c
> index ece6f92cf7d1..0e11fb3d908f 100644
> --- a/net/mptcp/options.c
> +++ b/net/mptcp/options.c
> @@ -550,7 +550,7 @@ static u64 add_addr_generate_hmac(u64 key1, u64 key2, u8 addr_id,
>  
>  	mptcp_crypto_hmac_sha(key1, key2, msg, 7, hmac);
>  
> -	return get_unaligned_be64(hmac);
> +	return get_unaligned_be64(&hmac[MPTCP_ADDR_HMAC_LEN - sizeof(u64)]);

as I was trying to do the same change in out-of-tree MPTCP, I realize
something maybe a little bit late:

I think MPTCP_ADDR_HMAC_LEN is the wrong variable, no?

We want to take the rightmost bits of the SHA-256 output. Meaning, we need
to get the right most out of mptcp_hashed_key in mptcp_crypto_hmac_sha().

Because this latter function is already only copying the first 160bits to
hashed_out - thus we are missing the remaining bits...


Right?


Christoph



>  }
>  
>  #if IS_ENABLED(CONFIG_MPTCP_IPV6)
> @@ -567,7 +567,7 @@ static u64 add_addr6_generate_hmac(u64 key1, u64 key2, u8 addr_id,
>  
>  	mptcp_crypto_hmac_sha(key1, key2, msg, 19, hmac);
>  
> -	return get_unaligned_be64(hmac);
> +	return get_unaligned_be64(&hmac[MPTCP_ADDR_HMAC_LEN - sizeof(u64)]);
>  }
>  #endif
>  
> -- 
> 2.25.4
> _______________________________________________
> mptcp mailing list -- mptcp(a)lists.01.org
> To unsubscribe send an email to mptcp-leave(a)lists.01.org

[MPTCP] Re: [PATCH mptcp-next] mptcp: use rightmost 64 bits in ADD_ADDR HMAC

[Matthieu Baerts]

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

Hi Todd,

On 19/05/2020 18:00, Todd Malsbary wrote:
> Hi Matthieu,
> 
> On Tue, 2020-05-19 at 17:11 +0200, Matthieu Baerts wrote:
>> Hi Todd,
>>
>> On 19/05/2020 02:30, Christoph Paasch wrote:
>>> On 18/05/20 - 15:29:16, Todd Malsbary wrote:
>>>> This changes the HMAC used in the ADD_ADDR option from the leftmost 64
>>>> bits to the rightmost 64 bits as described in RFC 8684, section 3.4.1.
>>>>
>>>> Signed-off-by: Todd Malsbary <todd.malsbary(a)linux.intel.com>
>>>
>>> Nice find!
>>
>> Yes, good catch!
>>
>> Reviewed-by: Matthieu Baerts <matthieu.baerts(a)tessares.net>
>>
>> I guess we should send that to -net branch, ideally before the release
>> of v5.7, not to introduce incompatibilities between versions. What do
>> you think?
>>
> 
> Sounds good to me.  Just to double check on the process: send this
> patch out again to the netdev list with [PATCH net] as the prefix, and
> cc this list?

(We continued the discussion on IRC).
In short: yes but "Fixes" tag is required.

Cheers,
Matt
-- 
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

[MPTCP] Re: [PATCH mptcp-next] mptcp: use rightmost 64 bits in ADD_ADDR HMAC

[Todd Malsbary]

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

Hi Matthieu,

On Tue, 2020-05-19 at 17:11 +0200, Matthieu Baerts wrote:
> Hi Todd,
> 
> On 19/05/2020 02:30, Christoph Paasch wrote:
> > On 18/05/20 - 15:29:16, Todd Malsbary wrote:
> > > This changes the HMAC used in the ADD_ADDR option from the leftmost 64
> > > bits to the rightmost 64 bits as described in RFC 8684, section 3.4.1.
> > > 
> > > Signed-off-by: Todd Malsbary <todd.malsbary(a)linux.intel.com>
> > 
> > Nice find!
> 
> Yes, good catch!
> 
> Reviewed-by: Matthieu Baerts <matthieu.baerts(a)tessares.net>
> 
> I guess we should send that to -net branch, ideally before the release 
> of v5.7, not to introduce incompatibilities between versions. What do 
> you think?
> 

Sounds good to me.  Just to double check on the process: send this
patch out again to the netdev list with [PATCH net] as the prefix, and
cc this list?

Thanks,
-Todd

[MPTCP] Re: [PATCH mptcp-next] mptcp: use rightmost 64 bits in ADD_ADDR HMAC

[Matthieu Baerts]

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

Hi Todd,

On 19/05/2020 02:30, Christoph Paasch wrote:
> On 18/05/20 - 15:29:16, Todd Malsbary wrote:
>> This changes the HMAC used in the ADD_ADDR option from the leftmost 64
>> bits to the rightmost 64 bits as described in RFC 8684, section 3.4.1.
>>
>> Signed-off-by: Todd Malsbary <todd.malsbary(a)linux.intel.com>
> 
> Nice find!

Yes, good catch!

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

I guess we should send that to -net branch, ideally before the release 
of v5.7, not to introduce incompatibilities between versions. What do 
you think?

 > I found this while adding support to packetdrill for the ADD_ADDR v1
 > option.  The computation I used for the HMAC in the pending
 > packetdrill work gave a different result than what was being computed
 > by the kernel.

Good to hear you are looking at adding ADD_ADDRv1 support to packetdrill! :)

Cheers,
Matt
-- 
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