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

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?

If it is easy, we could also allow folks to define the corresponding
decision macro manually, when building Squid, so that they can overwrite
our automation when needed (e.g., when compiling Squid for use with a
different OpenSSL version). I do not think we need to make that choice
an explicit ./configure option though.

Thank you,

Alex.
Received on Thu May 09 2013 - 21:01:35 MDT