Re: Random AH01842 errors in mod_session_crypto

[Yann Ylavic]

On Tue, Oct 4, 2016 at 5:29 PM, Graham Leggett  wrote:
> On 4 Oct 2016, at 15:47, Paul Spangler  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?


>
> Keys are read at server start and reused. Trying to regenerate the key on 
> every request has performance implications.

This is not what mod_session_crypto seems to be doing, passphrases are
read at load time but the keys are not created there.

Is mod_session_crypto supposed to make a fake call to
apr_crypto_passphrase() in post_config and reuse that key (with a
different salt) for runtime calls?
It seems that apr_crypto_passphrase()'s **key is updated for each
call, though...


Regards,
Yann.

Re: Random AH01842 errors in mod_session_crypto

[Paul Spangler]

On 10/4/2016 10:29 AM, Graham Leggett wrote:

On 4 Oct 2016, at 15:47, Paul Spangler  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.


It's possible I'm looking a different version of the code, but when I 
try that patch:


apr_crypto_key_t *key = NULL;
...
key = apr_pcalloc(r->pool, sizeof *key);

mod_session_crypto.c: In function 'decrypt_string':
mod_session_crypto.c:249:11: error: dereferencing pointer to incomplete type



Keys are read at server start and reused. Trying to regenerate the key on every 
request has performance implications.



mod_session_crypto's passphrases can also be read from .htaccess, which 
means at least some keys may be unknown at server start. I agree that 
regenerating the keys on every request is not ideal. I'm only 
questioning the feasibility of reusing keys that may come and go from 
request to request.


Regards,
Paul Spangler
LabVIEW R
National Instruments

Re: Random AH01842 errors in mod_session_crypto

[Graham Leggett]

On 4 Oct 2016, at 15:47, Paul Spangler  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.

Keys are read at server start and reused. Trying to regenerate the key on every 
request has performance implications.

Regards,
Graham
--

Re: Random AH01842 errors in mod_session_crypto

[Paul Spangler]

On 9/12/2016 2:41 PM, Yann Ylavic wrote:

On Mon, Sep 12, 2016 at 10:31 AM, Ewald Dieterich  wrote:

On 06/13/2016 09:38 AM, Ewald Dieterich wrote:

Looks like the problem is this:


Thanks for invertigating!


Yes, I recently found a case where this error comes up as well and this 
investigation helped a lot. I'm also interested in resuming the 
discussion about potentially fixing it.




Thanks for the patch, possibly a simpler way to fix it would be:

Index: modules/session/mod_session_crypto.c
===
--- modules/session/mod_session_crypto.c(revision 1760149)
+++ modules/session/mod_session_crypto.c(working copy)
@@ -246,6 +246,8 @@ static apr_status_t decrypt_string(request_rec * r
 return res;
 }

+key = apr_pcalloc(r->pool, sizeof *key);
+
 /* try each passphrase in turn */
 for (; i < dconf->passphrases->nelts; i++) {
 const char *passphrase = APR_ARRAY_IDX(dconf->passphrases, i, char *);
_

so that crypto_passphrase() will use the given key instead of
allocating a new one.


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.


My initial reaction is thinking that the crypto providers should be 
allocating the keys using the pool passed to apr_crypto_passphrase 
instead of the one passed to apr_crypto_make. But it looks like a 
relatively recent change to APR trunk [1] makes it more explicit in the 
documentation that the allocated keys last until the context is cleaned up.


That implies we either need a context per request, like Ewald's patch, 
or use a lock (to avoid multithreded pool access) and reuse the keys (to 
avoid increased memory consumption). But the keys may change per 
directory, so I don't know if that's feasible.


[1] https://svn.apache.org/viewvc?view=revision=1752008

Regards,
Paul Spangler
LabVIEW R
National Instruments