Menu ▾ ▴

#1236 Curl double-free with pkcs12 handling

closed-fixed
Daniel Stenberg
None
5
2014-12-29
2013-05-31
Nikaiw
No

Summary :

There is a double free bug in curl when using p12 client certificate to connect to an https server.

Steps to reproduce :

Connect to any https server using a client certificate in pkcs12 format.

curl -vvv --cert yourcertificate.p12:yourpassword --cert-type P12 https://yourserver.com

eg:

  • About to connect() to ** port 443 (#0)
  • Trying ****...
  • Connected to * (...*) port 443 (#0)
  • successfully set certificate verify locations:
  • CAfile: none
    CApath: /etc/ssl/certs
  • SSLv3, TLS handshake, Client hello (1):
  • SSLv3, TLS handshake, Server hello (2):
  • SSLv3, TLS handshake, CERT (11):
  • SSLv3, TLS alert, Server hello (2):
  • SSL certificate problem: unable to get local issuer certificate
  • Closing connection 0
    Erreur de segmentation (core dumped)

Bug is 100% reproducible.

Affected version :

Affected version are curl-7.24 to 7.30.

Details :

Bug was introduced in following commit to fix a memory leak:

https://github.com/bagder/curl/commit/6ea7acf5a96786f7514be4fbce174cdc8bedfdd1

on line 512 of ssluse.c: sk_X509_pop_free(ca, X509_free);

curl is freeing data related to the handling of the certificate which may have already being freed by SSL_CTX_free call made on line 865 of ssluse.c when we are closing the ssl connexion.

Here is a valgrind stacktrace:

==4136== Invalid free() / delete / delete[] / realloc()
==4136== at 0x4C2BA6C: free (vg_replace_malloc.c:446)
==4136== by 0x5B964EC: CRYPTO_free (mem.c:397)
==4136== by 0x5C4029F: asn1_item_combine_free (tasn_fre.c:176)
==4136== by 0x5C40484: ASN1_item_free (tasn_fre.c:71)
==4136== by 0x5C1E96F: sk_pop_free (stack.c:283)
==4136== by 0x590F806: SSL_CTX_free (ssl_lib.c:1947)
==4136== by 0x4E67BD4: Curl_ossl_close (ssluse.c:865)
==4136== by 0x4E89822: Curl_ssl_close (sslgen.c:421)
==4136== by 0x4E60906: Curl_disconnect (url.c:2565)
==4136== by 0x4E81B40: multi_runsingle (multi.c:1637)
==4136== by 0x4E81D54: curl_multi_perform (multi.c:1708)
==4136== by 0x4E76CDC: curl_easy_perform (easy.c:448)
==4136== Address 0x6636d00 is 0 bytes inside a block of size 184 free'd
==4136== at 0x4C2BA6C: free (vg_replace_malloc.c:446)
==4136== by 0x5B964EC: CRYPTO_free (mem.c:397)
==4136== by 0x5C4029F: asn1_item_combine_free (tasn_fre.c:176)
==4136== by 0x5C40484: ASN1_item_free (tasn_fre.c:71)
==4136== by 0x5C1E96F: sk_pop_free (stack.c:283)
==4136== by 0x4E67378: cert_stuff (ssluse.c:510)
==4136== by 0x4E68F54: ossl_connect_step1 (ssluse.c:1524)
==4136== by 0x4E6BBC8: ossl_connect_common (ssluse.c:2433)
==4136== by 0x4E6BE46: Curl_ossl_connect_nonblocking (ssluse.c:2522)
==4136== by 0x4E890A2: Curl_ssl_connect_nonblocking (sslgen.c:218)
==4136== by 0x4E49D5C: https_connecting (http.c:1345)
==4136== by 0x4E49CBA: Curl_http_connect (http.c:1315)

I'm not sure the bug is exploitable nor i see any practical case.

1 Attachments
testca-test-0.p12

Discussion

  • Nikaiw

    Nikaiw - 2013-06-03

    nb: The joined pkcs12 certificate doesn't need password.

     

  • Daniel Stenberg

    Daniel Stenberg - 2013-06-03

    Are you really talking about line numbers in the 7.30.0 version of ssluse.c?

     

  • Nikaiw

    Nikaiw - 2013-06-10

    The line number i'm referring to are the line number of the commit, thus version 7.24. However, the valgrind stacktrace lines should be referring to the last version.

     

  • Nikaiw

    Nikaiw - 2013-06-10

    (Actually the function Curl_ossl_close() is on line 855 in ssluse.c of the version 7.24 and i guess at line 865 in latest)

     

  • Daniel Stenberg

    Daniel Stenberg - 2013-06-10
    • assigned_to: Daniel Stenberg
     

  • Daniel Stenberg

    Daniel Stenberg - 2013-06-10

    I've tried to repeat your issue with the p12 file you provided and an edited version of the command line you said you used, but I don't have any site that requires client cert so I don't know exactly how to repeat it as my command line will only end in a "curl: (58) unable to use client certificate (no key found or wrong pass phrase?)" ...

    curl -v --cert testca-test-0.p12:  https://www.sourceforge.com

    Why is a client cert using the substring 'ca' in it? It sounds suspiciously wrong to me.

    $ ./src/curl -V
curl 7.31.0-DEV (x86_64-unknown-linux-gnu) libcurl/7.31.0-DEV OpenSSL/1.0.1e zlib/1.2.8 c-ares/1.10.1-DEV libidn/1.25 libssh2/1.4.4_DEV librtmp/2.3
Protocols: dict file ftp ftps gopher http https imap imaps ldap ldaps pop3 pop3s rtmp rtsp scp sftp smtp smtps telnet tftp 
Features: AsynchDNS Debug TrackMemory GSS-Negotiate IDN IPv6 Largefile NTLM NTLM_WB SSL libz TLS-SRP Metalink

    Any chance you have a suggested patch for the problem?

     

  • Nikaiw

    Nikaiw - 2013-06-10

    You forgot to put the "--cert-type P12" in the command line argument.

    curl --cert testca-test-0.p12 --cert-type P12 https://www.sourceforge.com
Erreur de segmentation (core dumped)

curl --version
curl 7.29.0 (x86_64-pc-linux-gnu) libcurl/7.29.0 OpenSSL/1.0.1c zlib/1.2.7 libidn/1.25 librtmp/2.3
Protocols: dict file ftp ftps gopher http https imap imaps ldap ldaps pop3 pop3s rtmp rtsp smtp smtps telnet tftp 
Features: GSS-Negotiate IDN IPv6 Largefile NTLM NTLM_WB SSL libz TLS-SRP

    It succesfully crashed on 7.30 as well.
    However, I surprisingly couldn't reproduce either with your 7.31 developement version:

    ./curl -V
curl 7.31.0-20130610 (x86_64-unknown-linux-gnu) libcurl/7.31.0-20130610 OpenSSL/1.0.1c zlib/1.2.7 libssh2/1.4.2
Protocols: dict file ftp ftps gopher http https imap imaps pop3 pop3s rtsp scp sftp smtp smtps telnet tftp 
Features: IPv6 Largefile NTLM NTLM_WB SSL libz TLS-SRP

    I don't have a suggested patch I don't know curl code enough, i only notice we could avoid the problem by commenting out the last "sk_X509_pop_free(ca, X509_free);" but It could and would probably introduce a previously patched memory leak.

     

    Last edit: Nikaiw 2013-06-10

  • Nikaiw

    Nikaiw - 2013-06-10

    Even in 7.31 valgrind is reporting.

    ==24005== Invalid free() / delete / delete[] / realloc()
==24005==    at 0x4C2BA6C: free (vg_replace_malloc.c:446)
==24005==    by 0x59434EC: CRYPTO_free (mem.c:397)
==24005==    by 0x59E8FAD: x509_cb (x_x509.c:126)
==24005==    by 0x59ED28D: asn1_item_combine_free (tasn_fre.c:173)
==24005==    by 0x59ED484: ASN1_item_free (tasn_fre.c:71)
==24005==    by 0x59CB96F: sk_pop_free (stack.c:283)
==24005==    by 0x56BC806: SSL_CTX_free (ssl_lib.c:1947)
==24005==    by 0x4E5A475: Curl_ossl_close (in /tmp/curl-7.31.0-20130610/lib/.libs/libcurl.so.4.3.0)
==24005==    by 0x4E52A0A: Curl_disconnect (in /tmp/curl-7.31.0-20130610/lib/.libs/libcurl.so.4.3.0)
==24005==    by 0x4E66DCF: curl_multi_cleanup (in /tmp/curl-7.31.0-20130610/lib/.libs/libcurl.so.4.3.0)
==24005==    by 0x4E4E927: Curl_close (in /tmp/curl-7.31.0-20130610/lib/.libs/libcurl.so.4.3.0)
==24005==    by 0x409BEC: operate (in /tmp/curl-7.31.0-20130610/src/.libs/lt-curl)
==24005==  Address 0x65e34f0 is 0 bytes inside a block of size 268 free'd
==24005==    at 0x4C2BA6C: free (vg_replace_malloc.c:446)
==24005==    by 0x59434EC: CRYPTO_free (mem.c:397)
==24005==    by 0x59E8FAD: x509_cb (x_x509.c:126)
==24005==    by 0x59ED28D: asn1_item_combine_free (tasn_fre.c:173)
==24005==    by 0x59ED484: ASN1_item_free (tasn_fre.c:71)
==24005==    by 0x59CB96F: sk_pop_free (stack.c:283)
==24005==    by 0x4E5A013: ossl_connect_common (in /tmp/curl-7.31.0-20130610/lib/.libs/libcurl.so.4.3.0)
==24005==    by 0x4E6C3E7: Curl_ssl_connect_nonblocking (in /tmp/curl-7.31.0-20130610/lib/.libs/libcurl.so.4.3.0)
==24005==    by 0x4E42E7D: https_connecting (in /tmp/curl-7.31.0-20130610/lib/.libs/libcurl.so.4.3.0)
==24005==    by 0x4E5584E: Curl_protocol_connect (in /tmp/curl-7.31.0-20130610/lib/.libs/libcurl.so.4.3.0)
==24005==    by 0x4E67FB8: multi_runsingle (in /tmp/curl-7.31.0-20130610/lib/.libs/libcurl.so.4.3.0)
==24005==    by 0x4E68B24: curl_multi_perform (in /tmp/curl-7.31.0-20130610/lib/.libs/libcurl.so.4.3.0)

    So the crash is maybe avoid just because of how the heap is used. My 7.31 compiled version is linked with way less library.

     

  • Daniel Stenberg

    Daniel Stenberg - 2013-06-10

    Thanks, I can repeat it at will now. But I haven't quite worked out what the suitable way forward is.

    I don't quite understand how we are supposed to deal with the list we get back in the 'ca' argument to the PKCS12_parse() call.

    If we call sk_X509_pop_free() like today (I do have a cleaned up code patch to propose but that isn't strictly relevant so I'll save that for now) we get the error you report here, but if we remove the call completely we instead get a memory leak! Have you figured out what the correct logic is?

     

  • Nikaiw

    Nikaiw - 2013-06-10

    It's a tricky bug I spend some time on it. I haven't figured out yet what is the correct logic. I think curl should be able to know if the certificate structure has already be cleaned by a closing of the ssl connexion.

    The usual way to avoid such problem is to set the pointer to NULL after it is freed. But in this case, I think the certificate parsed structure is part of another big structure which is free (as the client certificate is part of the ssl connection), and the cleaning is made into the libssl library.

     

  • Daniel Stenberg

    Daniel Stenberg - 2013-06-10

    It is the call to SSL_CTX_add_extra_chain_cert() that creates the problem. If we remove that call, things work just fine with the sk_X509_pop_free() call still present.

     

  • Daniel Stenberg

    Daniel Stenberg - 2013-06-10
    • status: open --> closed-fixed
     

  • Daniel Stenberg

    Daniel Stenberg - 2013-06-10

    I found it. In commit 7b97f03f09 I no longer get any crash and no leak. Please try it out and see that it works for you too!

     

  • Nikaiw

    Nikaiw - 2013-06-13

    Fix is fine for me.

     
MongoDB Logo MongoDB