Re: [PATCH v5 01/10] stdlib: Add arc4random, arc4random_buf, and arc4random_uniform (BZ #4417)

[Adhemerval Zanella <adhemerval.zanella@linaro.org>]

On 17/05/2022 06:10, Yann Droneaud wrote:
>> +
>> +static inline void
>> +chacha20_block (uint32_t *state, uint32_t *stream)
>> +{
>> +  uint32_t x[CHACHA20_STATE_LEN];
>> +  memcpy (x, state, sizeof x);
>> +
>> +  for (int i = 0; i < 20; i += 2)
>> +    {
>> +      QROUND (x[0], x[4], x[8],  x[12]);
>> +      QROUND (x[1], x[5], x[9],  x[13]);
>> +      QROUND (x[2], x[6], x[10], x[14]);
>> +      QROUND (x[3], x[7], x[11], x[15]);
>> +
>> +      QROUND (x[0], x[5], x[10], x[15]);
>> +      QROUND (x[1], x[6], x[11], x[12]);
>> +      QROUND (x[2], x[7], x[8],  x[13]);
>> +      QROUND (x[3], x[4], x[9],  x[14]);
>> +    }
>> +
>> +  /* Unroll the loop a bit.  */
>> +  for (int i = 0; i < CHACHA20_BLOCK_WORDS / 4; i++)
>> +    {
>> +      stream[i*4+0] = set_state (x[i*4+0] + state[i*4+0]);
>> +      stream[i*4+1] = set_state (x[i*4+1] + state[i*4+1]);
>> +      stream[i*4+2] = set_state (x[i*4+2] + state[i*4+2]);
>> +      stream[i*4+3] = set_state (x[i*4+3] + state[i*4+3]);
>> +    }
>> +
>> +  state[12]++;
> 
> 
> I tempted to ask to clear x[] from the stack as this information could be used to recover the state from the stream.
> 
> (In OpenBSD implementation, x's are variables instead, thus they can be pushed on the stack, then there's no way to clear them).

I think I can use variables instead of an array, it seems to show better performance
and it would avoid calling explicit_bzero.

> 
> 
>> +}
>> +
>> +static void
>> +chacha20_crypt (uint32_t *state, uint8_t *dst, const uint8_t *src,
>> +        size_t bytes)
>> +{
>> +  uint32_t stream[CHACHA20_BLOCK_WORDS];
>> +
>> +  while (bytes >= CHACHA20_BLOCK_SIZE)
>> +    {
>> +      chacha20_block (state, stream);
>> +#ifdef CHACHA20_XOR_FINAL
>> +      for (int i = 0; i < CHACHA20_BLOCK_WORDS; i++)
>> +    stream[i] ^= read_unaligned_32 (&src[i * sizeof (uint32_t)]);
>> +#endif
>> +      memcpy (dst, stream, CHACHA20_BLOCK_SIZE);
>> +      bytes -= CHACHA20_BLOCK_SIZE;
>> +      dst += CHACHA20_BLOCK_SIZE;
>> +      src += CHACHA20_BLOCK_SIZE;
>> +    }
>> +  if (bytes != 0)
>> +    {
>> +      chacha20_block (state, stream);
>> +#ifdef CHACHA20_XOR_FINAL
>> +      for (int i = 0; i < CHACHA20_BLOCK_WORDS; i++)
>> +    stream[i] ^= read_unaligned_32 (&src[i * sizeof (uint32_t)]);
>> +#endif
>> +      memcpy (dst, stream, bytes);
>> +    }
>> +}
> 
> 
> Same here, I would clear stream[] as it would leak up to 64 bytes of the random stream on the stack.
> 
> (In OpenBSD implementation, the stream is written into the destination buffer, using a temporary buffer only for the last block if not a multiple of ChaCha20 block size long, but this temporary buffer is not cleared either).

I think we can make chacha20_block write direct on stream and make
the final block issue explicit_bzero (it wont' be used since 
arc4random internal buffer is multiple of CHACHA20_BLOCK_SIZE).