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

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.

> 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.

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.

> 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

Amos
Received on Tue May 14 2013 - 09:53:06 MDT