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

[Conrad Meyer]

Hi,

See inline comments 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;
> +
> +#ifndef NO_OPENSSL
> +       unsigned char challenge[AUTH_LENGTH];
> +       unsigned char keystr[PASSWD_LENGTH];

Here, keystr is not zero initialized.

> +       unsigned char crypt_expected[AUTH_LENGTH];
> +
> +       DES_key_schedule ks;
> +       int i;
> +#endif
> +
>         pthread_t tid;
> -        uint32_t sres;
> +       uint32_t sres;
>         int len;
>
>         rc->cfd = cfd;
> @@ -751,19 +777,91 @@ rfb_handle(struct rfb_softc *rc, int cfd)
> ...
> +       case SECURITY_TYPE_VNC_AUTH:
> +               /*
> +                * The client encrypts the challenge with DES, using a password
> +                * supplied by the user as the key.
> +                * To form the key, the password is truncated to
> +                * eight characters, or padded with null bytes on the right.

Note that strncpy below does not fill the remainder of the buffer with
nuls if rc->password is shorter than 7 characters.

> +                * The client then sends the resulting 16-bytes response.
> +                */
> +#ifndef NO_OPENSSL
> +               strncpy(keystr, rc->password, PASSWD_LENGTH);
> +
> +               /* VNC clients encrypts the challenge with all the bit fields
> +                * in each byte of the password mirrored.
> +                * Here we flip each byte of the keystr.
> +                */
> +               for (i = 0; i < PASSWD_LENGTH; i++) {
> +                       keystr[i] = (keystr[i] & 0xF0) >> 4
> +                                 | (keystr[i] & 0x0F) << 4;
> +                       keystr[i] = (keystr[i] & 0xCC) >> 2
> +                                 | (keystr[i] & 0x33) << 2;
> +                       keystr[i] = (keystr[i] & 0xAA) >> 1
> +                                 | (keystr[i] & 0x55) << 1;
> +               }

Above is the first place stack garbage in keystr is accessed if
rc->password was shorter than 7 characters.

> +
> ...
> +               /* Encrypt the Challenge with DES */
> +               DES_set_key((C_Block *)keystr, &ks);

Stack garbage in keystr is used as a DES block here.

> +               DES_ecb_encrypt((C_Block *)challenge,
> +                               (C_Block *)crypt_expected, &ks, DES_ENCRYPT);
> +               DES_ecb_encrypt((C_Block *)(challenge + PASSWD_LENGTH),
> +                               (C_Block *)(crypt_expected + PASSWD_LENGTH),
> +                               &ks, DES_ENCRYPT);
> +
> +               if (memcmp(crypt_expected, buf, AUTH_LENGTH) != 0) {
> +                       message = "Auth Failed: Invalid Password.";
> +                       sres = htonl(1);
> +               } else
> +                       sres = 0;
> +#else
> +               sres = 0;
> +               WPRINTF(("Auth not supported, no OpenSSL in your system"));
> +#endif
> +
> +               break;
> +       }
> +
> ...

I'd suggest zero initializing keystr.

I noticed this while investigating Coverity CID 1375945, which is sort
of a false positive.  It did helpfully point out the broken transition
from C string to fixed-length buffer, though.

Best,
Conrad