nginx memory leak in ngx_event_openssl.c with patch

[Nikolay Morozov]

on some error cases X509 certificate is leaking. I have attached the patch to correct this

[Sergey Kandaurov]

The patch looks good.
Please take a look at ​http://nginx.org/en/docs/contributing_changes.html.
It'd be best if you could use Mercurial to submit patches.

[Maxim Dounin]

Could you please clarify - are you seeing these leaks in practice, or you've found this by reading the code? It looks like these errors shouldn't happen in practice, as both X509_get_issuer_name() and X509_get_subject_name() will always return valid pointers.

[Nikolay Morozov]

In searching the memory leaks in production. There is a patched nginx with many external modules. I just found them analyzing code.

[Nikolay Morozov]

Both X509_get_issuer_name() and X509_get_subject_name() will always return valid pointers only if client certificate is ok, but there can be a defective certificate.
I wiil use mercurial to submit changes

[Maxim Dounin]

Both X509_get_issuer_name() and X509_get_subject_name() will always return valid pointers only if client certificate is ok, but there can be a defective certificate.

Sure, but these fields are mandatory, and handshake is expected to fail if such a defective certificate is provided by the client. As such, I suspect the issue is purely theoretical, and cannot lead to real memory leaks. (It needs to be fixed anyway, of course, but the question is about possible impact.)

[Nikolay Morozov]

ok, I have alreday submited the chanes to ngix-devel maillist with hg email

[Nikolay Morozov <n.morozov@…>]

In 7484:65074e13f171/nginx:

SSL: missing free calls in $ssl_client_s_dn and $ssl_client_i_dn.

If X509_get_issuer_name() or X509_get_subject_name() returned NULL,
this could lead to a certificate reference leak. It cannot happen
in practice though, since each function returns an internal pointer
to a mandatory subfield of the certificate successfully decoded by
d2i_X509() during certificate message processing (closes #1751).