[libvirt] [PATCH] conf: Drop restrictions on rng backend path

Currently we only allow /dev/random and /dev/hwrng as host input for <rng><backend model='random'/> device. This was added after various upstream discussions in commit 4932ef45 However this restriction has generated quite a few complaints over the years, so a new discussion was initiated: http://www.redhat.com/archives/libvir-list/2016-April/msg00987.html Several people suggested removing the restriction, and nobody really spoke up to defend it. So this patch drops the path restriction entirely https://bugzilla.redhat.com/show_bug.cgi?id=1074464 --- docs/formatdomain.html.in | 9 ++++----- docs/schemas/domaincommon.rng | 3 +-- src/conf/domain_conf.c | 8 -------- tests/qemuxml2argvdata/qemuxml2argv-virtio-rng-random.args | 2 +- tests/qemuxml2argvdata/qemuxml2argv-virtio-rng-random.xml | 2 +- tests/qemuxml2xmloutdata/qemuxml2xmlout-virtio-rng-random.xml | 2 +- 6 files changed, 8 insertions(+), 18 deletions(-) diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in index 895114b..132b261 100644 --- a/docs/formatdomain.html.in +++ b/docs/formatdomain.html.in @@ -6231,8 +6231,7 @@ qemu-kvm -net nic,model=? /dev/null <code>model</code> attribute. Supported source models are: </p> <ul> - <li>'random' — /dev/random (default) or /dev/hwrng - device as source (for now, no other sources are permitted)</li> + <li>'random' — a character device backend</li> <li>'egd' — a EGD protocol backend</li> </ul> </dd> @@ -6240,9 +6239,9 @@ qemu-kvm -net nic,model=? /dev/null <dd> <p> This backend type expects a non-blocking character device as input. - The only accepted paths are /dev/random and /dev/hwrng. The file - name is specified as contents of the <code>backend</code> element. - When no file name is specified the hypervisor default is used. + The file name is specified as contents of the + <code>backend</code> element. When no file name is specified the + this defaults to /dev/random. </p> </dd> <dt><code>backend model='egd'</code></dt> diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng index 3605afe..c91a3a9 100644 --- a/docs/schemas/domaincommon.rng +++ b/docs/schemas/domaincommon.rng @@ -4691,8 +4691,7 @@ <value>random</value> </attribute> <choice> - <value>/dev/random</value> - <value>/dev/hwrng</value> + <ref name='absFilePath'/> <empty/> </choice> </group> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 28248c8..f9cd69c 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -11512,14 +11512,6 @@ virDomainRNGDefParseXML(xmlNodePtr node, switch ((virDomainRNGBackend) def->backend) { case VIR_DOMAIN_RNG_BACKEND_RANDOM: def->source.file = virXPathString("string(./backend)", ctxt); - if (def->source.file && - STRNEQ(def->source.file, "/dev/random") && - STRNEQ(def->source.file, "/dev/hwrng")) { - virReportError(VIR_ERR_XML_ERROR, - _("file '%s' is not a supported random source"), - def->source.file); - goto error; - } break; case VIR_DOMAIN_RNG_BACKEND_EGD: diff --git a/tests/qemuxml2argvdata/qemuxml2argv-virtio-rng-random.args b/tests/qemuxml2argvdata/qemuxml2argv-virtio-rng-random.args index ba97c1e..5a4d47b 100644 --- a/tests/qemuxml2argvdata/qemuxml2argv-virtio-rng-random.args +++ b/tests/qemuxml2argvdata/qemuxml2argv-virtio-rng-random.args @@ -18,6 +18,6 @@ QEMU_AUDIO_DRV=none \ -boot c \ -usb \ -device virtio-balloon-pci,id=balloon0,bus=pci.0,addr=0x3 \ --object rng-random,id=objrng0,filename=/dev/hwrng \ +-object rng-random,id=objrng0,filename=/dev/urandom \ -device virtio-rng-pci,rng=objrng0,id=rng0,max-bytes=123,period=1234,bus=pci.0,\ addr=0x4 diff --git a/tests/qemuxml2argvdata/qemuxml2argv-virtio-rng-random.xml b/tests/qemuxml2argvdata/qemuxml2argv-virtio-rng-random.xml index 71bd21a..a6e91ff 100644 --- a/tests/qemuxml2argvdata/qemuxml2argv-virtio-rng-random.xml +++ b/tests/qemuxml2argvdata/qemuxml2argv-virtio-rng-random.xml @@ -21,7 +21,7 @@ <memballoon model='virtio'/> <rng model='virtio'> <rate bytes='123' period='1234'/> - <backend model='random'>/dev/hwrng</backend> + <backend model='random'>/dev/urandom</backend> </rng> </devices> </domain> diff --git a/tests/qemuxml2xmloutdata/qemuxml2xmlout-virtio-rng-random.xml b/tests/qemuxml2xmloutdata/qemuxml2xmlout-virtio-rng-random.xml index e8a0478..0bdefde 100644 --- a/tests/qemuxml2xmloutdata/qemuxml2xmlout-virtio-rng-random.xml +++ b/tests/qemuxml2xmloutdata/qemuxml2xmlout-virtio-rng-random.xml @@ -25,7 +25,7 @@ </memballoon> <rng model='virtio'> <rate bytes='123' period='1234'/> - <backend model='random'>/dev/hwrng</backend> + <backend model='random'>/dev/urandom</backend> <address type='pci' domain='0x0000' bus='0x00' slot='0x04' function='0x0'/> </rng> </devices> -- 2.7.3

