[openssl-dev] [openssl.org #3882] [BUGFIX] lh_SSL_SESSION_delete() not checked

[Short, Todd via RT]

Depending on how the comparison function was implemented, the insert could still succeed at the point mentioned.

In the case of the patch sent for RT 3883, the original implementation of the comparison function always failed if the client IP address was not set (given that RT 3883 does not require the IP address to be set before adding to the session database, this made sense - a NULL address should never match any other address, even a NULL address). Thus we would end up in a situation where no match was found in the lhash, but still deleting the structure from the list, causing an inconsistency. The compare function was repaired to always match itself, preventing this occurrence.

The patch makes the code in timeout_doall_arg() match the remove_session_lock() function, which does an lh_SESSION_retrieve() followed by lh_SSL_SESSION_delete() and SSL_SESSION_list_remove() if the retrieve() is successful.

Fundamentally, this patch is to keep the SSL_SESSION database in a consistent state, regardless of the behavior of the compare and hash functions. I consider that a “good” thing.

One could replace most of timeout_doall_arg() with remove_session_lock(lck=0) and have the same effect - but that won’t necessarily work with RT 3883’s patch since it does not set the session_id_length for client-side SSL_SESSIONs.

static void timeout_doall_arg(SSL_SESSION *s, TIMEOUT_PARAM *p)
{
    if ((p->time == 0) || (p->time > (s->time + s->timeout))) { /* timeout */
        /*
         * The reason we don't call SSL_CTX_remove_session() is to save on
         * locking overhead
         */
        (void)remove_session_lock(p->ctx, s, 0);
    }
}

--
-Todd Short
// tshort at akamai.com<mailto:tshort at akamai.com>
// “One if by land, two if by sea, three if by the Internet."

On May 31, 2015, at 4:46 PM, Salz, Rich via RT <rt at openssl.org<mailto:rt at openssl.org>> wrote:

Hmm, but does it? If you look for the comment '/* We *are* in trouble ... */'
in ssl_sess.c, you'll see that there is a similar kind of protection in place
already at the time of insert. So quite frankly and with all respect, I'm not
sure if this particular fix does anything of value any longer.

On Sun May 31 22:28:18 2015, tshort at akamai.com<mailto:tshort at akamai.com> wrote:
We (Akamai) had a bad session compare function at one point; the
compare was fixed, but also added this change to protect the LHASH.

So, yes, this can only really happen if one has a bad comparison
function.

--
-Todd Short
// tshort at akamai.com<mailto:tshort at akamai.com>
// Sent from my iPhone
// "One if by land, two if by sea, three if by the Internet."


On May 31, 2015, at 4:22 PM, Richard Levitte via RT <rt at openssl.org<mailto:rt at openssl.org>>
wrote:

I'm not sure how that can happen, as each SSL_SESSION in that lhash
will have
unique content. This is assured by the way lh_insert functions and
by
ssl_session_cmp (which gets called by getrn in lhash.c, via the
function
pointer cf).

So while your suggestion will most probably work as a band aid, I'm
thinking
you've really found a bug in ssl_session_cmp or in the lhash code
itself. Could
you verify?

On Sun May 31 21:24:04 2015, tshort at akamai.com<mailto:tshort at akamai.com> wrote:
No,

The second code sample removes a matching instance, but not
necessarily the same instance. If they are not the same instance,
then
it would need to be re-inserted in and else clause.

This is a fine distinction.

This would leave to having the list and hash not contain the same
contents: Either the number of items is different, or the two sets
of
items are different.

There's a similar example in the code, I believe, but I'd have to
search for it.

--
-Todd Short
// tshort at akamai.com<mailto:tshort at akamai.com>
// Sent from my iPhone
// "One if by land, two if by sea, three if by the Internet."


On May 31, 2015, at 12:43 PM, Richard Levitte via RT
<rt at openssl.org<mailto:rt at openssl.org>> wrote:

You solution does the following:

if (lh_SSL_SESSION_retrieve(p->cache, s) == s) {
(void)lh_SSL_SESSION_delete(p->cache, s);
...

Would you agree that the following does the same?

if (lh_SSL_SESSION_delete(p->cache, s) == s) {
...


On Sat May 30 09:48:06 2015, tshort at akamai.com<mailto:tshort at akamai.com> wrote:
Hello OpenSSL Org:

This is a change that Akamai has made to its
implementation of OpenSSL.

Version: master branch
Description:
lh_SSL_SESSION_delete() not checked

Fix an OpenSSL issue where the
return code of lh_SSL_SESSION_delete()
is not checked, causing a
stale reference in the lhash.

Github link:


https://github.com/akamai/openssl/commit/3a114c2f0e3bf241732fef7a2d339a230ca68abc
And attachment.

Thank you.
--
-Todd Short
// tshort at akamai.com
// “One if by land, two if by sea, three if by the Internet.”


--
Richard Levitte
levitte at openssl.org


--
Richard Levitte
levitte at openssl.org




--
Richard Levitte
levitte at openssl.org