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

[Short, Todd]

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