[libvirt PATCH] qemu: validate VNC password length

Daniel P. Berrangé posted 1 patch 2 years, 4 months ago
Test syntax-check failed
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/libvirt tags/patchew/20211216104853.3568446-1-berrange@redhat.com
src/qemu/qemu_conf.c     | 6 ++++++
src/qemu/qemu_validate.c | 8 ++++++++
2 files changed, 14 insertions(+)
[libvirt PATCH] qemu: validate VNC password length
Posted by Daniel P. Berrangé 2 years, 4 months ago
The VNC password authentication scheme is quite horrendous in that it
takes the user password and directly uses it as a DES case. DES is a
byte 8 keyed cipher, so the VNC password can never be more than 8
characters long. Anything over that length will be silently dropped.

We should validate this length restriction when accepting user XML
configs and report an error. For the global VNC password we don't
really want to break daemon startup by reporting an error, but
logging a warning is worthwhile.

https://bugzilla.redhat.com/show_bug.cgi?id=1506689
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
---
 src/qemu/qemu_conf.c     | 6 ++++++
 src/qemu/qemu_validate.c | 8 ++++++++
 2 files changed, 14 insertions(+)

diff --git a/src/qemu/qemu_conf.c b/src/qemu/qemu_conf.c
index 7eb04e66a0..6077457ff4 100644
--- a/src/qemu/qemu_conf.c
+++ b/src/qemu/qemu_conf.c
@@ -451,6 +451,12 @@ virQEMUDriverConfigLoadVNCEntry(virQEMUDriverConfig *cfg,
     if (virConfGetValueBool(conf, "vnc_allow_host_audio", &cfg->vncAllowHostAudio) < 0)
         return -1;
 
+    if (cfg->vncPassword &&
+        strlen(cfg->vncPassword) > 8) {
+        VIR_WARN("VNC password is %zu characters long, only 8 permitted, truncating",
+                 strlen(cfg->vncPassword));
+        cfg->vncPassword[8] = '\0';
+    }
     return 0;
 }
 
diff --git a/src/qemu/qemu_validate.c b/src/qemu/qemu_validate.c
index f9a195e991..46b40303f6 100644
--- a/src/qemu/qemu_validate.c
+++ b/src/qemu/qemu_validate.c
@@ -4109,6 +4109,14 @@ qemuValidateDomainDeviceDefVNCGraphics(const virDomainGraphicsDef *graphics,
         return -1;
     }
 
+    if (graphics->data.vnc.auth.passwd &&
+        strlen(graphics->data.vnc.auth.passwd) > 8) {
+        virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
+                       _("VNC password is %zu characters long, only 8 permitted"),
+                       strlen(graphics->data.vnc.auth.passwd));
+        return -1;
+    }
+
     return 0;
 }
 
-- 
2.33.1

Re: [libvirt PATCH] qemu: validate VNC password length
Posted by Pavel Hrdina 2 years, 4 months ago
On Thu, Dec 16, 2021 at 10:48:53AM +0000, Daniel P. Berrangé wrote:
> The VNC password authentication scheme is quite horrendous in that it
> takes the user password and directly uses it as a DES case. DES is a
> byte 8 keyed cipher, so the VNC password can never be more than 8
> characters long. Anything over that length will be silently dropped.
> 
> We should validate this length restriction when accepting user XML
> configs and report an error. For the global VNC password we don't
> really want to break daemon startup by reporting an error, but
> logging a warning is worthwhile.
> 
> https://bugzilla.redhat.com/show_bug.cgi?id=1506689
> Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
> ---
>  src/qemu/qemu_conf.c     | 6 ++++++
>  src/qemu/qemu_validate.c | 8 ++++++++
>  2 files changed, 14 insertions(+)

Reviewed-by: Pavel Hrdina <phrdina@redhat.com>