On Wed, Oct 27, 2010 at 02:36:46PM +0200, Guillaume Destuynder wrote:
> Index: unix/vncviewer/CConn.cxx
> ===================================================================
> --- unix/vncviewer/CConn.cxx (revision 4175)
> +++ unix/vncviewer/CConn.cxx (working copy)
> +// getCertificateReply() is called by the CSecurity object when it needs to
> get
> +// a reply from the user (display/accept certificate dialog).
>
> +int CConn::getCertificateReply(char* cert)
> +{
> + TXMsgBox msgBox(dpy, cert, MB_YESNO, "VNC Viewer: Confirm certificate
> information");
> + if (msgBox.show())
> + return 0;
> + return 1;
> +}
Basic question: Wouldn't a generic messagebox (like in
http://e9925248.users.sourceforge.net/0005-client-side-TLS-tunnel.patch
File common/rfb/UserMsgBox.h) be more useful?
With this approch, each SecurityType need it's own callback function
to ask things.
> +int CConn::saveCertificate(char *data)
> +{
> + char *homeDir = getenv("HOME");
> + if (!homeDir) {
> + vlog.error("Could not find environement variable $HOME");
> + return 1;
> + }
> +
> + CharArray vncDir(strlen(homeDir)+6);
> + sprintf(vncDir.buf, "%s/.vnc", homeDir);
> + CharArray certFile(strlen(vncDir.buf)+12);
> + sprintf(certFile.buf, "%s/x509.certs", vncDir.buf);
> +
> + int result = mkdir(vncDir.buf, 0755);
> + if (result == -1 && errno != EEXIST) {
> + vlog.error("Could not create .vnc directory: %s.", strerror(errno));
> + return 1;
> + }
The vncDir mkdir code is the same as in main of vncviewer.cpp - maybe something
could be merged.
> + FILE *f = fopen(certFile.buf, "w+");
> +
> + if (!f) {
> + vlog.error("Couldnt open/create .vnc/x509.certs file: %s.",
> strerror(errno));
> + return 1;
> + }
> + fprintf(f, "%s", data);
> + fclose(f);
> + return 0;
> +}
So it only stores the last used certificate. Users with more than one
vnc server can take no advantage, as the certificate is overwritten
again and again.
> // CConnection callback methods
>
> Index: common/rfb/CSecurityTLS.cxx
> ===================================================================
> --- common/rfb/CSecurityTLS.cxx (revision 4175)
> +++ common/rfb/CSecurityTLS.cxx (working copy)
> @@ -2,6 +2,7 @@
> * Copyright (C) 2004 Red Hat Inc.
> * Copyright (C) 2005 Martin Koegler
> * Copyright (C) 2010 TigerVNC Team
> + * Copyright (C) 2010 m-privacy GmbH
> *
> * This is free software; you can redistribute it and/or modify
> * it under the terms of the GNU General Public License as published by
> @@ -27,6 +28,9 @@
> #error "This header should not be compiled without HAVE_GNUTLS defined"
> #endif
>
> +#include <stdlib.h>
> +#include <unistd.h>
> +
> #include <rfb/CSecurityTLS.h>
> #include <rfb/SSecurityVeNCrypt.h>
> #include <rfb/CConnection.h>
> @@ -34,6 +38,7 @@
> #include <rfb/Exception.h>
> #include <rdr/TLSInStream.h>
> #include <rdr/TLSOutStream.h>
> +#include <os/tilde.h>
>
> #include <gnutls/x509.h>
>
> @@ -41,7 +46,7 @@
>
> using namespace rfb;
>
> -StringParameter CSecurityTLS::x509ca("x509ca", "X509 CA certificate", "",
> ConfViewer);
> +StringParameter CSecurityTLS::x509ca("x509ca", "X509 CA certificate",
> "~/.vnc/x509.certs", ConfViewer);
If you intend to use the last saved certificate by default, why do you
use two different constructions patterns for this file name (with one being
fixed)?
> StringParameter CSecurityTLS::x509crl("x509crl", "X509 CRL file", "",
> ConfViewer);
>
> static LogWriter vlog("TLS");
> @@ -72,8 +77,12 @@
> CSecurityTLS::CSecurityTLS(bool _anon) : session(0), anon_cred(0),
> anon(_anon), fis(0), fos(0)
> {
> - cafile = x509ca.getData();
> - crlfile = x509crl.getData();
> + cafile = tilde_expand(x509ca.getData());
> + if (access(cafile, F_OK))
> + cafile = "";
> + crlfile = tilde_expand(x509crl.getData());
> + if (access(crlfile, F_OK))
> + crlfile = "";
> }
~ expansion in parameters passed via command line is not very common for unix
commands.
If you want ~ expansion in a shell script, you don't escape - if you
want, you pass the ~ as '~'. This would break this.
> void CSecurityTLS::shutdown()
> @@ -206,6 +215,7 @@
> const gnutls_datum *cert_list;
> unsigned int cert_list_size = 0;
> unsigned int i;
> + gnutls_datum_t info;
>
> if (anon)
> return;
> @@ -226,12 +236,18 @@
> throw AuthFailureException("certificate verification failed");
> }
>
> - if (status & GNUTLS_CERT_SIGNER_NOT_FOUND)
> - throw AuthFailureException("certificate issuer unknown");
> + if (status & GNUTLS_CERT_REVOKED) {
> + throw AuthFailureException("certificate has been revoked");
> + }
>
> - if (status & GNUTLS_CERT_INVALID)
> - throw AuthFailureException("certificate not trusted");
> + if (status & GNUTLS_CERT_EXPIRED) {
> + throw AuthFailureException("certificate has expired");
> + }
Wishlist: Being able to ignore expired certificates via yes/no question.
> + if (status & GNUTLS_CERT_NOT_ACTIVATED) {
> + throw AuthFailureException("certificate has not been activated");
> + }
> +
> for (i = 0; i < cert_list_size; i++) {
> gnutls_x509_crt crt;
> gnutls_x509_crt_init(&crt);
> @@ -239,12 +255,61 @@
> if (gnutls_x509_crt_import(crt, &cert_list[i],GNUTLS_X509_FMT_DER) < 0)
> throw AuthFailureException("Decoding of certificate failed");
>
> + if (gnutls_x509_crt_print(crt, GNUTLS_CRT_PRINT_ONELINE, &info)) {
> + gnutls_free(info.data);
> + throw AuthFailureException("Could not find certificate to display");
> + }
> +
> if (gnutls_x509_crt_check_hostname(crt, client->getServerName()) == 0) {
> -#if 0
> - throw AuthFailureException("Hostname mismatch"); /* Non-fatal for
> now... */
> -#endif
> + char buf[255];
> + sprintf(buf, "Hostname (%s) does not match any certificate, do you
> want to continue?", client->getServerName());
> + vlog.debug("hostname mistmatch");
> + if ((CSecurityTLS::ctd)->getCertificateReply(buf)) {
> + exit(1);
I would throw an AuthFailureException here.
> + }
> }
> +
> + if (status & GNUTLS_CERT_SIGNER_NOT_FOUND) {
> + size_t out_size;
> + char *out_buf;
> + char *certinfo;
> + int len = 0;
> +
> + vlog.debug("certificate issuer unknown");
> + vlog.debug("%s", info.data);
> +
> + len = snprintf(NULL, 0, "This certificate has been signed by an
> unknown authority:\n\n%s\n\nDo you want to save it and continue?\n ",
> info.data);
> + if (len < 0)
> + AuthFailureException("certificate decoding error");
> +
> + certinfo = (char*) malloc(len);
> + snprintf(certinfo, len, "This certificate has been signed by an
> unknown authority:\n\n%s\n\nDo you want to save it and continue? ",
> info.data);
> +
> + for (int i=0;i<len-1;i++)
> + if (certinfo[i] == ',' && certinfo[i+1] == ' ')
> + certinfo[i] = '\n';
> +
> + if ((CSecurityTLS::ctd)->getCertificateReply(certinfo)) {
> + free(certinfo);
> + exit(1);
I would throw an AuthFailureException here.
> + }
> + free(certinfo);
> +
> + if (gnutls_x509_crt_export(crt, GNUTLS_X509_FMT_PEM, NULL, &out_size)
> == GNUTLS_E_SHORT_MEMORY_BUFFER)
> + AuthFailureException("out of memory");
> +
> + //Save cert
> + out_buf = (char *) malloc(out_size);
> + if (gnutls_x509_crt_export(crt, GNUTLS_X509_FMT_PEM, out_buf,
> &out_size) < 0)
> + AuthFailureException("certificate issuer unknown, and certificate
> export failed");
> +
> + if ((CSecurityTLS::ctd)->saveCertificate(out_buf))
> + AuthFailureException("certificate save failed");
I wouldn't abort here. Failing to save the certificate is not fatal for all
users.
> + free(out_buf);
> + } else if (status & GNUTLS_CERT_INVALID)
> + throw AuthFailureException("certificate not trusted");
> gnutls_x509_crt_deinit(crt);
> + gnutls_free(info.data);
> }
> }
>
Regards,
Martin Kögler
------------------------------------------------------------------------------
Nokia and AT&T present the 2010 Calling All Innovators-North America contest
Create new apps & games for the Nokia N8 for consumers in U.S. and Canada
$10 million total in prizes - $4M cash, 500 devices, nearly $6M in marketing
Develop with Nokia Qt SDK, Web Runtime, or Java and Publish to Ovi Store
http://p.sf.net/sfu/nokia-dev2dev
_______________________________________________
Tigervnc-devel mailing list
Tigervnc-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/tigervnc-devel
