You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@nuttx.apache.org by GitBox <gi...@apache.org> on 2020/12/07 06:49:24 UTC
[GitHub] [incubator-nuttx] anchao opened a new pull request #2488: libs/libc: correct the getrandom(2) prototype
anchao opened a new pull request #2488:
URL: https://github.com/apache/incubator-nuttx/pull/2488
## Summary
libs/libc: correct the getrandom(2) prototype
libs/libc/getrandom(2): add ORed bit mask definition
## Impact
getrandom(2) prototype
Reference here:
https://man7.org/linux/man-pages/man2/getrandom.2.html
## Testing
getrandom test
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [incubator-nuttx] xiaoxiang781216 edited a comment on pull request #2488: libs/libc: correct the getrandom(2) prototype
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 edited a comment on pull request #2488:
URL: https://github.com/apache/incubator-nuttx/pull/2488#issuecomment-740830262
> > 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
> > ```
>
> Implementation bug, corrected in #2497
>
> > ```
> > 2. user call getrandom too early and then no enough entropy is collected yet
> > ```
>
> Not a problem in practise. We fill most of otherwise empty internal flash space with good quality random bytes during device production and then can activate flash read-out-protection. It is also easy to store gathered entropy across resets (internal flash, BBSRAM) and reseed back to entropy pool during board initialization phase.
This is your implementation detail, but our factory process is totally different.
>
> > 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");
> > ```
>
> Better to do as the warning says :) You can always change this to PANIC() if want to be really sure we never run past this point with insufficient entropy.
>
It's a runtime error, not a programming error at all, we should return errror here and let the caller decide what best action should to take.
> > 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.
>
> Can any real device do anything other than PANIC() if some of buses or other HW it really needs are broken?
>
I don't think so, it's depends on the usecase. The caller may has more reasonable choice than PANIC(e.g. refuse the security connection).
> > It's arc4random_buf design problem: it doesn't return the error code. We should avoid to use this function, but it is wrong to drop getrandom return value.
>
> arc4random_buf() is _very carefully designed_ by people who know how to design cryptographical software. Linux and Glibc maintainers botched this with their overly complex and error prone interface. arc4random functions were deliberately designed to be easy to use to avoid crypto failures due to misuse by their caller. Of course this places burden to implementator to get it right. Note that we have other APIs that can never fail, getpid(), getuid(), longjmp(), pthread_getspecific() etc.
>
> It seems Austin Group commenters got confused about this, interesting discussion though: https://www.austingroupbugs.net/view.php?id=859
If you think arc4random_buf is better design, please remove getrandom(wrong prototype) and implement arc4random_buf instead. But since it's impossible to return error code for arc4random_buf, how to handle the low entropy case? PANIC, I don't think it's good choice as note before.
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [incubator-nuttx] juniskane commented on pull request #2488: libs/libc: correct the getrandom(2) prototype
Posted by GitBox <gi...@apache.org>.
juniskane commented on pull request #2488:
URL: https://github.com/apache/incubator-nuttx/pull/2488#issuecomment-740798274
> 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
Implementation bug, corrected in https://github.com/apache/incubator-nuttx/pull/2497
> 2. user call getrandom too early and then no enough entropy is collected yet
Not a problem in practise. We fill most of otherwise empty internal flash space with good quality random bytes during device production and then can activate flash read-out-protection. It is also easy to store gathered entropy across resets (internal flash, BBSRAM) and reseed back to entropy pool during board initialization phase.
> 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");
Better to do as the warning says :) You can always change this to PANIC() if want to be really sure we never run past this point with insufficient entropy.
> 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.
Can any real device do anything other than PANIC() if some of buses or other HW it really needs are broken?
> It's arc4random_buf design problem: it doesn't return the error code. We should avoid to use this function, but it is wrong to drop getrandom return value.
arc4random_buf() is *very carefully designed* by people who know how to design cryptographical software. Linux and Glibc maintainers botched this with their overly complex and error prone interface. arc4random functions were deliberately designed to be easy to use to avoid crypto failures due to misuse by their caller. Of course this places burden to implementator to get it right. Note that we have other APIs that can never fail, getpid(), getuid(), longjmp(), pthread_getspecific() etc.
It seems Austin Group commenters got confused about this, interesting discussion though: https://www.austingroupbugs.net/view.php?id=859
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [incubator-nuttx] juniskane commented on pull request #2488: libs/libc: correct the getrandom(2) prototype
Posted by GitBox <gi...@apache.org>.
juniskane commented on pull request #2488:
URL: https://github.com/apache/incubator-nuttx/pull/2488#issuecomment-743118817
> Because the current implementation only support the nonblocking behaviour, the right thing is checking GRND_NONBLOCK flag and return return error directly if user forget to set it.
No, the correct thing is to ignore GRND_NONBLOCK as it is not needed in this implementation. getrandom() with flags == 0 should not return any spurious errors if it can complete the request. I think with current implementation, flags can be ignore completely for now. But since you and anchao want to bring the badly designed Linux version to NuttX, I'd expect you to be mindful about little details like this. Have you checked, if the FreeBSD and Linux versions are different from each other in some subtle way? We cannot completely ignore Solaris precedent (and others who might have implemented getrandom()), as people might port Solaris code to NuttX as well.
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [incubator-nuttx] xiaoxiang781216 edited a comment on pull request #2488: crypto/arc4random: rename getrandom to arc4random_buf
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 edited a comment on pull request #2488:
URL: https://github.com/apache/incubator-nuttx/pull/2488#issuecomment-743764060
> > Have you checked, if the FreeBSD and Linux versions are different from each other in some subtle way? We cannot completely ignore Solaris precedent (and others who might have implemented getrandom()), as people might port Solaris code to NuttX as well.
>
> See my thoughts above about introducing some kind of CONFIG_COMPAT_LINUX, CONFIG_COMPAT_BSD options. The idea is: if none of these options are activated, NuttX will only be Posix. If CONFIG_COMPAT_LINUX, that activates some linux compatibility interfaces. If there is a question which version of an interface should be used, the CONFIG_COMPAT_* options can be used for disambiguation. The disadvantage of this idea is more difficult maintenance. The advantage is that if people want some Linux or BSD interfaces to make it easier to port programs from those OS, the interfaces can be added, but are optional. So we don't violate the Inviolables :-)
libc implementer already use _XXX_SOURCE to enable the non standard functions:
https://ftp.gnu.org/old-gnu/Manuals/glibc-2.2.3/html_node/libc_13.html
it's better to follow this practice to improve the compatibility.
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [incubator-nuttx] xiaoxiang781216 commented on pull request #2488: libs/libc: correct the getrandom(2) prototype
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on pull request #2488:
URL: https://github.com/apache/incubator-nuttx/pull/2488#issuecomment-740830262
> > 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
> > ```
>
> Implementation bug, corrected in #2497
>
> > ```
> > 2. user call getrandom too early and then no enough entropy is collected yet
> > ```
>
> Not a problem in practise. We fill most of otherwise empty internal flash space with good quality random bytes during device production and then can activate flash read-out-protection. It is also easy to store gathered entropy across resets (internal flash, BBSRAM) and reseed back to entropy pool during board initialization phase.
This is your implementation detail, but our factory process is totally different.
>
> > 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");
> > ```
>
> Better to do as the warning says :) You can always change this to PANIC() if want to be really sure we never run past this point with insufficient entropy.
>
It's a runtime error, not a programming error at all, we should return errror here and let the caller decide what best action should to take.
> > 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.
>
> Can any real device do anything other than PANIC() if some of buses or other HW it really needs are broken?
>
I don't think so, it's depends on the usecase. The caller may has more reasonable choice than PANIC(e.g. refuse the security connection).
> > It's arc4random_buf design problem: it doesn't return the error code. We should avoid to use this function, but it is wrong to drop getrandom return value.
>
> arc4random_buf() is _very carefully designed_ by people who know how to design cryptographical software. Linux and Glibc maintainers botched this with their overly complex and error prone interface. arc4random functions were deliberately designed to be easy to use to avoid crypto failures due to misuse by their caller. Of course this places burden to implementator to get it right. Note that we have other APIs that can never fail, getpid(), getuid(), longjmp(), pthread_getspecific() etc.
>
> It seems Austin Group commenters got confused about this, interesting discussion though: https://www.austingroupbugs.net/view.php?id=859
If you think arc4random_buf is better design, please remove getrandom and implement arc4random_buf instead. But since it's impossible to return error code for arc4random_buf, how to handle the low entropy case? PANIC, I don't think it's good choice as note before.
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [incubator-nuttx] xiaoxiang781216 commented on pull request #2488: libs/libc: correct the getrandom(2) prototype
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on pull request #2488:
URL: https://github.com/apache/incubator-nuttx/pull/2488#issuecomment-739950396
> I'm opposed to this as the new flags don't do anything, they are just cruft copied over from Linux.
>
> NuttX is a new OS, it does not have to copy every non-standard feature from Linux.
But why do you have the right to add a new function which doesn't follow OpenGroup or other Unix variant? If you have this right, does it mean I also can add any funcitons I like?
getrandom has the same prototype in both FreeBSD and Linux:
https://www.freebsd.org/cgi/man.cgi?query=getrandom&sektion=2&manpath=freebsd-release-ports
https://man7.org/linux/man-pages/man2/getrandom.2.html
I don't think that there is any reason why we don't follow them.
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [incubator-nuttx] xiaoxiang781216 commented on pull request #2488: crypto/arc4random: rename getrandom to arc4random_buf
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on pull request #2488:
URL: https://github.com/apache/incubator-nuttx/pull/2488#issuecomment-744015013
@juniskane thanks, I will merge it now.
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [incubator-nuttx] juniskane edited a comment on pull request #2488: libs/libc: correct the getrandom(2) prototype
Posted by GitBox <gi...@apache.org>.
juniskane edited a comment on pull request #2488:
URL: https://github.com/apache/incubator-nuttx/pull/2488#issuecomment-740469936
> But now we support many different transport layer(IPv6, Unix Domain, Bluetooth, CAN) which represent addresss in the different way(sockaddr_in6, sockaddr_un, sockaddr_can). Should we change addr type from `struct sockaddr *` to `void *`? I don't think so.
Nobody is suggesting we should change well entrenched APIs like socket functions. They are also in OpenGroup specs of course. I think simple library wrappers like bzero() are fine as well, as there is widespread existing practise to have such compatibility wrappers, they are trivial to implement, harmless to those who don't need them, and everyone agrees how they work.
> Do you know why @anchao find the wrong getrandom prototype? Because he blindly believe that NuttX implement getrandom as man described and then follow the documentation to write the code, but finally it fail to compile. I think nobody like this experience especially we always emphasize NuttX standard compliance.
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!
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`
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?
> 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.
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)?
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [incubator-nuttx] xiaoxiang781216 commented on pull request #2488: libs/libc: correct the getrandom(2) prototype
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on pull request #2488:
URL: https://github.com/apache/incubator-nuttx/pull/2488#issuecomment-742715569
But, all these aren't the justification to change the function prototype. Because the current implementation only support the nonblocking behaviour, the right thing is checking GRND_NONBLOCK flag and return return error directly if user forget to set it.
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [incubator-nuttx] juniskane commented on pull request #2488: libs/libc: correct the getrandom(2) prototype
Posted by GitBox <gi...@apache.org>.
juniskane commented on pull request #2488:
URL: https://github.com/apache/incubator-nuttx/pull/2488#issuecomment-739961321
> getrandom has the same prototype in both FreeBSD and Linux:
> https://www.freebsd.org/cgi/man.cgi?query=getrandom&sektion=2&manpath=freebsd-release-ports
> https://man7.org/linux/man-pages/man2/getrandom.2.html
> I don't think that there is any reason why we don't follow them.
NuttX getrandom() interface is modeled after OpenBSD's getentropy() and arc4random_buf():
https://man.openbsd.org/getentropy
https://man.openbsd.org/arc4random.3
It was originally called speckrandom_buf() but was changed later to getrandom() by my colleague when the underlying cipher was changed to BLAKE2Xs. Today, I would create it as nx_getrandom() to be clear that it is a NuttX interface (but I don't propose this change/rename now, after several years of use).
Linux is not the pinnacle of OS development. I'm opposed to idea that new operating systems mindlessly imitate Linux even when things can be improved with little effort. Linux people have the idea that no user-space callers every have to change due to kernel changes, so they are stuck with bad interfaces forever. That is a good rule for OS with huge installed space, but not for us. Interfaces that have two parameters are easier to use than ones with three. Interfaces that cannot ever fail are more reliable, secure and easier to use than ones that can.
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [incubator-nuttx] xiaoxiang781216 edited a comment on pull request #2488: libs/libc: correct the getrandom(2) prototype
Posted by GitBox <gi...@apache.org>.
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?
> 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:
users@infra.apache.org
[GitHub] [incubator-nuttx] hartmannathan commented on pull request #2488: libs/libc: correct the getrandom(2) prototype
Posted by GitBox <gi...@apache.org>.
hartmannathan commented on pull request #2488:
URL: https://github.com/apache/incubator-nuttx/pull/2488#issuecomment-741800372
> Recently, it seems that the objectives of the OS have shifted. Historically, for a dozen years, the objective has been to be a Unix-work alike and to conform strictly to the requirements of a Unix system as specified at OpenGroup.org. This is still stated pretty clearly in the INVIOLABLES.md:
>
> ```
> ## Strict POSIX compliance
>
> - Strict conformance to the portable standard OS interface as defined at
> OpenGroup.org.
> - A deeply embedded system requires some special support. Special
> support must be minimized.
> - The portable interface must never be compromised only for the sake of
> expediency.
> - Expediency or even improved performance are not justifications for
> violation of the strict POSIX interface.
> ```
>
> Today, I think that objective has morphed; the OS is now treated like a Linux work-alike system. That is not necessarily bad, it is just a different objective. The only "bad" thing about this is that it happened with no conscious consent of the community.
>
> As a standard, Unix-like OS, inclusion of non-standard Linux interfaces is inappropriate; as a Linux work-alike OS, it is almost mandatory to include these non-standard interfaces. Being another Linux clone is not a very lofty objective for an OS although it many be practical for businesses who want to host Linux applications on NuttX.
Thinking out loud here: What about a compromise: by default NuttX is a Unix-like OS with POSIX interface as originally intended. In addition, if the user wants certain Linux-like APIs, the user could activate a new Kconfig option, say, CONFIG_LINUX_COMPAT. Likewise, if the user wants certain BSD-like APIs, a similar Kconfig option CONFIG_BSD_COMPAT could exist... The idea being to give people a way to add Linux/BSD APIs they need/want, but keeping them optional so that other users could exclude them.
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [incubator-nuttx] xiaoxiang781216 edited a comment on pull request #2488: libs/libc: correct the getrandom(2) prototype
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 edited a comment on pull request #2488:
URL: https://github.com/apache/incubator-nuttx/pull/2488#issuecomment-739960360
> Recently, it seems that the objectives of the OS have shifted. Historically, for a dozen years, the objective has been to be a Unix-work alike and to conform strictly to the requirements of a Unix system as specified at OpenGroup.org. This is still stated pretty clearly in the INVIOLABLES.md:
>
> ```
> ## Strict POSIX compliance
>
> - Strict conformance to the portable standard OS interface as defined at
> OpenGroup.org.
> - A deeply embedded system requires some special support. Special
> support must be minimized.
> - The portable interface must never be compromised only for the sake of
> expediency.
> - Expediency or even improved performance are not justifications for
> violation of the strict POSIX interface.
> ```
>
> Today, I think that objective has morphed; the OS is now treated like a Linux work-alike system. That is not necessarily bad, it is just a different objective. The only "bad" thing about this is that it happened with no conscious consent of the community.
>
> As a standard, Unix-like OS, inclusion of non-standard Linux interfaces is inappropriate; as a Linux work-alike OS, it is almost mandatory to include these non-standard interfaces. Being another Linux clone is not a very lofty objective for an OS although it many be practical for businesses who want to host Linux applications on NuttX.
I need said that there are many features in NuttX which not defined by POSIX:
1. bluetooth, can, ieee802154 and netlink socket
2. Many libc function only define in FreeBSD or Linux(e.g. bzero...)
3. nx graphics stack
So my question is that should we remove all these features from code base to confirm the pure POSIX compliance or extend the rule to cover the functions defined by FreeBSD or Linux but not in POSIX.
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [incubator-nuttx] juniskane commented on pull request #2488: libs/libc: correct the getrandom(2) prototype
Posted by GitBox <gi...@apache.org>.
juniskane commented on pull request #2488:
URL: https://github.com/apache/incubator-nuttx/pull/2488#issuecomment-739913380
I'm opposed to this as the new flags don't do anything, they are just cruft copied over from Linux.
NuttX is a new OS, it does not have to copy every non-standard feature from Linux.
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [incubator-nuttx] juniskane commented on pull request #2488: libs/libc: correct the getrandom(2) prototype
Posted by GitBox <gi...@apache.org>.
juniskane commented on pull request #2488:
URL: https://github.com/apache/incubator-nuttx/pull/2488#issuecomment-743120568
So the plan is rename current function as arc4random_buf and also add the three argument Linux version? I guess I have to live with that. That way we are compatible with both Linux and *BSD. arc4random_int(), arch4random_uniform() and the blocking behavior in (mostly theoretical if system designer anticipated it) low entropy case can be added in later commits, if someone wants to do that. The latter must not cause arc4random*() functions to block in any case.
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [incubator-nuttx] davids5 commented on pull request #2488: libs/libc: correct the getrandom(2) prototype
Posted by GitBox <gi...@apache.org>.
davids5 commented on pull request #2488:
URL: https://github.com/apache/incubator-nuttx/pull/2488#issuecomment-739922119
This is another hack to facilitate using third party Linix code that is not really supported. Is this what we as a group?
I would suggest that if you add the interface you have to add the code to supported and add a Kconfig to support the legacy interface.
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [incubator-nuttx] xiaoxiang781216 commented on pull request #2488: crypto/arc4random: rename getrandom to arc4random_buf
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on pull request #2488:
URL: https://github.com/apache/incubator-nuttx/pull/2488#issuecomment-743764482
@juniskane please review the update.
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [incubator-nuttx] juniskane commented on pull request #2488: libs/libc: correct the getrandom(2) prototype
Posted by GitBox <gi...@apache.org>.
juniskane commented on pull request #2488:
URL: https://github.com/apache/incubator-nuttx/pull/2488#issuecomment-742686053
On Solaris if GRND_RANDOM is not specified then getrandom() never blocks. On Linux, GRND_NONBLOCK is needed for this even for urandom pool as random source (at least with some kernel versions). Which way should NuttX behave?
Solaris getrandom() will either fail completely or will return a buffer filled with the requested size, Linux version can return partial buffers if interrupted by signal. Which way should NuttX behave?
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [incubator-nuttx] xiaoxiang781216 merged pull request #2488: crypto/arc4random: rename getrandom to arc4random_buf
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 merged pull request #2488:
URL: https://github.com/apache/incubator-nuttx/pull/2488
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [incubator-nuttx] patacongo commented on pull request #2488: libs/libc: correct the getrandom(2) prototype
Posted by GitBox <gi...@apache.org>.
patacongo commented on pull request #2488:
URL: https://github.com/apache/incubator-nuttx/pull/2488#issuecomment-739928330
Recently, it seems that the objectives of the OS have shifted. Historically, for a dozen years, the objective has been to be a Unix-work alike and to conform strictly to the requirements of a Unix system as specified at OpenGroup.org. This is still stated pretty clearly in the INVIOLABLES.md:
## Strict POSIX compliance
- Strict conformance to the portable standard OS interface as defined at
OpenGroup.org.
- A deeply embedded system requires some special support. Special
support must be minimized.
- The portable interface must never be compromised only for the sake of
expediency.
- Expediency or even improved performance are not justifications for
violation of the strict POSIX interface.
Today, I think that objective has morphed; the OS is now treated like a Linux work-alike system. That is not necessarily bad, it is just a different objective. The only "bad" thing about this is that it happened with no conscious consent of the community.
As a standard, Unix-like OS, inclusion of non-standard Linux interfaces is inappropriate; as a Linux work-alike OS, it is almost mandatory to include these non-standard interfaces. Being another Linux clone is not a very lofty objective for an OS although it many be practical for businesses who want to host Linux applications on NuttX.
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [incubator-nuttx] juniskane commented on pull request #2488: libs/libc: correct the getrandom(2) prototype
Posted by GitBox <gi...@apache.org>.
juniskane commented on pull request #2488:
URL: https://github.com/apache/incubator-nuttx/pull/2488#issuecomment-741601384
> If you think arc4random_buf is better design, please remove getrandom(wrong prototype) and implement arc4random_buf instead.
The title of this PR it incorrect as well, as it implies that some other prototype is somehow more correct than the existing one, which is not the case as Linux getrandom() is a non-standard, non-portable interface that exists on Linux alone.
> But since it's impossible to return error code for arc4random_buf, how to handle the low entropy case? PANIC, I don't think it's good choice as note before.
Low entropy case is mostly theoretical. If your MCU has a built-in RNG (most do these days), then seed the entropy pool from it during board init, before rest of the system is running. Most designs read ADC voltages, temperatures etc. early on so don't have to rely solely on HW RNG, which should be just one of the inputs to the entropy pool, not the only one. I have never had this supposed low entopy problem in practise.
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [incubator-nuttx] xiaoxiang781216 edited a comment on pull request #2488: libs/libc: correct the getrandom(2) prototype
Posted by GitBox <gi...@apache.org>.
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();
}
```
So, getrandom implement GRND_NONBLOCK semantic only. The secure implementation is return -1 and set errno to EAGAIN if entropy is low. Instead return the weak random number without any indication.
> > 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, but 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:
users@infra.apache.org
[GitHub] [incubator-nuttx] juniskane edited a comment on pull request #2488: libs/libc: correct the getrandom(2) prototype
Posted by GitBox <gi...@apache.org>.
juniskane edited a comment on pull request #2488:
URL: https://github.com/apache/incubator-nuttx/pull/2488#issuecomment-743120568
So the plan is rename current function as `arc4random_buf` and also add the three argument Linux version of `getrandom`? I guess I have to live with that. That way we are compatible with both Linux and various BSD Unixes. `arc4random_int()`, `arch4random_uniform()` and the blocking behavior in (mostly theoretical if system designer anticipated it) low entropy case can be added in later commits, if someone wants to do that. The latter must not cause arc4random*() functions to block in any case.
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [incubator-nuttx] hartmannathan commented on pull request #2488: libs/libc: correct the getrandom(2) prototype
Posted by GitBox <gi...@apache.org>.
hartmannathan commented on pull request #2488:
URL: https://github.com/apache/incubator-nuttx/pull/2488#issuecomment-743234459
> Have you checked, if the FreeBSD and Linux versions are different from each other in some subtle way? We cannot completely ignore Solaris precedent (and others who might have implemented getrandom()), as people might port Solaris code to NuttX as well.
See my thoughts above about introducing some kind of CONFIG_COMPAT_LINUX, CONFIG_COMPAT_BSD options. The idea is: if none of these options are activated, NuttX will only be Posix. If CONFIG_COMPAT_LINUX, that activates some linux compatibility interfaces. If there is a question which version of an interface should be used, the CONFIG_COMPAT_* options can be used for disambiguation. The disadvantage of this idea is more difficult maintenance. The advantage is that if people want some Linux or BSD interfaces to make it easier to port programs from those OS, the interfaces can be added, but are optional. So we don't violate the Inviolables :-)
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [incubator-nuttx] xiaoxiang781216 commented on pull request #2488: crypto/arc4random: rename getrandom to arc4random_buf
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on pull request #2488:
URL: https://github.com/apache/incubator-nuttx/pull/2488#issuecomment-743764060
> > Have you checked, if the FreeBSD and Linux versions are different from each other in some subtle way? We cannot completely ignore Solaris precedent (and others who might have implemented getrandom()), as people might port Solaris code to NuttX as well.
>
> See my thoughts above about introducing some kind of CONFIG_COMPAT_LINUX, CONFIG_COMPAT_BSD options. The idea is: if none of these options are activated, NuttX will only be Posix. If CONFIG_COMPAT_LINUX, that activates some linux compatibility interfaces. If there is a question which version of an interface should be used, the CONFIG_COMPAT_* options can be used for disambiguation. The disadvantage of this idea is more difficult maintenance. The advantage is that if people want some Linux or BSD interfaces to make it easier to port programs from those OS, the interfaces can be added, but are optional. So we don't violate the Inviolables :-)
libc implementer already use _XXX_SOURCE to enable the non standard functions:
https://ftp.gnu.org/old-gnu/Manuals/glibc-2.2.3/html_node/libc_13.html
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [incubator-nuttx] juniskane commented on pull request #2488: libs/libc: correct the getrandom(2) prototype
Posted by GitBox <gi...@apache.org>.
juniskane commented on pull request #2488:
URL: https://github.com/apache/incubator-nuttx/pull/2488#issuecomment-739924292
David, getrandom() itself is not a hack, as it adds feature that is not possible to implement with POSIX alone. The reasons why use getrandom() instead of open("/dev/urandom") have been discussed in the mailing list in past. (low max number of fd's configured, attacker can try to exhaust fds on purpose, need for slightly more error handling)
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [incubator-nuttx] xiaoxiang781216 commented on pull request #2488: libs/libc: correct the getrandom(2) prototype
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented 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?
>
> > 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?
> 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:
users@infra.apache.org
[GitHub] [incubator-nuttx] juniskane commented on pull request #2488: libs/libc: correct the getrandom(2) prototype
Posted by GitBox <gi...@apache.org>.
juniskane commented on pull request #2488:
URL: https://github.com/apache/incubator-nuttx/pull/2488#issuecomment-742660039
> I don't understand what we're arguing about, Whatever the name you like to call it getentropy，arc4random_ buf, it's up to you.
> but could you please bring the naming getrandom() back to POSIX ?
>
> Seriously, no one cares about your implementation，I only care about whether the definition of user interface can be easily accepted by everyone.
> Application developers only need to check the man page from POSIX !!!
>
> https://www.unix.com/man-page/posix/2/getrandom/
That is just the Linux man page for getrandom(). I am well familiar with Linux getrandom(2) as my colleagues and I studied it back in 2015 when we decided to improve the interface instead of mindlessly copying Linux. You seem to confuse POSIX with Linux manpages? getrandom() has never been in any standards, be they formal international standards or industry group de-facto standards.
Please see my reply to Xiao Xiang in https://github.com/apache/incubator-nuttx/pull/2497, I suggest you read in detail everything referenced there and in this thread as there is more to this topic than just the prototype. LKML mailing list has also had extensive discussions about getrandom().
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [incubator-nuttx] xiaoxiang781216 commented on pull request #2488: libs/libc: correct the getrandom(2) prototype
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on pull request #2488:
URL: https://github.com/apache/incubator-nuttx/pull/2488#issuecomment-743152250
> > Because the current implementation only support the nonblocking behaviour, the right thing is checking GRND_NONBLOCK flag and return return error directly if user forget to set it.
>
> No, the correct thing is to ignore GRND_NONBLOCK as it is not needed in this implementation. getrandom() with flags == 0 should not return any spurious errors if it can complete the request. I think with current implementation, flags can be ignore completely for now.
If the entropy is low, the currrent NuttX implementation isn't safe, because the returned value isn't really random in this case. It's a secure hole to use this value blindly without any caution.
> But since you and anchao want to bring the badly designed Linux version to NuttX, I'd expect you to be mindful about little details like this. Have you checked, if the FreeBSD and Linux versions are different from each other in some subtle way? We cannot completely ignore Solaris precedent (and others who might have implemented getrandom()), as people might port Solaris code to NuttX as well.
NuttX implement many POSIX API, it isn't impossible that NuttX has the same behaviour in all case as FreeBSD or Linux. But anyway, we need try the best to ensure the compability.
> So the plan is rename current function as `arc4random_buf` and also add the three argument Linux version of `getrandom`? I guess I have to live with that. That way we are compatible with both Linux and various BSD Unixes. `arc4random_int()`, `arch4random_uniform()` and the blocking behavior in (mostly theoretical if system designer anticipated it) low entropy case can be added in later commits, if someone wants to do that. The latter must not cause arc4random*() functions to block in any case.
getrandom will be removed, because the prototype is wrong. Only arc4random_buf exist in the future.
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [incubator-nuttx] xiaoxiang781216 commented on pull request #2488: libs/libc: correct the getrandom(2) prototype
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on pull request #2488:
URL: https://github.com/apache/incubator-nuttx/pull/2488#issuecomment-740054518
> > getrandom has the same prototype in both FreeBSD and Linux:
> > https://www.freebsd.org/cgi/man.cgi?query=getrandom&sektion=2&manpath=freebsd-release-ports
> > https://man7.org/linux/man-pages/man2/getrandom.2.html
> > I don't think that there is any reason why we don't follow them.
>
> NuttX getrandom() interface is modeled after OpenBSD's getentropy() and arc4random_buf():
> https://man.openbsd.org/getentropy
> https://man.openbsd.org/arc4random.3
It's fine that you model either getentropy, arc4random or getrandom.
> It was originally called speckrandom_buf()
it's fine if speckrandom_buf is just called by kernel function, but not good for userspace.
> but was changed later to getrandom() by my colleague when the underlying cipher was changed to BLAKE2Xs. Today, I would create it as nx_getrandom() to be clear that it is a NuttX interface (but I don't propose this change/rename now, after several years of use).
No, nx_xxx is just for OS internal function. If the function plan to expose to userspace(part of libc), it isn't a good idea to add nx_ prefix.
>
> Linux is not the pinnacle of OS development. I'm opposed to idea that new operating systems mindlessly imitate Linux even when things can be improved with little effort. Linux people have the idea that no user-space callers every have to change due to kernel changes, so they are stuck with bad interfaces forever.
POSIX compliance is NuttX's most important unique feature compared with other RTOS. There are many API design isn't perfect, does it mean we should change API prototype? For example, many socket API accept only IPv4 address:
```
int bind(int sockfd, FAR const struct sockaddr *addr, socklen_t addrlen);
int connect(int sockfd, FAR const struct sockaddr *addr, socklen_t addrlen);
int accept(int sockfd, FAR struct sockaddr *addr, FAR socklen_t *addrlen);
ssize_t sendto(int sockfd, FAR const void *buf, size_t len, int flags,
FAR const struct sockaddr *to, socklen_t tolen);
ssize_t recvfrom(int sockfd, FAR void *buf, size_t len, int flags,
FAR struct sockaddr *from, FAR socklen_t *fromlen);
```
But now we support many different transport layer(IPv6, Unix Domain, Bluetooth, CAN) which represent addresss in the different way(sockaddr_in6, sockaddr_un, sockaddr_can). Should we change addr type from `struct sockaddr *` to `void *`? I don't think so.
> That is a good rule for OS with huge installed space, but not for us.
Do you know why @anchao find the wrong getrandom prototype? Because he blindly believe that NuttX implement getrandom as man described and then follow the documentation to write the code, but finally it fail to compile. I think nobody like this experience especially we always emphasize NuttX standard compliance.
> Interfaces that have two parameters are easier to use than ones with three. Interfaces that cannot ever fail are more reliable, secure and easier to use than ones that can.
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?
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [incubator-nuttx] xiaoxiang781216 edited a comment on pull request #2488: libs/libc: correct the getrandom(2) prototype
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 edited a comment on pull request #2488:
URL: https://github.com/apache/incubator-nuttx/pull/2488#issuecomment-739950396
> I'm opposed to this as the new flags don't do anything, they are just cruft copied over from Linux.
>
> NuttX is a new OS, it does not have to copy every non-standard feature from Linux.
But why do you have the right to add a new function which doesn't follow OpenGroup or other Unix variant? If you have this right, does it mean I can add any funcitons I like?
getrandom has the same prototype in both FreeBSD and Linux:
https://www.freebsd.org/cgi/man.cgi?query=getrandom&sektion=2&manpath=freebsd-release-ports
https://man7.org/linux/man-pages/man2/getrandom.2.html
I don't think that there is any reason why we don't follow them.
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [incubator-nuttx] juniskane commented on pull request #2488: libs/libc: correct the getrandom(2) prototype
Posted by GitBox <gi...@apache.org>.
juniskane commented on pull request #2488:
URL: https://github.com/apache/incubator-nuttx/pull/2488#issuecomment-742675766
Solaris 11.3 man page says for getrandom(): "Otherwise, it returns 0 and sets errno to indicate the error." Linux/FreeBSD error return value is -1. This may be just an error in Solaris man page though.
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [incubator-nuttx] anchao commented on pull request #2488: libs/libc: correct the getrandom(2) prototype
Posted by GitBox <gi...@apache.org>.
anchao commented on pull request #2488:
URL: https://github.com/apache/incubator-nuttx/pull/2488#issuecomment-741921106
Hi @juniskane
I don't understand what we're arguing about, Whatever the name you like to call it getentropy，arc4random_ buf, it's up to you.
but could you please bring the naming getrandom() back to POSIX ?
Seriously, no one cares about your implementation，I only care about whether the definition of user interface can be easily accepted by everyone.
Application developers only need to check the man page from POSIX !!!
https://www.unix.com/man-page/posix/2/getrandom/
You prefer to define user interfaces in reliable, secure and easier to use, this is very commendable, the goal is consistent with us,
but the prototypes is a legacy of history, if we don't fix it soon, more developers will question it in the future，
Q："why the getrandom(2) does not strictly follow the POSIX compliance ?"
Juniskane： "Interfaces that have two parameters are easier to use than ones with three. Interfaces that cannot ever fail are more reliable...."
Q："why the getrandom(2) does not strictly follow the POSIX compliance? NuttX should be Strict POSIX compliance!"
Juniskane： "Oh, Dear developer, Interfaces that have two parameters are easier to use than ones with three. Interfaces that cannot ever fail are more reliable...."
What should we do like this "tomorrow"？
I think the best solution is to accept the legacy, accept the history, there is no absolute correctness or wrongness.
this is just because different people may view the same thing from different perspectives,
A standard interface will make it easier for application developers to feel the perfection of NuttX,
what an amazing thing to let a FreeBSD or Linux application run in NuttX without any changes,
I don't quite understand what will affect you if change to a compliance prototype?
getranom() is not in your openBSD page, Could you please change the name back to arc4random_buf(), thank you very much !!
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [incubator-nuttx] patacongo commented on pull request #2488: libs/libc: correct the getrandom(2) prototype
Posted by GitBox <gi...@apache.org>.
patacongo commented on pull request #2488:
URL: https://github.com/apache/incubator-nuttx/pull/2488#issuecomment-740772533
> 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
> ...
From #2497
getrandom() is not a cancellation point and must not implement any cancellation point logic. It should not use nxsem_wait_uninterruptible() but should use something like nxsem_wait_noncancelable(). nxsem_wait-noncancelable() has not been formalized but would simply ignore the ECANCELED error (as well as the EINTR error).
In that case, getrandom would correctly ignore the cancellation request and simply call nxsem_wait() again.
ECANCELED should only occur in the deferred cancellation mode. In other modes, the task calling getrandom() will be terminated immediately with no return value. (I think anyway. I haven't looked at the code in a long time). In the deferred cancellation mode, cancellation can only occur at a few cancellation points; getrandom() is not a cancellatoin point.
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [incubator-nuttx] xiaoxiang781216 commented on pull request #2488: libs/libc: correct the getrandom(2) prototype
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on pull request #2488:
URL: https://github.com/apache/incubator-nuttx/pull/2488#issuecomment-739960360
> Recently, it seems that the objectives of the OS have shifted. Historically, for a dozen years, the objective has been to be a Unix-work alike and to conform strictly to the requirements of a Unix system as specified at OpenGroup.org. This is still stated pretty clearly in the INVIOLABLES.md:
>
> ```
> ## Strict POSIX compliance
>
> - Strict conformance to the portable standard OS interface as defined at
> OpenGroup.org.
> - A deeply embedded system requires some special support. Special
> support must be minimized.
> - The portable interface must never be compromised only for the sake of
> expediency.
> - Expediency or even improved performance are not justifications for
> violation of the strict POSIX interface.
> ```
>
> Today, I think that objective has morphed; the OS is now treated like a Linux work-alike system. That is not necessarily bad, it is just a different objective. The only "bad" thing about this is that it happened with no conscious consent of the community.
>
> As a standard, Unix-like OS, inclusion of non-standard Linux interfaces is inappropriate; as a Linux work-alike OS, it is almost mandatory to include these non-standard interfaces. Being another Linux clone is not a very lofty objective for an OS although it many be practical for businesses who want to host Linux applications on NuttX.
I need said that there are many features in NuttX which not defined by POSIX:
1. bluetooth, can, ieee802154 and netlink socket
2. Many libc function only define in FreeBSD or Linux(e.g. bzero...)
3. nx graphics stack
So my question is that should we remove all these features from code base to confirm the pure POSIX compliance or extend the rule to cover the functions defined by FreeBSD or Linux but not in POSIX.
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [incubator-nuttx] patacongo edited a comment on pull request #2488: libs/libc: correct the getrandom(2) prototype
Posted by GitBox <gi...@apache.org>.
patacongo edited a comment on pull request #2488:
URL: https://github.com/apache/incubator-nuttx/pull/2488#issuecomment-740772533
> 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
> ...
From #2497
getrandom() is not a cancellation point and must not implement any cancellation point logic. It should not use nxsem_wait_uninterruptible() but should use something like nxsem_wait_noncancelable(). nxsem_wait-noncancelable() has not been formalized but would simply ignore the ECANCELED error (as well as the EINTR error).
In that case, getrandom would correctly ignore the cancellation request and simply call nxsem_wait() again.
ECANCELED should only occur in the deferred cancellation mode. In other modes, the task calling getrandom() will be terminated immediately with no return value. (I think anyway. I haven't looked at the code in a long time). In the deferred cancellation mode, cancellation can only occur at a few cancellation points; getrandom() is not a cancellatoin point.
In normal usage, ECANCELED is never seen by the application code. It is either not generated or is handled by the lower level cancellation point (which getrandom() is not). Hence, there is no need to return the ECANCELED error code.
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [incubator-nuttx] juniskane commented on pull request #2488: crypto/arc4random: rename getrandom to arc4random_buf
Posted by GitBox <gi...@apache.org>.
juniskane commented on pull request #2488:
URL: https://github.com/apache/incubator-nuttx/pull/2488#issuecomment-744013189
> @juniskane please review the update.
Rename commits look OK to me.
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [incubator-nuttx] xiaoxiang781216 edited a comment on pull request #2488: libs/libc: correct the getrandom(2) prototype
Posted by GitBox <gi...@apache.org>.
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:
users@infra.apache.org
[GitHub] [incubator-nuttx] xiaoxiang781216 commented on pull request #2488: libs/libc: correct the getrandom(2) prototype
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on pull request #2488:
URL: https://github.com/apache/incubator-nuttx/pull/2488#issuecomment-742929095
@anchao per the dissussion here #2497, please rename getrandom to arc4random_buf instead.
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [incubator-nuttx] juniskane commented on pull request #2488: libs/libc: correct the getrandom(2) prototype
Posted by GitBox <gi...@apache.org>.
juniskane commented on pull request #2488:
URL: https://github.com/apache/incubator-nuttx/pull/2488#issuecomment-740469936
> But now we support many different transport layer(IPv6, Unix Domain, Bluetooth, CAN) which represent addresss in the different way(sockaddr_in6, sockaddr_un, sockaddr_can). Should we change addr type from `struct sockaddr *` to `void *`? I don't think so.
Nobody is suggesting we should change well entrenched APIs like socket functions. They are also in OpenGroup specs of course. I think simple library wrappers like bzero() are fine as well, as there is widespread existing practise to have such compatibility wrappers, they are trivial to implement, harmless to those who don't need them, and everyone agrees how they work.
> Do you know why @anchao find the wrong getrandom prototype? Because he blindly believe that NuttX implement getrandom as man described and then follow the documentation to write the code, but finally it fail to compile. I think nobody like this experience especially we always emphasize NuttX standard compliance.
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!
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`
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?
> 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.
To better understand these issues, please try this exercise: 1) implement Linux getrandom() using OpenBSD's arc4random() function 2) implement arc4random() 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)?
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [incubator-nuttx] juniskane commented on pull request #2488: libs/libc: correct the getrandom(2) prototype
Posted by GitBox <gi...@apache.org>.
juniskane commented on pull request #2488:
URL: https://github.com/apache/incubator-nuttx/pull/2488#issuecomment-739926875
I think there is a real problem with getrandom(), not caused by this PR but introduced by Issue 619 https://github.com/apache/incubator-nuttx/issues/619
There is a comment in getrandom() function description: "Note that this function cannot fail, other than by asserting." But this is no longer true, if nxsem_wait_uninterruptible() return ECANCELLED because some other task called pthread_cancel() for the thread calling getrandom(). This is very bad, as caller might consume the buffer before next cancellation point. I think solution is to make this function a cancellation point? Any thoughts about this? I think non-standard functions can be cancellation points, nothing in POSIX is prohibiting that.
The **wrong** solution would be to change getrandom() return value to int and force every caller to deal with this error. It it bad for security if every caller needs to deal with cancelled or otherwise failing getrandom(). What to do when it fails? Default to some insecure mode like calling rand()? (NuttX is on purpose different than Linux in this regard.)
----------------------------------------------------------------
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:
users@infra.apache.org