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

login
register
mail settings
Submitter Daniel P. Berrange
Date Aug. 24, 2011, 11:01 a.m.
Message ID <1314183661-14483-1-git-send-email-berrange@redhat.com>
Download mbox | patch
Permalink /patch/111308/
State New
Headers show

Comments

Daniel P. Berrange - Aug. 24, 2011, 11:01 a.m.
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(-)
Anthony Liguori - Aug. 24, 2011, 12:45 p.m.
On 08/24/2011 06:01 AM, Daniel P. Berrange wrote:
> 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'.

But this is *not* a security problem.  Login becomes disabled as expected.

We should really not overload the semantics of the change command like 
this and instead introduce a new QMP operation to disable login.

> 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);
> +

This breaks checkpatch.pl

Regards,

Anthony Liguori

>   out:
>       if (ret != 0) {
>           qerror_report(QERR_SET_PASSWD_FAILED);
Daniel P. Berrange - Aug. 24, 2011, 12:50 p.m.
On Wed, Aug 24, 2011 at 07:45:06AM -0500, Anthony Liguori wrote:
> On 08/24/2011 06:01 AM, Daniel P. Berrange wrote:
> >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'.
> 
> But this is *not* a security problem.  Login becomes disabled as expected.

It *is* a security problem, because if you do

  change vnc password 123
  change vnc password ""
  change vnc password 456

you have lost the authentication settings you requested.

With this patch, changing the password to "" *still* disables
the login, without side effects on the auth scheme.

> We should really not overload the semantics of the change command
> like this and instead introduce a new QMP operation to disable
> login.

This change I mention below is the one that overloaded the semantics
by making a password change, also change the auth scheme, breaking
the original behaviour.  If we want apps to be able to change the
auth scheme that needs a separate monitor command.

The current behaviour is not usable and introduces a security problem
by changing auth scheme without being asked to.

Daniel
Anthony Liguori - Aug. 24, 2011, 12:55 p.m.
On 08/24/2011 07:50 AM, Daniel P. Berrange wrote:
> On Wed, Aug 24, 2011 at 07:45:06AM -0500, Anthony Liguori wrote:
>> On 08/24/2011 06:01 AM, Daniel P. Berrange wrote:
>>> 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'.
>>
>> But this is *not* a security problem.  Login becomes disabled as expected.
>
> It *is* a security problem, because if you do
>
>    change vnc password 123
>    change vnc password ""
>    change vnc password 456
>
> you have lost the authentication settings you requested.
>
> With this patch, changing the password to "" *still* disables
> the login, without side effects on the auth scheme.

Just because it isn't doing what you expect it to do doesn't make it a 
security problem.  This is the current behavior and you simply cannot 
write a management tool without being aware of this behavior for better 
or worse.

The password change interface should not be overloaded to deal disable 
login.  There should be a higher level QMP command to do this.

>
>> We should really not overload the semantics of the change command
>> like this and instead introduce a new QMP operation to disable
>> login.
>
> This change I mention below is the one that overloaded the semantics
> by making a password change, also change the auth scheme, breaking
> the original behaviour.  If we want apps to be able to change the
> auth scheme that needs a separate monitor command.
>
> The current behaviour is not usable and introduces a security problem
> by changing auth scheme without being asked to.

I'll buy an argument about usability but not about security.  We need a 
higher level command to disable login and a higher level command to set 
the vnc password.  This interface should be considered deprecated.

Regards,

Anthony Liguori

> Daniel
Daniel P. Berrange - Aug. 24, 2011, 1:02 p.m.
On Wed, Aug 24, 2011 at 07:55:38AM -0500, Anthony Liguori wrote:
> On 08/24/2011 07:50 AM, Daniel P. Berrange wrote:
> >On Wed, Aug 24, 2011 at 07:45:06AM -0500, Anthony Liguori wrote:
> >>On 08/24/2011 06:01 AM, Daniel P. Berrange wrote:
> >>>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'.
> >>
> >>But this is *not* a security problem.  Login becomes disabled as expected.
> >
> >It *is* a security problem, because if you do
> >
> >   change vnc password 123
> >   change vnc password ""
> >   change vnc password 456
> >
> >you have lost the authentication settings you requested.
> >
> >With this patch, changing the password to "" *still* disables
> >the login, without side effects on the auth scheme.
> 
> Just because it isn't doing what you expect it to do doesn't make it
> a security problem.  This is the current behavior and you simply
> cannot write a management tool without being aware of this behavior
> for better or worse.

This was *not* the behaviour for many releases. It is a regression
against the original behaviour of the change vnc password in QEMU
which we had succesfully worked with in libvirt since password+TLS
support was written for QEMU. The current behaviour is unusably
broken. It cannot be used without creating a security problem, where
as the original QEMU behaviour was succesfully usable. Simply saying
that we must create a new command, instead of fixing the QEMU regression
does nothing to help existing apps which are expecting current QEMU
releases to work as previous releases did & as the command is
*documented* :

  http://qemu.weilnetz.de/qemu-doc.html#vnc_005fsec_005fcertificate_005fpw

  [quote]
  3.11.5 With x509 certificates, client verification and passwords

  Finally, the previous method can be combined with VNC password authentication to provide two layers of authentication for clients.

  qemu [...OPTIONS...] -vnc :1,password,tls,x509verify=/etc/pki/qemu -monitor stdio
  (qemu) change vnc password
  Password: ********
  (qemu)
  [/quote]

This documented example no longer works because authentication is being
silently reset.

Daniel
Gerd Hoffmann - Aug. 24, 2011, 2:52 p.m.
Hi,

> I'll buy an argument about usability but not about security. We need a
> higher level command to disable login

expire_password vnc now

> and a higher level command to set
> the vnc password. This interface should be considered deprecated.

set_password vnc secret

HTH,
   Gerd

Patch

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);