svn commit: r319487 - head/usr.sbin/bhyve

[Conrad Meyer]

Hi,

Additionally, one more issue pointed out by Coverity below :-).

On Thu, Jun 1, 2017 at 7:35 PM, Marcelo Araujo <araujo at freebsd.org> wrote:
> Author: araujo
> Date: Fri Jun  2 02:35:16 2017
> New Revision: 319487
> URL: https://svnweb.freebsd.org/changeset/base/319487
>
> Log:
>   Add VNC Authentication support based on RFC6143 section 7.2.2.
>
> ...
> Modified: head/usr.sbin/bhyve/rfb.c
> ==============================================================================
> --- head/usr.sbin/bhyve/rfb.c   Fri Jun  2 01:00:40 2017        (r319486)
> +++ head/usr.sbin/bhyve/rfb.c   Fri Jun  2 02:35:16 2017        (r319487)
> ...
> @@ -739,8 +754,19 @@ rfb_handle(struct rfb_softc *rc, int cfd)
>  {
>         const char *vbuf = "RFB 003.008\n";
>         unsigned char buf[80];
> +       unsigned char *message;

Message is uninitialized.

> +
> +#ifndef NO_OPENSSL
> +       unsigned char challenge[AUTH_LENGTH];
> +       unsigned char keystr[PASSWD_LENGTH];
> +       unsigned char crypt_expected[AUTH_LENGTH];
> +
> +       DES_key_schedule ks;
> +       int i;
> +#endif
> +
>         pthread_t tid;
> -        uint32_t sres;
> +       uint32_t sres;

sres is also uninitialized.

>         int len;
>
>         rc->cfd = cfd;
> @@ -751,19 +777,91 @@ rfb_handle(struct rfb_softc *rc, int cfd)
>         /* 1b. Read client version */
>         len = read(cfd, buf, sizeof(buf));
>
> -       /* 2a. Send security type 'none' */
> +       /* 2a. Send security type */
>         buf[0] = 1;
> -       buf[1] = 1; /* none */
> +#ifndef NO_OPENSSL
> +       if (rc->password)
> +               buf[1] = SECURITY_TYPE_VNC_AUTH;
> +       else
> +               buf[1] = SECURITY_TYPE_NONE;
> +#else
> +       buf[1] = SECURITY_TYPE_NONE;
> +#endif
> +
>         stream_write(cfd, buf, 2);
>
> -
>         /* 2b. Read agreed security type */
>         len = stream_read(cfd, buf, 1);

A malicious server negotiation could respond in ways that break later
assumptions:

1. Respond to NONE with VNC_AUTH.  In this case rc->password will be
NULL and strncpy() below will cause a SIGSEGV.
2. Respond to VNC_AUTH with a bogus value.  In this case, neither sres
nor message is ever initialized.

> ...
> +       /* 2d. Write back a status */
>         stream_write(cfd, &sres, 4);

Bogus sres could be used here.

>
> +       if (sres) {
> +               *((uint32_t *) buf) = htonl(strlen(message));

Bogus message could be dereferenced here, resulting in SIGSEGV.

Additionally, aliasing char array buf via a uint32_t pointer is
invalid C.  I'd suggest instead:

be32enc(buf, strlen(message));

> +               stream_write(cfd, buf, 4);
> +                stream_write(cfd, message, strlen(message));
> +               goto done;
> +       }
> +
>         /* 3a. Read client shared-flag byte */
>         len = stream_read(cfd, buf, 1);
>
> ...

Best,
Conrad