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

[Richard Levitte via RT]

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


--
Richard Levitte
levitte at openssl.org