Date: Fri, 16 Jul 2004 11:41:14 +0200
From: Ralf S. Engelschall <rse@engelschall.com>
To: packet@packetstormsecurity.org
Cc: eay@cryptsoft.com, tjh@cryptsoft.com, virulent@siyahsapka.org,
    vuln@security.nnov.ru, vendor-sec@lst.de
Subject: [vendor-sec] Re: mod_ssl-2.8.18-1.3.31 Format string vulnerability

On Fri, Jul 16, 2004, packet@packetstormsecurity.org wrote:

> Thought you might like to look at this if you haven't seen it already.
> Any feedback on it is appreciated.
>
> -Packet Storm
>
> ----- Forwarded message from Virulent <virulent@siyahsapka.org> -----
> Delivered-To: packet@packetstormsecurity.org
> Delivered-To: submissions@packetstormsecurity.org
> From: "Virulent" <virulent@siyahsapka.org>
> To: <submissions@packetstormsecurity.org>
> Cc: <vuln@security.nnov.ru>
> Subject: mod_ssl-2.8.18-1.3.31 Format string vulnerability
> Date: Sun, 4 Jul 2004 16:18:55 +0300
> [...]
> The mod_ssl project provides strong cryptography for the Apache 1.3
> webserver via the Secure Sockets Layer (SSL v2/v3) and Transport Layer
> Security (TLS v1) protocols by the help of the Open Source SSL/TLS toolkit
> OpenSSL, which is based on SSLeay from Eric A. Young and Tim J. Hudson
> (http://www.modssl.org)
> [...]
> mod_ssl-2.8.18-1.3.31
> [...]
> Format string vulnerability.
> [...]
> +-----[ Vulnerable Code ]-----+
> [ssl_engine_log.c]
>
> void ssl_log(server_rec *s, int level, const char *msg, ...)
> {
> ......
>     /*  create custom message  */
>     ap_vsnprintf(vstr, sizeof(vstr), msg, ap);
> ......
> }
> [...]
> +-----[ Contact ]-----+
> http://virulent.siyahsapka.org
>    virulent@siyahsapka.org
> [...]
> ----- End forwarded message -----

Hmmm... the above code of ssl_log() itself is correct and not a
security bug: ssl_log() is a printf() style function where "msg" is the
format string and "ap" the va_list argument list (ok, perhaps "msg"
is the wrong name for this variable, here ;-). But a format string
vulnerability exists if ssl_log() is called by other code as "ssl_log(s,
l, msg);" instead of e.g. "ssl_log(s, l, "%s", msg);" where "msg" is a
user supplied string buffer. But this is a different aspect -- the above
ap_vsnprintf() call itself is 100% correct and AFAIK actually cannot be
made any different.

Nevertheless this issue forced me to again review all existing calls(!)
to ssl_log() in mod_ssl for any incorrect usage. And unfortunately I
had to discover a remaining call which is risky and under particular
circumstances could lead to a a vulnerability:

Index: ssl_engine_ext.c
===================================================================
RCS file: /e/modssl/cvs/mod_ssl/pkg.apache/src/modules/ssl/ssl_engine_ext.c,v
retrieving revision 1.50
diff -u -d -r1.50 ssl_engine_ext.c
--- ssl_engine_ext.c    11 May 2004 18:44:15 -0000  1.50
+++ ssl_engine_ext.c    16 Jul 2004 07:57:33 -0000
@@ -524,7 +524,7 @@
 #endif
         errmsg = ap_psprintf(r->pool, "SSL proxy connect failed (%s): peer %s:
%s",
                              cpVHostID, peer,
ERR_reason_error_string(ERR_get_error()));
-        ssl_log(r->server, SSL_LOG_ERROR, errmsg);
+        ssl_log(r->server, SSL_LOG_ERROR, "%s", errmsg);
         SSL_free(ssl);
         ap_ctx_set(fb->ctx, "ssl", NULL);
         return errmsg;

Here if "errmsg" could contain e.g. a "%s", a format string
vulnerability exists in mod_ssl. "cpVHostID" and the OpenSSL error
string are harmless IMHO, but "peer" comes from mod_proxy which
itself derives it from the URL the client gave it. So, as far as
I see it, in case Apache is driven as a proxy, and a HTTPS URL
https://foo%s.example.com/" is given, and in the example.com zone
there is a "foo%s" hostname (don't know whether such illegal hostnames
are really accepted and passed-through by resolver libraries), and a
connection to this host foo%s.example.com can be actually made, THEN
garbage is read by ap_vsnprintf() from the stack and the server could
crash, etc.

I've fixed mod_ssl with the above patch now and released mod_ssl 2.8.19
for Apache 1.3.31 on modssl.org. In mod_ssl for Apache 2.x this piece of
code does not exist AFAIK, so I've not fixed anything there.

So, although the original vulnerability report was incorrect, it at
least pushed the search (and fix) of a security bug in other code.
Thanks to Virulent <virulent@siyahsapka.org> for triggering this.