Re: [Qemu-devel] PATCH: RFC: Support SASL authentication in VNC

[Anthony Liguori <anthony@codemonkey.ws>]

Hi Dan,

Daniel P. Berrange wrote:
> Previously I provided patches for QEMU's VNC server to support SSL/TLS
> and x509 certificates. This provides good encryption capabilities for
> the VNC session. It doesn't really address the authentication problem
> though.
>
> I have been working to  create a new authentication type in the RFB
> protocol to address this need in a generic, extendable way, by mapping
> the SASL API into the RFB protocol. Since SASL is a generic plugin 
> based API, this will allow use of a huge range of auth mechanims over 
> VNC, without us having to add any more auth code. For example, PAM,
> Digest-MD5, GSSAPI/Kerberos, One-time key/password, LDAP password
> lookup, SQL db password lookup, and more.
>
> I have got a VNC auth type assigned by the RFB spec maintainers:
>
>   http://realvnc.com/pipermail/vnc-list/2008-December/059463.html
>
> With the full current spec  for the SASL extension currently documented 
> here:
>
>   http://realvnc.com/pipermail/vnc-list/2008-December/059462.html
>
> This patch provides an initial implementation of this extension for the
> QEMU VNC server. I am not requesting it is merged just yet, but I'd like
> get it all finished in time of the suggested QEMU release at the end
> of the month, so I'm sending the patch out now for early comments
>
> In most circumstances, it is neccessary to combine use of SASL, with
> the TLS + x509 support, since most  SASL mechanisms only provide for
> authentication.  In this case you would launch QEMU with
>
>    qemu ...   -vnc localhost:1,tls,x509,sasl
>
>
> The Kerberos GSSAPI mechanism for SASL is unusual in that it can also
> provide encryption of the data session (so called SSF layer in SASL
> terminology). If using this, QEMU can be launched with just
>
>    qemu ...    -vnc localhost:1,sasl
>
> The actual choice of SASL mechanism is controlled by the SASL service
> configuration file. THis patch integrates with Cyrus-SASL library and
> thus the config is in /etc/sasl2/qemu.conf.  This patch does not yet 
> support it, but I intend too add support for overriding this config
> with $HOME/.sasl2/qemu.conf, since most people use QEMU in their regular
> user accounts and may not have access to the system SASL configs. The
> updates to qemu-doc.texi show how to config SASL, or the Cyrus-SASL docs
> can be referred to.
>
> As I mentioned, this uses Cyrus-SASL libraries. The configure script
> probes for this, and disables SASL  support if not found, so it should
> not cause problems for places without Cyrus-SASL like Windows.
>
> The main missing points in this patch
>
>  - Authorization. Once we've authenticated the user, how do we 
>    decide whether they're allow to use VNC. eg, just because a
>    user has a valid Kerberos principle, does not imply we should
>    allow access. 
>
>    We really need an access control list, listing the allowed
>    SASL usernames and/or x509 client certificate CNAMEs which
>    are authorized. This should probably live in an external
>    file, perhaps allowing for ACLs against multiple different
>    QEMU network based devices. eg I could just add a arg
>
>         -acl  /path/to/aclfile
>   

Not a huge fan of using an acl file but I'm not terribly opposed to it 
either.  I like the monitor commands but obviously, we'll need a 
mechanism to list existing acls too.

