Re: [9front] vncv(1): support for RFB 3.8

["Iruatã Souza" <iru.muzgo@gmail.com>]

[-- Attachment #1: Type: text/plain, Size: 3248 bytes --]

On Sat, Sep 26, 2020 at 9:58 PM <ori@eigenstate.org> wrote:
>
> > On 23/09/2020 01:25, ori@eigenstate.org wrote:
> >> > Hi,
> >> >
> >> > The following patch adds support for RFB 3.8 in vncv(1).
> >> > It has been tested by connecting to a screen shared by gnome3 on
> >> > linux. Please let me know if it introduces any regressions.
> >>
> >> Can you re-generate the patch and either add it as an attachment
> >> or send it through something other than gmail's web interface?
> >>
> >> gmail mangles patches, wrappigng them and replacing tabs with
> >> spaces.
> >>
> > here it goes
>
> First off -- gross, newer versions let the client downgrade
> security. This is the opposite of what should be happening.
> But that's what the RFC says, so I guess we go with it.
>
> Ok with it in the client, but let's never implement it in the
> server.
>
> That said: Looking at the RFC, there are 3 versions of
> the protocol that should not be treated as 3.3:
>
> > Any version reported other than 3.7 or 3.8 should be treated as 3.3.
>
> Accordingly, we should probably recognize and error on 3.7 here, since
> we don't implement it.
>
> +       if(strncmp(msg, "RFB 003.", 8) != 0) {
>                 werrstr("bad rfb version \"%s\"", msg);
>                 return -1;
>         }
>
> Something like:
>
>         if(strncmp(msg, "RFB 003.", 8) != 0
>         || strncmp(msg, "RFB 003.007", VerLen) == 0)
>                 werrstr("bad rfb version \"%s\"", msg);
>                 return -1;
>         }
>

Thanks for the review, Ori! And thanks for testing, kvik!

A new patch is attached and handling of version 3.7 has been addressed.

> The zero types case also looks like it could be improved too:
> The RFC says:
>
>
>    If number-of-security-types is zero, then for some reason the
>    connection failed (e.g., the server cannot support the desired
>    protocol version).  This is followed by a string describing the
>    reason (where a string is specified as a length followed by that many
>    ASCII characters):
>
>              +---------------+--------------+---------------+
>              | No. of bytes  | Type [Value] | Description   |
>              +---------------+--------------+---------------+
>              | 4             | U32          | reason-length |
>              | reason-length | U8 array     | reason-string |
>              +---------------+--------------+---------------+
>
>    The server closes the connection after sending the reason-string.
>
> It'd be nice to show the server message to the user, it'd help
> with debugging (maybe). Something like:
>
>
>         char *err;
>         ntypes = vncrdchar(v);
>         if (ntypes == 0) {
>                 err = vncrdstring(v);
>                 werrstr("auth error: %s", s);
>                 free(err);
>                 return AFailed;
>         }
>

This case was already addressed in the first patch, so I didn't change
anything in that respect. sectype38 returns AFailed, so vncauth will
read the reason string and present it to the user.

> I don't have a vnc 3.8 server set up right now for testing, so if
> you want to look over the proposed changes and test, that'd be
> great.
>
> Thanks for the patch!
>

Everything works as expected in my setup.

[-- Attachment #2: 9front-vncv38.diff --]
[-- Type: text/x-patch, Size: 3147 bytes --]

diff -r 19baa5600a90 sys/src/cmd/vnc/auth.c
--- a/sys/src/cmd/vnc/auth.c	Mon Apr 06 01:31:35 2020 +0200
+++ b/sys/src/cmd/vnc/auth.c	Sun Sep 27 17:38:01 2020 +0200
@@ -9,14 +9,16 @@
 	VerLen	= 12
 };
 
