You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@httpd.apache.org by Ruediger Pluem <rp...@apache.org> on 2014/06/02 20:49:53 UTC
Re: Memory leak in mod_ssl ssl_callback_TmpDH
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: 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