Hi!
I know it has been a long time, but I have just continued to analyze
your submission.
I have not yet applied your patch. With respect to the SSL_SESSION_free()
problem, it would only cure the symptoms of incorrect SSL_SESSION_free()
use. It is not just the session list inside the SSL_CTX object; if a session
is used by an SSL object we would also find a dangling pointer that we
could not catch.
The point should not be to cover for incorrect use of SSL_SESSION_free()
and "magically" remove the session from the cache list, but to catch
this as an error... Unfortunately SSL_SESSION_free() does not return
diagnostic information (until now), so no application written with today's
API would catch the error message...
By now, I have updated the manual pages to reflect this problem and wait
for more input with respect to this problem.
Best regards,
Lutz
PS. Original message included in full length, as quite some time passed
since the thread came up. Please also note, that I already answered to
issue #3 in a seperate post several weeks ago.
On Wed, Aug 22, 2001 at 04:13:17PM -0700, Chris D. Peterson wrote:
> We've been having some problems with our old OpenSSL implementation.
> It turns out that the bug we were experiencing has, in fact, been
> fixed in the latest version of OpenSSL. In tracking down this problem
> we noticed several issues that still exist in the latest version of
> ssl/ssl_sess.c that I'd like to bring to your attention. Making
> changes to address these problems should improve the maintainablity
> and robustness of the OpenSSL code.
>
> 1) The implementation in SSL_SESSION_list_{add,remove}() seems dangerous.
>
> 2) Calling SSL_SESSION_free() on a SSL_SESSION that has been placed
> in the SESSION_list with SSL_SESSION_list_add() could leave dangling
> pointers.
>
> 3) The hash method used in SSL_CTX_add_session() is passed a SSL_SESSION
> object, but is expecting to hash on a character string.
>
> The rest of this message is a discussion of this details of these issues.
>
> - Issue # 1-------------------
>
> It appears that the SSL_SESSION_list is a doubly linked list with a
> head and tail kept inside the SSL_CTX object. Each SSL_SESSION object
> contains a next and prev pointer. Assuming that you add A,B,C, and D
> to the session list with SSL_SESSION_list() you should end up with the
> following:
>
> D prev->&D, next->C
> C prev->D, next->B
> B prev->C, next->A
> A prev->B, next->&A
> head->D
> tail->A
>
> Notice that the ends of the list contain a SSL_SESSION ** that have
> been cast back to a SSL_SESSION *. This seems exceeding dangerous
> because if someone along the line gets confused you could deref this
> SSL_SESSION ** as if it were an SSL_SESSION *. A more common
> programming technique would be to mark the ends of the list with NULL.
> So that if you try to deref something bad the library will crash
> immediately rather than writing into a bad memory address. Changes to
> this effect are fairly simple and I have included them, as well as a
> list integrity checker, at the end of this message.
>
> - Issue #2 -------------------
>
> Because the SSL_SESSION_list is linked through the SSL_SESSION *
> objects, it is absolutely critical that no SSL_SESSION object that is
> on the SSL_SESSION list have SSL_SESSION_free() called on it without
> first removing it from the SSL_SESSION_list. I believe that code
> should be added to make certain that this can not happen. The current
> code is excessivly fragile in this respect.
>
> The simplest solution would be an assert in SSL_SESSION_free() that checked
> that ss->prev and ss->next were NULL.
>
> A more comprehensive solution would be to check the next and prev
> pointers and if they were not NULL call SSL_SESSION_list_remove() on
> the session. Unfortunately this call requires the ctx pointer, so a
> change to the api for SSL_SESSION_free() is required. Care should
> also be taken to remove the session from the ctx->sessions lh_hash on
> free unless the SSL_SESSION_free() is being done in response to an
> lh_insert().
>
> - Issue #3 -------------------
>
> The lhash routines (e.g. lh_insert()) expect to be passed a NULL
> terminated string of characters to hash. But the code in ssl_sess.c
> is passing an SSL_SESSION *. Having a block of code that thinks it is
> reading a character string marching through a binary structure looking
> for a NULL terminator and then hashing on the contents sure seems like
> asking for trouble. Perhaps the session ID should be converted to a
> string representation and used as the hash key? Or a new entry point
> into these methods should be used that hashes a fixed number of bytes
> and uses the binary reprenentation of the session ID to has the
> values. In any case the current method is logicall just wrong athough
> it will generally work, because a 0 in the first byte of the session
> id is a rather rare occurance.
>
> I hope this information is useful, and you should feel free to
> contact me if you have any questions about the issues I have raised
> here.
>
> -- Chris Peterson
> [EMAIL PROTECTED]
>
> ------------------------------------------------------------
>
> Index: ssl_sess.c
> ===================================================================
> RCS file: /usr/aventail/prodroot/sdk/openssl/ssl/ssl_sess.c,v
> retrieving revision 1.3
> diff -c -r1.3 ssl_sess.c
> ***************
> *** 566,627 ****
> return(0);
> }
>
> /* locked by SSL_CTX in the calling function */
> static void SSL_SESSION_list_remove(ctx,s)
> SSL_CTX *ctx;
> SSL_SESSION *s;
> ! {
> ! if ((s->next == NULL) || (s->prev == NULL)) return;
>
> ! if (s->next == (SSL_SESSION *)&(ctx->session_cache_tail))
> ! { /* last element in list */
> ! if (s->prev == (SSL_SESSION *)&(ctx->session_cache_head))
> ! { /* only one element in list */
> ! ctx->session_cache_head=NULL;
> ! ctx->session_cache_tail=NULL;
> ! }
> ! else
> ! {
> ! ctx->session_cache_tail=s->prev;
> ! s->prev->next=(SSL_SESSION *)&(ctx->session_cache_tail);
> ! }
> ! }
> ! else
> ! {
> ! if (s->prev == (SSL_SESSION *)&(ctx->session_cache_head))
> ! { /* first element in list */
> ! ctx->session_cache_head=s->next;
> ! s->next->prev=(SSL_SESSION *)&(ctx->session_cache_head);
> ! }
> ! else
> ! { /* middle of list */
> ! s->next->prev=s->prev;
> ! s->prev->next=s->next;
> ! }
> ! }
> ! s->prev=s->next=NULL;
> }
>
> static void SSL_SESSION_list_add(ctx,s)
> SSL_CTX *ctx;
> SSL_SESSION *s;
> ! {
> ! if ((s->next != NULL) && (s->prev != NULL))
> SSL_SESSION_list_remove(ctx,s);
>
> ! if (ctx->session_cache_head == NULL)
> ! {
> ! ctx->session_cache_head=s;
> ! ctx->session_cache_tail=s;
> ! s->prev=(SSL_SESSION *)&(ctx->session_cache_head);
> ! s->next=(SSL_SESSION *)&(ctx->session_cache_tail);
> ! }
> ! else
> ! {
> ! s->next=ctx->session_cache_head;
> ! s->next->prev=s;
> ! s->prev=(SSL_SESSION *)&(ctx->session_cache_head);
> ! ctx->session_cache_head=s;
> ! }
> ! }
>
> --- 589,815 ----
> return(0);
> }
>
> +
> + /*
> + * A simple doubly linked list.
> + *
> + * The head and tail of the list are stored in the SSL_CTX.
> + *
> + * If you call SSL_SESSION_list_add on items A,B,C,D the
> + * list will look like this:
> + *
> + * D: D->next=C D->prev=NULL
> + * C: C->next=B C->prev=D
> + * B: B->next=A B->prev=C
> + * A: A->next=NULL A->prev=B
> + *
> + * ctx->session_cache_head = D
> + * ctx->session_cache_tail = A
> + */
> +
> + #define LIST_CHECK
> +
> + #ifdef LIST_CHECK
> + static void
> + SSL_SESSION_list_check(ctx, label)
> + SSL_CTX *ctx;
> + char *label;
> + {
> + int is_valid = 1; /* is the list valid? starts as TRUE */
> + int i, count = 0;
> + SSL_SESSION *s, **list;
> +
> + for (s = ctx->session_cache_head; s != NULL; s = s->next, count++) {}
> +
> + list = (SSL_SESSION **) Malloc(count * sizeof(SSL_SESSION *));
> +
> + for (i = 0, s = ctx->session_cache_head; s != NULL; s = s->next, i++) {
> + if (i >= count) {
> + fprintf(stderr, "Too many elements in next list!\n");
> + is_valid = 0; /* FALSE */
> + break;
> + }
> +
> + list[i] = s;
> +
> + if ((s->next == NULL) && (ctx->session_cache_tail != s)) {
> + is_valid = 0; /* FALSE */
> + fprintf(stderr, "%s %#X, but the tail is %#X\n",
> + "Last Element in next chain is",
> + (u_int) s, (u_int) ctx->session_cache_tail);
> + }
> + }
> +
> + for (i = count - 1,
> + s = ctx->session_cache_tail; s != NULL; s = s->prev, i--)
> + {
> + if (i < 0) {
> + fprintf(stderr, "Too many elements in prev list!\n");
> + is_valid = 0; /* FALSE */
> + break;
> + }
> +
> + if (list[i] != s) {
> + fprintf(stderr,
> + "The %d Element in the next chain is %#X, %s
>%#X\n",
> + i, (u_int) list[i],
> + "but the equilivent element in the prev chain
>is",
> + (u_int) s);
> + is_valid = 0; /* FALSE */
> + }
> + if ((s->prev == NULL) && (ctx->session_cache_head != s)) {
> + fprintf(stderr, "%s %#X, but the head is %#X\n",
> + "Last Element in prev chain is",
> + (u_int) s, (u_int) ctx->session_cache_head);
> + is_valid = 0; /* FALSE */
> + }
> + }
> +
> + if (ctx->session_cache_tail != NULL &&
> + ctx->session_cache_tail->next != NULL)
> + {
> + is_valid = 0; /* FALSE */
> + fprintf(stderr, "Tail's next pointer should be NULL, but is %#X\n",
> + (u_int) ctx->session_cache_tail->next);
> + }
> +
> + if (ctx->session_cache_head != NULL &&
> + ctx->session_cache_head->prev != NULL)
> + {
> + is_valid = 0; /* FALSE */
> + fprintf(stderr, "Head's prev pointer should be NULL, but is %#X\n",
> + (u_int) ctx->session_cache_head->prev);
> + }
> +
> + if (is_valid) {
> + fprintf(stderr, "%s, List Passed Internal Verification\n", label);
> + } else {
> + fprintf(stderr, "%s, List Failed Internal Verification\n", label);
> + }
> +
> + if (!is_valid) {
> + fprintf(stderr, "Head: %#X, Tail: %#X, %d elements\n",
> + (u_int) ctx->session_cache_head,
> + (u_int) ctx->session_cache_tail, count);
> +
> + for (s = ctx->session_cache_head; s != NULL; s = s->next) {
> + fprintf(stderr, "Elem %#X - prev %#X, next %#X\n",
> + (u_int) s, (u_int) s->prev, (u_int) s->next);
> + }
> +
> + for (s = ctx->session_cache_tail; s != NULL; s = s->prev) {
> + fprintf(stderr,"Elem %#X - prev %#X, next %#X\n",
> + (u_int) s, (u_int) s->prev, (u_int) s->next);
> + }
> + }
> +
> + Free(list);
> + }
> + #endif /* LIST_CHECK */
> +
> /* locked by SSL_CTX in the calling function */
> static void SSL_SESSION_list_remove(ctx,s)
> SSL_CTX *ctx;
> SSL_SESSION *s;
> ! {
> ! #ifdef LIST_CHECK
> ! fprintf(stderr, "Removing session. %#X\n", (u_int) s);
> ! SSL_SESSION_list_check(ctx, "Before remove");
> ! #endif
>
> ! /*
> ! * If I have a next element set the next element's prev to point to
> ! * point to my prev element. (Takes me out of the next chain).
> ! *
> ! * The NULL check handles the case where I am the tail.
> ! */
> !
> ! if (s->next != NULL) {
> ! s->next->prev = s->prev;
> ! }
> !
> ! /*
> ! * If I have a prev element set the prev element's next to point to
> ! * point to my next element. (Takes me out of the prev chain).
> ! *
> ! * The NULL check handles the case where I am the head.
> ! */
> !
> ! if (s->prev != NULL) {
> ! s->prev->next = s->next;
> ! }
> !
> ! /*
> ! * If I am the tail then set the tail to my prev element.
> ! * This also handles the case where I am the last element in the
> ! * list, setting the tail to NULL.
> ! */
> !
> ! if (ctx->session_cache_tail == s) { /* last element in list, reset tail */
> ! ctx->session_cache_tail = s->prev;
> }
>
> + /*
> + * If I am the head then set the head to my next element.
> + * This also handles the case where I am the last element in the
> + * list, setting the head to NULL.
> + */
> +
> + if (ctx->session_cache_head == s) { /* first element in list, reset head */
> + ctx->session_cache_head = s->next;
> + }
> +
> + /*
> + * In all cases cleare out my next and prev elements as I have
> + * now been removed from the list.
> + */
> +
> + s->prev = s->next = NULL;
> +
> + #ifdef LIST_CHECK
> + SSL_SESSION_list_check(ctx, "After remove");
> + #endif
> + }
> +
> static void SSL_SESSION_list_add(ctx,s)
> SSL_CTX *ctx;
> SSL_SESSION *s;
> ! {
> ! #ifdef LIST_CHECK
> ! fprintf(stderr, "Adding session. %#X\n", (u_int) s);
> ! SSL_SESSION_list_check(ctx, "Before add");
> ! #endif
> !
> ! if ((s->next != NULL) || (s->prev != NULL))
> SSL_SESSION_list_remove(ctx,s);
>
> ! /*
> ! * If no tail (this is first element, then make me the tail).
> ! */
> !
> ! if (ctx->session_cache_tail == NULL) {
> ! ctx->session_cache_tail = s;
> ! }
> !
> ! /*
> ! * If there is a head already then
> ! *
> ! * Set the old head's prev pointer to me.
> ! * Set my next pointer to the old head.
> ! */
> !
> ! if (ctx->session_cache_head != NULL) {
> ! (ctx->session_cache_head)->prev = s;
> ! s->next = ctx->session_cache_head;
> ! }
> !
> ! /*
> ! * I am always the new head.
> ! */
> !
> ! ctx->session_cache_head = s;
>
> + #ifdef LIST_CHECK
> + SSL_SESSION_list_check(ctx, "After add");
> + #endif
> + }
>
>
> ______________________________________________________________________
> OpenSSL Project http://www.openssl.org
> User Support Mailing List [EMAIL PROTECTED]
> Automated List Manager [EMAIL PROTECTED]
--
Lutz Jaenicke [EMAIL PROTECTED]
BTU Cottbus http://www.aet.TU-Cottbus.DE/personen/jaenicke/
Lehrstuhl Allgemeine Elektrotechnik Tel. +49 355 69-4129
Universitaetsplatz 3-4, D-03044 Cottbus Fax. +49 355 69-4153
______________________________________________________________________
OpenSSL Project http://www.openssl.org
User Support Mailing List [EMAIL PROTECTED]
Automated List Manager [EMAIL PROTECTED]
