diff mbox series

[v2,04/14] crypto: Use O_CLOEXEC in qcrypto_random_init

Message ID 20190314045526.13342-5-richard.henderson@linaro.org
State Superseded
Headers show
Series Add qemu_getrandom and ARMv8.5-RNG | expand

Commit Message

Richard Henderson March 14, 2019, 4:55 a.m. UTC
Avoids leaking the /dev/urandom fd into any child processes.

Cc: Daniel P. Berrangé <berrange@redhat.com>
Signed-off-by: Richard Henderson <richard.henderson@linaro.org>

---
 crypto/random-platform.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

-- 
2.17.1

Comments

Daniel P. Berrangé March 14, 2019, 3:34 p.m. UTC | #1
On Wed, Mar 13, 2019 at 09:55:16PM -0700, Richard Henderson wrote:
> Avoids leaking the /dev/urandom fd into any child processes.

> 

> Cc: Daniel P. Berrangé <berrange@redhat.com>

> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>

> ---

>  crypto/random-platform.c | 4 ++--

>  1 file changed, 2 insertions(+), 2 deletions(-)

> 

> diff --git a/crypto/random-platform.c b/crypto/random-platform.c

> index 0866f216dc..8bfce99a65 100644

> --- a/crypto/random-platform.c

> +++ b/crypto/random-platform.c

> @@ -42,9 +42,9 @@ int qcrypto_random_init(Error **errp)

>  #else

>      /* TBD perhaps also add support for BSD getentropy / Linux

>       * getrandom syscalls directly */

> -    fd = open("/dev/urandom", O_RDONLY);

> +    fd = open("/dev/urandom", O_RDONLY | O_CLOEXEC);

>      if (fd == -1 && errno == ENOENT) {

> -        fd = open("/dev/random", O_RDONLY);

> +        fd = open("/dev/random", O_RDONLY | O_CLOEXEC);

>      }

>  

>      if (fd < 0) {


Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>


Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|
Eric Blake March 14, 2019, 3:43 p.m. UTC | #2
On 3/14/19 10:34 AM, Daniel P. Berrangé wrote:
> On Wed, Mar 13, 2019 at 09:55:16PM -0700, Richard Henderson wrote:

>> Avoids leaking the /dev/urandom fd into any child processes.

>>

>> Cc: Daniel P. Berrangé <berrange@redhat.com>

>> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>

>> ---

>>  crypto/random-platform.c | 4 ++--

>>  1 file changed, 2 insertions(+), 2 deletions(-)

>>

>> diff --git a/crypto/random-platform.c b/crypto/random-platform.c

>> index 0866f216dc..8bfce99a65 100644

>> --- a/crypto/random-platform.c

>> +++ b/crypto/random-platform.c

>> @@ -42,9 +42,9 @@ int qcrypto_random_init(Error **errp)

>>  #else

>>      /* TBD perhaps also add support for BSD getentropy / Linux

>>       * getrandom syscalls directly */

>> -    fd = open("/dev/urandom", O_RDONLY);

>> +    fd = open("/dev/urandom", O_RDONLY | O_CLOEXEC);

>>      if (fd == -1 && errno == ENOENT) {

>> -        fd = open("/dev/random", O_RDONLY);

>> +        fd = open("/dev/random", O_RDONLY | O_CLOEXEC);

>>      }

>>  

>>      if (fd < 0) {

> 

> Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>


Are we at the point where we can declare open(O_CLOEXEC)
mandatory-supported on all systems we compile on, or do we need to use
qemu_open() to get the semantics we need (with proper fallback to
non-atomic fcntl() on older platforms)?

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org
Daniel P. Berrangé March 14, 2019, 3:52 p.m. UTC | #3
On Thu, Mar 14, 2019 at 10:43:11AM -0500, Eric Blake wrote:
> On 3/14/19 10:34 AM, Daniel P. Berrangé wrote:

> > On Wed, Mar 13, 2019 at 09:55:16PM -0700, Richard Henderson wrote:

> >> Avoids leaking the /dev/urandom fd into any child processes.

> >>

> >> Cc: Daniel P. Berrangé <berrange@redhat.com>

> >> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>

> >> ---

> >>  crypto/random-platform.c | 4 ++--

> >>  1 file changed, 2 insertions(+), 2 deletions(-)

> >>

> >> diff --git a/crypto/random-platform.c b/crypto/random-platform.c

> >> index 0866f216dc..8bfce99a65 100644

> >> --- a/crypto/random-platform.c

> >> +++ b/crypto/random-platform.c

> >> @@ -42,9 +42,9 @@ int qcrypto_random_init(Error **errp)

> >>  #else

> >>      /* TBD perhaps also add support for BSD getentropy / Linux

> >>       * getrandom syscalls directly */

> >> -    fd = open("/dev/urandom", O_RDONLY);

> >> +    fd = open("/dev/urandom", O_RDONLY | O_CLOEXEC);

> >>      if (fd == -1 && errno == ENOENT) {

> >> -        fd = open("/dev/random", O_RDONLY);

> >> +        fd = open("/dev/random", O_RDONLY | O_CLOEXEC);

> >>      }

> >>  

> >>      if (fd < 0) {

> > 

> > Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>

> 

> Are we at the point where we can declare open(O_CLOEXEC)

> mandatory-supported on all systems we compile on, or do we need to use

> qemu_open() to get the semantics we need (with proper fallback to

> non-atomic fcntl() on older platforms)?


It has been available on Linux for all our targetted distros. I can see
it in man pages for FreeBSD, NetBSD & OpenBSD too, and for macOS 10.6
(~2009).

I think windows is the main one which lacks it, but this #ifdef of the
code isn't built on Windows.

IOW, I think we're safe.

Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|
diff mbox series

Patch

diff --git a/crypto/random-platform.c b/crypto/random-platform.c
index 0866f216dc..8bfce99a65 100644
--- a/crypto/random-platform.c
+++ b/crypto/random-platform.c
@@ -42,9 +42,9 @@  int qcrypto_random_init(Error **errp)
 #else
     /* TBD perhaps also add support for BSD getentropy / Linux
      * getrandom syscalls directly */
-    fd = open("/dev/urandom", O_RDONLY);
+    fd = open("/dev/urandom", O_RDONLY | O_CLOEXEC);
     if (fd == -1 && errno == ENOENT) {
-        fd = open("/dev/random", O_RDONLY);
+        fd = open("/dev/random", O_RDONLY | O_CLOEXEC);
     }
 
     if (fd < 0) {