ioemu: empty vnc passwd

ioemu: empty vnc passwd

[Samuel Thibault]

Hello,

There is a small bug in xenstore.c: the following patch is needed
because else xenstore_read_vncpasswd would return 0 even when it is
unable to read the passwd.

diff -r 9e92672385a5 tools/ioemu/xenstore.c
--- a/tools/ioemu/xenstore.c	Wed Jan 23 13:37:03 2008 +0000
+++ b/tools/ioemu/xenstore.c	Wed Jan 23 15:53:01 2008 +0000
@@ -518,7 +518,7 @@ int xenstore_read_vncpasswd(int domid, c
         pwbuf[0] = '\0';
         free(uuid);
         free(path);
-        return rc;
+        return -1;
     }
 
     for (i=0; i<len && i<pwbuflen; i++) {

However, that means we can't use an empty passwd any more, while that
may be quite useful e.g. in testing environments, so that we would need
the following patch:

diff -r 9e92672385a5 tools/ioemu/vl.c
--- a/tools/ioemu/vl.c	Wed Jan 23 13:37:03 2008 +0000
+++ b/tools/ioemu/vl.c	Wed Jan 23 15:55:38 2008 +0000
@@ -7756,8 +7756,7 @@ int main(int argc, char **argv)
 	int vnc_display_port;
 	char password[20];
 	vnc_display_init(ds);
-	if (xenstore_read_vncpasswd(domid, password, sizeof(password)) < 0)
-	    exit(0);
+	xenstore_read_vncpasswd(domid, password, sizeof(password));
 	vnc_display_password(ds, password);
 	if ((vnc_display_port = vnc_display_open(ds, vnc_display, vncunused)) < 0) 
 	    exit (0);

in order to just ignore a missing passwd.
What do people think about that?

Signed-off-by: Samuel Thibault <samuel.thibault@eu.citrix.com>

Re: ioemu: empty vnc passwd

[Christoph Egger]

If we do a debug build let us assume we are in a testing environment.
There an empty vnc password is ok.
If we don't make a debug build, let us assume we are in a production 
environment where an empty vnc password is a security risk.

Christoph


On Wednesday 23 January 2008 17:11:30 Samuel Thibault wrote:
> Hello,
>
> There is a small bug in xenstore.c: the following patch is needed
> because else xenstore_read_vncpasswd would return 0 even when it is
> unable to read the passwd.
>
> diff -r 9e92672385a5 tools/ioemu/xenstore.c
> --- a/tools/ioemu/xenstore.c	Wed Jan 23 13:37:03 2008 +0000
> +++ b/tools/ioemu/xenstore.c	Wed Jan 23 15:53:01 2008 +0000
> @@ -518,7 +518,7 @@ int xenstore_read_vncpasswd(int domid, c
>          pwbuf[0] = '\0';
>          free(uuid);
>          free(path);
> -        return rc;
> +        return -1;
>      }
>
>      for (i=0; i<len && i<pwbuflen; i++) {
>
> However, that means we can't use an empty passwd any more, while that
> may be quite useful e.g. in testing environments, so that we would need
> the following patch:
>
> diff -r 9e92672385a5 tools/ioemu/vl.c
> --- a/tools/ioemu/vl.c	Wed Jan 23 13:37:03 2008 +0000
> +++ b/tools/ioemu/vl.c	Wed Jan 23 15:55:38 2008 +0000
> @@ -7756,8 +7756,7 @@ int main(int argc, char **argv)
>  	int vnc_display_port;
>  	char password[20];
>  	vnc_display_init(ds);
> -	if (xenstore_read_vncpasswd(domid, password, sizeof(password)) < 0)
> -	    exit(0);
> +	xenstore_read_vncpasswd(domid, password, sizeof(password));
>  	vnc_display_password(ds, password);
>  	if ((vnc_display_port = vnc_display_open(ds, vnc_display, vncunused)) <
> 0) exit (0);
>
> in order to just ignore a missing passwd.
> What do people think about that?
>
> Signed-off-by: Samuel Thibault <samuel.thibault@eu.citrix.com>
>
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xensource.com
> http://lists.xensource.com/xen-devel



-- 
AMD Saxony, Dresden, Germany
Operating System Research Center

Legal Information:
AMD Saxony Limited Liability Company & Co. KG
Sitz (Geschäftsanschrift):
   Wilschdorfer Landstr. 101, 01109 Dresden, Deutschland
Registergericht Dresden: HRA 4896
vertretungsberechtigter Komplementär:
   AMD Saxony LLC (Sitz Wilmington, Delaware, USA)
Geschäftsführer der AMD Saxony LLC:
   Dr. Hans-R. Deppe, Thomas McCoy

Re: ioemu: empty vnc passwd

[John Levon]

On Wed, Jan 23, 2008 at 05:19:33PM +0100, Christoph Egger wrote:

> If we do a debug build let us assume we are in a testing environment.
> There an empty vnc password is ok.
> If we don't make a debug build, let us assume we are in a production 
> environment where an empty vnc password is a security risk.

I don't agree with this logic. For example, testing teams will often use
non-debug builds, but still want the convenience of not having to make a
password

regards
john

Re: ioemu: empty vnc passwd

[Daniel P. Berrange]

On Wed, Jan 23, 2008 at 05:19:33PM +0100, Christoph Egger wrote:
> 
> If we do a debug build let us assume we are in a testing environment.
> There an empty vnc password is ok.
> If we don't make a debug build, let us assume we are in a production 
> environment where an empty vnc password is a security risk.

That logic is flawed. VNC may be configured to use TLS +x509 certificates
which provide real security. A VNC passwd is not really very credible 
security whether its zero or 8 chars in length. It shouldn't try to
second guess what an admin wants. 

VNC password authentication is turned on / off via the ',passwd' flag on
the -vnc command line to QEMU. If password auth is on, and a zero length 
string is found as a password, then all logins are completely disabled - 
the VNC password auth code will fail all logins. If passwd auth is off on 
the  command line, then any password stored in xenstore is irrelevant, no
matter what length it is.

Dan.
-- 
|=- Red Hat, Engineering, Emerging Technologies, Boston.  +1 978 392 2496 -=|
|=-           Perl modules: http://search.cpan.org/~danberr/              -=|
|=-               Projects: http://freshmeat.net/~danielpb/               -=|
|=-  GnuPG: 7D3B9505   F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505  -=| 

Re: ioemu: empty vnc passwd

[Christoph Egger]

On Wednesday 23 January 2008 17:28:11 Daniel P. Berrange wrote:
> On Wed, Jan 23, 2008 at 05:19:33PM +0100, Christoph Egger wrote:
> > If we do a debug build let us assume we are in a testing environment.
> > There an empty vnc password is ok.
> > If we don't make a debug build, let us assume we are in a production
> > environment where an empty vnc password is a security risk.
>
> That logic is flawed. VNC may be configured to use TLS +x509 certificates
> which provide real security. A VNC passwd is not really very credible
> security whether its zero or 8 chars in length. It shouldn't try to
> second guess what an admin wants.

That's right. vnc-auth is nothing. TLS (vnc security type 18) and
Tight (vnc security type 16) are much better.

> VNC password authentication is turned on / off via the ',passwd' flag on
> the -vnc command line to QEMU. If password auth is on, and a zero length
> string is found as a password, then all logins are completely disabled -
> the VNC password auth code will fail all logins. If passwd auth is off on
> the  command line, then any password stored in xenstore is irrelevant, no
> matter what length it is.
>
> Dan.



-- 
AMD Saxony, Dresden, Germany
Operating System Research Center

Legal Information:
AMD Saxony Limited Liability Company & Co. KG
Sitz (Geschäftsanschrift):
   Wilschdorfer Landstr. 101, 01109 Dresden, Deutschland
Registergericht Dresden: HRA 4896
vertretungsberechtigter Komplementär:
   AMD Saxony LLC (Sitz Wilmington, Delaware, USA)
Geschäftsführer der AMD Saxony LLC:
   Dr. Hans-R. Deppe, Thomas McCoy

Re: ioemu: empty vnc passwd

[Daniel P. Berrange]

On Wed, Jan 23, 2008 at 05:36:07PM +0100, Christoph Egger wrote:
> On Wednesday 23 January 2008 17:28:11 Daniel P. Berrange wrote:
> > On Wed, Jan 23, 2008 at 05:19:33PM +0100, Christoph Egger wrote:
> > > If we do a debug build let us assume we are in a testing environment.
> > > There an empty vnc password is ok.
> > > If we don't make a debug build, let us assume we are in a production
> > > environment where an empty vnc password is a security risk.
> >
> > That logic is flawed. VNC may be configured to use TLS +x509 certificates
> > which provide real security. A VNC passwd is not really very credible
> > security whether its zero or 8 chars in length. It shouldn't try to
> > second guess what an admin wants.
> 
> That's right. vnc-auth is nothing. TLS (vnc security type 18) and
> Tight (vnc security type 16) are much better.

Not really - the TLS auth scheme merely uses anonymous credentials, so while
the data stream is encrypted, it is susceptible to trivial MITM attack at
the time of connection setup.  QEMU implements the VeNCrypt security type
which uses TLS with x509 certificates, and will also optionally mandate that
the client presents their own certificate upon connecting.  This is supported
by virt-manager, vinagre, vencrypt's vncviewer and any client using GTK-VNC

See the QEMU docs for how this is all configured

   http://fabrice.bellard.free.fr/qemu/qemu-doc.html#SEC36

As for the Tight auth type, I've no idea what that does, but its not
implemented in current tightvnc releases - they only support the flawed
DES vnc password auth:

   http://www.tightvnc.com/faq.html#howsecure

  "Although TightVNC encrypts VNC passwords sent over the net, the rest 
   of the traffic is sent as is, unencrypted (for password encryption, 
   VNC uses a DES-encrypted challenge-response scheme, where the password 
   is limited by 8 characters, and the effective DES key length is 56 bits).
   So using TightVNC over the Internet can be a security risk"

Regards,
Dan.
-- 
|=- Red Hat, Engineering, Emerging Technologies, Boston.  +1 978 392 2496 -=|
|=-           Perl modules: http://search.cpan.org/~danberr/              -=|
|=-               Projects: http://freshmeat.net/~danielpb/               -=|
|=-  GnuPG: 7D3B9505   F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505  -=| 

Re: ioemu: empty vnc passwd

[Samuel Thibault]

Daniel P. Berrange, le Wed 23 Jan 2008 16:28:11 +0000, a écrit :
> VNC password authentication is turned on / off via the ',passwd' flag on
> the -vnc command line to QEMU. If password auth is on, and a zero length 
> string is found as a password, then all logins are completely disabled - 
> the VNC password auth code will fail all logins. If passwd auth is off on 
> the  command line, then any password stored in xenstore is irrelevant, no
> matter what length it is.

Ok, so the real fix seems to be to take that flag into account (which is
not the case currently).

Samuel

Re: ioemu: empty vnc passwd

[Daniel P. Berrange]

On Wed, Jan 23, 2008 at 04:42:33PM +0000, Samuel Thibault wrote:
> Daniel P. Berrange, le Wed 23 Jan 2008 16:28:11 +0000, a écrit :
> > VNC password authentication is turned on / off via the ',passwd' flag on
> > the -vnc command line to QEMU. If password auth is on, and a zero length 
> > string is found as a password, then all logins are completely disabled - 
> > the VNC password auth code will fail all logins. If passwd auth is off on 
> > the  command line, then any password stored in xenstore is irrelevant, no
> > matter what length it is.
> 
> Ok, so the real fix seems to be to take that flag into account (which is
> not the case currently).

Urm, its already processed. See vnc_display_open() in tools/ioemu/vnc.c

Dan.
-- 
|=- Red Hat, Engineering, Emerging Technologies, Boston.  +1 978 392 2496 -=|
|=-           Perl modules: http://search.cpan.org/~danberr/              -=|
|=-               Projects: http://freshmeat.net/~danielpb/               -=|
|=-  GnuPG: 7D3B9505   F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505  -=| 

Re: ioemu: empty vnc passwd

[Samuel Thibault]

Daniel P. Berrange, le Wed 23 Jan 2008 16:50:15 +0000, a écrit :
> On Wed, Jan 23, 2008 at 04:42:33PM +0000, Samuel Thibault wrote:
> > Daniel P. Berrange, le Wed 23 Jan 2008 16:28:11 +0000, a écrit :
> > > VNC password authentication is turned on / off via the ',passwd' flag on
> > > the -vnc command line to QEMU. If password auth is on, and a zero length 
> > > string is found as a password, then all logins are completely disabled - 
> > > the VNC password auth code will fail all logins. If passwd auth is off on 
> > > the  command line, then any password stored in xenstore is irrelevant, no
> > > matter what length it is.
> > 
> > Ok, so the real fix seems to be to take that flag into account (which is
> > not the case currently).
> 
> Urm, its already processed. See vnc_display_open() in tools/ioemu/vnc.c

Yes, but that doesn't prevent the exit() when the passwd can't be read
from XenStore.

Samuel

Re: ioemu: empty vnc passwd

[Samuel Thibault]

Samuel Thibault, le Wed 23 Jan 2008 16:42:33 +0000, a écrit :
> Daniel P. Berrange, le Wed 23 Jan 2008 16:28:11 +0000, a écrit :
> > VNC password authentication is turned on / off via the ',passwd' flag on
> > the -vnc command line to QEMU. If password auth is on, and a zero length 
> > string is found as a password, then all logins are completely disabled - 
> > the VNC password auth code will fail all logins. If passwd auth is off on 
> > the  command line, then any password stored in xenstore is irrelevant, no
> > matter what length it is.
> 
> Ok, so the real fix seems to be to take that flag into account (which is
> not the case currently).

Which actually boils down to applying the two patches I have proposed:
on a xenstore read failure, an empty password is stored (which is fine
when there is no passwd in the configuration), and hence if ',passwd'
was given on the -vnc command line (i.e. some passwd was given in the
configuration but it somehow didn't make through to xenstore), all
logins will be completely disabled, so we're on the safe side.

Samuel

Re: ioemu: empty vnc passwd

[Daniel P. Berrange]

On Wed, Jan 23, 2008 at 04:50:39PM +0000, Samuel Thibault wrote:
> Samuel Thibault, le Wed 23 Jan 2008 16:42:33 +0000, a écrit :
> > Daniel P. Berrange, le Wed 23 Jan 2008 16:28:11 +0000, a écrit :
> > > VNC password authentication is turned on / off via the ',passwd' flag on
> > > the -vnc command line to QEMU. If password auth is on, and a zero length 
> > > string is found as a password, then all logins are completely disabled - 
> > > the VNC password auth code will fail all logins. If passwd auth is off on 
> > > the  command line, then any password stored in xenstore is irrelevant, no
> > > matter what length it is.
> > 
> > Ok, so the real fix seems to be to take that flag into account (which is
> > not the case currently).
> 
> Which actually boils down to applying the two patches I have proposed:
> on a xenstore read failure, an empty password is stored (which is fine
> when there is no passwd in the configuration), and hence if ',passwd'
> was given on the -vnc command line (i.e. some passwd was given in the
> configuration but it somehow didn't make through to xenstore), all
> logins will be completely disabled, so we're on the safe side.

Yes, that sounds like correct behaviour - if password goes missing from
xenstore then clients are rejected

Dan.
-- 
|=- Red Hat, Engineering, Emerging Technologies, Boston.  +1 978 392 2496 -=|
|=-           Perl modules: http://search.cpan.org/~danberr/              -=|
|=-               Projects: http://freshmeat.net/~danielpb/               -=|
|=-  GnuPG: 7D3B9505   F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505  -=| 

Re: ioemu: empty vnc passwd

[Daniel P. Berrange]

On Wed, Jan 23, 2008 at 04:11:30PM +0000, Samuel Thibault wrote:
> Hello,
> 
> There is a small bug in xenstore.c: the following patch is needed
> because else xenstore_read_vncpasswd would return 0 even when it is
> unable to read the passwd.
> 
> diff -r 9e92672385a5 tools/ioemu/xenstore.c
> --- a/tools/ioemu/xenstore.c	Wed Jan 23 13:37:03 2008 +0000
> +++ b/tools/ioemu/xenstore.c	Wed Jan 23 15:53:01 2008 +0000
> @@ -518,7 +518,7 @@ int xenstore_read_vncpasswd(int domid, c
>          pwbuf[0] = '\0';
>          free(uuid);
>          free(path);
> -        return rc;
> +        return -1;
>      }
>  
>      for (i=0; i<len && i<pwbuflen; i++) {
> 
> However, that means we can't use an empty passwd any more, while that
> may be quite useful e.g. in testing environments, so that we would need
> the following patch:

This is handled in XenD.

  - If a non-empty passwd is found in the guest config, or in the
    global XenD config,, then qemu has  -vnc :1,passwd  on its CLI
    to turn on password auth.
  - If there is no guest password and no global XenD password,
    then   qemu has  -vnc :1   and no password auth is enabled
    at all.

If you were to turn on password auth, and the password is a zero
length string, then all clients would be rejected, because the QEMU
VNC server explicitly fails password auth for "". This shouldn't
occur, because XenD should't have turned on password auth in the
first place if the guest password was ""

Regards,
Dan.
-- 
|=- Red Hat, Engineering, Emerging Technologies, Boston.  +1 978 392 2496 -=|
|=-           Perl modules: http://search.cpan.org/~danberr/              -=|
|=-               Projects: http://freshmeat.net/~danielpb/               -=|
|=-  GnuPG: 7D3B9505   F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505  -=| 

Re: ioemu: empty vnc passwd

[John Levon]

On Wed, Jan 23, 2008 at 04:35:55PM +0000, Daniel P. Berrange wrote:

> > However, that means we can't use an empty passwd any more, while that
> > may be quite useful e.g. in testing environments, so that we would need
> > the following patch:
> 
> This is handled in XenD.
> 
>   - If a non-empty passwd is found in the guest config, or in the
>     global XenD config,, then qemu has  -vnc :1,passwd  on its CLI
>     to turn on password auth.
>   - If there is no guest password and no global XenD password,
>     then   qemu has  -vnc :1   and no password auth is enabled
>     at all.

I'm confused: if there's no config or xend password at all, then the
domain won't start:

            if vncpasswd is None:
                raise VmError('vncpasswd is not setup in vmconfig or '
                              'xend-config.sxp')

regards
john

Re: ioemu: empty vnc passwd

[Daniel P. Berrange]

On Wed, Jan 23, 2008 at 05:26:14PM +0000, John Levon wrote:
> On Wed, Jan 23, 2008 at 04:35:55PM +0000, Daniel P. Berrange wrote:
> 
> > > However, that means we can't use an empty passwd any more, while that
> > > may be quite useful e.g. in testing environments, so that we would need
> > > the following patch:
> > 
> > This is handled in XenD.
> > 
> >   - If a non-empty passwd is found in the guest config, or in the
> >     global XenD config,, then qemu has  -vnc :1,passwd  on its CLI
> >     to turn on password auth.
> >   - If there is no guest password and no global XenD password,
> >     then   qemu has  -vnc :1   and no password auth is enabled
> >     at all.
> 
> I'm confused: if there's no config or xend password at all, then the
> domain won't start:
> 
>             if vncpasswd is None:
>                 raise VmError('vncpasswd is not setup in vmconfig or '
>                               'xend-config.sxp')

Sorry, my bad description - by no xend password, i meant the default
xend-config.sxp which is in fact  ''.  Frankly this check above is
a waste of time - it should just treat None as ""

Dan.
-- 
|=- Red Hat, Engineering, Emerging Technologies, Boston.  +1 978 392 2496 -=|
|=-           Perl modules: http://search.cpan.org/~danberr/              -=|
|=-               Projects: http://freshmeat.net/~danielpb/               -=|
|=-  GnuPG: 7D3B9505   F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505  -=| 

Re: ioemu: empty vnc passwd

[John Levon]

On Wed, Jan 23, 2008 at 05:33:41PM +0000, Daniel P. Berrange wrote:

> > I'm confused: if there's no config or xend password at all, then the
> > domain won't start:
> > 
> >             if vncpasswd is None:
> >                 raise VmError('vncpasswd is not setup in vmconfig or '
> >                               'xend-config.sxp')
> 
> Sorry, my bad description - by no xend password, i meant the default
> xend-config.sxp which is in fact  ''.  Frankly this check above is
> a waste of time - it should just treat None as ""

Except on Solaris we don't have such a default - the user's forced to
set something (there doesn't seem to be even a vaguely secure default?)

regards
john

Re: ioemu: empty vnc passwd

[Daniel P. Berrange]

On Wed, Jan 23, 2008 at 05:38:20PM +0000, John Levon wrote:
> On Wed, Jan 23, 2008 at 05:33:41PM +0000, Daniel P. Berrange wrote:
> 
> > > I'm confused: if there's no config or xend password at all, then the
> > > domain won't start:
> > > 
> > >             if vncpasswd is None:
> > >                 raise VmError('vncpasswd is not setup in vmconfig or '
> > >                               'xend-config.sxp')
> > 
> > Sorry, my bad description - by no xend password, i meant the default
> > xend-config.sxp which is in fact  ''.  Frankly this check above is
> > a waste of time - it should just treat None as ""
> 
> Except on Solaris we don't have such a default - the user's forced to
> set something (there doesn't seem to be even a vaguely secure default?)

There's no sane default for VNC passwords - whether you have on or not
its still basically insecure due to design of the VNC auth, hence the
config just defaults to '' & 127.0.0.1 which is as good as you'll get 
for VNC over TCP. 

If we wanted a real secure out of the box setup, we'd need to make XenD 
only expose the VNC server as a UNIX domain socket, so that access can
be restricted to root. QEMU has this ability already - we simply don't
use it in Xen. Of course no VNC client knows how to connect to a VNC 
server over a UNIX domain socket directly. You can use netcat + ssh to
tunnel to/from a remote host. I could also extend GTK-VNC & virt-manager
and/or virt-viewer to support it pretty easily.

Regards,
Dan.
-- 
|=- Red Hat, Engineering, Emerging Technologies, Boston.  +1 978 392 2496 -=|
|=-           Perl modules: http://search.cpan.org/~danberr/              -=|
|=-               Projects: http://freshmeat.net/~danielpb/               -=|
|=-  GnuPG: 7D3B9505   F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505  -=| 

Re: ioemu: empty vnc passwd

[John Levon]

On Wed, Jan 23, 2008 at 05:43:38PM +0000, Daniel P. Berrange wrote:

> > Except on Solaris we don't have such a default - the user's forced to
> > set something (there doesn't seem to be even a vaguely secure default?)
> 
> There's no sane default for VNC passwords - whether you have on or not
> its still basically insecure due to design of the VNC auth, hence the
> config just defaults to '' & 127.0.0.1 which is as good as you'll get 
> for VNC over TCP. 

So the only sane default is "don't let it work at all", right? Which is
what we're doing.

> If we wanted a real secure out of the box setup, we'd need to make XenD 
> only expose the VNC server as a UNIX domain socket, so that access can
> be restricted to root.

Yep, like you mentioned on IRC.

> Of course no VNC client knows how to connect to a VNC 

But of course :) sigh.

> server over a UNIX domain socket directly. You can use netcat + ssh to
> tunnel to/from a remote host. I could also extend GTK-VNC & virt-manager
> and/or virt-viewer to support it pretty easily.

Both of those support the encryption extension already though, if I
understand it right - and that seems sane enough.

regards
john

Re: ioemu: empty vnc passwd

[Daniel P. Berrange]

On Wed, Jan 23, 2008 at 05:56:18PM +0000, John Levon wrote:
> > server over a UNIX domain socket directly. You can use netcat + ssh to
> > tunnel to/from a remote host. I could also extend GTK-VNC & virt-manager
> > and/or virt-viewer to support it pretty easily.
> 
> Both of those support the encryption extension already though, if I
> understand it right - and that seems sane enough.

Yes, they do.

Ultimately I intend to try doing a SASL auth type for VNC too, since that
would allow Kerberos single sign on, and all the other fun auth schemes
that SASL has - including real *secure* username+password auth.

Dan.
-- 
|=- Red Hat, Engineering, Emerging Technologies, Boston.  +1 978 392 2496 -=|
|=-           Perl modules: http://search.cpan.org/~danberr/              -=|
|=-               Projects: http://freshmeat.net/~danielpb/               -=|
|=-  GnuPG: 7D3B9505   F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505  -=| 

[PATCH] ioemu: handle empty vnc passwd

[Samuel Thibault]

ioemu: handle empty vnc passwd
Have xenstore_read_vncpasswd return -1 when it is unable to read the
passwd from XenStore (and store an empty password).  However, don't exit
in such case since it may just mean that the use didn't set a passwd.
If he really did, xend would have given the passwd flag in the -vnc
option, and the empty passwd would make the vnc authentication reject
any password anyway.

Signed-off-by: Samuel Thibault <samuel.thibault@eu.citrix.com>

diff -r 9e92672385a5 tools/ioemu/xenstore.c
--- a/tools/ioemu/xenstore.c	Wed Jan 23 13:37:03 2008 +0000
+++ b/tools/ioemu/xenstore.c	Wed Jan 23 15:53:01 2008 +0000
@@ -518,7 +518,7 @@ int xenstore_read_vncpasswd(int domid, c
         pwbuf[0] = '\0';
         free(uuid);
         free(path);
-        return rc;
+        return -1;
     }
diff -r 9e92672385a5 tools/ioemu/vl.c
--- a/tools/ioemu/vl.c	Wed Jan 23 13:37:03 2008 +0000
+++ b/tools/ioemu/vl.c	Wed Jan 23 15:55:38 2008 +0000
@@ -7756,8 +7756,7 @@ int main(int argc, char **argv)
 	int vnc_display_port;
 	char password[20];
 	vnc_display_init(ds);
-	if (xenstore_read_vncpasswd(domid, password, sizeof(password)) < 0)
-	    exit(0);
+	xenstore_read_vncpasswd(domid, password, sizeof(password));
 	vnc_display_password(ds, password);
 	if ((vnc_display_port = vnc_display_open(ds, vnc_display, vncunused)) < 0) 
 	    exit (0);

Re: [PATCH] ioemu: handle empty vnc passwd

[Keir Fraser]

If the only caller of xenstore_read_vncpasswd() is not checking the return
code, why continue to have xenstore_read_vncpasswd() return an error code at
all?

If that is fixed, and Daniel Berrange will ack the patch, then I'll take it.

 -- Keir

On 23/1/08 17:05, "Samuel Thibault" <samuel.thibault@eu.citrix.com> wrote:

> ioemu: handle empty vnc passwd
> Have xenstore_read_vncpasswd return -1 when it is unable to read the
> passwd from XenStore (and store an empty password).  However, don't exit
> in such case since it may just mean that the use didn't set a passwd.
> If he really did, xend would have given the passwd flag in the -vnc
> option, and the empty passwd would make the vnc authentication reject
> any password anyway.
> 
> Signed-off-by: Samuel Thibault <samuel.thibault@eu.citrix.com>
> 
> diff -r 9e92672385a5 tools/ioemu/xenstore.c
> --- a/tools/ioemu/xenstore.c Wed Jan 23 13:37:03 2008 +0000
> +++ b/tools/ioemu/xenstore.c Wed Jan 23 15:53:01 2008 +0000
> @@ -518,7 +518,7 @@ int xenstore_read_vncpasswd(int domid, c
>          pwbuf[0] = '\0';
>          free(uuid);
>          free(path);
> -        return rc;
> +        return -1;
>      }
> diff -r 9e92672385a5 tools/ioemu/vl.c
> --- a/tools/ioemu/vl.c Wed Jan 23 13:37:03 2008 +0000
> +++ b/tools/ioemu/vl.c Wed Jan 23 15:55:38 2008 +0000
> @@ -7756,8 +7756,7 @@ int main(int argc, char **argv)
> int vnc_display_port;
> char password[20];
> vnc_display_init(ds);
> - if (xenstore_read_vncpasswd(domid, password, sizeof(password)) < 0)
> -     exit(0);
> + xenstore_read_vncpasswd(domid, password, sizeof(password));
> vnc_display_password(ds, password);
> if ((vnc_display_port = vnc_display_open(ds, vnc_display, vncunused)) < 0)
>    exit (0);
> 
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xensource.com
> http://lists.xensource.com/xen-devel

Re: [PATCH] ioemu: handle empty vnc passwd

[Daniel P. Berrange]

On Wed, Jan 23, 2008 at 05:46:33PM +0000, Keir Fraser wrote:
> If the only caller of xenstore_read_vncpasswd() is not checking the return
> code, why continue to have xenstore_read_vncpasswd() return an error code at
> all?
> 
> If that is fixed, and Daniel Berrange will ack the patch, then I'll take it.

ACK - patch gets my vote.

Regards,
Dan.
-- 
|=- Red Hat, Engineering, Emerging Technologies, Boston.  +1 978 392 2496 -=|
|=-           Perl modules: http://search.cpan.org/~danberr/              -=|
|=-               Projects: http://freshmeat.net/~danielpb/               -=|
|=-  GnuPG: 7D3B9505   F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505  -=| 

Re: [PATCH] ioemu: handle empty vnc passwd

[Keir Fraser]

On 23/1/08 17:49, "Daniel P. Berrange" <berrange@redhat.com> wrote:

> On Wed, Jan 23, 2008 at 05:46:33PM +0000, Keir Fraser wrote:
>> If the only caller of xenstore_read_vncpasswd() is not checking the return
>> code, why continue to have xenstore_read_vncpasswd() return an error code at
>> all?
>> 
>> If that is fixed, and Daniel Berrange will ack the patch, then I'll take it.
> 
> ACK - patch gets my vote.

Okay, actually I can just fix up read_vncpasswd to return void myself.

 -- Keir