I am attaching a fix.
Still needs some discussion.
This patch does the following two checks:
1) Checks if the SSL_get_certificate is buggy
2) Checks it he workaround can be enabled.
Inside squid:
If the workaround can be used, enable it
else if the SSL_get_certificate is not buggy, use it
else hit an assertion
I select this approach:
1) because the workaround is significant faster than using the
SSL_get_certificate
2) to avoid the segfault if the SSL_get_certificate is buggy .
Problems:
I had problem with the LD_LIBRARY_PATH. For example if the user does
not want to use system libraries and use openSSL SDK installed under a
non standard directory, the test program will run with system libraries.
To avoid this someone should use the LD_LIBRARY_PATH in configure script:
./configure --with-openssl=/path/to/openssl/
LD_LIBRARY_PATH=/path/to/openssl/
I do not like this option, so in the test I am using the -wl,-rpath
compiler option to pass the openSSL libraries path.
But this option does not looks good too..
Also we may want to harden the workaround test to use a hardcoded
certificate instead of a NULL certificate. (I attached an example in a
previous mail)
Regards,
Christos
On 05/10/2013 06:18 PM, Alex Rousskov wrote:
> On 05/10/2013 09:03 AM, Tsantilas Christos wrote:
>> I am attaching a small utility which does all possible checks we can do.
>>
>>
>> On 05/10/2013 12:01 AM, Alex Rousskov wrote:
>>> On 05/09/2013 01:10 PM, Tsantilas Christos wrote:
>>>> On 05/09/2013 05:50 PM, Alex Rousskov wrote:
>>>> I can reporoduce the bug with the following simple program:
>>>>
>>>> int main(int argc, char *argv[])
>>>> {
>>>> SSLeay_add_ssl_algorithms();
>>>> SSL_CTX *sslContext = SSL_CTX_new(SSLv3_method());
>>>> SSL *ssl = SSL_new(sslContext);
>>>> X509 * cert = SSL_get_certificate(ssl);
>>>> return 0;
>>>> }
>>>>
>>>> This program crashes inside a function called by SSL_get_certificate.
>>>> I suppose we can check if the program exited normally or not, to decide
>>>> if the openSSL SDK is OK or have the bug.
>>>
>>> That sounds like a good idea to me. I would extend that test code to
>>> also include code to test that our workaround compiles. Here is a sketch:
>>>
>>> int main() {
>>> ...
>>> X509 *cert1 = workaroundCode(...);
>>> X509 *cert2 = directCode(...); // may crash!
>>> return cert1 == cert2 ? 0 : 1;
>>> }
>>>
>>> I assume that both cert1 and cert2 will be nil in your test case when
>>> everything works correctly, but that does not matter.
>>
>> Well checking with null certificates maybe is not enough good to detect
>> problems. The SSL and SSL_CTX structures used here mostly uninitialized
>> so it is possible to get a wrong field which is just zeroed.
>>
>> But maybe we can add in the check program a hard coded certificate. I am
>> using it in the check toll I am attaching.
>> Then to check if the workaround code works required the following:
>> const char *certTxt = "-----BEGIN CERTIFICATE----\n" .....;
>>
>> SSLeay_add_ssl_algorithms();
>> BIO *certBio = BIO_new(BIO_s_mem());
>> BIO_puts(certBio, certTxt);
>> X509 * cert = NULL;
>> PEM_read_bio_X509(certBio, &cert, 0, 0));
>> SSL_CTX *sslContext = SSL_CTX_new(SSLv3_method());
>> SSL_CTX_use_certificate(sslContext, cert));
>> X509 ***pCert = (X509 ***)sslContext->cert;
>> X509 *sslCtxCert = pCert && *pCert ? **pCert : NULL;
>> return (sslCtxCert != cert ? 1 : 0);
>>
>>
>>>
>>> Then, we can use one of the following two strategies:
>>>
>>> Conservative (work around the bug if possible and clearly necessary):
>>> If the test compiles but crashes, enable workaround.
>>> Otherwise, disable workaround.
>>>
>>> Midway (work around the bug if necessary or if work around works):
>>> If the test compiles but crashes, enable workaround.
>>> If the test compiles and returns 0, enable workaround.
>>> Otherwise, disable workaround.
>>>
>>> Aggressive (work around the bug if possible):
>>> If the test compiles, enable workaround.
>>> Otherwise, disable workaround.
>>>
>>>
>>> I think we should use either the Midway or the Aggressive approach to
>>> accommodate more cases, but I am not sure. What do you think?
>>
>> I think the best is the aggressive with some additions:
>>
>> - Check if the workaround work, using a code similar to the above.
>> - If not work try to use SSL object and SSL_get_certificate function. If
>> this one crashes, disable SSL bumping (only here needed).
>
> I am worried that we are over-engineering this, which will result in
> test failures for reasons other than OpenSSL bugs. However, if you
> prefer to add real certificate check, I am not objecting to it. If we do
> run into problems with this more complex check, we can backtrack and
> simplify it. Just do whatever you think is best.
>
>
> Thank you,
>
> Alex.
>
>
=== added file 'acinclude/ssl_get_certificate.m4'
--- acinclude/ssl_get_certificate.m4 1970-01-01 00:00:00 +0000
+++ acinclude/ssl_get_certificate.m4 2013-05-13 17:46:04 +0000
@@ -0,0 +1,92 @@
+dnl
+dnl AUTHOR: Squid Web Cache team
+dnl
+dnl SQUID Web Proxy Cache http://www.squid-cache.org/
+dnl ----------------------------------------------------------
+dnl Squid is the result of efforts by numerous individuals from
+dnl the Internet community; see the CONTRIBUTORS file for full
+dnl details. Many organizations have provided support for Squid's
+dnl development; see the SPONSORS file for full details. Squid is
+dnl Copyrighted (C) 2001 by the Regents of the University of
+dnl California; see the COPYRIGHT file for full details. Squid
+dnl incorporates software developed and/or copyrighted by other
+dnl sources; see the CREDITS file for full details.
+dnl
+dnl This program is free software; you can redistribute it and/or modify
+dnl it under the terms of the GNU General Public License as published by
+dnl the Free Software Foundation; either version 2 of the License, or
+dnl (at your option) any later version.
+dnl
+dnl This program is distributed in the hope that it will be useful,
+dnl but WITHOUT ANY WARRANTY; without even the implied warranty of
+dnl MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+dnl GNU General Public License for more details.
+dnl
+dnl You should have received a copy of the GNU General Public License
+dnl along with this program; if not, write to the Free Software
+dnl Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA 02111, USA.
+
+dnl Checks whether the OpenSSL SSL_get_certificate crashes squid and if a
+dnl workaround can be used instead of using the SSL_get_certificate
+
+AC_DEFUN([SQUID_CHECK_OPENSSL_GETCERTIFICATE_WORKS],[
+ AH_TEMPLATE(SQUID_SSLGETCERTIFICATE_BUGGY, "Define to 1 if the SSL_get_certificate crashes squid")
+ AH_TEMPLATE(SQUID_USE_SSLGETCERTIFICATE_HACK, "Define to 1 to use squid workaround for SSL_get_certificate")
+ OLDLIBS="$LIBS"
+ LIBS="$LIBS $SSLLIB"
+ if test "x$SSLLIBDIR" != "x"; then
+ LIBS="$LIBS -Wl,-rpath -Wl,$SSLLIBDIR"
+ fi
+
+ AC_MSG_CHECKING(whether the SSL_get_certificate is buggy)
+ AC_RUN_IFELSE([
+ AC_LANG_PROGRAM(
+ [
+ #include <openssl/ssl.h>
+ #include <openssl/err.h>
+ ],
+ [
+ SSLeay_add_ssl_algorithms();
+ SSL_CTX *sslContext = SSL_CTX_new(SSLv3_method());
+ SSL *ssl = SSL_new(sslContext);
+ X509* cert = SSL_get_certificate(ssl);
+ return 0;
+ ])
+ ],
+ [
+ AC_MSG_RESULT([no])
+ ],
+ [
+ AC_DEFINE(SQUID_SSLGETCERTIFICATE_BUGGY, 1)
+ AC_MSG_RESULT([yes])
+ ],
+ [])
+
+ AC_MSG_CHECKING(whether the workaround for SSL_get_certificate works)
+ AC_RUN_IFELSE([
+ AC_LANG_PROGRAM(
+ [
+ #include <openssl/ssl.h>
+ #include <openssl/err.h>
+ ],
+ [
+ SSLeay_add_ssl_algorithms();
+ SSL_CTX *sslContext = SSL_CTX_new(SSLv3_method());
+ X509 ***pCert = (X509 ***)sslContext->cert;
+ X509 *sslCtxCert = pCert && *pCert ? **pCert : (X509 *)0x1;
+ if (sslCtxCert != NULL)
+ return 1;
+ return 0;
+ ])
+ ],
+ [
+ AC_MSG_RESULT([yes])
+ AC_DEFINE(SQUID_USE_SSLGETCERTIFICATE_HACK, 1)
+ ],
+ [
+ AC_MSG_RESULT([no])
+ ],
+[])
+
+LIBS=$OLDLIBS
+])
=== modified file 'configure.ac'
--- configure.ac 2013-01-22 06:29:59 +0000
+++ configure.ac 2013-05-13 15:50:41 +0000
@@ -1,40 +1,41 @@
AC_INIT([Squid Web Proxy],[3.HEAD-BZR],[http://bugs.squid-cache.org/],[squid])
AC_PREREQ(2.61)
AC_CONFIG_HEADERS([include/autoconf.h])
AC_CONFIG_AUX_DIR(cfgaux)
AC_CONFIG_SRCDIR([src/main.cc])
AM_INIT_AUTOMAKE([tar-ustar nostdinc])
AC_REVISION($Revision$)dnl
AC_PREFIX_DEFAULT(/usr/local/squid)
AM_MAINTAINER_MODE
m4_include([acinclude/init.m4])
m4_include([acinclude/squid-util.m4])
m4_include([acinclude/compiler-flags.m4])
m4_include([acinclude/os-deps.m4])
m4_include([acinclude/krb5.m4])
m4_include([acinclude/pam.m4])
m4_include([acinclude/pkg.m4])
m4_include([acinclude/lib-checks.m4])
m4_include([acinclude/ax_cxx_compile_stdcxx_0x.m4])
m4_include([acinclude/ax_cxx_0x_types.m4])
+m4_include([acinclude/ssl_get_certificate.m4])
PRESET_CFLAGS="$CFLAGS"
PRESET_CXXFLAGS="$CXXFLAGS"
PRESET_LDFLAGS="$LDFLAGS"
dnl Set default LDFLAGS
if test "x$LDFLAGS" = "x" ; then
LDFLAGS="-g"
fi
# Check for GNU cc
AC_PROG_CC
AM_PROG_CC_C_O
AC_PROG_CXX
AC_LANG([C++])
AC_CANONICAL_HOST
# might be cross-compiling
if test "x$HOSTCXX" = "x"; then
HOSTCXX="$CXX"
@@ -1238,40 +1239,43 @@
[Define this to include code for SSL gatewaying support])
AC_MSG_NOTICE([Using OpenSSL MD5 implementation: ${with_openssl:=no}])
SQUID_DEFINE_BOOL(USE_OPENSSL,${with_openssl},
[Define this to make use of the OpenSSL libraries for MD5 calculation rather than Squid-supplied MD5 implementation or if building with SSL encryption])
if test "x$enable_ssl" = "xyes"; then
if test "x$SSLLIB" = "x"; then
SSLLIB="-lcrypto" # for MD5 routines
fi
# This is a workaround for RedHat 9 brain damage..
if test -d /usr/kerberos/include -a "x$SSLLIBDIR" = "x" -a -f /usr/include/openssl/kssl.h; then
AC_MSG_NOTICE([OpenSSL depends on Kerberos])
SSLLIBDIR="/usr/kerberos/lib"
CPPFLAGS="$CPPFLAGS -I/usr/kerberos/include"
fi
fi
if test "x$SSLLIBDIR" != "x" ; then
SSLLIB="-L$SSLLIBDIR $SSLLIB"
fi
AC_SUBST(SSLLIB)
+if test "x$with_openssl" = "xyes"; then
+SQUID_CHECK_OPENSSL_GETCERTIFICATE_WORKS
+fi
AC_ARG_ENABLE(forw-via-db,
AS_HELP_STRING([--enable-forw-via-db],[Enable Forw/Via database]), [
SQUID_YESNO([$enableval],[unrecognized argument to --enable-forw-via-db: $enableval])
])
SQUID_DEFINE_BOOL(USE_FORW_VIA_DB,${enable_forw_via_db:=no},
[Enable Forw/Via database])
AC_MSG_NOTICE([Forw/Via database enabled: $enable_forw_via_db])
AC_ARG_ENABLE(cache-digests,
AS_HELP_STRING([--enable-cache-digests],
[Use Cache Digests. See http://wiki.squid-cache.org/SquidFaq/CacheDigests]),
[
SQUID_YESNO($enableval,
[unrecognized argument to --enable-cache-digests: $enableval])
])
SQUID_DEFINE_BOOL(USE_CACHE_DIGESTS,${enable_cache_digests:=no},
[Use Cache Digests for locating objects in neighbor caches.])
AC_MSG_NOTICE([Cache Digests enabled: $enable_cache_digests])
=== modified file 'src/ssl/support.cc'
--- src/ssl/support.cc 2013-01-30 15:39:37 +0000
+++ src/ssl/support.cc 2013-05-13 18:01:30 +0000
@@ -1433,43 +1433,55 @@
SSL_CTX *
Ssl::generateSslContext(CertificateProperties const &properties, AnyP::PortCfg &port)
{
Ssl::X509_Pointer cert;
Ssl::EVP_PKEY_Pointer pkey;
if (!generateSslCertificate(cert, pkey, properties))
return NULL;
if (!cert)
return NULL;
if (!pkey)
return NULL;
return createSSLContext(cert, pkey, port);
}
bool Ssl::verifySslCertificate(SSL_CTX * sslContext, CertificateProperties const &properties)
{
+ // SSL_get_certificate is buggy in openssl versions 1.0.1d and 1.0.1e
+ // Try to retrieve certificate directly from SSL_CTX object
+#if defined(SQUID_USE_SSLGETCERTIFICATE_HACK)
+ X509 ***pCert = (X509 ***)sslContext->cert;
+ X509 * cert = pCert && *pCert ? **pCert : NULL;
+#elif defined(SQUID_SSLGETCERTIFICATE_BUGGY)
+ X509 * cert = NULL;
+ assert(0);
+#else
// Temporary ssl for getting X509 certificate from SSL_CTX.
Ssl::SSL_Pointer ssl(SSL_new(sslContext));
X509 * cert = SSL_get_certificate(ssl.get());
+#endif
+ if (!cert)
+ return false;
ASN1_TIME * time_notBefore = X509_get_notBefore(cert);
ASN1_TIME * time_notAfter = X509_get_notAfter(cert);
bool ret = (X509_cmp_current_time(time_notBefore) < 0 && X509_cmp_current_time(time_notAfter) > 0);
if (!ret)
return false;
return certificateMatchesProperties(cert, properties);
}
bool
Ssl::setClientSNI(SSL *ssl, const char *fqdn)
{
//The SSL_CTRL_SET_TLSEXT_HOSTNAME is a openssl macro which indicates
// if the TLS servername extension (SNI) is enabled in openssl library.
#if defined(SSL_CTRL_SET_TLSEXT_HOSTNAME)
if (!SSL_set_tlsext_host_name(ssl, fqdn)) {
const int ssl_error = ERR_get_error();
debugs(83, 3, "WARNING: unable to set TLS servername extension (SNI): " <<
ERR_error_string(ssl_error, NULL) << "\n");
return false;
