Re: [Qemu-devel] [RFC] Virtio RNG: Consider changing the default entropy source to /dev/urandom?

Re: [Qemu-devel] [RFC] Virtio RNG: Consider changing the default entropy source to /dev/urandom?

[Kashyap Chamarthy]

[Reviving this old thread as I don't think we came to a conclusion on
this.]

On Fri, Sep 21, 2018 at 05:43:23PM +0200, Kashyap Chamarthy wrote:
> Hi folks,
> 
> As Markus pointed out in this 'qemu-devel' thread[1],
> backends/rng-random.c uses '/dev/random' in TYPE_RNG_RANDOM's
> instance_init() method:
> 
>     [...]
>     static void rng_random_init(Object *obj)
>     {
>         RngRandom *s = RNG_RANDOM(obj);
>     
>         object_property_add_str(obj, "filename",
>                                 rng_random_get_filename,
>                                 rng_random_set_filename,
>                                 NULL);
>     
>         s->filename = g_strdup("/dev/random");
>         s->fd = -1;
>     }
>     [...]
> 
> And I've looked at hw/virtio/virtio-rng.c:
> 
>     [...]
>     static void virtio_rng_device_realize(DeviceState *dev, Error **errp)
>     {
>     [...]
>     
>         if (vrng->conf.rng == NULL) {
>             vrng->conf.default_backend = RNG_RANDOM(object_new(TYPE_RNG_RANDOM));
>     [...]
> 
> From the above, I'm assuming QEMU uses `/dev/random` as the _default_
> entropy source for a 'virtio-rng-pci' device.  If my assumption is
> correct, any reason why not to change the default entropy source for
> 'virtio-rng-pci' devices to `/dev/urandom` (which is the preferred[2]
> source of entropy)?
> 
> And I understand (thanks: Eric Blake for correcting my confusion) that
> there are two cases to distinguish:
> 
> (a) When QEMU needs a random number, the entropy source it chooses.
>     IIUC, the answer is: QEMU defers to GnuTLS by default, which uses
>     getrandom(2), which in turn uses '/dev/urandom' as its entropy
>     source; if getrandom(2) isn't available, GnuTLS uses `/dev/urandom`
>     anyway.  (Thanks: Nikos for clarifying this.)
> 
>     If QEMU is built with GnuTLS _disabled_, which I'm not sure if any
>     Linux distribution does, then it uses libgcrypt, which in turn uses
>     the undesired and legacy `/dev/random` as the default entropy
>     source.
> 
> (b) When QEMU exposes a Virtio RNG device to the guest, that device
>     needs a source of entropy, and IIUC, that source needs to be
>     "non-blocking" (i.e. `/dev/urandom`).  However, currently QEMU
>     defaults to the problematic `/dev/random`.
> 
> I'd like to get some more clarity on case (b).  
> 
> 
> [1] https://lists.nongnu.org/archive/html/qemu-devel/2018-06/msg08335.html
>     -- RNG: Any reason QEMU doesn't default to `/dev/urandom`
> 
> [2] http://man7.org/linux/man-pages/man4/urandom.4.html
> 
> 
> -- 
> /kashyap
> 

-- 
/kashyap

Re: [Qemu-devel] [RFC] Virtio RNG: Consider changing the default entropy source to /dev/urandom?

[Kashyap Chamarthy]

[Reviving this old thread as I don't think we came to a conclusion on
this.]

On Fri, Sep 21, 2018 at 05:43:23PM +0200, Kashyap Chamarthy wrote:
> Hi folks,
> 
> As Markus pointed out in this 'qemu-devel' thread[1],
> backends/rng-random.c uses '/dev/random' in TYPE_RNG_RANDOM's
> instance_init() method:
> 
>     [...]
>     static void rng_random_init(Object *obj)
>     {
>         RngRandom *s = RNG_RANDOM(obj);
>     
>         object_property_add_str(obj, "filename",
>                                 rng_random_get_filename,
>                                 rng_random_set_filename,
>                                 NULL);
>     
>         s->filename = g_strdup("/dev/random");
>         s->fd = -1;
>     }
>     [...]
> 
> And I've looked at hw/virtio/virtio-rng.c:
> 
>     [...]
>     static void virtio_rng_device_realize(DeviceState *dev, Error **errp)
>     {
>     [...]
>     
>         if (vrng->conf.rng == NULL) {
>             vrng->conf.default_backend = RNG_RANDOM(object_new(TYPE_RNG_RANDOM));
>     [...]
> 
> From the above, I'm assuming QEMU uses `/dev/random` as the _default_
> entropy source for a 'virtio-rng-pci' device.  If my assumption is
> correct, any reason why not to change the default entropy source for
> 'virtio-rng-pci' devices to `/dev/urandom` (which is the preferred[2]
> source of entropy)?
> 
> And I understand (thanks: Eric Blake for correcting my confusion) that
> there are two cases to distinguish:
> 
> (a) When QEMU needs a random number, the entropy source it chooses.
>     IIUC, the answer is: QEMU defers to GnuTLS by default, which uses
>     getrandom(2), which in turn uses '/dev/urandom' as its entropy
>     source; if getrandom(2) isn't available, GnuTLS uses `/dev/urandom`
>     anyway.  (Thanks: Nikos for clarifying this.)
> 
>     If QEMU is built with GnuTLS _disabled_, which I'm not sure if any
>     Linux distribution does, then it uses libgcrypt, which in turn uses
>     the undesired and legacy `/dev/random` as the default entropy
>     source.
> 
> (b) When QEMU exposes a Virtio RNG device to the guest, that device
>     needs a source of entropy, and IIUC, that source needs to be
>     "non-blocking" (i.e. `/dev/urandom`).  However, currently QEMU
>     defaults to the problematic `/dev/random`.
> 
> I'd like to get some more clarity on case (b).  
> 
> 
> [1] https://lists.nongnu.org/archive/html/qemu-devel/2018-06/msg08335.html
>     -- RNG: Any reason QEMU doesn't default to `/dev/urandom`
> 
> [2] http://man7.org/linux/man-pages/man4/urandom.4.html
> 
> 
> -- 
> /kashyap
> 

-- 
/kashyap

Re: [Qemu-devel] [RFC] Virtio RNG: Consider changing the default entropy source to /dev/urandom?

[Richard W.M. Jones]

On Thu, May 02, 2019 at 08:02:01PM +0200, Kashyap Chamarthy wrote:
> [Reviving this old thread as I don't think we came to a conclusion on
> this.]

I guess the best thing is to submit the obvious 1 line patch and see
what people think about it?  (I agree the default ought to be changed.)

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
libguestfs lets you edit virtual machines.  Supports shell scripting,
bindings from many languages.  http://libguestfs.org

Re: [Qemu-devel] [RFC] Virtio RNG: Consider changing the default entropy source to /dev/urandom?

[Richard W.M. Jones]

On Thu, May 02, 2019 at 08:02:01PM +0200, Kashyap Chamarthy wrote:
> [Reviving this old thread as I don't think we came to a conclusion on
> this.]

I guess the best thing is to submit the obvious 1 line patch and see
what people think about it?  (I agree the default ought to be changed.)

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
libguestfs lets you edit virtual machines.  Supports shell scripting,
bindings from many languages.  http://libguestfs.org

Re: [Qemu-devel] [RFC] Virtio RNG: Consider changing the default entropy source to /dev/urandom?

[Nikos Mavrogiannopoulos]

In terms of RHEL what is preferred is (1) use a crypto lib, and (2) if
that's not possible use getrandom(). That is summarized in this
article:

https://www.redhat.com/en/blog/understanding-red-hat-enterprise-linux-random-number-generator-interface

On Thu, May 2, 2019 at 8:02 PM Kashyap Chamarthy <kchamart@redhat.com> wrote:
>
> [Reviving this old thread as I don't think we came to a conclusion on
> this.]
>
> On Fri, Sep 21, 2018 at 05:43:23PM +0200, Kashyap Chamarthy wrote:
> > Hi folks,
> >
> > As Markus pointed out in this 'qemu-devel' thread[1],
> > backends/rng-random.c uses '/dev/random' in TYPE_RNG_RANDOM's
> > instance_init() method:
> >
> >     [...]
> >     static void rng_random_init(Object *obj)
> >     {
> >         RngRandom *s = RNG_RANDOM(obj);
> >
> >         object_property_add_str(obj, "filename",
> >                                 rng_random_get_filename,
> >                                 rng_random_set_filename,
> >                                 NULL);
> >
> >         s->filename = g_strdup("/dev/random");
> >         s->fd = -1;
> >     }
> >     [...]
> >
> > And I've looked at hw/virtio/virtio-rng.c:
> >
> >     [...]
> >     static void virtio_rng_device_realize(DeviceState *dev, Error **errp)
> >     {
> >     [...]
> >
> >         if (vrng->conf.rng == NULL) {
> >             vrng->conf.default_backend = RNG_RANDOM(object_new(TYPE_RNG_RANDOM));
> >     [...]
> >
> > From the above, I'm assuming QEMU uses `/dev/random` as the _default_
> > entropy source for a 'virtio-rng-pci' device.  If my assumption is
> > correct, any reason why not to change the default entropy source for
> > 'virtio-rng-pci' devices to `/dev/urandom` (which is the preferred[2]
> > source of entropy)?
> >
> > And I understand (thanks: Eric Blake for correcting my confusion) that
> > there are two cases to distinguish:
> >
> > (a) When QEMU needs a random number, the entropy source it chooses.
> >     IIUC, the answer is: QEMU defers to GnuTLS by default, which uses
> >     getrandom(2), which in turn uses '/dev/urandom' as its entropy
> >     source; if getrandom(2) isn't available, GnuTLS uses `/dev/urandom`
> >     anyway.  (Thanks: Nikos for clarifying this.)
> >
> >     If QEMU is built with GnuTLS _disabled_, which I'm not sure if any
> >     Linux distribution does, then it uses libgcrypt, which in turn uses
> >     the undesired and legacy `/dev/random` as the default entropy
> >     source.
> >
> > (b) When QEMU exposes a Virtio RNG device to the guest, that device
> >     needs a source of entropy, and IIUC, that source needs to be
> >     "non-blocking" (i.e. `/dev/urandom`).  However, currently QEMU
> >     defaults to the problematic `/dev/random`.
> >
> > I'd like to get some more clarity on case (b).
> >
> >
> > [1] https://lists.nongnu.org/archive/html/qemu-devel/2018-06/msg08335.html
> >     -- RNG: Any reason QEMU doesn't default to `/dev/urandom`
> >
> > [2] http://man7.org/linux/man-pages/man4/urandom.4.html
> >
> >
> > --
> > /kashyap
> >
>
> --
> /kashyap

Re: [Qemu-devel] [RFC] Virtio RNG: Consider changing the default entropy source to /dev/urandom?

[Daniel P. Berrangé]

On Tue, May 07, 2019 at 11:59:05AM +0200, Nikos Mavrogiannopoulos wrote:
> In terms of RHEL what is preferred is (1) use a crypto lib, and (2) if
> that's not possible use getrandom(). That is summarized in this
> article:
> 
> https://www.redhat.com/en/blog/understanding-red-hat-enterprise-linux-random-number-generator-interface

For QEMU this would mean re-writing the code to use qcrypto_random_bytes
instead. This internal API is backed by a crypto lib if available,
falling back to /dev/urandom or /dev/random on UNIX, or CryptGenRandom
on Windows. We could add getrandom() support there too.

The main question is whether to implement a new backends/rng-builtin.c
or modify backends/rng-random.c so that it has a NULL filename by
default, which would be taken as meaning use the qcrypto_random_bytes
API.  The latter benefits that all existing VMs which don't have a
filename set would get the new behaviour. The latter has downside
that it is not discoverable from mgmt apps, so they won't know if
they can rely on it or not.

Thus I'd probably tend towards a new backend for discoverability.

> 
> On Thu, May 2, 2019 at 8:02 PM Kashyap Chamarthy <kchamart@redhat.com> wrote:
> >
> > [Reviving this old thread as I don't think we came to a conclusion on
> > this.]
> >
> > On Fri, Sep 21, 2018 at 05:43:23PM +0200, Kashyap Chamarthy wrote:
> > > Hi folks,
> > >
> > > As Markus pointed out in this 'qemu-devel' thread[1],
> > > backends/rng-random.c uses '/dev/random' in TYPE_RNG_RANDOM's
> > > instance_init() method:
> > >
> > >     [...]
> > >     static void rng_random_init(Object *obj)
> > >     {
> > >         RngRandom *s = RNG_RANDOM(obj);
> > >
> > >         object_property_add_str(obj, "filename",
> > >                                 rng_random_get_filename,
> > >                                 rng_random_set_filename,
> > >                                 NULL);
> > >
> > >         s->filename = g_strdup("/dev/random");
> > >         s->fd = -1;
> > >     }
> > >     [...]
> > >
> > > And I've looked at hw/virtio/virtio-rng.c:
> > >
> > >     [...]
> > >     static void virtio_rng_device_realize(DeviceState *dev, Error **errp)
> > >     {
> > >     [...]
> > >
> > >         if (vrng->conf.rng == NULL) {
> > >             vrng->conf.default_backend = RNG_RANDOM(object_new(TYPE_RNG_RANDOM));
> > >     [...]
> > >
> > > From the above, I'm assuming QEMU uses `/dev/random` as the _default_
> > > entropy source for a 'virtio-rng-pci' device.  If my assumption is
> > > correct, any reason why not to change the default entropy source for
> > > 'virtio-rng-pci' devices to `/dev/urandom` (which is the preferred[2]
> > > source of entropy)?
> > >
> > > And I understand (thanks: Eric Blake for correcting my confusion) that
> > > there are two cases to distinguish:
> > >
> > > (a) When QEMU needs a random number, the entropy source it chooses.
> > >     IIUC, the answer is: QEMU defers to GnuTLS by default, which uses
> > >     getrandom(2), which in turn uses '/dev/urandom' as its entropy
> > >     source; if getrandom(2) isn't available, GnuTLS uses `/dev/urandom`
> > >     anyway.  (Thanks: Nikos for clarifying this.)
> > >
> > >     If QEMU is built with GnuTLS _disabled_, which I'm not sure if any
> > >     Linux distribution does, then it uses libgcrypt, which in turn uses
> > >     the undesired and legacy `/dev/random` as the default entropy
> > >     source.
> > >
> > > (b) When QEMU exposes a Virtio RNG device to the guest, that device
> > >     needs a source of entropy, and IIUC, that source needs to be
> > >     "non-blocking" (i.e. `/dev/urandom`).  However, currently QEMU
> > >     defaults to the problematic `/dev/random`.
> > >
> > > I'd like to get some more clarity on case (b).
> > >
> > >
> > > [1] https://lists.nongnu.org/archive/html/qemu-devel/2018-06/msg08335.html
> > >     -- RNG: Any reason QEMU doesn't default to `/dev/urandom`
> > >
> > > [2] http://man7.org/linux/man-pages/man4/urandom.4.html
> > >
> > >
> > > --
> > > /kashyap
> > >
> >
> > --
> > /kashyap
> 

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 :|

Re: [Qemu-devel] [RFC] Virtio RNG: Consider changing the default entropy source to /dev/urandom?

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 1720 bytes --]

On 5/7/19 10:22 AM, Daniel P. Berrangé wrote:
> On Tue, May 07, 2019 at 11:59:05AM +0200, Nikos Mavrogiannopoulos wrote:
>> In terms of RHEL what is preferred is (1) use a crypto lib, and (2) if
>> that's not possible use getrandom(). That is summarized in this
>> article:
>>
>> https://www.redhat.com/en/blog/understanding-red-hat-enterprise-linux-random-number-generator-interface
> 
> For QEMU this would mean re-writing the code to use qcrypto_random_bytes
> instead. This internal API is backed by a crypto lib if available,
> falling back to /dev/urandom or /dev/random on UNIX, or CryptGenRandom
> on Windows. We could add getrandom() support there too.
> 
> The main question is whether to implement a new backends/rng-builtin.c
> or modify backends/rng-random.c so that it has a NULL filename by
> default, which would be taken as meaning use the qcrypto_random_bytes
> API.  The latter benefits that all existing VMs which don't have a
> filename set would get the new behaviour. The latter has downside
> that it is not discoverable from mgmt apps, so they won't know if
> they can rely on it or not.

Alas, our example in misc.json is:

# -> { "execute": "object-add",
#      "arguments": { "qom-type": "rng-random", "id": "rng1",
#                     "props": { "filename": "/dev/hwrng" } } }
# <- { "return": {} }

which is indeed one of the interfaces that evades introspection at the
present, so even if we made filename a StrOrNull, which would normally
show up in introspection when done on any other command, it is invisible
here. :(

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [Qemu-devel] [RFC] Virtio RNG: Consider changing the default entropy source to /dev/urandom?

[Richard Henderson]

On 5/7/19 8:22 AM, Daniel P. Berrangé wrote:
> On Tue, May 07, 2019 at 11:59:05AM +0200, Nikos Mavrogiannopoulos wrote:
>> In terms of RHEL what is preferred is (1) use a crypto lib, and (2) if
>> that's not possible use getrandom(). That is summarized in this
>> article:
>>
>> https://www.redhat.com/en/blog/understanding-red-hat-enterprise-linux-random-number-generator-interface
> 
> For QEMU this would mean re-writing the code to use qcrypto_random_bytes
> instead. This internal API is backed by a crypto lib if available,
> falling back to /dev/urandom or /dev/random on UNIX, or CryptGenRandom
> on Windows. We could add getrandom() support there too.

At least this last step is done:

  https://patchwork.ozlabs.org/patch/1056828/


> The main question is whether to implement a new backends/rng-builtin.c
> or modify backends/rng-random.c so that it has a NULL filename by
> default, which would be taken as meaning use the qcrypto_random_bytes
> API.  The latter benefits that all existing VMs which don't have a
> filename set would get the new behaviour. The latter has downside
> that it is not discoverable from mgmt apps, so they won't know if
> they can rely on it or not.
> 
> Thus I'd probably tend towards a new backend for discoverability.

What does it mean to rely on the filename, really?

We could special case "/dev/urandom" as qcrypto_random_bytes, which would end
up using getrandom(2) or /dev/urandom via the crypto lib anyway.

We could even special case "/dev/random" as getrandom(2) w/GRND_RANDOM, if we
cared to bypass the crypto lib.

Only oddballs like "/dev/myhwrng" truly need to go through the filesystem
interface in order to preserve behaviour, I would think.


r~

Re: [Qemu-devel] [RFC] Virtio RNG: Consider changing the default entropy source to /dev/urandom?

[Daniel P. Berrangé]

On Tue, May 07, 2019 at 10:14:25AM -0700, Richard Henderson wrote:
> On 5/7/19 8:22 AM, Daniel P. Berrangé wrote:
> > On Tue, May 07, 2019 at 11:59:05AM +0200, Nikos Mavrogiannopoulos wrote:
> >> In terms of RHEL what is preferred is (1) use a crypto lib, and (2) if
> >> that's not possible use getrandom(). That is summarized in this
> >> article:
> >>
> >> https://www.redhat.com/en/blog/understanding-red-hat-enterprise-linux-random-number-generator-interface
> > 
> > For QEMU this would mean re-writing the code to use qcrypto_random_bytes
> > instead. This internal API is backed by a crypto lib if available,
> > falling back to /dev/urandom or /dev/random on UNIX, or CryptGenRandom
> > on Windows. We could add getrandom() support there too.
> 
> At least this last step is done:
> 
>   https://patchwork.ozlabs.org/patch/1056828/

Ah yes, I forgot you had done that already, which is nice.

> > The main question is whether to implement a new backends/rng-builtin.c
> > or modify backends/rng-random.c so that it has a NULL filename by
> > default, which would be taken as meaning use the qcrypto_random_bytes
> > API.  The latter benefits that all existing VMs which don't have a
> > filename set would get the new behaviour. The latter has downside
> > that it is not discoverable from mgmt apps, so they won't know if
> > they can rely on it or not.
> > 
> > Thus I'd probably tend towards a new backend for discoverability.
> 
> What does it mean to rely on the filename, really?
> 
> We could special case "/dev/urandom" as qcrypto_random_bytes, which would end
> up using getrandom(2) or /dev/urandom via the crypto lib anyway.
> 
> We could even special case "/dev/random" as getrandom(2) w/GRND_RANDOM, if we
> cared to bypass the crypto lib.

IME magic like this has a habit of coming back to bite you. eg if we later
find there is some use case where its important to /really/ use /dev/urandom
and we've magically turned /dev/urandom into a call to gnutls, we're stuck.

If we want to use qcrypto_random_bytes we must have an explicit way to get
that, rather than changing semantics of existing filenames apps might be
passing.

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 :|