subject:"svn commit\: r319487 \- head\/usr.sbin\/bhyve"

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

2017-06-11 Thread Marcelo Araujo

2017-06-11 2:35 GMT+08:00 Conrad Meyer :

> On Sat, Jun 10, 2017 at 11:14 AM, Peter Grehan  wrote:
> >  strncpy() is specified to zero-fill if the source is shorter than the
> > length. Are we missing something ?
> >
> >  The other issues you brought up look valid.
>
> Hi Peter,
>
> Oops — didn't realize that about strncpy()!  Thanks for the correction.
>
> Best,
> Conrad
>

Thanks guys,

I will take a look on it very soon and will put you both into the review.


Best,
-- 

-- 
Marcelo Araujo(__)ara...@freebsd.org
\\\'',)http://www.FreeBSD.org    \/  \ ^
Power To Server. .\. /_)
___
svn-src-all@freebsd.org mailing list
https://lists.freebsd.org/mailman/listinfo/svn-src-all
To unsubscribe, send any mail to "svn-src-all-unsubscr...@freebsd.org"

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

2017-06-10 Thread Conrad Meyer

On Sat, Jun 10, 2017 at 11:14 AM, Peter Grehan  wrote:
>  strncpy() is specified to zero-fill if the source is shorter than the
> length. Are we missing something ?
>
>  The other issues you brought up look valid.

Hi Peter,

Oops — didn't realize that about strncpy()!  Thanks for the correction.

Best,
Conrad
___
svn-src-all@freebsd.org mailing list
https://lists.freebsd.org/mailman/listinfo/svn-src-all
To unsubscribe, send any mail to "svn-src-all-unsubscr...@freebsd.org"

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

2017-06-10 Thread Peter Grehan


Hi Conrad,


Here, keystr is not zero initialized
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);


 strncpy() is specified to zero-fill if the source is shorter than the 
length. Are we missing something ?


 The other issues you brought up look valid.

later,

Peter.
___
svn-src-all@freebsd.org mailing list
https://lists.freebsd.org/mailman/listinfo/svn-src-all
To unsubscribe, send any mail to "svn-src-all-unsubscr...@freebsd.org"

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

2017-06-10 Thread Conrad Meyer

Hi,

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

On Thu, Jun 1, 2017 at 7:35 PM, Marcelo Araujo  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, , 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
___
svn-src-all@freebsd.org mailing list
https://lists.freebsd.org/mailman/listinfo/svn-src-all
To unsubscribe, send any mail to "svn-src-all-unsubscr...@freebsd.org"

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

2017-06-10 Thread Conrad Meyer

Forgot to mention — this one was CID 1375949.

Additionally additionally,

On Thu, Jun 1, 2017 at 7:35 PM, Marcelo Araujo  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];
> +   unsigned char crypt_expected[AUTH_LENGTH];
> +
> +   DES_key_schedule ks;
> +   int i;
> +#endif
> +
> pthread_t tid;

This is uninitialized.

> -uint32_t sres;
> +   uint32_t sres;
> int len;
>
> rc->cfd = cfd;
> @@ -751,19 +777,91 @@ rfb_handle(struct rfb_softc *rc, int cfd)
> ...
> +   /* 2c. Do VNC authentication */
> +   switch (buf[0]) {
> +   case SECURITY_TYPE_NONE:
> +   sres = 0;
> +   break;
> +   case SECURITY_TYPE_VNC_AUTH:
 ...
>
> +
> +   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;
> +   }
> +
> +   /* 2d. Write back a status */
> stream_write(cfd, , 4);
>
> +   if (sres) {
> +   *((uint32_t *) buf) = htonl(strlen(message));
> +   stream_write(cfd, buf, 4);
> +stream_write(cfd, message, strlen(message));
> +   goto done;
> +   }

When authentication fails, 'done:' label will pthread_join(tid), which
is also uninitialized at this point.  This is CID 1375950.

Best,
Conrad
___
svn-src-all@freebsd.org mailing list
https://lists.freebsd.org/mailman/listinfo/svn-src-all
To unsubscribe, send any mail to "svn-src-all-unsubscr...@freebsd.org"

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

2017-06-10 Thread Conrad Meyer

Hi,

See inline comments below.

On Thu, Jun 1, 2017 at 7:35 PM, Marcelo Araujo  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, );

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

