From mboxrd@z Thu Jan  1 00:00:00 1970
Received: from mimir.eigenstate.org ([206.124.132.107]) by ewsd; Sat Sep 26 15:58:10 EDT 2020
Received: from abbatoir.fios-router.home (pool-74-101-2-6.nycmny.fios.verizon.net [74.101.2.6])
	by mimir.eigenstate.org (OpenSMTPD) with ESMTPSA id 4240f873 (TLSv1.2:ECDHE-RSA-AES256-SHA:256:NO);
	Sat, 26 Sep 2020 12:58:02 -0700 (PDT)
Message-ID: <EF093381C8E99796F1350E8030900B5B@eigenstate.org>
To: iru.muzgo@gmail.com, ori@eigenstate.org
CC: 9front@9front.org
Subject: Re: [9front] vncv(1): support for RFB 3.8
Date: Sat, 26 Sep 2020 12:58:00 -0700
From: ori@eigenstate.org
In-Reply-To: <8917ba98-ba07-096f-eaed-3fa98f4c001c@gmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset="US-ASCII"
Content-Transfer-Encoding: 7bit
List-ID: <9front.9front.org>
List-Help: <http://lists.9front.org>
X-Glyph: ➈
X-Bullshit: webscale RESTful HTTP deep-learning realtime-java content-driven browser 

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