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

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