Re: (ITS#5849) memory leak when using client certificates

[dhawes@vt.edu]

dhawes@vt.edu wrote:
> dhawes@vt.edu wrote:
>> Full_Name: David Hawes
>> Version: 2.4.13
>> OS: Debian GNU/Linux 4.0
>> URL: 
>> Submission from: (NULL) (98.117.88.57)
>>
>>
>> As outlined at:
>>
>> http://www.openldap.org/lists/openldap-software/200811/msg00136.html
>>
>> OpenLDAP 2.4 leaks memory when TLS client certificates are used.  Running slapd
>> under valgrind yields the following:
>>
>> ==13311== 4,906 (92 direct, 4,814 indirect) bytes in 1 blocks are
>> definitely lost in loss record 19 of 23
>> ==13311==    at 0x401D898: malloc (vg_replace_malloc.c:207)
>> ==13311==    by 0x41FCCC4: default_malloc_ex (mem.c:79)
>> ==13311==    by 0x41FD33F: CRYPTO_malloc (mem.c:304)
>> ==13311==    by 0x428CA65: asn1_item_ex_combine_new (tasn_new.c:191)
>> ==13311==    by 0x428C79C: ASN1_item_ex_new (tasn_new.c:85)
>> ==13311==    by 0x428ECAA: ASN1_item_ex_d2i (tasn_dec.c:399)
>> ==13311==    by 0x428E5F9: ASN1_item_d2i (tasn_dec.c:134)
>> ==13311==    by 0x4286A57: d2i_X509 (x_x509.c:136)
>> ==13311==    by 0x4194F26: ssl3_get_client_certificate (s3_srvr.c:2521)
>> ==13311==    by 0x4191897: ssl3_accept (s3_srvr.c:462)
>> ==13311==    by 0x41AD930: SSL_accept (ssl_lib.c:867)
>> ==13311==    by 0x815D00E: ldap_pvt_tls_accept (tls.c:1594)
>> ==13311==    by 0x8076926: connection_read_thread (connection.c:1286)
>> ==13311==    by 0x813CEE5: ldap_int_thread_pool_wrapper (tpool.c:663)
>> ==13311==    by 0x415823F: start_thread (in
>> /lib/tls/i686/cmov/libpthread-2.3.6.so)
>> ==13311==    by 0x43ED49D: clone (in /lib/tls/i686/cmov/libc-2.3.6.so)
>>
>> Philip Guenther notes:
>>
>> In 2.4.x, tls_get_cert_dn() leaks a reference to the client's X509 cert: 
>> the call to SSL_get_peer_certificate() in tls_get_cert() increments the 
>> reference count on the cert and it never gets decremented by a call to 
>> X509_free().  Simply adding the call there might not be safe, depending on 
>> whether the berval that tls_get_cert_dn() sets up relies on the underlying 
>> X509 to stay valid for longer than this chain of calls, as the X509 may be 
>> invalidated by a rehandshake.
> 
> Calling X509_free(x) before the return statement in tls_get_cert_dn()
> does fix the memory leak, though I do not know if this is proper, as
> Philip noted.

My apologies for not checking the ITS before sending that message.

The fix in HEAD works.