You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@httpd.apache.org by Rainer Jung <ra...@kippdata.de> on 2014/05/17 07:00:30 UTC
Memory leak in mod_ssl ssl_callback_TmpDH
While doing some customization of mod_ssl I checked for memory leaks on
Solaris using libumem and found 5 allocations that happen for each
handshake and do not seem to get freed.
Versions: httpd 2.4 head plus OpenSSL 1.0.1g
> ::findleaks
...
000b9688 85 002779c8 mod_ssl.so`default_malloc_ex+0x20
000b8788 85 00185d10 mod_ssl.so`default_malloc_ex+0x20
000bea08 84 00178690 mod_ssl.so`default_malloc_ex+0x20
000b8288 84 00131ab8 mod_ssl.so`default_malloc_ex+0x20
000b8788 84 00185d88 mod_ssl.so`default_malloc_ex+0x20
...
The number 84/85 is the number of connections handled.
The five allocation stacks always point to ssl_callback_TmpDH, at
offsets 0x12c, 0xec and 0x140:
> 002779c8::bufctl_audit
ADDR BUFADDR TIMESTAMP THREAD
CACHE LASTLOG CONTENTS
2779c8 275c78 277d6c5029c999 1
b9688 a5b58 0
libumem.so.1`umem_cache_alloc+0x210
libumem.so.1`umem_alloc+0x60
libumem.so.1`malloc+0x28
mod_ssl.so`default_malloc_ex+0x20
mod_ssl.so`CRYPTO_malloc+0x88
mod_ssl.so`DH_new_method+0x24
mod_ssl.so`ssl_callback_TmpDH+0x12c
mod_ssl.so`ssl3_send_server_key_exchange+0xad8
> 00185d88::bufctl_audit
ADDR BUFADDR TIMESTAMP THREAD
CACHE LASTLOG CONTENTS
185d88 180ed8 277d6c502a3e23 1
b8788 a5e14 0
libumem.so.1`umem_cache_alloc+0x13c
libumem.so.1`umem_alloc+0x60
libumem.so.1`malloc+0x28
mod_ssl.so`default_malloc_ex+0x20
mod_ssl.so`CRYPTO_malloc+0x88
mod_ssl.so`BN_new+0x24
mod_ssl.so`BN_dec2bn+0x220
mod_ssl.so`ssl_callback_TmpDH+0xec
mod_ssl.so`ssl3_send_server_key_exchange+0xad8
> 00131ab8::bufctl_audit
ADDR BUFADDR TIMESTAMP THREAD
CACHE LASTLOG CONTENTS
131ab8 12b620 277d6c502a483a 1
b8288 a5e78 0
libumem.so.1`umem_cache_alloc+0x210
libumem.so.1`umem_alloc+0x60
libumem.so.1`malloc+0x28
mod_ssl.so`default_malloc_ex+0x20
mod_ssl.so`CRYPTO_malloc+0x88
mod_ssl.so`bn_expand_internal+0x44
mod_ssl.so`bn_expand2+0x20
mod_ssl.so`BN_dec2bn+0x1bc
mod_ssl.so`ssl_callback_TmpDH+0xec
mod_ssl.so`ssl3_send_server_key_exchange+0xad8
> 00185d10::bufctl_audit
ADDR BUFADDR TIMESTAMP THREAD
CACHE LASTLOG CONTENTS
185d10 180f08 277d6c502a0e49 1
b8788 a5d4c 0
libumem.so.1`umem_cache_alloc+0x13c
libumem.so.1`umem_alloc+0x60
libumem.so.1`malloc+0x28
mod_ssl.so`default_malloc_ex+0x20
mod_ssl.so`CRYPTO_malloc+0x88
mod_ssl.so`BN_new+0x24
mod_ssl.so`BN_bin2bn+0x11c
mod_ssl.so`ssl_callback_TmpDH+0x140
mod_ssl.so`ssl3_send_server_key_exchange+0xad8
> 00178690::bufctl_audit
ADDR BUFADDR TIMESTAMP THREAD
CACHE LASTLOG CONTENTS
178690 17f3c0 277d6c502a23c5 1
bea08 a5db0 0
libumem.so.1`umem_cache_alloc+0x13c
libumem.so.1`umem_alloc+0x60
libumem.so.1`malloc+0x28
mod_ssl.so`default_malloc_ex+0x20
mod_ssl.so`CRYPTO_malloc+0x88
mod_ssl.so`bn_expand_internal+0x44
mod_ssl.so`bn_expand2+0x20
mod_ssl.so`BN_bin2bn+0xf0
mod_ssl.so`ssl_callback_TmpDH+0x140
mod_ssl.so`ssl3_send_server_key_exchange+0xad8
ssl_callback_TmpDH is in modules/ssl/ssl_engine_kernel.c.
I think this is due to the changes in modules/ssl/ssl_engine_init.c
applied by the backported r1542327.
Function ssl_callback_TmpDH calls functions get_dh4096() or get_dh3072()
or get_dh2048() or get_dh1024(), which are defined by macro make_get_dh
and include code:
DH *dh;
if (!(dh = DH_new())) {
^^^^^^
return NULL;
}
dh->p = get_##rfc##_prime_##size(NULL);
BN_dec2bn(&dh->g, #gen);
^^^^^^^^^
if (!dh->p || !dh->g) {
DH_free(dh);
return NULL;
}
return dh;
So the first three leaks come from here. For the leak with BN_bin2bn it
is hidden in get_##rfc##_prime_##size, e.g. get_rfc3526_prime_4096()
contains the line
return BN_bin2bn(RFC3526_PRIME_4096,sizeof(RFC3526_PRIME_4096),bn);
It seems to me that a single call to DH_free(dh) would suffice to free
all 5 allocations.
So the question is, where can those allocations be best freed? Is it the
application (mod_ssl) responsibility, or should OpenSSL free it after it
no longer uses the callback set via SSL_CTX_set_tmp_dh_callback? If the
returned DH is only used for a single connection, we could try to free
during connection shutdown.
I would prefer if someone with a better knowledge of mod_ssl would look
further into it. We have to pass around the DH to be able to free it
later and find the right place to free it.
Regards,
Rainer
RE: Memory leak in mod_ssl ssl_callback_TmpDH
Posted by Plüm,
Rüdiger,
Vodafone Group <ru...@vodafone.com>.
> -----Original Message-----
> From: Yann Ylavic [mailto:ylavic.dev@gmail.com]
> Sent: Mittwoch, 28. Mai 2014 12:38
> To: httpd
> Subject: Re: Memory leak in mod_ssl ssl_callback_TmpDH
>
> On Wed, May 28, 2014 at 9:18 AM, Plüm, Rüdiger, Vodafone Group
> <ru...@vodafone.com> wrote:
> >
> >
> >> -----Original Message-----
> >> From: Yann Ylavic [mailto:ylavic.dev@gmail.com]
> >> Sent: Mittwoch, 28. Mai 2014 01:25
> >> To: httpd
> >> Subject: Re: Memory leak in mod_ssl ssl_callback_TmpDH
> >>
> >> On Tue, May 27, 2014 at 10:33 PM, Ruediger Pluem <rp...@apache.org>
> >> wrote:
> >> >
> >> > #define make_get_dh(rfc,size,gen) \
> >> > static DH *get_dh##size(void) \
> >> > @@ -1339,7 +1344,7 @@
> >> > DH_free(dh_tmp); \
> >> > return NULL; \
> >> > } \
> >> > - dh = dh_tmp; \
> >> > + apr_atomic_xchgptr((volatile void **)&dh, dh_tmp); \
> >> > return dh; \
> >> > }
> >> >
> >>
> >> I think you still need to handle the race (leak) with something like :
> >
> > To be honest I am not worried about the leak as it is very limited and
> does not harm.
>
> Well, "fixing" it does not harm too much either (and quiets valgrind a
> bit, this has potential #cores leaks).
> We probably need an #include "apr_atomic.h" at the top too...
>
Well if we see it this strict I would prefer Joe's original proposal and do that stuff
during initialisation, even if we don't need that parameters at all. IMHO generating them is cheap during
init phase such we only hand out the existing stuff in the callback later on.
But it requires some moving of code.
Regards
Rüdiger
Re: Memory leak in mod_ssl ssl_callback_TmpDH
Posted by Yann Ylavic <yl...@gmail.com>.
On Wed, May 28, 2014 at 9:18 AM, Plüm, Rüdiger, Vodafone Group
<ru...@vodafone.com> wrote:
>
>
>> -----Original Message-----
>> From: Yann Ylavic [mailto:ylavic.dev@gmail.com]
>> Sent: Mittwoch, 28. Mai 2014 01:25
>> To: httpd
>> Subject: Re: Memory leak in mod_ssl ssl_callback_TmpDH
>>
>> On Tue, May 27, 2014 at 10:33 PM, Ruediger Pluem <rp...@apache.org>
>> wrote:
>> >
>> > #define make_get_dh(rfc,size,gen) \
>> > static DH *get_dh##size(void) \
>> > @@ -1339,7 +1344,7 @@
>> > DH_free(dh_tmp); \
>> > return NULL; \
>> > } \
>> > - dh = dh_tmp; \
>> > + apr_atomic_xchgptr((volatile void **)&dh, dh_tmp); \
>> > return dh; \
>> > }
>> >
>>
>> I think you still need to handle the race (leak) with something like :
>
> To be honest I am not worried about the leak as it is very limited and does not harm.
Well, "fixing" it does not harm too much either (and quiets valgrind a
bit, this has potential #cores leaks).
We probably need an #include "apr_atomic.h" at the top too...
Regards.
RE: Memory leak in mod_ssl ssl_callback_TmpDH
Posted by Plüm,
Rüdiger,
Vodafone Group <ru...@vodafone.com>.
> -----Original Message-----
> From: Yann Ylavic [mailto:ylavic.dev@gmail.com]
> Sent: Mittwoch, 28. Mai 2014 01:25
> To: httpd
> Subject: Re: Memory leak in mod_ssl ssl_callback_TmpDH
>
> On Tue, May 27, 2014 at 10:33 PM, Ruediger Pluem <rp...@apache.org>
> wrote:
> >
> > #define make_get_dh(rfc,size,gen) \
> > static DH *get_dh##size(void) \
> > @@ -1339,7 +1344,7 @@
> > DH_free(dh_tmp); \
> > return NULL; \
> > } \
> > - dh = dh_tmp; \
> > + apr_atomic_xchgptr((volatile void **)&dh, dh_tmp); \
> > return dh; \
> > }
> >
>
> I think you still need to handle the race (leak) with something like :
To be honest I am not worried about the leak as it is very limited and does not harm.
Regards
Rüdiger
Re: Memory leak in mod_ssl ssl_callback_TmpDH
Posted by Yann Ylavic <yl...@gmail.com>.
On Tue, May 27, 2014 at 10:33 PM, Ruediger Pluem <rp...@apache.org> wrote:
>
> #define make_get_dh(rfc,size,gen) \
> static DH *get_dh##size(void) \
> @@ -1339,7 +1344,7 @@
> DH_free(dh_tmp); \
> return NULL; \
> } \
> - dh = dh_tmp; \
> + apr_atomic_xchgptr((volatile void **)&dh, dh_tmp); \
> return dh; \
> }
>
I think you still need to handle the race (leak) with something like :
Index: modules/ssl/ssl_engine_kernel.c
===================================================================
--- modules/ssl/ssl_engine_kernel.c (revision 1597892)
+++ modules/ssl/ssl_engine_kernel.c (working copy)
@@ -1324,7 +1325,7 @@ const authz_provider ssl_authz_provider_verify_cli
#define make_get_dh(rfc,size,gen) \
static DH *get_dh##size(void) \
{ \
- static DH *dh = NULL; \
+ static void *volatile dh = NULL; \
DH *dh_tmp; \
\
if (dh) { \
@@ -1339,7 +1340,9 @@ static DH *get_dh##size(void) \
DH_free(dh_tmp); \
return NULL; \
} \
- dh = dh_tmp; \
+ if (apr_atomic_casptr((volatile void **)&dh, dh_tmp, NULL)) { \
+ DH_free(dh_tmp); \
+ } \
return dh; \
}
Regards,
Yann.
PS: there is a problem with the declaration of apr_atomic_casptr() and
apr_atomic_xchgptr() in APR, "volatile void **mem" instead of "void
*volatile *mem". Hence the cast is mandatory here.
Re: yet another mod_ssl temp DH handling tweak
Posted by Joe Orton <jo...@redhat.com>.
On Sat, Jun 21, 2014 at 09:24:05AM +0200, Kaspar Brand wrote:
> On 19.06.2014 23:17, Joe Orton wrote:
> > I was reminded that there was a request to use the larger key sizes as
> > well.
>
> Using ephemeral DH keys with sizes > 4096 bits in TLS seems way overkill
> for the next decade or so (3072 bits are already considered to have a
> 128-bit symmetric-key strength), but if it makes people happy to use
> unreasonably large keys, then so be it... the docs for
> SSLCertificateFile should also be updated in this case.
Thanks to you & Rüdiger for review! r1605827 & r1605829
> > +/* Storage and initialization for DH parameters. */
> > +static struct dhparam {
> > + BIGNUM *(*const prime)(BIGNUM *); /* function to generate... */
> > + DH *dh; /* ...this, used for keys.... */
> > + const unsigned int min; /* ...of length >= this. */
> > +} dhparams[] = {
> > + { get_rfc3526_prime_8192, NULL, 6145 },
> > + { get_rfc3526_prime_6144, NULL, 4097 },
> > + { get_rfc3526_prime_4096, NULL, 3073 },
> > + { get_rfc3526_prime_3072, NULL, 2049 },
> > + { get_rfc3526_prime_2048, NULL, 1025 },
> > + { get_rfc2409_prime_1024, NULL, 0 }
> > +};
>
> Perhaps the "min" values could increased somewhat -
> 7168/5120/3584/2560/1536 (i.e. "half way" between two steps)?
I've a mild preference for keeping to the 1K multiples, since half way
is kind of arbitrary... if you or anybody else feels strongly about this
I'm happy to adjust.
Regards, Joe
Re: yet another mod_ssl temp DH handling tweak
Posted by Ruediger Pluem <rp...@apache.org>.
Joe Orton wrote:
> On Thu, Jun 19, 2014 at 04:29:17PM +0100, Joe Orton wrote:
>> One more tweak here, proposed by Hubert from our QA team who has been
>> reviewing all this stuff. Hubert argued we should be erring on the side
>> of stronger not weaker here, particularly because of the possibility of
>> 2048-bit keys being identified as 2047 in rare cases.
>
> I was reminded that there was a request to use the larger key sizes as
> well. So with apologies, I over-engineered this even more:
>
> Any comments/objections/hatemail?
+1
Regards
Rüdiger
Re: yet another mod_ssl temp DH handling tweak
Posted by Kaspar Brand <ht...@velox.ch>.
On 19.06.2014 23:17, Joe Orton wrote:
> I was reminded that there was a request to use the larger key sizes as
> well.
Using ephemeral DH keys with sizes > 4096 bits in TLS seems way overkill
for the next decade or so (3072 bits are already considered to have a
128-bit symmetric-key strength), but if it makes people happy to use
unreasonably large keys, then so be it... the docs for
SSLCertificateFile should also be updated in this case.
> +/* Storage and initialization for DH parameters. */
> +static struct dhparam {
> + BIGNUM *(*const prime)(BIGNUM *); /* function to generate... */
> + DH *dh; /* ...this, used for keys.... */
> + const unsigned int min; /* ...of length >= this. */
> +} dhparams[] = {
> + { get_rfc3526_prime_8192, NULL, 6145 },
> + { get_rfc3526_prime_6144, NULL, 4097 },
> + { get_rfc3526_prime_4096, NULL, 3073 },
> + { get_rfc3526_prime_3072, NULL, 2049 },
> + { get_rfc3526_prime_2048, NULL, 1025 },
> + { get_rfc2409_prime_1024, NULL, 0 }
> +};
Perhaps the "min" values could increased somewhat -
7168/5120/3584/2560/1536 (i.e. "half way" between two steps)?
Kaspar
Re: yet another mod_ssl temp DH handling tweak
Posted by Joe Orton <jo...@redhat.com>.
On Thu, Jun 19, 2014 at 04:29:17PM +0100, Joe Orton wrote:
> One more tweak here, proposed by Hubert from our QA team who has been
> reviewing all this stuff. Hubert argued we should be erring on the side
> of stronger not weaker here, particularly because of the possibility of
> 2048-bit keys being identified as 2047 in rare cases.
I was reminded that there was a request to use the larger key sizes as
well. So with apologies, I over-engineered this even more:
Any comments/objections/hatemail?
Index: ssl_engine_init.c
===================================================================
--- ssl_engine_init.c (revision 1603915)
+++ ssl_engine_init.c (working copy)
@@ -67,29 +67,39 @@
return dh;
}
-static DH *dhparam1024, *dhparam2048, *dhparam3072, *dhparam4096;
+/* Storage and initialization for DH parameters. */
+static struct dhparam {
+ BIGNUM *(*const prime)(BIGNUM *); /* function to generate... */
+ DH *dh; /* ...this, used for keys.... */
+ const unsigned int min; /* ...of length >= this. */
+} dhparams[] = {
+ { get_rfc3526_prime_8192, NULL, 6145 },
+ { get_rfc3526_prime_6144, NULL, 4097 },
+ { get_rfc3526_prime_4096, NULL, 3073 },
+ { get_rfc3526_prime_3072, NULL, 2049 },
+ { get_rfc3526_prime_2048, NULL, 1025 },
+ { get_rfc2409_prime_1024, NULL, 0 }
+};
static void init_dh_params(void)
{
- /*
- * Prepare DH parameters from 1024 to 4096 bits, in 1024-bit increments
- */
- dhparam1024 = make_dh_params(get_rfc2409_prime_1024, "2");
- dhparam2048 = make_dh_params(get_rfc3526_prime_2048, "2");
- dhparam3072 = make_dh_params(get_rfc3526_prime_3072, "2");
- dhparam4096 = make_dh_params(get_rfc3526_prime_4096, "2");
+ unsigned n;
+
+ for (n = 0; n < sizeof(dhparams)/sizeof(dhparams[0]); n++)
+ dhparams[n].dh = make_dh_params(dhparams[n].prime, "2");
}
static void free_dh_params(void)
{
+ unsigned n;
+
/* DH_free() is a noop for a NULL parameter, so these are harmless
* in the (unexpected) case where these variables are already
* NULL. */
- DH_free(dhparam1024);
- DH_free(dhparam2048);
- DH_free(dhparam3072);
- DH_free(dhparam4096);
- dhparam1024 = dhparam2048 = dhparam3072 = dhparam4096 = NULL;
+ for (n = 0; n < sizeof(dhparams)/sizeof(dhparams[0]); n++) {
+ DH_free(dhparams[n].dh);
+ dhparams[n].dh = NULL;
+ }
}
/* Hand out the same DH structure though once generated as we leak
@@ -101,14 +111,13 @@
* to our copy. */
DH *modssl_get_dh_params(unsigned keylen)
{
- if (keylen >= 4096)
- return dhparam4096;
- else if (keylen >= 3072)
- return dhparam3072;
- else if (keylen >= 2048)
- return dhparam2048;
- else
- return dhparam1024;
+ unsigned n;
+
+ for (n = 0; n < sizeof(dhparams)/sizeof(dhparams[0]); n++)
+ if (keylen >= dhparams[n].min)
+ return dhparams[n].dh;
+
+ return NULL; /* impossible to reach. */
}
static void ssl_add_version_components(apr_pool_t *p,
yet another mod_ssl temp DH handling tweak
Posted by Joe Orton <jo...@redhat.com>.
One more tweak here, proposed by Hubert from our QA team who has been
reviewing all this stuff. Hubert argued we should be erring on the side
of stronger not weaker here, particularly because of the possibility of
2048-bit keys being identified as 2047 in rare cases.
Is this reasonable?
Index: ssl_engine_init.c
===================================================================
--- ssl_engine_init.c (revision 1603915)
+++ ssl_engine_init.c (working copy)
@@ -101,11 +101,11 @@
* to our copy. */
DH *modssl_get_dh_params(unsigned keylen)
{
- if (keylen >= 4096)
+ if (keylen > 3072)
return dhparam4096;
- else if (keylen >= 3072)
+ else if (keylen > 2048)
return dhparam3072;
- else if (keylen >= 2048)
+ else if (keylen > 1024)
return dhparam2048;
else
return dhparam1024;
Re: Memory leak in mod_ssl ssl_callback_TmpDH
Posted by Joe Orton <jo...@redhat.com>.
On Sat, Jun 14, 2014 at 10:35:28AM +0200, Kaspar Brand wrote:
> Just a quick reminder... I think it's important to backport
> r1597349/r1598107 + the additional adjustment for 2.4.10 (happy to vote
> once it's settled on trunk).
Sorry guys. http://svn.apache.org/viewvc?view=revision&revision=1603915
Re: Memory leak in mod_ssl ssl_callback_TmpDH
Posted by Kaspar Brand <ht...@velox.ch>.
On 02.06.2014 20:49, Ruediger Pluem wrote:
> Joe Orton wrote:
>> On Wed, May 28, 2014 at 10:10:16PM +0200, Ruediger Pluem wrote:
>>> Thanks, but I missed some stuff during review:
>>>
>>> 1. We don't need to have two DH pointers in make_dh_params
>>
>> Doh!
>>
>>> 2. There possible frees on NULL pointers in free_dh_params:
>>
>> This is unnecessary because DH_free() does that already, but that
>> deserves a comment too. I'll fix this with your patch for (1) shortly,
>> thanks!
>>
>
> Are you waiting for any action from my side? Just want to avoid that we wait for each other and deadlock :-)
Just a quick reminder... I think it's important to backport
r1597349/r1598107 + the additional adjustment for 2.4.10 (happy to vote
once it's settled on trunk).
Kaspar
Re: Memory leak in mod_ssl ssl_callback_TmpDH
Posted by Ruediger Pluem <rp...@apache.org>.
Joe Orton wrote:
> On Wed, May 28, 2014 at 10:10:16PM +0200, Ruediger Pluem wrote:
>> Thanks, but I missed some stuff during review:
>>
>> 1. We don't need to have two DH pointers in make_dh_params
>
> Doh!
>
>> 2. There possible frees on NULL pointers in free_dh_params:
>
> This is unnecessary because DH_free() does that already, but that
> deserves a comment too. I'll fix this with your patch for (1) shortly,
> thanks!
>
Are you waiting for any action from my side? Just want to avoid that we wait for each other and deadlock :-)
Regards
Rüdiger
Re: Memory leak in mod_ssl ssl_callback_TmpDH
Posted by Joe Orton <jo...@redhat.com>.
On Wed, May 28, 2014 at 10:10:16PM +0200, Ruediger Pluem wrote:
> Thanks, but I missed some stuff during review:
>
> 1. We don't need to have two DH pointers in make_dh_params
Doh!
> 2. There possible frees on NULL pointers in free_dh_params:
This is unnecessary because DH_free() does that already, but that
deserves a comment too. I'll fix this with your patch for (1) shortly,
thanks!
Re: Memory leak in mod_ssl ssl_callback_TmpDH
Posted by Ruediger Pluem <rp...@apache.org>.
Joe Orton wrote:
> On Wed, May 28, 2014 at 01:58:05PM +0000, Plüm, Rüdiger, Vodafone Group wrote:
>> Looks great. Care to commit?
>
> http://svn.apache.org/viewvc?view=revision&revision=1598107
>
>
Thanks, but I missed some stuff during review:
1. We don't need to have two DH pointers in make_dh_params
2. There possible frees on NULL pointers in free_dh_params:
The following patch should fix this:
Index: modules/ssl/ssl_engine_init.c
===================================================================
--- modules/ssl/ssl_engine_init.c (revision 1598117)
+++ modules/ssl/ssl_engine_init.c (working copy)
@@ -54,21 +54,16 @@
static DH *make_dh_params(BIGNUM *(*prime)(BIGNUM *), const char *gen)
{
DH *dh = NULL;
- DH *dh_tmp;
- if (dh) {
- return dh;
- }
- if (!(dh_tmp = DH_new())) {
+ if (!(dh = DH_new())) {
return NULL;
}
- dh_tmp->p = prime(NULL);
- BN_dec2bn(&dh_tmp->g, gen);
- if (!dh_tmp->p || !dh_tmp->g) {
- DH_free(dh_tmp);
+ dh->p = prime(NULL);
+ BN_dec2bn(&dh->g, gen);
+ if (!dh->p || !dh->g) {
+ DH_free(dh);
return NULL;
}
- dh = dh_tmp;
return dh;
}
@@ -87,10 +82,18 @@
static void free_dh_params(void)
{
- DH_free(dhparam1024);
- DH_free(dhparam2048);
- DH_free(dhparam3072);
- DH_free(dhparam4096);
+ if (dhparam1024) {
+ DH_free(dhparam1024);
+ }
+ if (dhparam2048) {
+ DH_free(dhparam2048);
+ }
+ if (dhparam3072) {
+ DH_free(dhparam3072);
+ }
+ if (dhparam4096) {
+ DH_free(dhparam4096);
+ }
dhparam1024 = dhparam2048 = dhparam3072 = dhparam4096 = NULL;
}
Objections or should I commit?
Regards
Rüdiger
Re: Memory leak in mod_ssl ssl_callback_TmpDH
Posted by Joe Orton <jo...@redhat.com>.
On Wed, May 28, 2014 at 01:58:05PM +0000, Plüm, Rüdiger, Vodafone Group wrote:
> Looks great. Care to commit?
http://svn.apache.org/viewvc?view=revision&revision=1598107
RE: Memory leak in mod_ssl ssl_callback_TmpDH
Posted by Plüm,
Rüdiger,
Vodafone Group <ru...@vodafone.com>.
> -----Original Message-----
> From: Joe Orton [mailto:jorton@redhat.com]
> Sent: Mittwoch, 28. Mai 2014 15:43
> To: dev@httpd.apache.org
> Subject: Re: Memory leak in mod_ssl ssl_callback_TmpDH
>
> On Wed, May 28, 2014 at 09:05:06AM +0200, Kaspar Brand wrote:
> > Agree, CPU time is not a concern, get_dh_XXX() is only about populating
> > DH with builtin constants (DH_generate_key happens at connection time,
> > and is fast anyway).
>
> Ah, I did not realise. That is even more reason to do this at startup
> then, surely. Users do complain (tho rarely) about modules which leak
> across reloads, so it is worth caring about that too, and we can stay
> with globals rather than polluting the module structure.
>
> Any objections? This seems cleaner than fussing around with atomics and
> ignoring the race/leak.
Looks great. Care to commit?
Regards
Rüdiger
Re: Memory leak in mod_ssl ssl_callback_TmpDH
Posted by Joe Orton <jo...@redhat.com>.
On Wed, May 28, 2014 at 09:05:06AM +0200, Kaspar Brand wrote:
> Agree, CPU time is not a concern, get_dh_XXX() is only about populating
> DH with builtin constants (DH_generate_key happens at connection time,
> and is fast anyway).
Ah, I did not realise. That is even more reason to do this at startup
then, surely. Users do complain (tho rarely) about modules which leak
across reloads, so it is worth caring about that too, and we can stay
with globals rather than polluting the module structure.
Any objections? This seems cleaner than fussing around with atomics and
ignoring the race/leak.
Index: modules/ssl/ssl_engine_init.c
===================================================================
--- modules/ssl/ssl_engine_init.c (revision 1597999)
+++ modules/ssl/ssl_engine_init.c (working copy)
@@ -47,6 +47,73 @@
#define KEYTYPES "RSA or DSA"
#endif
+/*
+ * Grab well-defined DH parameters from OpenSSL, see <openssl/bn.h>
+ * (get_rfc*) for all available primes.
+ */
+static DH *make_dh_params(BIGNUM *(*prime)(BIGNUM *), const char *gen)
+{
+ DH *dh = NULL;
+ DH *dh_tmp;
+
+ if (dh) {
+ return dh;
+ }
+ if (!(dh_tmp = DH_new())) {
+ return NULL;
+ }
+ dh_tmp->p = prime(NULL);
+ BN_dec2bn(&dh_tmp->g, gen);
+ if (!dh_tmp->p || !dh_tmp->g) {
+ DH_free(dh_tmp);
+ return NULL;
+ }
+ dh = dh_tmp;
+ return dh;
+}
+
+static DH *dhparam1024, *dhparam2048, *dhparam3072, *dhparam4096;
+
+static void init_dh_params(void)
+{
+ /*
+ * Prepare DH parameters from 1024 to 4096 bits, in 1024-bit increments
+ */
+ dhparam1024 = make_dh_params(get_rfc2409_prime_1024, "2");
+ dhparam2048 = make_dh_params(get_rfc3526_prime_2048, "2");
+ dhparam3072 = make_dh_params(get_rfc3526_prime_3072, "2");
+ dhparam4096 = make_dh_params(get_rfc3526_prime_4096, "2");
+}
+
+static void free_dh_params(void)
+{
+ DH_free(dhparam1024);
+ DH_free(dhparam2048);
+ DH_free(dhparam3072);
+ DH_free(dhparam4096);
+ dhparam1024 = dhparam2048 = dhparam3072 = dhparam4096 = NULL;
+}
+
+/* Hand out the same DH structure though once generated as we leak
+ * memory otherwise and freeing the structure up after use would be
+ * hard to track and in fact is not needed at all as it is safe to
+ * use the same parameters over and over again security wise (in
+ * contrast to the keys itself) and code safe as the returned structure
+ * is duplicated by OpenSSL anyway. Hence no modification happens
+ * to our copy. */
+DH *modssl_get_dh_params(unsigned keylen)
+{
+ if (keylen >= 4096)
+ return dhparam4096;
+ else if (keylen >= 3072)
+ return dhparam3072;
+ else if (keylen >= 2048)
+ return dhparam2048;
+ else
+ return dhparam1024;
+
+}
+
static void ssl_add_version_components(apr_pool_t *p,
server_rec *s)
{
@@ -277,6 +344,8 @@
SSL_init_app_data2_idx(); /* for SSL_get_app_data2() at request time */
+ init_dh_params();
+
return OK;
}
@@ -1706,5 +1775,7 @@
ssl_init_ctx_cleanup(sc->server);
}
+ free_dh_params();
+
return APR_SUCCESS;
}
Index: modules/ssl/ssl_engine_kernel.c
===================================================================
--- modules/ssl/ssl_engine_kernel.c (revision 1597999)
+++ modules/ssl/ssl_engine_kernel.c (working copy)
@@ -1311,47 +1311,6 @@
*/
/*
- * Grab well-defined DH parameters from OpenSSL, see <openssl/bn.h>
- * (get_rfc*) for all available primes.
- * Hand out the same DH structure though once generated as we leak
- * memory otherwise and freeing the structure up after use would be
- * hard to track and in fact is not needed at all as it is safe to
- * use the same parameters over and over again security wise (in
- * contrast to the keys itself) and code safe as the returned structure
- * is duplicated by OpenSSL anyway. Hence no modification happens
- * to our copy.
- */
-#define make_get_dh(rfc,size,gen) \
-static DH *get_dh##size(void) \
-{ \
- static DH *dh = NULL; \
- DH *dh_tmp; \
-\
- if (dh) { \
- return dh; \
- } \
- if (!(dh_tmp = DH_new())) { \
- return NULL; \
- } \
- dh_tmp->p = get_##rfc##_prime_##size(NULL); \
- BN_dec2bn(&dh_tmp->g, #gen); \
- if (!dh_tmp->p || !dh_tmp->g) { \
- DH_free(dh_tmp); \
- return NULL; \
- } \
- dh = dh_tmp; \
- return dh; \
-}
-
-/*
- * Prepare DH parameters from 1024 to 4096 bits, in 1024-bit increments
- */
-make_get_dh(rfc2409, 1024, 2)
-make_get_dh(rfc3526, 2048, 2)
-make_get_dh(rfc3526, 3072, 2)
-make_get_dh(rfc3526, 4096, 2)
-
-/*
* Hand out standard DH parameters, based on the authentication strength
*/
DH *ssl_callback_TmpDH(SSL *ssl, int export, int keylen)
@@ -1390,14 +1349,8 @@
ap_log_cerror(APLOG_MARK, APLOG_TRACE2, 0, c,
"handing out built-in DH parameters for %d-bit authenticated connection", keylen);
- if (keylen >= 4096)
- return get_dh4096();
- else if (keylen >= 3072)
- return get_dh3072();
- else if (keylen >= 2048)
- return get_dh2048();
- else
- return get_dh1024();
+ return modssl_get_dh_params(keylen);
+
}
/*
Index: modules/ssl/ssl_private.h
===================================================================
--- modules/ssl/ssl_private.h (revision 1597999)
+++ modules/ssl/ssl_private.h (working copy)
@@ -931,6 +931,11 @@
conn_rec *c, apr_pool_t *p);
#endif
+/* Retrieve DH parameters for given key length. Return value should
+ * be treated as unmutable, since it is stored in process-global
+ * memory. */
+DH *modssl_get_dh_params(unsigned keylen);
+
#if HAVE_VALGRIND
extern int ssl_running_on_valgrind;
#endif
Re: Memory leak in mod_ssl ssl_callback_TmpDH
Posted by Kaspar Brand <ht...@velox.ch>.
On 27.05.2014 22:33, Ruediger Pluem wrote:
> Joe Orton wrote:
>> We have a potential race here between threads doing the param
>> generation, right?
>>
>> + static DH *dh = NULL; \
>> + DH *dh_tmp; \
>> ...
>> + dh = dh_tmp; \
>>
>> though it would not matter who wins the race *if* we could rely on
>> pointer assignment being atomic - which is a fairly dubious assumption,
>
> That was my assumption. If we cannot assume that the pointer assignment is atomic,
> then we have a race problem.
>
> Would the following patch address your concern?
I would be in favor of fixing the potential race condition in this (or a
similar) way... otherwise we would 1) again have to hardcode
algorithm-dependent things into one of our structs and 2) "generate" DH
parameters at startup even though they might never get be needed (if
someone turns off DHE with !kEDH in SSLCipherSuite, none of these DH
parameters would ever be used).
>> it might be better to do it once at startup to save CPU time anyway?
>
> I am not that worried about CPU because I guess we fairly quickly get dh set to a valid value
Agree, CPU time is not a concern, get_dh_XXX() is only about populating
DH with builtin constants (DH_generate_key happens at connection time,
and is fast anyway).
Kaspar
Re: Memory leak in mod_ssl ssl_callback_TmpDH
Posted by Ruediger Pluem <rp...@apache.org>.
Joe Orton wrote:
> On Sat, May 24, 2014 at 10:32:35AM +0200, Kaspar Brand wrote:
>> On 19.05.2014 10:15, Plüm, Rüdiger, Vodafone Group wrote:
>>> Maybe stupid idea, but can't we do that once and hand it out over
>>> and over again?
>>
>> Not a stupid idea at all - I think it's actually the most sensible
>> solution to this problem. This is what OpenSSL does with the
>> DH parameters provided by the callback in s3_srvr.c:ssl3_send_server_key_exchange():
>
> This may be a stupid question: if we are doing this once per process
> lifetime would it not be better to do it at init time, and store the
> results somewhere other than a static variable?
>
> We have a potential race here between threads doing the param
> generation, right?
>
> + static DH *dh = NULL; \
> + DH *dh_tmp; \
> ...
> + dh = dh_tmp; \
>
> though it would not matter who wins the race *if* we could rely on
> pointer assignment being atomic - which is a fairly dubious assumption,
That was my assumption. If we cannot assume that the pointer assignment is atomic,
then we have a race problem.
Would the following patch address your concern?
Index: modules/ssl/ssl_engine_kernel.c
===================================================================
--- modules/ssl/ssl_engine_kernel.c (revision 1597665)
+++ modules/ssl/ssl_engine_kernel.c (working copy)
@@ -31,6 +31,7 @@
#include "ssl_private.h"
#include "mod_ssl.h"
#include "util_md5.h"
+#include "apr_atomic.h"
static void ssl_configure_env(request_rec *r, SSLConnRec *sslconn);
#ifdef HAVE_TLSEXT
@@ -1320,6 +1321,10 @@
* contrast to the keys itself) and code safe as the returned structure
* is duplicated by OpenSSL anyway. Hence no modification happens
* to our copy.
+ *
+ * Use apr_atomic_xchgptr to assign the result to dh as we might have
+ * multiple threads calling us in parallel and pointer assignment might
+ * not be atomic.
*/
#define make_get_dh(rfc,size,gen) \
static DH *get_dh##size(void) \
@@ -1339,7 +1344,7 @@
DH_free(dh_tmp); \
return NULL; \
} \
- dh = dh_tmp; \
+ apr_atomic_xchgptr((volatile void **)&dh, dh_tmp); \
return dh; \
}
> and at least deserves a comment. If a potential race is possible here
> it might be better to do it once at startup to save CPU time anyway?
I am not that worried about CPU because I guess we fairly quickly get dh set to a valid value and afterwards stuff is
fast, but for sure this would be another option.
Regards
Rüdiger
Re: Memory leak in mod_ssl ssl_callback_TmpDH
Posted by Joe Orton <jo...@redhat.com>.
On Sat, May 24, 2014 at 10:32:35AM +0200, Kaspar Brand wrote:
> On 19.05.2014 10:15, Plüm, Rüdiger, Vodafone Group wrote:
> > Maybe stupid idea, but can't we do that once and hand it out over
> > and over again?
>
> Not a stupid idea at all - I think it's actually the most sensible
> solution to this problem. This is what OpenSSL does with the
> DH parameters provided by the callback in s3_srvr.c:ssl3_send_server_key_exchange():
This may be a stupid question: if we are doing this once per process
lifetime would it not be better to do it at init time, and store the
results somewhere other than a static variable?
We have a potential race here between threads doing the param
generation, right?
+ static DH *dh = NULL; \
+ DH *dh_tmp; \
...
+ dh = dh_tmp; \
though it would not matter who wins the race *if* we could rely on
pointer assignment being atomic - which is a fairly dubious assumption,
and at least deserves a comment. If a potential race is possible here
it might be better to do it once at startup to save CPU time anyway?
Regards, Joe
Re: Memory leak in mod_ssl ssl_callback_TmpDH
Posted by Ruediger Pluem <rp...@apache.org>.
Kaspar Brand wrote:
> On 19.05.2014 10:15, Plüm, Rüdiger, Vodafone Group wrote:
>> Maybe stupid idea, but can't we do that once and hand it out over
>> and over again?
>
>
>> So something like:
>
> Looks good to me. I suggest adding a short comment which explains
> the rationale for using dh and dh_tmp (the SSL_CTX_set_tmp_dh_callback(3)
> man page e.g. doesn't make it clear that reusing the parameters
> within the lifetime of a process is actually a must to prevent memory
> from leaking).
Thanks for the detailed review. r1597349. Feel free to tune the comment.
Regards
Rüdiger
Re: Memory leak in mod_ssl ssl_callback_TmpDH
Posted by Kaspar Brand <ht...@velox.ch>.
On 19.05.2014 10:15, Plüm, Rüdiger, Vodafone Group wrote:
> Maybe stupid idea, but can't we do that once and hand it out over
> and over again?
Not a stupid idea at all - I think it's actually the most sensible
solution to this problem. This is what OpenSSL does with the
DH parameters provided by the callback in s3_srvr.c:ssl3_send_server_key_exchange():
if (type & SSL_kEDH)
{
dhp=cert->dh_tmp;
if ((dhp == NULL) && (s->cert->dh_tmp_cb != NULL))
dhp=s->cert->dh_tmp_cb(s,
SSL_C_IS_EXPORT(s->s3->tmp.new_cipher),
SSL_C_EXPORT_PKEYLENGTH(s->s3->tmp.new_cipher));
if (dhp == NULL)
{
al=SSL_AD_HANDSHAKE_FAILURE;
SSLerr(SSL_F_SSL3_SEND_SERVER_KEY_EXCHANGE,SSL_R_MISSING_TMP_DH_KEY);
goto f_err;
}
if (s->s3->tmp.dh != NULL)
{
SSLerr(SSL_F_SSL3_SEND_SERVER_KEY_EXCHANGE, ERR_R_INTERNAL_ERROR);
goto err;
}
if ((dh=DHparams_dup(dhp)) == NULL)
{
SSLerr(SSL_F_SSL3_SEND_SERVER_KEY_EXCHANGE,ERR_R_DH_LIB);
goto err;
}
s->s3->tmp.dh=dh;
if ((dhp->pub_key == NULL ||
dhp->priv_key == NULL ||
(s->options & SSL_OP_SINGLE_DH_USE)))
{
if(!DH_generate_key(dh))
{
SSLerr(SSL_F_SSL3_SEND_SERVER_KEY_EXCHANGE,
ERR_R_DH_LIB);
goto err;
}
}
else
{
dh->pub_key=BN_dup(dhp->pub_key);
dh->priv_key=BN_dup(dhp->priv_key);
if ((dh->pub_key == NULL) ||
(dh->priv_key == NULL))
{
SSLerr(SSL_F_SSL3_SEND_SERVER_KEY_EXCHANGE,ERR_R_DH_LIB);
goto err;
}
}
r[0]=dh->p;
r[1]=dh->g;
r[2]=dh->pub_key;
}
I.e., the "dhp" parameters provided by the callback (dh_tmp_cb) are
duplicated, and as Rainer mentioned, we would have to keep track
of dhp ourselves in order to be able to free it ourselves later on.
(In 2.2.x or before 2.4.7, these parameters/keys are part of mod_ssl's
global SSLModConfigRec, that's how the leak is avoided there.)
> It looks like to me that we only generate the parameters, not
> the keys. And reusing the same parameters should be save.
Correct as well, yes. DH_new sets pub_key and priv_key both to NULL,
so DH_generate_key is always called by ssl3_send_server_key_exchange().
> So something like:
Looks good to me. I suggest adding a short comment which explains
the rationale for using dh and dh_tmp (the SSL_CTX_set_tmp_dh_callback(3)
man page e.g. doesn't make it clear that reusing the parameters
within the lifetime of a process is actually a must to prevent memory
from leaking).
Kaspar
RE: Memory leak in mod_ssl ssl_callback_TmpDH
Posted by Plüm,
Rüdiger,
Vodafone Group <ru...@vodafone.com>.
> -----Original Message-----
> From: Rainer Jung [mailto:rainer.jung@kippdata.de]
> Sent: Samstag, 17. Mai 2014 07:00
> To: dev@httpd.apache.org
> Subject: Memory leak in mod_ssl ssl_callback_TmpDH
>
> While doing some customization of mod_ssl I checked for memory leaks on
> Solaris using libumem and found 5 allocations that happen for each
> handshake and do not seem to get freed.
>
> Versions: httpd 2.4 head plus OpenSSL 1.0.1g
>
> > ::findleaks
> ...
> 000b9688 85 002779c8 mod_ssl.so`default_malloc_ex+0x20
> 000b8788 85 00185d10 mod_ssl.so`default_malloc_ex+0x20
> 000bea08 84 00178690 mod_ssl.so`default_malloc_ex+0x20
> 000b8288 84 00131ab8 mod_ssl.so`default_malloc_ex+0x20
> 000b8788 84 00185d88 mod_ssl.so`default_malloc_ex+0x20
>
> ...
>
> The number 84/85 is the number of connections handled.
>
> The five allocation stacks always point to ssl_callback_TmpDH, at
> offsets 0x12c, 0xec and 0x140:
>
> > 002779c8::bufctl_audit
> ADDR BUFADDR TIMESTAMP THREAD
> CACHE LASTLOG CONTENTS
> 2779c8 275c78 277d6c5029c999 1
> b9688 a5b58 0
> libumem.so.1`umem_cache_alloc+0x210
> libumem.so.1`umem_alloc+0x60
> libumem.so.1`malloc+0x28
> mod_ssl.so`default_malloc_ex+0x20
> mod_ssl.so`CRYPTO_malloc+0x88
> mod_ssl.so`DH_new_method+0x24
> mod_ssl.so`ssl_callback_TmpDH+0x12c
> mod_ssl.so`ssl3_send_server_key_exchange+0xad8
>
> > 00185d88::bufctl_audit
> ADDR BUFADDR TIMESTAMP THREAD
> CACHE LASTLOG CONTENTS
> 185d88 180ed8 277d6c502a3e23 1
> b8788 a5e14 0
> libumem.so.1`umem_cache_alloc+0x13c
> libumem.so.1`umem_alloc+0x60
> libumem.so.1`malloc+0x28
> mod_ssl.so`default_malloc_ex+0x20
> mod_ssl.so`CRYPTO_malloc+0x88
> mod_ssl.so`BN_new+0x24
> mod_ssl.so`BN_dec2bn+0x220
> mod_ssl.so`ssl_callback_TmpDH+0xec
> mod_ssl.so`ssl3_send_server_key_exchange+0xad8
>
> > 00131ab8::bufctl_audit
> ADDR BUFADDR TIMESTAMP THREAD
> CACHE LASTLOG CONTENTS
> 131ab8 12b620 277d6c502a483a 1
> b8288 a5e78 0
> libumem.so.1`umem_cache_alloc+0x210
> libumem.so.1`umem_alloc+0x60
> libumem.so.1`malloc+0x28
> mod_ssl.so`default_malloc_ex+0x20
> mod_ssl.so`CRYPTO_malloc+0x88
> mod_ssl.so`bn_expand_internal+0x44
> mod_ssl.so`bn_expand2+0x20
> mod_ssl.so`BN_dec2bn+0x1bc
> mod_ssl.so`ssl_callback_TmpDH+0xec
> mod_ssl.so`ssl3_send_server_key_exchange+0xad8
>
> > 00185d10::bufctl_audit
> ADDR BUFADDR TIMESTAMP THREAD
> CACHE LASTLOG CONTENTS
> 185d10 180f08 277d6c502a0e49 1
> b8788 a5d4c 0
> libumem.so.1`umem_cache_alloc+0x13c
> libumem.so.1`umem_alloc+0x60
> libumem.so.1`malloc+0x28
> mod_ssl.so`default_malloc_ex+0x20
> mod_ssl.so`CRYPTO_malloc+0x88
> mod_ssl.so`BN_new+0x24
> mod_ssl.so`BN_bin2bn+0x11c
> mod_ssl.so`ssl_callback_TmpDH+0x140
> mod_ssl.so`ssl3_send_server_key_exchange+0xad8
>
> > 00178690::bufctl_audit
> ADDR BUFADDR TIMESTAMP THREAD
> CACHE LASTLOG CONTENTS
> 178690 17f3c0 277d6c502a23c5 1
> bea08 a5db0 0
> libumem.so.1`umem_cache_alloc+0x13c
> libumem.so.1`umem_alloc+0x60
> libumem.so.1`malloc+0x28
> mod_ssl.so`default_malloc_ex+0x20
> mod_ssl.so`CRYPTO_malloc+0x88
> mod_ssl.so`bn_expand_internal+0x44
> mod_ssl.so`bn_expand2+0x20
> mod_ssl.so`BN_bin2bn+0xf0
> mod_ssl.so`ssl_callback_TmpDH+0x140
> mod_ssl.so`ssl3_send_server_key_exchange+0xad8
>
> ssl_callback_TmpDH is in modules/ssl/ssl_engine_kernel.c.
>
> I think this is due to the changes in modules/ssl/ssl_engine_init.c
> applied by the backported r1542327.
>
> Function ssl_callback_TmpDH calls functions get_dh4096() or get_dh3072()
> or get_dh2048() or get_dh1024(), which are defined by macro make_get_dh
> and include code:
>
> DH *dh;
> if (!(dh = DH_new())) {
> ^^^^^^
> return NULL;
> }
> dh->p = get_##rfc##_prime_##size(NULL);
> BN_dec2bn(&dh->g, #gen);
> ^^^^^^^^^
> if (!dh->p || !dh->g) {
> DH_free(dh);
> return NULL;
> }
> return dh;
>
> So the first three leaks come from here. For the leak with BN_bin2bn it
> is hidden in get_##rfc##_prime_##size, e.g. get_rfc3526_prime_4096()
> contains the line
>
> return BN_bin2bn(RFC3526_PRIME_4096,sizeof(RFC3526_PRIME_4096),bn);
>
> It seems to me that a single call to DH_free(dh) would suffice to free
> all 5 allocations.
Maybe stupid idea, but can't we do that once and hand it out over and over again?
It looks like to me that we only generate the parameters, not the keys. And reusing the same parameters should be save.
So something like:
Index: ssl_engine_kernel.c
===================================================================
--- ssl_engine_kernel.c (revision 1592470)
+++ ssl_engine_kernel.c (working copy)
@@ -1317,16 +1317,22 @@
#define make_get_dh(rfc,size,gen) \
static DH *get_dh##size(void) \
{ \
- DH *dh; \
- if (!(dh = DH_new())) { \
+ static DH *dh = NULL; \
+ DH *dh_tmp; \
+\
+ if (dh) { \
+ return dh; \
+ } \
+ if (!(dh_tmp = DH_new())) { \
return NULL; \
} \
- dh->p = get_##rfc##_prime_##size(NULL); \
- BN_dec2bn(&dh->g, #gen); \
- if (!dh->p || !dh->g) { \
- DH_free(dh); \
+ dh_tmp->p = get_##rfc##_prime_##size(NULL); \
+ BN_dec2bn(&dh_tmp->g, #gen); \
+ if (!dh_tmp->p || !dh_tmp->g) { \
+ DH_free(dh_tmp); \
return NULL; \
} \
+ dh = dh_tmp; \
return dh; \
}
Regards
Rüdiger