<div dir="ltr">This had been fixed, see <a href="https://github.com/BLAKE2/BLAKE2/commit/a18d2b03fb0df1458c447d488816b51b1164c006">https://github.com/BLAKE2/BLAKE2/commit/a18d2b03fb0df1458c447d488816b51b1164c006</a></div><br><div class="gmail_quote"><div dir="ltr">On Wed, Oct 14, 2015 at 1:05 PM Jean-Philippe Aumasson <<a href="mailto:jeanphilippe.aumasson@gmail.com">jeanphilippe.aumasson@gmail.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr">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).<div><br></div><div>We'll update our code.</div></div><div dir="ltr"><div><br><div><br><div class="gmail_quote"><div dir="ltr">On Wed, Oct 14, 2015 at 12:29 PM Jason A. Donenfeld <<a href="mailto:Jason@zx2c4.com" target="_blank">Jason@zx2c4.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">Hi JP,<br>
<br>
Since you're on the noise list now, and we've (I've?) been talking<br>
about using BLAKE2 as a substitute for HMAC in the KDF, here's a<br>
considerable concern with noise:<br>
<br>
After various operations have been done, a function called Split() is<br>
called to begin a noise transport session. This function is currently<br>
defined as:<br>
<br>
    Split(): Creates two child CipherState objects by calling HKDF(ck, empty)<br>
    where empty is a zero-length byte sequence. The first child's k is<br>
set to the<br>
    first output from HKDF(), and the second child's k is set to the<br>
second output<br>
    from HKDF(). Both children's n value is set to zero. Both children<br>
are returned.<br>
<br>
Calling HKDF(ck, [empty]) might have some issues. Keeping with the<br>
HKDF extract-then-expand construction, but replacing HMAC with keyed<br>
BLAKE2, this amounts to:<br>
<br>
    temp_key = BLAKE2(chaining_key, [empty])<br>
    output1 = BLAKE2(temp_key, 0x01)<br>
    output2 = BLAKE2(temp_key, output1 || 0x02)<br>
    return (output1, output2)<br>
<br>
The problematic line is the first one:<br>
<br>
    temp_key = BLAKE2(chaining_key, [empty])<br>
<br>
Examining the reference implementation shows several checks at the<br>
beginning of the function:<br>
<br>
int blake2b( uint8_t *out, const void *in, const void *key, const<br>
uint8_t outlen, const uint64_t inlen, uint8_t keylen )<br>
{<br>
  blake2b_state S[1];<br>
  /* Verify parameters */<br>
  if ( NULL == in ) return -1;<br>
  if ( NULL == out ) return -1;<br>
  if( NULL == key ) keylen = 0;<br>
  if( keylen > 0 )<br>
    if( blake2b_init_key( S, outlen, key, keylen ) < 0 ) return -1;<br>
  else<br>
    if( blake2b_init( S, outlen ) < 0 ) return -1;<br>
  blake2b_update( S, ( const uint8_t * )in, inlen );<br>
  blake2b_final( S, out, outlen );<br>
  return 0;<br>
}<br>
<br>
Notably, the problem is with:<br>
<br>
  if ( NULL == in ) return -1;<br>
<br>
If there is an empty input, error out, it says. This would make BLAKE2<br>
unsuitable for the function above. It is possible, though, that this<br>
is an implementation error for keyed BLAKE2. The reason is that the<br>
call to "blake2b_init_key( S, outlen, key, keylen )" results in<br>
calling "blake2b_update" on the key. Thus, the only difference between<br>
calling BLAKE2 as a standard hash function and calling BLAKE2 as a<br>
keyed hash function with no data would be in the latter, the param<br>
block contains the keysize. Therefore I suspect the problematic line<br>
above would be better replaced with:<br>
<br>
  if ( NULL == in && ( NULL == key || 0 == keylen ) ) return -1;<br>
<br>
Would this be an acceptable modification? Or would it, in fact, break<br>
important assumptions underlying BLAKE2?<br>
<br>
Thanks,<br>
Jason<br>
</blockquote></div></div></div></div></blockquote></div>