> +   DES_ecb_encrypt((C_Block *)challenge,
> +   (C_Block *)crypt_expected, , DES_ENCRYPT);
> +   DES_ecb_encrypt((C_Block *)(challenge + PASSWD_LENGTH),
> +   (C_Block *)(crypt_expected + PASSWD_LENGTH),
> +   , 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
___
svn-src-all@freebsd.org mailing list
https://lists.freebsd.org/mailman/listinfo/svn-src-all
To unsubscribe, send any mail to "svn-src-all-unsubscr...@freebsd.org"

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

2017-06-01 Thread Marcelo Araujo

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.
  
  Submitted by: Fabian Freyer 
  Reworked by:  myself
  Reviewed by:  grehan, rgrimes and jilles
  MFC after:1 week.
  Relnotes: Yes.
  Sponsored by: iXsystems, Inc.
  Differential Revision:https://reviews.freebsd.org/D10818

Modified:
  head/usr.sbin/bhyve/Makefile
  head/usr.sbin/bhyve/bhyve.8
  head/usr.sbin/bhyve/pci_fbuf.c
  head/usr.sbin/bhyve/rfb.c
  head/usr.sbin/bhyve/rfb.h

Modified: head/usr.sbin/bhyve/Makefile
==
--- head/usr.sbin/bhyve/MakefileFri Jun  2 01:00:40 2017
(r319486)
+++ head/usr.sbin/bhyve/MakefileFri Jun  2 02:35:16 2017
(r319487)
@@ -2,6 +2,8 @@
 # $FreeBSD$
 #
 
+.include 
+
 PROG=  bhyve
 PACKAGE=   bhyve
 
@@ -62,6 +64,12 @@ SRCS=\
 SRCS+= vmm_instruction_emul.c
 
 LIBADD=vmmapi md pthread z
+
+.if ${MK_OPENSSL} == "no"
+CFLAGS+=-DNO_OPENSSL
+.else
+LIBADD+=   crypto
+.endif
 
 CFLAGS+= -I${BHYVE_SYSDIR}/sys/dev/e1000
 CFLAGS+= -I${BHYVE_SYSDIR}/sys/dev/mii

Modified: head/usr.sbin/bhyve/bhyve.8
==
--- head/usr.sbin/bhyve/bhyve.8 Fri Jun  2 01:00:40 2017(r319486)
+++ head/usr.sbin/bhyve/bhyve.8 Fri Jun  2 02:35:16 2017(r319487)
@@ -24,7 +24,7 @@
 .\"
 .\" $FreeBSD$
 .\"
-.Dd May 3, 2017
+.Dd May 22, 2017
 .Dt BHYVE 8
 .Os
 .Sh NAME
@@ -309,7 +309,7 @@ Emergency write is advertised, but no-op at present.
 .Pp
 Framebuffer devices:
 .Bl -tag -width 10n
-.It Oo rfb= Ns Oo Ar IP: Oc Ns Ar port Oc Ns Oo ,w= Ns Ar width Oc Ns Oo ,h= 
Ns Ar height Oc Ns Oo ,vga= Ns Ar vgaconf Oc Ns Oo Ns ,wait Oc
+.It Oo rfb= Ns Oo Ar IP: Oc Ns Ar port Oc Ns Oo ,w= Ns Ar width Oc Ns Oo ,h= 
Ns Ar height Oc Ns Oo ,vga= Ns Ar vgaconf Oc Ns Oo Ns ,wait Oc Ns Oo ,password= 
Ns Ar password Oc
 .Bl -tag -width 8n
 .It Ar IP:port
 An
@@ -368,6 +368,11 @@ Instruct
 to only boot upon the initiation of a VNC connection, simplifying the 
installation
 of operating systems that require immediate keyboard input.
 This can be removed for post-installation use.
+.It password
+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 or SSH.
 .El
 .El
 .Pp

Modified: head/usr.sbin/bhyve/pci_fbuf.c
==
--- head/usr.sbin/bhyve/pci_fbuf.c  Fri Jun  2 01:00:40 2017
(r319486)
+++ head/usr.sbin/bhyve/pci_fbuf.c  Fri Jun  2 02:35:16 2017
(r319487)
@@ -93,6 +93,7 @@ struct pci_fbuf_softc {
 
/* rfb server */
char  *rfb_host;
+   char  *rfb_password;
int   rfb_port;
int   rfb_wait;
int   vga_enabled;
@@ -285,7 +286,8 @@ pci_fbuf_parse_opts(struct pci_fbuf_softc *sc, char *o
goto done;
} else if (sc->memregs.height == 0)
sc->memregs.height = 1080;
-
+   } else if (!strcmp(xopts, "password")) {
+   sc->rfb_password = config;
} else {
pci_fbuf_usage(xopts);
ret = -1;
@@ -407,7 +409,7 @@ pci_fbuf_init(struct vmctx *ctx, struct pci_devinst *p
 
memset((void *)sc->fb_base, 0, FB_SIZE);
 
-   error = rfb_init(sc->rfb_host, sc->rfb_port, sc->rfb_wait);
+   error = rfb_init(sc->rfb_host, sc->rfb_port, sc->rfb_wait, 
sc->rfb_password);
 done:
if (error)
free(sc);

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)
@@ -60,10 +60,23 @@ __FBSDID("$FreeBSD$");
 #include "rfb.h"
 #include "sockstream.h"
 
+#ifndef NO_OPENSSL
+#include 
+#endif
+
 static int rfb_debug = 0;
 #defineDPRINTF(params) if (rfb_debug) printf params
 #defineWPRINTF(params) printf params
 
+#define AUTH_LENGTH16
+#define PASSWD_LENGTH  8
+
+#define SECURITY_TYPE_NONE 1
+#define SECURITY_TYPE_VNC_AUTH 2
+
+#define AUTH_FAILED_UNAUTH 1
+#define AUTH_FAILED_ERROR 2
+
 struct rfb_softc {
int sfd;
pthread_t   tid;
@@ -72,10 +85,12 @@ struct rfb_softc {
 
int width, height;
 
-   boolenc_raw_ok;
-   boolenc_zlib_ok;
-   boolenc_resize_ok;
+   char*password;
 
+   boolenc_raw_ok;
+   bool

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

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

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

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

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

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

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

7 matches

Site Navigation

Mail list logo

Footer information