xiaoxiang781216 edited a comment on pull request #2488:
URL: https://github.com/apache/incubator-nuttx/pull/2488#issuecomment-740693509
> It is good it failed to compile. That allows programmer to notice, that
maybe something is different for NuttX. Users should not expect that Linux man
pages describe NuttX APIs. This is especially important for security code. I
have seen at least one TLS implementation that resorts back to using rand() if
getrandom() fails. It is very good that that kind of insecure code fails to
compile for NuttX!
>
But now you directly ignore the error at all, could you tell me how can the
caller handle these error case securely?
1. nxsem_wait_uninterruptible return fail
2. user call getrandom too early and then no enough entropy is collected yet
> It is unfortunate the NuttX function is called getrandom(). I originally
wanted it to have a distinctive name, but it was changed to getrandom() by
other people.
>
> This commit just introduces more confusion to system as it adds things
that do not work or are contradicting existing code, like this piece in
comments:
>
> `GRND_NONBLOCK Don't block and return EAGAIN instead`
>
If some flags don't make sense, we can remove it, but the prototype should
keep consistently with the practice.
> NuttX getrandom() cannot return EGAIN and it cannot block (for an
indefinite time at least, as the semaphore there can not really cause
starvation). Comment is incorrect, because it hints that getrandom() blocks by
default. On Linux it does, which is just another design failure of the Linux
version. Why should we ever block for random numbers, or even have a blocking
API, since we already have the ability to generate nearly infinite sequences of
them without blocking?
>
No, from the spec:
```
GRND_NONBLOCK
By default, when reading from the random source, getrandom()
blocks if no random bytes are available, and when reading from
the urandom source, it blocks if the entropy pool has not yet
been initialized. If the GRND_NONBLOCK flag is set, then
getrandom() does not block in these cases, but instead
immediately returns -1 with errno set to EAGAIN.
```
If the entropy isn't collected enough, we should block and wait the enough
entropy by default, or return -1 with errno set to EAGAIN if user pass
GRND_NONBLOCK. But your implementation just log a warning:
```
static void rng_buf_internal(FAR void *bytes, size_t nbytes)
{
if (!g_rng.output_initialized)
{
if (g_rng.rd_newentr < MIN_SEED_NEW_ENTROPY_WORDS)
{
cryptwarn("Entropy pool RNG initialized with very low entropy. "
" Consider implementing CONFIG_BOARD_INITRNGSEED!\n");
}
rng_reseed();
}
else if (g_rng.rd_newentr >= MAX_SEED_NEW_ENTROPY_WORDS)
{
/* Initial entropy is low. Reseed when we have accumulated more. */
rng_reseed();
}
else if (g_rng.blake2xs.out_node_offset == UINT32_MAX)
{
/* Maximum BLAKE2Xs output reached (2^32-1 output blocks, maximum 128
* GiB bytes), reseed.
*/
rng_reseed();
}
```
> > getrandom is designed to can be implemented by the hardware crypto
accelerator, nobody can make 100% guarantee that the hardware will always work
as expect(especially some accelerator connect to CPU through SPI, SDIO, USB
bus). Now we have no way to report the error with your prototype?
>
> getrandom() is not designed specifically for hardware crypto accelerators.
We use it in real products with MCUs that lack hardware RNG. It only needs
enough initial entropy for one cryptographical key (unless generate huge
amounts, several Gb, of output, then it must re-key itself) and it can get that
from sensors, interrupt timings, factory production line etc.
>
But how you tell caller if there isn't enough entropy has been collected
yet? or the external hardware RNG can't access because I2C/SPI/USB bus is
broken.
> To better understand these issues, please try this exercise: 1) implement
Linux getrandom() using OpenBSD's arc4random_buf() function 2) implement
arc4random_buf() using Linux's getrandom(). Part 1) is trivial, but for 2) one
has an insurmountable problem, what to do if getrandom() fails (for any reason)?
It's arc4random_buf design problem: it doesn't return the error code. We
should avoid to use this function, it is wrong to drop getrandom return value.
If you look at how NuttX implementation now, there are many places the random
generatation isn't really random:
```
void getrandom(FAR void *bytes, size_t nbytes)
{
int ret;
ret = nxsem_wait_uninterruptible(&g_rng.rd_sem);
if (ret >= 0)
{
rng_buf_internal(bytes, nbytes);
nxsem_post(&g_rng.rd_sem);
}
}
static void rng_buf_internal(FAR void *bytes, size_t nbytes)
{
if (!g_rng.output_initialized)
{
if (g_rng.rd_newentr < MIN_SEED_NEW_ENTROPY_WORDS)
{
cryptwarn("Entropy pool RNG initialized with very low entropy. "
" Consider implementing CONFIG_BOARD_INITRNGSEED!\n");
}
rng_reseed();
}
else if (g_rng.rd_newentr >= MAX_SEED_NEW_ENTROPY_WORDS)
{
/* Initial entropy is low. Reseed when we have accumulated more. */
rng_reseed();
}
else if (g_rng.blake2xs.out_node_offset == UINT32_MAX)
{
/* Maximum BLAKE2Xs output reached (2^32-1 output blocks, maximum 128
* GiB bytes), reseed.
*/
rng_reseed();
}
```
Do you think it's more secure to blindly ignore the error?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
