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

On 10/05/2013 4:49 p.m., Amos Jeffries wrote:
> On 10/05/2013 9:01 a.m., Alex Rousskov wrote:
>> On 05/09/2013 01:10 PM, Tsantilas Christos wrote:
>>> On 05/09/2013 05:50 PM, Alex Rousskov wrote:
>>>> I wonder if we should change strategy from compile-time OpenSSL
>>>> version
>>>> checks to something like this:
>>>>
>>>> // Temporary ssl for getting X509 certificate from SSL_CTX.
>>>> Ssl::SSL_Pointer ssl(SSL_new(sslContext));
>>>> // Various OpenSSL versions crash on a SSL_get_certificate() call:
>>>> // http://bugs.squid-cache.org/show_bug.cgi?id=3816#c16
>>>> // so we avoid that call by extracting certificate directly:
>>>> X509 *cert = ssl->cert ? ssl->cert->key->x509 : NULL;
>>>> if (!cert)
>>>> return false;
>>> The ssl->cert is of type CERT ("struct cert_st") and it is defined in a
>>> header file which is not available to the public openSSL SDK...
>>> So the above (ssl->cert->key->x509) can not be used unless we copy the
>>> "struct cert_st" definition inside squid...
>> Understood. My suggestion above is not a good solution then.
>>
>>
>>> 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.
>>
>> 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?
>
> We are slightly constrained by autoconf test harness here. To get the
> desired result we will have to run two tests, first the check to see
> if its necessary, then the check to see if the workaround is possible.
>
> Something like this (but without the bugs):
>
> AC_DEFUN([SQUID_CHECK_OPENSSL_GETCERTIFICATE_WORKS],[
> AH_TEMPLATE(SQUID_USE_SSLGETCERTIFICATE_HACK)
> AC_RUN_IFELSE([
> SSLeay_add_ssl_algorithms();
> SSL_CTX *sslContext = SSL_CTX_new(SSLv3_method());
> SSL *ssl = SSL_new(sslContext);
> X509 * cert = SSL_get_certificate(ssl);
> ],[],[
> ... check the workaround will compile. Enable if it does.
> ],[])
> ])
>
> This will need to go into acinclude/lib-checks.m4, which also includes
> several other examples you can see for ideas.
>
> HTH
> Amos

PS. I should say, I'm happy to do all the autoconf integration if you
like. I just require somebody with a better knowledge of the bugs and
workarounds to write the trivial code that forms the meat of the two
test macros.

Amos
Received on Fri May 10 2013 - 04:54:11 MDT