[Spice-devel] [PATCH 9/9] client: make use of ssl_verify.c
Christophe Fergeau
cfergeau at redhat.com
Sat Apr 30 01:33:52 PDT 2011
On Tue, Jan 25, 2011 at 07:17:28PM +0100, Marc-André Lureau wrote:
> ---
> client/application.cpp | 20 ++-
> client/red_client.cpp | 2 +-
> client/red_peer.cpp | 374 +++---------------------------------------------
> client/red_peer.h | 23 +---
> 4 files changed, 36 insertions(+), 383 deletions(-)
>
> diff --git a/client/application.cpp b/client/application.cpp
> index d1aef1a..4601df7 100644
> --- a/client/application.cpp
> +++ b/client/application.cpp
> @@ -351,7 +351,7 @@ Application::Application()
> , _monitors (NULL)
> , _title ("SPICEc:%d")
> , _sys_key_intercept_mode (false)
> - , _enable_controller (false)
> + , _enable_controller (false)
> #ifdef USE_GUI
> , _gui_mode (GUI_MODE_FULL)
> #endif // USE_GUI
> @@ -392,7 +392,7 @@ Application::Application()
> _canvas_types[0] = CANVAS_OPTION_SW;
> #endif
>
> - _host_auth_opt.type_flags = RedPeer::HostAuthOptions::HOST_AUTH_OP_NAME;
> + _host_auth_opt.type_flags = SPICE_SSL_VERIFY_OP_HOSTNAME;
>
> Platform::get_app_data_dir(_host_auth_opt.CA_file, app_name);
> Platform::path_append(_host_auth_opt.CA_file, CA_FILE_NAME);
> @@ -1996,9 +1996,12 @@ bool Application::set_host_cert_subject(const char* subject, const char* arg0)
> std::string subject_str(subject);
> std::string::const_iterator iter = subject_str.begin();
> std::string entry;
> - _host_auth_opt.type_flags = RedPeer::HostAuthOptions::HOST_AUTH_OP_SUBJECT;
> - _host_auth_opt.host_subject.clear();
> + _host_auth_opt.type_flags = SPICE_SSL_VERIFY_OP_SUBJECT;
> + _host_auth_opt.host_subject = subject;
>
> + /* the follow is only checking code, subject is parsed later
> + ssl_verify.c. It could be removed, along with
> + CertFieldValuePair... */
I don't think it has much added value and could confuse some people, so I'd
just drop it.
> while (true) {
> if ((iter == subject_str.end()) || (*iter == ',')) {
> RedPeer::HostAuthOptions::CertFieldValuePair entry_pair;
> @@ -2011,7 +2014,6 @@ bool Application::set_host_cert_subject(const char* subject, const char* arg0)
> }
> entry_pair.first = entry.substr(0, value_pos);
> entry_pair.second = entry.substr(value_pos + 1);
> - _host_auth_opt.host_subject.push_back(entry_pair);
> DBG(0, "subject entry: %s=%s", entry_pair.first.c_str(), entry_pair.second.c_str());
> if (iter == subject_str.end()) {
> break;
> @@ -2035,6 +2037,7 @@ bool Application::set_host_cert_subject(const char* subject, const char* arg0)
> }
> iter++;
> }
> +
> return true;
> }
>
> @@ -2278,8 +2281,9 @@ bool Application::process_cmd_line(int argc, char** argv)
> #ifdef USE_SMARTCARD
> parser.add(SPICE_OPT_SMARTCARD, "smartcard", "enable smartcard channel");
> parser.add(SPICE_OPT_NOSMARTCARD, "nosmartcard", "disable smartcard channel");
> - parser.add(SPICE_OPT_SMARTCARD_CERT, "smartcard-cert", "Use virtual reader+card with given cert(s)",
> - "smartcard-cert", true);
> + parser.add(SPICE_OPT_SMARTCARD_CERT, "smartcard-cert",
> + "Use virtual reader+card with given cert(s)",
> + "smartcard-cert", true);
Unrelated white space cleanup, let's say it went unnoticed :)
> parser.set_multi(SPICE_OPT_SMARTCARD_CERT, ',');
> parser.add(SPICE_OPT_SMARTCARD_DB, "smartcard-db", "Use given db for smartcard certs");
> #endif
> @@ -2506,7 +2510,7 @@ void spice_log(unsigned int type, const char *function, const char *format, ...)
> Platform::get_thread_id(),
> function_to_func_name(function).c_str(),
> formated_message.c_str());
> - fflush(log_file);
> + fflush(log_file);
> }
>
> if (type >= LOG_WARN) {
> diff --git a/client/red_client.cpp b/client/red_client.cpp
> index c632a21..494f30a 100644
> --- a/client/red_client.cpp
> +++ b/client/red_client.cpp
> @@ -267,7 +267,7 @@ void Migrate::start(const SpiceMsgMainMigrationBegin* migrate)
> _host.assign((char *)migrate->host_data);
> _port = migrate->port ? migrate->port : -1;
> _sport = migrate->sport ? migrate->sport : -1;
> - _auth_options.type_flags = RedPeer::HostAuthOptions::HOST_AUTH_OP_PUBKEY;
> + _auth_options.type_flags = SPICE_SSL_VERIFY_OP_PUBKEY;
> _auth_options.host_pubkey.assign(migrate->pub_key_data, migrate->pub_key_data + migrate->pub_key_size);
> }
>
> diff --git a/client/red_peer.cpp b/client/red_peer.cpp
> index 6ff5844..5b9b82c 100644
> --- a/client/red_peer.cpp
> +++ b/client/red_peer.cpp
> @@ -24,12 +24,9 @@
> #include "utils.h"
> #include "debug.h"
> #include "platform_utils.h"
> +#include "ssl_verify.h"
>
> -typedef struct SslVerifyCbData {
> - RedPeer::HostAuthOptions info;
> - const char* host_name;
> - bool all_preverify_ok;
> -} SslVerifyCbData;
> +#include "../common/ssl_verify.c"
I don't think this will be needed if you add the extern "C" in the other
patch.
>
> static void ssl_error()
> {
> @@ -132,346 +129,12 @@ void RedPeer::connect_unsecure(const char* host, int portnr)
> }
> }
>
> -bool RedPeer::verify_pubkey(X509* cert, const HostAuthOptions::PublicKey& key)
> -{
> - EVP_PKEY* cert_pubkey = NULL;
> - EVP_PKEY* orig_pubkey = NULL;
> - BIO* bio = NULL;
> - uint8_t* c_key = NULL;
> - int ret = 0;
> -
> - if (key.empty()) {
> - return false;
> - }
> -
> - ASSERT(cert);
> -
> - try {
> - cert_pubkey = X509_get_pubkey(cert);
> - if (!cert_pubkey) {
> - THROW("reading public key from certificate failed");
> - }
> -
> - c_key = new uint8_t[key.size()];
> - memcpy(c_key, &key[0], key.size());
> -
> - bio = BIO_new_mem_buf((void*)c_key, key.size());
> - if (!bio) {
> - THROW("creating BIO failed");
> - }
> -
> - orig_pubkey = d2i_PUBKEY_bio(bio, NULL);
> - if (!orig_pubkey) {
> - THROW("reading pubkey from bio failed");
> - }
> -
> - ret = EVP_PKEY_cmp(orig_pubkey, cert_pubkey);
> -
> - BIO_free(bio);
> - EVP_PKEY_free(orig_pubkey);
> - EVP_PKEY_free(cert_pubkey);
> - delete []c_key;
> - if (ret == 1) {
> - DBG(0, "public keys match");
> - return true;
> - } else if (ret == 0) {
> - DBG(0, "public keys mismatch");
> - return false;
> - } else {
> - DBG(0, "public keys types mismatch");
> - return false;
> - }
> - } catch (Exception& e) {
> - LOG_WARN("%s", e.what());
> -
> - if (bio) {
> - BIO_free(bio);
> - }
> -
> - if (orig_pubkey) {
> - EVP_PKEY_free(orig_pubkey);
> - }
> -
> - if (cert_pubkey) {
> - EVP_PKEY_free(cert_pubkey);
> - }
> - delete []c_key;
> - return false;
> - }
> -}
> -
> -/* From gnutls: compare host_name against certificate, taking account of wildcards.
> - * return true on success or false on error.
> - *
> - * note: cert_name_size is required as X509 certs can contain embedded NULs in
> - * the strings such as CN or subjectAltName
> - */
> -bool RedPeer::x509_cert_host_name_compare(const char *cert_name, int cert_name_size,
> - const char *host_name)
> -{
> - /* find the first different character */
> - for (; *cert_name && *host_name && (toupper(*cert_name) == toupper(*host_name));
> - cert_name++, host_name++, cert_name_size--);
> -
> - /* the strings are the same */
> - if (cert_name_size == 0 && *host_name == '\0')
> - return true;
> -
> - if (*cert_name == '*')
> - {
> - /* a wildcard certificate */
> - cert_name++;
> - cert_name_size--;
> -
> - while (true)
> - {
> - /* Use a recursive call to allow multiple wildcards */
> - if (RedPeer::x509_cert_host_name_compare(cert_name, cert_name_size, host_name)) {
> - return true;
> - }
> -
> - /* wildcards are only allowed to match a single domain
> - component or component fragment */
> - if (*host_name == '\0' || *host_name == '.')
> - break;
> - host_name++;
> - }
> -
> - return false;
> - }
> -
> - return false;
> -}
> -
> -/*
> - * From gnutls_x509_crt_check_hostname - compares the hostname with certificate's hostname
> - *
> - * This function will check if the given certificate's subject matches
> - * the hostname. This is a basic implementation of the matching
> - * described in RFC2818 (HTTPS), which takes into account wildcards,
> - * and the DNSName/IPAddress subject alternative name PKIX extension.
> - *
> - */
> -bool RedPeer::verify_host_name(X509* cert, const char* host_name)
> -{
> - GENERAL_NAMES* subject_alt_names;
> - bool found_dns_name = false;
> - struct in_addr addr;
> - int addr_len = 0;
> - bool cn_match = false;
> -
> - ASSERT(cert);
> -
> - // only IpV4 supported
> - if (inet_aton(host_name, &addr)) {
> - addr_len = sizeof(struct in_addr);
> - }
> -
> - /* try matching against:
> - * 1) a DNS name or IP address as an alternative name (subjectAltName) extension
> - * in the certificate
> - * 2) the common name (CN) in the certificate
> - *
> - * either of these may be of the form: *.domain.tld
> - *
> - * only try (2) if there is no subjectAltName extension of
> - * type dNSName
> - */
> -
> -
> - subject_alt_names = (GENERAL_NAMES*)X509_get_ext_d2i(cert, NID_subject_alt_name, NULL, NULL);
> -
> - if (subject_alt_names) {
> - int num_alts = sk_GENERAL_NAME_num(subject_alt_names);
> - for (int i = 0; i < num_alts; i++) {
> - const GENERAL_NAME* name = sk_GENERAL_NAME_value(subject_alt_names, i);
> - if (name->type == GEN_DNS) {
> - found_dns_name = true;
> - if (RedPeer::x509_cert_host_name_compare((char *)ASN1_STRING_data(name->d.dNSName),
> - ASN1_STRING_length(name->d.dNSName),
> - host_name)) {
> - DBG(0, "alt name match=%s", ASN1_STRING_data(name->d.dNSName));
> - GENERAL_NAMES_free(subject_alt_names);
> - return true;
> - }
> - } else if (name->type == GEN_IPADD) {
> - int alt_ip_len = ASN1_STRING_length(name->d.iPAddress);
> - found_dns_name = true;
> - if ((addr_len == alt_ip_len)&&
> - !memcmp(ASN1_STRING_data(name->d.iPAddress), &addr, addr_len)) {
> - DBG(0, "alt name IP match=%s",
> - inet_ntoa(*((struct in_addr*)ASN1_STRING_data(name->d.dNSName))));
> - GENERAL_NAMES_free(subject_alt_names);
> - return true;
> - }
> - }
> - }
> - GENERAL_NAMES_free(subject_alt_names);
> - }
> -
> - if (found_dns_name)
> - {
> - DBG(0, "SubjectAltName mismatch");
> - return false;
> - }
> -
> - /* extracting commonNames */
> - X509_NAME* subject = X509_get_subject_name(cert);
> - if (subject) {
> - int pos = -1;
> - X509_NAME_ENTRY* cn_entry;
> - ASN1_STRING* cn_asn1;
> -
> - while ((pos = X509_NAME_get_index_by_NID(subject, NID_commonName, pos)) != -1) {
> - cn_entry = X509_NAME_get_entry(subject, pos);
> - if (!cn_entry) {
> - continue;
> - }
> - cn_asn1 = X509_NAME_ENTRY_get_data(cn_entry);
> - if (!cn_asn1) {
> - continue;
> - }
> -
> - if (RedPeer::x509_cert_host_name_compare((char*)ASN1_STRING_data(cn_asn1),
> - ASN1_STRING_length(cn_asn1),
> - host_name)) {
> - DBG(0, "common name match=%s", (char*)ASN1_STRING_data(cn_asn1));
> - cn_match = true;
> - break;
> - }
> - }
> - }
> -
> - if (!cn_match) {
> - DBG(0, "common name mismatch");
> - }
> - return cn_match;
> -
> -}
> -
> -bool RedPeer::verify_subject(X509* cert, const HostAuthOptions::CertFieldValueList& subject)
> -{
> - X509_NAME* cert_subject = NULL;
> - HostAuthOptions::CertFieldValueList::const_iterator subject_iter;
> - X509_NAME* in_subject;
> - int ret;
> -
> - ASSERT(cert);
> -
> - cert_subject = X509_get_subject_name(cert);
> - if (!cert_subject) {
> - LOG_WARN("reading certificate subject failed");
> - return false;
> - }
> -
> - if ((size_t)X509_NAME_entry_count(cert_subject) != subject.size()) {
> - DBG(0, "subject mismatch: #entries cert=%d, input=%d",
> - X509_NAME_entry_count(cert_subject), subject.size());
> - return false;
> - }
> -
> - in_subject = X509_NAME_new();
> - if (!in_subject) {
> - LOG_WARN("failed to allocate X509_NAME");
> - return false;
> - }
> -
> - for (subject_iter = subject.begin(); subject_iter != subject.end(); subject_iter++) {
> - if (!X509_NAME_add_entry_by_txt(in_subject,
> - subject_iter->first.c_str(),
> - MBSTRING_UTF8,
> - (const unsigned char*)subject_iter->second.c_str(),
> - subject_iter->second.length(), -1, 0)) {
> - LOG_WARN("failed to add entry %s=%s to X509_NAME",
> - subject_iter->first.c_str(), subject_iter->second.c_str());
> - X509_NAME_free(in_subject);
> - return false;
> - }
> - }
> -
> - ret = X509_NAME_cmp(cert_subject, in_subject);
> - X509_NAME_free(in_subject);
> -
> - if (ret == 0) {
> - DBG(0, "subjects match");
> - return true;
> - } else {
> - DBG(0, "subjects mismatch");
> - return false;
> - }
> -}
> -
> -int RedPeer::ssl_verify_callback(int preverify_ok, X509_STORE_CTX *ctx)
> -{
> - int depth;
> - SSL *ssl;
> - X509* cert;
> - SslVerifyCbData* verify_data;
> - int auth_flags;
> -
> - depth = X509_STORE_CTX_get_error_depth(ctx);
> -
> - ssl = (SSL*)X509_STORE_CTX_get_ex_data(ctx, SSL_get_ex_data_X509_STORE_CTX_idx());
> - if (!ssl) {
> - LOG_WARN("failed to get ssl connection");
> - return 0;
> - }
> -
> - verify_data = (SslVerifyCbData*)SSL_get_app_data(ssl);
> - auth_flags = verify_data->info.type_flags;
> -
> - if (depth > 0) {
> - // if certificate verification failed, we can still authorize the server
> - // if its public key matches the one we hold in the peer_connect_options.
> - if (!preverify_ok) {
> - DBG(0, "openssl verify failed at depth=%d", depth);
> - verify_data->all_preverify_ok = false;
> - if (auth_flags & HostAuthOptions::HOST_AUTH_OP_PUBKEY) {
> - return 1;
> - } else {
> - return 0;
> - }
> - } else {
> - return preverify_ok;
> - }
> - }
> -
> - /* depth == 0 */
> - cert = X509_STORE_CTX_get_current_cert(ctx);
> - if (!cert) {
> - LOG_WARN("failed to get server certificate");
> - return 0;
> - }
> -
> - if (auth_flags & HostAuthOptions::HOST_AUTH_OP_PUBKEY) {
> - if (verify_pubkey(cert, verify_data->info.host_pubkey)) {
> - return 1;
> - }
> - }
> -
> - if (!verify_data->all_preverify_ok || !preverify_ok) {
> - return 0;
> - }
> -
> - if (auth_flags & HostAuthOptions::HOST_AUTH_OP_NAME) {
> - if (verify_host_name(cert, verify_data->host_name)) {
> - return 1;
> - }
> - }
> -
> - if (auth_flags & HostAuthOptions::HOST_AUTH_OP_SUBJECT) {
> - if (verify_subject(cert, verify_data->info.host_subject)) {
> - return 1;
> - }
> - }
> - return 0;
> -}
>
> void RedPeer::connect_secure(const ConnectionOptions& options, const char* host)
> {
> int return_code;
> - int auth_flags;
> - SslVerifyCbData auth_data;
> + SPICE_SSL_VERIFY_OP auth_flags;
> + SpiceOpenSSLVerify* verify = NULL;
>
> connect_unsecure(host, options.secure_port);
> ASSERT(_ctx == NULL && _ssl == NULL && _peer != INVALID_SOCKET);
> @@ -482,27 +145,23 @@ void RedPeer::connect_secure(const ConnectionOptions& options, const char* host)
> #else
> SSL_METHOD *ssl_method = TLSv1_method();
> #endif
> - auth_data.info = options.host_auth;
> - auth_data.host_name = host;
> - auth_data.all_preverify_ok = true;
> -
> _ctx = SSL_CTX_new(ssl_method);
> if (_ctx == NULL) {
> ssl_error();
> }
>
> - auth_flags = auth_data.info.type_flags;
> - if ((auth_flags & RedPeer::HostAuthOptions::HOST_AUTH_OP_NAME) ||
> - (auth_flags & RedPeer::HostAuthOptions::HOST_AUTH_OP_SUBJECT)) {
> - std::string CA_file = auth_data.info.CA_file;
> + auth_flags = options.host_auth.type_flags;
> + if ((auth_flags & SPICE_SSL_VERIFY_OP_HOSTNAME) ||
> + (auth_flags & SPICE_SSL_VERIFY_OP_SUBJECT)) {
> + std::string CA_file = options.host_auth.CA_file;
> ASSERT(!CA_file.empty());
>
> return_code = SSL_CTX_load_verify_locations(_ctx, CA_file.c_str(), NULL);
> if (return_code != 1) {
> - if (auth_flags & RedPeer::HostAuthOptions::HOST_AUTH_OP_PUBKEY) {
> + if (auth_flags & SPICE_SSL_VERIFY_OP_PUBKEY) {
> LOG_WARN("SSL_CTX_load_verify_locations failed, CA_file=%s. "
> "only pubkey authentication is active", CA_file.c_str());
> - auth_data.info.type_flags = RedPeer::HostAuthOptions::HOST_AUTH_OP_PUBKEY;
> + auth_flags = SPICE_SSL_VERIFY_OP_PUBKEY;
> }
> else {
> LOG_WARN("SSL_CTX_load_verify_locations failed CA_file=%s", CA_file.c_str());
> @@ -511,10 +170,6 @@ void RedPeer::connect_secure(const ConnectionOptions& options, const char* host)
> }
> }
>
> - if (auth_flags) {
> - SSL_CTX_set_verify(_ctx, SSL_VERIFY_PEER, ssl_verify_callback);
> - }
> -
> return_code = SSL_CTX_set_cipher_list(_ctx, options.ciphers.c_str());
> if (return_code != 1) {
> LOG_WARN("SSL_CTX_set_cipher_list failed, ciphers=%s", options.ciphers.c_str());
> @@ -526,13 +181,19 @@ void RedPeer::connect_secure(const ConnectionOptions& options, const char* host)
> THROW("create ssl failed");
> }
>
> + verify = spice_openssl_verify_new(
> + _ssl, auth_flags,
> + host,
> + (char*)&options.host_auth.host_pubkey[0],
> + options.host_auth.host_pubkey.size(),
> + options.host_auth.host_subject.c_str());
> +
> BIO* sbio = BIO_new_socket(_peer, BIO_NOCLOSE);
> if (!sbio) {
> THROW("alloc new socket bio failed");
> }
>
> SSL_set_bio(_ssl, sbio, sbio);
> - SSL_set_app_data(_ssl, &auth_data);
>
> return_code = SSL_connect(_ssl);
> if (return_code <= 0) {
> @@ -543,6 +204,7 @@ void RedPeer::connect_secure(const ConnectionOptions& options, const char* host)
> }
> } catch (...) {
> Lock lock(_lock);
> + spice_openssl_verify_free(verify);
> cleanup();
> throw;
> }
A call to spice_openssl_verify_free is needed outside the catch() block
too.
Christophe
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 198 bytes
Desc: not available
URL: <http://lists.freedesktop.org/archives/spice-devel/attachments/20110430/2bbba0be/attachment-0001.pgp>
More information about the Spice-devel
mailing list