-static char version[VerLen+1] = "RFB 003.003\n";
+static char version33[VerLen+1] = "RFB 003.003\n";
+static char version38[VerLen+1] = "RFB 003.008\n";
+static int srvversion;
 
 int
 vncsrvhandshake(Vnc *v)
 {
 	char msg[VerLen+1];
 
-	strecpy(msg, msg+sizeof msg, version);
+	strecpy(msg, msg+sizeof msg, version33);
 	if(verbose)
 		fprint(2, "server version: %s\n", msg);
 	vncwrbytes(v, msg, VerLen);
@@ -35,18 +37,52 @@
 
 	msg[VerLen] = 0;
 	vncrdbytes(v, msg, VerLen);
-	if(strncmp(msg, "RFB ", 4) != 0){
+	if(strncmp(msg, "RFB 003.", 8) != 0 ||
+	   strncmp(msg, "RFB 003.007\n", VerLen) == 0){
 		werrstr("bad rfb version \"%s\"", msg);
 		return -1;
 	}
+	if(strncmp(msg, "RFB 003.008\n", VerLen) == 0)
+		srvversion = 38;
+	else
+		srvversion = 33;
+
 	if(verbose)
 		fprint(2, "server version: %s\n", msg);
-	strcpy(msg, version);
+	strcpy(msg, version38);
 	vncwrbytes(v, msg, VerLen);
 	vncflush(v);
 	return 0;
 }
 
+ulong
+sectype38(Vnc *v)
+{
+	ulong auth, type;
+	int i, ntypes;
+
+	ntypes = vncrdchar(v);
+	if(ntypes == 0){
+		werrstr("no security types from server");
+		return AFailed;
+	}
+
+	/* choose the "most secure" security type */
+	auth = AFailed;
+	for(i = 0; i < ntypes; i++){
+		type = vncrdchar(v);
+		if(verbose){
+			fprint(2, "auth type %s\n",
+				type == AFailed ? "Invalid" :
+				type == ANoAuth ? "None" :
+				type == AVncAuth ? "VNC" : "Unknown");
+		}
+		if(type > auth)
+			auth = type;
+	}
+	return auth;
+}
+
 int
 vncauth(Vnc *v, char *keypattern)
 {
@@ -56,7 +92,9 @@
 
 	if(keypattern == nil)
 		keypattern = "";
-	auth = vncrdlong(v);
+
+	auth = srvversion == 38 ? sectype38(v) : vncrdlong(v);
+
 	switch(auth){
 	default:
 		werrstr("unknown auth type 0x%lux", auth);
@@ -65,6 +103,7 @@
 		return -1;
 
 	case AFailed:
+	failed:
 		reason = vncrdstring(v);
 		werrstr("%s", reason);
 		if(verbose)
@@ -72,11 +111,20 @@
 		return -1;
 
 	case ANoAuth:
+		if(srvversion == 38){
+			vncwrchar(v, auth);
+			vncflush(v);
+		}
 		if(verbose)
 			fprint(2, "no auth needed\n");
 		break;
 
 	case AVncAuth:
+		if(srvversion == 38){
+			vncwrchar(v, auth);
+			vncflush(v);
+		}
+
 		vncrdbytes(v, chal, VncChalLen);
 		if(auth_respond(chal, VncChalLen, nil, 0, chal, VncChalLen, auth_getkey,
 			"proto=vnc role=client server=%s %s", serveraddr, keypattern) != VncChalLen){
@@ -84,13 +132,20 @@
 		}
 		vncwrbytes(v, chal, VncChalLen);
 		vncflush(v);
+		break;
+	}
 
-		auth = vncrdlong(v);
+	/* in version 3.8 the auth status is always sent, in 3.3 only in AVncAuth */
+	if(srvversion == 38 || auth == AVncAuth){
+		auth = vncrdlong(v); /* auth status */
 		switch(auth){
 		default:
 			werrstr("unknown server response 0x%lux", auth);
 			return -1;
 		case VncAuthFailed:
+			if (srvversion == 38)
+				goto failed;
+
 			werrstr("server says authentication failed");
 			return -1;
 		case VncAuthTooMany:
@@ -99,7 +154,6 @@
 		case VncAuthOK:
 			break;
 		}
-		break;
 	}
 	return 0;
 }