[openssl-dev] [openssl.org #3882] [BUGFIX] lh_SSL_SESSION_delete() not checked
It appears to be defensive programming against a buggy compare routine. So closing this as requested. -- Ticket here: http://rt.openssl.org/Ticket/Display.html?id=3882 Please log in as guest with password guest if prompted -- openssl-dev mailing list To unsubscribe: https://mta.openssl.org/mailman/listinfo/openssl-dev
Re: [openssl-dev] [openssl.org #3882] [BUGFIX] lh_SSL_SESSION_delete() not checked
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
// tsh...@akamai.commailto:tsh...@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
r...@openssl.orgmailto:r...@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, tsh...@akamai.commailto:tsh...@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
// tsh...@akamai.commailto:tsh...@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
r...@openssl.orgmailto:r...@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, tsh...@akamai.commailto:tsh...@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
// tsh...@akamai.commailto:tsh...@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
r...@openssl.orgmailto:r...@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, tsh...@akamai.commailto:tsh...@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
// tsh...@akamai.com
// “One if by land, two if by sea, three if by the Internet.”
--
Richard Levitte
levi...@openssl.org
--
Richard Levitte
levi...@openssl.org
--
Richard Levitte
levi...@openssl.org
___
openssl-dev mailing list
To unsubscribe:
Re: [openssl-dev] [openssl.org #3882] [BUGFIX] lh_SSL_SESSION_delete() not checked
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
// tsh...@akamai.commailto:tsh...@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
r...@openssl.orgmailto:r...@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, tsh...@akamai.commailto:tsh...@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
// tsh...@akamai.commailto:tsh...@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
r...@openssl.orgmailto:r...@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, tsh...@akamai.commailto:tsh...@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
// tsh...@akamai.commailto:tsh...@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
r...@openssl.orgmailto:r...@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, tsh...@akamai.commailto:tsh...@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
// tsh...@akamai.com
// “One if by land, two if by sea, three if by the Internet.”
--
Richard Levitte
levi...@openssl.org
--
Richard Levitte
levi...@openssl.org
--
Richard Levitte
levi...@openssl.org
___
openssl-dev mailing list
To unsubscribe:
Re: [openssl-dev] [openssl.org #3882] [BUGFIX] lh_SSL_SESSION_delete() not checked
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
// tsh...@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 r...@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, tsh...@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
// tsh...@akamai.com
// “One if by land, two if by sea, three if by the Internet.”
--
Richard Levitte
levi...@openssl.org
___
openssl-dev mailing list
To unsubscribe: https://mta.openssl.org/mailman/listinfo/openssl-dev
[openssl-dev] [openssl.org #3882] [BUGFIX] lh_SSL_SESSION_delete() not checked
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, tsh...@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
// tsh...@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 r...@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, tsh...@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
// tsh...@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
r...@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, tsh...@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
// tsh...@akamai.com
// “One if by land, two if by sea, three if by the Internet.”
--
Richard Levitte
levi...@openssl.org
--
Richard Levitte
levi...@openssl.org
--
Richard Levitte
levi...@openssl.org
___
openssl-dev mailing list
To unsubscribe: https://mta.openssl.org/mailman/listinfo/openssl-dev
[openssl-dev] [openssl.org #3882] [BUGFIX] lh_SSL_SESSION_delete() not checked
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, tsh...@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
// tsh...@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
r...@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, tsh...@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
// tsh...@akamai.com
// “One if by land, two if by sea, three if by the Internet.”
--
Richard Levitte
levi...@openssl.org
--
Richard Levitte
levi...@openssl.org
___
openssl-dev mailing list
To unsubscribe: https://mta.openssl.org/mailman/listinfo/openssl-dev
[openssl-dev] [openssl.org #3882] [BUGFIX] lh_SSL_SESSION_delete() not checked
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, tsh...@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
// tsh...@akamai.com
// “One if by land, two if by sea, three if by the Internet.”
--
Richard Levitte
levi...@openssl.org
___
openssl-dev mailing list
To unsubscribe: https://mta.openssl.org/mailman/listinfo/openssl-dev
Re: [openssl-dev] [openssl.org #3882] [BUGFIX] lh_SSL_SESSION_delete() not checked
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
// tsh...@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 r...@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, tsh...@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
// tsh...@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
r...@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, tsh...@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
// tsh...@akamai.com
// “One if by land, two if by sea, three if by the Internet.”
--
Richard Levitte
levi...@openssl.org
--
Richard Levitte
levi...@openssl.org
___
openssl-dev mailing list
To unsubscribe: https://mta.openssl.org/mailman/listinfo/openssl-dev
Re: [openssl-dev] [openssl.org #3882] [BUGFIX] lh_SSL_SESSION_delete() not checked
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
// tsh...@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 r...@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, tsh...@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
// tsh...@akamai.com
// “One if by land, two if by sea, three if by the Internet.”
--
Richard Levitte
levi...@openssl.org
___
openssl-dev mailing list
To unsubscribe: https://mta.openssl.org/mailman/listinfo/openssl-dev
[openssl-dev] [openssl.org #3882] [BUGFIX] lh_SSL_SESSION_delete() not checked
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 // tsh...@akamai.com // “One if by land, two if by sea, three if by the Internet.” 0001-lh_SSL_SESSION_delete-not-checked.patch Description: Binary data ___ openssl-dev mailing list To unsubscribe: https://mta.openssl.org/mailman/listinfo/openssl-dev
