You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@httpd.apache.org by Ewald Dieterich <ew...@mailbox.org> on 2016/09/12 08:31:58 UTC
Re: Random AH01842 errors in mod_session_crypto
On 06/13/2016 09:38 AM, Ewald Dieterich wrote:
> I configured form authentication with mod_auth_form, mod_session_cookie
> and mod_session_crypto in Apache 2.4.20 on Debian unstable and get
> random AH01842 errors ("decrypt session failed, wrong passphrase"). The
> passphrase was not changed when this happens.
>
> It looks like the error occurs when the following conditions are met:
>
> * mpm_worker enabled (never experienced the error with mpm_prefork)
> * Same user doing multiple requests in parallel using the same session
> (don't see the error when the user is doing only sequential requests)
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.
* APR documentation about pool thread-safety: "Note that most operations
on pools are not thread-safe: a single pool should only be accessed by a
single thread at any given time."
I changed mod_session_crypto to use the request pool instead and it
seems that this fixed my problem.
I think this also fixes a memory consumption problem: Keys are only
created, but never explicitly destroyed (or reused). So for every
request memory is allocated from the global pool, but this memory is
never freed during the lifetime of mod_session_crypto. Using the request
pool fixes this problem because it is destroyed when the request is done.
See the attached patch session-crypto.patch that I created for 2.4.20.
Re: Random AH01842 errors in mod_session_crypto
Posted by Paul Spangler <pa...@ni.com>.
On 10/4/2016 10:29 AM, Graham Leggett wrote:
> On 4 Oct 2016, at 15:47, Paul Spangler <pa...@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.
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&D
National Instruments
Re: Random AH01842 errors in mod_session_crypto
Posted by William A Rowe Jr <wr...@rowe-clan.net>.
On Fri, Dec 2, 2016 at 4:28 PM, Yann Ylavic <yl...@gmail.com> wrote:
> On Fri, Dec 2, 2016 at 10:06 PM, Yann Ylavic <yl...@gmail.com> wrote:
> > On Wed, Oct 5, 2016 at 12:23 PM, Yann Ylavic <yl...@gmail.com>
> wrote:
> >>
> >> Patch attached, WDYT?
> >
> > Ping, probably is worth considering for 1.6 (even 1.5) ?
>
> Committed to APR trunk (r1772414), 1.6.x (r1772415) and 1.5.x (r1772416).
>
Excellent! Docs edits also welcome for clarification.
Re: Random AH01842 errors in mod_session_crypto
Posted by William A Rowe Jr <wr...@rowe-clan.net>.
On Fri, Dec 2, 2016 at 4:28 PM, Yann Ylavic <yl...@gmail.com> wrote:
> On Fri, Dec 2, 2016 at 10:06 PM, Yann Ylavic <yl...@gmail.com> wrote:
> > On Wed, Oct 5, 2016 at 12:23 PM, Yann Ylavic <yl...@gmail.com>
> wrote:
> >>
> >> Patch attached, WDYT?
> >
> > Ping, probably is worth considering for 1.6 (even 1.5) ?
>
> Committed to APR trunk (r1772414), 1.6.x (r1772415) and 1.5.x (r1772416).
>
Excellent! Docs edits also welcome for clarification.
Re: Random AH01842 errors in mod_session_crypto
Posted by Yann Ylavic <yl...@gmail.com>.
On Fri, Dec 2, 2016 at 10:06 PM, Yann Ylavic <yl...@gmail.com> wrote:
> On Wed, Oct 5, 2016 at 12:23 PM, Yann Ylavic <yl...@gmail.com> wrote:
>>
>> Patch attached, WDYT?
>
> Ping, probably is worth considering for 1.6 (even 1.5) ?
Committed to APR trunk (r1772414), 1.6.x (r1772415) and 1.5.x (r1772416).
Regards,
Yann.
Re: Random AH01842 errors in mod_session_crypto
Posted by William A Rowe Jr <wr...@rowe-clan.net>.
On Fri, Dec 2, 2016 at 3:06 PM, Yann Ylavic <yl...@gmail.com> wrote:
> On Wed, Oct 5, 2016 at 12:23 PM, Yann Ylavic <yl...@gmail.com> wrote:
> >
> > Patch attached, WDYT?
>
> Ping, probably is worth considering for 1.6 (even 1.5) ?
>
Provided you don't *break* the API contract with 1.x.x, corrections
can be applied to 1.(current). This looks like it falls in that category.
Things get wonky when you have duplicates-permitted and decide
that's bad, so change the behavior to duplicates-disallowed. That
would actually violate any 1.x release update.
But as long as we are conforming the behavior to the published
description, bug fixes are welcome.
Re: Random AH01842 errors in mod_session_crypto
Posted by William A Rowe Jr <wr...@rowe-clan.net>.
On Fri, Dec 2, 2016 at 3:06 PM, Yann Ylavic <yl...@gmail.com> wrote:
> On Wed, Oct 5, 2016 at 12:23 PM, Yann Ylavic <yl...@gmail.com> wrote:
> >
> > Patch attached, WDYT?
>
> Ping, probably is worth considering for 1.6 (even 1.5) ?
>
Provided you don't *break* the API contract with 1.x.x, corrections
can be applied to 1.(current). This looks like it falls in that category.
Things get wonky when you have duplicates-permitted and decide
that's bad, so change the behavior to duplicates-disallowed. That
would actually violate any 1.x release update.
But as long as we are conforming the behavior to the published
description, bug fixes are welcome.
Re: Random AH01842 errors in mod_session_crypto
Posted by Yann Ylavic <yl...@gmail.com>.
On Fri, Dec 2, 2016 at 10:06 PM, Yann Ylavic <yl...@gmail.com> wrote:
> On Wed, Oct 5, 2016 at 12:23 PM, Yann Ylavic <yl...@gmail.com> wrote:
>>
>> Patch attached, WDYT?
>
> Ping, probably is worth considering for 1.6 (even 1.5) ?
Committed to APR trunk (r1772414), 1.6.x (r1772415) and 1.5.x (r1772416).
Regards,
Yann.
Re: Random AH01842 errors in mod_session_crypto
Posted by Yann Ylavic <yl...@gmail.com>.
On Wed, Oct 5, 2016 at 12:23 PM, Yann Ylavic <yl...@gmail.com> wrote:
>
> Patch attached, WDYT?
Ping, probably is worth considering for 1.6 (even 1.5) ?
>
> Regards,
> Yann.
Re: Random AH01842 errors in mod_session_crypto
Posted by Yann Ylavic <yl...@gmail.com>.
On Wed, Oct 5, 2016 at 12:23 PM, Yann Ylavic <yl...@gmail.com> wrote:
>
> Patch attached, WDYT?
Ping, probably is worth considering for 1.6 (even 1.5) ?
>
> Regards,
> Yann.
Re: Random AH01842 errors in mod_session_crypto
Posted by Yann Ylavic <yl...@gmail.com>.
[Adding dev@apr, with a little abstract]
On Mon, Sep 12, 2016 at 10:31 AM, Ewald Dieterich <ew...@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 <yl...@gmail.com> wrote:
> On Tue, Oct 4, 2016 at 5:29 PM, Graham Leggett <mi...@sharp.fm>
> wrote:
>> On 4 Oct 2016, at 15:47, Paul Spangler <pa...@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?
Regards,
Yann.
Re: Random AH01842 errors in mod_session_crypto
Posted by Yann Ylavic <yl...@gmail.com>.
[Adding dev@apr, with a little abstract]
On Mon, Sep 12, 2016 at 10:31 AM, Ewald Dieterich <ew...@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 <yl...@gmail.com> wrote:
> On Tue, Oct 4, 2016 at 5:29 PM, Graham Leggett <mi...@sharp.fm>
> wrote:
>> On 4 Oct 2016, at 15:47, Paul Spangler <pa...@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?
Regards,
Yann.
Re: Random AH01842 errors in mod_session_crypto
Posted by Yann Ylavic <yl...@gmail.com>.
On Tue, Oct 4, 2016 at 5:29 PM, Graham Leggett <mi...@sharp.fm> wrote:
> On 4 Oct 2016, at 15:47, Paul Spangler <pa...@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?
>
> 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
Posted by Graham Leggett <mi...@sharp.fm>.
On 4 Oct 2016, at 15:47, Paul Spangler <pa...@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.
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
Posted by Paul Spangler <pa...@ni.com>.
On 9/12/2016 2:41 PM, Yann Ylavic wrote:
> On Mon, Sep 12, 2016 at 10:31 AM, Ewald Dieterich <ew...@mailbox.org> 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&revision=1752008
Regards,
Paul Spangler
LabVIEW R&D
National Instruments
Re: Random AH01842 errors in mod_session_crypto
Posted by Yann Ylavic <yl...@gmail.com>.
On Mon, Sep 12, 2016 at 10:31 AM, Ewald Dieterich <ew...@mailbox.org> wrote:
> On 06/13/2016 09:38 AM, Ewald Dieterich wrote:
>
> Looks like the problem is this:
Thanks for invertigating!
>
> * 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.
True, this is because we give apr_crypto_passphrase() a NULL key.
> * Multiple threads might access the global pool at the same time.
Correct, hence the data corruption (possibly crash).
>
> I changed mod_session_crypto to use the request pool instead and it seems
> that this fixed my problem.
>
> I think this also fixes a memory consumption problem: Keys are only created,
> but never explicitly destroyed (or reused). So for every request memory is
> allocated from the global pool, but this memory is never freed during the
> lifetime of mod_session_crypto. Using the request pool fixes this problem
> because it is destroyed when the request is done.
>
> See the attached patch session-crypto.patch that I created for 2.4.20.
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.
Regards,
Yann.