On Mon, Mar 16, 2015 at 01:17:16PM +0000, Alex Bennée wrote: > > Daniel P. Berrange <berra...@redhat.com> writes: > > > The way the websockets TLS code was integrated into the VNC server > > made it insecure and essentially useless. The only time that the > > websockets TLS support could be used is if the primary VNC server > <snip> > > > > With this patch applied a number of things change > > > > - TLS is not activated for websockets unless the 'tls' flag is > > actually given. > > - Non-TLS websockets connections are dropped if TLS is active > > - The client certificate is validated after handshake completes > > if the 'x509verify' flag is given > > - Separate VNC auth scheme is tracked for websockets server, > > since it makes no sense to try to use VeNCrypt over a TLS > > enabled websockets connection. > > - The separate "VncDisplayTLS ws_tls" field is dropped, since > > the auth setup ensures we can never have multiple TLS sessions. > > I wonder if the mechanical changes to the tls field could be separated > from the logic changes to the handling of authentication and certificate > checking?
They are rather intertwined, because the need for this duplicated TLS field was a result of the way auth was mishandled. So cleaning up one implies cleaning up the other & vica-verca. > > @@ -422,13 +417,6 @@ void vnc_tls_client_cleanup(struct VncState *vs) > > vs->tls.session = NULL; > > } > > g_free(vs->tls.dname); > > -#ifdef CONFIG_VNC_WS > > - if (vs->ws_tls.session) { > > - gnutls_deinit(vs->ws_tls.session); > > - vs->ws_tls.session = NULL; > > - } > > - g_free(vs->ws_tls.dname); > > -#endif /* CONFIG_VNC_WS */ > > I get we have added a bunch of exit cases earlier on that clean-up but > what happens when we do a clean shutdown? Have we just leaked? We deleted the vs->ws_tls field from the struct entirely, so there's never any data to be leaked here. > Perhaps the tls.session cleanup code should be in a shared function? This is patch is a precursor to a major refactoring & cleanup of the TLS code which will reduce code duplication significantly https://github.com/berrange/qemu/commits/qemu-io-channel-4 Regards, Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|