On Wed, Apr 20, 2016 at 06:19:25PM -0400, Cole Robinson wrote:
Currently we only allow /dev/random and /dev/hwrng as host input for <rng><backend model='random'/> device. This was added after various upstream discussions in commit 4932ef45
However this restriction has generated quite a few complaints over the years, so a new discussion was initiated:
http://www.redhat.com/archives/libvir-list/2016-April/msg00987.html
Several people suggested removing the restriction, and nobody really spoke up to defend it. So this patch drops the path restriction entirely
https://bugzilla.redhat.com/show_bug.cgi?id=1074464 --- docs/formatdomain.html.in | 9 ++++----- docs/schemas/domaincommon.rng | 3 +-- src/conf/domain_conf.c | 8 -------- tests/qemuxml2argvdata/qemuxml2argv-virtio-rng-random.args | 2 +- tests/qemuxml2argvdata/qemuxml2argv-virtio-rng-random.xml | 2 +- tests/qemuxml2xmloutdata/qemuxml2xmlout-virtio-rng-random.xml | 2 +- 6 files changed, 8 insertions(+), 18 deletions(-)
diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in index 895114b..132b261 100644 --- a/docs/formatdomain.html.in +++ b/docs/formatdomain.html.in @@ -6231,8 +6231,7 @@ qemu-kvm -net nic,model=? /dev/null <code>model</code> attribute. Supported source models are: </p> <ul> - <li>'random' — /dev/random (default) or /dev/hwrng - device as source (for now, no other sources are permitted)</li> + <li>'random' — a character device backend</li> <li>'egd' — a EGD protocol backend</li> </ul> </dd> @@ -6240,9 +6239,9 @@ qemu-kvm -net nic,model=? /dev/null <dd> <p> This backend type expects a non-blocking character device as input. - The only accepted paths are /dev/random and /dev/hwrng. The file - name is specified as contents of the <code>backend</code> element. - When no file name is specified the hypervisor default is used. + The file name is specified as contents of the + <code>backend</code> element. When no file name is specified the + this defaults to /dev/random. </p> </dd> <dt><code>backend model='egd'</code></dt> diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng index 3605afe..c91a3a9 100644 --- a/docs/schemas/domaincommon.rng +++ b/docs/schemas/domaincommon.rng @@ -4691,8 +4691,7 @@ <value>random</value> </attribute> <choice> - <value>/dev/random</value> - <value>/dev/hwrng</value> + <ref name='absFilePath'/> <empty/> </choice> </group> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 28248c8..f9cd69c 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -11512,14 +11512,6 @@ virDomainRNGDefParseXML(xmlNodePtr node, switch ((virDomainRNGBackend) def->backend) { case VIR_DOMAIN_RNG_BACKEND_RANDOM: def->source.file = virXPathString("string(./backend)", ctxt); - if (def->source.file && - STRNEQ(def->source.file, "/dev/random") && - STRNEQ(def->source.file, "/dev/hwrng")) { - virReportError(VIR_ERR_XML_ERROR, - _("file '%s' is not a supported random source"), - def->source.file); - goto error; - } break;
case VIR_DOMAIN_RNG_BACKEND_EGD: diff --git a/tests/qemuxml2argvdata/qemuxml2argv-virtio-rng-random.args b/tests/qemuxml2argvdata/qemuxml2argv-virtio-rng-random.args index ba97c1e..5a4d47b 100644 --- a/tests/qemuxml2argvdata/qemuxml2argv-virtio-rng-random.args +++ b/tests/qemuxml2argvdata/qemuxml2argv-virtio-rng-random.args @@ -18,6 +18,6 @@ QEMU_AUDIO_DRV=none \ -boot c \ -usb \ -device virtio-balloon-pci,id=balloon0,bus=pci.0,addr=0x3 \ --object rng-random,id=objrng0,filename=/dev/hwrng \ +-object rng-random,id=objrng0,filename=/dev/urandom \ -device virtio-rng-pci,rng=objrng0,id=rng0,max-bytes=123,period=1234,bus=pci.0,\ addr=0x4 diff --git a/tests/qemuxml2argvdata/qemuxml2argv-virtio-rng-random.xml b/tests/qemuxml2argvdata/qemuxml2argv-virtio-rng-random.xml index 71bd21a..a6e91ff 100644 --- a/tests/qemuxml2argvdata/qemuxml2argv-virtio-rng-random.xml +++ b/tests/qemuxml2argvdata/qemuxml2argv-virtio-rng-random.xml @@ -21,7 +21,7 @@ <memballoon model='virtio'/> <rng model='virtio'> <rate bytes='123' period='1234'/> - <backend model='random'>/dev/hwrng</backend> + <backend model='random'>/dev/urandom</backend> </rng> </devices> </domain> diff --git a/tests/qemuxml2xmloutdata/qemuxml2xmlout-virtio-rng-random.xml b/tests/qemuxml2xmloutdata/qemuxml2xmlout-virtio-rng-random.xml index e8a0478..0bdefde 100644 --- a/tests/qemuxml2xmloutdata/qemuxml2xmlout-virtio-rng-random.xml +++ b/tests/qemuxml2xmloutdata/qemuxml2xmlout-virtio-rng-random.xml @@ -25,7 +25,7 @@ </memballoon> <rng model='virtio'> <rate bytes='123' period='1234'/> - <backend model='random'>/dev/hwrng</backend> + <backend model='random'>/dev/urandom</backend> <address type='pci' domain='0x0000' bus='0x00' slot='0x04' function='0x0'/> </rng> </devices>
ACK. Rich. -- Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones Read my programming and virtualization blog: http://rwmj.wordpress.com Fedora Windows cross-compiler. Compile Windows programs, test, and build Windows installers. Over 100 libraries supported. http://fedoraproject.org/wiki/MinGW

On Wed, Apr 20, 2016 at 18:19:25 -0400, Cole Robinson wrote:
Currently we only allow /dev/random and /dev/hwrng as host input for <rng><backend model='random'/> device. This was added after various upstream discussions in commit 4932ef45
However this restriction has generated quite a few complaints over the years, so a new discussion was initiated:
http://www.redhat.com/archives/libvir-list/2016-April/msg00987.html
Several people suggested removing the restriction, and nobody really spoke up to defend it. So this patch drops the path restriction entirely
https://bugzilla.redhat.com/show_bug.cgi?id=1074464 --- docs/formatdomain.html.in | 9 ++++----- docs/schemas/domaincommon.rng | 3 +-- src/conf/domain_conf.c | 8 -------- tests/qemuxml2argvdata/qemuxml2argv-virtio-rng-random.args | 2 +- tests/qemuxml2argvdata/qemuxml2argv-virtio-rng-random.xml | 2 +- tests/qemuxml2xmloutdata/qemuxml2xmlout-virtio-rng-random.xml | 2 +- 6 files changed, 8 insertions(+), 18 deletions(-)
diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in index 895114b..132b261 100644 --- a/docs/formatdomain.html.in +++ b/docs/formatdomain.html.in
...
@@ -6240,9 +6239,9 @@ qemu-kvm -net nic,model=? /dev/null <dd> <p> This backend type expects a non-blocking character device as input. - The only accepted paths are /dev/random and /dev/hwrng. The file - name is specified as contents of the <code>backend</code> element. - When no file name is specified the hypervisor default is used. + The file name is specified as contents of the + <code>backend</code> element. When no file name is specified the + this defaults to /dev/random.
I'd rather keep the "hypervisor default" wording. /dev/urandom is in no way platform or hypervisor agnostic. You may add though that for qemu driver the default indeed is /dev/random. Peter

On Thu, 2016-04-21 at 10:06 +0200, Peter Krempa wrote:
On Wed, Apr 20, 2016 at 18:19:25 -0400, Cole Robinson wrote:
@@ -6240,9 +6239,9 @@ qemu-kvm -net nic,model=? /dev/null <dd> <p> This backend type expects a non-blocking character device as input. - The only accepted paths are /dev/random and /dev/hwrng. The file - name is specified as contents of the <code>backend</code> element. - When no file name is specified the hypervisor default is used. + The file name is specified as contents of the + <code>backend</code> element. When no file name is specified the + this defaults to /dev/random.
I'd rather keep the "hypervisor default" wording. /dev/urandom is in no way platform or hypervisor agnostic. You may add though that for qemu driver the default indeed is /dev/random.
Also, "the this defaults" is quite obviously a typo :) Cheers. -- Andrea Bolognani Software Engineer - Virtualization Team

On 04/21/2016 04:06 AM, Peter Krempa wrote:
On Wed, Apr 20, 2016 at 18:19:25 -0400, Cole Robinson wrote:
Currently we only allow /dev/random and /dev/hwrng as host input for <rng><backend model='random'/> device. This was added after various upstream discussions in commit 4932ef45
However this restriction has generated quite a few complaints over the years, so a new discussion was initiated:
http://www.redhat.com/archives/libvir-list/2016-April/msg00987.html
Several people suggested removing the restriction, and nobody really spoke up to defend it. So this patch drops the path restriction entirely
https://bugzilla.redhat.com/show_bug.cgi?id=1074464 --- docs/formatdomain.html.in | 9 ++++----- docs/schemas/domaincommon.rng | 3 +-- src/conf/domain_conf.c | 8 -------- tests/qemuxml2argvdata/qemuxml2argv-virtio-rng-random.args | 2 +- tests/qemuxml2argvdata/qemuxml2argv-virtio-rng-random.xml | 2 +- tests/qemuxml2xmloutdata/qemuxml2xmlout-virtio-rng-random.xml | 2 +- 6 files changed, 8 insertions(+), 18 deletions(-)
diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in index 895114b..132b261 100644 --- a/docs/formatdomain.html.in +++ b/docs/formatdomain.html.in
...
@@ -6240,9 +6239,9 @@ qemu-kvm -net nic,model=? /dev/null <dd> <p> This backend type expects a non-blocking character device as input. - The only accepted paths are /dev/random and /dev/hwrng. The file - name is specified as contents of the <code>backend</code> element. - When no file name is specified the hypervisor default is used. + The file name is specified as contents of the + <code>backend</code> element. When no file name is specified the + this defaults to /dev/random.
I'd rather keep the "hypervisor default" wording. /dev/urandom is in no way platform or hypervisor agnostic. You may add though that for qemu driver the default indeed is /dev/random.
Thanks, fixed locally. I'll wait till see if any other comments come in before deciding whether to post a V2 or not - Cole

On Wed, Apr 20, 2016 at 06:19:25PM -0400, Cole Robinson wrote:
Currently we only allow /dev/random and /dev/hwrng as host input for <rng><backend model='random'/> device. This was added after various upstream discussions in commit 4932ef45
However this restriction has generated quite a few complaints over the years, so a new discussion was initiated:
http://www.redhat.com/archives/libvir-list/2016-April/msg00987.html
Several people suggested removing the restriction, and nobody really spoke up to defend it. So this patch drops the path restriction entirely
ACK, despite explicit request for details, no one has been able to give a clear description of a security problem in using urandom. It has all just been hand-wavey assertions with nothing to back it up, against other people's analysis showing urandom to be safe. Regards, Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|

On 04/26/2016 11:07 AM, Daniel P. Berrange wrote:
On Wed, Apr 20, 2016 at 06:19:25PM -0400, Cole Robinson wrote:
Currently we only allow /dev/random and /dev/hwrng as host input for <rng><backend model='random'/> device. This was added after various upstream discussions in commit 4932ef45
However this restriction has generated quite a few complaints over the years, so a new discussion was initiated:
http://www.redhat.com/archives/libvir-list/2016-April/msg00987.html
Several people suggested removing the restriction, and nobody really spoke up to defend it. So this patch drops the path restriction entirely
ACK, despite explicit request for details, no one has been able to give a clear description of a security problem in using urandom. It has all just been hand-wavey assertions with nothing to back it up, against other people's analysis showing urandom to be safe.
Thanks, I've pushed the patch now - Cole
participants (5)
-
Andrea Bolognani
-
Cole Robinson
-
Daniel P. Berrange
-
Peter Krempa
-
Richard W.M. Jones