Page MenuHomeFreeBSD

VNC authentication based on review D7029
ClosedPublic

Authored by araujo on May 19 2017, 2:54 PM.
Tags
None
Referenced Files
Unknown Object (File)
Wed, Dec 11, 5:18 PM
Unknown Object (File)
Mon, Dec 9, 10:43 PM
Unknown Object (File)
Sat, Dec 7, 12:43 PM
Unknown Object (File)
Mon, Dec 2, 1:20 AM
Unknown Object (File)
Nov 24 2024, 3:01 AM
Unknown Object (File)
Nov 22 2024, 11:50 PM
Unknown Object (File)
Nov 19 2024, 10:45 AM
Unknown Object (File)
Nov 19 2024, 10:43 AM
Subscribers

Details

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.

Diff Detail

Repository
rS FreeBSD src repository - subversion
Lint
Lint Passed
Unit
No Test Coverage
Build Status
Buildable 9378
Build 9838: arc lint + arc unit

Event Timeline

wblock added inline comments.
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 added inline comments.
usr.sbin/bhyve/Makefile
64

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

This comment needs to be adjusted.

780

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

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.

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].

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.

Fix an unnecessary extra tab in AUTH_LENGTH.

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

usr.sbin/bhyve/Makefile
66

Leftover debug ?

68

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
800

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);

}

826

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

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
832

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.

833

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

Is strlen(message) somehow bounds restricted some place?

usr.sbin/bhyve/Makefile
66

Yes, I will remove in the next diff update.

68

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
800

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

What do you think?

826

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

832

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.

833

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

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

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

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

usr.sbin/bhyve/rfb.c
833

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

usr.sbin/bhyve/rfb.c
833

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

usr.sbin/bhyve/rfb.c
833

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.

  • Remove strncpy suggested by @grehan.

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

This revision is now accepted and ready to land.Jun 1 2017, 10:28 PM
This revision was automatically updated to reflect the committed changes.