Re: A new workaround for bug 3816: ssl_crtd crash with OpenSSL v...

On 05/14/2013 12:52 PM, Amos Jeffries wrote:
> On 14/05/2013 9:11 p.m., Tsantilas Christos wrote:
>> On 05/14/2013 06:55 AM, Amos Jeffries wrote:
>>> On 14/05/2013 6:28 a.m., Tsantilas Christos wrote:
>>>> 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
>>> Looks like good progress.
>>>
>>> Have you tried moving the m4_include statement after AC_SUBST(SSLLIB)?
>>> The the m4_include will expand the file in-place inside configure.ac.
>>>
>>> Have you tried passing the flags as an argument to the check macro? eg.
>>> SQUID_CHECK_OPENSSL_GETCERTIFICATE_WORKS([$SSLLIB])
>> The $SSLLIB variable can be used without problem in the check. This is
>> not a problem. The problem is different. Imagine the following scenario:
>>
>> 1) The user has installed in your system a buggy version of openSSL. The
>> configure script finds the SSL_get_certificate buggy
>> 2) Installs a new version under the /opt/openSSL/
>> 3) Runs again the configure script:
>> ./configure --with-openssl=/opt/openSSL/
>> The check will be compiled using:
>> g++ -o conftest test.cc -I/opt/openSSL/include -L/opt/openSSL/lib
>> -lssl
>> But it will run as
>> ./conftest
>> This is means that the conftest will use the system ssl libraries
>> installed under the "/usr/lib/". The test for SSL_get_certificate will
>> fail again in this case.
>
> And Squid built with the same options will also encounter this identical
> library problem when its run as "/usr/bin/squid" or whatever.

Yes.
However squid binary builds with the correct options.

>
>> To avoid this problem I used in this patch the -rpath:
>> g++ -o conftest test.cc -I/opt/openSSL/include -L/opt/openSSL/lib
>> -lssl -Wl,-rpath -Wl,/opt/openSSL/lib
>>
>> Is this acceptable? Maybe the -rpath is not available in all systems (it
>> is a linker parameter, not a g++ parameter)...
>
> I would go with -W,-rpath=/opt/openSSL/lib so that nothing like libtool
> can play games with the ordering. Some versions try to be overly-smart.

OK, I will keep it.

>
> We also need to ensure that the main squid binaries are also built with
> the same extra options when --with-openssl is given parameters. So a
> SSLFLAGS global may be needed creating.

With compiler flags we do not have such problems. We are not using an
SSLFLAGS but we are updating the CPPFLAGS directly, so we are sure that
the include dir is the correct one.
The CPPFLAGS used in generated makefiles too so the squid binaries build
correctly.

Currently, after squid build, the system admin have to update the ld.so
configuration or set LD_LIBRARY_PATH to load the correct openSSL libraries.

This is also for Kerveros, LDAP libraries, XML libraries, and expat
libraries. Probably this is requires a separate patch fixing all of these...

We can just add comment after configure script finishes to inform user
that he have to update the ld.so configuration too....

>
>> An alternate maybe is to set the LD_RUN_PATH or LD_LIBRARY_PATH
>> environment variable before run the check and unset it after the test
>> finishes:
>>
>> OLD_LD_RUN_PATH="$LD_RUN_PATH"
>> if test "x$SSLLIBDIR" != "x"; then
>> LD_RUN_PATH="$LD_RUN_PATH:$SSLLIBDIR"
>> fi
>> SQUID_CHECK_OPENSSL_GETCERTIFICATE_WORKS
>> LD_RUN_PATH="$OLD_LD_RUN_PATH"
>>
>> But still we may have problems. Are the LD_RUN_PATH or LD_LIBRARY_PATH
>> environment variables available in all systems?
>
> Probably not, and the issue existing for bin/squid still needs it later
> anyway.
>
> It is time I suppose to ask, are you wanting to fix all of these known
> OpenSSL hack bugs in one update, or do them separately?
> If you want do them separately then you have IMHO fixed the bug 3816
> issue already, this path problem is a second bug todo, and bug 3759 is
> still waiting a fix.
>
> NP: bug 3759 is hitting the entire RHEL hierarchy of distros now.
> http://build.squid-cache.org/job/3.HEAD-amd64-centos-6/lastFailedBuild/console,
> http://bugs.squid-cache.org/show_bug.cgi?id=3759

Aggr... The bug3759 it is related to bug3232 which force us to build a
patch which used the new openSSL interface for TXT_DB api and a
workaround because the new interface was buggy.
But I am not sure that just checking if new API exist or not is enough.

I will provide a fix for this on a separate patch.
Is there any know Fedore/redhat openSSL package which has these problem?
I need to look on their patch before fix this problem...

Other checks we are doing using OpenSSL version are:
1) Use "SSL_METHOD *" or "const SSL_METHOD *" with SSL*_method()
functions (0.1.x requires const)
2) check if TLSv1_1_client_method() and TLSv1_2_client_method() exists.
These functions added on openSSL-1.0.1 release.

We may want to add them too in configure script.

>
> Amos
>
Received on Tue May 14 2013 - 11:14:13 MDT