[noise] BLAKE2 as extractor with empty data

[Jason A. Donenfeld]

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