apr-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Yann Ylavic <ylavic....@gmail.com>
Subject Re: Random AH01842 errors in mod_session_crypto
Date Wed, 05 Oct 2016 10:23:54 GMT
[Adding dev@apr, with a little abstract]

On Mon, Sep 12, 2016 at 10:31 AM, Ewald Dieterich <ewald@mailbox.org> 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 <ylavic.dev@gmail.com> wrote:
> On Tue, Oct 4, 2016 at 5:29 PM, Graham Leggett <minfrin@sharp.fm>
> wrote:
>> On 4 Oct 2016, at 15:47, Paul Spangler <paul.spangler@ni.com>
>> 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?


View raw message