vncv(1): support for RFB 3.8

vncv(1): support for RFB 3.8

[Iruatã Souza]

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.

diff -r 19baa5600a90 sys/man/1/vnc
--- a/sys/man/1/vnc    Mon Apr 06 01:31:35 2020 +0200
+++ b/sys/man/1/vnc    Mon Jun 22 19:25:55 2020 +0200
@@ -204,6 +204,3 @@
 .I Vncv
 does no verification of the TLS certificate presented
 by the server.
-.PP
-.I Vncv
-supports only version 3.3 of the RFB protocol.
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    Mon Jun 22 19:25:55 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,51 @@

     msg[VerLen] = 0;
     vncrdbytes(v, msg, VerLen);
-    if(strncmp(msg, "RFB ", 4) != 0){
+    if(strncmp(msg, "RFB 003.", 8) != 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 +91,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 +102,7 @@
         return -1;

     case AFailed:
+    failed:
         reason = vncrdstring(v);
         werrstr("%s", reason);
         if(verbose)
@@ -72,11 +110,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 +131,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 +153,6 @@
         case VncAuthOK:
             break;
         }
-        break;
     }
     return 0;
 }

Re: vncv(1): support for RFB 3.8

[Iruatã Souza]

Hi,

Did anyone try this? kvik?

On Mon, Jun 22, 2020 at 7:37 PM Iruatã Souza <iru.muzgo@gmail.com> 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.
>
> diff -r 19baa5600a90 sys/man/1/vnc
> --- a/sys/man/1/vnc    Mon Apr 06 01:31:35 2020 +0200
> +++ b/sys/man/1/vnc    Mon Jun 22 19:25:55 2020 +0200
> @@ -204,6 +204,3 @@
>  .I Vncv
>  does no verification of the TLS certificate presented
>  by the server.
> -.PP
> -.I Vncv
> -supports only version 3.3 of the RFB protocol.
> 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    Mon Jun 22 19:25:55 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,51 @@
>
>      msg[VerLen] = 0;
>      vncrdbytes(v, msg, VerLen);
> -    if(strncmp(msg, "RFB ", 4) != 0){
> +    if(strncmp(msg, "RFB 003.", 8) != 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 +91,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 +102,7 @@
>          return -1;
>
>      case AFailed:
> +    failed:
>          reason = vncrdstring(v);
>          werrstr("%s", reason);
>          if(verbose)
> @@ -72,11 +110,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 +131,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 +153,6 @@
>          case VncAuthOK:
>              break;
>          }
> -        break;
>      }
>      return 0;
>  }

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

[ori]

> Hi,
> 
> Did anyone try this? kvik?
> 

Thanks for pinging -- I haven't tested it yet. Just wondering,
does this solve any problems, or make any user-visible difference?

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

[ori]

>> Hi,
>> 
>> Did anyone try this? kvik?
>> 
> 
> Thanks for pinging -- I haven't tested it yet. Just wondering,
> does this solve any problems, or make any user-visible difference?

To put it another way, what prompted writing the patch?

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

[Silas McCroskey]

> +    if(strncmp(msg, "RFB 003.008\n", VerLen) == 0)
> +        srvversion = 38;
> +    else
> +        srvversion = 33;

> +            if (srvversion == 38)

This kind of thing should almost certainly be using enums instead of
magic numbers.

 - sam-d

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

[Silas McCroskey]

Well beyond just the usual "avoid magic numbers" advice, with version
numbers in particular enums let you use a format like VERS_3_8 or so
to make the inherent separation more clear, especially to distinguish
between something like 3.11.1 and 3.1.11.

- sam-d

