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

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

66–72

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

780

This comment needs to be adjusted.

797

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

808

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

usr.sbin/bhyve/rfb.h

36–40

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

[rgrimes]

usr.sbin/bhyve/bhyve.8

373

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

Leftover debug ?

70

type -> CFLAGS

usr.sbin/bhyve/bhyve.8

375

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

usr.sbin/bhyve/pci_fbuf.c

290

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

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

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

[rgrimes]

usr.sbin/bhyve/bhyve.8

312

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

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

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

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.

858

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

Is strlen(message) somehow bounds restricted some place?

[araujo]

usr.sbin/bhyve/Makefile

68

Yes, I will remove in the next diff update.

70

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

usr.sbin/bhyve/bhyve.8

312

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

375

Done! Will be updated in the next diff.

usr.sbin/bhyve/pci_fbuf.c

290

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

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

What do you think?

843

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

849

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.

858

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

858

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

858

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

[grehan]

usr.sbin/bhyve/rfb.c

858

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.