>  Makefile.target |    5 
>  configure       |   34 ++
>  qemu-doc.texi   |   94 ++++++
>  qemu.sasl       |   34 ++
>  vnc.c           |  800 ++++++++++++++++++++++++++++++++++++++++++++++++++++++--
>  vnc.h           |   16 -
>  6 files changed, 964 insertions(+), 19 deletions(-)
>
> Regards,
> Daniel
>
>
> Index: Makefile.target
> ===================================================================
> --- Makefile.target	(revision 6511)
> +++ Makefile.target	(working copy)
> @@ -554,6 +554,11 @@
>  LIBS += $(CONFIG_VNC_TLS_LIBS)
>  endif
>  
> +ifdef CONFIG_VNC_SASL
> +CPPFLAGS += $(CONFIG_VNC_SASL_CFLAGS)
> +LIBS += $(CONFIG_VNC_SASL_LIBS)
> +endif
> +
>  ifdef CONFIG_BLUEZ
>  LIBS += $(CONFIG_BLUEZ_LIBS)
>  endif
> Index: vnc.c
> ===================================================================
> --- vnc.c	(revision 6511)
> +++ vnc.c	(working copy)
> @@ -43,8 +43,12 @@
>  #include <gnutls/x509.h>
>  #endif /* CONFIG_VNC_TLS */
>  
> -// #define _VNC_DEBUG 1
> +#ifdef CONFIG_VNC_SASL
> +#include <sasl/sasl.h>
> +#endif
>  
> +#define _VNC_DEBUG 1
> +
>  #ifdef _VNC_DEBUG
>  #define VNC_DEBUG(fmt, ...) do { fprintf(stderr, fmt, ## __VA_ARGS__); } while (0)
>  
> @@ -123,6 +127,32 @@
>      char *x509cert;
>      char *x509key;
>  #endif
> +
> +#ifdef CONFIG_VNC_SASL
> +    sasl_conn_t *saslconn;
> +    /* If we want to negotiate an SSF layer with client */
> +    int saslWantSSF :1;
> +    /* If we are now running the SSF layer */
> +    int saslRunSSF :1;
> +    /*
> +     * If this is non-zero, then wait for that many bytes
> +     * to be written plain, before switching to SSF encoding
> +     * This allows the VNC auth result to finish being
> +     * written in plain.
> +     */
> +    unsigned int saslWaitWriteSSF;
> +
> +    /*
> +     * Buffering encoded data to allow more clear data
> +     * to be stuffed onto the output buffer
> +     */
> +    const uint8_t *saslEncoded;
> +    unsigned int saslEncodedLength;
> +    unsigned int saslEncodedOffset;
> +    char *saslUsername;
> +    char *saslMechlist;
> +#endif
>   

I think we could use a little more love here that moved the sasl bits to 
a vnc-sasl.c provide that it's a sane thing to do.

> +
>      char challenge[VNC_AUTH_CHALLENGE_SIZE];
>  
>  #ifdef CONFIG_VNC_TLS
> @@ -829,6 +859,18 @@
>  	}
>  	vs->wiremode = VNC_WIREMODE_CLEAR;
>  #endif /* CONFIG_VNC_TLS */
> +#ifdef CONFIG_VNC_SASL
> +        if (vs->saslconn) {
> +            vs->saslRunSSF = vs->saslWaitWriteSSF = vs->saslWantSSF = 0;
> +            vs->saslEncodedLength = vs->saslEncodedOffset = 0;
> +            vs->saslEncoded = NULL;
> +            free(vs->saslUsername);
> +            free(vs->saslMechlist);
> +            vs->saslUsername = vs->saslMechlist = NULL;
> +            sasl_dispose(&vs->saslconn);
> +            vs->saslconn = NULL;
> +        }
> +#endif /* CONFIG_VNC_SASL */
>          audio_del(vs);
>  	return 0;
>      }
> @@ -840,14 +882,12 @@
>      vnc_client_io_error(vs, -1, EINVAL);
>  }
>  
> -static void vnc_client_write(void *opaque)
> +static long vnc_client_write_wire(VncState *vs, const uint8_t *data, size_t datalen)
>  {
>      long ret;
> -    VncState *vs = opaque;
> -
>  #ifdef CONFIG_VNC_TLS
>      if (vs->tls_session) {
> -	ret = gnutls_write(vs->tls_session, vs->output.buffer, vs->output.offset);
> +	ret = gnutls_write(vs->tls_session, data, datalen);
>  	if (ret < 0) {
>  	    if (ret == GNUTLS_E_AGAIN)
>  		errno = EAGAIN;
> @@ -857,35 +897,117 @@
>  	}
>      } else
>  #endif /* CONFIG_VNC_TLS */
> -	ret = send(vs->csock, vs->output.buffer, vs->output.offset, 0);
> -    ret = vnc_client_io_error(vs, ret, socket_error());
> +	ret = send(vs->csock, data, datalen, 0);
> +    VNC_DEBUG("Wrote wire %p %d -> %ld\n", data, datalen, ret);
> +    return vnc_client_io_error(vs, ret, socket_error());
> +}
>   

