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

[Anthony Liguori <anthony@codemonkey.ws>]

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