Le 15/10/2015 16:55, Willy Tarreau a écrit :
Hi Christopher,
On Thu, Oct 15, 2015 at 03:22:52PM +0200, Christopher Faulet wrote:
Le 15/10/2015 14:45, Seri, Kim a écrit :
Christopher Faulet <cfaulet@...> writes:
I confirm the bug. Here is a very quick patch. Could you confirm that it
works for you ?
Hi,
I can confirm this patch fixes the crash!!
cf. because of my mail service, I've changed my e-mail
Thanks a lot.
Great!
Willy, is it ok to you if I add the CO_FL_DYN_SSL_CTX flag to track
connections with a generated SSL certificate or do you prefer I find
another way to fix the bug ?
I'm still having doubts on the fix, because I feel like we're working
around a design issue here. First, the problem is that it's unclear
to me in which condition we may end up calling this code. How can it
happen that we end up in this code with an empty LRU tree ? Can we
generate cookies without a cert cache ? Or can the cert cache be empty
with some certs still in use ? If the later, maybe instead we should
keep a reference to the cache using the refcount so that we don't kill
the entry as long as it's being used.
Indeed, this is mostly a matter of being sure that we free an ssl_ctx
that was allocated, so there should be other ways to do it than adding
more SSL knowledge into the session. I'm not opposed to merging this
fix as a quick one to fix the trouble for the affected users, but I'd
prefer that we find a cleaner solution if possible.
Hi,
First the LRU tree is only initialized when the SSL certs generation is
configured on a bind line. So, in the most of cases, it is NULL (it is
not the same thing than empty).
When the SSL certs generation is used, if the cache is not NULL, a such
certificate is pushed in the cache and there is no need to release it
when the connection is closed.
But it can be disabled in the configuration. So in that case, we must
free the generated certificate when the connection is closed.
Then here, we have really a bug. Here is the buggy part:
3125) if (conn->xprt_ctx) {
3126) #ifdef SSL_CTRL_SET_TLSEXT_HOSTNAME
3127) if (!ssl_ctx_lru_tree && objt_listener(conn->target)) {
3128) SSL_CTX *ctx = SSL_get_SSL_CTX(conn->xprt_ctx);
3129) if (ctx != 3130)
SSL_CTX_free(ctx);
3131) }
#endif
3133) SSL_free(conn->xprt_ctx);
3134) conn->xprt_ctx = NULL;
3135) sslconns--;
3136) }
The check on the line 3127 is not enough to determine if this is a
generated certificate or not. Because ssl_ctx_lru_tree is NULL,
generated certificates, if any, must be freed. But here ctx should also
be compared to all SNI certificates and not only to default_ctx. Because
of this bug, when a SNI certificate is used for a connection, it is
erroneously freed when this connection is closed.
In my patch, I chosen to use a specific flag on the connection instead
of doing certificates comparisons. This seems to me easier to understand
and more efficient. But it could be discussed, there are many other
solutions I guess.
finally, we can of course discuss the design of this feature. There is
no problem. I will be happy to find a more elegant way to handle it, if
it is possible.
--
Christopher Faulet
