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

[Mat Martineau <mathew.j.martineau at linux.intel.com>]

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