These changes seem extraneous.  I'll just assume the patch needs cleanup 
for such things.

> +#ifdef CONFIG_VNC_SASL
> +static long vnc_client_write_sasl(VncState *vs)
> +{
> +    long ret;
> +
> +    VNC_DEBUG("Write SASL: Pending output %p size %d offset %d Encoded: %p size %d offset %d\n",
> +              vs->output.buffer, vs->output.capacity, vs->output.offset,
> +              vs->saslEncoded, vs->saslEncodedLength, vs->saslEncodedOffset);
> +
> +    if (!vs->saslEncoded) {
> +        int err;
> +        err = sasl_encode(vs->saslconn,
> +                          (char *)vs->output.buffer,
> +                          vs->output.offset,
> +                          (const char **)&vs->saslEncoded,
> +                          &vs->saslEncodedLength);
> +        if (err != SASL_OK)
> +            return vnc_client_io_error(vs, -1, EIO);
> +
> +        vs->output.offset = 0;
> +        vs->saslEncodedOffset = 0;
> +    }
> +
> +    ret = vnc_client_write_wire(vs,
> +                                vs->saslEncoded + vs->saslEncodedOffset,
> +                                vs->saslEncodedLength - vs->saslEncodedOffset);
>      if (!ret)
> -	return;
> +        return 0;
>  
> +    vs->saslEncodedOffset += ret;
> +    if (vs->saslEncodedOffset == vs->saslEncodedLength) {
> +        vs->saslEncoded = NULL;
> +        vs->saslEncodedOffset = vs->saslEncodedLength = 0;
> +    }
> +
> +    /* Can't merge this block with one above, because
> +     * someone might have written more unencrypted
> +     * data in vs->output while we were processing
> +     * SASL encoded output
> +     */
> +    if (vs->output.offset == 0) {
> +	qemu_set_fd_handler2(vs->csock, NULL, vnc_client_read, NULL, vs);
> +    }
> +
> +    return ret;
> +}
> +#endif /* CONFIG_VNC_SASL */
> +
> +static long vnc_client_write_plain(VncState *vs)
> +{
> +    long ret;
> +
> +#ifdef CONFIG_VNC_SASL
> +    VNC_DEBUG("Write Plain: Pending output %p size %d offset %d. Wait SSF %d\n",
> +              vs->output.buffer, vs->output.capacity, vs->output.offset,
> +              vs->saslWaitWriteSSF);
> +
> +    if (vs->saslconn &&
> +        vs->saslRunSSF &&
> +        vs->saslWaitWriteSSF) {
> +        ret = vnc_client_write_wire(vs, vs->output.buffer, vs->saslWaitWriteSSF);
> +        if (ret)
> +            vs->saslWaitWriteSSF -= ret;
> +    } else
> +#endif /* CONFIG_VNC_SASL */
> +        ret = vnc_client_write_wire(vs, vs->output.buffer, vs->output.offset);
> +    if (!ret)
> +        return 0;
> +
>      memmove(vs->output.buffer, vs->output.buffer + ret, (vs->output.offset - ret));
>      vs->output.offset -= ret;
>  
>      if (vs->output.offset == 0) {
>  	qemu_set_fd_handler2(vs->csock, NULL, vnc_client_read, NULL, vs);
>      }
> +
> +    return ret;
>  }
>   

So there are three possible layers of write interaction.  SASL -> TLS -> 
Socket.  I think adding a mechanism to register write hooks to implement 
SASL and TLS would be good.  If you can, moving the TLS support into a 
separate file would be helpful too.

>  
> +#ifdef CONFIG_VNC_SASL
> +/* Max amount of data we send/recv for SASL steps to prevent DOS */
> +#define SASL_DATA_MAX_LEN (1024 * 1024)

Be careful with this.  Kerberos tickets can contain arbitrary data (for 
instance, the MS PAC).  This can make the data transfer size be much 
larger than you'd expect.

I like the general idea here.  SASL is a nice mechanism and the 
implementation is pretty straight forward.  I'll do a more thorough 
review when you get something closer to ready for inclusion.

Regards,

Anthony Liguori