[Qemu-devel] [PATCH STABLE-0.14/0.15/master] CVE-2011-0011: fix VNC password change to not touch authentication settings

* [Qemu-devel] [PATCH STABLE-0.14/0.15/master] CVE-2011-0011: fix VNC password change to not touch authentication settings
@ 2011-08-24 11:01 Daniel P. Berrange
  2011-08-24 12:45 ` Anthony Liguori
  0 siblings, 1 reply; 6+ messages in thread
From: Daniel P. Berrange @ 2011-08-24 11:01 UTC (permalink / raw)
  To: qemu-devel

From: "Daniel P. Berrange" <berrange@redhat.com>

In CVE-2011-0011 it was noted that setting an empty password
would disable all authentication for the VNC password. Commit
1cd20f8bf0ecb9d1d1bd5e2ffab3b88835380c9b attempted to fix this
but it just broke it in a different way, because now instead
of blindly disabling all authentication, it blindly resets all
authentication to 'VNC'. This disables any TLS auth that might
have been enabled, which is pratically as bad as the original
problem.

eg, consider launching QEMU with TLS + password as per the
docs section 3.11.5

   $ qemu [...OPTIONS...] -vnc :1,password,tls -monitor stdio
   (qemu) info vnc
   Server:
        address: 0.0.0.0:5901
           auth: vencrypt+tls+vnc
   Client: none
   (qemu) change vnc password "123"
   (qemu) info vnc
   Server:
        address: 0.0.0.0:5901
           auth: vnc
   Client: none

After setting the password, the TLS auth has been disabled
meaning all communications are back in cleartext. The
'change vnc password' command must *never* touch the 'vs->auth'
field under any circumstances.

Similarly setting the password to "" (which causes all auth
attempts to fail) must *not* touch vs->auth, otherwise it
breaks the following sequence

   $ qemu [...OPTIONS...] -vnc :1,password,tls -monitor stdio
   (qemu) info vnc
   Server:
        address: 0.0.0.0:5901
           auth: vencrypt+tls+vnc
   Client: none
   (qemu) change vnc password "123"
   (qemu) info vnc
   Server:
        address: 0.0.0.0:5901
           auth: vencrypt+tls+vnc
   Client: none
   (qemu) change vnc password ""
   (qemu) info vnc
   Server:
        address: 0.0.0.0:5901
           auth: vnc
   Client: none
   (qemu) change vnc password "456"
   (qemu) info vnc
   Server:
        address: 0.0.0.0:5901
           auth: vnc
   Client: none

This patch puts the behaviour back to what it was before the
original mistaken commit 52c18be9e99dabe295321153fda7fce9f76647ac

* ui/vnc.c: Do not touch 'vs->auth' when changing password and
  remove unneccessary 'vnc_disable_login' method
* monitor.c: Remove call to 'vnc_disable_login'

Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
---
 console.h |    1 -
 monitor.c |    8 --------
 ui/vnc.c  |   30 +++---------------------------
 3 files changed, 3 insertions(+), 36 deletions(-)

diff --git a/console.h b/console.h
index 67d1373..2eb03a1 100644
--- a/console.h
+++ b/console.h
@@ -373,7 +373,6 @@ void vnc_display_init(DisplayState *ds);
 void vnc_display_close(DisplayState *ds);
 int vnc_display_open(DisplayState *ds, const char *display);
 void vnc_display_add_client(DisplayState *ds, int csock, int skipauth);
-int vnc_display_disable_login(DisplayState *ds);
 char *vnc_display_local_addr(DisplayState *ds);
 #ifdef CONFIG_VNC
 int vnc_display_password(DisplayState *ds, const char *password);
diff --git a/monitor.c b/monitor.c
index 1b8ba2c..59af05a 100644
--- a/monitor.c
+++ b/monitor.c
@@ -1023,14 +1023,6 @@ static int do_quit(Monitor *mon, const QDict *qdict, QObject **ret_data)
 #ifdef CONFIG_VNC
 static int change_vnc_password(const char *password)
 {
-    if (!password || !password[0]) {
-        if (vnc_display_disable_login(NULL)) {
-            qerror_report(QERR_SET_PASSWD_FAILED);
-            return -1;
-        }
-        return 0;
-    }
-
     if (vnc_display_password(NULL, password) < 0) {
         qerror_report(QERR_SET_PASSWD_FAILED);
         return -1;
diff --git a/ui/vnc.c b/ui/vnc.c
index f1e27d9..f7fc7d2 100644
--- a/ui/vnc.c
+++ b/ui/vnc.c
@@ -2635,24 +2635,6 @@ void vnc_display_close(DisplayState *ds)
 #endif
 }
 
-int vnc_display_disable_login(DisplayState *ds)
-{
-    VncDisplay *vs = ds ? (VncDisplay *)ds->opaque : vnc_display;
-
-    if (!vs) {
-        return -1;
-    }
-
-    if (vs->password) {
-        qemu_free(vs->password);
-    }
-
-    vs->password = NULL;
-    vs->auth = VNC_AUTH_VNC;
-
-    return 0;
-}
-
 int vnc_display_password(DisplayState *ds, const char *password)
 {
     int ret = 0;
@@ -2663,19 +2645,13 @@ int vnc_display_password(DisplayState *ds, const char *password)
         goto out;
     }
 
-    if (!password) {
-        /* This is not the intention of this interface but err on the side
-           of being safe */
-        ret = vnc_display_disable_login(ds);
-        goto out;
-    }
-
     if (vs->password) {
         qemu_free(vs->password);
         vs->password = NULL;
     }
-    vs->password = qemu_strdup(password);
-    vs->auth = VNC_AUTH_VNC;
+    if (password)
+	vs->password = qemu_strdup(password);
+
 out:
     if (ret != 0) {
         qerror_report(QERR_SET_PASSWD_FAILED);
-- 
1.7.6

^ permalink raw reply related	[flat|nested] 6+ messages in thread