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.