* vncv(1): support for RFB 3.8 @ 2020-06-22 17:37 Iruatã Souza 2020-09-22 18:25 ` Iruatã Souza 2020-09-22 23:25 ` [9front] " ori 0 siblings, 2 replies; 15+ messages in thread From: Iruatã Souza @ 2020-06-22 17:37 UTC (permalink / raw) To: 9front 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; } ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: vncv(1): support for RFB 3.8 2020-06-22 17:37 vncv(1): support for RFB 3.8 Iruatã Souza @ 2020-09-22 18:25 ` Iruatã Souza 2020-09-22 20:08 ` [9front] " ori 2020-09-22 23:25 ` [9front] " ori 1 sibling, 1 reply; 15+ messages in thread From: Iruatã Souza @ 2020-09-22 18:25 UTC (permalink / raw) To: 9front 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; > } ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [9front] Re: vncv(1): support for RFB 3.8 2020-09-22 18:25 ` Iruatã Souza @ 2020-09-22 20:08 ` ori 2020-09-22 20:11 ` ori 0 siblings, 1 reply; 15+ messages in thread From: ori @ 2020-09-22 20:08 UTC (permalink / raw) To: iru.muzgo, 9front > 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? ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [9front] Re: vncv(1): support for RFB 3.8 2020-09-22 20:08 ` [9front] " ori @ 2020-09-22 20:11 ` ori 2020-09-22 20:36 ` Silas McCroskey 0 siblings, 1 reply; 15+ messages in thread From: ori @ 2020-09-22 20:11 UTC (permalink / raw) To: ori, iru.muzgo, 9front >> 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? ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [9front] Re: vncv(1): support for RFB 3.8 2020-09-22 20:11 ` ori @ 2020-09-22 20:36 ` Silas McCroskey [not found] ` <CABJnqBRGr4cjFJsnuSjEUjHWtRyvDd2Pq9xZ=2Xn3jOziWKEiw@mail.gmail.com> 0 siblings, 1 reply; 15+ messages in thread From: Silas McCroskey @ 2020-09-22 20:36 UTC (permalink / raw) To: 9front; +Cc: ori, iru.muzgo > + 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 ^ permalink raw reply [flat|nested] 15+ messages in thread
[parent not found: <CABJnqBRGr4cjFJsnuSjEUjHWtRyvDd2Pq9xZ=2Xn3jOziWKEiw@mail.gmail.com>]
* Re: [9front] Re: vncv(1): support for RFB 3.8 [not found] ` <CABJnqBRGr4cjFJsnuSjEUjHWtRyvDd2Pq9xZ=2Xn3jOziWKEiw@mail.gmail.com> @ 2020-09-22 23:10 ` Silas McCroskey 2020-09-22 23:31 ` hiro 0 siblings, 1 reply; 15+ messages in thread From: Silas McCroskey @ 2020-09-22 23:10 UTC (permalink / raw) To: Iruatã Souza; +Cc: 9front, ori 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. ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [9front] Re: vncv(1): support for RFB 3.8 2020-09-22 23:10 ` Silas McCroskey @ 2020-09-22 23:31 ` hiro 0 siblings, 0 replies; 15+ messages in thread From: hiro @ 2020-09-22 23:31 UTC (permalink / raw) To: 9front 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. > ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [9front] vncv(1): support for RFB 3.8 2020-06-22 17:37 vncv(1): support for RFB 3.8 Iruatã Souza 2020-09-22 18:25 ` Iruatã Souza @ 2020-09-22 23:25 ` ori 2020-09-23 6:41 ` Iruatã Souza 2020-09-26 19:39 ` kvik 1 sibling, 2 replies; 15+ messages in thread From: ori @ 2020-09-22 23:25 UTC (permalink / raw) To: iru.muzgo, 9front > 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. ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [9front] vncv(1): support for RFB 3.8 2020-09-22 23:25 ` [9front] " ori @ 2020-09-23 6:41 ` Iruatã Souza 2020-09-26 19:58 ` ori 2020-09-26 19:39 ` kvik 1 sibling, 1 reply; 15+ messages in thread From: Iruatã Souza @ 2020-09-23 6:41 UTC (permalink / raw) To: ori; +Cc: 9front [-- 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; } ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [9front] vncv(1): support for RFB 3.8 2020-09-23 6:41 ` Iruatã Souza @ 2020-09-26 19:58 ` ori 2020-09-26 20:42 ` hiro 2020-09-27 17:12 ` Iruatã Souza 0 siblings, 2 replies; 15+ messages in thread From: ori @ 2020-09-26 19:58 UTC (permalink / raw) To: iru.muzgo, ori; +Cc: 9front > 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! ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [9front] vncv(1): support for RFB 3.8 2020-09-26 19:58 ` ori @ 2020-09-26 20:42 ` hiro 2020-09-26 21:31 ` ori 2020-09-27 17:12 ` Iruatã Souza 1 sibling, 1 reply; 15+ messages in thread From: hiro @ 2020-09-26 20:42 UTC (permalink / raw) To: 9front 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! > > ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [9front] vncv(1): support for RFB 3.8 2020-09-26 20:42 ` hiro @ 2020-09-26 21:31 ` ori 0 siblings, 0 replies; 15+ messages in thread From: ori @ 2020-09-26 21:31 UTC (permalink / raw) To: 23hiro, 9front > 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. ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [9front] vncv(1): support for RFB 3.8 2020-09-26 19:58 ` ori 2020-09-26 20:42 ` hiro @ 2020-09-27 17:12 ` Iruatã Souza 2020-09-27 16:58 ` ori 1 sibling, 1 reply; 15+ messages in thread From: Iruatã Souza @ 2020-09-27 17:12 UTC (permalink / raw) To: ori; +Cc: 9front [-- 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; } ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [9front] vncv(1): support for RFB 3.8 2020-09-27 17:12 ` Iruatã Souza @ 2020-09-27 16:58 ` ori 0 siblings, 0 replies; 15+ messages in thread From: ori @ 2020-09-27 16:58 UTC (permalink / raw) To: iru.muzgo, ori; +Cc: 9front > 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. ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [9front] vncv(1): support for RFB 3.8 2020-09-22 23:25 ` [9front] " ori 2020-09-23 6:41 ` Iruatã Souza @ 2020-09-26 19:39 ` kvik 1 sibling, 0 replies; 15+ messages in thread From: kvik @ 2020-09-26 19:39 UTC (permalink / raw) To: 9front 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. ^ permalink raw reply [flat|nested] 15+ messages in thread
end of thread, other threads:[~2020-09-27 16:58 UTC | newest] Thread overview: 15+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2020-06-22 17:37 vncv(1): support for RFB 3.8 Iruatã Souza 2020-09-22 18:25 ` Iruatã Souza 2020-09-22 20:08 ` [9front] " ori 2020-09-22 20:11 ` ori 2020-09-22 20:36 ` Silas McCroskey [not found] ` <CABJnqBRGr4cjFJsnuSjEUjHWtRyvDd2Pq9xZ=2Xn3jOziWKEiw@mail.gmail.com> 2020-09-22 23:10 ` Silas McCroskey 2020-09-22 23:31 ` hiro 2020-09-22 23:25 ` [9front] " ori 2020-09-23 6:41 ` Iruatã Souza 2020-09-26 19:58 ` ori 2020-09-26 20:42 ` hiro 2020-09-26 21:31 ` ori 2020-09-27 17:12 ` Iruatã Souza 2020-09-27 16:58 ` ori 2020-09-26 19:39 ` kvik
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).