From 573d7b209dc17924b94185e02de954b26e7f308c Mon Sep 17 00:00:00 2001 From: Stephen Lombardo Date: Tue, 9 Jul 2013 11:26:40 -0400 Subject: [PATCH 1/5] improve initialization and thread safety for default crypto provider --- src/crypto_impl.c | 65 ++++++++++++++++++++++++++++++++++------------- 1 file changed, 47 insertions(+), 18 deletions(-) diff --git a/src/crypto_impl.c b/src/crypto_impl.c index 589696d7d8..1e7fa9961f 100644 --- a/src/crypto_impl.c +++ b/src/crypto_impl.c @@ -66,7 +66,8 @@ typedef struct { static unsigned int default_flags = DEFAULT_CIPHER_FLAGS; static unsigned char hmac_salt_mask = HMAC_SALT_MASK; - +static unsigned int sqlcipher_activate_count = 0; +static sqlite3_mutex* sqlcipher_provider_mutex = NULL; static sqlcipher_provider *default_provider = NULL; struct codec_ctx { @@ -81,6 +82,7 @@ struct codec_ctx { }; int sqlcipher_register_provider(sqlcipher_provider *p) { + sqlite3_mutex_enter(sqlcipher_provider_mutex); if(default_provider != NULL && default_provider != p) { /* only free the current registerd provider if it has been initialized and it isn't a pointer to the same provider passed to the function @@ -88,6 +90,7 @@ int sqlcipher_register_provider(sqlcipher_provider *p) { sqlcipher_free(default_provider, sizeof(sqlcipher_provider)); } default_provider = p; + sqlite3_mutex_leave(sqlcipher_provider_mutex); return SQLITE_OK; } @@ -99,35 +102,57 @@ sqlcipher_provider* sqlcipher_get_provider() { } void sqlcipher_activate() { - sqlcipher_provider *p; - sqlite3_mutex_enter(sqlite3MutexAlloc(SQLITE_MUTEX_STATIC_MASTER)); - p = sqlcipher_malloc(sizeof(sqlcipher_provider)); - { + sqlite3_mutex_enter(sqlite3_mutex_alloc(SQLITE_MUTEX_STATIC_MASTER)); + + if(sqlcipher_provider_mutex == NULL) { + /* allocate a new mutex to guard access to the provider */ + sqlcipher_provider_mutex = sqlite3_mutex_alloc(SQLITE_MUTEX_FAST); + } + + /* check to see if there is a provider registered at this point + if there no provider registered at this point, register the + default provider */ + if(sqlcipher_get_provider() == NULL) { + sqlcipher_provider *p = sqlcipher_malloc(sizeof(sqlcipher_provider)); #if defined (SQLCIPHER_CRYPTO_CC) - extern int sqlcipher_cc_setup(sqlcipher_provider *p); - sqlcipher_cc_setup(p); + extern int sqlcipher_cc_setup(sqlcipher_provider *p); + sqlcipher_cc_setup(p); #elif defined (SQLCIPHER_CRYPTO_LIBTOMCRYPT) - extern int sqlcipher_ltc_setup(sqlcipher_provider *p); - sqlcipher_ltc_setup(p); + extern int sqlcipher_ltc_setup(sqlcipher_provider *p); + sqlcipher_ltc_setup(p); #elif defined (SQLCIPHER_CRYPTO_OPENSSL) - extern int sqlcipher_openssl_setup(sqlcipher_provider *p); - sqlcipher_openssl_setup(p); + extern int sqlcipher_openssl_setup(sqlcipher_provider *p); + sqlcipher_openssl_setup(p); #else #error "NO DEFAULT SQLCIPHER CRYPTO PROVIDER DEFINED" #endif + sqlcipher_register_provider(p); } - sqlcipher_register_provider(p); - sqlite3_mutex_leave(sqlite3MutexAlloc(SQLITE_MUTEX_STATIC_MASTER)); + sqlcipher_activate_count++; /* increment activation count */ + + sqlite3_mutex_leave(sqlite3_mutex_alloc(SQLITE_MUTEX_STATIC_MASTER)); } void sqlcipher_deactivate() { - sqlite3_mutex_enter(sqlite3MutexAlloc(SQLITE_MUTEX_STATIC_MASTER)); - if(default_provider != NULL) { - sqlcipher_free(default_provider, sizeof(sqlcipher_provider)); - default_provider = NULL; + sqlite3_mutex_enter(sqlite3_mutex_alloc(SQLITE_MUTEX_STATIC_MASTER)); + sqlcipher_activate_count--; + /* if no connections are using sqlcipher, cleanup globals */ + if(sqlcipher_activate_count < 1) { + sqlite3_mutex_enter(sqlcipher_provider_mutex); + if(default_provider != NULL) { + sqlcipher_free(default_provider, sizeof(sqlcipher_provider)); + default_provider = NULL; + } + sqlite3_mutex_leave(sqlcipher_provider_mutex); + + /* last connection closed, free provider mutex*/ + sqlite3_mutex_free(sqlcipher_provider_mutex); + sqlcipher_provider_mutex = NULL; + + sqlcipher_activate_count = 0; /* reset activation count */ } - sqlite3_mutex_leave(sqlite3MutexAlloc(SQLITE_MUTEX_STATIC_MASTER)); + sqlite3_mutex_leave(sqlite3_mutex_alloc(SQLITE_MUTEX_STATIC_MASTER)); } /* constant time memset using volitile to avoid having the memset @@ -235,7 +260,11 @@ static int sqlcipher_cipher_ctx_init(cipher_ctx **iCtx) { ctx->provider = (sqlcipher_provider *) sqlcipher_malloc(sizeof(sqlcipher_provider)); if(ctx->provider == NULL) return SQLITE_NOMEM; + + /* make a copy of the provider to be used for the duration of the context */ + sqlite3_mutex_enter(sqlcipher_provider_mutex); memcpy(ctx->provider, default_provider, sizeof(sqlcipher_provider)); + sqlite3_mutex_leave(sqlcipher_provider_mutex); if((rc = ctx->provider->ctx_init(&ctx->provider_ctx)) != SQLITE_OK) return rc; ctx->key = (unsigned char *) sqlcipher_malloc(CIPHER_MAX_KEY_SZ); From f3389d23a28c730fc004f0ccd3914b8e5a9af77f Mon Sep 17 00:00:00 2001 From: Stephen Lombardo Date: Tue, 9 Jul 2013 11:30:19 -0400 Subject: [PATCH 2/5] mutex around RAND_bytes() --- src/crypto_openssl.c | 61 +++++++++++++++++++++++++++++++------------- 1 file changed, 43 insertions(+), 18 deletions(-) diff --git a/src/crypto_openssl.c b/src/crypto_openssl.c index 698bc74072..9a4719ea8c 100644 --- a/src/crypto_openssl.c +++ b/src/crypto_openssl.c @@ -45,10 +45,12 @@ typedef struct { static unsigned int openssl_external_init = 0; static unsigned int openssl_init_count = 0; - +static sqlite3_mutex* openssl_rand_mutex = NULL; static int sqlcipher_openssl_add_random(void *ctx, void *buffer, int length) { + sqlite3_mutex_enter(openssl_rand_mutex); RAND_add(buffer, length, 0); + sqlite3_mutex_leave(openssl_rand_mutex); return SQLITE_OK; } @@ -59,19 +61,30 @@ static int sqlcipher_openssl_add_random(void *ctx, void *buffer, int length) { sqlcipher_openssl_deactivate() will free the EVP structures. */ static int sqlcipher_openssl_activate(void *ctx) { - /* we'll initialize openssl and increment the internal init counter + /* initialize openssl and increment the internal init counter but only if it hasn't been initalized outside of SQLCipher by this program e.g. on startup */ + sqlite3_mutex_enter(sqlite3_mutex_alloc(SQLITE_MUTEX_STATIC_MASTER)); + if(openssl_init_count == 0 && EVP_get_cipherbyname(CIPHER) != NULL) { + /* if openssl has not yet been initialized by this library, but + a call to get_cipherbyname works, then the openssl library + has been initialized externally already. */ openssl_external_init = 1; } - if(openssl_external_init == 0) { - if(openssl_init_count == 0) { - OpenSSL_add_all_algorithms(); - } - openssl_init_count++; + if(openssl_init_count == 0 && openssl_external_init == 0) { + /* if the library was not externally initialized, then should be now */ + OpenSSL_add_all_algorithms(); } + + if(openssl_rand_mutex == NULL) { + /* allocate a mutex to guard against concurrent calls to RAND_bytes() */ + openssl_rand_mutex = sqlite3_mutex_alloc(SQLITE_MUTEX_FAST); + } + + openssl_init_count++; + sqlite3_mutex_leave(sqlite3_mutex_alloc(SQLITE_MUTEX_STATIC_MASTER)); return SQLITE_OK; } @@ -79,20 +92,22 @@ static int sqlcipher_openssl_activate(void *ctx) { freeing the EVP structures on the final deactivation to ensure that OpenSSL memory is cleaned up */ static int sqlcipher_openssl_deactivate(void *ctx) { - sqlite3_mutex_enter(sqlite3MutexAlloc(SQLITE_MUTEX_STATIC_MASTER)); - /* If it is initialized externally, then the init counter should never be greater than zero. - This should prevent SQLCipher from "cleaning up" openssl - when something else in the program might be using it. */ - if(openssl_external_init == 0) { - openssl_init_count--; - /* if the counter reaches zero after it's decremented release EVP memory + sqlite3_mutex_enter(sqlite3_mutex_alloc(SQLITE_MUTEX_STATIC_MASTER)); + openssl_init_count--; + + if(openssl_init_count == 0) { + if(openssl_external_init == 0) { + /* if OpenSSL hasn't be initialized externally, and the counter reaches zero + after it's decremented, release EVP memory Note: this code will only be reached if OpensSSL_add_all_algorithms() - is called by SQLCipher internally. */ - if(openssl_init_count == 0) { + is called by SQLCipher internally. This should prevent SQLCipher from + "cleaning up" openssl when it was initialized externally by the program */ EVP_cleanup(); } + sqlite3_mutex_free(openssl_rand_mutex); + openssl_rand_mutex = NULL; } - sqlite3_mutex_leave(sqlite3MutexAlloc(SQLITE_MUTEX_STATIC_MASTER)); + sqlite3_mutex_leave(sqlite3_mutex_alloc(SQLITE_MUTEX_STATIC_MASTER)); return SQLITE_OK; } @@ -102,7 +117,17 @@ static const char* sqlcipher_openssl_get_provider_name(void *ctx) { /* generate a defined number of random bytes */ static int sqlcipher_openssl_random (void *ctx, void *buffer, int length) { - return (RAND_bytes((unsigned char *)buffer, length) == 1) ? SQLITE_OK : SQLITE_ERROR; + int rc = 0; + /* concurrent calls to RAND_bytes can cause a crash under some openssl versions when a + naive application doesn't use CRYPTO_set_locking_callback and + CRYPTO_THREADID_set_callback to ensure openssl thread safety. + This is simple workaround to prevent this common crash + but a more proper solution is that applications setup platform-appropriate + thread saftey in openssl externally */ + sqlite3_mutex_enter(openssl_rand_mutex); + rc = RAND_bytes((unsigned char *)buffer, length); + sqlite3_mutex_leave(openssl_rand_mutex); + return (rc == 1) ? SQLITE_OK : SQLITE_ERROR; } static int sqlcipher_openssl_hmac(void *ctx, unsigned char *hmac_key, int key_sz, unsigned char *in, int in_sz, unsigned char *in2, int in2_sz, unsigned char *out) { From 8866a9f6cfae25ceb0b2978a043d2e27c31fe7a3 Mon Sep 17 00:00:00 2001 From: Stephen Lombardo Date: Tue, 9 Jul 2013 11:57:02 -0400 Subject: [PATCH 3/5] allow -DSQLCIPHER_OPENSSL_NO_MUTEX_RAND to disable openssl rand mutex --- src/crypto_openssl.c | 12 ++++++++++++ 1 file changed, 12 insertions(+) diff --git a/src/crypto_openssl.c b/src/crypto_openssl.c index 9a4719ea8c..6035b7cf09 100644 --- a/src/crypto_openssl.c +++ b/src/crypto_openssl.c @@ -48,9 +48,13 @@ static unsigned int openssl_init_count = 0; static sqlite3_mutex* openssl_rand_mutex = NULL; static int sqlcipher_openssl_add_random(void *ctx, void *buffer, int length) { +#ifndef SQLCIPHER_OPENSSL_NO_MUTEX_RAND sqlite3_mutex_enter(openssl_rand_mutex); +#endif RAND_add(buffer, length, 0); +#ifndef SQLCIPHER_OPENSSL_NO_MUTEX_RAND sqlite3_mutex_leave(openssl_rand_mutex); +#endif return SQLITE_OK; } @@ -78,10 +82,12 @@ static int sqlcipher_openssl_activate(void *ctx) { OpenSSL_add_all_algorithms(); } +#ifndef SQLCIPHER_OPENSSL_NO_MUTEX_RAND if(openssl_rand_mutex == NULL) { /* allocate a mutex to guard against concurrent calls to RAND_bytes() */ openssl_rand_mutex = sqlite3_mutex_alloc(SQLITE_MUTEX_FAST); } +#endif openssl_init_count++; sqlite3_mutex_leave(sqlite3_mutex_alloc(SQLITE_MUTEX_STATIC_MASTER)); @@ -104,8 +110,10 @@ static int sqlcipher_openssl_deactivate(void *ctx) { "cleaning up" openssl when it was initialized externally by the program */ EVP_cleanup(); } +#ifndef SQLCIPHER_OPENSSL_NO_MUTEX_RAND sqlite3_mutex_free(openssl_rand_mutex); openssl_rand_mutex = NULL; +#endif } sqlite3_mutex_leave(sqlite3_mutex_alloc(SQLITE_MUTEX_STATIC_MASTER)); return SQLITE_OK; @@ -124,9 +132,13 @@ static int sqlcipher_openssl_random (void *ctx, void *buffer, int length) { This is simple workaround to prevent this common crash but a more proper solution is that applications setup platform-appropriate thread saftey in openssl externally */ +#ifndef SQLCIPHER_OPENSSL_NO_MUTEX_RAND sqlite3_mutex_enter(openssl_rand_mutex); +#endif rc = RAND_bytes((unsigned char *)buffer, length); +#ifndef SQLCIPHER_OPENSSL_NO_MUTEX_RAND sqlite3_mutex_leave(openssl_rand_mutex); +#endif return (rc == 1) ? SQLITE_OK : SQLITE_ERROR; } From 15c09c04c7c17e9f627d8a22d63252dbbbc72572 Mon Sep 17 00:00:00 2001 From: Nick Parker Date: Tue, 9 Jul 2013 11:56:01 -0500 Subject: [PATCH 4/5] Update README --- README | 1 + 1 file changed, 1 insertion(+) diff --git a/README b/README index 1b0a46191f..0378dad485 100644 --- a/README +++ b/README @@ -18,6 +18,7 @@ an iPhone data vault and password manager (http://getstrip.com). - Good security practices (CBC mode, key derivation) - Zero-configuration and application level cryptography - Algorithms provided by the peer reviewed OpenSSL crypto library. +- Configurable crypto providers [Compiling] From 337eac9fd96331a17b59274b162e21cc62221fd6 Mon Sep 17 00:00:00 2001 From: Stephen Lombardo Date: Wed, 10 Jul 2013 10:45:44 -0400 Subject: [PATCH 5/5] bump cipher_version to 2.2.1 --- src/crypto.h | 2 +- test/crypto.test | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/src/crypto.h b/src/crypto.h index 39924f7789..a45b57ca69 100644 --- a/src/crypto.h +++ b/src/crypto.h @@ -44,7 +44,7 @@ #define FILE_HEADER_SZ 16 #ifndef CIPHER_VERSION -#define CIPHER_VERSION "2.2.0" +#define CIPHER_VERSION "2.2.1" #endif #ifndef CIPHER diff --git a/test/crypto.test b/test/crypto.test index 90439c6294..8a58fe0a6f 100644 --- a/test/crypto.test +++ b/test/crypto.test @@ -1418,7 +1418,7 @@ do_test verify-pragma-cipher-version { execsql { PRAGMA cipher_version; } -} {2.2.0} +} {2.2.1} db close file delete -force test.db