From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([140.186.70.92]:49168) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1QwCpU-0000yM-7p for qemu-devel@nongnu.org; Wed, 24 Aug 2011 08:45:13 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1QwCpS-0003Ax-Kz for qemu-devel@nongnu.org; Wed, 24 Aug 2011 08:45:12 -0400 Received: from mail-yi0-f45.google.com ([209.85.218.45]:40567) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1QwCpS-0003Al-HP for qemu-devel@nongnu.org; Wed, 24 Aug 2011 08:45:10 -0400 Received: by yih10 with SMTP id 10so927301yih.4 for ; Wed, 24 Aug 2011 05:45:09 -0700 (PDT) Message-ID: <4E54F252.7020007@codemonkey.ws> Date: Wed, 24 Aug 2011 07:45:06 -0500 From: Anthony Liguori MIME-Version: 1.0 References: <1314183661-14483-1-git-send-email-berrange@redhat.com> In-Reply-To: <1314183661-14483-1-git-send-email-berrange@redhat.com> Content-Type: text/plain; charset=ISO-8859-1; format=flowed Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH STABLE-0.14/0.15/master] CVE-2011-0011: fix VNC password change to not touch authentication settings List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: "Daniel P. Berrange" Cc: qemu-devel@nongnu.org On 08/24/2011 06:01 AM, Daniel P. Berrange wrote: > From: "Daniel P. Berrange" > > 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 > --- > 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);