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

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
Received on Fri May 10 2013 - 04:49:57 MDT