[Adding dev@apr, with a little abstract]
On Mon, Sep 12, 2016 at 10:31 AM, Ewald Dieterich <[email protected]> wrote:
>
> Looks like the problem is this:
>
> * In session_crypto_init() a crypto context is created from a global pool
> (server->pconf).
> * In encrypt_string() and decrypt_string() a key is created from the context
> via apr_crypto_passphrase() using the global pool for allocating memory for
> the key.
> * Multiple threads might access the global pool at the same time.
Unless given an already allocated key, apr_crypto_passphrase() and
apr_crypto_key() will create/allocate the key from the pool of an
apr_crypto_t (the *const* context), not the given pool.
So we tried to allocate the key before the call to apr_crypto_passphrase()...
On Tue, Oct 4, 2016 at 6:21 PM, Yann Ylavic <[email protected]> wrote:
> On Tue, Oct 4, 2016 at 5:29 PM, Graham Leggett <[email protected]>
> wrote:
>> On 4 Oct 2016, at 15:47, Paul Spangler <[email protected]>
>> wrote:
>>
>>> From my understanding, apr_crypto_key_t is an opaque struct
>>> defined separately by each crypto provider, so mod_session_crypto
>>> will not be able to do the sizeof.
>>
>> That's a sizeof a pointer to apr_crypto_key_t, not the sizeof
>> apr_crypto_key_t itself.
>
> I think Paul is correct, apr_crypto_passphrase() requires its given
> *(apr_crypto_key_t**)key to be not NULL, otherwise it will allocate
> one from its (providers's) array, which is not thread safe.
>
> How are we supposed to have a *key not NULL given apr_crypto_key_t is
> opaque?
... no way.
>>
>> Keys are read at server start and reused. Trying to regenerate the
>> key on every request has performance implications.
Looks like the heavy operation is creating the context (apr_crypto_t),
not the key, but anyway the keys need to be generated at run time,
taking the salt into account.
So I don't see the point of caching (and allocating) the keys in the
context while we could use the given runtime pool.
If the user wanted some cache s/he could still handle it based on
her/his own pool, but:
> It seems that apr_crypto_passphrase()'s **key is updated for each
> call, though...
so we can't really cache it anyway.
Hence I propose to remove the keys' cache in apr_crypto, and let the
things work with the given pool(s)...
Patch attached, WDYT?
Regards,
Yann.
Index: crypto/apr_crypto_commoncrypto.c
===================================================================
--- crypto/apr_crypto_commoncrypto.c (revision 1763363)
+++ crypto/apr_crypto_commoncrypto.c (working copy)
@@ -41,7 +41,6 @@ struct apr_crypto_t
apr_pool_t *pool;
const apr_crypto_driver_t *provider;
apu_err_t *result;
- apr_array_header_t *keys;
apr_hash_t *types;
apr_hash_t *modes;
apr_random_t *rng;
@@ -206,11 +205,6 @@ static apr_status_t crypto_make(apr_crypto_t **ff,
return APR_ENOMEM;
}
- f->keys = apr_array_make(pool, 10, sizeof(apr_crypto_key_t));
- if (!f->keys) {
- return APR_ENOMEM;
- }
-
f->types = apr_hash_make(pool);
if (!f->types) {
return APR_ENOMEM;
@@ -388,7 +382,7 @@ static apr_status_t crypto_key(apr_crypto_key_t **
apr_crypto_key_t *key = *k;
if (!key) {
- *k = key = apr_array_push(f->keys);
+ *k = key = apr_pcalloc(p, sizeof *key);
}
if (!key) {
return APR_ENOMEM;
@@ -480,11 +474,11 @@ static apr_status_t crypto_passphrase(apr_crypto_k
apr_crypto_key_t *key = *k;
if (!key) {
- *k = key = apr_array_push(f->keys);
+ *k = key = apr_pcalloc(p, sizeof *key);
+ if (!key) {
+ return APR_ENOMEM;
+ }
}
- if (!key) {
- return APR_ENOMEM;
- }
key->f = f;
key->provider = f->provider;
Index: crypto/apr_crypto_nss.c
===================================================================
--- crypto/apr_crypto_nss.c (revision 1763363)
+++ crypto/apr_crypto_nss.c (working copy)
@@ -50,7 +50,6 @@ struct apr_crypto_t {
apr_pool_t *pool;
const apr_crypto_driver_t *provider;
apu_err_t *result;
- apr_array_header_t *keys;
apr_crypto_config_t *config;
apr_hash_t *types;
apr_hash_t *modes;
@@ -266,6 +265,15 @@ static apr_status_t crypto_block_cleanup_helper(vo
return crypto_block_cleanup(block);
}
+static apr_status_t crypto_key_cleanup(void *data)
+{
+ apr_crypto_key_t *key = data;
+ if (key->symKey) {
+ PK11_FreeSymKey(key->symKey);
+ key->symKey = NULL;
+ }
+ return APR_SUCCESS;
+}
/**
* @brief Clean encryption / decryption context.
* @note After cleanup, a context is free to be reused if necessary.
@@ -274,15 +282,6 @@ static apr_status_t crypto_block_cleanup_helper(vo
*/
static apr_status_t crypto_cleanup(apr_crypto_t *f)
{
- apr_crypto_key_t *key;
- if (f->keys) {
- while ((key = apr_array_pop(f->keys))) {
- if (key->symKey) {
- PK11_FreeSymKey(key->symKey);
- key->symKey = NULL;
- }
- }
- }
return APR_SUCCESS;
}
@@ -326,7 +325,6 @@ static apr_status_t crypto_make(apr_crypto_t **ff,
if (!f->result) {
return APR_ENOMEM;
}
- f->keys = apr_array_make(pool, 10, sizeof(apr_crypto_key_t));
f->types = apr_hash_make(pool);
if (!f->types) {
@@ -491,11 +489,13 @@ static apr_status_t crypto_key(apr_crypto_key_t **
key = *k;
if (!key) {
- *k = key = apr_array_push(f->keys);
+ *k = key = apr_pcalloc(p, sizeof *key);
+ if (!key) {
+ return APR_ENOMEM;
+ }
+ apr_pool_cleanup_register(p, key, crypto_key_cleanup,
+ apr_pool_cleanup_null);
}
- if (!key) {
- return APR_ENOMEM;
- }
key->f = f;
key->provider = f->provider;
@@ -683,11 +683,13 @@ static apr_status_t crypto_passphrase(apr_crypto_k
apr_crypto_key_t *key = *k;
if (!key) {
- *k = key = apr_array_push(f->keys);
+ *k = key = apr_pcalloc(p, sizeof *key);
+ if (!key) {
+ return APR_ENOMEM;
+ }
+ apr_pool_cleanup_register(p, key, crypto_key_cleanup,
+ apr_pool_cleanup_null);
}
- if (!key) {
- return APR_ENOMEM;
- }
key->f = f;
key->provider = f->provider;
Index: crypto/apr_crypto_openssl.c
===================================================================
--- crypto/apr_crypto_openssl.c (revision 1763363)
+++ crypto/apr_crypto_openssl.c (working copy)
@@ -40,7 +40,6 @@ struct apr_crypto_t {
apr_pool_t *pool;
const apr_crypto_driver_t *provider;
apu_err_t *result;
- apr_array_header_t *keys;
apr_crypto_config_t *config;
apr_hash_t *types;
apr_hash_t *modes;
@@ -268,11 +267,6 @@ static apr_status_t crypto_make(apr_crypto_t **ff,
return APR_ENOMEM;
}
- f->keys = apr_array_make(pool, 10, sizeof(apr_crypto_key_t));
- if (!f->keys) {
- return APR_ENOMEM;
- }
-
f->types = apr_hash_make(pool);
if (!f->types) {
return APR_ENOMEM;
@@ -432,11 +426,11 @@ static apr_status_t crypto_key(apr_crypto_key_t **
apr_status_t rv;
if (!key) {
- *k = key = apr_array_push(f->keys);
+ *k = key = apr_pcalloc(p, sizeof *key);
+ if (!key) {
+ return APR_ENOMEM;
+ }
}
- if (!key) {
- return APR_ENOMEM;
- }
key->f = f;
key->provider = f->provider;
@@ -533,11 +527,11 @@ static apr_status_t crypto_passphrase(apr_crypto_k
apr_status_t rv;
if (!key) {
- *k = key = apr_array_push(f->keys);
+ *k = key = apr_pcalloc(p, sizeof *key);
+ if (!key) {
+ return APR_ENOMEM;
+ }
}
- if (!key) {
- return APR_ENOMEM;
- }
key->f = f;
key->provider = f->provider;
