VNC authentication based on review D7029

[araujo]

Summary

Implement RFC6143 section 7.2.2 VNC Authentication.
It was entire based on review D7029.

Test Plan

Tested with tightvnc, chrome vnc and RealVnc.

[araujo]

style(9) fix.

[wblock]

usr.sbin/bhyve/bhyve.8

373 ↗

(On Diff #28567)

This is a little unclear. Maybe:

The password implementation only provides compatibility with VNC clients.
It does not provide any additional security for the VNC protocol.

(Which is still a little weak, probably ought to say something about VNC being unencrypted and all.)

[jilles]

usr.sbin/bhyve/Makefile

64 ↗

(On Diff #28567)

I think this will break building with OpenSSL disabled (WITHOUT_OPENSSL=1).

You could disable the new code if MK_OPENSSL is no.

usr.sbin/bhyve/rfb.c

767 ↗

(On Diff #28567)

This comment needs to be adjusted.

780 ↗

(On Diff #28567)

Although it is consistent with the rest of this code, it is wrong to use buf without checking how much data was actually read.

791 ↗

(On Diff #28567)

This truncates or pads the password to 16 bytes, while the comment says 8.

usr.sbin/bhyve/rfb.h

36–40 ↗

(On Diff #28567)

These constants are not necessary for rfb's clients and can therefore go into rfb.c.

[rgrimes]

usr.sbin/bhyve/bhyve.8

373 ↗

(On Diff #28567)

Why not just quote the RFC?

This type of authentication is known to be cryptographically weak and
   is not intended for use on untrusted networks.  Many implementations
   will want to use stronger security, such as running the session over
   an encrypted channel provided by IPsec [RFC4301] or SSH [RFC4254].

[araujo]

Addressed most issues exposed here. However the bhyve group is still concerned about the dependency of OpenSSL as there are some effort to replace it in our base system.

[araujo]

Fix an unnecessary extra tab in AUTH_LENGTH.

[araujo]

Fix style(9).

[araujo]

Bring back the openssl DES implementation that seemed to be more clear.
All these changes were discussed with @grehan, @rgrimes and @jhb.

[grehan]

usr.sbin/bhyve/Makefile

68 ↗

(On Diff #28734)

Leftover debug ?

70 ↗

(On Diff #28734)

type -> CFLAGS

usr.sbin/bhyve/bhyve.8

375 ↗

(On Diff #28734)

I don't think the RFC references add anything here and could be removed.

usr.sbin/bhyve/pci_fbuf.c

290 ↗

(On Diff #28734)

May want to warn (or error out) if the password is longer than 8 bytes, since that is length used in the key generation.

usr.sbin/bhyve/rfb.c

817 ↗

(On Diff #28734)

The bit reversal can be done with a 1-liner as per the example code I sent

static uint8_t
bit_reverse(uint8_t val)
{
/* From https://graphics.stanford.edu/~seander/bithacks.html */
return (((val * 0x80200802ULL) & 0x0884422110ULL) *

	    0x0101010101ULL >> 32);

}

843 ↗

(On Diff #28734)

Might want to see what other VNC serves write back as the error here and copy it.

[rgrimes]

usr.sbin/bhyve/bhyve.8

312 ↗

(On Diff #28734)

How do we conditional the man page? password= well not work if its build without OpenSSL, but the man page provides no info about this possible problem.

375 ↗

(On Diff #28734)

Reasonable, just strike the RFC 4301 and RFC4254 which are bound to change over time anyway, refering to IPsec and SSH leads the user down the right path.

usr.sbin/bhyve/pci_fbuf.c

290 ↗

(On Diff #28734)

Warning would be fine, error would be a POLA, many people use vnc passwords longer than the 8 bytes supported by the protocol and every server and client I have run either silently ignores this, or beeps as you type extra characters.

usr.sbin/bhyve/rfb.c

849 ↗

(On Diff #28734)

This should probably be a call to usage(), and usage() should be adjusted to have -password or no -password based on #ifdef HAVEW_OPENSSL, at least that is more the unix norm for commands that do not parse from unknown arguments.

860 ↗

(On Diff #28734)

Isnt this just the same as strcpy(buf + 4, message)?

Is strlen(message) somehow bounds restricted some place?

[araujo]

usr.sbin/bhyve/Makefile

68 ↗

(On Diff #28734)

Yes, I will remove in the next diff update.

70 ↗

(On Diff #28734)

Very bad typo, don't know how that happens :D

usr.sbin/bhyve/bhyve.8

312 ↗

(On Diff #28734)

As far as I know, we don't have it. I guess we have the same situation with RFB without UEFI.

375 ↗

(On Diff #28734)

Done! Will be updated in the next diff.

usr.sbin/bhyve/pci_fbuf.c

290 ↗

(On Diff #28734)

I'm with @rgrimes, I never saw any other VNC server/client report it, most of those that I saw run silently or just ignores it.

usr.sbin/bhyve/rfb.c

817 ↗

(On Diff #28734)

Yes I saw that, but I thought would be better avoid two loops to do the same thing.

What do you think?

843 ↗

(On Diff #28734)

RealVNC returns 'Wrong Password'. There is no standard, IMHO, the current message looks understandable.

849 ↗

(On Diff #28734)

Actually we just bypass the password in case there is no OpenSSL and print out a message in case user didn't notice that his OS is without OpenSSL.

860 ↗

(On Diff #28734)

strncpy() avoid buffer overflow. See at strcpy(3).

[araujo]

• Fix leftover debug in Makefile as well as typo with CFLAGS.
• Remove RFCs from bhyve(8).

[araujo]

Change the way how to detect OpenSSL, actually I'm checking ifndef and not ifdef.

Tested using:
/etc/src.conf WITHOUT_OPENSSL="YES"

[rgrimes]

usr.sbin/bhyve/rfb.c

860 ↗

(On Diff #28770)

The use of strlen(message) makes this have the exact some buffer overflow bug that using strcpy would have.

[araujo]

usr.sbin/bhyve/rfb.c

860 ↗

(On Diff #28770)

I can't see that; Do you have a better suggestion?

[grehan]

usr.sbin/bhyve/rfb.c

860 ↗

(On Diff #28770)

There's no need to copy the string into a buffer. You can simply stream_write() the byte-swapped size, and follow it with a stream_write() of the string.

[araujo]

• Remove strncpy suggested by @grehan.

[grehan]

Please credit Fabian Freyer (from D7029) as the original submitter of this code when committing.