> -----Original Message-----
> From: Rainer Jung [mailto:[email protected]]
> Sent: Samstag, 17. Mai 2014 07:00
> To: [email protected]
> 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
