Bug fix: leak of peer certificate
am 26.10.2002 00:03:44 von nyhThere 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
nyh@math.technion.ac.il |-----------------------------------------
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 modssl-users@modssl.org
Automated List Manager majordomo@modssl.org