There is a memory leak in mod_ssl-2.8.11-1.3.27 when client-authentication is used. The peer certificates are leaked - as much as 3-4K per request.
I am enclosing a description of the memory leak, and a suggested patch to mod_ssl-2.8.11-1.3.27 to fix it. I'd appreciate if it (or some variant of the same idea) will be applied to mod_ssl. I haven't yet looked whether the same leak exists in Apache 2 and whether it should be fixed there too. Thanks to Zvi Har'El for researching and fixing this bug with me. Description of the bug: ----------------------- When Apache+mod_ssl is configured to require authentication of clients, the X509 certificate that the client sends gets saved inside the SSL_SESSION object. To access this certificate, OpenSSL provides a function SSL_get_peer_certificate(). Mod_ssl uses this function in a number of places, at least once per connection. OpenSSL's memory management relies on reference counts; an object is not really freed before its reference counts becomes zero. The SSL_get_peer_certificate() manual expressly warns that: "The reference count of the X509 object is incremented by one, so that it will not be destroyed when the session containing the peer certificate is freed. The X509 object must be explicitly freed using X509_free()." However, mod_ssl does call SSL_get_peer_certificate() a number of times without later X509_free'ing its result. Because one such mistake happens at every connection (in ssl_hook_NewConnection()), peer certificates will never ever get freed. Not even if the enclosing SSL_SESSION object get freed. This in-memory certificate object can quite big, in my tests over 3K (over 5 times the size of the rest of the session object). In some circumstances, if Apache processes do not get killed often enough, this could lead to huge leaks in the order of megabytes *per Apache process*. In fact, researching this bug was started when one of our machines went down (swapping like mad) after as little as one minute of very heavy test load. The solution to this bug is to appropriately call X509_free every time the code gets the certificate object and is done with it. This is what the patch below does. Notes: ------ The following patch also includes changes that I wrote about a couple of days ago. They change free() calls to OPENSSL_free() where necessary in mod_ssl. A quick reminder: memory returned by OpenSSL functions like X509_NAME_oneline is allocated by OPENSSL_malloc, and should be freed with OPENSSL_free, not with free(). This caused me a lot of problems when trying to debug this memory leak (because OPENSSL_malloc and OPENSSL_free calls did not match up), so I think it would be good to clean this up once and for all. One note about reproducing this leak: By default, mod_ssl does not make any attempts to disable OpenSSL's internal session cache (we discussed this a bit on this list a few days ago), which is huge (20,000-sessions long) by default. In this case, the session object for the first 20,000 sessions in a certain Apache process are deliberately not freed, and obviously the peer certificate (if any) inside them aren't freed as well. Only when one lowers the size of this internal cache (with SSL_CTX_sess_set_cache_size()) or disables it completely (the upcoming SSL_SESS_CACHE_NO_INTERNAL option to SSL_CTX_set_session_cache_mode()), one notices how all the certificates never got freed anyway. With this patch, and with internal session cache disabled, Apache processes will not grow at all, not even after numerous client-authenticated requests. The patch itself: ----------------- diff -ur mod_ssl-2.8.11-1.3.27-dist/pkg.sslmod/ssl_engine_ext.c mod_ssl-2.8.11-1.3.27/pkg.sslmod/ssl_engine_ext.c --- mod_ssl-2.8.11-1.3.27-dist/pkg.sslmod/ssl_engine_ext.c 2002-03-27 18:47:58.000000000 +0200 +++ mod_ssl-2.8.11-1.3.27/pkg.sslmod/ssl_engine_ext.c 2002-10-25 17:15:22.000000000 ++0200 @@ -624,7 +624,7 @@ ssl_log(s, SSL_LOG_DEBUG, "SSL Proxy: (%s) no acceptable CA list, sending %s", servername, cp != NULL ? cp : "-unknown-"); - free(cp); + OPENSSL_free(cp); /* export structures to the caller */ *x509 = xi->x509; *pkey = xi->x_pkey->dec_pkey; @@ -643,7 +643,7 @@ cp = X509_NAME_oneline(X509_get_subject_name(xi->x509), NULL, 0); ssl_log(s, SSL_LOG_DEBUG, "SSL Proxy: (%s) sending %s", servername, cp != NULL ? cp : "-unknown-"); - free(cp); + OPENSSL_free(cp); /* export structures to the caller */ *x509 = xi->x509; *pkey = xi->x_pkey->dec_pkey; @@ -717,8 +717,8 @@ servername, peer != NULL ? peer : "-unknown-", errdepth, cp != NULL ? cp : "-unknown-", cp2 != NULL ? cp2 : "-unknown"); - free(cp); - free(cp2); + OPENSSL_free(cp); + OPENSSL_free(cp2); /* * If we already know it's not ok, log the real reason diff -ur mod_ssl-2.8.11-1.3.27-dist/pkg.sslmod/ssl_engine_kernel.c mod_ssl-2.8.11-1.3.27/pkg.sslmod/ssl_engine_kernel.c --- mod_ssl-2.8.11-1.3.27-dist/pkg.sslmod/ssl_engine_kernel.c 2002-10-04 16:30:37.000000000 +0300 +++ mod_ssl-2.8.11-1.3.27/pkg.sslmod/ssl_engine_kernel.c 2002-10-25 +17:33:14.000000000 +0200 @@ -386,7 +386,8 @@ if ((xs = SSL_get_peer_certificate(ssl)) != NULL) { cp = X509_NAME_oneline(X509_get_subject_name(xs), NULL, 0); ap_ctx_set(fb->ctx, "ssl::client::dn", ap_pstrdup(conn->pool, cp)); - free(cp); + OPENSSL_free(cp); + X509_free(xs); } /* @@ -865,11 +866,12 @@ /* optimization */ if ( dc->nOptions & SSL_OPT_OPTRENEGOTIATE && nVerifyOld == SSL_VERIFY_NONE - && SSL_get_peer_certificate(ssl) != NULL) + && (cert = SSL_get_peer_certificate(ssl)) != NULL) renegotiate_quick = TRUE; ssl_log(r->server, SSL_LOG_TRACE, "Changed client verification type will force %srenegotiation", renegotiate_quick ? "quick " : ""); + X509_free(cert); } } } @@ -1029,7 +1031,8 @@ cp = X509_NAME_oneline(X509_get_subject_name(cert), NULL, 0); ap_ctx_set(r->connection->client->ctx, "ssl::client::dn", ap_pstrdup(r->connection->pool, cp)); - free(cp); + OPENSSL_free(cp); + X509_free(cert); } /* @@ -1042,12 +1045,15 @@ "Re-negotiation handshake failed: Client verification failed"); return FORBIDDEN; } + cert = NULL; if ( dc->nVerifyClient == SSL_CVERIFY_REQUIRE - && SSL_get_peer_certificate(ssl) == NULL ) { + && (cert = SSL_get_peer_certificate(ssl)) == NULL ) { ssl_log(r->server, SSL_LOG_ERROR, "Re-negotiation handshake failed: Client certificate missing"); return FORBIDDEN; } + if (cert != NULL) + X509_free(cert); } } @@ -1498,9 +1504,9 @@ errdepth, cp != NULL ? cp : "-unknown-", cp2 != NULL ? cp2 : "-unknown"); if (cp) - free(cp); + OPENSSL_free(cp); if (cp2) - free(cp2); + OPENSSL_free(cp2); /* * Check for optionally acceptable non-verifiable issuer situation @@ -1655,7 +1661,7 @@ BIO_free(bio); cp2 = X509_NAME_oneline(subject, NULL, 0); ssl_log(s, SSL_LOG_TRACE, "CA CRL: Issuer: %s, %s", cp2, cp); - free(cp2); + OPENSSL_free(cp2); free(cp); } @@ -1719,7 +1725,7 @@ "Certificate with serial %ld (0x%lX) " "revoked per CRL from issuer %s", serial, serial, cp); - free(cp); + OPENSSL_free(cp); X509_STORE_CTX_set_error(ctx, X509_V_ERR_CERT_REVOKED); X509_OBJECT_free_contents(&obj); diff -ur mod_ssl-2.8.11-1.3.27-dist/pkg.sslmod/ssl_engine_vars.c mod_ssl-2.8.11-1.3.27/pkg.sslmod/ssl_engine_vars.c --- mod_ssl-2.8.11-1.3.27-dist/pkg.sslmod/ssl_engine_vars.c 2002-06-29 10:42:45.000000000 +0300 +++ mod_ssl-2.8.11-1.3.27/pkg.sslmod/ssl_engine_vars.c 2002-10-25 17:33:40.000000000 ++0200 @@ -314,8 +314,10 @@ result = ssl_var_lookup_ssl_cert_verify(p, c); } else if (ssl != NULL && strlen(var) > 7 && strcEQn(var, "CLIENT_", 7)) { - if ((xs = SSL_get_peer_certificate(ssl)) != NULL) + if ((xs = SSL_get_peer_certificate(ssl)) != NULL) { result = ssl_var_lookup_ssl_cert(p, xs, var+7); + X509_free(xs); + } } else if (ssl != NULL && strlen(var) > 7 && strcEQn(var, "SERVER_", 7)) { if ((xs = SSL_get_certificate(ssl)) != NULL) @@ -352,7 +354,7 @@ xsname = X509_get_subject_name(xs); cp = X509_NAME_oneline(xsname, NULL, 0); result = ap_pstrdup(p, cp); - free(cp); + OPENSSL_free(cp); resdup = FALSE; } else if (strlen(var) > 5 && strcEQn(var, "S_DN_", 5)) { @@ -364,7 +366,7 @@ xsname = X509_get_issuer_name(xs); cp = X509_NAME_oneline(xsname, NULL, 0); result = ap_pstrdup(p, cp); - free(cp); + OPENSSL_free(cp); resdup = FALSE; } else if (strlen(var) > 5 && strcEQn(var, "I_DN_", 5)) { @@ -543,6 +545,8 @@ else /* client verification failed */ result = ap_psprintf(p, "FAILED:%s", verr); + if(xs != NULL) + X509_free(xs); return result; } -- Nadav Har'El | Friday, Oct 25 2002, 20 Heshvan 5763 [EMAIL PROTECTED] |----------------------------------------- Phone: +972-53-245868, ICQ 13349191 |In Fortran, God is real unless declared http://nadav.harel.org.il |an integer. ______________________________________________________________________ Apache Interface to OpenSSL (mod_ssl) www.modssl.org User Support Mailing List [EMAIL PROTECTED] Automated List Manager [EMAIL PROTECTED]