On Tue, Sep 22, 2020 at 1:58 PM Iruatã Souza <iru.muzgo@gmail.com> wrote:
>
> Le mar. 22 sept. 2020 à 22:36, Silas McCroskey <inkswinc@gmail.com> a écrit :
>>
>> > +    if(strncmp(msg, "RFB 003.008\n", VerLen) == 0)
>> > +        srvversion = 38;
>> > +    else
>> > +        srvversion = 33;
>>
>> > +            if (srvversion == 38)
>>
>> This kind of thing should almost certainly be using enums instead of
>> magic numbers.
>>
>>  - sam-d
>
>
> Usually I would promptly agree with that suggestion, but it got me thinking. In our specific case, is srvversion == SrvVersion38 actually clearer than srvversion == 38?
>
> In any case, I would happily change the patch if enums are preferred.

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

[ori]

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

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

[hiro]

i agree srvversion == 38 is clearer. clearly he has spent effort
minimizing this to achieve maximal expressiveness with least
redundency.

On 9/23/20, Silas McCroskey <inkswinc@gmail.com> wrote:
> Well beyond just the usual "avoid magic numbers" advice, with version
> numbers in particular enums let you use a format like VERS_3_8 or so
> to make the inherent separation more clear, especially to distinguish
> between something like 3.11.1 and 3.1.11.
>
> - sam-d
>
> On Tue, Sep 22, 2020 at 1:58 PM Iruatã Souza <iru.muzgo@gmail.com> wrote:
>>
>> Le mar. 22 sept. 2020 à 22:36, Silas McCroskey <inkswinc@gmail.com> a
>> écrit :
>>>
>>> > +    if(strncmp(msg, "RFB 003.008\n", VerLen) == 0)
>>> > +        srvversion = 38;
>>> > +    else
>>> > +        srvversion = 33;
>>>
>>> > +            if (srvversion == 38)
>>>
>>> This kind of thing should almost certainly be using enums instead of
>>> magic numbers.
>>>
>>>  - sam-d
>>
>>
>> Usually I would promptly agree with that suggestion, but it got me
>> thinking. In our specific case, is srvversion == SrvVersion38 actually
>> clearer than srvversion == 38?
>>
>> In any case, I would happily change the patch if enums are preferred.
>

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

[Iruatã Souza]

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


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


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

diff -r 19baa5600a90 sys/man/1/vnc
--- a/sys/man/1/vnc	Mon Apr 06 01:31:35 2020 +0200
+++ b/sys/man/1/vnc	Mon Jun 22 19:25:55 2020 +0200
@@ -204,6 +204,3 @@
 .I Vncv
 does no verification of the TLS certificate presented
 by the server.
-.PP
-.I Vncv
-supports only version 3.3 of the RFB protocol.
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	Mon Jun 22 19:25:55 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,51 @@
 
 	msg[VerLen] = 0;
 	vncrdbytes(v, msg, VerLen);
-	if(strncmp(msg, "RFB ", 4) != 0){
+	if(strncmp(msg, "RFB 003.", 8) != 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 +91,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 +102,7 @@
 		return -1;
 
 	case AFailed:
+	failed:
 		reason = vncrdstring(v);
 		werrstr("%s", reason);
 		if(verbose)
@@ -72,11 +110,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 +131,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 +153,6 @@
 		case VncAuthOK:
 			break;
 		}
-		break;
 	}
 	return 0;
 }

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

[kvik]

I haven't looked at the code in any detail but the patch works
great.  I can now connect to the VNC server provided by bhyve.

Thanks.

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

[ori]

> 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;
 	}

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;
	}

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!

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

[hiro]

there have been vnc RFC updates for security? remind me why why are
all tunneling vnc through ssh then!

On 9/26/20, ori@eigenstate.org <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;
>  	}
>
> 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;
> 	}
>
> 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!
>
>

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

[ori]

> there have been vnc RFC updates for security? remind me why why are
> all tunneling vnc through ssh then!

"security".

I'm just complaining that it became worse than it was.

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

[ori]

> Thanks for the review, Ori! And thanks for testing, kvik!
> 
> A new patch is attached and handling of version 3.7 has been addressed.

Looks good, works on old versions -- applying.

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

[Iruatã Souza]

[-- 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;
 }