From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from [140.186.70.92] (port=58541 helo=eggs.gnu.org) by lists.gnu.org with esmtp (Exim 4.43) id 1PoziC-0007DH-6u for qemu-devel@nongnu.org; Mon, 14 Feb 2011 09:47:37 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1PoziA-0003bq-EQ for qemu-devel@nongnu.org; Mon, 14 Feb 2011 09:47:36 -0500 Received: from mx1.redhat.com ([209.132.183.28]:13676) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1PoziA-0003bZ-4G for qemu-devel@nongnu.org; Mon, 14 Feb 2011 09:47:34 -0500 Date: Mon, 14 Feb 2011 14:47:26 +0000 From: "Daniel P. Berrange" Message-ID: <20110214144726.GL2729@redhat.com> References: <1296506599-7126-1-git-send-email-aliguori@us.ibm.com> <4D590A93.8080407@redhat.com> <4D591B9C.8060705@linux.vnet.ibm.com> <20110214122410.GG2729@redhat.com> <4D5938AF.904@linux.vnet.ibm.com> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: <4D5938AF.904@linux.vnet.ibm.com> Subject: [Qemu-devel] Re: [PATCH] vnc: Fix password expiration through 'change vnc ""' Reply-To: "Daniel P. Berrange" List-Id: qemu-devel.nongnu.org List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Anthony Liguori Cc: Neil Wilson , Gerd Hoffmann , qemu-devel@nongnu.org On Mon, Feb 14, 2011 at 08:14:07AM -0600, Anthony Liguori wrote: > On 02/14/2011 06:24 AM, Daniel P. Berrange wrote: > >On Mon, Feb 14, 2011 at 06:10:04AM -0600, Anthony Liguori wrote: > >>On 02/14/2011 04:57 AM, Gerd Hoffmann wrote: > >>>On 01/31/11 21:43, Anthony Liguori wrote: > >>>>commit 52c18be9e99dabe295321153fda7fce9f76647ac introduced a > >>>>regression in the > >>>>change vnc password command that changed the behavior of setting the VNC > >>>>password to an empty string from disabling login to disabling > >>>>authentication. > >>>> > >>>>This commit refactors the code to eliminate this overloaded semantics in > >>>>vnc_display_password and instead introduces the > >>>>vnc_display_disable_login. The > >>>>monitor implementation then determines the behavior of an empty > >>>>or missing > >>>>string. > >>>Hmm, now about simply never ever changing vs->auth? > >>If auth is none and you do a vnc change password "" then if we don't > >>set vs->auth to vnc, it won't have the desired effect. I really > >>dislike the semantics of this command but that was a past mistake. > >Actually blindly setting 'vs->auth' to 'vnc' is also a security flaw. > > But this is the semantics of the command. I agree it's stupid but a > security flaw is a regression and this is not a regression. > > This is why the set-password command no longer does any of this nonsense. > > >If using the VeNCrypt security method, then 'vs->auth' will be VENCRYPT > >and the 'vs->subauth' will possibly indicate the 'VNC' sub-auth scheme. > >So we really do want the change password command to leave 'vs->auth' > >alone completely - just change the password string, with no side effects > >on auth methods. If an app intends to use the change password command > >it will have already launched QEMU with neccessary -vnc flags to set the > >desired vs->auth and vs->subauth methods. > > I think I see how this could work but I'm not sure it's worth doing. > I'd rather just leave the (bad) semantics of this command alone and > deprecate the interface. If you make it blindly set 'vs->auth = VNC' then you haven't fully fixed the security flawed, because you will be downgrading from 'vencrypt+vnc' auth to just 'vnc' which loose all your data encryption capabilities. Just reverting the previous commit 52c18be9e99dabe295321153fda7fce9f76647ac fully addresses the problems leaving the command side-effect free as it was originally designed & implemented. Regards, Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|