| Message ID | 20230128184113.3502926-1-hauke@hauke-m.de |
|---|---|
| State | Superseded |
| Delegated to: | Hauke Mehrtens |
| Headers | show |
| Series | [ustream-ssl] ustream-mbedtls: Use getrandom() instead of /dev/urandom | expand |
On Sat, Jan 28, 2023 at 10:45 AM Hauke Mehrtens <hauke@hauke-m.de> wrote: > > Instead of keeping a file descriptor open just use the getrandom syscall > to get random data. This is supported by the musl, glibc and Linux for > some time now. > > This also improves the error handling in case this function returns not > as many bytes as expected. > > Signed-off-by: Hauke Mehrtens <hauke@hauke-m.de> Acked-by: Rosen Penev <rosenp@gmail.com> > --- > ustream-mbedtls.c | 23 +++++------------------ > 1 file changed, 5 insertions(+), 18 deletions(-) > > diff --git a/ustream-mbedtls.c b/ustream-mbedtls.c > index e79e37b..51ba2fa 100644 > --- a/ustream-mbedtls.c > +++ b/ustream-mbedtls.c > @@ -17,6 +17,7 @@ > */ > > #include <sys/types.h> > +#include <sys/random.h> > #include <fcntl.h> > #include <unistd.h> > #include <stdlib.h> > @@ -25,8 +26,6 @@ > #include "ustream-ssl.h" > #include "ustream-internal.h" > > -static int urandom_fd = -1; > - > static int s_ustream_read(void *ctx, unsigned char *buf, size_t len) > { > struct ustream *s = ctx; > @@ -66,21 +65,12 @@ __hidden void ustream_set_io(struct ustream_ssl_ctx *ctx, void *ssl, struct ustr > mbedtls_ssl_set_bio(ssl, conn, s_ustream_write, s_ustream_read, NULL); > } > > -static bool urandom_init(void) > -{ > - if (urandom_fd > -1) > - return true; > - > - urandom_fd = open("/dev/urandom", O_RDONLY); > - if (urandom_fd < 0) > - return false; > - > - return true; > -} > - > static int _urandom(void *ctx, unsigned char *out, size_t len) > { > - if (read(urandom_fd, out, len) < 0) > + ssize_t ret; > + > + ret = getrandom(out, len, 0); > + if (ret < 0 || (size_t)ret != len) > return MBEDTLS_ERR_ENTROPY_SOURCE_FAILED; > > return 0; > @@ -134,9 +124,6 @@ __ustream_ssl_context_new(bool server) > mbedtls_ssl_config *conf; > int ep; > > - if (!urandom_init()) > - return NULL; > - > ctx = calloc(1, sizeof(*ctx)); > if (!ctx) > return NULL; > -- > 2.39.0 > > > _______________________________________________ > openwrt-devel mailing list > openwrt-devel@lists.openwrt.org > https://lists.openwrt.org/mailman/listinfo/openwrt-devel
On Sat, 28 Jan 2023 19:41:13 +0100 Hauke Mehrtens <hauke@hauke-m.de> wrote: > Instead of keeping a file descriptor open just use the getrandom syscall > to get random data. This is supported by the musl, glibc and Linux for > some time now. > > This also improves the error handling in case this function returns not > as many bytes as expected. > > Signed-off-by: Hauke Mehrtens <hauke@hauke-m.de> > --- > ustream-mbedtls.c | 23 +++++------------------ > 1 file changed, 5 insertions(+), 18 deletions(-) > > diff --git a/ustream-mbedtls.c b/ustream-mbedtls.c > index e79e37b..51ba2fa 100644 > --- a/ustream-mbedtls.c > +++ b/ustream-mbedtls.c > @@ -17,6 +17,7 @@ > */ > > #include <sys/types.h> > +#include <sys/random.h> > #include <fcntl.h> > #include <unistd.h> > #include <stdlib.h> > @@ -25,8 +26,6 @@ > #include "ustream-ssl.h" > #include "ustream-internal.h" > > -static int urandom_fd = -1; > - > static int s_ustream_read(void *ctx, unsigned char *buf, size_t len) > { > struct ustream *s = ctx; > @@ -66,21 +65,12 @@ __hidden void ustream_set_io(struct ustream_ssl_ctx *ctx, void *ssl, struct ustr > mbedtls_ssl_set_bio(ssl, conn, s_ustream_write, s_ustream_read, NULL); > } > > -static bool urandom_init(void) > -{ > - if (urandom_fd > -1) > - return true; > - > - urandom_fd = open("/dev/urandom", O_RDONLY); > - if (urandom_fd < 0) > - return false; > - > - return true; > -} > - > static int _urandom(void *ctx, unsigned char *out, size_t len) > { > - if (read(urandom_fd, out, len) < 0) > + ssize_t ret; > + > + ret = getrandom(out, len, 0); > + if (ret < 0 || (size_t)ret != len) > return MBEDTLS_ERR_ENTROPY_SOURCE_FAILED; [...] drivers/char/random.c lines 1240- ... * Reading from /dev/urandom has the same functionality as calling * getrandom(2) with flags=GRND_INSECURE. Because it does not block * waiting for the RNG to be ready, it should not be used. Haven't audited mbedtls, but I assume it reads urandom for "lesser" entropy when needed. In any case, getrandom(out, len, GRND_INSECURE) would be the proper replacement. Torsten
On 1/29/23 15:13, Torsten Duwe wrote: > On Sat, 28 Jan 2023 19:41:13 +0100 > Hauke Mehrtens <hauke@hauke-m.de> wrote: > >> Instead of keeping a file descriptor open just use the getrandom syscall >> to get random data. This is supported by the musl, glibc and Linux for >> some time now. >> >> This also improves the error handling in case this function returns not >> as many bytes as expected. >> >> Signed-off-by: Hauke Mehrtens <hauke@hauke-m.de> >> --- >> ustream-mbedtls.c | 23 +++++------------------ >> 1 file changed, 5 insertions(+), 18 deletions(-) >> >> diff --git a/ustream-mbedtls.c b/ustream-mbedtls.c >> index e79e37b..51ba2fa 100644 >> --- a/ustream-mbedtls.c >> +++ b/ustream-mbedtls.c >> @@ -17,6 +17,7 @@ >> */ >> >> #include <sys/types.h> >> +#include <sys/random.h> >> #include <fcntl.h> >> #include <unistd.h> >> #include <stdlib.h> >> @@ -25,8 +26,6 @@ >> #include "ustream-ssl.h" >> #include "ustream-internal.h" >> >> -static int urandom_fd = -1; >> - >> static int s_ustream_read(void *ctx, unsigned char *buf, size_t len) >> { >> struct ustream *s = ctx; >> @@ -66,21 +65,12 @@ __hidden void ustream_set_io(struct ustream_ssl_ctx *ctx, void *ssl, struct ustr >> mbedtls_ssl_set_bio(ssl, conn, s_ustream_write, s_ustream_read, NULL); >> } >> >> -static bool urandom_init(void) >> -{ >> - if (urandom_fd > -1) >> - return true; >> - >> - urandom_fd = open("/dev/urandom", O_RDONLY); >> - if (urandom_fd < 0) >> - return false; >> - >> - return true; >> -} >> - >> static int _urandom(void *ctx, unsigned char *out, size_t len) >> { >> - if (read(urandom_fd, out, len) < 0) >> + ssize_t ret; >> + >> + ret = getrandom(out, len, 0); >> + if (ret < 0 || (size_t)ret != len) >> return MBEDTLS_ERR_ENTROPY_SOURCE_FAILED; > [...] > > drivers/char/random.c lines 1240- ... > * Reading from /dev/urandom has the same functionality as calling > * getrandom(2) with flags=GRND_INSECURE. Because it does not block > * waiting for the RNG to be ready, it should not be used. > > Haven't audited mbedtls, but I assume it reads urandom for "lesser" > entropy when needed. In any case, getrandom(out, len, GRND_INSECURE) > would be the proper replacement. > > Torsten Hi Torsten, The mapage says this: > By default, getrandom() draws entropy from the urandom source > (i.e., the same source as the /dev/urandom device). This > behavior can be changed via the flags argument. https://man7.org/linux/man-pages/man2/getrandom.2.html GRND_INSECURE is also not documented in the man page. The option was added to the Linux kernel 5.6 here: https://git.kernel.org/linus/75551dbf112c992bc6c99a972990b3f272247e23 The documentation says > GRND_INSECURE Return non-cryptographic random bytes We want to use the random bytes in mbedtls for cryptographic operations. I think giving no flags is the correct option here. I think the behavior of /dev/random changed some years ago. This article described it a bit: https://lwn.net/Articles/808575/ As far as I understood the code, giving no flags will guarantee that the random pool is initialized (crng_ready() returns true) and otherwise it is the same as using GRND_INSECURE. As we use it for cryptographic operations I think we should give no flags. Hauke
Hi Hauke! On Sun, 29 Jan 2023 17:08:38 +0100 Hauke Mehrtens <hauke@hauke-m.de> wrote: > > drivers/char/random.c lines 1240- ... > > * Reading from /dev/urandom has the same functionality as calling > > * getrandom(2) with flags=GRND_INSECURE. Because it does not block > > * waiting for the RNG to be ready, it should not be used. > > > > Haven't audited mbedtls, but I assume it reads urandom for "lesser" > > entropy when needed. In any case, getrandom(out, len, GRND_INSECURE) > > would be the proper replacement. > > > > Torsten > > Hi Torsten, > > The mapage says this: > > By default, getrandom() draws entropy from the urandom source > > (i.e., the same source as the /dev/urandom device). This > > behavior can be changed via the flags argument. > https://man7.org/linux/man-pages/man2/getrandom.2.html > > GRND_INSECURE is also not documented in the man page. Well, it exists in the kernel source and headers... > The option was added to the Linux kernel 5.6 here: > https://git.kernel.org/linus/75551dbf112c992bc6c99a972990b3f272247e23 > > The documentation says > > GRND_INSECURE Return non-cryptographic random bytes > We want to use the random bytes in mbedtls for cryptographic > operations. I think giving no flags is the correct option here. > > I think the behavior of /dev/random changed some years ago. This > article described it a bit: https://lwn.net/Articles/808575/ That's only the internal workings. BTW, the mentioned quote of Andy Lutomirski applies here in some sense. You read away the true entropy and might even block on it when pseudo- randomness might suffice, see below. > As far as I understood the code, giving no flags will guarantee that > the random pool is initialized (crng_ready() returns true) and > otherwise it is the same as using GRND_INSECURE. As we use it for > cryptographic operations I think we should give no flags. "cryptographic operations" is a wide area. Some randomness is required, to varying degrees, for long-term keys, session keys, IVs and padding. Especially for long living keys, each end every bit should be totally unpredictable, which should correspond to read an appropriate amount from /dev/random. IVs and padding can be generated from a pseudo-RNG whose initial state is "uncertain enough", usually /dev/urandom. GIT is cool. c6e9d6f388947986 2014-Jul-17 tytso: introduce getrandom(2) system call 75551dbf112c992b 2019-Dec-23 luto: add GRND_INSECURE ... 48446f198f9adcb4 2019-Dec-23 luto: ignore GRND_RANDOM The first commit also has a man page in the comment, which is probably what was recorded. The second one changes the no-flags behaviour, away from the man page text you quoted. Someone once mentioned on LKML that drivers/char/random.c needs better maintenance... ;) I had a quick look at mbedtls and its usage of the provided rng function. It generates not only padding and IVs, but also session keys. Especially on OpenWRT these are a trade-off IMHO. OpenWRT supports a lot of hardware that is low on entropy at boot (MIPS anyone?) Do you want to block early ssh sessions, maybe even for minutes, or would you rather risk eavesdropping on those early connections? Depending on your choice for ustream, you can keep the proposed code, but please rename the functions to "random", not "urandom". Or you want to stick with the current urandom behaviour but then please add Luto's GRND_INSECURE flag to achieve that. HTH, Torsten
Hi Torsten, Sorry for the late answer, I forgot about this mail thread. On 1/30/23 10:57, Torsten Duwe wrote: > Hi Hauke! > > On Sun, 29 Jan 2023 17:08:38 +0100 > Hauke Mehrtens <hauke@hauke-m.de> wrote: > >>> drivers/char/random.c lines 1240- ... >>> * Reading from /dev/urandom has the same functionality as calling >>> * getrandom(2) with flags=GRND_INSECURE. Because it does not block >>> * waiting for the RNG to be ready, it should not be used. >>> >>> Haven't audited mbedtls, but I assume it reads urandom for "lesser" >>> entropy when needed. In any case, getrandom(out, len, GRND_INSECURE) >>> would be the proper replacement. >>> >>> Torsten >> >> Hi Torsten, >> >> The mapage says this: >> > By default, getrandom() draws entropy from the urandom source >> > (i.e., the same source as the /dev/urandom device). This >> > behavior can be changed via the flags argument. >> https://man7.org/linux/man-pages/man2/getrandom.2.html >> >> GRND_INSECURE is also not documented in the man page. > > Well, it exists in the kernel source and headers... > >> The option was added to the Linux kernel 5.6 here: >> https://git.kernel.org/linus/75551dbf112c992bc6c99a972990b3f272247e23 >> >> The documentation says >> > GRND_INSECURE Return non-cryptographic random bytes >> We want to use the random bytes in mbedtls for cryptographic >> operations. I think giving no flags is the correct option here. >> >> I think the behavior of /dev/random changed some years ago. This >> article described it a bit: https://lwn.net/Articles/808575/ > > That's only the internal workings. > BTW, the mentioned quote of Andy Lutomirski applies here in some sense. > You read away the true entropy and might even block on it when pseudo- > randomness might suffice, see below. > >> As far as I understood the code, giving no flags will guarantee that >> the random pool is initialized (crng_ready() returns true) and >> otherwise it is the same as using GRND_INSECURE. As we use it for >> cryptographic operations I think we should give no flags. > > "cryptographic operations" is a wide area. Some randomness is required, > to varying degrees, for long-term keys, session keys, IVs and padding. ustreamss uses the randomness to generate session keys (including ephemeral keys), IVs and padding. The long term keys are generated in a different application. > Especially for long living keys, each end every bit should be totally > unpredictable, which should correspond to read an appropriate amount > from /dev/random. IVs and padding can be generated from a pseudo-RNG > whose initial state is "uncertain enough", usually /dev/urandom. > > GIT is cool. > c6e9d6f388947986 2014-Jul-17 tytso: introduce getrandom(2) system call > 75551dbf112c992b 2019-Dec-23 luto: add GRND_INSECURE ... > 48446f198f9adcb4 2019-Dec-23 luto: ignore GRND_RANDOM > > The first commit also has a man page in the comment, which is probably > what was recorded. The second one changes the no-flags behaviour, away > from the man page text you quoted. > > Someone once mentioned on LKML that drivers/char/random.c needs better > maintenance... ;) > > I had a quick look at mbedtls and its usage of the provided rng > function. It generates not only padding and IVs, but also session keys. > Especially on OpenWRT these are a trade-off IMHO. OpenWRT supports a > lot of hardware that is low on entropy at boot (MIPS anyone?) Do you > want to block early ssh sessions, maybe even for minutes, or would you > rather risk eavesdropping on those early connections? > > Depending on your choice for ustream, you can keep the proposed code, > but please rename the functions to "random", not "urandom". Or you want > to stick with the current urandom behaviour but then please add Luto's > GRND_INSECURE flag to achieve that. crng_ready() should only return false at bootup before the system got enough random bits, afterwards it never returns false. Even without GRND_INSECURE it will never run out of entropy. I think we should wait with creating TLS sessions till we have enough random data to do it securely. I tested this on a lantiq xrx200 (MIPS) device and it was initialized much before the LAN interface was up. The code in ustream-mbedtls.c was probably initially written when /dev/random was still blocking when too much entropy was read out of the pool. I will rename the function. Hauke
Hi Hauke! On Sun, 19 Feb 2023 21:06:15 +0100 Hauke Mehrtens <hauke@hauke-m.de> wrote: > Hi Torsten, > > Sorry for the late answer, I forgot about this mail thread. No problem. > > On Sun, 29 Jan 2023 17:08:38 +0100 > > Hauke Mehrtens <hauke@hauke-m.de> wrote: [...] > ustreamss uses the randomness to generate session keys (including > ephemeral keys), IVs and padding. The long term keys are generated in a > different application. [...] > > I think we should wait with creating TLS sessions till we have enough > random data to do it securely. I tested this on a lantiq xrx200 (MIPS) > device and it was initialized much before the LAN interface was up. ^^^^^^^^^^^ Yes. Good that it works out this way. Otherwise you'd have had a tough decision to make. > The code in ustream-mbedtls.c was probably initially written when > /dev/random was still blocking when too much entropy was read out of the > pool. I guess so, too. > I will rename the function. Cool. You can add my review tag if you want... Torsten
diff --git a/ustream-mbedtls.c b/ustream-mbedtls.c index e79e37b..51ba2fa 100644 --- a/ustream-mbedtls.c +++ b/ustream-mbedtls.c @@ -17,6 +17,7 @@ */ #include <sys/types.h> +#include <sys/random.h> #include <fcntl.h> #include <unistd.h> #include <stdlib.h> @@ -25,8 +26,6 @@ #include "ustream-ssl.h" #include "ustream-internal.h" -static int urandom_fd = -1; - static int s_ustream_read(void *ctx, unsigned char *buf, size_t len) { struct ustream *s = ctx; @@ -66,21 +65,12 @@ __hidden void ustream_set_io(struct ustream_ssl_ctx *ctx, void *ssl, struct ustr mbedtls_ssl_set_bio(ssl, conn, s_ustream_write, s_ustream_read, NULL); } -static bool urandom_init(void) -{ - if (urandom_fd > -1) - return true; - - urandom_fd = open("/dev/urandom", O_RDONLY); - if (urandom_fd < 0) - return false; - - return true; -} - static int _urandom(void *ctx, unsigned char *out, size_t len) { - if (read(urandom_fd, out, len) < 0) + ssize_t ret; + + ret = getrandom(out, len, 0); + if (ret < 0 || (size_t)ret != len) return MBEDTLS_ERR_ENTROPY_SOURCE_FAILED; return 0; @@ -134,9 +124,6 @@ __ustream_ssl_context_new(bool server) mbedtls_ssl_config *conf; int ep; - if (!urandom_init()) - return NULL; - ctx = calloc(1, sizeof(*ctx)); if (!ctx) return NULL;
Instead of keeping a file descriptor open just use the getrandom syscall to get random data. This is supported by the musl, glibc and Linux for some time now. This also improves the error handling in case this function returns not as many bytes as expected. Signed-off-by: Hauke Mehrtens <hauke@hauke-m.de> --- ustream-mbedtls.c | 23 +++++------------------ 1 file changed, 5 insertions(+), 18 deletions(-)