ioemu: empty vnc passwd

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

tools/ioemu/xenstore.c +1 -1

@@ -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

tools/ioemu/vl.c +1 -2

@@ -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 <[email protected]>

_______________________________________________
Xen-devel mailing list
[email protected]
http://lists.xensource.com/xen-devel

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.

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.

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

tools/ioemu/xenstore.c +1 -1

@@ -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:

Quoted 4 lines · 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.

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.

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.

Quoted 6 lines · On Wednesday 23 January 2008 17:28:11 Daniel P. Berrange wrote:

Quoted 4 lines · 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.

Quoted 4 lines

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.

Quoted 6 lines · 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).

Quoted 6 lines · 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).

Quoted 10 lines · Samuel Thibault, le Wed 23 Jan 2008 16:42:33 +0000, a écrit :

Quoted 6 lines · 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.

Quoted 8 lines · Samuel Thibault

Quoted 6 lines · 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.

Quoted 2 lines

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

Quoted 3 lines

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.

Quoted 4 lines · Daniel P. Berrange

Quoted 3 lines

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:

Quoted 8 lines

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

Quoted 6 lines

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 ""

Quoted 7 lines · Daniel P. Berrange

Quoted 6 lines

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

Quoted 3 lines

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

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 <[email protected]>

diff -r 9e92672385a5 tools/ioemu/xenstore.c

tools/ioemu/xenstore.c Wed Jan 23 15:53:01 2008 +0000 renamed from tools/ioemu/xenstore.c Wed Jan 23 13:37:03 2008 +0000 +1 -1

@@ -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

tools/ioemu/vl.c Wed Jan 23 15:55:38 2008 +0000 renamed from tools/ioemu/vl.c Wed Jan 23 13:37:03 2008 +0000 +1 -2

@@ -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
[email protected]
http://lists.xensource.com/xen-devel

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.

Quoted 2 lines

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.

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.

Quoted 3 lines

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.

Quoted 5 lines · 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.

ACK - patch gets my vote.