From 9d7f9a1b9120cb40b668a06847181eb9ddb164e2 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Carlos=20Mart=C3=ADn=20Nieto?= Date: Fri, 22 May 2015 19:50:10 +0200 Subject: Lock around encrypted I/O OpenSSL and often whatever libssh2 is using require their own set-up for concurrent operations to be safe. This means that by default, using both of these libraries in a threaded context is unsafe. Lock by default, and allow the user to tell us that they've set up threading for the underlying libraries. --- CHANGELOG.md | 9 ++++++++ THREADING.md | 7 +++++++ include/git2/sys/libssh2.h | 29 ++++++++++++++++++++++++++ include/git2/sys/openssl.h | 16 ++++++++++++++ src/global.c | 9 +++++++- src/global.h | 1 + src/openssl_stream.c | 49 ++++++++++++++++++++++++++++++++++++++++--- src/stream.h | 10 +++++++++ src/transports/ssh.c | 52 ++++++++++++++++++++++++++++++++++++++++------ 9 files changed, 172 insertions(+), 10 deletions(-) create mode 100644 include/git2/sys/libssh2.h diff --git a/CHANGELOG.md b/CHANGELOG.md index 8a0ae0e03..9a3d03cf9 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -49,6 +49,11 @@ support for HTTPS connections insead of OpenSSL. the error message, which allows you to get the "repository not found" messages. +* OpenSSL and libssh2 code now locks by default as a way to have safe + defaults. Use `git_openssl_set_threadsafe()` and + `git_libssh2_set_threadsafe()` resp. to disable the locks if you + have made sure their locking is set up. + ### API additions @@ -98,6 +103,10 @@ support for HTTPS connections insead of OpenSSL. configuration of the server, and tools can use this to show messages about failing to communicate with the server. +* `git_openssl_set_threadsafe()` and `git_libssh2_set_threadsafe()` + have been added to allow the user to specify that they have taken + care of setting up the threading for these libraries. + ### API removals * `git_remote_save()` and `git_remote_clear_refspecs()` has been diff --git a/THREADING.md b/THREADING.md index cf7ac1ff0..e703a6c76 100644 --- a/THREADING.md +++ b/THREADING.md @@ -64,6 +64,13 @@ it should use. This means that libgit2 cannot know what to set as the user of libgit2 may use OpenSSL independently and the locking settings must survive libgit2 shutting down. +As using these libraries without taking extra steps is unsafe, we err +on the side of caution and lock around operations with these +libraries. If you have set up thread safety for these librarires, you +may call `git_openssl_set_threadafe()` and/or +`git_libssh2_set_threadsafe()` which tell libgit2 that the libraries +are thread-safe themselves and there is no need to take these locks. + libgit2 does provide a last-resort convenience function `git_openssl_set_locking()` (available in `sys/openssl.h`) to use the platform-native mutex mechanisms to perform the locking, which you may diff --git a/include/git2/sys/libssh2.h b/include/git2/sys/libssh2.h new file mode 100644 index 000000000..ca1dec9fb --- /dev/null +++ b/include/git2/sys/libssh2.h @@ -0,0 +1,29 @@ +/* + * Copyright (C) the libgit2 contributors. All rights reserved. + * + * This file is part of libgit2, distributed under the GNU GPL v2 with + * a Linking Exception. For full terms see the included COPYING file. + */ +#ifndef INCLUDE_git_libssh2_h__ +#define INCLUDE_git_libssh2_h__ + +#include "git2/common.h" + +GIT_BEGIN_DECL + +/** + * Mark the libssh2 code as thread-safe + * + * By default we take a lock around libssh2 operations, as the + * thread-safety depends on the caller setting up the threading for + * the crytographic library it uses. If you have set up its threading, + * you may call this function to disable the lock, which would enable + * concurrent work. + * + * These locks are only used if the library was built with threading + * support. + */ +GIT_EXTERN(void) git_libssh2_set_threadsafe(void); + +GIT_END_DECL +#endif diff --git a/include/git2/sys/openssl.h b/include/git2/sys/openssl.h index b41c55c6d..5a5fc8f03 100644 --- a/include/git2/sys/openssl.h +++ b/include/git2/sys/openssl.h @@ -28,11 +28,27 @@ GIT_BEGIN_DECL * likely sets up locking. You should very strongly prefer that over * this function. * + * This calls `git_openssl_set_threadsafe()` enabling concurrent + * OpenSSL operations. + * * @return 0 on success, -1 if there are errors or if libgit2 was not * built with OpenSSL and threading support. */ GIT_EXTERN(int) git_openssl_set_locking(void); +/** + * Mark the OpenSSL code as thread-safe + * + * By default we take a lock around OpenSSL operations. If you have + * set up OpenSSL threading in your application, you may call this + * function to avoid taking these locks, which would enable concurrent + * work. + * + * These locks are only used if the library was built with threading + * support. + */ +GIT_EXTERN(void) git_openssl_set_threadsafe(void); + GIT_END_DECL #endif diff --git a/src/global.c b/src/global.c index 9f1a0bf10..f4005fa30 100644 --- a/src/global.c +++ b/src/global.c @@ -14,6 +14,7 @@ git_mutex git__mwindow_mutex; +git_mutex git__io_mutex; #define MAX_SHUTDOWN_CB 8 @@ -135,6 +136,7 @@ int git_openssl_set_locking(void) CRYPTO_set_locking_callback(openssl_locking_function); git__on_shutdown(shutdown_ssl_locking); + git_openssl_set_threadsafe(); return 0; # else giterr_set(GITERR_THREAD, "libgit2 as not built with threads"); @@ -190,7 +192,8 @@ static int synchronized_threads_init(void) int error; _tls_index = TlsAlloc(); - if (git_mutex_init(&git__mwindow_mutex)) + if (git_mutex_init(&git__mwindow_mutex) || + git_mutex_init(&git__io_mutex)) return -1; /* Initialize any other subsystems that have global state */ @@ -230,6 +233,7 @@ static void synchronized_threads_shutdown(void) TlsFree(_tls_index); git_mutex_free(&git__mwindow_mutex); + git_mutex_free(&git__io_mutex); } int git_libgit2_shutdown(void) @@ -299,6 +303,8 @@ static void init_once(void) { if ((init_error = git_mutex_init(&git__mwindow_mutex)) != 0) return; + if ((init_error = git_mutex_init(&git__io_mutex)) != 0) + return; pthread_key_create(&_tls_key, &cb__free_status); @@ -342,6 +348,7 @@ int git_libgit2_shutdown(void) pthread_key_delete(_tls_key); git_mutex_free(&git__mwindow_mutex); + git_mutex_free(&git__io_mutex); _once_init = new_once; return 0; diff --git a/src/global.h b/src/global.h index fdad6ba89..93ba7ddc7 100644 --- a/src/global.h +++ b/src/global.h @@ -25,6 +25,7 @@ extern SSL_CTX *git__ssl_ctx; git_global_st *git__global_state(void); extern git_mutex git__mwindow_mutex; +extern git_mutex git__io_mutex; #define GIT_GLOBAL (git__global_state()) diff --git a/src/openssl_stream.c b/src/openssl_stream.c index 9b2d5951c..e3d63aef4 100644 --- a/src/openssl_stream.c +++ b/src/openssl_stream.c @@ -26,6 +26,17 @@ #include #include +/* + * Whether we should take a lock around OpenSSL operations, the user + * can disable this if they've initialised OpenSSL's threading. + */ +static int should_lock = 1; + +void git_openssl_set_threadsafe(void) +{ + should_lock = 0; +} + static int ssl_set_error(SSL *ssl, int error) { int err; @@ -75,6 +86,8 @@ static int ssl_teardown(SSL *ssl) { int ret; + LOCK; + ret = SSL_shutdown(ssl); if (ret < 0) ret = ssl_set_error(ssl, ret); @@ -82,6 +95,9 @@ static int ssl_teardown(SSL *ssl) ret = 0; SSL_free(ssl); + + UNLOCK; + return ret; } @@ -109,7 +125,10 @@ static int verify_server_cert(SSL *ssl, const char *host) void *addr; int i = -1,j; + LOCK; + if (SSL_get_verify_result(ssl) != X509_V_OK) { + UNLOCK; giterr_set(GITERR_SSL, "The SSL certificate is invalid"); return GIT_ECERTIFICATE; } @@ -128,6 +147,7 @@ static int verify_server_cert(SSL *ssl, const char *host) cert = SSL_get_peer_certificate(ssl); if (!cert) { + UNLOCK; giterr_set(GITERR_SSL, "the server did not provide a certificate"); return -1; } @@ -167,8 +187,10 @@ static int verify_server_cert(SSL *ssl, const char *host) if (matched == 0) goto cert_fail_name; - if (matched == 1) + if (matched == 1) { + UNLOCK; return 0; + } /* If no alternative names are available, check the common name */ peer_name = X509_get_subject_name(cert); @@ -213,10 +235,12 @@ static int verify_server_cert(SSL *ssl, const char *host) return 0; on_error: + UNLOCK; OPENSSL_free(peer_cn); return ssl_set_error(ssl, 0); cert_fail_name: + UNLOCK; OPENSSL_free(peer_cn); giterr_set(GITERR_SSL, "hostname does not match certificate"); return GIT_ECERTIFICATE; @@ -239,7 +263,10 @@ int openssl_connect(git_stream *stream) if ((ret = git_stream_connect((git_stream *)st->socket)) < 0) return ret; + LOCK; + if ((ret = SSL_set_fd(st->ssl, st->socket->s)) <= 0) { + UNLOCK; openssl_close((git_stream *) st); return ssl_set_error(st->ssl, ret); } @@ -247,7 +274,10 @@ int openssl_connect(git_stream *stream) /* specify the host in case SNI is needed */ SSL_set_tlsext_host_name(st->ssl, st->socket->host); - if ((ret = SSL_connect(st->ssl)) <= 0) + ret = SSL_connect(st->ssl); + UNLOCK; + + if (ret <= 0) return ssl_set_error(st->ssl, ret); return verify_server_cert(st->ssl, st->socket->host); @@ -260,9 +290,12 @@ int openssl_certificate(git_cert **out, git_stream *stream) X509 *cert = SSL_get_peer_certificate(st->ssl); unsigned char *guard, *encoded_cert; + LOCK; + /* Retrieve the length of the certificate first */ len = i2d_X509(cert, NULL); if (len < 0) { + UNLOCK; giterr_set(GITERR_NET, "failed to retrieve certificate information"); return -1; } @@ -273,6 +306,7 @@ int openssl_certificate(git_cert **out, git_stream *stream) guard = encoded_cert; len = i2d_X509(cert, &guard); + UNLOCK; if (len < 0) { git__free(encoded_cert); giterr_set(GITERR_NET, "failed to retrieve certificate information"); @@ -294,7 +328,10 @@ ssize_t openssl_write(git_stream *stream, const char *data, size_t len, int flag GIT_UNUSED(flags); - if ((ret = SSL_write(st->ssl, data, len)) <= 0) { + LOCK; + ret = SSL_write(st->ssl, data, len); + UNLOCK; + if (ret <= 0) { return ssl_set_error(st->ssl, ret); } @@ -306,9 +343,13 @@ ssize_t openssl_read(git_stream *stream, void *data, size_t len) openssl_stream *st = (openssl_stream *) stream; int ret; + + LOCK; if ((ret = SSL_read(st->ssl, data, len)) <= 0) ssl_set_error(st->ssl, ret); + UNLOCK; + return ret; } @@ -342,7 +383,9 @@ int git_openssl_stream_new(git_stream **out, const char *host, const char *port) if (git_socket_stream_new((git_stream **) &st->socket, host, port)) return -1; + LOCK; st->ssl = SSL_new(git__ssl_ctx); + UNLOCK; if (st->ssl == NULL) { giterr_set(GITERR_SSL, "failed to create ssl object"); return -1; diff --git a/src/stream.h b/src/stream.h index d810e704d..0a9fc187b 100644 --- a/src/stream.h +++ b/src/stream.h @@ -50,4 +50,14 @@ GIT_INLINE(void) git_stream_free(git_stream *st) st->free(st); } +/* Macros for locking around OpenSSL and libssh2; it assumes you have a static 'should_lock' variable */ +#define LOCK do { \ + if (should_lock && git_mutex_lock(&git__io_mutex) < 0) { \ + giterr_set(GITERR_NET, "failed to lock IO mutex"); \ + return -1; \ + } \ +} while(0) + +#define UNLOCK git_mutex_unlock(&git__io_mutex) + #endif diff --git a/src/transports/ssh.c b/src/transports/ssh.c index 55f715b1d..cc25dcc78 100644 --- a/src/transports/ssh.c +++ b/src/transports/ssh.c @@ -15,6 +15,15 @@ #include "smart.h" #include "cred.h" #include "socket_stream.h" +#include "global.h" +#include "stream.h" + +static int should_lock = 1; + +void git_libssh2_set_threadsafe(void) +{ + should_lock = 0; +} #ifdef GIT_SSH @@ -97,7 +106,9 @@ static int send_command(ssh_stream *s) if (error < 0) goto cleanup; + LOCK; error = libssh2_channel_exec(s->channel, request.ptr); + UNLOCK; if (error < LIBSSH2_ERROR_NONE) { ssh_error(s->session, "SSH could not execute request"); goto cleanup; @@ -124,7 +135,10 @@ static int ssh_stream_read( if (!s->sent_command && send_command(s) < 0) return -1; - if ((rc = libssh2_channel_read(s->channel, buffer, buf_size)) < LIBSSH2_ERROR_NONE) { + LOCK; + rc = libssh2_channel_read(s->channel, buffer, buf_size); + UNLOCK; + if (rc < LIBSSH2_ERROR_NONE) { ssh_error(s->session, "SSH could not read data"); return -1; } @@ -134,9 +148,14 @@ static int ssh_stream_read( * not-found error, so read from stderr and signal EOF on * stderr. */ - if (rc == 0 && (rc = libssh2_channel_read_stderr(s->channel, buffer, buf_size)) > 0) { - giterr_set(GITERR_SSH, "%*s", rc, buffer); - return GIT_EEOF; + if (rc == 0) { + LOCK; + rc = libssh2_channel_read_stderr(s->channel, buffer, buf_size); + UNLOCK; + if (rc > 0) { + giterr_set(GITERR_SSH, "%*s", rc, buffer); + return GIT_EEOF; + } } @@ -158,7 +177,9 @@ static int ssh_stream_write( return -1; do { + LOCK; ret = libssh2_channel_write(s->channel, buffer + off, len - off); + UNLOCK; if (ret < 0) break; @@ -272,10 +293,15 @@ static int ssh_agent_auth(LIBSSH2_SESSION *session, git_cred_ssh_key *c) { struct libssh2_agent_publickey *curr, *prev = NULL; - LIBSSH2_AGENT *agent = libssh2_agent_init(session); + LIBSSH2_AGENT *agent; + + agent = libssh2_agent_init(session); - if (agent == NULL) + if (agent == NULL) { + UNLOCK; + giterr_set(GITERR_SSH, "failed to initialize agent connection"); return -1; + } rc = libssh2_agent_connect(agent); @@ -311,6 +337,7 @@ shutdown: libssh2_agent_disconnect(agent); libssh2_agent_free(agent); + UNLOCK; return rc; } @@ -321,6 +348,7 @@ static int _git_ssh_authenticate_session( { int rc; + LOCK; do { giterr_clear(); switch (cred->credtype) { @@ -374,6 +402,7 @@ static int _git_ssh_authenticate_session( rc = LIBSSH2_ERROR_AUTHENTICATION_FAILED; } } while (LIBSSH2_ERROR_EAGAIN == rc || LIBSSH2_ERROR_TIMEOUT == rc); + UNLOCK; if (rc == LIBSSH2_ERROR_PASSWORD_EXPIRED || rc == LIBSSH2_ERROR_AUTHENTICATION_FAILED) return GIT_EAUTH; @@ -434,8 +463,10 @@ static int _git_ssh_session_create( assert(session); + LOCK; s = libssh2_session_init(); if (!s) { + UNLOCK; giterr_set(GITERR_NET, "Failed to initialize SSH session"); return -1; } @@ -443,6 +474,7 @@ static int _git_ssh_session_create( do { rc = libssh2_session_startup(s, socket->s); } while (LIBSSH2_ERROR_EAGAIN == rc || LIBSSH2_ERROR_TIMEOUT == rc); + UNLOCK; if (rc != LIBSSH2_ERROR_NONE) { ssh_error(s, "Failed to start SSH session"); @@ -495,6 +527,7 @@ static int _git_ssh_setup_conn( (error = git_stream_connect(s->io)) < 0) goto done; + LOCK; if ((error = _git_ssh_session_create(&session, s->io)) < 0) goto done; @@ -517,6 +550,7 @@ static int _git_ssh_setup_conn( } if (cert.type == 0) { + UNLOCK; giterr_set(GITERR_SSH, "unable to get the host key"); error = -1; goto done; @@ -529,12 +563,14 @@ static int _git_ssh_setup_conn( error = t->owner->certificate_check_cb((git_cert *) cert_ptr, 0, host, t->owner->message_cb_payload); if (error < 0) { + UNLOCK; if (!giterr_last()) giterr_set(GITERR_NET, "user cancelled hostkey check"); goto done; } } + UNLOCK; /* we need the username to ask for auth methods */ if (!user) { @@ -580,7 +616,9 @@ static int _git_ssh_setup_conn( if (error < 0) goto done; + LOCK; channel = libssh2_channel_open_session(session); + UNLOCK; if (!channel) { error = -1; ssh_error(session, "Failed to open SSH channel"); @@ -726,7 +764,9 @@ static int list_auth_methods(int *out, LIBSSH2_SESSION *session, const char *use *out = 0; + LOCK; list = libssh2_userauth_list(session, username, strlen(username)); + UNLOCK; /* either error, or the remote accepts NONE auth, which is bizarre, let's punt */ if (list == NULL && !libssh2_userauth_authenticated(session)) -- cgit v1.2.1