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

[Daniel P. Berrange]

On Wed, Feb 04, 2009 at 01:10:59PM -0600, Anthony Liguori wrote:
> Daniel P. Berrange wrote:
> >
> >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.
> >

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

I don't particularly like it either, but need some kind of external
persistent config for the authorization data - and it needs to be
independant of any QEMU machine config file we create, because one
authorization list could be shared across many VMs.

The only other option I consider was to punt the problem on to the
caller, and require them to issue a series of monitor commands to
setup the ACL at startup. I don't particularly like that though
since it'd just be pushing the problem elsewhere.

Suggestions welcome from the audience...

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

I don't think it is desirable to move all the SASL code to an
external file - in particular the interaction at the I/O layer
I think is important to have in one place to make it clear to
read & see the whole picture.

I wouldn't mind moving all this metadata into a  separate struct
though, and just include an instance of that in the main QEMU
client struct. 

Likewise I think its reasonable to move the auth handshake impl
methods into a separate file, since they're nicely self-contained
and quite large pieces of code.

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

It is required actually - you have two layers of buffering here.

The vnc_write() methods, still put stuff into vs->output.buffer
as now. This buffer cannot expect to go straight onto the wire 
though. It may need to be passed through the SASL SSF layer, by
calling sasl_encode() (or sasl_decode() for recv'd data).

For clarity the psuedo-code logical algorithm, is now

  vnc_client_write() 
    if using SASL
       call vnc_client_write_sasl
    else
       call vnc_client_write_plain

  vnc_write_plain()
    call vnc_client_write_wire(vs->output.bufffer)


  vnc_write_sasl()
    data = sasl_encode(vs->output.buffer)
    call vnc_client_write_wire(data)

  vnc_write_wire()
    if using SSL
       call gnutls_write
    else
       call send()



There are a few complications in the SASL path relating to fact
that we have to deal with an intermediate buffer and cannot
guarentee that we will be able to write all of this intermediate
buffer in one go (if the socket gives EAGAIN). There's also a
scenario where we have decided to run SASL, but still have some
clear text data to send before actually switching in the encoding
routines.

The read path basically follows exactly same model / structure.


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

Well, 4 scenarios, and upto 3 layers

 - Direct to socket
 - TLS -> socket
 - SASL -> socket
 - SASL -> TLS -> socket

NB, the last case of SASL -> TLS -> socket, is ordinarily identical to
the TLS -> socket case, because we inform SASL that TLS is providing
a encryption layer, and thus sasl_encode/decode will (likely) become
a plain pass-through.  This is all decided based on the min_ssf/max_ssf
stuff we setup during the SASL auth process. We still do the sasl_decode
step though, because you never know when someone will invent a new SASL
plugin that will want to actually do something here, despite TLS already
providing encryption.

I don't think a write hooks would be particularly helpful at making
this clearer, because  SASL vs TLS use quite different approachs to
how/where the encryption is done. In SASL case you just have encode
and decode methods, and you have to take care of all buffering of
data yourself. In TLS case, you have replacement read/write methods
and TLS hides all the encoding & decoding from you. I think trying
to hide these approaches in a generic wrappers will make it harder
to understand exactly what is going on & who's doing it.

For TLS, it'd certainly be viable to move the auth handshake methods
to separate files since again that stuff is nicely self-contained.

I'd just rather have all the I/O code in one place, since that's
got subtle interactions between the layers.

> > 
> >+#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.

That is a place where I'm not hugely clear on where to draw the line. I
definitely need some kind of limit to avoid DOS attack from the client,
but how big is big enough ? I figured 1 MB would be sufficient for a
Kerberos ticket, unless someone has counter examples from the real
world ?
 
> 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.

FYI, the way I have structured the SASL / TLS / plain layering of I/O
is basically following the exact same model / structure I used in 
libvirt daemon for doing the same, so I'm fairly confident in the way
fits together.

Regards,
Daniel
-- 
|: Red Hat, Engineering, London   -o-   http://people.redhat.com/berrange/ :|
|: http://libvirt.org  -o-  http://virt-manager.org  -o-  http://ovirt.org :|
|: http://autobuild.org       -o-         http://search.cpan.org/~danberr/ :|
|: GnuPG: 7D3B9505  -o-  F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|