[noise] BLAKE2 as extractor with empty data

[Jean-Philippe Aumasson]

This had been fixed, see
https://github.com/BLAKE2/BLAKE2/commit/a18d2b03fb0df1458c447d488816b51b1164c006

On Wed, Oct 14, 2015 at 1:05 PM Jean-Philippe Aumasson <
jeanphilippe.aumasson at gmail.com> wrote:

> Thanks Jason for reporting this. We should allow "in" to be NULL if inlen
> is zero. It's ok allow (inlen==0) && (keylen==0) though (we should err on
> NULL pointers only if their length parameters is nonzero).
>
> We'll update our code.
>
>
> On Wed, Oct 14, 2015 at 12:29 PM Jason A. Donenfeld <Jason at zx2c4.com>
> wrote:
>
>> Hi JP,
>>
>> Since you're on the noise list now, and we've (I've?) been talking
>> about using BLAKE2 as a substitute for HMAC in the KDF, here's a
>> considerable concern with noise:
>>
>> After various operations have been done, a function called Split() is
>> called to begin a noise transport session. This function is currently
>> defined as:
>>
>>     Split(): Creates two child CipherState objects by calling HKDF(ck,
>> empty)
>>     where empty is a zero-length byte sequence. The first child's k is
>> set to the
>>     first output from HKDF(), and the second child's k is set to the
>> second output
>>     from HKDF(). Both children's n value is set to zero. Both children
>> are returned.
>>
>> Calling HKDF(ck, [empty]) might have some issues. Keeping with the
>> HKDF extract-then-expand construction, but replacing HMAC with keyed
>> BLAKE2, this amounts to:
>>
>>     temp_key = BLAKE2(chaining_key, [empty])
>>     output1 = BLAKE2(temp_key, 0x01)
>>     output2 = BLAKE2(temp_key, output1 || 0x02)
>>     return (output1, output2)
>>
>> The problematic line is the first one:
>>
>>     temp_key = BLAKE2(chaining_key, [empty])
>>
>> Examining the reference implementation shows several checks at the
>> beginning of the function:
>>
>> int blake2b( uint8_t *out, const void *in, const void *key, const
>> uint8_t outlen, const uint64_t inlen, uint8_t keylen )
>> {
>>   blake2b_state S[1];
>>   /* Verify parameters */
>>   if ( NULL == in ) return -1;
>>   if ( NULL == out ) return -1;
>>   if( NULL == key ) keylen = 0;
>>   if( keylen > 0 )
>>     if( blake2b_init_key( S, outlen, key, keylen ) < 0 ) return -1;
>>   else
>>     if( blake2b_init( S, outlen ) < 0 ) return -1;
>>   blake2b_update( S, ( const uint8_t * )in, inlen );
>>   blake2b_final( S, out, outlen );
>>   return 0;
>> }
>>
>> Notably, the problem is with:
>>
>>   if ( NULL == in ) return -1;
>>
>> If there is an empty input, error out, it says. This would make BLAKE2
>> unsuitable for the function above. It is possible, though, that this
>> is an implementation error for keyed BLAKE2. The reason is that the
>> call to "blake2b_init_key( S, outlen, key, keylen )" results in
>> calling "blake2b_update" on the key. Thus, the only difference between
>> calling BLAKE2 as a standard hash function and calling BLAKE2 as a
>> keyed hash function with no data would be in the latter, the param
>> block contains the keysize. Therefore I suspect the problematic line
>> above would be better replaced with:
>>
>>   if ( NULL == in && ( NULL == key || 0 == keylen ) ) return -1;
>>
>> Would this be an acceptable modification? Or would it, in fact, break
>> important assumptions underlying BLAKE2?
>>
>> Thanks,
>> Jason
>>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://moderncrypto.org/mail-archive/noise/attachments/20151014/6004cff0/attachment.html>